Cache hook returns improper values when using $select or query on methods other than find()

[6thManMovers]

Steps to reproduce

Since the update to the official database adapters that pass the context.params.query to methods other than find, this hook has two major problems.

Problem 1 - Because queries are now passed to all methods, this means the $select prop
can be used to filter the result on any method.

app.service('users').get(1, { query: { $select: ['name'] } });
// This will hydrate the cache with a record { id: 1, name: 'Johnny Cash' }

app.service('users').get(1);
// This will be be returned from the cache as  { id: 1, name: 'Johnny Cash' }
// instead of a fully hydrated record

This is the offending line

feathers-hooks-common/lib/services/cache.js

Line 20 in 234d9c4

if (context.method === 'find' && $select) return;

It can be updated to

// if (context.method === 'find' && $select) return;
if ($select) return;
// No mater what method is used, if the result is "filtered" via $select it should not
// be added to the cache

Problem 2 - When using a query with get() the record is cached and then no subsequent queries on get() to that record are respected.

app.service('users').get(1, { query: { name: 'Johnny Cash' } });
// This query is valid, user with ID 1 also has name `Johnny Cash` and this record is
// hydrated in the cache. 

app.service('users').get(1, , { query: { name: 'Patsy Cline' } });
// This will be be returned from the cache as  { id: 1, name: 'Johnny Cash' }
// even though user with ID 1's name is `Johnny Cash` and we queried by
// `Patsy Cline`. This should return a NotFound error

This is the offending line

feathers-hooks-common/lib/services/cache.js

Line 37 in 234d9c4

const value = cacheMap.get(key);

It could be update to something like

if (!context.params.query || !Object.keys(context.params.query).length) {
  const key = makeCacheKey(context.id);
  const value = cacheMap.get(key);

  if (value) context.result = value;
  return context;
}

Expected behavior

The cache only returns valid, fully hydrated records

Actual behavior

The cache is filled with invalid, partial records

I would be happy to open a PR to fix these bugs in this hook. But, I am curious how you guys think it should be handled. Should it be handled by simply being hardcoded as it is in the examples above? This is simple and follows the standard feathers/feathers-db-adapter styles. Or, do we pass in some more options? This is flexible...but adds more config (even though the defaults are fine, more config is...meh).

[DaddyWarbucks]

Whoops! I actually wrote this issue while signed into another account. Ping me for any questions about this rather than 6thManMovers.

[DaddyWarbucks]

There is also a major problem with patch() and remove() when multi: true. The hook uses the getItems util to get the data in the BEFORE hook to determine what records are being mutated. This assumes that the data will have the id. For example,

app.service('users').patch(1, { name: 'Johnny Cash' });
// This should work as expected.
// The record with ID 1 is removed from the cache in a before hook

app.service('users').patch(null, { name: 'Not JC' }, { query: { name: 'Johnny Cash' }});
// This breaks. The hook has no way to determine what the ID
// of the record is to remove it from the cache

app.service('users').patch(null, { id: 1, name: 'Not JC' }, { query: { name: 'Johnny Cash' }});
// I believe this would work because the `data` has the ID...but this is not how
// this patch with query is intended to work...I don't want to patch all records to have ID 1

When patching/removing multiple records, the records must be removed in an AFTER hook as opposed to in the before hook as they are done now.

[DaddyWarbucks]

There is also a major problem with patch() and remove() when multi: true. The hook uses the getItems util to get the data in the BEFORE hook to determine what records are being mutated. This assumes that the data will have the id. For example,

app.service('users').patch(1, { name: 'Johnny Cash' });
// This should work as expected.
// The record with ID 1 is removed from the cache in a before hook

app.service('users').patch(null, { name: 'Not JC' }, { query: { name: 'Johnny Cash' }});
// This breaks. The hook has no way to determine what the ID
// of the record is to remove it from the cache

app.service('users').patch(null, { id: 1, name: 'Not JC' }, { query: { name: 'Johnny Cash' }});
// I believe this would work because the `data` has the ID...but this is not how
// this patch with query is intended to work...I don't want to patch all records to have ID 1

When patching/removing multiple records, the records must be removed in an AFTER hook as opposed to in the before hook as they are done now.

This is actually not true. i was wrong about this comment. The cache is cleared properly when patching multi. The hook attempts to remove cached items in the before hook, but it then also sets those items in the after hook overwriting them there.

[daffl]

Closed via #576

[soulofmischief]

@DaddyWarbucks @daffl I seem to be experiencing the above issue, getting missing ID errors when using multi remove.

Is there something I'm missing? I can find time to look into the implementation tomorrow otherwise.

[DaddyWarbucks]

Can you share some code?