From d7956bdc6ce6f4162de8a00339f54b440e253d70 Mon Sep 17 00:00:00 2001 From: Arfan Liaqat Date: Sun, 22 Dec 2019 01:43:01 +0500 Subject: [PATCH 1/3] :bug: fix 404 on patch call (#1) 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? - #345 - #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 --- lib/service.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/service.js b/lib/service.js index cc29dd50..b70393b0 100755 --- a/lib/service.js +++ b/lib/service.js @@ -224,14 +224,14 @@ class Service extends AdapterService { _patch (id, data, params = {}) { const { query } = this.filterQuery(params); - const mapIds = data => data.map(current => current[this.id]); + const mapIds = data => Array.isArray(data) ? data.map(current => current[this.id]) : [data[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, { + const ids = this._getOrFind(id, Object.assign({}, params, { paginate: false - })).then(mapIds) : Promise.resolve([id]); + })).then(mapIds); // Handle case where data might be a mongoose model if (typeof data.toObject === 'function') { @@ -268,7 +268,7 @@ class Service extends AdapterService { 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 updatedQuery = { [this.id]: { $in: idList } }; const findParams = Object.assign({}, params, { paginate: false, query: $populate ? Object.assign(updatedQuery, { $populate }) : updatedQuery From 0dad5e7d20427c1d0a55e4ca7e02c689eacb61bc Mon Sep 17 00:00:00 2001 From: Arfan Liaqat Date: Sun, 22 Dec 2019 03:03:00 +0500 Subject: [PATCH 2/3] fix failing upsert unit test --- lib/service.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/service.js b/lib/service.js index b70393b0..94c0251b 100755 --- a/lib/service.js +++ b/lib/service.js @@ -286,6 +286,9 @@ class Service extends AdapterService { if (options.writeResult) { return writeResult; } + if ('upserted' in writeResult) { + return this._getOrFind(id, Object.assign({}, params, { query: { [this.id]: { $in: writeResult.upserted.map(doc => doc._id) } } }, { paginate: false })); + } return this._getOrFind(id, findParams); }); }).then(select(params, this.id)).catch(errorHandler); From fe0bc6b3d3117231978198154c5192aaa11a7488 Mon Sep 17 00:00:00 2001 From: Arfan Liaqat Date: Mon, 23 Dec 2019 22:06:14 +0500 Subject: [PATCH 3/3] :pencil2: fix minor typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b6c27d81..992f6287 100644 --- a/README.md +++ b/README.md @@ -298,7 +298,7 @@ For more information on MongoDB's collation feature, visit the [collation refere This adapter includes support to enable database transaction to rollback the persisted records for any error occured for a api call. This requires [Mongo-DB v4.x](https://docs.mongodb.com/manual/) installed and [replica-set](https://linode.com/docs/databases/mongodb/create-a-mongodb-replica-set/#start-replication-and-add-members) enabled. -Start working with transactin enabled by adding the following lines in `app.hooks.js` or `.hooks.js`. +Start working with transaction enabled by adding the following lines in `app.hooks.js` or `.hooks.js`. ```js const TransactionManager = require('feathers-mongoose').TransactionManager;