Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adds mongodb: {dataType: 'ObjectID'} to model properties #517

Merged
merged 1 commit into from
Jun 3, 2019
Merged

feat: adds mongodb: {dataType: 'ObjectID'} to model properties #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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"
}
```

Expand Down Expand Up @@ -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
176 changes: 119 additions & 57 deletions lib/mongodb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -180,6 +183,7 @@ function MongoDB(settings, dataSource) {
}

this.dataSource = dataSource;

if (
this.settings.enableOptimisedfindOrCreate === true ||
this.settings.enableOptimisedFindOrCreate === true ||
Expand Down Expand Up @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand All @@ -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;
};
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to handle prop.mongodb.dataType.toLowerCase() === 'objectid' too, so that string properties stored as ObjectIDs are handled in where queries too. Please start with a unit test that fails now.

I am ok to leave this part out of scope of this pull request if you prefer, but I think it should be implemented before we can call the user story done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given:

const ObjectIdTypeRegex = /objectid/i;

Shouldn't ObjectIdTypeRegex.test() take care of things?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should do it. Can we add a test where we specify the type as dataType: 'objectid'?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't ObjectIdTypeRegex.test() take care of things?

What will happen when the value is not an ObjectID string? IIUC, in your proposal the query will return no records, because the non-ObjectID string will not match any of the ObjectID values stored in the database.

Are we fine with that behavior?

I was thinking that we should reject such requests with 400 Bad Request error instead.

Also, what happens when strictObjectIDCoercion is enabled and the property is defined as {type: 'string', mongodb: {dataType: 'ObjectID'}}. Will we correctly coerce the string value to ObjectID, so that it can match any existing values in the database?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got your point, I was thinking of something else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't ObjectIdTypeRegex.test() take care of things?

What will happen when the value is not an ObjectID string? IIUC, in your proposal the query will return no records, because the non-ObjectID string will not match any of the ObjectID values stored in the database.

Are we fine with that behavior?

I was thinking that we should reject such requests with 400 Bad Request error instead.

Also, what happens when strictObjectIDCoercion is enabled and the property is defined as {type: 'string', mongodb: {dataType: 'ObjectID'}}. Will we correctly coerce the string value to ObjectID, so that it can match any existing values in the database?

I misunderstood this, sorry. Rejecting non-objectID string makes sense to me (I thought this was the behaviour). Also, for the case when strict coercion flag is enabled, we should coerce string values to ObjectID, and also reject/throw an error if we can't (invalid values).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos @hacksparrow is this something we can create a follow-up story on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a follow up story.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@b-admike @hacksparrow @bajtos
Created #545 as I believe it relates to this conversation. This commit and its related code appears to fix the issue related to string-properties-stored-as-ObjectId but does not account for a property that is an Array of ObjectID or Array of ObjectID-like-strings.


// Convert property to database column name
Expand All @@ -960,14 +964,17 @@ 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]};
} else if (spec === 'inq') {
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;
}),
Expand All @@ -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;
}),
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 =
Expand All @@ -1234,8 +1239,6 @@ MongoDB.prototype.getDatabaseColumnName = function(model, propName) {
propName = prop.columnName || prop.column || propName;
}

// Done
// console.log('->', propName);
return propName;
};

Expand Down Expand Up @@ -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) ||
bajtos marked this conversation as resolved.
Show resolved Hide resolved
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);
hacksparrow marked this conversation as resolved.
Show resolved Hide resolved
}
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) {
hacksparrow marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand Down Expand Up @@ -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];
Expand All @@ -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);
}
}

Expand All @@ -2072,4 +2135,3 @@ function hasDataType(dataType, propertyDef) {
propertyDef.mongodb.dataType &&
propertyDef.mongodb.dataType.toLowerCase() === dataType.toLowerCase();
}

Loading