From 2e805b1dc94f2c3c057700b9d463ca92ee4e72c3 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 10 Dec 2015 01:19:52 +1300 Subject: [PATCH 1/8] When doing autofetch on associations, cached instance may not have all the associations populated --- lib/Model.js | 16 ++ lib/Singleton.js | 3 + .../association-hasone-recursive.js | 138 ++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 test/integration/association-hasone-recursive.js diff --git a/lib/Model.js b/lib/Model.js index c684c4d2..7444df12 100644 --- a/lib/Model.js +++ b/lib/Model.js @@ -92,6 +92,21 @@ function Model(opts) { ExtendAssociation.extend(model, instance, opts.driver, extend_associations, assoc_opts); }; + // check all the auto-fetch associations have been populated for the given instance + var checkAssociations = function(instance) { + var checkAssn = function(associations) { + for (var assn = 0; assn < associations.length; assn++) { + if(associations[assn].autoFetch && ! instance.hasOwnProperty(associations[assn].name)) { + return false; + } + }; + return true; + }; + return checkAssn(one_associations) && + checkAssn(many_associations) && + checkAssn(extend_associations); + }; + var pending = 2, create_err = null; var instance = new Instance(model, { uid : inst_opts.uid, // singleton unique id @@ -112,6 +127,7 @@ function Model(opts) { extend_associations : extend_associations, association_properties : association_properties, setupAssociations : setupAssociations, + checkAssociations : checkAssociations, fieldToPropertyMap : fieldToPropertyMap, keyProperties : keyProperties }); diff --git a/lib/Singleton.js b/lib/Singleton.js index 6fbc5449..0266ace6 100644 --- a/lib/Singleton.js +++ b/lib/Singleton.js @@ -19,6 +19,9 @@ exports.get = function (key, opts, createCb, returnCb) { return createCb(returnCb); } else if (map[key].t !== null && map[key].t <= Date.now()) { delete map[key]; + } else if (! map[key].o.__opts.checkAssociations(map[key].o)) { + // instance in cache doesn't have all auto-fetch associations populated + return createCb(returnCb); } else { return returnCb(null, map[key].o); } diff --git a/test/integration/association-hasone-recursive.js b/test/integration/association-hasone-recursive.js new file mode 100644 index 00000000..e907b45b --- /dev/null +++ b/test/integration/association-hasone-recursive.js @@ -0,0 +1,138 @@ +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 Animal = null; + + var setup = function (opts) { + return function (done) { + db.settings.set('instance.cache', opts.cache); + db.settings.set('instance.returnAllErrors', true); + + Animal = db.define('animal', { + id : {type : "integer", key:true}, + name : {type : "text", size:"255"}, + damId : {type : "integer"}, + sireId : {type : "integer"} + }); + + Animal.hasOne('sire', Animal, {field: 'sireId', autoFetch:opts.autoFetch}); + Animal.hasOne('dam', Animal, {field: 'damId', autoFetch:opts.autoFetch}); + + helper.dropSync([Animal], function(err) { + if (err) return done(err); + Animal.create([{ id:1, name: 'Bronson', sireId:10, damId:20}, + + { id:10, name: 'McTavish', sireId:11, damId:12}, + { id:11, name:'Todd'}, + { id:12, name:'Jemima'}, + + { id:20, name: 'Suzy', sireId:21, damId:22}, + { id:21, name:'Liam'}, + { id:22, name:'Glencora'} + ], done); + }); + }; + }; + + before(function(done) { + helper.connect(function (connection) { + db = connection; + done(); + }); + }); + + [{cache:false, autoFetch:true}, + {cache:true, autoFetch:true} + ].forEach(function(opts) { + + describe("recursive hasOne() with " + (opts.cache ? "" : "out ") + "cache", function () { + before(setup(opts)); + + it("should get Bronson's sire & dam", function (done) { + Animal.find({name: "Bronson"}, function(err, animals) { + should.not.exist(err); + + animals[0].name.should.equal("Bronson"); + + // All of Bronson's Sire & Dam stuff should be present + animals[0].sireId.should.equal(10); + animals[0].damId.should.equal(20); + + animals[0].should.have.property("sire"); + animals[0].should.have.property("dam"); + + animals[0].sire.name.should.equal("McTavish"); + animals[0].dam.name.should.equal("Suzy"); + + // Bronson's GrandSire & GrandDam shouldn't be present + animals[0].sire.should.not.have.property("sire"); + animals[0].sire.should.not.have.property("dam"); + + animals[0].dam.should.not.have.property("sire"); + animals[0].dam.should.not.have.property("dam"); + + // but Bronson's GrandSire & GrandDam ID's should be known + animals[0].sire.sireId.should.equal(11); + animals[0].sire.damId.should.equal(12); + + animals[0].dam.sireId.should.equal(21); + animals[0].dam.damId.should.equal(22); + + return done(); + }); + }); + + it("should get McTavish's sire & dam", function (done) { + Animal.find({name: "McTavish"}, function(err, animals) { + should.not.exist(err); + + animals[0].name.should.equal("McTavish"); + + // All of McTavish's Sire & Dam stuff should be present (won't be with cache turned on) + animals[0].sireId.should.equal(11); + animals[0].damId.should.equal(12); + + animals[0].should.have.property("sire"); + animals[0].should.have.property("dam"); + + animals[0].sire.name.should.equal("Todd"); + animals[0].dam.name.should.equal("Jemima"); + + return done(); + }); + }); + + it("should get Suzy's sire & dam", function (done) { + Animal.find({name: "Suzy"}, function(err, animals) { + should.not.exist(err); + + animals[0].name.should.equal("Suzy"); + + // All of Suzy's Sire & Dam stuff should be present (won't be with cache turned on) + animals[0].sireId.should.equal(21); + animals[0].damId.should.equal(22); + + animals[0].should.have.property("sire") + animals[0].should.have.property("dam"); + + animals[0].sire.name.should.equal("Liam"); + animals[0].dam.name.should.equal("Glencora"); + + Animal.get(20, function(err, Suzy) { + should.not.exist(err); + + Suzy.name.should.equal("Suzy"); + Suzy.sire.name.should.equal("Liam"); + Suzy.dam.name.should.equal("Glencora"); + }); + return done(); + }); + }); + }); + }); +}); From d267788847a9fe7f269d3366b5a8e1a322c6e7da Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 10 Dec 2015 01:59:33 +1300 Subject: [PATCH 2/8] Check cache returns correct objects, auto-fetch on & off --- .../association-hasone-recursive.js | 82 +++++++++++-------- 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/test/integration/association-hasone-recursive.js b/test/integration/association-hasone-recursive.js index e907b45b..346cf4a8 100644 --- a/test/integration/association-hasone-recursive.js +++ b/test/integration/association-hasone-recursive.js @@ -18,10 +18,10 @@ describe("hasOne", function() { name : {type : "text", size:"255"}, damId : {type : "integer"}, sireId : {type : "integer"} - }); + }, {cache:opts.timeout}); - Animal.hasOne('sire', Animal, {field: 'sireId', autoFetch:opts.autoFetch}); - Animal.hasOne('dam', Animal, {field: 'damId', autoFetch:opts.autoFetch}); + Animal.hasOne('sire', Animal, {field: 'sireId', autoFetch:true}); + Animal.hasOne('dam', Animal, {field: 'damId', autoFetch:false}); helper.dropSync([Animal], function(err) { if (err) return done(err); @@ -46,89 +46,99 @@ describe("hasOne", function() { }); }); - [{cache:false, autoFetch:true}, - {cache:true, autoFetch:true} - ].forEach(function(opts) { + [false, true].forEach(function(cache) { - describe("recursive hasOne() with " + (opts.cache ? "" : "out ") + "cache", function () { - before(setup(opts)); + describe("recursive hasOne() with " + (cache ? "" : "out ") + "cache", function () { + before(setup({cache:cache, timeout:0.5})); - it("should get Bronson's sire & dam", function (done) { + it("should get Bronson's Sire but not Dam", function (done) { Animal.find({name: "Bronson"}, function(err, animals) { should.not.exist(err); animals[0].name.should.equal("Bronson"); - // All of Bronson's Sire & Dam stuff should be present + // All of Bronson's Sire & some of his Dam stuff should be present animals[0].sireId.should.equal(10); animals[0].damId.should.equal(20); animals[0].should.have.property("sire"); - animals[0].should.have.property("dam"); + animals[0].should.not.have.property("dam"); // no auto-fetch animals[0].sire.name.should.equal("McTavish"); - animals[0].dam.name.should.equal("Suzy"); - // Bronson's GrandSire & GrandDam shouldn't be present + // Bronson's paternal GrandSire & GrandDam shouldn't be present - autoFetchLimit animals[0].sire.should.not.have.property("sire"); animals[0].sire.should.not.have.property("dam"); - animals[0].dam.should.not.have.property("sire"); - animals[0].dam.should.not.have.property("dam"); - - // but Bronson's GrandSire & GrandDam ID's should be known + // but Bronson's paternal GrandSire & GrandDam ID's should be known animals[0].sire.sireId.should.equal(11); animals[0].sire.damId.should.equal(12); - animals[0].dam.sireId.should.equal(21); - animals[0].dam.damId.should.equal(22); - return done(); }); }); - it("should get McTavish's sire & dam", function (done) { + it("should get McTavish's Sire but not Dam", function (done) { Animal.find({name: "McTavish"}, function(err, animals) { should.not.exist(err); animals[0].name.should.equal("McTavish"); - // All of McTavish's Sire & Dam stuff should be present (won't be with cache turned on) + // All of McTavish's Sire & some of his Dam stuff should be present animals[0].sireId.should.equal(11); animals[0].damId.should.equal(12); - animals[0].should.have.property("sire"); - animals[0].should.have.property("dam"); + animals[0].should.have.property("sire"); // now fixed by stueynz + animals[0].should.not.have.property("dam"); // no auto-fetch - animals[0].sire.name.should.equal("Todd"); - animals[0].dam.name.should.equal("Jemima"); + animals[0].sire.name.should.equal("Todd"); // just to be sure + // Go get McTavish again + Animal.get(10, function(err, McTavish) { + should.not.exist(err); + if(cache) { + McTavish.id.should.equal(animals[0].id); + McTavish.name.should.equal(animals[0].name); + McTavish.should.equal(animals[0]); // Should be the same one from cache again + } + else { + McTavish.id.should.equal(animals[0].id); + McTavish.name.should.equal(animals[0].name); + McTavish.should.not.equal(animals[0]); // Should be different one (not using cache) + } + }); return done(); }); }); - it("should get Suzy's sire & dam", function (done) { + it("should get Suzy's Sire but not Dam", function (done) { Animal.find({name: "Suzy"}, function(err, animals) { should.not.exist(err); animals[0].name.should.equal("Suzy"); - // All of Suzy's Sire & Dam stuff should be present (won't be with cache turned on) + // All of Suzy's Sire & Dam stuff should be present animals[0].sireId.should.equal(21); animals[0].damId.should.equal(22); - animals[0].should.have.property("sire") - animals[0].should.have.property("dam"); + animals[0].should.have.property("sire") // now fixed by stueynz + animals[0].should.not.have.property("dam"); // no auto-fetch - animals[0].sire.name.should.equal("Liam"); - animals[0].dam.name.should.equal("Glencora"); + animals[0].sire.name.should.equal("Liam"); // just to be sure + // Go get Suzy again Animal.get(20, function(err, Suzy) { should.not.exist(err); - - Suzy.name.should.equal("Suzy"); - Suzy.sire.name.should.equal("Liam"); - Suzy.dam.name.should.equal("Glencora"); + if(cache) { + Suzy.id.should.equal(animals[0].id); + Suzy.name.should.equal(animals[0].name); + Suzy.should.equal(animals[0]); // Should be the same one from cache again + } + else { + Suzy.id.should.equal(animals[0].id); + Suzy.name.should.equal(animals[0].name); + Suzy.should.not.equal(animals[0]); // Should be different one (not using cache) + } }); return done(); }); From c23f1c507394d945e65d3155999d3f563d7cf449 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 10 Dec 2015 08:51:04 +1300 Subject: [PATCH 3/8] Ensure cache is still working as expected --- lib/Singleton.js | 16 ++++--- .../association-hasone-recursive.js | 48 ++++++++----------- 2 files changed, 29 insertions(+), 35 deletions(-) diff --git a/lib/Singleton.js b/lib/Singleton.js index 0266ace6..90397916 100644 --- a/lib/Singleton.js +++ b/lib/Singleton.js @@ -15,18 +15,20 @@ exports.get = function (key, opts, createCb, returnCb) { } if (map.hasOwnProperty(key)) { if (opts && opts.save_check && typeof map[key].o.saved === "function" && !map[key].o.saved()) { - // if not saved, don't return it, fetch original from db + // if not saved, don't return it, fetch original from db, but don't refresh the cache return createCb(returnCb); } else if (map[key].t !== null && map[key].t <= Date.now()) { delete map[key]; - } else if (! map[key].o.__opts.checkAssociations(map[key].o)) { - // instance in cache doesn't have all auto-fetch associations populated - return createCb(returnCb); - } else { - return returnCb(null, map[key].o); - } + } else { + var instance = map[key].o; + if(instance.__opts.checkAssociations(instance)) { + // instance in cache has all auto-fetch associations populated - we can re-use it + return returnCb(null, instance); + } + } } + // Can't re-use item in cache - or it's not in cache - re-fetch from db & load the cache createCb(function (err, value) { if (err) return returnCb(err); diff --git a/test/integration/association-hasone-recursive.js b/test/integration/association-hasone-recursive.js index 346cf4a8..d52d36e1 100644 --- a/test/integration/association-hasone-recursive.js +++ b/test/integration/association-hasone-recursive.js @@ -8,9 +8,9 @@ describe("hasOne", function() { var db = null; var Animal = null; - var setup = function (opts) { + var setup = function (cache) { return function (done) { - db.settings.set('instance.cache', opts.cache); + db.settings.set('instance.cache', cache); db.settings.set('instance.returnAllErrors', true); Animal = db.define('animal', { @@ -18,7 +18,7 @@ describe("hasOne", function() { name : {type : "text", size:"255"}, damId : {type : "integer"}, sireId : {type : "integer"} - }, {cache:opts.timeout}); + }); Animal.hasOne('sire', Animal, {field: 'sireId', autoFetch:true}); Animal.hasOne('dam', Animal, {field: 'damId', autoFetch:false}); @@ -46,10 +46,10 @@ describe("hasOne", function() { }); }); - [false, true].forEach(function(cache) { + [false, 1].forEach(function(cache) { describe("recursive hasOne() with " + (cache ? "" : "out ") + "cache", function () { - before(setup({cache:cache, timeout:0.5})); + before(setup(cache)); it("should get Bronson's Sire but not Dam", function (done) { Animal.find({name: "Bronson"}, function(err, animals) { @@ -74,7 +74,7 @@ describe("hasOne", function() { animals[0].sire.sireId.should.equal(11); animals[0].sire.damId.should.equal(12); - return done(); + return done(); }); }); @@ -93,21 +93,17 @@ describe("hasOne", function() { animals[0].sire.name.should.equal("Todd"); // just to be sure - // Go get McTavish again - Animal.get(10, function(err, McTavish) { + // Let's make sure the cache is still working... + Animal.get(animals[0].id, function(err, McTavish) { should.not.exist(err); + if(cache) { - McTavish.id.should.equal(animals[0].id); - McTavish.name.should.equal(animals[0].name); - McTavish.should.equal(animals[0]); // Should be the same one from cache again - } - else { - McTavish.id.should.equal(animals[0].id); - McTavish.name.should.equal(animals[0].name); - McTavish.should.not.equal(animals[0]); // Should be different one (not using cache) + McTavish.should.equal(animals[0]); + } else { + McTavish.should.not.equal(animals[0]); } + return done(); }); - return done(); }); }); @@ -126,21 +122,17 @@ describe("hasOne", function() { animals[0].sire.name.should.equal("Liam"); // just to be sure - // Go get Suzy again - Animal.get(20, function(err, Suzy) { + // Let's make sure the cache is still working... + Animal.get(animals[0].id, function(err, Suzy) { should.not.exist(err); + if(cache) { - Suzy.id.should.equal(animals[0].id); - Suzy.name.should.equal(animals[0].name); - Suzy.should.equal(animals[0]); // Should be the same one from cache again - } - else { - Suzy.id.should.equal(animals[0].id); - Suzy.name.should.equal(animals[0].name); - Suzy.should.not.equal(animals[0]); // Should be different one (not using cache) + Suzy.should.equal(animals[0]); + } else { + Suzy.should.not.equal(animals[0]); } + return done(); }); - return done(); }); }); }); From 350b4a4bb728830ad80b9a3ad31f37f3c7643f4d Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 17 Dec 2015 18:17:24 +1300 Subject: [PATCH 4/8] Using the 'autoFetch' option on get() and find() will override the 'autoFetch' setting from the association definition. --- lib/Associations/Extend.js | 6 ++- lib/Associations/Many.js | 6 ++- lib/Associations/One.js | 6 ++- lib/Model.js | 13 +++---- test/integration/association-extend.js | 52 +++++++++++++++++++++++++ test/integration/association-hasmany.js | 33 +++++++++++++++- test/integration/association-hasone.js | 21 ++++++++++ 7 files changed, 126 insertions(+), 11 deletions(-) diff --git a/lib/Associations/Extend.js b/lib/Associations/Extend.js index a6d42ac1..2e63ab30 100644 --- a/lib/Associations/Extend.js +++ b/lib/Associations/Extend.js @@ -214,7 +214,11 @@ function autoFetchInstance(Instance, association, opts, cb) { opts.autoFetchLimit = association.autoFetchLimit; } - if (opts.autoFetchLimit === 0 || (!opts.autoFetch && !association.autoFetch)) { + // autoFetchLimit has been reached - + // opts.autoFetch is present, so it overrides association.autoFetch + // opts.autoFetch not present, use value from original assn definition + if (opts.autoFetchLimit === 0 || (typeof opts.autoFetch !== "undefined" && ! opts.autoFetch) + || (typeof opts.autoFetch === "undefined" && !association.autoFetch)) { return cb(); } diff --git a/lib/Associations/Many.js b/lib/Associations/Many.js index 1500772d..2a0e2d9b 100644 --- a/lib/Associations/Many.js +++ b/lib/Associations/Many.js @@ -475,7 +475,11 @@ function autoFetchInstance(Instance, association, opts, cb) { opts.autoFetchLimit = association.autoFetchLimit; } - if (opts.autoFetchLimit === 0 || (!opts.autoFetch && !association.autoFetch)) { + // autoFetchLimit has been reached - + // opts.autoFetch is present, so it overrides association.autoFetch + // opts.autoFetch not present, use value from original assn definition + if (opts.autoFetchLimit === 0 || (typeof opts.autoFetch !== "undefined" && ! opts.autoFetch) + || (typeof opts.autoFetch === "undefined" && !association.autoFetch)) { return cb(); } diff --git a/lib/Associations/One.js b/lib/Associations/One.js index df8a3a46..fbf1ebcd 100644 --- a/lib/Associations/One.js +++ b/lib/Associations/One.js @@ -297,7 +297,11 @@ function autoFetchInstance(Instance, association, opts, cb) { opts.autoFetchLimit = association.autoFetchLimit; } - if (opts.autoFetchLimit === 0 || (!opts.autoFetch && !association.autoFetch)) { + // autoFetchLimit has been reached - + // opts.autoFetch is present, so it overrides association.autoFetch + // opts.autoFetch not present, use value from original assn definition + if (opts.autoFetchLimit === 0 || (typeof opts.autoFetch !== "undefined" && ! opts.autoFetch) + || (typeof opts.autoFetch === "undefined" && !association.autoFetch)) { return cb(); } diff --git a/lib/Model.js b/lib/Model.js index 7444df12..f45f2be2 100644 --- a/lib/Model.js +++ b/lib/Model.js @@ -81,7 +81,7 @@ function Model(opts) { } var assoc_opts = { - autoFetch : inst_opts.autoFetch || false, + autoFetch : inst_opts.autoFetch, autoFetchLimit : inst_opts.autoFetchLimit, cascadeRemove : inst_opts.cascadeRemove }; @@ -291,9 +291,6 @@ function Model(opts) { conditions[prop.mapsTo] = ids[i]; } - if (!options.hasOwnProperty("autoFetch")) { - options.autoFetch = opts.autoFetch; - } if (!options.hasOwnProperty("autoFetchLimit")) { options.autoFetchLimit = opts.autoFetchLimit; } @@ -315,7 +312,8 @@ function Model(opts) { Singleton.get(uid, { cache : (options.hasOwnProperty("cache") ? options.cache : opts.cache), - save_check : opts.settings.get("instance.cacheSaveCheck") + save_check : opts.settings.get("instance.cacheSaveCheck"), + autoFetch : options.autoFetch }, function (cb) { return createInstance(data[0], { uid : uid, @@ -425,12 +423,13 @@ function Model(opts) { // Now we can do the cache lookup Singleton.get(uid, { cache : options.cache, - save_check : opts.settings.get("instance.cacheSaveCheck") + save_check : opts.settings.get("instance.cacheSaveCheck"), + autoFetch : options.autoFetch }, function (cb) { return createInstance(data, { uid : uid, autoSave : opts.autoSave, - autoFetch : (options.autoFetchLimit === 0 ? false : (options.autoFetch || opts.autoFetch)), + autoFetch : (options.autoFetchLimit === 0 ? false : options.autoFetch), autoFetchLimit : options.autoFetchLimit, cascadeRemove : options.cascadeRemove, extra : options.extra, diff --git a/test/integration/association-extend.js b/test/integration/association-extend.js index a14f91e5..b2c4e0d4 100644 --- a/test/integration/association-extend.js +++ b/test/integration/association-extend.js @@ -97,6 +97,24 @@ describe("Model.extendsTo()", function() { Person.find().first(function (err, John) { should.equal(err, null); + John.should.not.have.property("address"); + + John.getAddress(function (err, Address) { + should.equal(err, null); + Address.should.be.a("object"); + Address.should.have.property("street", "Liberty"); + + return done(); + }); + }); + }); + + it("should honor autoFetch:TRUE on find", function (done) { + Person.find({name:"John Doe"}, {autoFetch:true}).first(function (err, John) { + should.equal(err, null); + + John.should.have.property("address"); + John.getAddress(function (err, Address) { should.equal(err, null); Address.should.be.a("object"); @@ -107,6 +125,40 @@ describe("Model.extendsTo()", function() { }); }); + it("should honor autoFetch:FALSE on find", function (done) { + Person.find({name:"John Doe"}, {autoFetch:false}).first(function (err, John) { + should.equal(err, null); + + John.should.not.have.property("address"); + return done(); + }); + }); + + it("should honor autoFetch:TRUE on get", function (done) { + Person.get(1, {autoFetch:true}, function (err, John) { + should.equal(err, null); + + John.should.have.property("address"); + + John.getAddress(function (err, Address) { + should.equal(err, null); + Address.should.be.a("object"); + Address.should.have.property("street", "Liberty"); + + return done(); + }); + }); + }); + + it("should honor autoFetch:FALSE on get", function (done) { + Person.get(1, {autoFetch:false}, function (err, John) { + should.equal(err, null); + + John.should.not.have.property("address"); + return done(); + }); + }); + it("should return error if not found", function (done) { Person.find().first(function (err, John) { should.equal(err, null); diff --git a/test/integration/association-hasmany.js b/test/integration/association-hasmany.js index 167d79d8..794187c3 100644 --- a/test/integration/association-hasmany.js +++ b/test/integration/association-hasmany.js @@ -553,7 +553,7 @@ describe("hasMany", function () { autoFetchPets : true })); - it("should fetch associations", function (done) { + it("should fetch associations on find", function (done) { Person.find({ name: "John" }).first(function (err, John) { should.equal(err, null); @@ -565,6 +565,37 @@ describe("hasMany", function () { }); }); + it("should fetch associations on get", function (done) { + Person.get(1, function (err, John) { + should.equal(err, null); + + John.should.have.property("pets"); + should(Array.isArray(John.pets)); + John.pets.length.should.equal(2); + + return done(); + }); + }); + + it("should honor autoFetch FALSE on find", function (done) { + + Person.find({ name: "John" }, {autoFetch:false}).first(function (err, John) { + should.equal(err, null); + + John.should.not.have.property("pets"); + return done(); + }); + }); + + it("should honor autoFetch FALSE on get", function (done) { + Person.get(1, {autoFetch: false}, function (err, John) { + should.equal(err, null); + + John.should.not.have.property("pets"); + return done(); + }); + }); + it("should save existing", function (done) { Person.create({ name: 'Bishan' }, function (err) { should.not.exist(err); diff --git a/test/integration/association-hasone.js b/test/integration/association-hasone.js index 91226471..431c79e1 100644 --- a/test/integration/association-hasone.js +++ b/test/integration/association-hasone.js @@ -174,6 +174,27 @@ describe("hasOne", function() { }); }); + describe("autofetching get override", function() { + it((!af ? "should" : "shouldn't") + " be done", function (done) { + Leaf.get(1, {autoFetch: !af}, function (err, leaf) { + should.not.exist(err); + should.equal(typeof leaf.tree, !af ? 'object' : 'undefined'); + + return done(); + }); + }); + }); + describe("autofetching find override", function() { + it((!af ? "should" : "shouldn't") + " be done", function (done) { + Leaf.find({size:14}, {autoFetch: !af}, function (err, leaves) { + should.not.exist(err); + should.equal(typeof leaves[0].tree, !af ? 'object' : 'undefined'); + + return done(); + }); + }); + }); + describe("associating by parent id", function () { var tree = null; From 02824e2429fa5d2a965dcf74bfc2b8736a51bfdb Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 17 Dec 2015 18:18:58 +1300 Subject: [PATCH 5/8] if get() or find() has locally set 'autoFetch' option, then don't use the cache --- lib/Singleton.js | 16 ++-- .../association-hasone-recursive.js | 90 +++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/lib/Singleton.js b/lib/Singleton.js index 90397916..17f7c27b 100644 --- a/lib/Singleton.js +++ b/lib/Singleton.js @@ -13,6 +13,10 @@ exports.get = function (key, opts, createCb, returnCb) { if (opts && opts.cache === false) { return createCb(returnCb); } + // we've got a custom autoFetch setting for this get/find so don't use the cache + if(typeof opts.autoFetch !== "undefined") { + return createCb(returnCb); + } if (map.hasOwnProperty(key)) { if (opts && opts.save_check && typeof map[key].o.saved === "function" && !map[key].o.saved()) { // if not saved, don't return it, fetch original from db, but don't refresh the cache @@ -20,12 +24,12 @@ exports.get = function (key, opts, createCb, returnCb) { } else if (map[key].t !== null && map[key].t <= Date.now()) { delete map[key]; } else { - var instance = map[key].o; - if(instance.__opts.checkAssociations(instance)) { - // instance in cache has all auto-fetch associations populated - we can re-use it - return returnCb(null, instance); - } - } + var instance = map[key].o; + if(instance.__opts.checkAssociations(instance)) { + // instance in cache has all auto-fetch associations populated - we can re-use it + return returnCb(null, instance); + } + } } // Can't re-use item in cache - or it's not in cache - re-fetch from db & load the cache diff --git a/test/integration/association-hasone-recursive.js b/test/integration/association-hasone-recursive.js index d52d36e1..747688bb 100644 --- a/test/integration/association-hasone-recursive.js +++ b/test/integration/association-hasone-recursive.js @@ -78,6 +78,96 @@ describe("hasOne", function() { }); }); + it("should honor auto-fetch override:TRUE on find", function (done) { + Animal.find({name: "Bronson"}, {autoFetch:true}, function(err, animals) { + should.not.exist(err); + + animals[0].name.should.equal("Bronson"); + + // All of Bronson's Sire & Dam stuff should be present + animals[0].sireId.should.equal(10); + animals[0].damId.should.equal(20); + + animals[0].should.have.property("sire"); + animals[0].should.have.property("dam"); // no auto-fetch - overridden for this find + + animals[0].sire.name.should.equal("McTavish"); + animals[0].dam.name.should.equal("Suzy"); + + // Bronson's paternal GrandSire & GrandDam shouldn't be present - autoFetchLimit + animals[0].sire.should.not.have.property("sire"); + animals[0].sire.should.not.have.property("dam"); + + // but Bronson's paternal GrandSire & GrandDam ID's should be known + animals[0].sire.sireId.should.equal(11); + animals[0].sire.damId.should.equal(12); + + return done(); + }); + }); + + it("should honor auto-fetch override:FALSE on find", function (done) { + Animal.find({name: "Bronson"}, {autoFetch:false}, function(err, animals) { + should.not.exist(err); + + animals[0].name.should.equal("Bronson"); + + // Only Bronson's Sire & Dam IDs should be present + animals[0].sireId.should.equal(10); + animals[0].damId.should.equal(20); + + animals[0].should.not.have.property("sire"); + animals[0].should.not.have.property("dam"); + + return done(); + }); + }); + + it("should honor auto-fetch override:TRUE on get", function (done) { + Animal.get(1, {autoFetch:true}, function(err, Bronson) { + should.not.exist(err); + + Bronson.name.should.equal("Bronson"); + + // All of Bronson's Sire & Dam stuff should be present + Bronson.sireId.should.equal(10); + Bronson.damId.should.equal(20); + + Bronson.should.have.property("sire"); + Bronson.should.have.property("dam"); // no auto-fetch - overridden for this find + + Bronson.sire.name.should.equal("McTavish"); + Bronson.dam.name.should.equal("Suzy"); + + // Bronson's paternal GrandSire & GrandDam shouldn't be present - autoFetchLimit + Bronson.sire.should.not.have.property("sire"); + Bronson.sire.should.not.have.property("dam"); + + // but Bronson's paternal GrandSire & GrandDam ID's should be known + Bronson.sire.sireId.should.equal(11); + Bronson.sire.damId.should.equal(12); + + return done(); + }); + }); + + it("should honor auto-fetch override:FALSE on get", function (done) { + Animal.get(1, {autoFetch:false}, function(err, Bronson) { + should.not.exist(err); + + Bronson.name.should.equal("Bronson"); + + // Only Bronson's Sire & Dam IDs should be present + Bronson.sireId.should.equal(10); + Bronson.damId.should.equal(20); + + Bronson.should.not.have.property("sire"); + Bronson.should.not.have.property("dam"); + + return done(); + }); + }); + it("should get McTavish's Sire but not Dam", function (done) { Animal.find({name: "McTavish"}, function(err, animals) { should.not.exist(err); From 51307fd4b6c1644573005a7d436c0bb0e6027050 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 17 Dec 2015 18:24:16 +1300 Subject: [PATCH 6/8] renamed checkAssociations() to hasAutoFetchAssociationsLoaded() as its more descriptive, if a little long --- lib/Model.js | 4 ++-- lib/Singleton.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Model.js b/lib/Model.js index f45f2be2..97e33413 100644 --- a/lib/Model.js +++ b/lib/Model.js @@ -93,7 +93,7 @@ function Model(opts) { }; // check all the auto-fetch associations have been populated for the given instance - var checkAssociations = function(instance) { + var hasAutoFetchAssociationsLoaded = function(instance) { var checkAssn = function(associations) { for (var assn = 0; assn < associations.length; assn++) { if(associations[assn].autoFetch && ! instance.hasOwnProperty(associations[assn].name)) { @@ -127,7 +127,7 @@ function Model(opts) { extend_associations : extend_associations, association_properties : association_properties, setupAssociations : setupAssociations, - checkAssociations : checkAssociations, + hasAutoFetchAssociationsLoaded: hasAutoFetchAssociationsLoaded, fieldToPropertyMap : fieldToPropertyMap, keyProperties : keyProperties }); diff --git a/lib/Singleton.js b/lib/Singleton.js index 17f7c27b..b08ec3c3 100644 --- a/lib/Singleton.js +++ b/lib/Singleton.js @@ -25,7 +25,7 @@ exports.get = function (key, opts, createCb, returnCb) { delete map[key]; } else { var instance = map[key].o; - if(instance.__opts.checkAssociations(instance)) { + if(instance.__opts.hasAutoFetchAssociationsLoaded(instance)) { // instance in cache has all auto-fetch associations populated - we can re-use it return returnCb(null, instance); } From e38be1192ccccca24d80799f27af87d94ac1f968 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Thu, 17 Dec 2015 22:56:14 +1300 Subject: [PATCH 7/8] Whoops, we can't assume what the Id values will be when testing get() functions; particularly for mongodb --- test/integration/association-extend.js | 7 ++++--- test/integration/association-hasmany.js | 14 +++++++++----- test/integration/association-hasone.js | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/test/integration/association-extend.js b/test/integration/association-extend.js index b2c4e0d4..275d689f 100644 --- a/test/integration/association-extend.js +++ b/test/integration/association-extend.js @@ -6,6 +6,7 @@ describe("Model.extendsTo()", function() { var db = null; var Person = null; var PersonAddress = null; + var johnId = null; var setup = function () { return function (done) { @@ -24,7 +25,7 @@ describe("Model.extendsTo()", function() { name: "John Doe" }, function (err, person) { should.not.exist(err); - + johnId = person[Person.id]; return person.setAddress(new PersonAddress({ street : "Liberty", number : 123 @@ -135,7 +136,7 @@ describe("Model.extendsTo()", function() { }); it("should honor autoFetch:TRUE on get", function (done) { - Person.get(1, {autoFetch:true}, function (err, John) { + Person.get(johnId, {autoFetch:true}, function (err, John) { should.equal(err, null); John.should.have.property("address"); @@ -151,7 +152,7 @@ describe("Model.extendsTo()", function() { }); it("should honor autoFetch:FALSE on get", function (done) { - Person.get(1, {autoFetch:false}, function (err, John) { + Person.get(johnId, {autoFetch:false}, function (err, John) { should.equal(err, null); John.should.not.have.property("address"); diff --git a/test/integration/association-hasmany.js b/test/integration/association-hasmany.js index 794187c3..0eeb24ed 100644 --- a/test/integration/association-hasmany.js +++ b/test/integration/association-hasmany.js @@ -10,6 +10,7 @@ describe("hasMany", function () { var db = null; var Person = null; var Pet = null; + var johnId = null; before(function(done) { helper.connect(function (connection) { @@ -62,9 +63,12 @@ describe("hasMany", function () { surname : "Dean", age : 18 }], function (err) { - Person.find({ name: "Jane" }, function (err, people) { - Pet.find({ name: "Mutt" }, function (err, pets) { - people[0].addPets(pets, done); + Person.find({ name: "John"}, function(err, people) { + johnId = people[0][Person.id]; + Person.find({ name: "Jane" }, function (err, people) { + Pet.find({ name: "Mutt" }, function (err, pets) { + people[0].addPets(pets, done); + }); }); }); }); @@ -566,7 +570,7 @@ describe("hasMany", function () { }); it("should fetch associations on get", function (done) { - Person.get(1, function (err, John) { + Person.get(johnId, function (err, John) { should.equal(err, null); John.should.have.property("pets"); @@ -588,7 +592,7 @@ describe("hasMany", function () { }); it("should honor autoFetch FALSE on get", function (done) { - Person.get(1, {autoFetch: false}, function (err, John) { + Person.get(johnId, {autoFetch: false}, function (err, John) { should.equal(err, null); John.should.not.have.property("pets"); diff --git a/test/integration/association-hasone.js b/test/integration/association-hasone.js index 431c79e1..4ce1e418 100644 --- a/test/integration/association-hasone.js +++ b/test/integration/association-hasone.js @@ -176,7 +176,7 @@ describe("hasOne", function() { describe("autofetching get override", function() { it((!af ? "should" : "shouldn't") + " be done", function (done) { - Leaf.get(1, {autoFetch: !af}, function (err, leaf) { + Leaf.get(leafId, {autoFetch: !af}, function (err, leaf) { should.not.exist(err); should.equal(typeof leaf.tree, !af ? 'object' : 'undefined'); From d7530918a57b09e6f1de46bdff8b4fe1d2161df0 Mon Sep 17 00:00:00 2001 From: Stuart McGrigor Date: Mon, 11 Jan 2016 11:34:24 +1300 Subject: [PATCH 8/8] When hasMany relationship has extra data, then the UID used by Singleton for the associative entity needs to include both keys to be sure we get the right set of extra data --- lib/Model.js | 9 +- test/integration/association-hasmany-extra.js | 93 +++++++++++-------- 2 files changed, 63 insertions(+), 39 deletions(-) diff --git a/lib/Model.js b/lib/Model.js index 97e33413..8dcac8d2 100644 --- a/lib/Model.js +++ b/lib/Model.js @@ -420,7 +420,14 @@ function Model(opts) { uid += "/" + data[opts.keys[i]]; } - // Now we can do the cache lookup + // When there is extra_info we need a slightly longer uid for the Singleton lookup + if(merge && options.extra_info) { + for(var j=0; j