From c5061687b1317ee98bfb5f95eadeb946156e103b Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Fri, 4 Dec 2015 17:58:43 +1300 Subject: [PATCH 1/6] hasOne() associations need to work when the FK value is 0. Pre-existing databases may've used zero as a valid entity identifier --- lib/Utilities.js | 5 +- test/integration/association-hasone-zeroid.js | 151 ++++++++++++++++++ 2 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 test/integration/association-hasone-zeroid.js diff --git a/lib/Utilities.js b/lib/Utilities.js index 04040a6d..f5164b90 100644 --- a/lib/Utilities.js +++ b/lib/Utilities.js @@ -131,9 +131,12 @@ exports.values = function (obj, keys) { return vals; }; +// Qn: is Zero a valid value for a FK column? +// Why? Well I've got a pre-existing database that started all its 'serial' IDs at zero... +// Answer: hasValues() is only used in hasOne association, so it's probably ok... exports.hasValues = function (obj, keys) { for (var i = 0; i < keys.length; i++) { - if (!obj[keys[i]]) return false; + if (!obj[keys[i]] && obj[keys[i]] !== 0) return false; // 0 is also a good value... } return true; }; diff --git a/test/integration/association-hasone-zeroid.js b/test/integration/association-hasone-zeroid.js new file mode 100644 index 00000000..06f5dbbb --- /dev/null +++ b/test/integration/association-hasone-zeroid.js @@ -0,0 +1,151 @@ +var ORM = require('../../'); +var helper = require('../support/spec_helper'); +var should = require('should'); +var async = require('async'); +var _ = require('lodash'); + +describe("hasOne", function() { + var db = null; + var Person = null; + var Pet = null; + + var setup = function (autoFetch) { + return function (done) { + db.settings.set('instance.cache', false); + db.settings.set('instance.returnAllErrors', true); + + Person = db.define('person', { + id : {type : "integer", mapsTo: "personID", key:true}, + firstName : {type : "text", size:"255"}, + lastName : {type : "text", size:"255"} + }); + + Pet = db.define('pet', { + id : {type : "integer", mapsTo:"petID", key:true}, + petName : {type : "text", size:"255"}, + ownerID : {type : "integer", size:"4"} + }); + + Pet.hasOne('owner', Person, {field: 'ownerID', autoFetch:autoFetch}); + + helper.dropSync([Person, Pet], function(err) { + if (err) return done(err); + Pet.create([{ + id: 10, + petName: 'Muttley', + owner: { + id: 12, + firstName: 'Stuey', + lastName: 'McG' + } + }, + { + id: 11, + petName: 'Snagglepuss', + owner: { + id: 0, + firstName: 'John', + lastName: 'Doe' + } + }], done); + }); + }; + }; + + before(function(done) { + helper.connect(function (connection) { + db = connection; + done(); + }); + }); + + describe("auto fetch", function () { + before(setup(true)); + + it("should work for non-zero ownerID ", function (done) { + Pet.find({petName: "Muttley"}, function(err, pets) { + should.not.exist(err); + + pets[0].petName.should.equal("Muttley"); + pets[0].should.have.property("id"); + pets[0].id.should.equal(10); + pets[0].ownerID.should.equal(12); + + pets[0].should.have.property("owner"); + pets[0].owner.firstName.should.equal("Stuey"); + + return done(); + }); + }); + + it("should work for zero ownerID ", function (done) { + Pet.find({petName: "Snagglepuss"}, function(err, pets) { + should.not.exist(err); + + pets[0].petName.should.equal("Snagglepuss"); + pets[0].should.have.property("id"); + pets[0].id.should.equal(11); + pets[0].ownerID.should.equal(0); + + pets[0].should.have.property("owner"); + pets[0].owner.firstName.should.equal("John"); + + return done(); + }); + }); + }); + + describe("no auto fetch", function () { + before(setup(false)); + + it("should work for non-zero ownerID ", function (done) { + Pet.find({petName: "Muttley"}, function(err, pets) { + should.not.exist(err); + + pets[0].petName.should.equal("Muttley"); + pets[0].should.have.property("id"); + pets[0].id.should.equal(10); + pets[0].ownerID.should.equal(12); + + pets[0].should.not.have.property("owner"); + + // But we should be able to see if its there + pets[0].hasOwner(function(result) { + should.equal(result, true); + }); + + // ...and then get it + pets[0].getOwner(function(result) { + result.firstName.should.equal("Stuey"); + }); + + return done(); + }); + }); + + it("should work for zero ownerID ", function (done) { + Pet.find({petName: "Snagglepuss"}, function(err, pets) { + should.not.exist(err); + + pets[0].petName.should.equal("Snagglepuss"); + pets[0].should.have.property("id"); + pets[0].id.should.equal(11); + pets[0].ownerID.should.equal(0); + + pets[0].should.not.have.property("owner"); + + // But we should be able to see if its there + pets[0].hasOwner(function(result) { + should.equal(result, true); + }); + + // ...and then get it + pets[0].getOwner(function(result) { + result.firstName.should.equal("John"); + }); + + return done(); + }); + }); + }); +}); From df04024cf3b718d30dfcbabc43a38790aa3a12f5 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Fri, 4 Dec 2015 20:45:58 +1300 Subject: [PATCH 2/6] hasOne() associations need to work when the FK value is 0. Pre-existing databases may've used zero as a valid entity identifier. --- test/integration/association-hasone-zeroid.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/association-hasone-zeroid.js b/test/integration/association-hasone-zeroid.js index 06f5dbbb..d1bd3637 100644 --- a/test/integration/association-hasone-zeroid.js +++ b/test/integration/association-hasone-zeroid.js @@ -110,12 +110,12 @@ describe("hasOne", function() { pets[0].should.not.have.property("owner"); // But we should be able to see if its there - pets[0].hasOwner(function(result) { + pets[0].hasOwner(function(err, result) { should.equal(result, true); }); // ...and then get it - pets[0].getOwner(function(result) { + pets[0].getOwner(function(err, result) { result.firstName.should.equal("Stuey"); }); @@ -135,12 +135,12 @@ describe("hasOne", function() { pets[0].should.not.have.property("owner"); // But we should be able to see if its there - pets[0].hasOwner(function(result) { + pets[0].hasOwner(function(err, result) { should.equal(result, true); }); // ...and then get it - pets[0].getOwner(function(result) { + pets[0].getOwner(function(err, result) { result.firstName.should.equal("John"); }); From 80ff972be9aa5e7017bcbc3c1860fb17c07e0144 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Mon, 7 Dec 2015 15:36:22 +1300 Subject: [PATCH 3/6] Zero is a valid value for an ID column, don't coerce it to NULL --- lib/Drivers/DML/postgres.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/Drivers/DML/postgres.js b/lib/Drivers/DML/postgres.js index 49748411..03b9c713 100644 --- a/lib/Drivers/DML/postgres.js +++ b/lib/Drivers/DML/postgres.js @@ -213,11 +213,10 @@ Driver.prototype.insert = function (table, data, keyProperties, cb) { if (keyProperties) { for (i = 0; i < keyProperties.length; i++) { prop = keyProperties[i]; - - ids[prop.name] = results[0][prop.mapsTo] || null; + // Zero is a valid value for an ID column + ids[prop.name] = results[0][prop.mapsTo] !== undefined ? results[0][prop.mapsTo] : null; } } - return cb(null, ids); }); }; From 6e331b44f174a242806bfecbefc495b518c6862e Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Mon, 7 Dec 2015 15:48:54 +1300 Subject: [PATCH 4/6] Zero is a valid value for an ID column, don't coerce it to NULL --- lib/Drivers/DML/sqlite.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Drivers/DML/sqlite.js b/lib/Drivers/DML/sqlite.js index ec7ec19d..e0474a3f 100644 --- a/lib/Drivers/DML/sqlite.js +++ b/lib/Drivers/DML/sqlite.js @@ -175,7 +175,8 @@ Driver.prototype.insert = function (table, data, keyProperties, cb) { } else { for (i = 0; i < keyProperties.length; i++) { prop = keyProperties[i]; - ids[prop.name] = data[prop.mapsTo] || null; + // Zero is a valid value for an ID column + ids[prop.name] = data[prop.mapsTo] !== undefined ? data[prop.mapsTo] : null; } return cb(null, ids); } From bb58e477c7defd339e72d1b435ee281c6754395b Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Mon, 7 Dec 2015 16:05:51 +1300 Subject: [PATCH 5/6] Zero is a valid value for an ID column, don't coerce it to NULL --- lib/Drivers/DML/redshift.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Drivers/DML/redshift.js b/lib/Drivers/DML/redshift.js index f984d5de..6b84974d 100644 --- a/lib/Drivers/DML/redshift.js +++ b/lib/Drivers/DML/redshift.js @@ -34,7 +34,8 @@ Driver.prototype.insert = function (table, data, keyProperties, cb) { } else { for(i = 0; i < keyProperties.length; i++) { prop = keyProperties[i]; - ids[prop.name] = data[prop.mapsTo] || null; + // Zero is a valid value for an ID column + ids[prop.name] = data[prop.mapsTo] !== undefined ? data[prop.mapsTo] : null; } return cb(null, ids); From 715c0295955e24fbf93d422e9e6ab14cf10e0824 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Tue, 8 Dec 2015 13:39:23 +1300 Subject: [PATCH 6/6] mapsTo has nothing to do with zeroID test-case --- test/integration/association-hasone-zeroid.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/association-hasone-zeroid.js b/test/integration/association-hasone-zeroid.js index d1bd3637..be1b392e 100644 --- a/test/integration/association-hasone-zeroid.js +++ b/test/integration/association-hasone-zeroid.js @@ -15,13 +15,13 @@ describe("hasOne", function() { db.settings.set('instance.returnAllErrors', true); Person = db.define('person', { - id : {type : "integer", mapsTo: "personID", key:true}, + id : {type : "integer", key:true}, firstName : {type : "text", size:"255"}, lastName : {type : "text", size:"255"} }); Pet = db.define('pet', { - id : {type : "integer", mapsTo:"petID", key:true}, + id : {type : "integer", key:true}, petName : {type : "text", size:"255"}, ownerID : {type : "integer", size:"4"} });