patch is incorrectly applying the entire query when re-fetching #321
Labels
Comments
2 tasks
arfanliaqat
added a commit
to arfanliaqat/feathers-mongoose
that referenced
this issue
Dec 21, 2019
Fix 404, when patch call modifies values in `params.query`.
### Summary
I've been patiently waiting for soft-delete fix, and this is desperate attempt to make soft-delete work without throwing 404. Any serious app won't exist without soft delete feature. Unfortunately i've not been able to implement soft-delete with, softDelete or softDelete2. It always throw 404 (atleast for me in my feathers mongoose setup). (This has already been documented why)
The fix is rather one liner after several hours of staring (and a bit thinking), hence it require more serious review by someone having more experience with feathersjs internals.
The following snippet of code is very beginning of patch method.
```
const { query } = this.filterQuery(params);
const mapIds = data => data.map(current => current[this.id]);
// By default we will just query for the one id. For multi patch
// we create a list of the ids of all items that will be changed
// to re-query them after the update
const ids = id === null ? this._find(Object.assign({}, params, {
paginate: false
})).then(mapIds) : Promise.resolve([id]);
```
After execution of above snippet we will have _ids produced by query. **Notice that this result is produced using all params. Which obviously includes `params.query`.** (see below)
Further down we see following snippet
```
return ids.then(idList => {
const { query: { $populate } = {} } = params;
// Create a new query that re-queries all ids that
// were originally changed
const updatedQuery = (idList.length && id === null) ? { [this.id]: { $in: idList } } : params.query;
const findParams = Object.assign({}, params, {
paginate: false,
query: $populate ? Object.assign(updatedQuery, { $populate }) : updatedQuery
});
...
```
Most important line is this.
```
const updatedQuery = (idList.length && id === null) ? { [this.id]: { $in: idList } } : params.query;
```
Changing it to following does resolve the 404 issues
```
const updatedQuery = { [this.id]: { $in: idList } };
```
## How?
`params.query` give obselete values to query which causes 404 because those old values are modified by patch call. If we think for second params.query should not be here at all since we have `idList`.
## Why remove params.query?
### Argument 1:
Because it has faithfully served its purposes in first snippet already. We get `idList` after the query is ran with `params` (see first snippet above). Here it is completely redundent. `idList.length` is also removed, but an empty idList won't find any match in mongodb
```
{[this.id]: { $in: [] }} <-- this wont return any results
```
### Argument 2:
A few lines above the same `params.query` returned an empty idList, it will probably do the same during updateMany call down the road and nothing will be updated. Hence we don't need to forward it to `_getOrFind` because its no use.
### What about additional parameters for patch call?
We can make following change in first snippet as follows
```
const mapIds = data => Array.isArray(data) ? data.map(current => current[this.id]) : data[this.id];
const ids = id === null ? this._getOrfind(id, Object.assign({}, params, {
paginate: false
})).then(mapIds);
```
This will ensure that ids are returned by query with addtional parameters.
- [x] Are there any open issues that are related to this?
- feathersjs-ecosystem#345
- feathersjs-ecosystem#321
- [x] Is this PR dependent on PRs in other repos?
- Not dependent though i noticed even new (yet unreleased) soft-delete hook won't work without this fix
|
this is fixed in #362 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Steps to reproduce
This issue happens when you issue a
.patchrequest where the payload modifies fields that are in the query. This is a regression from 6.x.In 6.x, if you did
(note: this fails in both single and multi cases, see explanation below)
Expected behavior
you will get back the modified row(s) if the query filter (and ID) matched any records.
Actual behavior
In 7.x, this no longer works.
This is because in service.js, this changed:
to
which incorrectly applies the original query filters to the data that's being re-fetched. This is wrong - patch should return me changed rows, and should not be applying my original filters (which will fail in this case, I just changed the data that I was searching for!)
BUT
This does break uses of
$populatewhich might also be part of the query. We, of course, use both in different scenarios. Ideally, the $populate portion would be pulled out and reapplied to the ensuing _getOrFind, but not the actual filters.System configuration
Tell us about the applicable parts of your setup.
Mongoose 7.3.2
Node 10.15.3
The text was updated successfully, but these errors were encountered: