feathersjs-ecosystem / feathers-objection

Feathers database adapter for Objection.js, an ORM based on KnexJS SQL query builder for Postgres, Redshift, MSSQL, MySQL, MariaDB, SQLite3, and Oracle. Forked from feathers-knex.
MIT License
98 stars 48 forks source link

Bug: Query customization doesn't apply to the count SQL query #120

Closed mikeconley12 closed 4 years ago

mikeconley12 commented 4 years ago

I created a reproduction repository: https://github.com/mikeconley12/feathers-objection . See the changes: https://github.com/feathersjs-ecosystem/feathers-objection/compare/master...mikeconley12:master

Steps to reproduce

  1. git clone https://github.com/mikeconley12/feathers-objection.git
  2. cd feathers-objection/
  3. npm i
  4. DEBUG=knex:query npm run example
  5. Open http://localhost:3030/todos?text=foo in a browser

In your terminal, you should see the following:

  knex:query select count(`todos`.`id`) as `total` from `todos` where `text` = ? undefined +5s
  knex:query select `todos`.* from `todos` where `text` = ? and `complete` = ? limit ? undefined +3ms

The first SQL query doesn't have the `complete` = ? condition, but the second SQL query has this condition.

dekelev commented 4 years ago

Please open bugs with informative description and examples.

mikeconley12 commented 4 years ago

I'm sorry if I'm not clear. Let me explain.

I customize the query in my example (this is an undocumented feature, but the documentation says: "It works like the Knex service adapter", and the Knex service adapter has customizing the query feature).

But my modifications to the query don't apply to the count SQL query. See logs:

  knex:query select count(`todos`.`id`) as `total` from `todos` where `text` = ? undefined +5s
  knex:query select `todos`.* from `todos` where `text` = ? and `complete` = ? limit ? undefined +3ms

I think the problem may be this:

You use params.objection here,

const q = params.objection || this.createQuery(params);

But in the count query, you don't use params.objection

const countQuery = this._createQuery(params);
dekelev commented 4 years ago

"It works like the Knex service adapter" - in no point this lib was ment to be like the Knex service adapter.

This lib was forked from the Knex service lib years ago and was built in a different way.

Please read the docs (README).

בתאריך שבת, 17 באוק׳ 2020, 15:27, מאת mikeconley12 ‏< notifications@github.com>:

I'm sorry if I'm not clear. Let me explain.

I customize the query in my example https://github.com/feathersjs-ecosystem/feathers-objection/compare/master...mikeconley12:master (this is an undocumented feature, but the documentation says: "It works like the Knex service adapter", and the Knex service adapter has customizing the query https://github.com/feathersjs-ecosystem/feathers-knex#customizing-the-query feature).

But my modifications https://github.com/mikeconley12/feathers-objection/blob/master/example/app.js#L47 to the query don't apply to the count SQL query. See logs:

knex:query select count(todos.id) as total from todos where text = ? undefined +5s knex:query select todos.* from todos where text = ? and complete = ? limit ? undefined +3ms

I think the problem may be this:

You use params.objection here https://github.com/feathersjs-ecosystem/feathers-objection/blob/master/src/index.js#L474 ,

const q = params.objection || this.createQuery(params);

But in the count query, you don't use https://github.com/feathersjs-ecosystem/feathers-objection/blob/master/src/index.js#L511 params.objection

const countQuery = this._createQuery(params);

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/feathersjs-ecosystem/feathers-objection/issues/120#issuecomment-710889250, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABB5E3N6FS62XZGT2EV6RILSLGEUXANCNFSM4SE5VXEQ .

mikeconley12 commented 4 years ago

Thanks for the answers!