Repository: parse-community/Parse-SDK-JS
Pull request #1540 (2022-09-04 10:10:10)

feat: generate `Parse.Object.objectId` automatically when `allowCustomObjectId` is enabled and no `objectId` is passed
### New Pull Request Checklist
<!--
    Check every following box [x] before submitting your PR.
    Click the "Preview" tab for better readability.
    Thanks for contributing to Parse Platform!
-->

- [x] I am not disclosing a [vulnerability](https://github.com/parse-community/parse-server/blob/master/SECURITY.md).
- [x] I am creating this PR in reference to an [issue](https://github.com/parse-community/Parse-SDK-JS/issues?q=is%3Aissue).

### Issue Description
Enabling `allowCustomObjectId` forces all objects to have custom `objectId`s instead of auto-generating when no `objectId` is passed, or when `objectId` is **null** or **undefined**. 

Related issue: https://github.com/parse-community/Parse-SDK-JS/issues/1402

### Approach
When `allowCustomObjectId` is enabled:
- Check if `objectId` has a custom value.
- If `objectId` has a value and is an empty string, throw 'objectId must not be empty' error.
- If `objectId` doesn't have a value, or is **null** or **undefined**, generate `objectId` automatically as if `allowCustomObjectId` were disabled.

### TODOs before merging
<!--
    Add TODOs that need to be completed before merging this PR.
    Delete TODOs that do not apply to this PR.
-->

- [x ] Add tests
- [ ] Add entry to changelog
- [ ] Add changes to documentation (guides, repository pages, in-code descriptions)

parse-github-assistant[bot]: I will reformat the title to use the proper commit message syntax.
parse-github-assistant[bot]: 
### Thanks for opening this pull request!

- 🎉 We are excited about your hands-on contribution!

<!-- parse-issue-bot-meta-tag-id {"suggestPr":false,"thanksFeature":false,"thanksPr":true} -->


codecov[bot]: # [Codecov](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1540?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) Report
> Merging [#1540](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) (ec80a44) into [alpha](https://codecov.io/gh/parse-community/Parse-SDK-JS/commit/e08f3b6f85ac16ff57da00b044b70159eaea95c1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) (e08f3b6) will **not change** coverage.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##            alpha    #1540   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          61       61           
  Lines        5959     5959           
  Branches     1361     1361           
=======================================
  Hits         5956     5956           
  Misses          3        3           
```


| [Impacted Files](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) | Coverage Δ | |
|---|---|---|
| [src/ParseObject.js](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-c3JjL1BhcnNlT2JqZWN0Lmpz) | `99.89% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community)

musthafa1996: Thanks for reviewing @dblythy! I have included your suggestions.
musthafa1996: Hi @dblythy! Were you able to have a look at the recent changes pushed to the PR?
dblythy: You might need to run `npm run lint` and fix syntax errors. I couldn't suggest changes to be syntactically correct as I couldn't provide suggestions across deleted lines. 
musthafa1996: > You might need to run `npm run lint` and fix syntax errors. I couldn't suggest changes to be syntactically correct as I couldn't provide suggestions across deleted lines.

Thanks. I’ll run and post the results.
musthafa1996: @dblythy, my bad. I have just fixed the syntax error and updated the PR. Please have a look now.

The following is the result of `nom run lint`:
`mohammed@Mohammeds-Mac-mini Parse-SDK-JS % npm run lint

> parse@3.4.4-alpha.2 lint
> eslint --cache src/ integration/

mohammed@Mohammeds-Mac-mini Parse-SDK-JS % `
dblythy: I will review again within 24 hrs 😊 
musthafa1996: Thank you 😊 
musthafa1996: > Can you check the integration check? `npm run integration`

Hi @dblythy and @mtrezza! I did get some time to look into this PR today. I have pushed the following updates:

- Remove unnecessary id existence check from `ParseObject` in case allowCustomObjectId is true.
- Fix tests for allowCustomObjectId.

Turns out that we don't need the following checks in `ParseObject` as they are already validated from ParseServer.
![Screenshot 2022-09-10 at 2 09 40 AM](https://user-images.githubusercontent.com/22429514/18
![Screenshot 2022-09-10 at 2 09 55 AM](https://user-images.githubusercontent.com/22429514/189439321-4d7c8b8b-15c0-4325-b747-07dcefc42913.png)
9439291-cbd0cb1e-9a97-457d-8670-709b62f1887e.png)

All the related tests seem to work fine now.

musthafa1996: There is one thing to note here. The error received from ParseServer says 'objectId must not be empty, null or undefined'. Yes, object saving fails when the `objectId` is **empty** or **null**. However, saving succeeds when `objectId` is **undefined** with automatic objectId generated. 

We probably need to update the error message on ParseServer. Please advice.
dblythy: > Turns out that we don't need the following checks in ParseObject as they are already validated from ParseServer

Even though the checks happen on Parse Server, the client side validation prevents unnecessary computation / network calls. There's no downside to have them run on the client side as well
musthafa1996: Okay. Let me add them back in that case.
musthafa1996: @dblythy, @mtrezza, I have added the validation back. All the test cases are also passed.
mtrezza: Could you take a look at the failing CI?
musthafa1996: @mtrezza, I have fixed the **packages-lock.json** file. Can you run the build again?
musthafa1996: @dblythy, pushed the **ParseObjectTes.js** updates. Can you check again?
musthafa1996: @dblythy, can you do a review?
dblythy: It's mostly good - just need to remove the `done()` from the tests
musthafa1996: @dblythy, the test starts failing when I remove `done()` from `allowCustomObjectId` test in **ParseObject-test.js**. Please check the screenshots below and share your thoughts.

With `done()`:
![Screenshot 2022-09-16 at 9 16 24 PM](https://user-images.githubusercontent.com/22429514/190678628-b68aeb2c-943b-4b8b-a85b-ae4b6ede17e8.png)

Without `done()`:
![Screenshot 2022-09-16 at 9 17 58 PM](https://user-images.githubusercontent.com/22429514/190678912-a037d8c1-49f0-40cc-945f-e736f0a51a28.png)


musthafa1996: @dblythy any idea why?
dblythy: when you remove `done()` you also must remove `async done => {`

So, remove `done()` but also change the function to `async () => {`
musthafa1996: Thanks :) 
Let me try that. I'm new to testing by the way :D
dblythy: No worries! It should work if you make the changes as per my [previous review](https://github.com/parse-community/Parse-SDK-JS/pull/1540#pullrequestreview-1103567948). Feel free to let us know if you need any more help!
musthafa1996: @dblythy, this is what I get after removing `done()`. Lint doesn't identify `fail()` method at lines 3822 and 3828,
![Screenshot 2022-09-19 at 12 23 32 PM](https://user-images.githubusercontent.com/22429514/190963744-956bcc3e-454c-432c-b164-ce107b44f08e.png)

musthafa1996: @dblythy, I have resolved the tests. Please review.
musthafa1996: What's pending for this PR to be merged now @dblythy, @mtrezza?
mtrezza: Waiting for CI to pass
musthafa1996: CI has been completed successfully.
mtrezza: @dblythy Is this ready for merge also from your side?
musthafa1996: @mtrezza, is there any actions required from me to get this merged?

Patch:
diff --git a/src/ParseObject.js b/src/ParseObject.js
index 87738d5..6e838b7 100644
--- a/src/ParseObject.js
+++ b/src/ParseObject.js
@@ -2399,12 +2399,6 @@ const DefaultController = {
         if (el instanceof ParseFile) {
           filesSaved.push(el.save(options));
         } else if (el instanceof ParseObject) {
-          if (allowCustomObjectId && !el.id) {
-            throw new ParseError(
-              ParseError.MISSING_OBJECT_ID,
-              'objectId must not be empty, null or undefined'
-            );
-          }
           pending.push(el);
         }
       });
@@ -2419,6 +2413,13 @@ const DefaultController = {
             const batch = [];
             const nextPending = [];
             pending.forEach(el => {
+              if (allowCustomObjectId && Object.prototype.hasOwnProperty.call(el, 'id') && !el.id) {
+                throw new ParseError(
+                  ParseError.MISSING_OBJECT_ID,
+                  'objectId must not be empty or null'
+                );
+              }
+
               if (batch.length < batchSize && canBeSerialized(el)) {
                 batch.push(el);
               } else {
@@ -2498,11 +2499,8 @@ const DefaultController = {
         });
       });
     } else if (target instanceof ParseObject) {
-      if (allowCustomObjectId && !target.id) {
-        throw new ParseError(
-          ParseError.MISSING_OBJECT_ID,
-          'objectId must not be empty, null or undefined'
-        );
+      if (allowCustomObjectId && Object.prototype.hasOwnProperty.call(target, 'id') && !target.id) {
+        throw new ParseError(ParseError.MISSING_OBJECT_ID, 'objectId must not be empty or null');
       }
       // generate _localId in case if cascadeSave=false
       target._getId();


Test patch:
diff --git a/integration/test/ParseObjectTest.js b/integration/test/ParseObjectTest.js
index 64e4a42..8efda1c 100644
--- a/integration/test/ParseObjectTest.js
+++ b/integration/test/ParseObjectTest.js
@@ -2060,59 +2060,135 @@ describe('Parse Object', () => {
     expect(obj.get('string')).toBeInstanceOf(String);
   });
 
-  it('allowCustomObjectId', async () => {
-    await reconfigureServer({ allowCustomObjectId: true });
-    Parse.allowCustomObjectId = true;
-    const customId = `${Date.now()}`;
-    const object = new Parse.Object('TestObject');
-    try {
+  describe('allowCustomObjectId', () => {
+    it('can save without setting an objectId', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const object = new Parse.Object('TestObject');
       await object.save();
-      fail();
-    } catch (error) {
-      expect(error.message).toBe('objectId must not be empty, null or undefined');
-    }
-    object.id = customId;
-    object.set('foo', 'bar');
-    await object.save();
-    expect(object.id).toBe(customId);
+      expect(object.id).toBeDefined();
+
+      Parse.allowCustomObjectId = false;
+    });
+
+    it('fails to save when objectId is empty', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const object = new Parse.Object('TestObject');
+      object.id = '';
+      await expectAsync(object.save()).toBeRejectedWith(
+        new Parse.Error(Parse.Error.MISSING_OBJECT_ID, 'objectId must not be empty or null')
+      );
+
+      Parse.allowCustomObjectId = false;
+    });
+
+    it('fails to save when objectId is null', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const object = new Parse.Object('TestObject');
+      object.id = null;
+      await expectAsync(object.save()).toBeRejectedWith(
+        new Parse.Error(Parse.Error.MISSING_OBJECT_ID, 'objectId must not be empty or null')
+      );
+
+      Parse.allowCustomObjectId = false;
+    });
+
+    it('can save with custom objectId', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const customId = `${Date.now()}`;
+      const object = new Parse.Object('TestObject');
+      object.id = customId;
+      object.set('foo', 'bar');
+      await object.save();
+      expect(object.id).toBe(customId);
+
+      const query = new Parse.Query('TestObject');
+      const result = await query.get(customId);
+      expect(result.get('foo')).toBe('bar');
+      expect(result.id).toBe(customId);
 
-    const query = new Parse.Query('TestObject');
-    const result = await query.get(customId);
-    expect(result.get('foo')).toBe('bar');
-    expect(result.id).toBe(customId);
+      result.set('foo', 'baz');
+      await result.save();
 
-    result.set('foo', 'baz');
-    await result.save();
+      const afterSave = await query.get(customId);
+      expect(afterSave.get('foo')).toBe('baz');
 
-    const afterSave = await query.get(customId);
-    expect(afterSave.get('foo')).toBe('baz');
-    Parse.allowCustomObjectId = false;
+      Parse.allowCustomObjectId = false;
+    });
   });
 
-  it('allowCustomObjectId saveAll', async () => {
-    await reconfigureServer({ allowCustomObjectId: true });
-    Parse.allowCustomObjectId = true;
-    const customId1 = `${Date.now()}`;
-    const customId2 = `${Date.now()}`;
-    const obj1 = new TestObject({ foo: 'bar' });
-    const obj2 = new TestObject({ foo: 'baz' });
-    try {
+  describe('allowCustomObjectId saveAll', () => {
+    it('can save without setting an objectId', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const obj1 = new TestObject({ foo: 'bar' });
+      const obj2 = new TestObject({ foo: 'baz' });
       await Parse.Object.saveAll([obj1, obj2]);
-      fail();
-    } catch (error) {
-      expect(error.message).toBe('objectId must not be empty, null or undefined');
-    }
-    obj1.id = customId1;
-    obj2.id = customId2;
-    await Parse.Object.saveAll([obj1, obj2]);
-    expect(obj1.id).toBe(customId1);
-    expect(obj2.id).toBe(customId2);
+      expect(obj1.id).toBeDefined();
+      expect(obj2.id).toBeDefined();
 
-    const query = new Parse.Query(TestObject);
-    const results = await query.find();
-    results.forEach(result => {
-      expect([customId1, customId2].includes(result.id));
+      Parse.allowCustomObjectId = false;
+    });
+
+    it('fails to save when objectId is empty', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const obj1 = new TestObject({ foo: 'bar' });
+      obj1.id = '';
+      const obj2 = new TestObject({ foo: 'baz' });
+      obj2.id = '';
+      await expectAsync(Parse.Object.saveAll([obj1, obj2])).toBeRejectedWith(
+        new Parse.Error(Parse.Error.MISSING_OBJECT_ID, 'objectId must not be empty or null')
+      );
+
+      Parse.allowCustomObjectId = false;
+    });
+
+    it('fails to save when objectId is null', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const obj1 = new TestObject({ foo: 'bar' });
+      obj1.id = null;
+      const obj2 = new TestObject({ foo: 'baz' });
+      obj2.id = null;
+      await expectAsync(Parse.Object.saveAll([obj1, obj2])).toBeRejectedWith(
+        new Parse.Error(Parse.Error.MISSING_OBJECT_ID, 'objectId must not be empty or null')
+      );
+
+      Parse.allowCustomObjectId = false;
+    });
+
+    it('can save with custom objectId', async () => {
+      await reconfigureServer({ allowCustomObjectId: true });
+      Parse.allowCustomObjectId = true;
+
+      const obj1 = new TestObject({ foo: 'bar' });
+      const customId1 = `${Date.now()}`;
+      obj1.id = customId1;
+      const obj2 = new TestObject({ foo: 'baz' });
+      const customId2 = `${Date.now()}`;
+      obj1.id = customId2;
+      await Parse.Object.saveAll([obj1, obj2]);
+      expect(obj1.id).toBeDefined();
+      expect(obj2.id).toBeDefined();
+
+      const query = new Parse.Query(TestObject);
+      const results = await query.find();
+      results.forEach(result => {
+        expect([customId1, customId2].includes(result.id));
+      });
+
+      Parse.allowCustomObjectId = false;
     });
-    Parse.allowCustomObjectId = false;
   });
 });
diff --git a/integration/test/helper.js b/integration/test/helper.js
index dbd0df1..836953f 100644
--- a/integration/test/helper.js
+++ b/integration/test/helper.js
@@ -72,6 +72,7 @@ const defaultConfiguration = {
     enableForAuthenticatedUser: true,
   },
   revokeSessionOnPasswordReset: false,
+  allowCustomObjectId: false,
 };
 
 const openConnections = {};
diff --git a/src/__tests__/ParseObject-test.js b/src/__tests__/ParseObject-test.js
index e8294ae..0e35f56 100644
--- a/src/__tests__/ParseObject-test.js
+++ b/src/__tests__/ParseObject-test.js
@@ -3807,27 +3807,22 @@ describe('ParseObject pin', () => {
     });
   });
 
-  it('can allowCustomObjectId', async done => {
+  it('can allowCustomObjectId', async () => {
     CoreManager.set('ALLOW_CUSTOM_OBJECT_ID', true);
     const o = new ParseObject('Person');
+    o.id = '';
     let params = o._getSaveParams();
     expect(params).toEqual({
       method: 'POST',
-      body: { objectId: undefined },
+      body: { objectId: '' },
       path: 'classes/Person',
     });
-    try {
-      await o.save();
-      done.fail();
-    } catch (error) {
-      expect(error.message).toBe('objectId must not be empty, null or undefined');
-    }
-    try {
-      await ParseObject.saveAll([o]);
-      done.fail();
-    } catch (error) {
-      expect(error.message).toBe('objectId must not be empty, null or undefined');
-    }
+    await expect(o.save()).rejects.toEqual(
+      new ParseError(ParseError.MISSING_OBJECT_ID, 'objectId must not be empty or null')
+    );
+    await expect(ParseObject.saveAll([o])).rejects.toEqual(
+      new ParseError(ParseError.MISSING_OBJECT_ID, 'objectId must not be empty or null')
+    );
     o._finishFetch({
       objectId: 'CUSTOM_ID',
       createdAt: { __type: 'Date', iso: new Date().toISOString() },
@@ -3840,6 +3835,5 @@ describe('ParseObject pin', () => {
       path: 'classes/Person/CUSTOM_ID',
     });
     CoreManager.set('ALLOW_CUSTOM_OBJECT_ID', false);
-    done();
   });
 });