Cleanup update (#1590)

* destructuring in DB controller * deleteObject in db adapter * Turns out we can't have delete by object ID because of ACLs... * Fix tests * destructure acl * Don't reject with object
2016-04-22 14:05:21 -07:00
parent ab827e3c2f
commit 0d094767cf
10 changed files with 192 additions and 170 deletions
--- a/spec/ParseACL.spec.js
+++ b/spec/ParseACL.spec.js
@@ -169,11 +169,11 @@ describe('Parse.ACL', () => {
            ok(object.get("ACL"));

            // Start making requests by the public, which should all fail.
-            Parse.User.logOut();
-
-            // Delete
-            object.destroy().then(() => {
+            Parse.User.logOut()
+            .then(() => object.destroy())
+            .then(() => {
              fail('destroy should fail');
+              done();
            }, error => {
              expect(error.code).toEqual(Parse.Error.OBJECT_NOT_FOUND);
              done();
--- a/spec/ParseAPI.spec.js
+++ b/spec/ParseAPI.spec.js
@@ -39,8 +39,8 @@ describe('miscellaneous', function() {
      expect(data.get('password')).toBeUndefined();
      done();
    }, function(err) {
-      console.log(err);
      fail(err);
+      done();
    });
  });

--- a/spec/ParseHooks.spec.js
+++ b/spec/ParseHooks.spec.js
@@ -37,50 +37,44 @@ describe('Hooks', () => {
     });
   });

-   it("should CRUD a function registration", (done) => {
-     // Create
-     Parse.Hooks.createFunction("My-Test-Function", "http://someurl").then((res) => {
-       expect(res.functionName).toBe("My-Test-Function");
-       expect(res.url).toBe("http://someurl")
-       // Find
-       return Parse.Hooks.getFunction("My-Test-Function");
-     }, (err) => {
-       fail(err);
-       done();
-     }).then((res) => {
-       expect(res).not.toBe(null);
-       expect(res).not.toBe(undefined);
-       expect(res.url).toBe("http://someurl");
-       // delete
-        return Parse.Hooks.updateFunction("My-Test-Function", "http://anotherurl");
-     }, (err) => {
-       fail(err);
-       done();
-     }).then((res) => {
-       expect(res.functionName).toBe("My-Test-Function");
-       expect(res.url).toBe("http://anotherurl")
-
-       return Parse.Hooks.deleteFunction("My-Test-Function");
-     }, (err) => {
-       fail(err);
-       done();
-     }).then((res) => {
-       // Find again! but should be deleted
-       return Parse.Hooks.getFunction("My-Test-Function");
-     }, (err) => {
-       fail(err);
-       done();
-     }).then((res) => {
-       fail("Should not succeed")
-       done();
-     }, (err) => {
-       expect(err).not.toBe(null);
-       expect(err).not.toBe(undefined);
-       expect(err.code).toBe(143);
-       expect(err.error).toBe("no function named: My-Test-Function is defined")
-       done();
-     })
-   });
+  it("should CRUD a function registration", (done) => {
+    // Create
+    Parse.Hooks.createFunction("My-Test-Function", "http://someurl")
+    .then(response => {
+      expect(response.functionName).toBe("My-Test-Function");
+      expect(response.url).toBe("http://someurl")
+      // Find
+      return Parse.Hooks.getFunction("My-Test-Function")
+    }).then(response => {
+      expect(response.url).toBe("http://someurl");
+      return Parse.Hooks.updateFunction("My-Test-Function", "http://anotherurl");
+    })
+    .then((res) => {
+      expect(res.functionName).toBe("My-Test-Function");
+      expect(res.url).toBe("http://anotherurl")
+      // delete
+      return Parse.Hooks.deleteFunction("My-Test-Function")
+    })
+    .then((res) => {
+      // Find again! but should be deleted
+      return Parse.Hooks.getFunction("My-Test-Function")
+      .then(res => {
+        fail("Failed to delete hook")
+        fail(res)
+        done();
+        return Promise.resolve();
+      }, (err) => {
+        expect(err.code).toBe(143);
+        expect(err.error).toBe("no function named: My-Test-Function is defined")
+        done();
+        return Promise.resolve();
+      })
+    })
+    .catch(error => {
+      fail(error);
+      done();
+    })
+  });

   it("should CRUD a trigger registration", (done) => {
     // Create
--- a/spec/PointerPermissions.spec.js
+++ b/spec/PointerPermissions.spec.js
@@ -41,8 +41,8 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
-  
+
+
  it('should work with write', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -107,7 +107,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should let a proper user find', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -137,7 +137,7 @@ describe('Pointer Permissions', () => {
      let q = new Parse.Query('AnObject');
      return q.find();
    }).then((res) => {
-      expect(res.length).toBe(0);     
+      expect(res.length).toBe(0);
    }).then(() => {
      return Parse.User.logIn('user2', 'password');
    }).then(() => {
@@ -167,7 +167,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should not allow creating objects', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -193,7 +193,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should handle multiple writeUserFields', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -235,7 +235,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should prevent creating pointer permission on missing field', (done) => {
    let config = new Config(Parse.applicationId);
    config.database.loadSchema().then((schema) => {
@@ -248,7 +248,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should prevent creating pointer permission on bad field', (done) => {
    let config = new Config(Parse.applicationId);
    config.database.loadSchema().then((schema) => {
@@ -261,7 +261,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should prevent creating pointer permission on bad field', (done) => {
    let config = new Config(Parse.applicationId);
    let object = new Parse.Object('AnObject');
@@ -278,14 +278,14 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('tests CLP / Pointer Perms / ACL write (PP Locked)', (done) => {
    /*
      tests:
      CLP: update open ({"*": true})
      PointerPerm: "owner"
      ACL: logged in user has access
-      
+
      The owner is another user than the ACL
     */
    let config = new Config(Parse.applicationId);
@@ -325,7 +325,7 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
+
  it('tests CLP / Pointer Perms / ACL write (ACL Locked)', (done) => {
    /*
      tests:
@@ -370,7 +370,7 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
+
  it('tests CLP / Pointer Perms / ACL write (ACL/PP OK)', (done) => {
    /*
      tests:
@@ -415,7 +415,7 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
+
  it('tests CLP / Pointer Perms / ACL read (PP locked)', (done) => {
    /*
      tests:
@@ -462,7 +462,7 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
+
  it('tests CLP / Pointer Perms / ACL read (PP/ACL OK)', (done) => {
    /*
      tests:
@@ -509,7 +509,7 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
+
  it('tests CLP / Pointer Perms / ACL read (ACL locked)', (done) => {
    /*
      tests:
@@ -554,7 +554,7 @@ describe('Pointer Permissions', () => {
      done();
    });
  });
-  
+
  it('should let master key find objects', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -569,7 +569,7 @@ describe('Pointer Permissions', () => {
      let q = new Parse.Query('AnObject');
      return q.find();
    }).then(() => {
-      
+
    }, (err) => {
      expect(err.code).toBe(101);
      return Promise.resolve();
@@ -584,7 +584,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should let master key get objects', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -599,7 +599,7 @@ describe('Pointer Permissions', () => {
      let q = new Parse.Query('AnObject');
      return q.get(object.id);
    }).then(() => {
-      
+
    }, (err) => {
      expect(err.code).toBe(101);
      return Promise.resolve();
@@ -615,8 +615,8 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
-  
+
+
  it('should let master key update objects', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -630,7 +630,7 @@ describe('Pointer Permissions', () => {
    }).then(() => {
      return object.save({'hello': 'bar'});
    }).then(() => {
-      
+
    }, (err) => {
      expect(err.code).toBe(101);
      return Promise.resolve();
@@ -644,7 +644,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should let master key delete objects', (done) => {
    let config = new Config(Parse.applicationId);
    let user = new Parse.User();
@@ -658,7 +658,7 @@ describe('Pointer Permissions', () => {
    }).then(() => {
      return object.destroy();
    }).then(() => {
-      
+      fail();
    }, (err) => {
      expect(err.code).toBe(101);
      return Promise.resolve();
@@ -671,7 +671,7 @@ describe('Pointer Permissions', () => {
      done();
    })
  });
-  
+
  it('should fail with invalid pointer perms', () => {
    let config = new Config(Parse.applicationId);
    config.database.loadSchema().then((schema) => {
@@ -682,7 +682,7 @@ describe('Pointer Permissions', () => {
      done();
     });
  });
-  
+
  it('should fail with invalid pointer perms', () => {
    let config = new Config(Parse.applicationId);
    config.database.loadSchema().then((schema) => {
@@ -693,5 +693,5 @@ describe('Pointer Permissions', () => {
      done();
     });
  })
-  
+
 });
--- a/spec/Schema.spec.js
+++ b/spec/Schema.spec.js
@@ -121,39 +121,39 @@ describe('SchemaController', () => {
  });

  it('class-level permissions test get', (done) => {
-    var user;
    var obj;
-    createTestUser().then((u) => {
-      user = u;
-      return config.database.loadSchema();
-    }).then((schema) => {
-      // Just to create a valid class
-      return schema.validateObject('Stuff', {foo: 'bar'});
-    }).then((schema) => {
-      var find = {};
-      var get = {};
-      get[user.id] = true;
-      return schema.setPermissions('Stuff', {
-        'find': find,
-        'get': get
-      });
-    }).then((schema) => {
-      obj = new Parse.Object('Stuff');
-      obj.set('foo', 'bar');
-      return obj.save();
-    }).then((o) => {
-      obj = o;
-      var query = new Parse.Query('Stuff');
-      return query.find();
-    }).then((results) => {
-      fail('Class permissions should have rejected this query.');
-      done();
-    }, (e) => {
-      var query = new Parse.Query('Stuff');
-      return query.get(obj.id).then((o) => {
+    createTestUser()
+    .then(user => {
+      console.log(user);
+      return config.database.loadSchema()
+      // Create a valid class
+      .then(schema => schema.validateObject('Stuff', {foo: 'bar'}))
+      .then(schema => {
+        var find = {};
+        var get = {};
+        get[user.id] = true;
+        return schema.setPermissions('Stuff', {
+          'find': find,
+          'get': get
+        });
+      }).then((schema) => {
+        obj = new Parse.Object('Stuff');
+        obj.set('foo', 'bar');
+        return obj.save();
+      }).then((o) => {
+        obj = o;
+        var query = new Parse.Query('Stuff');
+        return query.find();
+      }).then((results) => {
+        fail('Class permissions should have rejected this query.');
        done();
      }, (e) => {
-        fail('Class permissions should have allowed this get query');
+        var query = new Parse.Query('Stuff');
+        return query.get(obj.id).then((o) => {
+          done();
+        }, (e) => {
+          fail('Class permissions should have allowed this get query');
+        });
      });
    });
  });
--- a/spec/helper.js
+++ b/spec/helper.js
@@ -84,7 +84,7 @@ beforeEach(function(done) {
  Parse.initialize('test', 'test', 'test');
  Parse.serverURL = 'http://localhost:' + port + '/1';
  Parse.User.enableUnsafeCurrentUser();
-  done();
+  return TestUtils.destroyAllDataPermanently().then(done, fail);
 });

 afterEach(function(done) {
--- a/src/Adapters/Storage/Mongo/MongoStorageAdapter.js
+++ b/src/Adapters/Storage/Mongo/MongoStorageAdapter.js
@@ -159,6 +159,37 @@ export class MongoStorageAdapter {
    .then(collection => collection.insertOne(mongoObject));
  }

+  // Remove all objects that match the given parse query. Parse Query should be in Parse Format.
+  // 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 acl, schemaController, validate
+  // for lecacy reasons, Parse Server should later integrate acl into the query. Database adapters
+  // shouldn't know about acl.
+  deleteObjectsByQuery(className, query, acl, schemaController, validate) {
+    return this.adaptiveCollection(className)
+    .then(collection => {
+      let mongoWhere = transform.transformWhere(
+        schemaController,
+        className,
+        query,
+        { validate }
+      );
+      if (acl) {
+        mongoWhere = transform.addWriteACL(mongoWhere, acl);
+      }
+      return collection.deleteMany(mongoWhere)
+    })
+    .then(({ result }) => {
+      if (result.n === 0) {
+        throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Object not found.');
+      }
+      return Promise.resolve();
+    }, error => {
+      throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR, 'Database adapter error');
+    });
+  }
+
  get transform() {
    return transform;
  }
--- a/src/Adapters/Storage/Mongo/MongoTransform.js
+++ b/src/Adapters/Storage/Mongo/MongoTransform.js
@@ -195,8 +195,7 @@ function transformWhere(schema, className, restWhere, options = {validate: true}
  let transformKeyOptions = {query: true};
  transformKeyOptions.validate = options.validate;
  for (let restKey in restWhere) {
-    let out = transformKeyValue(schema, className, restKey, restWhere[restKey],
-                                transformKeyOptions);
+    let out = transformKeyValue(schema, className, restKey, restWhere[restKey], transformKeyOptions);
    mongoWhere[out.key] = out.value;
  }
  return mongoWhere;
@@ -333,8 +332,7 @@ function transformUpdate(schema, className, restUpdate) {
  }

  for (var restKey in restUpdate) {
-    var out = transformKeyValue(schema, className, restKey, restUpdate[restKey],
-                                {update: true});
+    var out = transformKeyValue(schema, className, restKey, restUpdate[restKey], {update: true});

    // 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
@@ -87,10 +87,10 @@ DatabaseController.prototype.redirectClassNameForKey = function(className, key)
 // Returns a promise that resolves to the new schema.
 // This does not update this.schema, because in a situation like a
 // batch request, that could confuse other users of the schema.
-DatabaseController.prototype.validateObject = function(className, object, query, options) {
+DatabaseController.prototype.validateObject = function(className, object, query, { acl }) {
  let schema;
-  let isMaster = !('acl' in options);
-  var aclGroup = options.acl || [];
+  let isMaster = acl === undefined;
+  var aclGroup = acl || [];
  return this.loadSchema().then(s => {
    schema = s;
    if (isMaster) {
@@ -131,14 +131,18 @@ DatabaseController.prototype.untransformObject = function(
 //   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.
-DatabaseController.prototype.update = function(className, query, update, options = {}) {
+DatabaseController.prototype.update = function(className, query, update, {
+  acl,
+  many,
+  upsert,
+} = {}) {

  const originalUpdate = update;
  // Make a copy of the object, so we don't mutate the incoming data.
  update = deepcopy(update);

-  var isMaster = !('acl' in options);
-  var aclGroup = options.acl || [];
+  var isMaster = acl === undefined;
+  var aclGroup = acl || [];
  var mongoUpdate, schema;
  return this.loadSchema()
    .then(s => {
@@ -152,19 +156,19 @@ DatabaseController.prototype.update = function(className, query, update, options
    .then(() => this.adapter.adaptiveCollection(className))
    .then(collection => {
      if (!isMaster) {
-        query = this.addPointerPermissions(schema, className, 'update', query, aclGroup); 
+        query = this.addPointerPermissions(schema, className, 'update', query, aclGroup);
      }
      if (!query) {
        return Promise.resolve();
      }
      var mongoWhere = this.transform.transformWhere(schema, className, query, {validate: !this.skipValidation});
-      if (options.acl) {
-        mongoWhere = this.transform.addWriteACL(mongoWhere, options.acl);
+      if (acl) {
+        mongoWhere = this.transform.addWriteACL(mongoWhere, acl);
      }
      mongoUpdate = this.transform.transformUpdate(schema, className, update, {validate: !this.skipValidation});
-      if (options.many) {
+      if (many) {
        return collection.updateMany(mongoWhere, mongoUpdate);
-      }else if (options.upsert) {
+      } else if (upsert) {
        return collection.upsertOne(mongoWhere, mongoUpdate);
      } else {
        return collection.findOneAndUpdate(mongoWhere, mongoUpdate);
@@ -203,9 +207,7 @@ function sanitizeDatabaseResult(originalObject, result) {
 // Returns a promise that resolves successfully when these are
 // processed.
 // This mutates update.
-DatabaseController.prototype.handleRelationUpdates = function(className,
-                                                         objectId,
-                                                         update) {
+DatabaseController.prototype.handleRelationUpdates = function(className, objectId, update) {
  var pending = [];
  var deleteMe = [];
  objectId = update.objectId || objectId;
@@ -282,51 +284,42 @@ DatabaseController.prototype.removeRelation = function(key, fromClassName, fromI
 //   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.
-DatabaseController.prototype.destroy = function(className, query, options = {}) {
-  var isMaster = !('acl' in options);
-  var aclGroup = options.acl || [];
+DatabaseController.prototype.destroy = function(className, query, { acl } = {}) {
+  const isMaster = acl === undefined;
+  const aclGroup = acl || [];

-  var schema;
  return this.loadSchema()
-    .then(s => {
-      schema = s;
+  .then(schemaController => {
+    return (isMaster ? Promise.resolve() : schemaController.validatePermission(className, aclGroup, 'delete'))
+    .then(() => {
      if (!isMaster) {
-        return schema.validatePermission(className, aclGroup, 'delete');
-      }
-      return Promise.resolve();
-    })
-    .then(() => this.adapter.adaptiveCollection(className))
-    .then(collection => {
-      if (!isMaster) {
-        query = this.addPointerPermissions(schema, className, 'delete', query, aclGroup); 
+        query = this.addPointerPermissions(schemaController, className, 'delete', query, aclGroup);
        if (!query) {
          throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Object not found.');
        }
      }
-      let mongoWhere = this.transform.transformWhere(schema, className, query, {validate: !this.skipValidation});
-      if (options.acl) {
-        mongoWhere = this.transform.addWriteACL(mongoWhere, options.acl);
-      }
-      return collection.deleteMany(mongoWhere);
-    })
-    .then(resp => {
-      //Check _Session to avoid changing password failed without any session.
-      // TODO: @nlutsenko Stop relying on `result.n`
-      if (resp.result.n === 0 && className !== "_Session") {
-        throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Object not found.');
-      }
+      // delete by query
+      return this.adapter.deleteObjectsByQuery(className, query, acl, schemaController, !this.skipValidation)
+      .catch(error => {
+        // When deleting sessions while changing passwords, don't throw an error if they don't have any sessions.
+        if (className === "_Session" && error.code === Parse.Error.OBJECT_NOT_FOUND) {
+          return Promise.resolve({});
+        }
+        throw error;
+      });
    });
+  });
 };

 // Inserts an object into the database.
 // Returns a promise that resolves successfully iff the object saved.
-DatabaseController.prototype.create = function(className, object, options = {}) {
+DatabaseController.prototype.create = function(className, object, { acl } = {}) {
  // Make a copy of the object, so we don't mutate the incoming data.
  let originalObject = object;
  object = deepcopy(object);

-  var isMaster = !('acl' in options);
-  var aclGroup = options.acl || [];
+  var isMaster = acl === undefined;
+  var aclGroup = acl || [];

  return this.validateClassName(className)
  .then(() => this.loadSchema())
@@ -570,27 +563,33 @@ DatabaseController.prototype.addNotInObjectIdsIds = function(ids = null, query)
 // TODO: make userIds not needed here. The db adapter shouldn't know
 // anything about users, ideally. Then, improve the format of the ACL
 // arg to work like the others.
-DatabaseController.prototype.find = function(className, query, options = {}) {
+DatabaseController.prototype.find = function(className, query, {
+  skip,
+  limit,
+  acl,
+  sort,
+  count,
+} = {}) {
  let mongoOptions = {};
-  if (options.skip) {
-    mongoOptions.skip = options.skip;
+  if (skip) {
+    mongoOptions.skip = skip;
  }
-  if (options.limit) {
-    mongoOptions.limit = options.limit;
+  if (limit) {
+    mongoOptions.limit = limit;
  }
-  let isMaster = !('acl' in options);
-  let aclGroup = options.acl || [];
+  let isMaster = acl === undefined;
+  let aclGroup = acl || [];
  let schema = null;
  let op = typeof query.objectId == 'string' && Object.keys(query).length === 1 ?
        'get' :
        'find';
  return this.loadSchema().then(s => {
    schema = s;
-    if (options.sort) {
+    if (sort) {
      mongoOptions.sort = {};
-      for (let key in options.sort) {
+      for (let key in sort) {
        let mongoKey = this.transform.transformKey(schema, className, key);
-        mongoOptions.sort[mongoKey] = options.sort[key];
+        mongoOptions.sort[mongoKey] = sort[key];
      }
    }

@@ -604,7 +603,7 @@ DatabaseController.prototype.find = function(className, query, options = {}) {
  .then(() => this.adapter.adaptiveCollection(className))
  .then(collection => {
    if (!isMaster) {
-      query = this.addPointerPermissions(schema, className, op, query, aclGroup); 
+      query = this.addPointerPermissions(schema, className, op, query, aclGroup);
    }
    if (!query) {
      if (op == 'get') {
@@ -618,7 +617,7 @@ DatabaseController.prototype.find = function(className, query, options = {}) {
    if (!isMaster) {
      mongoWhere = this.transform.addReadACL(mongoWhere, aclGroup);
    }
-    if (options.count) {
+    if (count) {
      delete mongoOptions.limit;
      return collection.count(mongoWhere, mongoOptions);
    } else {
--- a/src/cloud-code/Parse.Hooks.js
+++ b/src/cloud-code/Parse.Hooks.js
@@ -1,6 +1,6 @@
 var request = require("request");
 const send = function(method, path, body) {
-  
+
  var Parse = require("parse/node").Parse;

  var options = {
@@ -12,7 +12,7 @@ const send = function(method, path, body) {
        'Content-Type': 'application/json'
      },
  };
-  
+
  if (body) {
    if (typeof body == "object") {
      options.body = JSON.stringify(body);
@@ -20,7 +20,7 @@ const send = function(method, path, body) {
      options.body = body;
    }
  }
-  
+
  var promise = new Parse.Promise();
  request(options, function(err, response, body){
      if (err) {