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

Conversation

@felixmosh
Copy link
Contributor

commented Jul 6, 2019

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

@felixmosh felixmosh changed the title Feat: Allow to extend knex query builder Feat: Allow to extend knex query builder - closes #1158 Jul 6, 2019

@felixmosh felixmosh changed the title Feat: Allow to extend knex query builder - closes #1158 Feat: Allow to extend knex query builder Jul 6, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jul 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

Related issues:
#2071
#1579
#1456
#1158

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

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?

});

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

@felixmosh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

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

done();
});
});

This comment has been minimized.

Copy link
@elhigu

elhigu Jul 8, 2019

Collaborator

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
@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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

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

@felixmosh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

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

@felixmosh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Agree, how do i mark it as experimental?

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

Just by mentioning it in documentation

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

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

@felixmosh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

No problem, i will fix that at the weekend.

@felixmosh felixmosh force-pushed the felixmosh:extend-query-builder branch from 283e355 to c35b58e Jul 13, 2019

@@ -4,6 +4,12 @@ import * as Knex from 'knex';
// import Knex from 'knex'
// when "esModuleInterop": true

declare module 'knex' {
interface QueryBuilder {
onDuplicateUpdate(...columnNames: string[]): QueryBuilder;

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 13, 2019

Collaborator

Are any assertions possible on existence of this method?

This comment has been minimized.

Copy link
@felixmosh

felixmosh Jul 13, 2019

Author Contributor

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 13, 2019

LGTM!
@elhigu @lorefnon What do you think?

@felixmosh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

A documentation PR knex/documentation#218

@lorefnon

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

@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 kibertoad merged commit 8111bb3 into tgriesser:master Jul 23, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 88.249%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

Released in 0.19.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.