Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PUT/PATCH don't support mongoose update operations #224

Open
smirea opened this issue Jan 17, 2016 · 9 comments
Open

PUT/PATCH don't support mongoose update operations #224

smirea opened this issue Jan 17, 2016 · 9 comments

Comments

@smirea
Copy link
Contributor

smirea commented Jan 17, 2016

This should work:

curl -XPUT /api/v1/User -d {"$addToSet": {"friends": "BoB"}}

but because we use document.set instead of document.update we loose the ability to use any of the mongodb update operators

currently the way that request is handled it results in doing the following operation:
doc.save({'$addToSet.friends': 'BoB'})

instead of the desired:
doc.update({'$addToSet': {'friends': 'BoB}})

What do you think of this proposed solution? not tested yet, but seems to work on my use-cases

@Zertz
Copy link
Collaborator

Zertz commented Jan 18, 2016

TIL: I honestly was not aware of this method!

Could you post a PR to run our tests?

@smirea
Copy link
Contributor Author

smirea commented Feb 13, 2016

welp this is a late reply. I haven't had the chance to put that PR up. my patchwork broke a lot of tests since it changes part of the core functionality and I don't have the time atm to look into it. It was more a proof of concept to see how hard/easy would it be to get the functionality

So far I'm working around it by pushing to the list on the client side and sending the new list, but yeah it's not ideal

@Zertz
Copy link
Collaborator

Zertz commented Feb 21, 2016

By using update, we open the door to a bunch of useful methods, but it becomes rather complex to properly filter fields because we'd have to dig into a bunch of operators such as $addToSet, $push and so on.

While i agree that using save is not ideal, it is far simpler. That said, if someone is willing to commit time into this, you are more than welcome to! Just keep in mind that backward compatibility is important here.

@norpan
Copy link

norpan commented Mar 14, 2016

If you decide to change this, please give consideration to the different semantics of PUT and PATCH. PUT is meant to replace a whole document. PATCH on the other hand is meant for just this use case.

There is a very good reason to separate PUT from PATCH. PUT is idempotent, which means that a client can assume that applying a PUT operation twice will give the same result as applying it once. So, while $addToSet happens to be idempotent, operations like $push are not.

@wesratcliff
Copy link
Contributor

Curious if there are any thoughts on what the query/body will look like for nested array updates -- would it adhere to Mongoose operators or something like RFC6902.

@dortzur
Copy link

dortzur commented Oct 31, 2016

Any update on this?

@b-gran
Copy link

b-gran commented Jan 11, 2017

This is by no means a complete solution (doesn't handle access-protected routes or routes filtered by any other criteria), but here's what I'm using right now to solve this problem locally:

import _ from 'lodash';
import mongoose from 'mongoose';

function applyUpdateUsingRequestBody (model, getDocumentId) {
    if (!isModel(model)) {
        throw new Error(`Expected model to be a mongoose Model`);
    }

    if (!_.isFunction(getDocumentId) || getDocumentId.length !== 1) {
        throw new Error(`getDocumentId() must be a function of arity 1`);
    }

    return function (req, res, next) {
        const body = req.body;

        if (typeof body !== 'object') {
            return res.status(400).json({ message: 'The request body must be an object' });
        }

        const documentId = getDocumentId(req);

        if (_.isNil(documentId)) {
            return res.status(400).json({ message: 'No document id specified' });
        }

        return model.findOneAndUpdate({ _id: documentId }, body, { new: true }).then(result => {
            return res.status(200).json(result);
        }).catch(err => {
            return next(err);
        });
    }
}

function isModel (model) {
    return Object.getPrototypeOf(model) === mongoose.Model;
}

This function just takes the request body and passes it directly to Model#findOneAndUpdate(). It's very easy to use Document#update() instead, so we still get context filtering.

Currently, I mount this function at PUT /api/v1/model-name/:id/update, i.e.

const router = express.Router()
router.put(
  '/api/v1/model-name/:id/update',
  applyUpdateUsingRequestBody(model, req => req.params.id)
)
restify(router, model, { ... })

To incorporate this type of solution (passing request body to update()) into ERM, we would need to:

  1. Decide where to integrate it.
    • Do we change PUT /api/v1/model/:id to pass the request body to Document#update()?
    • Do we create a new endpoint (e.g. /api/v1/model/:id/update) that passes the request to Document#update()?
    • Do we add a configuration parameter (something like passRequestBodyToUpdate: Boolean) that modifies PUT or PATCH /api/v1/model/:id to use Document#update()?
  2. Apply access filtering
    • This would involve recursively searching the request body, looking for MongoDB update operators and removing the ones that try to modify properties disallowed according to req.access
  3. Find a way to handle population

What do you all think about this? There's also the option of json-patch, and there's been some progress made in #323 .

@Zertz
Copy link
Collaborator

Zertz commented Jan 14, 2017

I'm not too fond of adding deep endpoints, I think it should be left to the user to create routes that go beyond /model/:id, perhaps in the form of plugins. I know we do it already with /model/:id/shallow, but I don't think it should be in core either. Since we're mostly using plain old middleware, it should be feasible to create separate modules that fill this purpose, especially with all the decoupling work you've done in #314.

That said, I do agree that having a way to use operators such as $addToSet would definitely be useful.

@b-gran
Copy link

b-gran commented Jan 15, 2017

That's a good point about deep endpoints.

For me, the key feature in opening up all of the MongoDB update operators is actually field deletion, because it's not currently possible to remove a field from a document.

To perform PUT/PATCH ops, we're using

context.findOneAndUpdate(
  {},
  { $set: body },
  { ... }
)

If we update PUT to completely replace the document, we would at least get field deletion via PUT, but we still won't have deletion via PATCH (which will still need to use $set)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants