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

Add queryContext to schema and query builders #2314

Merged
merged 32 commits into from Feb 1, 2018

Conversation

Projects
None yet
2 participants
@joelmukuthu
Contributor

joelmukuthu commented Nov 8, 2017

Closes #2268

Adds a QueryBuilder.prototype.queryContext method, which allows configuring a context to be passed to wrapIdentifier and postProcessResponse per query builder.

Documentation: knex/documentation#68

joelmukuthu added some commits Nov 8, 2017

@@ -53,6 +53,9 @@ assign(Builder.prototype, {
if (!isUndefined(this._options)) {
cloned._options = clone(this._options);
}
if (!isUndefined(this._hookContext)) {
cloned._hookContext = clone(this._hookContext);

This comment has been minimized.

@joelmukuthu

joelmukuthu Nov 8, 2017

Contributor

@elhigu does it make sense to clone the context object as well? I imagine someone would want the same context object to be passed around (to be able to build up the context) but for now I cloned it to conform with the rest

This comment has been minimized.

@elhigu

elhigu Nov 9, 2017

Collaborator

well at least.. you really cannot do deep clone for it, so I would say that it is enough to copy only root element and if someone like some how shared content they just need to add nested structure to context and inner content will be just references to the same data... not sure if that is understandable at all..

This comment has been minimized.

@joelmukuthu

joelmukuthu Nov 10, 2017

Contributor

I understand.. but is there a reason to clone the root context itself?

This comment has been minimized.

@elhigu

elhigu Nov 14, 2017

Collaborator

@joelmukuthu yes. to be able to have different contexts for cloned queries. So I can clone query and then change / merge its root context without modifying the original. So that way it will be more versatile and user can decide how to use it.

This comment has been minimized.

@elhigu

elhigu Nov 14, 2017

Collaborator

Though it will be also caveat that must be documented.

This comment has been minimized.

@joelmukuthu

joelmukuthu Nov 15, 2017

Contributor

Makes perfect sense, thanks for the explanation!

@@ -119,7 +120,11 @@ export default class Formatter {
}
wrapAsIdentifier(value) {
return this.client.wrapIdentifier((value || '').trim());
let hookContext;
if (this.builder && isFunction(this.builder.hookContext)) {

This comment has been minimized.

@joelmukuthu

joelmukuthu Nov 15, 2017

Contributor

@elhigu I'm a bit unsure about these checks (along with this one).. they seem a bit hacky but I had to add them for CI to pass. do you know which cases formatter will be instantiated without a builder?

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Nov 16, 2017

@elhigu / @richraid21 this is ready for review, PTAL. Also, question about the naming of the method: since @elhigu mentioned that the same context could be passed when query events are emitted, should it be called something less specific as hookContext, maybe just context?

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Nov 27, 2017

hey @elhigu, have you had a chance to look at this?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 30, 2017

Sorry, I haven't got any extra time to put to knex last weeks and looks like I wont be having any until January.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 4, 2018

Just FYI, I'm almost back in business and haven't forgotten this, nor the other waiting PRs.

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Jan 4, 2018

@elhigu thanks and no worries. i'll fix up the merge conflicts in the mean time

joelmukuthu added some commits Jan 4, 2018

sqlite3: 'select `users_fancy_wrapper_was_here`.`foo_fancy_wrapper_was_here` as `bar_fancy_wrapper_was_here` from `schema_fancy_wrapper_was_here`.`users_fancy_wrapper_was_here`'
}, clientsWithCustomIdentifierWrapper);
})
})

This comment has been minimized.

@elhigu

elhigu Jan 7, 2018

Collaborator

To make tests a bit more complete should be also tested that

  1. if originalQuery context is changed that context of cloned query will remain intact.
  2. if originalQuery context is { objectInCtx: { value: 1 } } and if value is changed to 2 then in cloned context value also changes (because context is cloned as shallow copy)
return trx
.select()
.from('accounts')
.hookContext({ foo: 'bar' });

This comment has been minimized.

@elhigu

elhigu Jan 7, 2018

Collaborator

I'll ask some second and third opinions about this name :)

This comment has been minimized.

@elhigu

elhigu Jan 7, 2018

Collaborator

maybe queryContext() would be a bit nicer, I'll figure this out tomorrow

This comment has been minimized.

@elhigu

elhigu Jan 9, 2018

Collaborator

I actually didn't see the guy with opinions yesterday, so didn't get one. But I kind of like .queryContext it is not so specific that .hookContext is and it hints that context is bound to single query builder.

This comment has been minimized.

@joelmukuthu

joelmukuthu Jan 9, 2018

Contributor

Agreed, will update :)

This comment has been minimized.

@joelmukuthu

joelmukuthu Jan 9, 2018

Contributor

One thing though, this PR also adds that method to the schema builder. Is it okay to call it queryContext there as well or should be perhaps remove it altogether?

@@ -132,8 +132,12 @@ assign(Runner.prototype, {
return queryPromise
.then((resp) => {
const processedResponse = this.client.processResponse(resp, runner);
let queryContext;
if (this.builder && isFunction(this.builder.queryContext)) {

This comment has been minimized.

@elhigu

elhigu Jan 10, 2018

Collaborator

Is there some case when this.builder.queryContext is not a Function? Or when this.builder is not declared? If those checks are here only because some tests are not initialized with builder, then tests should be changed instead (for example by mocking builder).

This comment has been minimized.

@joelmukuthu

joelmukuthu Jan 10, 2018

Contributor

Yeah there were some failing tests and I assumed it meant that some code would also fail. I'll remove the checks and go about fixing those tests :)

@@ -53,7 +53,7 @@ Client_Oracledb.prototype.columnCompiler = function() {
return new ColumnCompiler(this, ...arguments)
}
Client_Oracledb.prototype.formatter = function() {
return new Oracledb_Formatter(this)
return new Oracledb_Formatter(this, ...arguments)

This comment has been minimized.

@elhigu

elhigu Jan 10, 2018

Collaborator

Shouldn't all these be

= function(builder) {
   return new Oracledb_Formatter(this, builder);
}

Thats more understandable for anyone reading the code.

This comment has been minimized.

@joelmukuthu

joelmukuthu Jan 10, 2018

Contributor

I had it like that at first but then I noticed that the convention was passing all the arguments, for example on line 53 there:

return new ColumnCompiler(this, ...arguments)

Or is that because those are the generic compiler and transaction classes?

I agree though that it's more readable..

This comment has been minimized.

@elhigu

elhigu Jan 10, 2018

Collaborator

I suppose it has just been personal preference of the one who originally implemented oracledb dialect.

This comment has been minimized.

@joelmukuthu

joelmukuthu Jan 10, 2018

Contributor

Hmm.. but then it's also the same on the mssql dialect. I don't mean to prod I'm just a sucker for conventions :)

This comment has been minimized.

@elhigu

elhigu Jan 10, 2018

Collaborator

Both dialects are pretty new ones and might have been used as a reference for each other. But sure you are right, since that syntax is used everywhere in those dialects, there is no reason why it couldn't be used also in this PR. So no need to change that.

@elhigu

Hi, I just finished checking the code. This looks already great. There was only 2 more comments that could be improved / were a bit unclear to me.

Other that that documentation PR is needed to knex/docuementation repo. In docs it would be good to mention also all the places where context is passed when query is ran.

minor: use more descriptive variable name
i.e. refactor: `context` => `queryContext`
@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Jan 10, 2018

I fixed all the cases of the missing builder instance for the formatter, and ended up adding queryContext to QueryBuilder, Raw, SchemaBuilder, TableBuilder and TableCompiler.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 11, 2018

queryContext to QueryBuilder, Raw, SchemaBuilder, TableBuilder and TableCompiler

These new places, where it is available will also need tests and documentation. Other than that PR still looks good. Also looks like all tests broke.

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Jan 15, 2018

I've tried to add tests for the schema, table and column builders and found some weird cases where the context needed to be passed down from SchemaCompiler => TableCompiler => ColumnCompiler. Hoping for a passing build then will update the docs tomorrow.

@joelmukuthu joelmukuthu changed the title from Add hookContext to query builder to Add queryContext to schema and query builders Jan 16, 2018

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Jan 17, 2018

I added the tests and updated the docs. PTAL :)

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Jan 31, 2018

Hi @elhigu. This has been ready for review for a while now, is there anything I can do to help out with the reviewing process?

@elhigu

elhigu approved these changes Feb 1, 2018

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 1, 2018

Thanks for the patience with this. Those tests were really great, I'm really happy to merge this in! 👍

@elhigu elhigu merged commit bf1fa63 into tgriesser:master Feb 1, 2018

1 check passed

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

This comment has been minimized.

Contributor

joelmukuthu commented Feb 2, 2018

Great, thanks! And yes the tests were awesome, helped me find so many hidden bugs

@joelmukuthu joelmukuthu deleted the joelmukuthu:feat/hook-context branch Feb 5, 2018

joelmukuthu added a commit to joelmukuthu/knex that referenced this pull request Feb 16, 2018

Add queryContext to schema and query builders (tgriesser#2314)
* feat(query-builder): add hookContext for wrapIdentifier

* refactor: use isUndefined

* test(transaction): test passing of hookContext

* feat(runnner): pass context to postProcessResponse

* test(runner): test postProcessResponse for raw responses

* test(raw): test passing of hookContext

* feat: add hookContext to Raw and SchemaBuilder

* test(transaction): fix test for hookContext

* chore: fix lint error

* fix: check for hookContext before calling it

* test(transaction): fix hookContext test

* chore: remove whitespace

* test(hookContext): test cloning of context object

* refactor: hookContext -> queryContext

* minor: use more descriptive variable name

i.e. refactor: `context` => `queryContext`

* fix: remove unnecessary checks for query builder

* fix(Raw): pass query builder to formatter

* fix(SchemaCompiler): pass schema builder to formatter

* refactor: add addQueryContext helper

* feat: add queryContext to TableBuilder and ColumnBuilder

* fix(TableCompiler): pass table builder to formatter

* fix(ColumnCompiler): pass column builder to formatter

* fix(pushQuery): fix passing builder to formatter

* test(Schema|Table|ColumnCompiler): test passing queryContext

* fix(SchemaCompiler): pass queryContext to TableCompiler

* fix(TableCompiler): pass queryContext to ColumnCompiler

* test: add queryContext tests for all schema dialects

* test(TableCompiler): test overwriting queryContext from SchemaCompiler

* test(Raw): test passing queryContext to wrapIdentifier

* tests: run all the tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment