Support params.query in update()

[TimNZ]

I have permissions hooks that add fields to params.query for all service operations to limit records that can be updated/retrieved to a filtered set by 'owner_id'.

update() is the only method that doesn't support params.query to augment the final query.

I understand why, since it is only supposed to act on a specific record by id, which should always be unique.

If you don't accept the PR I understand.

In the meantime I've created a custom Service class extended from the sequelize Service class and overridden update()

Also want to avoid doing a hook that has to query for the record first to do the check before an update, to avoid a double query.

I may implement feathers-cache at some point, but don't know how robust that is yet in scaling and multiple server instances.

[daffl]

Thank you for the pull request! Earlier versions of Feathers database adapters allowed to do this and it cause many issues with people replacing their entire collections when all they wanted to do was update one field.

What is a good use case where multi update does something that the already support multi patch doesn't (adding a test might also help to clarify 😄 )?

[TimNZ]

Yeah, I did the PR and then thought "hmm, I guess they'll want a test" 😄

Adding params.query support is not for multi update like Patch, as the id field is still enforced in Update.

The mods are to make it consistent with all the other methods, and to make it easier for hooks to enforce some additional filter on what can be updated.
i.e. a permissions framework that enforces a 'owner_id = req.user.id'

Avoids the need for an additional query by a before hook to do the check.

As I said, an edge case, and understand if you choose not to merge.
Not a major real world issue.

I've got my workaround and happy to continue using that.

[daffl]

Ah ok, if the constraint is still enforce that makes sense. A simple test would still be nice though.

[TimNZ]

The current Update doesn't enforce the id by testing for truthy, it is passed as is to _get(), which doesn't enforce it either - should I add a truthy check in Update () and throw an error if not provided?

[TimNZ]

@daffl Bump, re last comment to get this tidied away.

"The current Update doesn't enforce the id by testing for truthy, it is passed as is to _get(), which doesn't enforce it either -
should I add a truthy check in Update () and throw an error if not provided?"

[daffl]

Is that necessary and/or related to this PR? Like I mentioned before, a test would still be nice.

[TimNZ]

Never mind, reread the Service docs and see update() is meant to support id = null and support params.query, it was just not implemented in this package.

https://docs.feathersjs.com/api/services.html#updateid-data-params

I'll add tests

[schettino]

Is this going to be merged? I have faced the same issue when setting a permission criteria inside a hook.

[TimNZ]

Forgot.

Added tests (which maybe need a more experienced person to tidy up) and fixed a bug (tests are useful).

[daffl]

Awesome, thanks for adding those tests. This looks good to me so I'll go ahead and publish as a minor release.