Move field deletion logic into mongo adapter (#1471)

spec/Schema.spec.js

@@ -684,12 +684,13 @@ describe('Schema', () => {
       .then(() => schema.deleteField('relationField', 'NewClass', config.database))
       .then(() => schema.reloadData())
       .then(() => {
-        expect(schema['data']['NewClass']).toEqual({
+        const expectedSchema = {
           objectId: { type: 'String' },
           updatedAt: { type: 'Date' },
           createdAt: { type: 'Date' },
           ACL: { type: 'ACL' }
-        });
+        };
+        expect(dd(schema.data.NewClass, expectedSchema)).toEqual(undefined);
         done();
       });
     });
@@ -716,6 +717,10 @@ describe('Schema', () => {
         done();
         Parse.Object.enableSingleInstance();
       });
+    })
+    .catch(error => {
+      fail(error);
+      done();
     });
   });
 

src/Adapters/Storage/Mongo/MongoCollection.js

@@ -28,7 +28,6 @@ export default class MongoCollection {
 
         var index = {};
         index[key] = '2d';
-        //TODO: condiser moving index creation logic into Schema.js
         return this._mongoCollection.createIndex(index)
           // Retry, but just once.
           .then(() => this._rawFind(query, { skip, limit, sort }));

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

@@ -1,7 +1,7 @@
-
-import MongoCollection from './MongoCollection';
-import MongoSchemaCollection from './MongoSchemaCollection';
+import MongoCollection                          from './MongoCollection';
+import MongoSchemaCollection                    from './MongoSchemaCollection';
 import {parse as parseUrl, format as formatUrl} from '../../../vendor/mongodbUrl';
+import _                                        from 'lodash';
 
 let mongodb = require('mongodb');
 let MongoClient = mongodb.MongoClient;
@@ -78,6 +78,52 @@ export class MongoStorageAdapter {
       });
     });
   }
+
+  // Remove the column and all the data. For Relations, the _Join collection is handled
+  // specially, this function does not delete _Join columns. It should, however, indicate
+  // that the relation fields does not exist anymore. In mongo, this means removing it from
+  // the _SCHEMA collection.  There should be no actual data in the collection under the same name
+  // as the relation column, so it's fine to attempt to delete it. If the fields listed to be
+  // deleted do not exist, this function should return successfully anyways. Checking for
+  // attempts to delete non-existent fields is the responsibility of Parse Server.
+
+  // Pointer field names are passed for legacy reasons: the original mongo
+  // format stored pointer field names differently in the database, and therefore
+  // needed to know the type of the field before it could delete it. Future database
+  // adatpers should ignore the pointerFieldNames argument. All the field names are in
+  // fieldNames, they show up additionally in the pointerFieldNames database for use
+  // by the mongo adapter, which deals with the legacy mongo format.
+
+  // This function is not obligated to delete fields atomically. It is given the field
+  // names in a list so that databases that are capable of deleting fields atomically
+  // may do so.
+
+  // Returns a Promise.
+
+  // This function currently accepts the collectionPrefix and adaptive collection as a paramater because it isn't
+  // actually capable of determining the location of it's own _SCHEMA collection without having
+  // the collectionPrefix. Also, Schemas.js, the caller of this function, only stores the collection
+  // itself, and not the prefix. Eventually Parse Server won't care what a SchemaCollection is and
+  // will just tell the DB adapter to do things and it will do them.
+  deleteFields(className: string, fieldNames, pointerFieldNames, collectionPrefix, adaptiveCollection) {
+    const nonPointerFieldNames = _.difference(fieldNames, pointerFieldNames);
+    const mongoFormatNames = nonPointerFieldNames.concat(pointerFieldNames.map(name => `_p_${name}`));
+    const collectionUpdate = { '$unset' : {} };
+    mongoFormatNames.forEach(name => {
+      collectionUpdate['$unset'][name] = null;
+    });
+
+    const schemaUpdate = { '$unset' : {} };
+    fieldNames.forEach(name => {
+      schemaUpdate['$unset'][name] = null;
+    });
+
+    return adaptiveCollection.updateMany({}, collectionUpdate)
+    .then(updateResult => {
+      return this.schemaCollection(collectionPrefix)
+    })
+    .then(schemaCollection => schemaCollection.updateSchema(className, schemaUpdate));
+  }
 }
 
 export default MongoStorageAdapter;

src/Schema.js

@@ -514,40 +514,36 @@ class Schema {
     }
 
     return this.reloadData()
-      .then(() => {
-        return this.hasClass(className)
-          .then(hasClass => {
-            if (!hasClass) {
-              throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
-            }
-            if (!this.data[className][fieldName]) {
-              throw new Parse.Error(255, `Field ${fieldName} does not exist, cannot delete.`);
-            }
+    .then(() => this.hasClass(className))
+    .then(hasClass => {
+      if (!hasClass) {
+        throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
+      }
+      if (!this.data[className][fieldName]) {
+        throw new Parse.Error(255, `Field ${fieldName} does not exist, cannot delete.`);
+      }
 
-            if (this.data[className][fieldName].type == 'Relation') {
-              //For relations, drop the _Join table
-              return database.dropCollection(`_Join:${fieldName}:${className}`)
-              .then(() => {
-                return Promise.resolve();
-              }, error => {
-                if (error.message == 'ns not found') {
-                  return Promise.resolve();
-                }
-                return Promise.reject(error);
-              });
-            }
+      if (this.data[className][fieldName].type == 'Relation') {
+        //For relations, drop the _Join table
+        return database.adaptiveCollection(className).then(collection => {
+          return database.adapter.deleteFields(className, [fieldName], [], database.collectionPrefix, collection);
+        })
+        .then(() => database.dropCollection(`_Join:${fieldName}:${className}`))
+        .catch(error => {
+          // 'ns not found' means collection was already gone. Ignore deletion attempt.
+          // TODO: 'ns not found' is a mongo implementation detail. Move it into mongo adapter.
+          if (error.message == 'ns not found') {
+            return Promise.resolve();
+          }
+          return Promise.reject(error);
+        });
+      }
 
-            // for non-relations, remove all the data.
-            // This is necessary to ensure that the data is still gone if they add the same field.
-            return database.adaptiveCollection(className)
-              .then(collection => {
-                let mongoFieldName = this.data[className][fieldName].type === 'Pointer' ? `_p_${fieldName}` : fieldName;
-                return collection.updateMany({}, { "$unset": { [mongoFieldName]: null } });
-              });
-          })
-          // Save the _SCHEMA object
-          .then(() => this._collection.updateSchema(className, { $unset: { [fieldName]: null } }));
-      });
+      const fieldNames = [fieldName];
+      const pointerFieldNames = this.data[className][fieldName].type === 'Pointer' ? [fieldName] : [];
+      return database.adaptiveCollection(className)
+      .then(collection => database.adapter.deleteFields(className, fieldNames, pointerFieldNames, database.collectionPrefix, collection));
+    });
   }
 
   // Validates an object provided in REST format.