Service call sometimes returns data from wrong table when called internally

[RobIsHere]

Steps to reproduce

• create a knex db service
• add a find before hook that uses hook.params.knex with any custom query:

hook => {
    hook.params.knex = hook.service.createQuery(hook.params.query);
}

• at find after hook add a populate common hook including something from other service

Expected behavior

• it should find the data from other service and include it

Actual behavior

• it includes the record in this service again in itself

System configuration

Newest versions of feathers

Reason

https://github.com/feathersjs/feathers-knex/blob/master/src/index.js#L132
var q = params.knex || this.createQuery(params);

this reuses a previously executed query, although it has been set in the "parent" service fetching the parent record. As params.knex is already set, the params of the called "child" service are ignored and the parent query is run again.

How to solve this?

[RobIsHere]

Could we just unset params.knex after using it in the service? After https://github.com/feathersjs/feathers-knex/blob/master/src/index.js#L132

That assumes that a the custom query is only used once per service call. But I think that's no restriction?

I think this can't be solved in the populate hook because it also affects manual calls to the other service with passing params.

[daffl]

First, I would avoid using the populate hook with an SQL database if possible. That's what SQL joins are for which are much more performant.

The easiest fix for your issue is probably to set params.knex = null; in an after all hook. I'm not sure if the adapter should do this (we got bit several times mutating existing objects in the database adapters).

[RobIsHere]

The problem even exists if I just call another service after find without using populate. Performance isn't a problem in my current application. It will never exceed a critical amount of data. For me it would be great to have the hooks in place also for the included data rather than sql joins.

And it's really surprising. I needed about 3 hours to find out what's going on, digging into all related source codes. So I think setting params.knex = null in the hook is just a hack because you would never expect, that you have to do this.

I understand that you got bitten (and I've seen that you are working on a new hook object structure).
On the other hand knex is a most knex-specific attribute, so who should be responsible for resetting this if not knex?

(A side note: the service specific after all hook is called before the after find hook. This is another surprise that I found. Is this intended behaviour?)

[daffl]

This should only happen if you pass hook.params to the other service calls in which case it is done explicitly (just as setting params.knex is) and it is up to you to make sure that those are the params you want.

In combination with populate it is indeed not ideal (most other hooks don't call other services so it probably not a problem) but I don't think there is too much that can be done because individual service adapters are not aware of their hook object..

The all hook order is intended that way and documented in the hook API:

Pro Tip: When using the full object, all is a special keyword meaning this hook will run for all methods. all hooks will be registered before other method specific hooks.

[RobIsHere]

Thank you for the clarification about the hooks docs. I mixed it up with application hooks where after is called after all others and before before all.

You are absolutely right, there's no ideal solution at least now. Wiping params.knex out after it has been seen by knex isn't a highlight in software engineering.

But from a practical use-case standpoint: who would want to run the same query twice. And if params.knex would be documented as "cleaned after execution", ...

Btw: Thank you and the other contributors for this excellent framework. I love it!

[daffl]

I'm wondering if we should add some failsafe that just ignores params.knex if it's not for the services own table name.

[RobIsHere]

After thinking about it a while: I think the failsave would defeat one of the use cases of params.knex.
Having a custom query that accesses another table/stored procedure in e.g. create than at find/get is a use case for params.knex.

[RobIsHere]

I think the underlying problem is, that populate would have to have knowledge about params.knex and params.sequelize. For stashing and restoring them afterwards. Which is not good from the engineering perspective.

So having params.knex and params.sequelize, ... inside of some subkey would help here:
params.database.knex, params.database.sequelize.

Than populate could do something like:

let tmp = params.database;
params.database = {};
(query other service)
params.database = tmp;

Additionally on forwarding params outside of populate, you know which key you must omit independent of the database implementation. So when you change from sequelize to knex - like I did now because of an ongoing hassle with german words wrong pluralization - it will continue working like before.

Thinking farther

Actually what params.knex and params.sequelize is doing is: execute an action on the database. It is a kind of imperative command and more than just a parameter.

So why not making a key for this kind of thing, instead of "database" it could be "action".
params.action.knex / params.action.sequelize
Where action could be defined as "Execute an action on the called service". (Which implies: no recursiveness)