From 610767b6c8ffc15aec8ab36679fd8be4d95fffac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 26 Feb 2015 16:29:29 +0100 Subject: [PATCH 1/2] Change tracking requires a string id set to GUID Print a deprecation warning when a persisted model is tracking changes but does not have a client-generated unique string id property (GUID). --- lib/persisted-model.js | 10 ++++++++++ package.json | 6 ++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/persisted-model.js b/lib/persisted-model.js index 6e9d07e5..13ca19c4 100644 --- a/lib/persisted-model.js +++ b/lib/persisted-model.js @@ -7,6 +7,7 @@ var registry = require('./registry'); var runtime = require('./runtime'); var assert = require('assert'); var async = require('async'); +var deprecated = require('depd')('loopback'); /** * Extends Model with basic query and CRUD support. @@ -1041,6 +1042,15 @@ PersistedModel.enableChangeTracking = function() { assert(this.dataSource, 'Cannot enableChangeTracking(): ' + this.modelName + ' is not attached to a dataSource'); + var idName = this.getIdName(); + var idProp = this.definition.properties[idName]; + var idType = idProp && idProp.type; + var idDefn = idProp && idProp.defaultFn; + if (idType !== String || !(idDefn === 'uuid' || idDefn === 'guid')) { + deprecated('The model ' + this.modelName + ' is tracking changes, ' + + 'which requries a string id with GUID/UUID default value.'); + } + Change.attachTo(this.dataSource); Change.getCheckpointModel().attachTo(this.dataSource); diff --git a/package.json b/package.json index 198a29ce..b0196f31 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "canonical-json": "0.0.4", "continuation-local-storage": "~3.1.1", "debug": "~2.0.0", + "depd": "^1.0.0", "ejs": "~1.0.0", "express": "^4.10.2", "inflection": "~1.4.2", @@ -52,7 +53,7 @@ "underscore.string": "~2.3.3" }, "peerDependencies": { - "loopback-datasource-juggler": "^2.17.0" + "loopback-datasource-juggler": "^2.19.0" }, "devDependencies": { "browserify": "~4.2.3", @@ -79,7 +80,7 @@ "karma-phantomjs-launcher": "~0.1.4", "karma-script-launcher": "~0.1.0", "loopback-boot": "^1.1.0", - "loopback-datasource-juggler": "^2.17.0", + "loopback-datasource-juggler": "^2.19.0", "loopback-testing": "~0.2.0", "mocha": "~1.21.4", "strong-task-emitter": "0.0.x", @@ -94,6 +95,7 @@ "./lib/server-app.js": "./lib/browser-express.js", "connect": false, "nodemailer": false, + "depd": "loopback-datasource-juggler/lib/browser.depd.js", "bcrypt": false }, "license": { From f21b29e34a52962b97db4ab31cf960b58a675475 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 26 Feb 2015 16:36:44 +0100 Subject: [PATCH 2/2] test: setup GUID for all models tracking changes Fix all test models that are tracking changes so that they have a generated unique string id. Make model names unique where possible to prevent tests reusing the same model which causes unintented side effects. --- test/change.test.js | 10 +++++++--- test/model.test.js | 15 ++++++++++----- test/remote-connector.test.js | 3 ++- test/replication.test.js | 22 ++++++++++++---------- test/util/model-tests.js | 14 ++++++++------ 5 files changed, 39 insertions(+), 25 deletions(-) diff --git a/test/change.test.js b/test/change.test.js index 6192bda5..8c76b7af 100644 --- a/test/change.test.js +++ b/test/change.test.js @@ -6,9 +6,13 @@ describe('Change', function() { var memory = loopback.createDataSource({ connector: loopback.Memory }); - TestModel = loopback.PersistedModel.extend('chtest', {}, { - trackChanges: true - }); + TestModel = loopback.PersistedModel.extend('ChangeTestModel', + { + id: { id: true, type: 'string', defaultFn: 'guid' } + }, + { + trackChanges: true + }); this.modelName = TestModel.modelName; TestModel.attachTo(memory); Change = TestModel.getChangeModel(); diff --git a/test/model.test.js b/test/model.test.js index ad6c305f..db05e13e 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -16,7 +16,7 @@ describe('Model / PersistedModel', function() { describe('Model.validatesUniquenessOf(property, options)', function() { it('Ensure the value for `property` is unique', function(done) { - var User = PersistedModel.extend('user', { + var User = PersistedModel.extend('ValidatedUser', { 'first': String, 'last': String, 'age': Number, @@ -72,6 +72,7 @@ describe.onServer('Remote Methods', function() { beforeEach(function() { User = PersistedModel.extend('user', { + id: { id: true, type: String, defaultFn: 'guid' }, 'first': String, 'last': String, 'age': Number, @@ -489,10 +490,10 @@ describe.onServer('Remote Methods', function() { }); }); - describe('PersistelModel remote methods', function() { + describe('PersistedModel remote methods', function() { it('includes all aliases', function() { var app = loopback(); - var model = PersistedModel.extend('persistedModel'); + var model = PersistedModel.extend('PersistedModelForAliases'); app.dataSource('db', { connector: 'memory' }); app.model(model, { dataSource: 'db' }); @@ -500,7 +501,7 @@ describe.onServer('Remote Methods', function() { var metadata = app.handler('rest') .adapter .getClasses() - .filter(function(c) { return c.name === 'persistedModel'; })[0]; + .filter(function(c) { return c.name === model.modelName; })[0]; var methodNames = []; metadata.methods.forEach(function(method) { @@ -509,7 +510,11 @@ describe.onServer('Remote Methods', function() { }); expect(methodNames).to.have.members([ - 'destroyAll', 'deleteAll', 'remove', + // NOTE(bajtos) These three methods are disabled by default + // Because all tests share the same global registry model + // and one of the tests was enabling remoting of "destroyAll", + // this test was seeing this method (with all aliases) as public + // 'destroyAll', 'deleteAll', 'remove', 'create', 'upsert', 'updateOrCreate', 'exists', diff --git a/test/remote-connector.test.js b/test/remote-connector.test.js index 39a078e8..e267c2e8 100644 --- a/test/remote-connector.test.js +++ b/test/remote-connector.test.js @@ -20,7 +20,8 @@ describe('RemoteConnector', function() { }); }, onDefine: function(Model) { - var RemoteModel = Model.extend(Model.modelName); + var RemoteModel = Model.extend('Remote' + Model.modelName, {}, + { plural: Model.pluralModelName }); RemoteModel.attachTo(loopback.createDataSource({ connector: loopback.Memory })); diff --git a/test/replication.test.js b/test/replication.test.js index 260e75d6..83575f6f 100644 --- a/test/replication.test.js +++ b/test/replication.test.js @@ -14,14 +14,16 @@ describe('Replication / Change APIs', function() { dataSource = this.dataSource = loopback.createDataSource({ connector: loopback.Memory }); - SourceModel = this.SourceModel = PersistedModel.extend('SourceModel', {}, { - trackChanges: true - }); + SourceModel = this.SourceModel = PersistedModel.extend('SourceModel', + { id: { id: true, type: String, defaultFn: 'guid' } }, + { trackChanges: true }); + SourceModel.attachTo(dataSource); - TargetModel = this.TargetModel = PersistedModel.extend('TargetModel', {}, { - trackChanges: true - }); + TargetModel = this.TargetModel = PersistedModel.extend('TargetModel', + { id: { id: true, type: String, defaultFn: 'guid' } }, + { trackChanges: true }); + TargetModel.attachTo(dataSource); test.startingCheckpoint = -1; @@ -169,11 +171,11 @@ describe('Replication / Change APIs', function() { var test = this; this.conflict.models(function(err, source, target) { assert.deepEqual(source.toJSON(), { - id: 1, + id: test.model.id, name: 'source update' }); assert.deepEqual(target.toJSON(), { - id: 1, + id: test.model.id, name: 'target update' }); done(); @@ -242,7 +244,7 @@ describe('Replication / Change APIs', function() { this.conflict.models(function(err, source, target) { assert.equal(source, null); assert.deepEqual(target.toJSON(), { - id: 1, + id: test.model.id, name: 'target update' }); done(); @@ -311,7 +313,7 @@ describe('Replication / Change APIs', function() { this.conflict.models(function(err, source, target) { assert.equal(target, null); assert.deepEqual(source.toJSON(), { - id: 1, + id: test.model.id, name: 'source update' }); done(); diff --git a/test/util/model-tests.js b/test/util/model-tests.js index 9a72e861..487e9820 100644 --- a/test/util/model-tests.js +++ b/test/util/model-tests.js @@ -36,7 +36,8 @@ module.exports = function defineModelTestsWithDataSource(options) { return extendedModel; }; - User = PersistedModel.extend('user', { + User = PersistedModel.extend('UtilUser', { + id: { id: true, type: String, defaultFn: 'guid' }, 'first': String, 'last': String, 'age': Number, @@ -48,8 +49,6 @@ module.exports = function defineModelTestsWithDataSource(options) { trackChanges: true }); - // enable destroy all for testing - User.destroyAll.shared = true; User.attachTo(dataSource); }); @@ -187,10 +186,13 @@ module.exports = function defineModelTestsWithDataSource(options) { it('Remove a model from the attached data source', function(done) { User.create({first: 'joe', last: 'bob'}, function(err, user) { User.findById(user.id, function(err, foundUser) { + if (err) return done(err); assert.equal(user.id, foundUser.id); - foundUser.destroy(function() { - User.findById(user.id, function(err, notFound) { - assert.equal(notFound, null); + User.deleteById(foundUser.id, function(err) { + if (err) return done(err); + User.find({ where: { id: user.id } }, function(err, found) { + if (err) return done(err); + assert.equal(found.length, 0); done(); }); });