Move "No two geopoints" logic into mongo adapter

Author: drew-gross

spec/ParseGeoPoint.spec.js

@@ -85,8 +85,8 @@ describe('Parse.GeoPoint testing', () => {
       equal(results.length, 3);
       done();
     }, (err) => {
-      console.log(err);
-      fail();
+      fail("Couldn't query GeoPoint");
+      fail(err)
     });
   });
 

src/Adapters/Storage/Mongo/MongoSchemaCollection.js

@@ -184,21 +184,45 @@ class MongoSchemaCollection {
     return this._collection.upsertOne(_mongoSchemaQueryFromNameQuery(name, query), update);
   }
 
-  updateField(className: string, fieldName: string, type: string) {
-    // We don't have this field. Update the schema.
-    // Note that we use the $exists guard and $set to avoid race
-    // conditions in the database. This is important!
-    let query = {};
-    query[fieldName] = { '$exists': false };
-    let update = {};
-    if (typeof type === 'string') {
-      type = {
-        type: type
+  // Add a field to the schema. If database does not support the field
+  // type (e.g. mongo doesn't support more than one GeoPoint in a class) reject with an "Incorrect Type"
+  // Parse error with a desciptive message. If the field already exists, this function must
+  // not modify the schema, and must reject with an error. Exact error format is TBD. If this function
+  // is called for a class that doesn't exist, this function must create that class.
+
+  // TODO: throw an error if an unsupported field type is passed. Deciding whether a type is supported
+  // should be the job of the adapter. Some adapters may not support GeoPoint at all. Others may
+  // Support additional types that Mongo doesn't, like Money, or something.
+
+  // TODO: don't spend an extra query on finding the schema if the type we are trying to add isn't a GeoPoint.
+  addFieldIfNotExists(className: string, fieldName: string, type: string) {
+    return this.findSchema(className)
+    .then(schema => {
+      // The schema exists. Check for existing GeoPoints.
+      if (type.type === 'GeoPoint') {
+        // Make sure there are not other geopoint fields
+        if (Object.keys(schema.fields).some(existingField => schema.fields[existingField].type === 'GeoPoint')) {
+          return Promise.reject(new Parse.Error(Parse.Error.INCORRECT_TYPE, 'MongoDB only supports one GeoPoint field in a class.'));
+        }
       }
-    }
-    update[fieldName] = parseFieldTypeToMongoFieldType(type);
-    update = {'$set': update};
-    return this.upsertSchema(className, query, update);
+      return Promise.resolve();
+    }, error => {
+      // If error is undefined, the schema doesn't exist, and we can create the schema with the field.
+      // If some other error, reject with it.
+      if (error === undefined) {
+        return Promise.resolve()
+      }
+      throw Promise.reject(error);
+    })
+    .then(() => {
+      // We use $exists and $set to avoid overwriting the field type if it
+      // already exists. (it could have added inbetween the last query and the update)
+      return this.upsertSchema(
+        className,
+        { [fieldName]: { '$exists': false } },
+        { '$set' : { [fieldName]: parseFieldTypeToMongoFieldType(type) } }
+      );
+    });
   }
 }
 

src/Schema.js

@@ -464,18 +464,11 @@ class Schema {
         return Promise.resolve(this);
       }
 
-      if (type === 'GeoPoint') {
-        // Make sure there are not other geopoint fields
-        for (let otherKey in this.data[className]) {
-          if (this.data[className][otherKey].type === 'GeoPoint') {
-            throw new Parse.Error(
-              Parse.Error.INCORRECT_TYPE,
-              'there can only be one geopoint field in a class');
-          }
-        }
+      if (typeof type === 'string') {
+        type = { type };
       }
 
-      return this._collection.updateField(className, fieldName, type).then(() => {
+      return this._collection.addFieldIfNotExists(className, fieldName, type).then(() => {
         // The update succeeded. Reload the schema
         return this.reloadData();
       }, () => {

src/transform.js

@@ -346,23 +346,20 @@ function CannotTransform() {}
 // Raises an error if this cannot possibly be valid REST format.
 // Returns CannotTransform if it's just not an atom, or if force is
 // true, throws an error.
-function transformAtom(atom, force, options) {
-  options = options || {};
-  var inArray = options.inArray;
-  var inObject = options.inObject;
+function transformAtom(atom, force, {
+  inArray,
+  inObject,
+} = {}) {
   switch(typeof atom) {
   case 'string':
   case 'number':
   case 'boolean':
     return atom;
-
   case 'undefined':
     return atom;
   case 'symbol':
   case 'function':
-    throw new Parse.Error(Parse.Error.INVALID_JSON,
-                          'cannot transform value: ' + atom);
-
+    throw new Parse.Error(Parse.Error.INVALID_JSON, `cannot transform value: ${atom}`);
   case 'object':
     if (atom instanceof Date) {
       // Technically dates are not rest format, but, it seems pretty
@@ -377,7 +374,7 @@ function transformAtom(atom, force, options) {
     // TODO: check validity harder for the __type-defined types
     if (atom.__type == 'Pointer') {
       if (!inArray && !inObject) {
-        return atom.className + '$' + atom.objectId;
+        return `${atom.className}$${atom.objectId}`;
       }
       return {
         __type: 'Pointer',
@@ -402,15 +399,13 @@ function transformAtom(atom, force, options) {
     }
 
     if (force) {
-      throw new Parse.Error(Parse.Error.INVALID_JSON,
-                            'bad atom: ' + atom);
+      throw new Parse.Error(Parse.Error.INVALID_JSON, `bad atom: ${atom}`);
     }
     return CannotTransform;
 
   default:
     // I don't think typeof can ever let us get here
-    throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR,
-                          'really did not expect value: ' + atom);
+    throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR, `really did not expect value: ${atom}`);
   }
 }