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

Add test with transaction

  • Loading branch information...
felixmosh committed Jul 13, 2019
commit 298266c4a0de2a5cbc1b55981a7aeb20143cf7b4
@@ -1,6 +1,7 @@
const Raw = require('./raw');
const Client = require('./client');
const QueryBuilder = require('./query/builder');
const QueryInterface = require('./query/methods');

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

// Expose Client on the main Knex namespace.
Knex.Client = Client;
Knex.QueryBuilder = { extend: QueryBuilder.extend };
Knex.QueryBuilder = {
extend: function(methodName, fn) {
QueryBuilder.extend(methodName, fn);
QueryInterface.push(methodName);
},
};

/* eslint no-console:0 */

@@ -496,6 +496,18 @@ describe('knex', () => {
});
});

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 have custom method with transaction', async () => {
Knex.QueryBuilder.extend('customSelect', function(value) {
return this.select(this.client.raw(`${value} as value`));
});

const knex = Knex(sqliteConfig);
const trx = await knex.transaction();

const result = await trx.customSelect(42);
expect(result[0].value).to.equal(42);
});

it('should throw exception when extending existing method', () => {
expect(() =>
Knex.QueryBuilder.extend('select', function(value) {})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.