Break schemaController dependency. (#1901)

* Break dependency on MongoCollection for updateMany * Move transformWhere usage into MongoTransform * Pass parse schema into transformUpdate * break dependency on schemaController * remove schema parameter * move key name validation up one level * Move validation out of mongo adapter * Move validation into Parse Server and transformUpdate in Mongo Adapter * Update mongo adapter * Use adapter API * use and fix mongo adapter api * Remove/rename stuff * Kill transform in DBController * better imports for transform * Tidy ConfigRouter * Remove schemaController in more places * Remove comment
2016-05-24 17:21:20 -07:00
parent c4499d25e0
commit c9a138d9ca
5 changed files with 89 additions and 116 deletions
--- a/spec/MongoTransform.spec.js
+++ b/spec/MongoTransform.spec.js
@@ -5,26 +5,11 @@ let transform = require('../src/Adapters/Storage/Mongo/MongoTransform');
 let dd = require('deep-diff');
 let mongodb = require('mongodb');

-var dummySchema = {
-    data: {},
-    getExpectedType: function(className, key) {
-      if (key == 'userPointer') {
-        return { type: 'Pointer', targetClass: '_User' };
-      } else if (key == 'picture') {
-        return { type: 'File' };
-      } else if (key == 'location') {
-        return { type: 'GeoPoint' };
-      }
-      return;
-    },
-};
-
-
 describe('parseObjectToMongoObjectForCreate', () => {

  it('a basic number', (done) => {
    var input = {five: 5};
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, {
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, {
      fields: {five: {type: 'Number'}}
    });
    jequal(input, output);
@@ -36,7 +21,7 @@ describe('parseObjectToMongoObjectForCreate', () => {
      createdAt: "2015-10-06T21:24:50.332Z",
      updatedAt: "2015-10-06T21:24:50.332Z"
    };
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, { fields: {} });
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, { fields: {} });
    expect(output._created_at instanceof Date).toBe(true);
    expect(output._updated_at instanceof Date).toBe(true);
    done();
@@ -48,7 +33,7 @@ describe('parseObjectToMongoObjectForCreate', () => {
      objectId: 'myId',
      className: 'Blah',
    };
-    var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, {pointers: [pointer]},{
+    var out = transform.parseObjectToMongoObjectForCreate(null, {pointers: [pointer]},{
      fields: {pointers: {type: 'Array'}}
    });
    jequal([pointer], out.pointers);
@@ -59,14 +44,14 @@ describe('parseObjectToMongoObjectForCreate', () => {
  //have __op delete in a new object. Figure out what this should actually be testing.
  notWorking('a delete op', (done) => {
    var input = {deleteMe: {__op: 'Delete'}};
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, { fields: {} });
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, { fields: {} });
    jequal(output, {});
    done();
  });

  it('basic ACL', (done) => {
    var input = {ACL: {'0123': {'read': true, 'write': true}}};
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, { fields: {} });
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, { fields: {} });
    // This just checks that it doesn't crash, but it should check format.
    done();
  });
@@ -74,7 +59,7 @@ describe('parseObjectToMongoObjectForCreate', () => {
  describe('GeoPoints', () => {
    it('plain', (done) => {
      var geoPoint = {__type: 'GeoPoint', longitude: 180, latitude: -180};
-      var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, {location: geoPoint},{
+      var out = transform.parseObjectToMongoObjectForCreate(null, {location: geoPoint},{
        fields: {location: {type: 'GeoPoint'}}
      });
      expect(out.location).toEqual([180, -180]);
@@ -83,7 +68,7 @@ describe('parseObjectToMongoObjectForCreate', () => {

    it('in array', (done) => {
      var geoPoint = {__type: 'GeoPoint', longitude: 180, latitude: -180};
-      var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, {locations: [geoPoint, geoPoint]},{
+      var out = transform.parseObjectToMongoObjectForCreate(null, {locations: [geoPoint, geoPoint]},{
        fields: {locations: {type: 'Array'}}
      });
      expect(out.locations).toEqual([geoPoint, geoPoint]);
@@ -92,7 +77,7 @@ describe('parseObjectToMongoObjectForCreate', () => {

    it('in sub-object', (done) => {
      var geoPoint = {__type: 'GeoPoint', longitude: 180, latitude: -180};
-      var out = transform.parseObjectToMongoObjectForCreate(dummySchema, null, { locations: { start: geoPoint }},{
+      var out = transform.parseObjectToMongoObjectForCreate(null, { locations: { start: geoPoint }},{
        fields: {locations: {type: 'Object'}}
      });
      expect(out).toEqual({ locations: { start: geoPoint } });
@@ -206,7 +191,7 @@ describe('transform schema key changes', () => {
    var input = {
      somePointer: {__type: 'Pointer', className: 'Micro', objectId: 'oft'}
    };
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, {
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, {
      fields: {somePointer: {type: 'Pointer'}}
    });
    expect(typeof output._p_somePointer).toEqual('string');
@@ -218,7 +203,7 @@ describe('transform schema key changes', () => {
    var input = {
      userPointer: {__type: 'Pointer', className: '_User', objectId: 'qwerty'}
    };
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, {
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, {
      fields: {userPointer: {type: 'Pointer'}}
    });
    expect(typeof output._p_userPointer).toEqual('string');
@@ -233,7 +218,7 @@ describe('transform schema key changes', () => {
        "Kevin": { "write": true }
      }
    };
-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, { fields: {} });
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, { fields: {} });
    expect(typeof output._rperm).toEqual('object');
    expect(typeof output._wperm).toEqual('object');
    expect(output.ACL).toBeUndefined();
@@ -250,7 +235,7 @@ describe('transform schema key changes', () => {
      }
    };

-    var output = transform.parseObjectToMongoObjectForCreate(dummySchema, null, input, { fields: {} });
+    var output = transform.parseObjectToMongoObjectForCreate(null, input, { fields: {} });
    expect(typeof output._acl).toEqual('object');
    expect(output._acl["Kevin"].w).toBeTruthy();
    expect(output._acl["Kevin"].r).toBeUndefined();
--- a/src/Adapters/Storage/Mongo/MongoStorageAdapter.js
+++ b/src/Adapters/Storage/Mongo/MongoStorageAdapter.js
@@ -1,8 +1,17 @@
-import MongoCollection                          from './MongoCollection';
-import MongoSchemaCollection                    from './MongoSchemaCollection';
-import {parse as parseUrl, format as formatUrl} from '../../../vendor/mongodbUrl';
-import * as transform                           from './MongoTransform';
-import _                                        from 'lodash';
+import MongoCollection       from './MongoCollection';
+import MongoSchemaCollection from './MongoSchemaCollection';
+import {
+  parse as parseUrl,
+  format as formatUrl,
+} from '../../../vendor/mongodbUrl';
+import {
+  parseObjectToMongoObjectForCreate,
+  mongoObjectToParseObject,
+  transformKey,
+  transformWhere,
+  transformUpdate,
+} from './MongoTransform';
+import _                     from 'lodash';

 let mongodb = require('mongodb');
 let MongoClient = mongodb.MongoClient;
@@ -159,12 +168,11 @@ export class MongoStorageAdapter {
    .then(schemasCollection => schemasCollection._fechOneSchemaFrom_SCHEMA(className));
  }

-  // TODO: As yet not particularly well specified. Creates an object. Shouldn't need the
-  // schemaController, but MongoTransform still needs it :( maybe shouldn't even need the schema,
+  // TODO: As yet not particularly well specified. Creates an object. Maybe shouldn't even need the schema,
  // and should infer from the type. Or maybe does need the schema for validations. Or maybe needs
  // the schem only for the legacy mongo format. We'll figure that out later.
-  createObject(className, object, schemaController, parseFormatSchema) {
-    const mongoObject = transform.parseObjectToMongoObjectForCreate(schemaController, className, object, parseFormatSchema);
+  createObject(className, object, schema) {
+    const mongoObject = parseObjectToMongoObjectForCreate(className, object, schema);
    return this.adaptiveCollection(className)
    .then(collection => collection.insertOne(mongoObject))
    .catch(error => {
@@ -176,15 +184,13 @@ export class MongoStorageAdapter {
    });
  }

-  // Remove all objects that match the given parse query. Parse Query should be in Parse Format.
+  // Remove all objects that match the given Parse Query.
  // If no objects match, reject with OBJECT_NOT_FOUND. If objects are found and deleted, resolve with undefined.
  // If there is some other error, reject with INTERNAL_SERVER_ERROR.
-
-  // Currently accepts the schema, that may not actually be necessary.
  deleteObjectsByQuery(className, query, schema) {
    return this.adaptiveCollection(className)
    .then(collection => {
-      let mongoWhere = transform.transformWhere(className, query, schema);
+      let mongoWhere = transformWhere(className, query, schema);
      return collection.deleteMany(mongoWhere)
    })
    .then(({ result }) => {
@@ -197,23 +203,43 @@ export class MongoStorageAdapter {
    });
  }

+  // Apply the update to all objects that match the given Parse Query.
+  updateObjectsByQuery(className, query, schema, update) {
+    const mongoUpdate = transformUpdate(className, update, schema);
+    const mongoWhere = transformWhere(className, query, schema);
+    return this.adaptiveCollection(className)
+    .then(collection => collection.updateMany(mongoWhere, mongoUpdate));
+  }
+
+  // Hopefully we can get rid of this in favor of updateObjectsByQuery.
+  findOneAndUpdate(className, query, schema, update) {
+    const mongoUpdate = transformUpdate(className, update, schema);
+    const mongoWhere = transformWhere(className, query, schema);
+    return this.adaptiveCollection(className)
+    .then(collection => collection.findOneAndUpdate(mongoWhere, mongoUpdate));
+  }
+
+  // Hopefully we can get rid of this. It's only used for config and hooks.
+  upsertOneObject(className, query, schema, update) {
+    const mongoUpdate = transformUpdate(className, update, schema);
+    const mongoWhere = transformWhere(className, query, schema);
+    return this.adaptiveCollection(className)
+    .then(collection => collection.upsertOne(mongoWhere, mongoUpdate));
+  }
+
  // Executes a find. Accepts: className, query in Parse format, and { skip, limit, sort }.
  find(className, query, schema, { skip, limit, sort }) {
-    let mongoWhere = this.transform.transformWhere(className, query, schema);
-    let mongoSort = _.mapKeys(sort, (value, fieldName) => transform.transformKey(className, fieldName, schema));
+    let mongoWhere = transformWhere(className, query, schema);
+    let mongoSort = _.mapKeys(sort, (value, fieldName) => transformKey(className, fieldName, schema));
    return this.adaptiveCollection(className)
    .then(collection => collection.find(mongoWhere, { skip, limit, sort: mongoSort }))
-    .then(objects => objects.map(object => transform.mongoObjectToParseObject(className, object, schema)));
+    .then(objects => objects.map(object => mongoObjectToParseObject(className, object, schema)));
  }

  // Executs a count.
  count(className, query, schema) {
    return this.adaptiveCollection(className)
-    .then(collection => collection.count(transform.transformWhere(className, query, schema)));
-  }
-
-  get transform() {
-    return transform;
+    .then(collection => collection.count(transformWhere(className, query, schema)));
  }
 }

--- a/src/Adapters/Storage/Mongo/MongoTransform.js
+++ b/src/Adapters/Storage/Mongo/MongoTransform.js
@@ -19,7 +19,7 @@ const transformKey = (className, fieldName, schema) => {
  return fieldName;
 }

-const transformKeyValueForUpdate = (schema, className, restKey, restValue) => {
+const transformKeyValueForUpdate = (className, restKey, restValue, parseFormatSchema) => {
  // Check if the schema is known since it's a built-in field.
  var key = restKey;
  var timeField = false;
@@ -38,12 +38,6 @@ const transformKeyValueForUpdate = (schema, className, restKey, restValue) => {
    key = '_updated_at';
    timeField = true;
    break;
-  case '_email_verify_token':
-    key = "_email_verify_token";
-    break;
-  case '_perishable_token':
-    key = "_perishable_token";
-    break;
  case 'sessionToken':
  case '_session_token':
    key = '_session_token';
@@ -57,26 +51,9 @@ const transformKeyValueForUpdate = (schema, className, restKey, restValue) => {
  case '_wperm':
    return {key: key, value: restValue};
    break;
-  case '$or':
-    throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, 'you can only use $or in queries');
-  case '$and':
-    throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, 'you can only use $and in queries');
-  default:
-    // Other auth data
-    var authDataMatch = key.match(/^authData\.([a-zA-Z0-9_]+)\.id$/);
-    if (authDataMatch) {
-      throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, 'can only query on ' + key);
-    }
  }

-  // Handle special schema key changes
-  // TODO: it seems like this is likely to have edge cases where
-  // pointer types are missed
-  var expected = undefined;
-  if (schema && schema.getExpectedType) {
-    expected = schema.getExpectedType(className, key);
-  }
-  if ((expected && expected.type == 'Pointer') || (!expected && restValue && restValue.__type == 'Pointer')) {
+  if ((parseFormatSchema.fields[key] && parseFormatSchema.fields[key].type === 'Pointer') || (!parseFormatSchema.fields[key] && restValue && restValue.__type == 'Pointer')) {
    key = '_p_' + key;
  }

@@ -101,9 +78,6 @@ const transformKeyValueForUpdate = (schema, className, restKey, restValue) => {
  }

  // Handle normal objects by recursing
-  if (Object.keys(restValue).some(key => key.includes('$') || key.includes('.'))) {
-    throw new Parse.Error(Parse.Error.INVALID_NESTED_KEY, "Nested keys should not contain the '$' or '.' characters");
-  }
  value = _.mapValues(restValue, transformInteriorValue);
  return {key, value};
 }
@@ -223,13 +197,7 @@ function transformWhere(className, restWhere, schema) {
  return mongoWhere;
 }

-const parseObjectKeyValueToMongoObjectKeyValue = (
-  schema,
-  className,
-  restKey,
-  restValue,
-  parseFormatSchema
-) => {
+const parseObjectKeyValueToMongoObjectKeyValue = (className, restKey, restValue, schema) => {
  // Check if the schema is known since it's a built-in field.
  let transformedValue;
  let coercedToDate;
@@ -267,7 +235,7 @@ const parseObjectKeyValueToMongoObjectKeyValue = (
  if (restValue && restValue.__type !== 'Bytes') {
    //Note: We may not know the type of a field here, as the user could be saving (null) to a field
    //That never existed before, meaning we can't infer the type.
-    if (parseFormatSchema.fields[restKey] && parseFormatSchema.fields[restKey].type == 'Pointer' || restValue.__type == 'Pointer') {
+    if (schema.fields[restKey] && schema.fields[restKey].type == 'Pointer' || restValue.__type == 'Pointer') {
      restKey = '_p_' + restKey;
    }
  }
@@ -305,18 +273,17 @@ const parseObjectKeyValueToMongoObjectKeyValue = (

 // Main exposed method to create new objects.
 // restCreate is the "create" clause in REST API form.
-function parseObjectToMongoObjectForCreate(schema, className, restCreate, parseFormatSchema) {
+function parseObjectToMongoObjectForCreate(className, restCreate, schema) {
  if (className == '_User') {
     restCreate = transformAuthData(restCreate);
  }
  var mongoCreate = transformACL(restCreate);
  for (let restKey in restCreate) {
    let { key, value } = parseObjectKeyValueToMongoObjectKeyValue(
-      schema,
      className,
      restKey,
      restCreate[restKey],
-      parseFormatSchema
+      schema
    );
    if (value !== undefined) {
      mongoCreate[key] = value;
@@ -326,10 +293,7 @@ function parseObjectToMongoObjectForCreate(schema, className, restCreate, parseF
 }

 // Main exposed method to help update old objects.
-function transformUpdate(schema, className, restUpdate) {
-  if (!restUpdate) {
-    throw 'got empty restUpdate';
-  }
+const transformUpdate = (className, restUpdate, parseFormatSchema) => {
  if (className == '_User') {
    restUpdate = transformAuthData(restUpdate);
  }
@@ -348,9 +312,8 @@ function transformUpdate(schema, className, restUpdate) {
      mongoUpdate['$set']['_acl'] = acl._acl;
    }
  }
-
  for (var restKey in restUpdate) {
-    var out = transformKeyValueForUpdate(schema, className, restKey, restUpdate[restKey]);
+    var out = transformKeyValueForUpdate(className, restKey, restUpdate[restKey], parseFormatSchema);

    // If the output value is an object with any $ keys, it's an
    // operator that needs to be lifted onto the top level update
--- a/src/Controllers/DatabaseController.js
+++ b/src/Controllers/DatabaseController.js
@@ -62,12 +62,6 @@ function DatabaseController(adapter, { skipValidation } = {}) {
  this.schemaPromise = null;
  this.skipValidation = !!skipValidation;
  this.connect();
-
-  Object.defineProperty(this, 'transform', {
-     get: function() {
-       return adapter.transform;
-     }
-  })
 }

 DatabaseController.prototype.WithoutValidation = function() {
@@ -171,6 +165,7 @@ const filterSensitiveData = (isMaster, aclGroup, className, object) => {
 //   acl:  a list of strings. If the object to be updated has an ACL,
 //         one of the provided strings must provide the caller with
 //         write permissions.
+const specialKeysForUpdate = ['_hashed_password', '_perishable_token', '_email_verify_token'];
 DatabaseController.prototype.update = function(className, query, update, {
  acl,
  many,
@@ -188,8 +183,7 @@ DatabaseController.prototype.update = function(className, query, update, {
  .then(schemaController => {
    return (isMaster ? Promise.resolve() : schemaController.validatePermission(className, aclGroup, 'update'))
    .then(() => this.handleRelationUpdates(className, query.objectId, update))
-    .then(() => this.adapter.adaptiveCollection(className))
-    .then(collection => {
+    .then(() => {
      if (!isMaster) {
        query = this.addPointerPermissions(schemaController, className, 'update', query, aclGroup);
      }
@@ -209,20 +203,27 @@ DatabaseController.prototype.update = function(className, query, update, {
        }
        throw error;
      })
-      .then(parseFormatSchema => {
-        var mongoWhere = this.transform.transformWhere(className, query, parseFormatSchema);
-        mongoUpdate = this.transform.transformUpdate(
-          schemaController,
-          className,
-          update,
-          {validate: !this.skipValidation}
-        );
+      .then(schema => {
+        Object.keys(update).forEach(fieldName => {
+          if (fieldName.match(/^authData\.([a-zA-Z0-9_]+)\.id$/)) {
+            throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, `Invalid field name for update: ${fieldName}`);
+          }
+          fieldName = fieldName.split('.')[0];
+          if (!SchemaController.fieldNameIsValid(fieldName) && !specialKeysForUpdate.includes(fieldName)) {
+            throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, `Invalid field name for update: ${fieldName}`);
+          }
+        });
+        for (let updateOperation in update) {
+          if (Object.keys(updateOperation).some(innerKey => innerKey.includes('$') || innerKey.includes('.'))) {
+            throw new Parse.Error(Parse.Error.INVALID_NESTED_KEY, "Nested keys should not contain the '$' or '.' characters");
+          }
+        }
        if (many) {
-          return collection.updateMany(mongoWhere, mongoUpdate);
+          return this.adapter.updateObjectsByQuery(className, query, schema, update);
        } else if (upsert) {
-          return collection.upsertOne(mongoWhere, mongoUpdate);
+          return this.adapter.upsertOneObject(className, query, schema, update);
        } else {
-          return collection.findOneAndUpdate(mongoWhere, mongoUpdate);
+          return this.adapter.findOneAndUpdate(className, query, schema, update);
        }
      });
    })
@@ -393,7 +394,7 @@ DatabaseController.prototype.create = function(className, object, { acl } = {})
    .then(() => this.handleRelationUpdates(className, null, object))
    .then(() => schemaController.enforceClassExists(className))
    .then(() => schemaController.getOneSchema(className, true))
-    .then(schema => this.adapter.createObject(className, object, schemaController, schema))
+    .then(schema => this.adapter.createObject(className, object, schema))
    .then(result => sanitizeDatabaseResult(originalObject, result.ops[0]));
  })
 };
--- a/src/Routers/GlobalConfigRouter.js
+++ b/src/Routers/GlobalConfigRouter.js
@@ -24,9 +24,7 @@ export class GlobalConfigRouter extends PromiseRouter {
      return acc;
    }, {});
    let database = req.config.database.WithoutValidation();
-    return database.update('_GlobalConfig', {objectId: 1}, update, {upsert: true}).then(() => {
-      return Promise.resolve({ response: { result: true } });
-    });
+    return database.update('_GlobalConfig', {objectId: 1}, update, {upsert: true}).then(() => ({ response: { result: true } }));
  }

  mountRoutes() {