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

Add queryContext to schema and query builders #2314

Merged
merged 32 commits into from Feb 1, 2018

Conversation

@joelmukuthu
Copy link
Contributor

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

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

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
Author Contributor

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

This comment has been minimized.

@elhigu

elhigu Nov 14, 2017
Member

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

Though it will be also caveat that must be documented.

This comment has been minimized.

@joelmukuthu

joelmukuthu Nov 15, 2017
Author Contributor

Makes perfect sense, thanks for the explanation!

@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from d433fbe to 486e637 Nov 15, 2017
@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from 569bf47 to d2266e4 Nov 15, 2017
@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from a888bb5 to 2ce2ce3 Nov 15, 2017
@@ -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
Author 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
Copy link
Contributor Author

@joelmukuthu 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
Copy link
Contributor Author

@joelmukuthu joelmukuthu commented Nov 27, 2017

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

@elhigu
Copy link
Member

@elhigu 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
Copy link
Member

@elhigu elhigu commented Jan 4, 2018

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

@joelmukuthu
Copy link
Contributor Author

@joelmukuthu joelmukuthu commented Jan 4, 2018

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

@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from 9c3d65b to fdc2dfe 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
Member

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
Member

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

This comment has been minimized.

@elhigu

elhigu Jan 7, 2018
Member

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

This comment has been minimized.

@elhigu

elhigu Jan 9, 2018
Member

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
Author Contributor

Agreed, will update :)

This comment has been minimized.

@joelmukuthu

joelmukuthu Jan 9, 2018
Author 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?

@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from 194ca09 to be317bc Jan 10, 2018
@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from 3af6a99 to 1feb6f5 Jan 10, 2018
@joelmukuthu
Copy link
Contributor Author

@joelmukuthu 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
Copy link
Member

@elhigu 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
Copy link
Contributor Author

@joelmukuthu 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 Add hookContext to query builder Add queryContext to schema and query builders Jan 16, 2018
@joelmukuthu joelmukuthu force-pushed the joelmukuthu:feat/hook-context branch from 3df3583 to 09be564 Jan 17, 2018
@joelmukuthu
Copy link
Contributor Author

@joelmukuthu joelmukuthu commented Jan 17, 2018

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

@joelmukuthu
Copy link
Contributor Author

@joelmukuthu 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
Copy link
Member

@elhigu 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 knex:master Feb 1, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joelmukuthu
Copy link
Contributor Author

@joelmukuthu 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
* 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants