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

Optionally use DEFAULT instead of NULL when translating undefined to SQL value. #1043

Merged
merged 3 commits into from Nov 23, 2015

Conversation

Projects
None yet
3 participants
@elhigu
Collaborator

elhigu commented Nov 4, 2015

Fixes #662 by adding new configuration option replaceUndefinedWithDefault which can be given to all other clients except sqlitewhich does not support DEFAULT.

Change is backwards compatible, since default behavior is not changed, so it should be possible to add to 0.9.1.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 13, 2015

@rhys-vdw @bendrucker @tgriesser Any comments for this pull request? Anything I can do to get it merged? I have also two more waiting for review :) Looks like @rhys-vdw has been only one reviewing and merging new stuff in lately... is there any other people administrating currently? I appreciate the work you've been putting to knex 👍

@chrisbroome

This comment has been minimized.

chrisbroome commented Nov 13, 2015

FWIW, I really really really like this idea. undefined being converted to SQL NULL surprised me quite a bit when I first started using the library, so I hope it gets merged soon, also. 😄

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 14, 2015

Hey guys, sorry for the unresponsiveness. Yes, it does seem that I'm the only one processing PRs for the most part, presently. I'd be happy to have a hand if either of you are interested.

Thoughts: I see no value in ever assuming that a column's default value is NULL. I'd like this to be the default behaviour, and simply use this everywhere except sqlite.

It seems unlikely that anybody would be relying on this behaviour, so it should be fairly safe to make change. If you were relying on undefined being NULL then your column's default is almost certainly going to be NULL anyway.

To me it makes more sense to throw a TypeError when trying to insert undefined in sqlite. undefined it not the same as NULL, so this is still a bad assumption here.

The config flag could then instead be useNullAsDefault (or similar) instead. Here, instead of throwing we could print a warning such as "Warning: sqlite does not support inserting default values, set the useNullAsDefault flag explicitly" if the option has not been set.

var sqlite = Knex({ client: sqlite ... });

// warning: sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs <hyperlink>).

sqlite('table').insert([{ a: 'a' }, { b: 'b' }])
  .then(...)
  .catch(error => {
    // TypeError: `sqlite` does not support inserting default values. Specify values explicitly or use the `useNullAsDefault` config flag. (see docs <hyperlink>).
  });

Does that makes sense?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 14, 2015

@rhys-vdw I agree completely. Having NULL as default behavior makes no sense. Only reason I didn't change it was to keep pull request backwards compatible.

I'm happy to make replacement for undefined to be DEFAULT and throw error on sqlite. I'll update this pull request a bit later.

About reviewing pull requests I'm happy to help. I can't make any promises how much I have time, but at least something :) Is there some process I should follow / prioritisation between PQ:s to check?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 14, 2015

@rhys-vdw I added one more commit there which changes default behavior to use DEFAULT, adds error and warning messages for sqlite and adds one test to check that trying to use undefined => DEFAULT actually throws an error on sqlite.

elhigu added a commit that referenced this pull request Nov 23, 2015

Merge pull request #1043 from elhigu/allow-using-default-instead-of-n…
…ull-when-translating-undefined-to-sql

Optionally use DEFAULT instead of NULL when translating undefined to SQL value.

@elhigu elhigu merged commit b3452c5 into tgriesser:master Nov 23, 2015

1 check passed

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

@elhigu elhigu deleted the elhigu:allow-using-default-instead-of-null-when-translating-undefined-to-sql branch Nov 23, 2015

@@ -18,6 +19,9 @@ var SQLite3_DDL = require('./schema/ddl')
function Client_SQLite3(config) {
Client.call(this, config)
if (!config.useNullAsDefault) {

This comment has been minimized.

@rhys-vdw

rhys-vdw Nov 24, 2015

Collaborator

Hey, just looking at this (post merge, sorry). I think here it should be if (_.isUndefined(config.useNullAsDefault)). false is a valid option if user prefers to raise an error, but does not wish to see the warning.

@elhigu

This comment has been minimized.

@elhigu

elhigu Nov 24, 2015

Collaborator

Kind of good idea, but I can't think of any real world situation when one would like to do it.

I mean that if one decides to use sqlite instead of some other db, it requires modifying configuration at least to set config.client. Then one decides that I don't want startup warning, but I still would like that knex throws an error if I try to add multiple columns with partly undefined keys.

That said @rhys-vdw let me know what you think and I'll add another pull request to allow suppress warning by setting config.useNullAsDefault = false if it is wanted.

This comment has been minimized.

@rhys-vdw

rhys-vdw Nov 25, 2015

Collaborator

Users may wish to get an error when client code fails to properly format the input data for sqlite. I think it's better to give the option. Also the warning wording suggests that setting it to false would suppress the warning.

I'll add another pull request to allow suppress warning by setting config.useNullAsDefault = false if it is wanted.

That would be great, thanks. :)

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