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
merged 3 commits into from Sep 27, 2017

Conversation

Projects
None yet
2 participants
@elhigu
Collaborator

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

@elhigu elhigu referenced this pull request Sep 21, 2017

Merged

To native sql #53

@elhigu

This comment has been minimized.

Collaborator

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.

@wubzz

This comment has been minimized.

Collaborator

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 = {
      method,
      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') && this.single.as) {
       query.as = this.single.as;
    }

    if(this._undefinedInWhereClause) {
      debugBindings(defaults.bindings)
      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..

@elhigu

This comment has been minimized.

Collaborator

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.

@elhigu

This comment has been minimized.

Collaborator

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 merged commit 15639d0 into master Sep 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

elhigu added a commit to ivanslf/knex that referenced this pull request Oct 16, 2017

Build native sql for a dialect without to string (tgriesser#2237)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment