Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add validation in .offset() #2908

Merged
merged 14 commits into from Oct 15, 2019
Merged

add validation in .offset() #2908

merged 14 commits into from Oct 15, 2019

Conversation

HurSungYun
Copy link
Contributor

@HurSungYun HurSungYun commented Nov 15, 2018

I believe value in .offset(value) must be integer like .limit(value)

However, there is no warning message for this.

Copy link
Member

@elhigu elhigu left a comment

Good idea, but PR is missing tests. Also maybe knex.raw / query builder should be allowed. At least it should be tested what that method does in case of those.

@HurSungYun
Copy link
Contributor Author

@HurSungYun HurSungYun commented Nov 16, 2018

@elhigu

https://github.com/tgriesser/knex/blob/master/test/unit/query/builder.js#L6457

I believe this test case covers my PR now.

Is there any scenarios that test case cannot cover?

@elhigu
Copy link
Member

@elhigu elhigu commented Nov 16, 2018

Actually that case looks like that it has been designed so that passing null actually clears the offset, thus it shouldn't show warning. Also in case of warning it should check that warning is actually emitted. It can be done by using custom loggers in test.

Also I would prefer throwing an error instead of warning.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 16, 2018

@HurSungYun @elhigu Actually, passing null to reset sounds like a valid case, no?

@HurSungYun
Copy link
Contributor Author

@HurSungYun HurSungYun commented Nov 16, 2018

We discussed something like this #2897

It concluded that everything like this should be valid, so I believe this one also should be valid case

@HurSungYun
Copy link
Contributor Author

@HurSungYun HurSungYun commented Nov 16, 2018

@elhigu

I believe this test cases are enough for checking whether warning message is emitted.

@elhigu
Copy link
Member

@elhigu elhigu commented Nov 18, 2018

Now this prevents using knex.raw / query builder as a parameter for offset, which has been working previously.

@HurSungYun
Copy link
Contributor Author

@HurSungYun HurSungYun commented Nov 19, 2018

My bad. I misunderstood your comment. I'll revise it to work properly.

@HurSungYun
Copy link
Contributor Author

@HurSungYun HurSungYun commented Nov 20, 2018

The code of .limit() is like this.

  // Only allow a single "limit" to be set for the current query.
  limit(value) {
    const val = parseInt(value, 10);
    if (isNaN(val)) {
      this.client.logger.warn('A valid integer must be provided to limit');
    } else {
      this._single.limit = val;
    }
    return this;
  },

I tried some tests in Node 8,

> client('test').select().limit(client.raw('3')).toSQL()
{ method: 'select',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ 3 ],
  __knexQueryUid: 'cd175d39-f411-4210-b242-519a0c295d47',
  sql: 'select * from `test` limit ?' }

> client('test').select().limit(client.raw('?', [3])).offset(undefined).toSQL()
{ method: 'select',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ 3 ],
  __knexQueryUid: 'b90d327a-54de-4e40-9ee4-4d6e03e058cf',
  sql: 'select * from `test` limit ?' }

> parseInt(client.raw('3'), 10)
3

So, it does not break using knex.raw as a parameter if the evaluated result of knex.raw can be parsed to int and if that cannot be parse to int, it is invalid input anyway.

I would like to suggest two options for .offset() validation

offset(value) {
  if (
    value instanceof Builder ||
    value instanceof Raw ||
    isNil(value)
  ) {
    this._single.offset = value;
  } else {
    const val = parseInt(value, 10);
    if (isNaN(val)) {
      this.client.logger.warn('A valid integer must be provided to offset');
    } else {
      this._single.offset = val;
    }
  }
  return this;
},
// check all types of value and value = null, undefined case

OR

offset(value) {
  if (isNil(value)) {
    this._single.offset = value; // for resetting
  } else {
    const val = parseInt(value, 10);
    if (isNaN(val)) {
      this.client.logger.warn('A valid integer must be provided to offset');
    } else {
      this._single.offset = val;
    }
  }
  return this;
},
// check case of value = null, undefined 

I understand all this should be backward compatible, but I think it's weird that there's no validation code in contrast with .limit()

@NeoyeElf
Copy link

@NeoyeElf NeoyeElf commented Sep 26, 2019

when will this changes be releaed?

@HurSungYun HurSungYun reopened this Oct 8, 2019
@HurSungYun
Copy link
Contributor Author

@HurSungYun HurSungYun commented Oct 8, 2019

@NeoyeElf I fixed something but i don't know when this PR is merged

@HurSungYun HurSungYun closed this Oct 8, 2019
@HurSungYun HurSungYun reopened this Oct 8, 2019
oracledb: new Oracledb_Client(
Object.assign({ client: 'oracledb' }, customLoggerConfig)
),
// sqlite3: new SQLite3_Client(
Copy link
Collaborator

@kibertoad kibertoad Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be commented out?

Copy link
Contributor Author

@HurSungYun HurSungYun Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was no sqlite3 tests using offset a year ago.

maybe someone have missed test code so I did the same thing.

still some test does not include sqlite3, but it may be fixed in another PR.

anyway I un-commented it.

test/unit/query/builder.js Outdated Show resolved Hide resolved
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 14, 2019

Sorry for the delay in reviewing. This can be merged after couple of minor comments are addressed.

@HurSungYun HurSungYun closed this Oct 15, 2019
@HurSungYun HurSungYun reopened this Oct 15, 2019
@kibertoad kibertoad merged commit c532275 into knex:master Oct 15, 2019
1 check passed
@rjkhan
Copy link

@rjkhan rjkhan commented Jan 13, 2020

what is the default value of offset()

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jan 13, 2020

@rjkhan No offset, I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants