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

[elhigu]

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]

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

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]

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]

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

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

[rhys-vdw]

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

[elhigu]

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.

[rhys-vdw]

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. :)