diegohaz / querymen

Querystring parser middleware for MongoDB, Express and Nodejs (MEN)
Other
130 stars 35 forks source link

Wrong pagination params on cursor #50

Open drochag opened 7 years ago

drochag commented 7 years ago

I have the following Schema


    new querySchema({
      type: String,
      garment: String,
      brand: String,
      supplier: String,
      name: RegExp
    })

Haven't touched anything regarding pagination or limit, however after updating to 2.1.3 to include the cache problem, which I was having as well, this happens:

http://localhost:9000/branch-products?sort=-createdAt&page=1&limit=10
Mongoose: branchproducts.find({}, { skip: 0, limit: 10, sort: { createdAt: -1 }, fields: {} })

http://localhost:9000/branch-products?sort=-createdAt&page=2&limit=10
Mongoose: branchproducts.find({}, { skip: 30, limit: 10, sort: { createdAt: -1 }, fields: {} })

http://localhost:9000/branch-products?sort=-createdAt&page=3&limit=10
Mongoose: branchproducts.find({}, { skip: 60, limit: 10, sort: { createdAt: -1 }, fields: {} })

http://localhost:9000/branch-products?sort=-createdAt&page=4&limit=10
Mongoose: branchproducts.find({}, { skip: 90, limit: 10, sort: { createdAt: -1 }, fields: {} })

As if the limit was 30. Any ideas?

chemitaxis commented 7 years ago

Hi @DanMMX

I have tested and this is the result I have:

request(app)
      .get('/tests?sort=-createdAt&page=12&limit=20')
      .query(new querymen.Schema({
        name: String
      }))
      .expect(200)
      .end((err, res) => {
        if (err) throw err
        t.equal(res.body.length, 0, 'should return empty array')
      })

tests.find({}, { skip: 220, limit: 20, sort: { createdAt: -1 }, fields: {} })

Limit default value is 30... Can you please check this? Thanks.

drochag commented 7 years ago

Yes, I understand the limit default is 30, however, sending a limit of 10 pagination should be skipping _this.param('limit').value() * (value - 1), right? And for a limit of 10 on page 2, skip should be 10 * 1 but instead it gets 30 as if limit was 30 (same for page 3, 4, etc...).

Here's how the req.query looks before passing through querymen:


URL: http://localhost:9000/branch-products?sort=-createdAt&page=1&limit=10
req.query { sort: '-createdAt', page: '1', limit: '10' }
<-- querymen middleware -->
cursor { skip: 0, limit: 10, sort: { createdAt: -1 } }
Mongoose: branchproducts.find({}, { skip: 0, limit: 10, sort: { createdAt: -1 }, fields: {} })

URL: http://localhost:9000/branch-products?sort=-createdAt&page=2&limit=10
req.query { sort: '-createdAt', page: '2', limit: '10' }
<-- querymen middleware -->
cursor { skip: 30, limit: 10, sort: { createdAt: -1 } }
Mongoose: branchproducts.find({}, { skip: 30, limit: 10, sort: { createdAt: -1 }, fields: {} })

URL:http://localhost:9000/branch-products?sort=-createdAt&page=3&limit=10
req.query { sort: '-createdAt', page: '3', limit: '10' }
<-- querymen middleware -->
cursor { skip: 60, limit: 10, sort: { createdAt: -1 } }
Mongoose: branchproducts.find({}, { skip: 60, limit: 10, sort: { createdAt: -1 }, fields: {} })

It looks like the limit gets ignored at some point when the skip parse happens.

@chemitaxis ^

drochag commented 7 years ago

Further info (debugging querymen/dist/querymen-schema.js) :


URL: /branch-products?sort=-createdAt&page=1&limit=10
req.query { sort: '-createdAt', page: '1', limit: '10' }
querymen-schema.js:269 - get |  this.params[name].value(): 30 |  name: limit // here limit is 30 ¿?
querymen-schema.js:138 - page#parse |  _this.param('limit').value(): 30 |  page value: 1 // here limit is 30 ¿?
querymen-schema.js:269 - get |  this.params[name].value(): 30 |  name: limit // here limit is 30 ¿?
querymen-schema.js:147 - limit#parse |  limit value: 10 // here limit is 10 - this is ok
cursor { skip: 0, limit: 10, sort: { createdAt: -1 } }
Mongoose: branchproducts.find({}, { skip: 0, limit: 10, sort: { createdAt: -1 }, fields: {} })

URL: /branch-products?sort=-createdAt&page=2&limit=10
req.query { sort: '-createdAt', page: '2', limit: '10' }
querymen-schema.js:269 - get |  this.params[name].value(): 30 |  name: limit // here limit is 30 ¿?
querymen-schema.js:138 - page#parse |  _this.param('limit').value(): 30 |  page value: 2 // here limit is 30 ¿?
querymen-schema.js:269 - get |  this.params[name].value(): 30 |  name: limit // here limit is 30 ¿?
querymen-schema.js:147 - limit#parse |  limit value: 10 // here limit is 10 - this is ok
cursor { skip: 30, limit: 10, sort: { createdAt: -1 } }
Mongoose: branchproducts.find({}, { skip: 30, limit: 10, sort: { createdAt: -1 }, fields: {} })

URL: /branch-products?sort=-createdAt&page=3&limit=10
req.query { sort: '-createdAt', page: '3', limit: '10' }
querymen-schema.js:269 - get |  this.params[name].value(): 30 |  name: limit // here limit is 30 ¿?
querymen-schema.js:138 - page#parse |  _this.param('limit').value(): 30 |  page value: 3 // here limit is 30 ¿?
querymen-schema.js:269 - get |  this.params[name].value(): 30 |  name: limit // here limit is 30 ¿?
querymen-schema.js:147 - limit#parse |  limit value: 10 // here limit is 10 - this is ok
cursor { skip: 60, limit: 10, sort: { createdAt: -1 } }
Mongoose: branchproducts.find({}, { skip: 60, limit: 10, sort: { createdAt: -1 }, fields: {} })

Haven't looked deeply into the querymen-schema.js code, but it clearly looks that the get is somehow getting the default values instead of the current params.

chemitaxis commented 7 years ago

Ok, I will look again... but the test is working 🤔

Do you know how to pass the test? Clone and run test?

And we are using querymen in production without problems...

chemitaxis commented 7 years ago

I have created a project test, and all is working fine... can you check it please? Thanks.

https://github.com/chemitaxis/test_rest

drochag commented 7 years ago

I don't see any test regarding pagination on https://github.com/chemitaxis/test_rest I'll try to make a PR reproducing the problems, but I've found that the problem happens after using a custom Schema.

import { middleware as query, Schema as querySchema } from 'querymen'

// this works
router.get('/',
  token({ required: true, role: 'branch' }),
  query(),
  index)

// this doesn't
router.get('/',
  token({ required: true, role: 'branch' }),
  query(
    new querySchema({
      branchCollection: String,
      composition: String,
      brand: String,
      code: RegExp
    })
  ),
  index)
chemitaxis commented 7 years ago

Hi @DanMMX ok, I can reproduce it right now...

If you create the querySchema, inside the middleware, it fails, if you do that:

const customSchema = new querySchema({
  branchCollection: String,
  composition: String,
  brand: String,
  code: RegExp
})
// this works too
router.get('/',
  token({ customSchema }),
  query(),
  index)

I will check it... until that, you can use previous version... sorry for the inconvenience. PR is welcome if you find an error ;)

drochag commented 7 years ago

@chemitaxis can you explain further this comments? not sure I can understand them, and confirming after that PR this bug was introduced.

drochag commented 7 years ago

@diegohaz can you take a look at this one? You can see the PR #51 to check the test that is failing, the bug was introduced after #49

however I'm not able to solve #30 without triggering this bug

chemitaxis commented 7 years ago

I will make a new PR tomorrow removing the previous changes... it's worth the solution applyed that the problem in my opinion

diegohaz commented 7 years ago

I think we should revert the last change since it introduced another bug and try to solve #30 in another way, trying to pass the failing tests on #51 and #52. What do you think?

drochag commented 7 years ago

Sounds nice … however both things (with cache problem and pagination problem) are a big issue on my app. Have spent several hours checking the bug, my only guess is it is related to rich-param or somehow caching the previous things.

Haven't been able to find out a solution for one without introducing the 2nd one 😢

gitcloned commented 4 years ago

Any update on this issue.. by default skip assumes page size to be 30. Anyway to override and specify custom page size

gitcloned commented 4 years ago

got it solved .. passed custom option for "default" in "limit", while creating Schema class.

iamarpitpatidar commented 3 years ago

Hi, any update on the issue? this issue is still a problem.

deivisonresende commented 1 year ago

got it solved .. passed custom option for "default" in "limit", while creating Schema class.

If pass limit: { default: 10 } inside of Schema instance, resolved pagination called with 10, but doesn't works when calling with other limit value :

page=1&limit=30 => { skip: 0, limit: 30, sort: { createdAt: -1 } } page=2&limit=30 => { skip: 10, limit: 30, sort: { createdAt: -1 } } page=3&limit=30 => { skip: 20, limit: 30, sort: { createdAt: -1 } }

Is there any version that doesn't get this error when using Schema?