Skip to content

Commit

Permalink
Feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
Hage Yaapa committed Jun 3, 2019
1 parent e6d504b commit 3503be6
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 50 deletions.
75 changes: 41 additions & 34 deletions lib/mongodb.js
View file
Expand Up @@ -18,7 +18,7 @@ const Connector = require('loopback-connector').Connector;
const debug = require('debug')('loopback:connector:mongodb');
const Decimal128 = mongodb.Decimal128;

const ObjectIdRegex = /^[0-9a-fA-F]{24}$/;
const ObjectIdValueRegex = /^[0-9a-fA-F]{24}$/;
const ObjectIdTypeRegex = /objectid/i;

exports.ObjectID = ObjectID;
Expand All @@ -38,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 (ObjectIdRegex.test(id)) {
if (ObjectIdValueRegex.test(id)) {
return bson.ObjectID(id);
} else {
return id;
Expand Down Expand Up @@ -1873,34 +1873,51 @@ MongoDB.prototype.ping = function(cb) {
}
};

// Determine if a property must have an ObjectID value
function mustBeObjectID(propDef) {
// 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 && ObjectIdTypeRegex.test(propDef.mongodb.dataType)) return true;

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 enableStrictObjectIDCoercion(modelCtor, options) {
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 (mustBeObjectID(propDef)) {
if (isStoredAsObjectID(propDef)) {
if (isObjectIDProperty(modelCtor, propDef, propValue)) {
return ObjectID(propValue);
} else {
throw new Error(`${propValue} is not an ObjectID string`);
}
} else if (enableStrictObjectIDCoercion(modelCtor)) {
} else if (isStrictObjectIDCoercionEnabled(modelCtor)) {
if (isObjectIDProperty(modelCtor, propDef, propValue)) {
return ObjectID(propValue);
}
} else if (ObjectIdRegex.test(propValue)) {
} else if (ObjectIdValueRegex.test(propValue)) {
return ObjectID(propValue);
}
return propValue;
Expand All @@ -1911,29 +1928,13 @@ function coerceToObjectId(modelCtor, propDef, propValue) {
*
*/
function isObjectIDProperty(modelCtor, propDef, value, options) {
if (
propDef &&
(propDef.type === ObjectID ||
(Array.isArray(propDef.type) && propDef.type[0] === ObjectID))
) {
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) {
// dataType is specified as ObjectID
if (propDef && propDef.mongodb && ObjectIdTypeRegex.test(propDef.mongodb.dataType)) {
// value looks like an ObjectID, and it should be treated as one
// because `prop.mongodb.dataType` is set to 'ObjectID'
// this condition overrides the `strictObjectIDCoercion` setting
if (value.match(ObjectIdRegex)) return true;
// if dataType is specified as ObjectID, value is expected to an ObjectID-like string
return false;
} else if (!value.match(ObjectIdRegex)) {
// obviously not an ObjectID
return false;
}

// value looks like an ObjectID, determine its nature based on `strictObjectIDCoercion`
// coerce only when strictObjectIDCoercion is disabled
return !enableStrictObjectIDCoercion(modelCtor, options);
} else {
return false;
}
Expand Down Expand Up @@ -2083,7 +2084,6 @@ function coercePropertyValue(modelCtor, propValue, propDef, setValue) {
// Process only mongo-specific data types
if (propDef && propDef.mongodb && propDef.mongodb.dataType) {
const dataType = propDef.mongodb.dataType;
// Data types specified as string values
if (typeof dataType === 'string') {
if (hasDataType('decimal128', propDef)) {
if (Array.isArray(propValue)) {
Expand All @@ -2093,13 +2093,20 @@ function coercePropertyValue(modelCtor, propValue, propDef, setValue) {
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`);
}
}
// ObjectID data type
} else if (ObjectIdTypeRegex.test(dataType)) {
} 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);
}
Expand Down
19 changes: 16 additions & 3 deletions test/id.test.js
View file
Expand Up @@ -345,18 +345,31 @@ describe('strictObjectIDCoercion', function() {
{strictObjectIDCoercion: true}
);

const User1 = ds.createModel(
'user4',
{
id: {type: String, id: true, mongodb: {dataType: 'objectid'}},
name: String,
},
{strictObjectIDCoercion: true}
);

beforeEach(function(done) {
User.deleteAll(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() {
// Why is this not working?
// await User.create({id: 'abc', name: 'John'}).should.be.rejectedWith(/not an ObjectID string/);
try {
await User.create({id: 'abc', name: 'John'});
} catch (e) {
Expand Down
26 changes: 13 additions & 13 deletions test/objectid.test.js
View file
Expand Up @@ -57,19 +57,19 @@ describe('ObjectID', function() {
ObjectID(id).should.be.equal(123);
});

context('strictObjectIDCoercion', function() {
context('set to true', function() {
it('should not coerce to ObjectID', async function() {
Book = ds.createModel(
'book',
{
xid: {type: String},
},
{strictObjectIDCoercion: true}
);
const book = await Book.create({xid: objectIDLikeString});
book.xid.should.equal(objectIDLikeString);
});
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/);
}
});
});
});

0 comments on commit 3503be6

Please sign in to comment.