From 888d15ce1cf052a511fbbf99c27dd88c4ebb0d82 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 12 Jun 2014 23:35:20 -0700 Subject: [PATCH] Optimize model instantiation and conversion --- lib/dao.js | 14 +- lib/model-builder.js | 65 ++++---- lib/model.js | 331 +++++++++++++++++++------------------- test/loopback-dl.test.js | 13 +- test/manipulation.test.js | 8 - 5 files changed, 213 insertions(+), 218 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index d4bd40de..062ada91 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -162,7 +162,6 @@ DataAccessObject.create = function (data, callback) { this._adapter().create(modelName, this.constructor._forDB(obj.toObject(true)), function (err, id, rev) { if (id) { obj.__data[_idName] = id; - obj.__dataWas[_idName] = id; defineReadonlyProp(obj, _idName, id); } if (rev) { @@ -353,8 +352,7 @@ DataAccessObject.findById = function find(id, cb) { if (!getIdValue(this, data)) { setIdValue(this, data, id); } - obj = new this(); - obj._initProperties(data); + obj = new this(data, {applySetters: false}); } cb(err, obj); }.bind(this)); @@ -689,9 +687,7 @@ DataAccessObject.find = function find(query, cb) { this.getDataSource().connector.all(this.modelName, query, function (err, data) { if (data && data.forEach) { data.forEach(function (d, i) { - var obj = new self(); - - obj._initProperties(d, {fields: query.fields}); + var obj = new self(d, {fields: query.fields, applySetters: false}); if (query && query.include) { if (query.collect) { @@ -1059,12 +1055,6 @@ DataAccessObject.prototype.updateAttributes = function updateAttributes(data, cb } inst._adapter().updateAttributes(model, getIdValue(inst.constructor, inst), inst.constructor._forDB(typedData), function (err) { - if (!err) { - // update $was attrs - for (var key in data) { - inst.__dataWas[key] = inst.__data[key]; - } - } done.call(inst, function () { saveDone.call(inst, function () { if(cb) cb(err, inst); diff --git a/lib/model-builder.js b/lib/model-builder.js index f0b126c6..89c98d1e 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -264,7 +264,11 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett // A function to loop through the properties ModelClass.forEachProperty = function (cb) { - Object.keys(ModelClass.definition.properties).forEach(cb); + var props = ModelClass.definition.properties; + var keys = Object.keys(props); + for (var i = 0, n = keys.length; i < n; i++) { + cb(keys[i], props[keys[i]]); + } }; // A function to attach the model class to a data source @@ -309,27 +313,22 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett } // Merging the properties - Object.keys(properties).forEach(function (key) { + var keys = Object.keys(properties); + for (var i = 0, n = keys.length; i < n; i++) { + var key = keys[i]; + if (idFound && properties[key].id) { // don't inherit id properties - return; + continue; } if (subclassProperties[key] === undefined) { subclassProperties[key] = properties[key]; } - }); + } // Merge the settings subclassSettings = mergeSettings(settings, subclassSettings); - /* - Object.keys(settings).forEach(function (key) { - if(subclassSettings[key] === undefined) { - subclassSettings[key] = settings[key]; - } - }); - */ - // Define the subclass var subClass = modelBuilder.define(className, subclassProperties, subclassSettings, ModelClass); @@ -370,20 +369,15 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett var DataType = ModelClass.definition.properties[propertyName].type; if (Array.isArray(DataType) || DataType === Array) { DataType = List; - } else if (DataType.name === 'Date') { - var OrigDate = Date; - DataType = function Date(arg) { - return new OrigDate(arg); - }; + } else if (DataType === Date) { + DataType = DateType; } else if (typeof DataType === 'string') { DataType = modelBuilder.resolveType(DataType); } if (ModelClass.setter[propertyName]) { ModelClass.setter[propertyName].call(this, value); // Try setter first } else { - if (!this.__data) { - this.__data = {}; - } + this.__data = this.__data || {}; if (value === null || value === undefined) { this.__data[propertyName] = value; } else { @@ -401,15 +395,6 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett enumerable: true }); - // $was --> __dataWas. - Object.defineProperty(ModelClass.prototype, propertyName + '$was', { - get: function () { - return this.__dataWas && this.__dataWas[propertyName]; - }, - configurable: true, - enumerable: false - }); - // FIXME: [rfeng] Do we need to keep the raw data? // Use $ as the prefix to avoid conflicts with properties such as _id Object.defineProperty(ModelClass.prototype, '$' + propertyName, { @@ -427,7 +412,13 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett }); }; - ModelClass.forEachProperty(ModelClass.registerProperty); + var props = ModelClass.definition.properties; + var keys = Object.keys(props); + var size = keys.length; + for (i = 0; i < size; i++) { + var propertyName = keys[i]; + ModelClass.registerProperty(propertyName); + } ModelClass.emit('defined', ModelClass); @@ -435,6 +426,11 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett }; +// DataType for Date +function DateType(arg) { + return new Date(arg); +} + /** * Define single property named `propertyName` on `model` * @@ -479,10 +475,11 @@ ModelBuilder.prototype.defineProperty = function (model, propertyName, propertyD */ ModelBuilder.prototype.extendModel = function (model, props) { var t = this; - Object.keys(props).forEach(function (propName) { - var definition = props[propName]; - t.defineProperty(model, propName, definition); - }); + var keys = Object.keys(props); + for (var i = 0; i < keys.length; i++) { + var definition = props[keys[i]]; + t.defineProperty(model, keys[i], definition); + } }; ModelBuilder.prototype.copyModel = function copyModel(Master) { diff --git a/lib/model.js b/lib/model.js index 508c5b6d..aaf5e29f 100644 --- a/lib/model.js +++ b/lib/model.js @@ -8,13 +8,20 @@ module.exports = ModelBaseClass; */ var util = require('util'); -var traverse = require('traverse'); var jutil = require('./jutil'); var List = require('./list'); var Hookable = require('./hooks'); var validations = require('./validations.js'); -var BASE_TYPES = ['String', 'Boolean', 'Number', 'Date', 'Text', 'ObjectID']; +// Set up an object for quick lookup +var BASE_TYPES = { + 'String': true, + 'Boolean': true, + 'Number': true, + 'Date': true, + 'Text': true, + 'ObjectID': true +}; /** * Model class: base class for all persistent objects. @@ -33,18 +40,6 @@ function ModelBaseClass(data, options) { this._initProperties(data, options); } -// FIXME: [rfeng] We need to make sure the input data should not be mutated. Disabled cloning for now to get tests passing -function clone(data) { - /* - if(!(data instanceof ModelBaseClass)) { - if(data && (Array.isArray(data) || 'object' === typeof data)) { - return traverse(data).clone(); - } - } - */ - return data; -} - /** * Initialize the model instance with a list of properties * @param {Object} data The data object @@ -58,10 +53,10 @@ ModelBaseClass.prototype._initProperties = function (data, options) { var ctor = this.constructor; if(data instanceof ctor) { - // Convert the data to be plain object to avoid polutions + // Convert the data to be plain object to avoid pollutions data = data.toObject(false); } - var properties = ctor.definition.build(); + var properties = ctor.definition.properties; data = data || {}; options = options || {}; @@ -71,133 +66,124 @@ ModelBaseClass.prototype._initProperties = function (data, options) { if(strict === undefined) { strict = ctor.definition.settings.strict; } - Object.defineProperty(this, '__cachedRelations', { - writable: true, - enumerable: false, - configurable: true, - value: {} - }); - Object.defineProperty(this, '__data', { - writable: true, - enumerable: false, - configurable: true, - value: {} - }); + if (ctor.hideInternalProperties) { + // Object.defineProperty() is expensive. We only try to make the internal + // properties hidden (non-enumerable) if the model class has the + // `hideInternalProperties` set to true + Object.defineProperties(this, { + __cachedRelations: { + writable: true, + enumerable: false, + configurable: true, + value: {} + }, - Object.defineProperty(this, '__dataWas', { - writable: true, - enumerable: false, - configurable: true, - value: {} - }); + __data: { + writable: true, + enumerable: false, + configurable: true, + value: {} + }, - /** - * Instance level data source - */ - Object.defineProperty(this, '__dataSource', { - writable: true, - enumerable: false, - configurable: true, - value: options.dataSource - }); + // Instance level data source + __dataSource: { + writable: true, + enumerable: false, + configurable: true, + value: options.dataSource + }, - /** - * Instance level strict mode - */ - Object.defineProperty(this, '__strict', { - writable: true, - enumerable: false, - configurable: true, - value: strict - }); + // Instance level strict mode + __strict: { + writable: true, + enumerable: false, + configurable: true, + value: strict + } + }); + } else { + this.__cachedRelations = {}; + this.__data = {}; + this.__dataSource = options.dataSource; + this.__strict = strict; + } if (data.__cachedRelations) { this.__cachedRelations = data.__cachedRelations; } - for (var i in data) { - if (i in properties && typeof data[i] !== 'function') { - this.__data[i] = this.__dataWas[i] = clone(data[i]); - } else if (i in ctor.relations) { - if (ctor.relations[i].type === 'belongsTo' && data[i] !== null && data[i] !== undefined) { + var keys = Object.keys(data); + var size = keys.length; + var p, propVal; + for (var k = 0; k < size; k++) { + p = keys[k]; + propVal = data[p]; + if (typeof propVal === 'function') { + continue; + } + if (properties[p]) { + // Managed property + if (applySetters) { + self[p] = propVal; + } else { + self.__data[p] = propVal; + } + } else if (ctor.relations[p]) { + // Relation + if (ctor.relations[p].type === 'belongsTo' && propVal != null) { // If the related model is populated - this.__data[ctor.relations[i].keyFrom] = this.__dataWas[i] = data[i][ctor.relations[i].keyTo]; + self.__data[ctor.relations[p].keyFrom] = propVal[ctor.relations[p].keyTo]; } - this.__cachedRelations[i] = data[i]; + self.__cachedRelations[p] = propVal; } else { + // Un-managed property if (strict === false) { - this.__data[i] = this.__dataWas[i] = clone(data[i]); + self[p] = self.__data[p] = propVal; } else if (strict === 'throw') { - throw new Error('Unknown property: ' + i); + throw new Error('Unknown property: ' + p); } } } - var propertyName; - if (applySetters === true) { - for (propertyName in data) { - if (typeof data[propertyName] !== 'function' && ((propertyName in properties) || (propertyName in ctor.relations))) { - self[propertyName] = self.__data[propertyName] || data[propertyName]; + keys = Object.keys(properties); + size = keys.length; + + for (k = 0; k < size; k++) { + p = keys[k]; + propVal = self.__data[p]; + + // Set default values + if (propVal === undefined) { + var def = properties[p]['default']; + if (def !== undefined) { + if (typeof def === 'function') { + self.__data[p] = def(); + } else { + self.__data[p] = def; + } } } - } - // Set the unknown properties as properties to the object - if (strict === false) { - for (propertyName in data) { - if (typeof data[propertyName] !== 'function' && !(propertyName in properties)) { - self[propertyName] = self.__data[propertyName] || data[propertyName]; - } - } - } - - ctor.forEachProperty(function (propertyName) { - - if (undefined === self.__data[propertyName]) { - self.__data[propertyName] = self.__dataWas[propertyName] = getDefault(propertyName); - } else { - self.__dataWas[propertyName] = self.__data[propertyName]; - } - - }); - - ctor.forEachProperty(function (propertyName) { - - var type = properties[propertyName].type; - - if (BASE_TYPES.indexOf(type.name) === -1) { - if (typeof self.__data[propertyName] !== 'object' && self.__data[propertyName]) { + // Handle complex types (JSON/Object) + var type = properties[p].type; + if (! BASE_TYPES[type.name]) { + if (typeof self.__data[p] !== 'object' && self.__data[p]) { try { - self.__data[propertyName] = JSON.parse(self.__data[propertyName] + ''); + self.__data[p] = JSON.parse(self.__data[p] + ''); } catch (e) { - self.__data[propertyName] = String(self.__data[propertyName]); + self.__data[p] = String(self.__data[p]); } } if (type.name === 'Array' || Array.isArray(type)) { - if (!(self.__data[propertyName] instanceof List) - && self.__data[propertyName] !== undefined - && self.__data[propertyName] !== null ) { - self.__data[propertyName] = List(self.__data[propertyName], type, self); + if (!(self.__data[p] instanceof List) + && self.__data[p] !== undefined + && self.__data[p] !== null ) { + self.__data[p] = List(self.__data[p], type, self); } } } - - }); - - function getDefault(propertyName) { - var def = properties[propertyName]['default']; - if (def !== undefined) { - if (typeof def === 'function') { - return def(); - } else { - return def; - } - } else { - return undefined; - } } - this.trigger('initialize'); }; @@ -242,7 +228,7 @@ ModelBaseClass.toString = function () { * @param {Boolean} onlySchema Restrict properties to dataSource only. Default is false. If true, the function returns only properties defined in the schema; Otherwise it returns all enumerable properties. */ ModelBaseClass.prototype.toObject = function (onlySchema, removeHidden) { - if(onlySchema === undefined) { + if (onlySchema === undefined) { onlySchema = true; } var data = {}; @@ -250,47 +236,63 @@ ModelBaseClass.prototype.toObject = function (onlySchema, removeHidden) { var Model = this.constructor; // if it is already an Object - if(Model === Object) return self; + if (Model === Object) { + return self; + } var strict = this.__strict; var schemaLess = (strict === false) || !onlySchema; - this.constructor.forEachProperty(function (propertyName) { - if (removeHidden && Model.isHiddenProperty(propertyName)) { - return; - } - if (typeof self[propertyName] === 'function') { - return; - } - - if (self[propertyName] instanceof List) { - data[propertyName] = self[propertyName].toObject(!schemaLess, removeHidden); - } else if (self.__data.hasOwnProperty(propertyName)) { - if (self[propertyName] !== undefined && self[propertyName] !== null && self[propertyName].toObject) { - data[propertyName] = self[propertyName].toObject(!schemaLess, removeHidden); - } else { - data[propertyName] = self[propertyName]; - } - } else { - data[propertyName] = null; - } - }); + var props = Model.definition.properties; + var keys = Object.keys(props); + var propertyName, val; + for (var i = 0; i < keys.length; i++) { + propertyName = keys[i]; + val = self[propertyName]; + + // Exclude functions + if (typeof val === 'function') { + continue; + } + // Exclude hidden properties + if (removeHidden && Model.isHiddenProperty(propertyName)) { + continue; + } + + if (val instanceof List) { + data[propertyName] = val.toObject(!schemaLess, removeHidden); + } else { + if (val !== undefined && val !== null && val.toObject) { + data[propertyName] = val.toObject(!schemaLess, removeHidden); + } else { + data[propertyName] = val; + } + } + } - var val = null; if (schemaLess) { // Find its own properties which can be set via myModel.myProperty = 'myValue'. // If the property is not declared in the model definition, no setter will be // triggered to add it to __data - for (var propertyName in self) { - if(removeHidden && Model.isHiddenProperty(propertyName)) { + keys = Object.keys(self); + var size = keys.length; + for (i = 0; i < size; i++) { + propertyName = keys[i]; + if (props[propertyName]) { continue; } - if(self.hasOwnProperty(propertyName) && (!data.hasOwnProperty(propertyName))) { - val = self[propertyName]; + if (propertyName.indexOf('__') === 0) { + continue; + } + if (removeHidden && Model.isHiddenProperty(propertyName)) { + continue; + } + val = self[propertyName]; + if (val !== undefined && data[propertyName] === undefined) { if (typeof val === 'function') { continue; } - if (val !== undefined && val !== null && val.toObject) { + if (val !== null && val.toObject) { data[propertyName] = val.toObject(!schemaLess, removeHidden); } else { data[propertyName] = val; @@ -298,15 +300,25 @@ ModelBaseClass.prototype.toObject = function (onlySchema, removeHidden) { } } // Now continue to check __data - for (propertyName in self.__data) { - if (!data.hasOwnProperty(propertyName)) { - if(removeHidden && Model.isHiddenProperty(propertyName)) { + keys = Object.keys(self.__data); + size = keys.length; + for (i = 0; i < size; i++) { + propertyName = keys[i]; + if (propertyName.indexOf('__') === 0) { + continue; + } + if (data[propertyName] === undefined) { + if (removeHidden && Model.isHiddenProperty(propertyName)) { continue; } - val = self.hasOwnProperty(propertyName) ? self[propertyName] : self.__data[propertyName]; + var ownVal = self[propertyName]; + // The ownVal can be a relation function + val = (ownVal !== undefined && (typeof ownVal !== 'function')) + ? ownVal : self.__data[propertyName]; if (typeof val === 'function') { continue; } + if (val !== undefined && val !== null && val.toObject) { data[propertyName] = val.toObject(!schemaLess, removeHidden); } else { @@ -319,12 +331,20 @@ ModelBaseClass.prototype.toObject = function (onlySchema, removeHidden) { return data; }; -ModelBaseClass.isHiddenProperty = function(propertyName) { +ModelBaseClass.isHiddenProperty = function (propertyName) { var Model = this; var settings = Model.definition && Model.definition.settings; - var hiddenProperties = settings && settings.hidden; - if(hiddenProperties) { - return ~hiddenProperties.indexOf(propertyName); + var hiddenProperties = settings && (settings.hiddenProperties || settings.hidden); + if (Array.isArray(hiddenProperties)) { + // Cache the hidden properties as an object for quick lookup + settings.hiddenProperties = {}; + for (var i = 0; i < hiddenProperties.length; i++) { + settings.hiddenProperties[hiddenProperties[i]] = true; + } + hiddenProperties = settings.hiddenProperties; + } + if (hiddenProperties) { + return hiddenProperties[propertyName]; } else { return false; } @@ -340,16 +360,6 @@ ModelBaseClass.prototype.fromObject = function (obj) { } }; -/** - * Checks is property changed based on current property and initial value - * - * @param {String} propertyName Property name - * @return Boolean - */ -ModelBaseClass.prototype.propertyChanged = function propertyChanged(propertyName) { - return this.__data[propertyName] !== this.__dataWas[propertyName]; -}; - /** * Reset dirty attributes. * This method does not perform any database operations; it just resets the object to its @@ -361,9 +371,6 @@ ModelBaseClass.prototype.reset = function () { if (k !== 'id' && !obj.constructor.dataSource.definitions[obj.constructor.modelName].properties[k]) { delete obj[k]; } - if (obj.propertyChanged(k)) { - obj[k] = obj[k + '$was']; - } } }; diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index d1d2b0a8..3fbba174 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1299,15 +1299,24 @@ describe('Load models from json', function () { customer.should.not.have.property('bio'); // The properties are defined at prototype level - assert.equal(Object.keys(customer).length, 0); + assert.equal(Object.keys(customer).filter(function (k) { + // Remove internal properties + return k.indexOf('__') === -1; + }).length, 0); var count = 0; for (var p in customer) { + if (p.indexOf('__') === 0) { + continue; + } if (typeof customer[p] !== 'function') { count++; } } assert.equal(count, 7); // Please note there is an injected id from User prototype - assert.equal(Object.keys(customer.toObject()).length, 6); + assert.equal(Object.keys(customer.toObject()).filter(function (k) { + // Remove internal properties + return k.indexOf('__') === -1; + }).length, 6); done(null, customer); }); diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 5fd6ce84..d31b3b9d 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -133,14 +133,11 @@ describe('manipulation', function () { Person.findOne(function (err, p) { should.not.exist(err); p.name = 'Hans'; - p.propertyChanged('name').should.be.true; p.save(function (err) { should.not.exist(err); - p.propertyChanged('name').should.be.false; Person.findOne(function (err, p) { should.not.exist(err); p.name.should.equal('Hans'); - p.propertyChanged('name').should.be.false; done(); }); }); @@ -157,10 +154,8 @@ describe('manipulation', function () { p.name = 'Nana'; p.save(function (err) { should.exist(err); - p.propertyChanged('name').should.be.true; p.save({validate: false}, function (err) { should.not.exist(err); - p.propertyChanged('name').should.be.false; done(); }); }); @@ -244,10 +239,7 @@ describe('manipulation', function () { person = new Person({name: hw}); person.name.should.equal(hw); - person.propertyChanged('name').should.be.false; person.name = 'Goodbye, Lenin'; - person.name$was.should.equal(hw); - person.propertyChanged('name').should.be.true; (person.createdAt >= now).should.be.true; person.isNewRecord().should.be.true; });