service.patch leads to 404 if query contains the patched field #345
Comments
toovy
changed the title
|
@toovy I am also a problem like you, and my error after patch 'NotFound: No record found for id '5d8a001bae64e4e9a8c74aa8' |
|
If someone can provide a small breaking example that can be included in a test it would be much easier to start a fix. |
|
@daffl I've provided an example at https://github.com/triggercode/feathers-mongoose-patch-issue. The example has two branches, master and feathers-mongoose-bug. In both the services tests are used to reproduce the bug. To setup the master branch a The setup of the feathers-mongoose-bug is a bit more complex as we need a mongodb. So I've included everything needed, assuming that docker is installed: I noticed that there might be an timinig issue as well. Looking at my await statements everything seems to be correct. Mostly all tests fail, randomly the first one passes and returns a record. Thanks for looking into it! |
|
Greetings... Any update on this? |
|
Related issue: #321 |
|
This seems to happen with feathers-sequelize as well |
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 |
Hi,
this is a cross-post from feathers-hooks-commons, as I'm not sure, which component is responsible for this issue. I'l summarise the relevant parts here, for more details please also see feathersjs-ecosystem/feathers-hooks-common#532.
Summary
In the code at
https://github.com/feathersjs-ecosystem/feathers-mongoose/blob/master/lib/service.js#L278
mongoose is used to patch a record, using the the filtered
params.query, and in L286 this query is more or less used to find the same record again after the patch. In the case of softDelete2 the query contains{deletedAt: -1}that should be probably used during the patch call as query parameter. In L278 the query still contains{deletedAt: -1}, but as data has set deletedAt to another value in between the record is not found, leading to a 404.This is probably true for all patch calls that use a value A as query and modify this value A during the patch call.
Any thoughts?
The text was updated successfully, but these errors were encountered: