diff --git a/README.md b/README.md index 7be62f2ce..0e789f975 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ If you create a MongoDB data source using the data source generator as described ## Creating a MongoDB data source -Use the [Data source generator](http://loopback.io/doc/en/lb3/Data-source-generator.html) to add a MongoDB data source to your application. +Use the [Data source generator](http://loopback.io/doc/en/lb3/Data-source-generator.html) to add a MongoDB data source to your application. The generator will prompt for the database server hostname, port, and other settings required to connect to a MongoDB database. It will also run the `npm install` command above for you. @@ -64,7 +64,7 @@ If you run a MongoDB with authentification ([Docker's example here](https://gith `$pop`, `$pullAll`, `$pull`, `$pushAll`, `$push`, and `$bit`. Default is `false`. - **enableGeoIndexing**: Set to `true` to enable 2dsphere indexing for model properties of type `GeoPoint`. This allows for indexed ```near``` queries. Default is `false`. -- **lazyConnect**: +- **lazyConnect**: - Default is `false`. - If set to `true`, the database instance will not be attached to the datasource and the connection is deferred. - It will try to establish the connection automatically once users hit the endpoint. If the mongodb server is offline, the app will start, however, the endpoints will not work. @@ -74,7 +74,7 @@ of type `GeoPoint`. This allows for indexed ```near``` queries. Default is `fal ### Setting the url property in datasource.json -You can set the `url` property to a connection URL in `datasources.json` to override individual connection parameters such as `host`, `user`, and `password`. +You can set the `url` property to a connection URL in `datasources.json` to override individual connection parameters such as `host`, `user`, and `password`. Additionally, you can override the global `url` property in environment-specific data source configuration files, for example for production in `datasources.production.json`, and use the individual connection parameters `host`, `user`, `password`, and `port`. To do this, you _must_ set `url` to `false`, null, or “” (empty string). If you set `url` to `undefined` or remove the `url` property altogether, the override will not work. @@ -90,7 +90,7 @@ For example, for production, use `datasources.production.json` as follows (for e "password": "mypassword", "name": "mydb", "user": "me", - "connector": "mongodb" + "connector": "mongodb" } ``` @@ -290,11 +290,29 @@ myModelName.find( {where: {id: {inq: ['59460487e9532ae90c324b59', '59460487e9532ae90c324b5a']}}}, {strictObjectIDCoercion: true}, function(err, result) { - // ... + // ... } ) ``` +## dataType: 'ObjectID' + +You can set a model property's `mongodb` property definition `dataType` to "ObjectID" to enforce ObjectID coercion +irrespective of the `strictObjectIDCoercion` setting. + +In the following example, the `id` and `xid` will be coerced to `ObjectID` even if `strictObjectIDCoercion` is set to true. + +```js +const User = ds.createModel( + 'user', + { + id: {type: String, id: true, mongodb: {dataType: 'ObjectID'}}, + xid: {type: String, mongodb: {dataType: 'ObjectID'}} + }, + {strictObjectIDCoercion: true} +); +``` + ## Release notes * 1.1.7 - Do not return MongoDB-specific _id to client API, except if specifically specified in the model definition diff --git a/lib/mongodb.js b/lib/mongodb.js index 918b9cfd1..2ab9299c7 100644 --- a/lib/mongodb.js +++ b/lib/mongodb.js @@ -18,6 +18,9 @@ const Connector = require('loopback-connector').Connector; const debug = require('debug')('loopback:connector:mongodb'); const Decimal128 = mongodb.Decimal128; +const ObjectIdValueRegex = /^[0-9a-fA-F]{24}$/; +const ObjectIdTypeRegex = /objectid/i; + exports.ObjectID = ObjectID; /*! * Convert the id to be a BSON ObjectID if it is compatible @@ -35,7 +38,7 @@ function ObjectID(id) { // MongoDB's ObjectID constructor accepts number, 12-byte string or 24-byte // hex string. For LoopBack, we only allow 24-byte hex string, but 12-byte // string such as 'line-by-line' should be kept as string - if (/^[0-9a-fA-F]{24}$/.test(id)) { + if (ObjectIdValueRegex.test(id)) { return bson.ObjectID(id); } else { return id; @@ -180,6 +183,7 @@ function MongoDB(settings, dataSource) { } this.dataSource = dataSource; + if ( this.settings.enableOptimisedfindOrCreate === true || this.settings.enableOptimisedFindOrCreate === true || @@ -412,11 +416,11 @@ MongoDB.prototype.fromDatabase = function(modelName, data) { * @param {Object} data The JSON data to convert */ MongoDB.prototype.toDatabase = function(modelName, data) { - const modelInstance = this._models[modelName].model; - const props = this._models[modelName].properties; + const modelCtor = this._models[modelName]; + const props = modelCtor.properties; if (this.settings.enableGeoIndexing !== true) { - visitAllProperties(data, modelInstance, coerceDecimalProperty); + visitAllProperties(data, modelCtor, coercePropertyValue); // Override custom column names data = this.fromPropertyToDatabaseNames(modelName, data); return data; @@ -433,7 +437,7 @@ MongoDB.prototype.toDatabase = function(modelName, data) { } } - visitAllProperties(data, modelInstance, coerceDecimalProperty); + visitAllProperties(data, modelCtor, coercePropertyValue); // Override custom column names data = this.fromPropertyToDatabaseNames(modelName, data); if (debug.enabled) debug('toDatabase data: ', util.inspect(data)); @@ -528,6 +532,7 @@ MongoDB.prototype.coerceId = function(modelName, id, options) { // Type conversion for id const idProp = self.getPropertyDefinition(modelName, idName); + if (idProp && typeof idProp.type === 'function') { if (!(idValue instanceof idProp.type)) { idValue = idProp.type(id); @@ -537,9 +542,8 @@ MongoDB.prototype.coerceId = function(modelName, id, options) { } } - if (self.isObjectIDProperty(modelName, idProp, idValue, options)) { - idValue = ObjectID(idValue); - } + const modelCtor = this._models[modelName]; + idValue = coerceToObjectId(modelCtor, idProp, idValue); } return idValue; }; @@ -841,7 +845,7 @@ MongoDB.prototype.updateOrCreate = function updateOrCreate( /** * Replace model instance if it exists or create a new one if it doesn't * - * @param {String} modelName The name of the model + * @param {String} modelName The model name * @param {Object} data The model instance data * @param {Object} options The options object * @param {Function} [cb] The callback function @@ -942,12 +946,12 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) { const prop = self.getPropertyDefinition(modelName, propName); - const isDecimal = prop && - prop.mongodb && prop.mongodb.dataType && - prop.mongodb.dataType.toLowerCase() === 'decimal128'; - if (isDecimal) { - cond = Decimal128.fromString(cond); - debug('buildWhere decimal value: %s, constructor name: %s', cond, cond.constructor.name); + if (prop && prop.mongodb && typeof prop.mongodb.dataType === 'string') { + const isDecimal = prop.mongodb.dataType.toLowerCase() === 'decimal128'; + if (isDecimal) { + cond = Decimal128.fromString(cond); + debug('buildWhere decimal value: %s, constructor name: %s', cond, cond.constructor.name); + } } // Convert property to database column name @@ -960,6 +964,9 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) { spec = Object.keys(cond)[0]; cond = cond[spec]; } + + const modelCtor = self._models[modelName]; + if (spec) { if (spec === 'between') { query[k] = {$gte: cond[0], $lte: cond[1]}; @@ -967,7 +974,7 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) { cond = [].concat(cond || []); query[k] = { $in: cond.map(function(x) { - if (self.isObjectIDProperty(modelName, prop, x, options)) + if (isObjectIDProperty(modelCtor, prop, x, options)) return ObjectID(x); return x; }), @@ -976,7 +983,7 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) { cond = [].concat(cond || []); query[k] = { $nin: cond.map(function(x) { - if (self.isObjectIDProperty(modelName, prop, x, options)) + if (isObjectIDProperty(modelCtor, prop, x, options)) return ObjectID(x); return x; }), @@ -1010,7 +1017,7 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) { // Null: 10 query[k] = {$type: 10}; } else { - if (self.isObjectIDProperty(modelName, prop, cond, options)) { + if (isObjectIDProperty(modelCtor, prop, cond, options)) { cond = ObjectID(cond); } query[k] = cond; @@ -1217,8 +1224,6 @@ MongoDB.prototype.getDatabaseColumnName = function(model, propName) { const prop = model.properties[propName] || {}; - // console.log('getDatabaseColumnName', propName, prop); - // Try mongo overrides if (prop.mongodb) { propName = @@ -1234,8 +1239,6 @@ MongoDB.prototype.getDatabaseColumnName = function(model, propName) { propName = prop.columnName || prop.column || propName; } - // Done - // console.log('->', propName); return propName; }; @@ -1870,30 +1873,72 @@ MongoDB.prototype.ping = function(cb) { } }; +// Case insensitive check if a string looks like "ObjectID" +function typeIsObjectId(input) { + if (!input) return false; + return typeof input === 'string' && input.match(ObjectIdTypeRegex); +} + +// Determine if a property must be stored as ObjectID +function isStoredAsObjectID(propDef) { + if (!propDef) return false; + + if (propDef.mongodb) { + if (ObjectIdTypeRegex.test(propDef.mongodb.dataType)) return true; + } else if (propDef.type) { + if (typeof propDef.type === 'string' && typeIsObjectId(propDef.type)) return true; + else if (Array.isArray(propDef.type)) { + if (propDef.type[0] === ObjectID || typeIsObjectId(propDef.type[0])) { + return true; + } + } + } + return false; +} + +// Determine if strictObjectIDCoercion should be enabled +function isStrictObjectIDCoercionEnabled(modelCtor, options) { + const settings = modelCtor.settings; + return (settings && settings.strictObjectIDCoercion) || + (modelCtor.model && modelCtor.model.getConnector().settings.strictObjectIDCoercion) || + options && + options.strictObjectIDCoercion; +} + +// Tries to coerce a property into ObjectID after checking multiple conditions +function coerceToObjectId(modelCtor, propDef, propValue) { + if (isStoredAsObjectID(propDef)) { + if (isObjectIDProperty(modelCtor, propDef, propValue)) { + return ObjectID(propValue); + } else { + throw new Error(`${propValue} is not an ObjectID string`); + } + } else if (isStrictObjectIDCoercionEnabled(modelCtor)) { + if (isObjectIDProperty(modelCtor, propDef, propValue)) { + return ObjectID(propValue); + } + } else if (ObjectIdValueRegex.test(propValue)) { + return ObjectID(propValue); + } + return propValue; +} + /** * Check whether the property is an ObjectID (or Array thereof) * */ -MongoDB.prototype.isObjectIDProperty = function(modelName, prop, value, options) { - if ( - prop && - (prop.type === ObjectID || - (Array.isArray(prop.type) && prop.type[0] === ObjectID)) - ) { +function isObjectIDProperty(modelCtor, propDef, value, options) { + if (!propDef) return false; + + if (typeof value === 'string' && value.match(ObjectIdValueRegex)) { + if (isStoredAsObjectID(propDef)) return true; + else return !isStrictObjectIDCoercionEnabled(modelCtor, options); + } else if (value instanceof ObjectID) { return true; - } else if ('string' === typeof value) { - const settings = this._models[modelName] && this._models[modelName].settings; - options = options || {}; - const strict = - (settings && settings.strictObjectIDCoercion) || - this.settings.strictObjectIDCoercion || - options.strictObjectIDCoercion; - if (strict) return false; // unless explicitly typed, don't coerce - return /^[0-9a-fA-F]{24}$/.test(value); } else { return false; } -}; +} function sanitizeFilter(filter, options) { options = Object.assign({}, options); @@ -2001,15 +2046,14 @@ function optimizedFindOrCreate(modelName, filter, data, options, callback) { } /** - * * @param {*} data Plain Data Object for the matching property definition(s) - * @param {*} modelCtorOrDef Model constructor or definition + * @param {*} modelCtor Model constructor * @param {*} visitor A callback function which takes a property value and * definition to apply custom property coercion */ -function visitAllProperties(data, modelCtorOrDef, visitor) { +function visitAllProperties(data, modelCtor, visitor) { if (data === null || data === undefined) return; - const modelProps = modelCtorOrDef.properties ? modelCtorOrDef.properties : modelCtorOrDef.definition.properties; + const modelProps = modelCtor.properties ? modelCtor.properties : modelCtor.definition.properties; const allProps = new Set(Object.keys(data).concat(Object.keys(modelProps))); for (const p of allProps) { const value = data[p]; @@ -2024,28 +2068,47 @@ function visitAllProperties(data, modelCtorOrDef, visitor) { visitAllProperties(value, def.type.definition, visitor); } } else { - visitor(value, def, (newValue) => { data[p] = newValue; }); + visitor(modelCtor, value, def, (newValue) => { data[p] = newValue; }); } continue; } } /** - * - * @param {*} propValue Property value to coerce into a Decimal128 value - * @param {*} propDef Property definition to check if property is MongoDB - * Decimal128 type + * @param {*} modelCtor Model constructor + * @param {*} propValue Property value to coerce into special types supported by the connector + * @param {*} propDef Property definition to check if property is for MongoDB */ -function coerceDecimalProperty(propValue, propDef, setValue) { - let updatedValue; - if (hasDataType('decimal128', propDef)) { - if (Array.isArray(propValue)) { - updatedValue = propValue.map(val => Decimal128.fromString(val)); - return setValue(updatedValue); - } else { - updatedValue = Decimal128.fromString(propValue); - return setValue(updatedValue); +function coercePropertyValue(modelCtor, propValue, propDef, setValue) { + let coercedValue; + // Process only mongo-specific data types + if (propDef && propDef.mongodb && propDef.mongodb.dataType) { + const dataType = propDef.mongodb.dataType; + if (typeof dataType === 'string') { + if (hasDataType('decimal128', propDef)) { + if (Array.isArray(propValue)) { + coercedValue = propValue.map(val => Decimal128.fromString(val)); + return setValue(coercedValue); + } else { + coercedValue = Decimal128.fromString(propValue); + return setValue(coercedValue); + } + } else if (typeIsObjectId(dataType)) { + if (isObjectIDProperty(modelCtor, propDef, propValue)) { + coercedValue = ObjectID(propValue); + return setValue(coercedValue); + } else { + throw new Error(`${propValue} is not an ObjectID string`); + } + } + } else if (dataType instanceof ObjectID) { + coercedValue = ObjectID(propValue); + return setValue(coercedValue); } + } else { + // Object ID coercibility depends on multiple factors, let coerceToObjectId() handle it + propValue = coerceToObjectId(modelCtor, propDef, propValue); + setValue(propValue); } } @@ -2072,4 +2135,3 @@ function hasDataType(dataType, propertyDef) { propertyDef.mongodb.dataType && propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase(); } - diff --git a/test/id.test.js b/test/id.test.js index c7b218b73..90d630c1a 100644 --- a/test/id.test.js +++ b/test/id.test.js @@ -237,7 +237,6 @@ describe('mongodb default id name', function() { age: 30, }, function(err, customer) { - // console.log(customer); customer.should.have.property('id'); Customer1.findById(customer.id, function(err, customer1) { customer1.id.should.eql(customer.id); @@ -247,3 +246,165 @@ describe('mongodb default id name', function() { ); }); }); + +describe('strictObjectIDCoercion', function() { + const ObjectID = ds.connector.getDefaultIdType(); + const objectIdLikeString = '7cd2ad46ffc580ba45d3cb1f'; + + context('set to false (default)', function() { + const User = ds.createModel( + 'user1', + { + id: {type: String, id: true}, + name: String, + } + ); + + beforeEach(function(done) { + User.deleteAll(done); + }); + + it('should coerce to ObjectID', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + }); + + it('should find model with ObjectID id', async function() { + const user = await User.create({name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + const found = await User.findById(user.id); + found.toObject().name.should.equal('John'); + }); + + it('should find model with ObjectID-like id', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + user.id.should.eql(ObjectID(objectIdLikeString)); + const found = await User.findById(objectIdLikeString); + found.toObject().name.should.equal('John'); + }); + + it('should find model with string id', async function() { + const user = await User.create({id: 'a', name: 'John'}); + user.id.should.be.an.instanceOf(String); + user.id.should.equal('a'); + const found = await User.findById('a'); + found.toObject().name.should.equal('John'); + }); + }); + + context('set to true', function() { + const User = ds.createModel( + 'user2', + { + id: {type: String, id: true}, + name: String, + }, + {strictObjectIDCoercion: true} + ); + + beforeEach(function(done) { + User.deleteAll(done); + }); + + it('should not coerce to ObjectID', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + user.id.should.equal(objectIdLikeString); + }); + + it('should not find model with ObjectID id', async function() { + const user = await User.create({name: 'John'}); + const found = await User.findById(user.id); + (found === null).should.be.true(); + }); + + it('should find model with ObjectID-like string id', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + user.id.should.not.be.an.instanceOf(ds.ObjectID); + user.id.should.eql(objectIdLikeString); + const found = await User.findById(objectIdLikeString); + found.toObject().name.should.equal('John'); + }); + + it('should find model with string id', async function() { + const user = await User.create({id: 'a', name: 'John'}); + user.id.should.be.an.instanceOf(String); + user.id.should.equal('a'); + const found = await User.findById('a'); + found.toObject().name.should.equal('John'); + }); + }); + + context('set to true, id type set to ObjectID', function() { + const User = ds.createModel( + 'user3', + { + id: {type: String, id: true, mongodb: {dataType: 'ObjectID'}}, + name: String, + }, + {strictObjectIDCoercion: true} + ); + + const User1 = ds.createModel( + 'user4', + { + id: {type: String, id: true, mongodb: {dataType: 'objectid'}}, + name: String, + }, + {strictObjectIDCoercion: true} + ); + + beforeEach(function(done) { + User.deleteAll(); + User1.deleteAll(done); + }); + + it('should coerce to ObjectID', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + }); + + it('should coerce to ObjectID (lowercase)', async function() { + const user = await User1.create({id: objectIdLikeString, name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + }); + + it('should throw if id is not a ObjectID-like string', async function() { + try { + await User.create({id: 'abc', name: 'John'}); + } catch (e) { + e.message.should.match(/not an ObjectID string/); + } + }); + + it('should find model with ObjectID id', async function() { + const user = await User.create({name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + const found = await User.findById(user.id); + found.toObject().name.should.equal('John'); + }); + + // This works by coercing string to ObjectID, overriding `strictObjectIDCoercion: true` + it('should find model with ObjectID-like id', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + user.id.should.be.an.instanceOf(ds.ObjectID); + user.id.should.eql(ObjectID(objectIdLikeString)); + const found = await User.findById(objectIdLikeString); + found.toObject().name.should.equal('John'); + }); + + it('should update model with ObjectID id', async function() { + const user = await User.create({name: 'John'}); + await User.update({id: user.id, name: 'Jon'}); + const found = await User.findById(user.id); + found.name.should.equal('Jon'); + }); + + it('should update model with ObjectID-like id', async function() { + const user = await User.create({id: objectIdLikeString, name: 'John'}); + await User.update({id: objectIdLikeString, name: 'Jon'}); + const found = await User.findById(objectIdLikeString); + found.name.should.equal('Jon'); + }); + }); +}); diff --git a/test/objectid.test.js b/test/objectid.test.js index 5d31a1258..c4ef65f78 100644 --- a/test/objectid.test.js +++ b/test/objectid.test.js @@ -7,11 +7,13 @@ require('./init.js'); -let ds, Book, Chapter; +let Book, Chapter; +const ds = global.getDataSource(); +const ObjectID = ds.connector.getDefaultIdType(); +const objectIDLikeString = '7cd2ad46ffc580ba45d3cb1f'; describe('ObjectID', function() { before(function() { - ds = global.getDataSource(); Book = ds.define('Book'); Chapter = ds.define('Chapter'); Book.hasMany('chapters'); @@ -33,7 +35,7 @@ describe('ObjectID', function() { it('should convert 24 byte hex string as ObjectID', function() { const ObjectID = ds.connector.getDefaultIdType(); - const str = '52fcef5c0325ace8dcb7a0bd'; + const str = objectIDLikeString; ObjectID(str).should.be.an.instanceOf(ds.ObjectID); }); @@ -55,18 +57,19 @@ describe('ObjectID', function() { ObjectID(id).should.be.equal(123); }); - it('coerces ObjectID', function() { - const coercedId = ds.connector.isObjectIDProperty('Book', {}, '52fcef5c0325ace8dcb7a0bd'); - coercedId.should.be.True(); - }); - - it('given strictObjectIDCoercion: true, does not coerce ObjectID', function() { - const coercedId = ds.connector.isObjectIDProperty( - 'Book', - {}, - '52fcef5c0325ace8dcb7a0bd', - {strictObjectIDCoercion: true} - ); - coercedId.should.be.False(); + context('ObjectID type', function() { + it('should throw if value is not an ObjectID', async function() { + Book = ds.createModel( + 'book1', + { + xid: {type: String, mongodb: {dataType: 'objectid'}}, + } + ); + try { + await Book.create({xid: 'x'}); + } catch (e) { + e.message.should.match(/not an ObjectID string/); + } + }); }); });