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

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion index.html
Expand Up @@ -1154,10 +1154,31 @@ <h3>Join Methods</h3>
knex('coords').insert([{x: 20}, {y: 30}, {x: 10, y: 20}])
</pre>


<pre class="display">
// Returns [2] in "mysql", "sqlite"; [2, 3] in "postgresql"
knex.insert([{title: 'Great Gatsby'}, {title: 'Fahrenheit 451'}], 'id').into('books')
</pre>

<p>
If one prefers that undefined keys are replaced with <tt>NULL</tt> instead of <tt>DEFAULT</tt>
one may give <tt>useNullAsDefault</tt> configuration parameter in knex config.
</p>

<pre>
<code class="js">
var knex = require('knex')({
client: 'mysql',
connection: {
host : '127.0.0.1',
user : 'your_database_user',
password : 'your_database_password',
database : 'myapp_test'
},
useNullAsDefault: true
});
knex('coords').insert([{x: 20}, {y: 30}, {x: 10, y: 20}])
// insert into `coords` (`x`, `y`) values (20, NULL), (NULL, 30), (10, 20)
</code>
</pre>

<p id="Builder-returning">
Expand Down
11 changes: 11 additions & 0 deletions src/client.js
@@ -1,4 +1,5 @@

var _ = require('lodash')
var Promise = require('./promise')
var helpers = require('./helpers')

Expand Down Expand Up @@ -39,6 +40,10 @@ function Client(config = {}) {
this.initializePool(config)
}
}
this.valueForUndefined = this.raw('DEFAULT');
if (config.useNullAsDefault) {
this.valueForUndefined = null
}
}
inherits(Client, EventEmitter)

Expand Down Expand Up @@ -134,6 +139,12 @@ assign(Client.prototype, {
return this._stream.call(this, connection, obj, stream, options)
},

prepBindings: function(bindings) {
return _.map(bindings, function(binding) {
return binding === undefined ? this.valueForUndefined : binding
}, this);
},

wrapIdentifier: function(value) {
return (value !== '*' ? '"' + value.replace(/"/g, '""') + '"' : '*')
},
Expand Down
3 changes: 3 additions & 0 deletions src/dialects/oracle/index.js
Expand Up @@ -64,6 +64,9 @@ assign(Client_Oracle.prototype, {
else if (Buffer.isBuffer(value)) {
return SqlString.bufferToString(value)
}
else if (value === undefined) {
return this.valueForUndefined
}
return value
}, this)
},
Expand Down
4 changes: 2 additions & 2 deletions src/dialects/postgres/index.js
Expand Up @@ -54,8 +54,8 @@ assign(Client_PG.prototype, {
// Prep the bindings as needed by PostgreSQL.
prepBindings: function(bindings, tz) {
return _.map(bindings, function(binding) {
return utils.prepareValue(binding, tz)
});
return utils.prepareValue(binding, tz, this.valueForUndefined)
}, this);
},

// Get a raw connection, called by the `pool` whenever a new
Expand Down
7 changes: 5 additions & 2 deletions src/dialects/postgres/utils.js
Expand Up @@ -34,7 +34,7 @@ var arrayString;
// to their 'raw' counterparts for use as a postgres parameter
// note: you can override this function to provide your own conversion mechanism
// for complex types, etc...
var prepareValue = function (val, seen) {
var prepareValue = function (val, seen, valueForUndefined) {
if (val instanceof Buffer) {
return val;
}
Expand All @@ -44,9 +44,12 @@ var prepareValue = function (val, seen) {
if (Array.isArray(val)) {
return arrayString(val);
}
if (val === null || val === undefined) {
if (val === null) {
return null;
}
if (val === undefined) {
return valueForUndefined;
}
if (typeof val === 'object') {
return prepareObject(val, seen);
}
Expand Down
14 changes: 14 additions & 0 deletions src/dialects/sqlite3/index.js
@@ -1,6 +1,7 @@

// SQLite3
// -------
var _ = require('lodash')
var Promise = require('../../promise')

var inherits = require('inherits')
Expand All @@ -18,6 +19,9 @@ var SQLite3_DDL = require('./schema/ddl')

function Client_SQLite3(config) {
Client.call(this, config)
if (!config.useNullAsDefault) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

helpers.warn('sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs http://knexjs.org/#Builder-insert).');
}
}
inherits(Client_SQLite3, Client)

Expand Down Expand Up @@ -108,6 +112,16 @@ assign(Client_SQLite3.prototype, {
})
},

prepBindings: function(bindings) {
return _.map(bindings, function(binding) {
if (binding === undefined && this.valueForUndefined !== null) {
throw new TypeError("`sqlite` does not support inserting default values. Specify values explicitly or use the `useNullAsDefault` config flag. (see docs http://knexjs.org/#Builder-insert).");
} else {
return binding
}
}, this);
},

// Ensures the response is returned in the same format as other clients.
processResponse: function(obj, runner) {
var ctx = obj.context;
Expand Down
10 changes: 9 additions & 1 deletion src/query/string.js
Expand Up @@ -4,7 +4,11 @@ var SqlString = exports;
var helpers = require('../helpers')

SqlString.escape = function(val, timeZone) {
if (val == null) {
// Cant do require on top of file beacuse Raw is not yet initialized when this file is
// executed for the first time
var Raw = require('../raw')

if (val === null || val === undefined) {
return 'NULL';
}

Expand All @@ -25,6 +29,10 @@ SqlString.escape = function(val, timeZone) {
return SqlString.arrayToList(val, timeZone);
}

if (val instanceof Raw) {
return val;
}

if (typeof val === 'object') {
try {
val = JSON.stringify(val)
Expand Down
42 changes: 38 additions & 4 deletions test/unit/query/builder.js
Expand Up @@ -16,6 +16,15 @@ var clients = {
default: new Client({})
}

var useNullAsDefaultConfig = { useNullAsDefault: true };
var clientsWithNullAsDefault = {
mysql: new MySQL_Client(useNullAsDefaultConfig),
postgres: new PG_Client(useNullAsDefaultConfig),
oracle: new Oracle_Client(useNullAsDefaultConfig),
sqlite3: new SQLite3_Client(useNullAsDefaultConfig),
default: new Client(useNullAsDefaultConfig)
}

function qb() {
return clients.default.queryBuilder()
}
Expand All @@ -39,10 +48,11 @@ function verifySqlResult(dialect, expectedObj, sqlObj) {
});
}

function testsql(chain, valuesToCheck) {
function testsql(chain, valuesToCheck, selectedClients) {
selectedClients = selectedClients || clients;
Object.keys(valuesToCheck).forEach(function(key) {
var newChain = chain.clone()
newChain.client = clients[key]
newChain.client = selectedClients[key]
var sqlAndBindings = newChain.toSQL()

var checkValue = valuesToCheck[key]
Expand All @@ -54,10 +64,11 @@ function testsql(chain, valuesToCheck) {
})
}

function testquery(chain, valuesToCheck) {
function testquery(chain, valuesToCheck, selectedClients) {
selectedClients = selectedClients || clients;
Object.keys(valuesToCheck).forEach(function(key) {
var newChain = chain.clone()
newChain.client = clients[key]
newChain.client = selectedClients[key]
var sqlString = newChain.toQuery()
var checkValue = valuesToCheck[key]
expect(checkValue).to.equal(sqlString)
Expand Down Expand Up @@ -1398,6 +1409,29 @@ describe("QueryBuilder", function() {
});
});

it("multiple inserts with partly undefined keys client with configuration nullAsDefault: true", function() {
testquery(qb().from('users').insert([{email: 'foo', name: 'taylor'}, {name: 'dayle'}]), {
mysql: "insert into `users` (`email`, `name`) values ('foo', 'taylor'), (NULL, 'dayle')",
sqlite3: 'insert into "users" ("email", "name") select \'foo\' as "email", \'taylor\' as "name" union all select NULL as "email", \'dayle\' as "name"',
oracle: 'begin execute immediate \'insert into "users" ("email", "name") values (:1, :2)\' using \'foo\', \'taylor\'; execute immediate \'insert into "users" ("email", "name") values (:1, :2)\' using NULL, \'dayle\';end;',
default: 'insert into "users" ("email", "name") values (\'foo\', \'taylor\'), (NULL, \'dayle\')'
}, clientsWithNullAsDefault);
});

it("multiple inserts with partly undefined keys", function() {
testquery(qb().from('users').insert([{email: 'foo', name: 'taylor'}, {name: 'dayle'}]), {
mysql: "insert into `users` (`email`, `name`) values ('foo', 'taylor'), (DEFAULT, 'dayle')",
oracle: 'begin execute immediate \'insert into "users" ("email", "name") values (:1, :2)\' using \'foo\', \'taylor\'; execute immediate \'insert into "users" ("email", "name") values (:1, :2)\' using DEFAULT, \'dayle\';end;',
default: 'insert into "users" ("email", "name") values (\'foo\', \'taylor\'), (DEFAULT, \'dayle\')'
});
});

it("multiple inserts with partly undefined keys throw error with sqlite", function() {
expect(function () {
testquery(qb().from('users').insert([{email: 'foo', name: 'taylor'}, {name: 'dayle'}]), { sqlite3: "" });
}).to.throw(TypeError)
});

it("multiple inserts with returning", function() {
// returning only supported directly by postgres and with workaround with oracle
// other databases implicitly return the inserted id
Expand Down