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

Feat: Allow to extend knex query builder #3334

Merged
merged 9 commits into from Jul 23, 2019
@@ -1,5 +1,6 @@
const Raw = require('./raw');
const Client = require('./client');
const QueryBuilder = require('./query/builder');

const makeKnex = require('./util/make-knex');
const parseConnection = require('./util/parse-connection');
@@ -56,6 +57,7 @@ function Knex(config) {

// Expose Client on the main Knex namespace.
Knex.Client = Client;
Knex.QueryBuilder = { extend: QueryBuilder.extend };

/* eslint no-console:0 */

@@ -43,6 +43,7 @@ function Builder(client) {
this._notFlag = false;
this._asColumnFlag = false;
}

inherits(Builder, EventEmitter);

const validateWithArgs = function(alias, statement, method) {
@@ -1219,4 +1220,14 @@ Builder.prototype.del = Builder.prototype.delete;
require('../interface')(Builder);
helpers.addQueryContext(Builder);

Builder.extend = function(methodName, fn) {
if (Builder.prototype.hasOwnProperty(methodName)) {
throw new Error(
`Can't extend QueryBuilder with existing method ('${methodName}').`
);
}

assign(Builder.prototype, { [methodName]: fn });
};

module.exports = Builder;
@@ -1,4 +1,5 @@
const Knex = require('../../src/index');
const QueryBuilder = require('../../src/query/builder');
const { expect } = require('chai');
const bluebird = require('bluebird');
const sqliteConfig = require('../knexfile').sqlite3;
@@ -468,4 +469,37 @@ describe('knex', () => {
});
});
});

describe('extend query builder', () => {
let connection;
beforeEach(() => {
connection = new sqlite3.Database(':memory:');
});

afterEach(() => {
connection.close();
delete QueryBuilder.prototype.customSelect;
});

it('should extend default queryBuilder', (done) => {
Knex.QueryBuilder.extend('customSelect', function(value) {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 7, 2019

Collaborator

This leaks extended querybuilder outside of thus test, I think. Would be good to delete custom metode in after() block.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 7, 2019

Author Contributor

So you suggest to add remove method as well? cause with the current api, it always adds the method for the entire app.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 7, 2019

Collaborator

wouldn't simple delete Knex.Querybuilder.prototype.customMethod work? I don't think we should expose deletion method via API.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 7, 2019

Author Contributor

It will work, I will update my PR soon

return this.select(this.client.raw(`${value} as value`));
});

const knex = Knex({ client: 'sqlite3' });
knex
.connection(connection)
.customSelect(42)
.then((result) => {
expect(result[0].value).to.equal(42);
done();
});
});

This comment has been minimized.

Copy link
@elhigu

elhigu Jul 8, 2019

Member

There should be also tests that custom method is available for example in transaction and maybe in some cases one likes to add it to join builder or some nested subquery builder.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 8, 2019

Author Contributor

Not sure I'm following, the extend is done on the Builder.prototype only once in the app (before any querying).
There is no difference if you are using the custom method with sub-query or any other chain.

I'm missing something?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

Transaction is a different entity, not an original knex builder, but user would be justified to expect custom methods on it as well. It is highly likely we'd need to copy them over, but test would show that. Same with join builder, it's a different thing.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 8, 2019

Author Contributor

So, should I add extend to them as well? should it be separate method for each?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

No, it would be better if they were inherited automatically. User needs not to concern himself with difference between all three.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 8, 2019

Author Contributor

Agree, do Transaction & Join Builder are inherites from Builder? If so, the requirement is to check that it extends the prototype, meaning that is available at the child class?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

Nope:
src/transaction.js
src/query/joinclause.js (TBH, join builder is a farily different beast, I'm not sure querybuilder methods make much sense there. @elhigu, what use-case did you have in mind?)

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

I'm not sure what is going to be safest and cleanest solution here, TBH. Maybe QueryBuilder.extend could also extend prototype of Transaction, or overall .extend method could be moved to general Knex.extend that covers both QueryBuilder and Transaction.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 8, 2019

Author Contributor

What is the use case of extending join & transaction?
In my opinion, It is not correct to extend all of the builders with one method,

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

You can use transaction pretty much same way you can use knex instance. See example from documentation:

// Using trx as a transaction object:
const trx = await knex.transaction();

const books = [
  {title: 'Canterbury Tales'},
  {title: 'Moby Dick'},
  {title: 'Hamlet'}
];

trx('catalogues')
  .insert({name: 'Old Books'}, 'id')
  .then(function(ids) {
    books.forEach((book) => book.catalogue_id = ids[0]);
    return trx('books').insert(books);
  })
  .then(trx.commit)
  .catch(trx.rollback);
})

Considering that currently transaction supports same query-building methods that knex.QueryBuilder does, as a user I would be very surprised if I'm supposed to set those up separately.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

Although looking at the code, I suspect this is going to just work (test to prove that would be most welcome, though). The way transactor is created is this:

  const transactor = makeKnex(trxClient);

So what gets called is this:

function makeKnex(client) {
  // The object we're potentially using to kick off an initial chain.
  function knex(tableName, options) {
    return createQueryBuilder(knex.context, tableName, options);
  }

  redefineProperties(knex, client);
  return knex;
}

Unless I'm missing something, this is going to call same QueryBuilder constructor in the end.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 8, 2019

Author Contributor

Ha, I'm not using transaction in that way ;)

knex.transaction((trx) => {
  knex(t1).transacting(trx)...
})

If it has the same query methods, it make sense to extend it as well.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 8, 2019

Collaborator

trx var in this context is the same transactor you would get from await knex.transaction(), and see above, I suspect it already works.

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 8, 2019

Author Contributor

Let me add a test for it to assure it, if it not works we will need to think how extend it as well

it('should throw exception when extending existing method', () => {
expect(() =>
Knex.QueryBuilder.extend('select', function(value) {})
).to.throw(`Can't extend QueryBuilder with existing method ('select')`);
});
});
});
@@ -353,6 +353,10 @@ interface Knex<TRecord extends {} = any, TResult = any[]>
seed: Knex.Seeder;
fn: Knex.FunctionHelper;
ref: Knex.RefBuilder;

QueryBuilder: {
extend(methodName: string, fn: Function): void;
};
}

declare function Knex<TRecord = any, TResult = unknown[]>(
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.