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

Ensure that 'client' is provided in knex config object #1822

Merged
merged 3 commits into from Dec 11, 2016

Conversation

Projects
None yet
5 participants
@wubzz
Collaborator

wubzz commented Dec 8, 2016

The option is documented as required, but it's currently not being validated. This leads to strange side effects such as ones describe in #1628.

I also took the liberty of adding modifiers to the base columpcompiler as it currently does not have this property, but it should.

@wubzz wubzz requested a review from elhigu Dec 8, 2016

@elhigu elhigu merged commit a4bfee7 into tgriesser:master Dec 11, 2016

1 check passed

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

abegines added a commit to abegines/knex that referenced this pull request Dec 12, 2016

Merge pull request #1 from tgriesser/master
Ensure that 'client' is provided in knex config object (tgriesser#1822)

elhigu added a commit to elhigu/knex that referenced this pull request Feb 15, 2017

Ensure that 'client' is provided in knex config object (tgriesser#1822)
* Ensure that 'client' is provided in knex config object

* sqlite3 columncompiler: Assign 'modifiers' --after-- super() call.

* oracle columncompiler: Assign 'modifiers' --after-- super() call.
@eddieajau

This comment has been minimized.

eddieajau commented Feb 20, 2017

There is a small regression with this. Previously when calling knex migrate:make the config option was not required. Now knex migrate:make errors because Knex is not correctly configured in the local development environment (but would be configured correctly when knex was actually run in a production environment).

@venables

This comment has been minimized.

venables commented Feb 20, 2017

I disagree with throwing the exception here. One of the documented features of Knex is:

You can even use knex without a connection, just for its query building features. Just pass in an empty object when initializing the library. Specify a client if you are interested in a particular flavour of SQL.
var knex = require('knex')({});
knex('table').insert({a: 'b'}).returning('*').toString();
// "insert into "table" ("a") values ('b')"

I think we should revert the Exception bit, and treat the bug as fixed by the PR by adding this.modifiers = [].

venables added a commit to venables/knex that referenced this pull request Feb 20, 2017

Revert "Ensure that 'client' is provided in knex config object (tgrie…
…sser#1822)"

This reverts some of commit a4bfee7.

It leaves the fixes to `this.modifiers` for the base schema
columncompiler, the oracle columncompiler, and the sqlite3
columncompiler.
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2017

Looks like this one actually was a breaking change, since there kind of was documented. I'm fine with reverting and releasing fixed version as 0.12.8 with explication in changelog.

I don't like the feature of initializing builder without dialect, but it clearly was intended to work. @wubzz does it sound ok to you? or was there actually documentation somewhere that client was required configuration parameter?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2017

Actually yeah... other issue actually did state the place where it was mentioned.

Initializing the Library

The knex module is itself a function which takes a configuration object for Knex, accepting a few parameters. The client parameter is required and determines which client adapter will be used with the library.

So I think we should remove the invalid part of the documentation. I would prefer to have client as required, because currently trying to use knex without client is very badly tested and probably will keep on breaking in the future.

But since there was inconsistent documentation about the feature this should have been made with minor version bump instead of patch version...

@venables

This comment has been minimized.

venables commented Feb 20, 2017

Thanks @elhigu - I have found the "client-less" initializer to have been helpful over the years, but I understand your concerns.

I am happy to help write better test coverage for the feature if that's desired.

Another option would be to allow setting the client like client: "none" -- forcing it to be explicitly called out could help prevent user errors and confusion (which seems to be the goal here, right?)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2017

@venables I still don't understand why clientless initialization is more helpful that for example just using { client: 'pg' } initialization? I understand initializing without connection to be able to test queries, but without client the generated SQL is really not runnable anywhere...

Anyways if that feature is really wanted and better tested in that case { client : "none" } sounds like a good way to initialize the clientless builder.

@venables

This comment has been minimized.

venables commented Feb 20, 2017

I won't have all the use-cases for clientless initialization, but the use-cases that I've hit professionally have been:

  1. Unit tests. We use this feature heavily in our tests. The knex project uses/used it in tests as well.
  2. Needing to define a Bookshelf model before the database connection is established. We Initialized Bookshelf with a client-less Knex instance and swapped the Knex instance once Knex was connected.

All things we can work around, but I think it is a really great feature -- even if it were hid behind some sort of advanced none setting which uses the base schema class.

This is a feature that Knex was advertising as a good thing, and I agree.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 21, 2017

@elhigu I still stand firm that 'client' should always be required. It's unfortunate that the documentation has double standards. The use cases above don't sound too convincing to me. One could just as easily supply client in unit tests and the second use case sounds like a 'cheap trick' (no offense) that the library should support in a different way.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 21, 2017

@venables In both of those use cases one can initialize { client: 'pg' } or { client: 'sqlite3' }. Only thing which just will be inconvenient is that unit test result checking might need to be fixed a little. However { client: 'pg' } query generation is very near to current clientless output.

I still have not seen any case where { client: 'none' } would be any better choice that { client: 'pg' } or any other real client without giving connection details for query unit testing.

You mentioned about using the base schema class... indeed most of the dialect differences are in schema. Is there any specific reasons why using base schema would be desired instead of some real schema of real dialect?

For now I feel that the best choice would be to revert throwing the error, update changelogs and release 0.12.8.

Then reapply the fix and release 0.13.0 and mark it as a breaking change and fix the documentation.

@venables

This comment has been minimized.

venables commented Feb 21, 2017

@wubzz @elhigu Thanks for the response. The unit test use-case was my primary concern, but it looks like the database connection pool is not actually created until a query is executed -- which might be another piece of confusion from the documentation:

Initializing the library should normally only ever happen once in your application, as it creates a connection pool for the current database, you should use the instance returned from the initialize call throughout your library.

So I think we should be all set by setting our tests to client: 'pg' like you suggest, since the database connection would not be established upon initialization.

Thank you again for looking into this.

I'll close the Revert PR -- I'll leave the decision to you guys about 13.0 vs 12.8

@TimonLukas

This comment has been minimized.

TimonLukas commented Feb 22, 2017

My use case for this was to use Knex with IBMs DB2. Without specifying a client it actually generated valid queries, which the other client types do not seem to do. Oh well, I just resorted to manually writing my queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment