Alt SPIKE: New objectid

SPIKE.md

@@ -0,0 +1,111 @@
+# Spike: Robust handling of ObjectID type for MongoDB
+
+Ref: https://github.com/strongloop/loopback-next/issues/3456
+
+## Table of contents
+
+- [The problem](#the-problem)
+- [Proposed solution](#proposed-solution)
+- [Approaches](#approaches)
+- [Developer Notes](#developer-notes)
+- [Implementation Proposal](#implementation)
+- [Follow-up tasks](#follow-up-tasks)
+
+## The Problem
+
+`ObjectID` is a native MongoDB datatype, which is used as the datatype of its primary key (_id_) field; other fields can also be of `ObjectID` type.
+
+MongoDB distinguishes between id values that are a `strin` and an `ObjectID`. A query using a `string` value does not match records having `ObjectID` value and vice versa. This is especially important for foreign keys, e.g. when `Order` property `customerId` is referencing an `id` of a `Customer`, then both properties must always hold `ObjectID` values. If one of the property (typically `customerId`) is stored as a `string`, then LB relations stop working.
+
+In the current version of Juggler, the interpretation of `ObjectID` field is a sub-optimal experience supported by a complex
+determination process in an already complex codebase.
+
+Examples of bad experiences: 
+
+1. Fields need not be set to `mongodb: {dataType: 'ObjectID'}` to be interpreted as `ObjectID`.
+2. Unless `strictObjectIDCoercion` is set to `true`, any string
+that looks like an `ObjectID` will automatically get converted to `ObjectID` and you won't be able to find the object if you
+tried searching the database by that string, because it is not a string any more.
+3. To add to the confusion, this behavior can be configured at model and property levels.
+
+Complexity is sometimes unavoidable, but a less complex state is always the desired state. It creates better experience for
+the consumers, and makes development and maintenance a more pleasant experience for the creators.
+
+## Proposed Solution
+
+Database-specific types like `ObjectID` and `Decimal128` should be considered as an implementation detail of a database connector.
+
+Values of these types must not leak from the connector to Juggler and user code, code outside the connector should see a database-agnostic type like a `string`. The connector must always convert incoming values (property values, filtering queries) from database-agnostic type to database-specific type on input, and convert database-specific types back to database-agnostic type on output.
+
+Specifically for `ObjectID`:
+- Properties that should be stored as `ObjectID` in MongoDB must be defined as `{type: 'string', mongodb: {dataType: 'ObjectID'}}`.
+- There will be no auto-magic coercion from `string` to `ObjectID` for other properties (those defined without `dataType: 'ObjectID'`).
+
+Users will no longer have to deal with `ObjectID` anywhere in their experience with
+LoopBack/Juggler except only in one location - the model file.
+
+## Approaches
+
+### 1. Top level ObjectID
+
+A top level `dataType: 'ObjectID'` was suggested, but it was discarded for the following reason:
+
+1. If a top level `ObjectID` is supported for MongoDB, then database-specific types like `MULTIPOLYGON`, `VARCHAR`, `NCLOB` etc
+may also be required to be supported. Considering the number of database-specific types and the number of databases we support,
+we will end up with a really huge list.
+2. Inflexible when switching connectors. Cannot do this:
+
+```js
+{
+  type: 'number',
+  jsonSchema: {
+    format: 'int32',
+  },
+  mysql: {
+    dataType: 'bit',
+  },
+  postgresql: {
+    dataType: 'bit',
+    length: 1,
+  }
+}
+```
+
+### 2. mongodb: {dataType: 'objectID'}
+
+Setting a property to `mongodb: {dataType: 'ObjectID'}`, as it is already being done, is the better approach for marking the 
+property as an `ObjectID`. The same approach is also already used by `Decimal128` -  `mongodb: {dataType: 'decimal128'}`.
+
+## Proof of concept
+
+The tests in `test/new-objectid.test.js` is a demonstration of the behavior of the new `mongodb: {dataType: 'ObjectID'}` property.
+
+## Implementation
+
+All changes are limited to `loopback-connetor-mongodb` only, Juggler is not affected.
+
+- Before querying database:
+  - Walk through the data object and convert all properties defined as `mongodb: {dataType: 'ObjectID'}`.
+- When receiving data from database
+  - Walk through the data object and convert all properties defined as `mongodb: {dataType: 'ObjectID'}` to `String`
+- Primary key:
+  - if not specified, will be interpreted as `mongodb: {dataType: 'ObjectID'}`
+  - if specified with `{id: true}`, need not add `mongodb: {dataType: 'ObjectID'}` (it will be automatically inferred)
+- There are some pieces of code that can be refactored, which involves the new changes. I think we should refactor it now now, than accumulate tech debt.
+
+
+## Notes
+
+- The changes will break compatibility with LB 3.x. Which is fine, we just need to clearly document it and remove `juggler@3` from the test suite.
+- Add a "Long Term Support" section to README (see [strongloop/loopback](https://github.com/strongloop/loopback#module-long-term-support-policy) for inspiration) and also explain which connector versions are compatible with which LoopBack versions.
+
+## Follow-up Tasks
+
+1. [Remove MongoDB.prototype.getDefaultIdType](https://github.com/strongloop/loopback-connector-mongodb/issues/600)
+2. [ObjectID string interface](https://github.com/strongloop/loopback-connector-mongodb/issues/598)
+3. [SPIKE: Connector level model validation](https://github.com/strongloop/loopback-connector-mongodb/issues/599)
+4. [Primary key property not validated in `.create()`](https://github.com/strongloop/loopback-datasource-juggler/issues/1868)
+5. [Refactor DAO methods](https://github.com/strongloop/loopback-datasource-juggler/issues/1867)
+
+4 and 5 are somewhat related but not a requirement, they were discovered and created out of the [first spike](https://github.com/strongloop/loopback-next/issues/3456#issuecomment-673421592).
+

lib/mongodb.js

@@ -348,9 +348,10 @@ MongoDB.prototype.getTypes = function() {
   return ['db', 'nosql', 'mongodb'];
 };
 
-MongoDB.prototype.getDefaultIdType = function() {
-  return ObjectID;
-};
+// SPIKE: get rid of this. TODO: remove other redundant code too
+// MongoDB.prototype.getDefaultIdType = function() {
+//   return ObjectID;
+// };
 
 /**
  * Get collection name for a given model
@@ -384,12 +385,15 @@ MongoDB.prototype.collection = function(modelName) {
  * @param {String} modelName The model name
  * @param {Object} data The data from DB
  */
+// SPIKE: This is must be called on every query response 
 MongoDB.prototype.fromDatabase = function(modelName, data) {
   if (!data) {
     return null;
   }
+
   const modelInfo = this._models[modelName] || this.dataSource.modelBuilder.definitions[modelName];
   const props = modelInfo.properties;
+
   for (const p in props) {
     const prop = props[p];
     if (prop && prop.type === Buffer) {
@@ -420,6 +424,12 @@ MongoDB.prototype.fromDatabase = function(modelName, data) {
 
   data = this.fromDatabaseToPropertyNames(modelName, data);
 
+  const modelCtor = this._models[modelName];
+  // SPIKE: modelCtor is not applicable for nested objects
+  // SPIKE: required because of line 421
+  if (modelCtor) {
+    visitAllProperties(data, modelCtor, uncoercePropertyValue);
+  }
   return data;
 };
 
@@ -429,10 +439,12 @@ MongoDB.prototype.fromDatabase = function(modelName, data) {
  * @param {String} modelName The model name
  * @param {Object} data The JSON data to convert
  */
+// SPIKE: This must be called before every query
 MongoDB.prototype.toDatabase = function(modelName, data) {
   const modelCtor = this._models[modelName];
   const props = modelCtor.properties;
 
+  // SPIKE: we shouldn't need to call visitAllProperties twice!
   if (this.settings.enableGeoIndexing !== true) {
     visitAllProperties(data, modelCtor, coercePropertyValue);
     // Override custom column names
@@ -555,10 +567,8 @@ MongoDB.prototype.coerceId = function(modelName, id, options) {
         idValue = id;
       }
     }
-
-    const modelCtor = this._models[modelName];
-    idValue = coerceToObjectId(modelCtor, idProp, idValue);
   }
+
   return idValue;
 };
 
@@ -609,6 +619,8 @@ MongoDB.prototype.create = function(modelName, data, options, callback) {
       // Restore the data object
       delete data._id;
       data[idName] = idValue;
+      // SPIKE
+      //console.log(data)
       callback(err, err ? null : idValue);
     });
   });
@@ -973,9 +985,12 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) {
       if (Decimal128TypeRegex.test(propDef.mongodb.dataType)) {
         cond = Decimal128.fromString(cond);
         debug('buildWhere decimal value: %s, constructor name: %s', cond, cond.constructor.name);
-      } else if (isStoredAsObjectID(propDef)) {
+      } else if (hasDataType('objectid', propDef)) {
         cond = ObjectID(cond);
       }
+    // SPIKE: for implicitly autogenerated ids
+    } else if (propDef && propDef[idName]) {
+      cond = ObjectID(cond);
     }
 
     // Convert property to database column name
@@ -1042,7 +1057,7 @@ MongoDB.prototype.buildWhere = function(modelName, where, options) {
         // Null: 10
         query[k] = {$type: 10};
       } else {
-        if (isObjectIDProperty(modelCtor, propDef, cond, options)) {
+        if (hasDataType('objectid', propDef)) {
           cond = ObjectID(cond);
         }
         query[k] = cond;
@@ -1498,6 +1513,25 @@ MongoDB.prototype.count = function count(modelName, where, options, callback) {
   });
 };
 
+// SPIKE: merge this with coerceId
+// Not doing here, the spike time is way past what was timboxed, but we have the general direction
+MongoDB.prototype.coerceIdx = function(modelName, id, options) {
+  // See https://github.com/strongloop/loopback-connector-mongodb/issues/206
+  if (id == null) return id;
+  const self = this;
+  let idValue = id;
+  const idName = self.idName(modelName);
+
+  // Type conversion for id
+  const idProp = self.getPropertyDefinition(modelName, idName);
+
+  if (idProp && idProp.mongodb && idProp.mongodb.dataType.match(/objectid/i)) {
+    idValue = ObjectID(idValue);
+  }
+
+  return idValue;
+};
+
 /**
  * Replace properties for the model instance data
  * @param {String} modelName The model name
@@ -1508,7 +1542,8 @@ MongoDB.prototype.count = function count(modelName, where, options, callback) {
  */
 MongoDB.prototype.replaceById = function replace(modelName, id, data, options, cb) {
   if (this.debug) debug('replace', modelName, id, data);
-  const oid = this.coerceId(modelName, id, options);
+  // SPIKE: all other instances of this.coerceId should be updated 
+  const oid = this.coerceIdx(modelName, id, data, options);
   this.replaceWithOptions(modelName, oid, data, {upsert: false}, function(
     err,
     data,
@@ -1941,73 +1976,6 @@ 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)
- *
- */
-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 mongodb.ObjectID) {
-    return true;
-  } else {
-    return false;
-  }
-}
-
 /**
  * Removes extra dollar sign '$' for operators
  *
@@ -2132,6 +2100,11 @@ function optimizedFindOrCreate(modelName, filter, data, options, callback) {
  */
 function visitAllProperties(data, modelCtor, visitor) {
   if (data === null || data === undefined) return;
+  // SPIKE: for debugging only
+  if (!modelCtor) {
+    console.log(data);
+    throw new Error('Missing modelCtor');
+  }
   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) {
@@ -2172,35 +2145,47 @@ function coercePropertyValue(modelCtor, propValue, propDef, setValue) {
           coercedValue = Decimal128.fromString(propValue);
           return setValue(coercedValue);
         }
-      } else if (typeIsObjectId(dataType)) {
+      } else if (hasDataType('objectid', propDef)) {
         if (Array.isArray(propValue)) {
-          propValue = propValue.map(val => {
-            if (isObjectIDProperty(modelCtor, propDef, val)) {
-              return coercedValue = ObjectID(val);
-            } else {
-              throw new Error(`${val} is not an ObjectID string`);
-            }
-          });
+          propValue = propValue.map(val => ObjectID(val));
           return setValue(propValue);
-        } else if (isObjectIDProperty(modelCtor, propDef, propValue)) {
+        } else {
           coercedValue = ObjectID(propValue);
           return setValue(coercedValue);
-        } else {
-          throw new Error(`${propValue} is not an ObjectID string`);
         }
       }
     }
-  } else {
-    // Object ID coercibility depends on multiple factors, let coerceToObjectId() handle it
-    if (Array.isArray(propValue)) {
-      propValue = propValue.map(val => coerceToObjectId(modelCtor, propDef, val));
-    } else {
-      propValue = coerceToObjectId(modelCtor, propDef, propValue);
+  }
+}
+
+// TODO: use a better name in implementation
+function uncoercePropertyValue(modelCtor, propValue, propDef, setValue) {
+  let coercedValue;
+  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 (hasDataType('objectid', propDef)) {
+        if (Array.isArray(propValue)) {
+          propValue = propValue.map(val => val.toString());
+          return setValue(propValue);
+        } else {
+          coercedValue = propValue.toString();
+          return setValue(coercedValue);
+        }
+      }
     }
-    setValue(propValue);
   }
 }
 
+
 /**
 * A utility function which checks for nested property definitions
 *