Patching with query does not return expected results in query property was patched

[DaddyWarbucks]

When patching with a query where the updated data also contains the property of the query, the method does not return any results.

For example

// Notice how we are both querying by the completed as well as updating the completed props
app.service('posts').patch(
  null,
  { completed: true },
  { query: { completed: false } }
)

This line is where the problem is:

feathers-sequelize/lib/index.js

Line 238 in 8b46599

query: Object.assign({ [this.id]: { $in: idList } }, params.query)

The query that is built to re-query the results is creating an impossible state in this instance

// This re-query is attempting to query by id's that changed, which is accurate, but it
// is also trying to query by the OLD completed status which has now been updated
{ id: { $in: [1] }, completed:  false }

I am not quite sure the best way to handle this. We could make the new query be just the { id: { $in: idList }. That is accurate and would work. We don't really need the rest of the params.query because params.query has already produced the idList for us. But we lose access the the $select and potentially some other things?

// Skip the rest of the query and only re-query by the idList
const findParams = Object.assign({}, params, { query: { [this.id]: { $in: idList } } });

We could also add the idList in an $or. Maybe something like

const findParams = Object.assign({}, params, Object.assign({}, {
  query: Object.assign({ $or: [{[this.id]: { $in: idList }}] }, params.query)
}));

Probably the best solution is to keep just the idList and $select. The idList is the only "query" that we need. Although $select is in the query, its intent is not to "query" data but rather to "filter" it. And $select is the only operator in which this is the case (at least in the common feathers query standard). So I think it is safe to just keep it.

const findParams = Object.assign(
    {},
    params,
    {
      query: {
        [this.id]: { $in: idList },
        $select: params.query.$select
      }
    }
  );

[DaddyWarbucks]

I started implementing this in a PR and ran into a few unexpected snags. The tests are failing for the common adapter tests. Which I can't seem to find. The code references @feathersjs/adapter-tests but I do not see that in the core Feathers monorepo. Am I missing something there?

My basic implementation was this

const findQuery = { [this.id]: { $in: idList } };

if (params.query && params.query.$select) {
    findQuery.$select = [...params.query.$select];
}

if (params.query && params.query.$sort) {
    findQuery.$sort = Object.assign({}, params.query.$sort);
}

const findParams = Object.assign({}, params, { query: findQuery });

This solution skips merging the rest of the params.query and only keeps the idList and those queries that "shape" the result rather than actually "querying". This makes logical sense in my head, but fails the adapter tests mentioned above.

As clean as I think it is to just keep that minimal query, I am sure there are cases it will fail to cover.

Another solution may be to modify the findParams.query with the data as well. Something like

// This is the code that is currently implemented for findParams
const findParams = Object.assign({}, params, Object.assign({}, {
    query: Object.assign({ [this.id]: { $in: idList } }, params.query)
}));

// Loop over the data and update the findParams.query to what we expect the record
// to have been updated to, rather than trying to query by what the record was before update
Object.keys(data).forEach(key => {
    if (params && params.query[key]) {
        findParams.query[key] = data[key];
    } 
});

[daffl]

Ha, good timing I actually just moved those to /~https://github.com/feathersjs/databases ... yesterday 😺 What would be great to start with would be a PR to with a failing test for the cases. I think you are totally right and

feathers-sequelize/lib/index.js

Line 238 in 8b46599

query: Object.assign({ [this.id]: { $in: idList } }, params.query)

should actually be

query: { [this.id]: { $in: idList } }

Potentially merged with the query filtered params ($select, $limit etc.).

[daffl]

Closed via #350