Feat: Allow to extend knex query builder

[felixmosh]

This PR allows to extend query builder with custom functions such as paginate or onDuplicateUpdate that will allow add custom behaviour or specific dialect methods over the basic query builder functions.

knex('products')
    .where('price', '<', 20)
    .paginate(15, 1, true)
    .then(paginator => {
        console.log(paginator.current_page);
        console.log(paginator.data);
    });

knex()
  .insert({ id: 1, name: 'foo' })
  .into(customers)
  .onDuplicateUpdate('name');

// which will generate

Insert  into customers (id, name) values (1, 'foo') on duplicate key update name=Values(name)

Currently there is no way to properly extend the QueryBuilder without monkey-patching the QueryBuilder.prototype.

closes #1158

[elhigu]

This feature is wanted and it has been discussed before. I want to research earlier discussions and review the code, since for this I want knex to have carefully designed api and functionality.

This fundamental features should have design discussions before doing PRs or to be prepared to redo the whole implementation in a different manner.

[felixmosh]

Since my implementation is small one i would happy to redo it.
I'm open to discussion 😊

[kibertoad]

TBH, this seems a simple and non-intrusive solution, so I would vote for including this implementation.
My only concern is about TypeScript part of the equation - my assumption is that since QueryBuilder is part of Knex namespace, it should be possible to augment its interface locally and have additional methods on it. I wonder if it's possible to add test for this in https://github.com/tgriesser/knex/blob/master/types/test.ts

One additional suggestion: it might make sense to throw a warning when you are overriding existing method using .extend()

[kibertoad]

Related issues:
#2071
#1579
#1456
#1158

[kibertoad]

There doesn't seem to be many valuable proposals in prior issues, TBH. I'd be more inclined to give the tooling by which a user could implement addOperator as a plugin. from #1456 doesn't seem feasible in he nearby future, and, frankly, sounds like an over-engineering for simple cases where this particular three-lines long solution would do just fine.

[felixmosh]

TBH, this seems a simple and non-intrusive solution, so I would vote for including this implementation.
My only concern is about TypeScript part of the equation - my assumption is that since QueryBuilder is part of Knex namespace, it should be possible to augment its interface locally and have additional methods on it. I wonder if it's possible to add test for this in https://github.com/tgriesser/knex/blob/master/types/test.ts

One additional suggestion: it might make sense to throw a warning when you are overriding existing method using .extend()

I can check, TS by design allows to extend interfaces, but there some case it won't work.
I'm already using that at https://github.com/felixmosh/knex-on-duplicate-update/blob/master/types.d.ts
I will add a test for this.

[felixmosh]

There doesn't seem to be many valuable proposals in prior issues, TBH. I'd be more inclined to give the tooling by which a user could implement addOperator as a plugin. from #1456 doesn't seem feasible in he nearby future, and, frankly, sounds like an over-engineering for simple cases where this particular three-lines long solution would do just fine.

Agree, it can be a base for more pluggable solution (if needed).

What do you think regarding exposing QueryBuilder constructor on knex?

[kibertoad]

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

[felixmosh]

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

[kibertoad]

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

[felixmosh]

It will work, I will update my PR soon

[felixmosh]

@kibertoad I'm not sure how to test the type augmentation of the QueryBuilder extension, can you give me some pointers?

[elhigu]

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.

[felixmosh]

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?

[kibertoad]

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.

[felixmosh]

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

[kibertoad]

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

[kibertoad]

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.

[kibertoad]

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.

[felixmosh]

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.

[kibertoad]

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

[felixmosh]

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

[kibertoad]

@felixmosh Regarding testing: not sure, never tested for this case myself, but from my understanding it is possible to assert literal types for specific lines.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/v8/node-tests.ts
e. g. // $ExpectType <T extends Function>(fn: T, message: string) => T
Not sure if it's going to work, but worth a try.

[elhigu]

Sounds like only the issue #1158 had some really meaningful discussion around the area. Right now I think that the only drawback with this extension method is to be able to nicely override old methods with extended functionality. Being able to call original implementation of rewritten method sounds like something that I would like to have (I've used that quite a lot with objection).

I think that for now we could add this way that just allows to add stuff to prototype and pass it around to other builders as experimental extension support (to be able to deprecate it fast if it is not sufficient). From that experience we can decide later on if we actually want to make extending to work with real inheritance and setting then query builder instances to knex in a bit same way that it was suggested here #1158 (comment) and how it was implemented for objection).

[elhigu]

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

For transaction all the same methods should be found that are found from main query builder... so all use cases I suppose :)

For join builder and subquery builder probably doesn't need to have all the extensions... I suppose extender method could have opts : { queryBuilder: true, joinBuilder: true, subBuilder: true } object to tell if what we are extending (I don't remember if there really is a separate subquery builder though).

[elhigu]

Then another thing which needs testing is to check that instance created with knex.withUserParams({customUserParam: 'table1'}); works fine with extended functionality

[kibertoad]

@elhigu Given that joinBuilder has methods along the line of

  – onIn
  – onNotIn
  – onNull
  – onNotNull
  – onExists
  – onNotExists
  – onBetween
  – onNotBetween

, is that really related to extending QueryBuilder per se? Probably there could be separate method exposed for extending just the JoinBuilder, but I would say that would be a separate PR altogether.

[elhigu]

Separate PR sounds fine if that even is ever needed. I agree that it doesn't sound really important.

[felixmosh]

Sorry for the confusion, what are the action items? :)

[kibertoad]

@felixmosh

1. Add test for using custom methods from transaction (const trx = await knex.transaction; trx.customMethod())
2. Add test for using custom methods from knex copy with user params (const knewWithParams = knex.withUserParam({}); knewWithParams .customMethod())
3. Add documentation in https://github.com/knex/documentation
4. Try adding a test in https://github.com/tgriesser/knex/blob/master/types/test.ts that would assert interface augmentation to work.
5. This one was suggested by @elhigu -> possibility to override existing methods while preserving reference to original methods somehow (I guess those could be reattached to original object using some prefix or container). Not sure if this works correctly for custom methods attached to QueryBuilder, needs a try. I would imagine having something like

function where() {
console.log('log stuff')
this.original.where(...args) // or this.originalWhere(...args)
}

knex.QueryBuilder.extend('where', where)

[felixmosh]

@felixmosh

1. Add test for using custom methods from transaction (const trx = await knex.transaction; trx.customMethod())
2. Add test for using custom methods from knex copy with user params (const knewWithParams = knex.withUserParam({}); knewWithParams .customMethod())
3. Add documentation in https://github.com/knex/documentation
4. Try adding a test in https://github.com/tgriesser/knex/blob/master/types/test.ts that would assert interface augmentation to work.
5. This one was suggested by @elhigu -> possibility to override existing methods while preserving reference to original methods somehow (I guess those could be reattached to original object using some prefix or container). Not sure if this works correctly for custom methods attached to QueryBuilder, needs a try. I would imagine having something like

function where() {
console.log('log stuff')
this.original.where(...args) // or this.originalWhere(...args)
}

knex.QueryBuilder.extend('where', where)

Thanx, will work on it at the evening.
Regarding point #5, I think that this should be a separate feature, currently it is not allowed to override existing methods.

[kibertoad]

@felixmosh Sounds good, as long as you think that could be introduced later in a non-breaking way.

[elhigu]

Lets mark this support experimental, so it will be ok to break when we have more user experience about how it should work.

[felixmosh]

Agree, how do i mark it as experimental?

[elhigu]

Just by mentioning it in documentation

[kibertoad]

Unfortunately, this needs to be updated to accomodate for /src -> /lib movement of knex files. Should be fairly easy fix, though.

[felixmosh]

No problem, i will fix that at the weekend.

[kibertoad]

Are any assertions possible on existence of this method?

[felixmosh]

I've improved it & added a usage.
:]

[kibertoad]

LGTM!
@elhigu @lorefnon What do you think?

[felixmosh]

A documentation PR knex/documentation#218

[lorefnon]

@lorefnon What do you think?

I am personally not a huge fan of multiple libraries extending shared context, and in general, one import potentially changing the outcome of another import (across files).

I think this will also cause jump to definition to not work (even in a pure typescript codebase - it will just jump to the declaration which would have to be separately defined).

But given that people see value in this, and also because the current knex codebase is already not very statically explorable (because of not using ES6 classes in some places, extensive aliasing etc. features like jump-to-definition, ide outline etc. often don't work), hence I don't have a very strong opinion here.

[kibertoad]

Released in 0.19.1