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

Conversation

HurSungYun
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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 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 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

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

@HurSungYun
Copy link
Contributor Author

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

@elhigu

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

@elhigu
Copy link
Member

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 commented Nov 19, 2018

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

@HurSungYun
Copy link
Contributor Author

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

when will this changes be releaed?

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

@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

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

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.

@kibertoad
Copy link
Collaborator

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
@rjkhan
Copy link

rjkhan commented Jan 13, 2020

what is the default value of offset()

@kibertoad
Copy link
Collaborator

@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