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

Build native sql for a dialect without to string #2237

merged 3 commits into from Sep 27, 2017


Copy link

@elhigu elhigu commented Sep 21, 2017

There has been a problem that knex hasn't been able to be used as a just query builder, except with SQL where all values are interpolated to one string, which has been passed as a string to driver.

This PR adds a way to get toSQL() result also in native SQL dialects format, where bindings are given separately and SQL string with dialects replacements are provided separately.

For example with postgresql dialalect query would be like:

  sql: `select * from "table" where id = $1`,
  bindings: [1]

Feature is implemented to the result of toSQL()as a getter function to prevent memory overhead of storing SQL strings and bindings always in two different format.

Here is also one related issue, where this was first requested #1286

Copy link
Member Author

@elhigu elhigu commented Sep 21, 2017

@wubzz any comments? To me its a bit hacky to add anonymous function to every toSQL() result, but it felt a better option than changing return type of toSQL() to some new class instance.

Copy link

@wubzz wubzz commented Sep 22, 2017

@elhigu I agree adding it a new class is overkill. A new function is sufficient enough. However for reading purposes I'd probably suggest something along the lines of:

  toSQL(method, tz) {
    this._undefinedInWhereClause = false;

    method = method || this.method

    const query = {
      options: reduce(this.options, assign, {}),
      timeout: this.timeout,
      cancelOnTimeout: this.cancelOnTimeout,
      sql: this[method](),
      bindings: this.formatter.bindings || [],
      __knexQueryUid: uuid.v4(),
      toNative: () => ({
       sql: this.client.positionBindings(query.sql),
       bindings: this.client.prepBindings(query.bindings),

   if ((method === 'select' || method === 'first') && { =;

    if(this._undefinedInWhereClause) {
      throw new Error(
        `Undefined binding(s) detected when compiling ` +
        `${method.toUpperCase()} query: ${query.sql}`

    return query;

But this suggestion isn't really because of your change, the function was already kind of strange to interpret, especially the part where it forces sql to become an object only to call assign for evidentally no reason at all?

Pardon the strange indentation..

Copy link
Member Author

@elhigu elhigu commented Sep 23, 2017

@wubzz Thanks for comments. I agree that original way how that method was written is a bit odd. I'll try to do something to it before merging.

Copy link
Member Author

@elhigu elhigu commented Sep 27, 2017

Looks like actually that assign value to query is necessary, since there are more attributes than just sql inside the value. I cleaned it up a bit anyways though.

@elhigu elhigu force-pushed the build-native-sql-for-a-dialect-without-to-string branch from fba5ad9 to 8f395cb Sep 27, 2017
@elhigu elhigu merged commit 15639d0 into master Sep 27, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
continuous-integration/travis-ci/push The Travis CI build passed
elhigu added a commit to ivanfilhoz/knex that referenced this pull request Oct 16, 2017
* Exposed also positionBinding method to client base class and refactored calls to em

* Added toSQL().toNative() getter which returns dialect specfic sql and bindings.

* Refactored toSQL method implementation
@kibertoad kibertoad deleted the build-native-sql-for-a-dialect-without-to-string branch Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants