cannblw / knex-paginator

Simple paginator for Knex. It adds the .paginate() function to Knex's query builder.
15 stars 10 forks source link

revert "fix error then join" commit #2

Closed xausky closed 5 years ago

xausky commented 5 years ago

In the last PR I removed the join block from the Count statement, from a performance perspective, but it caused a problem. such as:

await knex('mods').select('mods.id', 'mods.name', 'author.name AS author', 'count', 'viewer', 'update')
            .leftJoin('author', 'mods.author', 'author.id').where(function () {
                this.where('author.name', 'LIKE', '%' + req.query.query + '%').orWhere('mods.name', 'LIKE', '%' + req.query.query + '%')
            }).andWhere('index', 'allow').orderByRaw('count + viewer DESC').paginate(12, page, true);

Use join's table in the where statement. An error occurred:

error: missing FROM-clause entry for table "author"
    at Connection.parseE (/home/xausky/Documents/umms/node_modules/pg/lib/connection.js:554:11)
    at Connection.parseMessage (/home/xausky/Documents/umms/node_modules/pg/lib/connection.js:379:19)
    at TLSSocket.<anonymous> (/home/xausky/Documents/umms/node_modules/pg/lib/connection.js:119:22)
    at TLSSocket.emit (events.js:182:13)
    at addChunk (_stream_readable.js:283:12)
    at readableAddChunk (_stream_readable.js:264:11)
    at TLSSocket.Readable.push (_stream_readable.js:219:10)

So I should revert that commit.

cannblw commented 5 years ago

Can you send the output SQL generated by that statement? You can do this by adding the function .toSQL(). Also, which database package are you using?

xausky commented 5 years ago

this debug output

{ method: 'first',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ '%w%', '%w%', 'allow', 1 ],
  __knexQueryUid: '431cd298-2d6f-4018-88b8-d98ff89556b5',
  sql:
   'select count(*) as "total" from "mods" where ("author"."name" LIKE ? or "mods"."name" LIKE ?) and "index" = ? limit ?' }
{ method: 'select',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ '%w%', '%w%', 'allow', 12 ],
  __knexQueryUid: 'a6be20de-aa52-45f4-9cbd-e8d71f61cccb',
  sql:
   'select "mods"."id", "mods"."name", "author"."name" as "author", "count", "viewer", "update" from "mods" left join "author" on "mods"."author" = "author"."id" where ("author"."name" LIKE ? or "mods"."name" LIKE ?)and "index" = ? order by count + viewer DESC limit ?' }
{ error: missing FROM-clause entry for table "author"
    at Connection.parseE (/home/xausky/Documents/umms/node_modules/pg/lib/connection.js:554:11)
    at Connection.parseMessage (/home/xausky/Documents/umms/node_modules/pg/lib/connection.js:379:19)
    at TLSSocket.<anonymous> (/home/xausky/Documents/umms/node_modules/pg/lib/connection.js:119:22)
    at TLSSocket.emit (events.js:182:13)
    at addChunk (_stream_readable.js:283:12)
    at readableAddChunk (_stream_readable.js:264:11)
    at TLSSocket.Readable.push (_stream_readable.js:219:10)
    at TLSWrap.onStreamRead [as onread] (internal/stream_base_commons.js:94:17)

this dependencies

    "knex": "^0.13.0",
    "knex-paginator": "^1.3.0",
    "pg": "^7.5.0",
jacobriddle commented 5 years ago

I just ran into this issue and forked the repository with the intention of making this exact change. The code removed in this PR is also breaking pagination for queries with multiple joins using the MySQL database connector. In effect, the splice() call results in only the last join being added to the COUNT(*) sql statement. This means any columns used in the last join that come from previously joined tables will not be valid.

A couple of issues have been created since this PR went out (#4, #5). I can't think of a case where you would not want the total count for the exact query you will be paginating.

@cannblw could you take a look at merging this PR now that the details you have requested have been provided?

cannblw commented 5 years ago

Sorry, I have had limited time to work on this project. I'm going to check it out now.

cannblw commented 5 years ago

I had a look at the commit and it seems to fix the error.

Once again, thank you for your time, and please let me know if you find any side-effects because this change.