Fix for #1840, Strip operations from results, forwards delete operations to SDKs (#1946)

* Adding a test demonstrating issue #1840.

* Fixes #1840

* Adds failing test with other use case

- That test fails on parse.com as well

* Bumps parse to 1.9.0

* exclude pg db

* Exclude pg on other test

* Adds clientSDK compatibility check for forward deletion

- Mark js1.9.0 as compatible

* Strips all operations from result

- fix for #1606

Author: flovilmart

package.json

@@ -42,6 +42,7 @@
     "redis": "2.6.2",
     "request": "2.73.0",
     "request-promise": "3.0.0",
+    "semver": "^5.2.0",
     "tv4": "1.2.7",
     "winston": "2.2.0",
     "winston-daily-rotate-file": "1.1.5",

spec/ClientSDK.spec.js

@@ -0,0 +1,41 @@
+var ClientSDK = require('../src/ClientSDK');
+
+describe('ClientSDK', () => {
+  it('should properly parse the SDK versions', () => {
+      let clientSDKFromVersion = ClientSDK.fromString;
+      expect(clientSDKFromVersion('i1.1.1')).toEqual({
+          sdk: 'i',
+          version: '1.1.1'
+      });
+      expect(clientSDKFromVersion('i1')).toEqual({
+          sdk: 'i',
+          version: '1'
+      });
+      expect(clientSDKFromVersion('apple-tv1.13.0')).toEqual({
+          sdk: 'apple-tv',
+          version: '1.13.0'
+      });
+      expect(clientSDKFromVersion('js1.9.0')).toEqual({
+          sdk: 'js',
+          version: '1.9.0'
+      });
+  });
+  
+  it('should properly sastisfy', () => {
+    expect(ClientSDK.compatible({
+      js: '>=1.9.0'
+    })("js1.9.0")).toBe(true);
+    
+    expect(ClientSDK.compatible({
+      js: '>=1.9.0'
+    })("js2.0.0")).toBe(true);
+
+    expect(ClientSDK.compatible({
+      js: '>=1.9.0'
+    })("js1.8.0")).toBe(false);
+
+    expect(ClientSDK.compatible({
+      js: '>=1.9.0'
+    })(undefined)).toBe(true);
+  })
+})

spec/CloudCode.spec.js

@@ -666,4 +666,114 @@ describe('Cloud Code', () => {
       done();
     });
   });
+
+  it_exclude_dbs(['postgres'])('should fully delete objects when using `unset` with beforeSave (regression test for #1840)', done => {
+    var TestObject = Parse.Object.extend('TestObject');
+    var NoBeforeSaveObject = Parse.Object.extend('NoBeforeSave');
+    var BeforeSaveObject = Parse.Object.extend('BeforeSaveChanged');
+
+    Parse.Cloud.beforeSave('BeforeSaveChanged', (req, res) => {
+      var object = req.object;
+      object.set('before', 'save');
+      res.success();
+    });
+
+    Parse.Cloud.define('removeme', (req, res) => {
+      var testObject = new TestObject();
+      testObject.save()
+      .then(testObject => {
+        var object = new NoBeforeSaveObject({remove: testObject});
+        return object.save();
+      })
+      .then(object => {
+        object.unset('remove');
+        return object.save();
+      })
+      .then(object => {
+        res.success(object);
+      });
+    });
+
+    Parse.Cloud.define('removeme2', (req, res) => {
+      var testObject = new TestObject();
+      testObject.save()
+      .then(testObject => {
+        var object = new BeforeSaveObject({remove: testObject});
+        return object.save();
+      })
+      .then(object => {
+        object.unset('remove');
+        return object.save();
+      })
+      .then(object => {
+        res.success(object);
+      });
+    });
+
+    Parse.Cloud.run('removeme')
+    .then(aNoBeforeSaveObj => {
+      expect(aNoBeforeSaveObj.get('remove')).toEqual(undefined);
+
+      return Parse.Cloud.run('removeme2');
+    })
+    .then(aBeforeSaveObj => {
+      expect(aBeforeSaveObj.get('before')).toEqual('save');
+      expect(aBeforeSaveObj.get('remove')).toEqual(undefined);
+      done();
+    });
+  });
+
+  it_exclude_dbs(['postgres'])('should fully delete objects when using `unset` with beforeSave (regression test for #1840)', done => {
+    var TestObject = Parse.Object.extend('TestObject');
+    var BeforeSaveObject = Parse.Object.extend('BeforeSaveChanged');
+
+    Parse.Cloud.beforeSave('BeforeSaveChanged', (req, res) => {
+      var object = req.object;
+      object.set('before', 'save');
+      object.unset('remove');
+      res.success();
+    });
+
+    let object;
+    let testObject = new TestObject({key: 'value'});
+    testObject.save().then(() => {
+       object = new BeforeSaveObject();
+       return object.save().then(() => {
+          object.set({remove:testObject})
+          return object.save();
+       });
+    }).then((objectAgain) => {
+       expect(objectAgain.get('remove')).toBeUndefined();
+       expect(object.get('remove')).toBeUndefined();
+       done();
+    }).fail((err) => {
+      console.error(err);
+      done();
+    })
+  });
+
+  it_exclude_dbs(['postgres'])('should not include relation op (regression test for #1606)', done => {
+    var TestObject = Parse.Object.extend('TestObject');
+    var BeforeSaveObject = Parse.Object.extend('BeforeSaveChanged');
+    let testObj;
+    Parse.Cloud.beforeSave('BeforeSaveChanged', (req, res) => {
+      var object = req.object;
+      object.set('before', 'save');
+      testObj = new TestObject();
+      testObj.save().then(() => {
+        object.relation('testsRelation').add(testObj);
+        res.success();
+      })
+    });
+
+    let object = new BeforeSaveObject();
+    object.save().then((objectAgain) => {
+      // Originally it would throw as it would be a non-relation
+      expect(() => { objectAgain.relation('testsRelation') }).not.toThrow();
+      done();
+    }).fail((err) => {
+      console.error(err);
+      done();
+    })
+  });
 });

spec/Middlewares.spec.js

@@ -66,24 +66,4 @@ describe('middlewares', () => {
             });
         });
     });
-
-    it('should properly parse the SDK versions', () => {
-        let clientSDKFromVersion = middlewares.clientSDKFromVersion;
-        expect(clientSDKFromVersion('i1.1.1')).toEqual({
-            sdk: 'i',
-            version: '1.1.1'
-        });
-        expect(clientSDKFromVersion('i1')).toEqual({
-            sdk: 'i',
-            version: '1'
-        });
-        expect(clientSDKFromVersion('apple-tv1.13.0')).toEqual({
-            sdk: 'apple-tv',
-            version: '1.13.0'
-        });
-        expect(clientSDKFromVersion('js1.9.0')).toEqual({
-            sdk: 'js',
-            version: '1.9.0'
-        });
-    })
 });

src/ClientSDK.js

@@ -0,0 +1,40 @@
+var semver = require('semver');
+
+function compatible(compatibleSDK) {
+  return function(clientSDK) {
+    if (typeof clientSDK === 'string') {
+      clientSDK = fromString(clientSDK);
+    }
+    // REST API, or custom SDK
+    if (!clientSDK) {
+      return true;
+    }
+    let clientVersion = clientSDK.version;
+    let compatiblityVersion = compatibleSDK[clientSDK.sdk];
+    return semver.satisfies(clientVersion, compatiblityVersion);
+  }
+}
+
+function supportsForwardDelete(clientSDK) {
+  return compatible({
+    js: '>=1.9.0'
+  })(clientSDK);
+}
+
+function fromString(version) {
+  let versionRE = /([-a-zA-Z]+)([0-9\.]+)/;
+  let match = version.toLowerCase().match(versionRE);
+  if (match && match.length === 3) {
+    return {
+      sdk: match[1],
+      version: match[2]
+    }
+  }
+  return undefined;
+}
+
+module.exports = {
+  compatible,
+  supportsForwardDelete,
+  fromString
+}

src/RestWrite.js

@@ -11,6 +11,7 @@ var cryptoUtils = require('./cryptoUtils');
 var passwordCrypto = require('./password');
 var Parse = require('parse/node');
 var triggers = require('./triggers');
+var ClientSDK = require('./ClientSDK');
 import RestQuery from './RestQuery';
 import _         from 'lodash';
 
@@ -774,9 +775,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
     .then(response => {
       response.updatedAt = this.updatedAt;
       if (this.storage.changedByTrigger) {
-        Object.keys(this.data).forEach(fieldName => {
-          response[fieldName] = response[fieldName] || this.data[fieldName];
-        });
+        this.updateResponseWithData(response, this.data);
       }
       this.response = { response };
     });
@@ -834,9 +833,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
         response.username = this.data.username;
       }
       if (this.storage.changedByTrigger) {
-        Object.keys(this.data).forEach(fieldName => {
-          response[fieldName] = response[fieldName] || this.data[fieldName];
-        });
+        this.updateResponseWithData(response, this.data);
       }
       this.response = {
         status: 201,
@@ -925,5 +922,24 @@ RestWrite.prototype.cleanUserAuthData = function() {
   }
 };
 
+RestWrite.prototype.updateResponseWithData = function(response, data) {
+  let clientSupportsDelete = ClientSDK.supportsForwardDelete(this.clientSDK);
+  Object.keys(data).forEach(fieldName => {
+    let dataValue = data[fieldName];
+    let responseValue = response[fieldName];
+
+    response[fieldName] = responseValue || dataValue;
+    
+    // Strips operations from responses
+    if (response[fieldName] && response[fieldName].__op) {
+      delete response[fieldName];
+      if (clientSupportsDelete && dataValue.__op == 'Delete') {
+        response[fieldName] = dataValue;
+      }
+    }
+  });
+  return response;
+}
+
 export default RestWrite;
 module.exports = RestWrite;

src/middlewares.js

@@ -5,17 +5,7 @@ var Parse = require('parse/node').Parse;
 
 var auth = require('./Auth');
 var Config = require('./Config');
-
-function clientSDKFromVersion(version) {
-  let versionRE = /([-a-zA-Z]+)([0-9\.]+)/;
-  let match = version.toLowerCase().match(versionRE);
-  if (match && match.length === 3) {
-    return {
-      sdk: match[1],
-      version: match[2]
-    }
-  }
-}
+var ClientSDK = require('./ClientSDK');
 
 // Checks that the request is authorized for this app and checks user
 // auth too.
@@ -106,7 +96,7 @@ function handleParseHeaders(req, res, next) {
   }
 
   if (info.clientVersion) {
-    info.clientSDK = clientSDKFromVersion(info.clientVersion);
+    info.clientSDK = ClientSDK.fromString(info.clientVersion);
   }
 
   if (fileViaJSON) {
@@ -300,5 +290,4 @@ module.exports = {
   handleParseHeaders: handleParseHeaders,
   enforceMasterKeyAccess: enforceMasterKeyAccess,
   promiseEnforceMasterKeyAccess,
-  clientSDKFromVersion
 };