🐛 fix 404 on patch call

[arfanliaqat]

Fix 404, when patch call modifies values in params.query.

Summary

I've been patiently waiting for soft-delete fix. Any serious app won't exist without soft delete feature. Unfortunately i've not been able to implement soft-delete with, softDelete or softDelete2 hooks. They always throw 404 (atleast for me in my feathers mongoose setup). (The causes have already been documented. see related issues below). This fix attempts to make soft-delete work without throwing 404. Would appreciate a review by someone having more experience with feathersjs internals.

The fix is contained to _patch method only.

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 request after updateMany because its no use.

What about additional parameters for patch call?

We can make following change in first snippet

const mapIds = data => Array.isArray(data) ? data.map(current => current[this.id]) : data[this.id];

const ids = this._getOrfind(id, Object.assign({}, params, {
      paginate: false
    })).then(mapIds);

This will ensure that ids are returned by query with addtional parameters.

• Are there any open issues that are related to this?
  • service.patch leads to 404 if query contains the patched field #345
  • patch is incorrectly applying the entire query when re-fetching #321
• 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

[arfanliaqat]

🤦‍♂️ unit tests!... its late night here... will try to send an update tomorrow

[marshallswain]

So, I'm having trouble releasing this because all of the transaction manager tests are failing for me, locally. If you want to pull the latest master and troubleshoot, that would probably help move things along quicker. As soon as they pass, I can run the publish script. Sorry for the delay.

[marshallswain]

Ah. This is failing because my local mongo setup isn't a replica set. It's working in Travis, so I'll go ahead and publish.

[marshallswain]

Released as [email protected]