Repository: parse-community/Parse-SDK-JS
Pull request #1219 (2020-09-10 20:39:19)

Bug/server 6899/embedded document increment
A bug has been found where incrementing an embedded document property works... until one attempts to **save()** the object.  This will clear out any other properties in the document UNTIL the object is fetched again from the database.  Everything in the database is correct, it is just the local copy that is broken after the **Parse.Object.save()**
[Parse Server bug report](https://github.com/parse-community/parse-server/issues/6899)

I believe this is due to how it handles what is returned from the **save()** and how that is merged with the local data.

I created a new unit test and a new integration test that demonstrate this behavior.

I attempted to resolve the issue with a subsequent commit and although it works when I use the change in my project... it does not result in passing tests.

Would appreciate some insight from @dplewis et al.

dplewis: Thanks for the PR. I also found another internal bug with nested objects. I'll submit it in a separate PR.
dplewis: >  I really wonder if the cleaner fix to this issue would be for the save() to return the entire object even if only part of it changed. Thoughts?

I believe if you add a beforeSave to a class it would return the object. Can you add a beforeSave to the tests?
peril-parse-community[bot]: 
<!--
  0 failure: 
  0 warning: 
  
  1 markdown notices
  DangerID: danger-id-Peril;
-->



Danger run resulted in 1 warning; to find out more, see the [checks page](https://github.com/parse-community/Parse-SDK-JS/runs/1391764595).
<p align="right">
  Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a>
</p>

codecov[bot]: # [Codecov](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=h1) Report
> Merging [#1219](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=desc) (7bd287e) into [master](https://codecov.io/gh/parse-community/Parse-SDK-JS/commit/e155079173eec44e2713942bd02865265ee2c91f?el=desc) (e155079) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219/graphs/tree.svg?width=650&height=150&src=pr&token=8nCENM0QHj)](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1219   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files          57       57           
  Lines        5400     5403    +3     
  Branches     1198     1199    +1     
=======================================
+ Hits         5399     5402    +3     
  Misses          1        1           
```


| [Impacted Files](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/ParseObject.js](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219/diff?src=pr&el=tree#diff-c3JjL1BhcnNlT2JqZWN0Lmpz) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=footer). Last update [e155079...7bd287e](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1219?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

rdhelms: @dplewis @MobileVet The tests should be passing now - rather than checking that the `val` in `_handleSaveResponse` was an object and _not_ any of the Parse classes, I opted for a check that its prototype was the same as `Object.prototype`, which should be false for any instance of any of the Parse classes.

I was also able to verify that this works in our app for our particular use case.

Could you clarify what you meant by adding a `beforeSave` to the tests? I'm happy to add that in as well once I understand the goal there.
rdhelms: On a separate note, I have some questions regarding the optimal dev setup when working in this repo - like how to get the Flow VSCode extension to interpret these flow types correctly (or if the flow types are even still accurate?) and whether the Jest VSCode extension still works with this repo. The Jasmine VSCode extension still seems to work well with the integration tests. Not sure where the best place would be to ask for help about all that...would a separate issue be preferable?

dplewis: @rdhelms I'm actually working on this right now. There are some server issues when using nested objects. I submitted some PR's yesterday

https://github.com/parse-community/parse-server/pull/7005

>  I opted for a check that its prototype was the same as Object.prototype

This is a pretty cool trick. I was trying to figure out a way to simplify this.

> optimal dev setup when working in this repo

A separate issue would be nice to open the discussion. I personally use command line for everything. Too lazy to reinstall the extentions.

Patch:
diff --git a/src/ParseObject.js b/src/ParseObject.js
index 1e778d9..e9f7c68 100644
--- a/src/ParseObject.js
+++ b/src/ParseObject.js
@@ -395,7 +395,7 @@ class ParseObject {
     for (attr in pending) {
       if (pending[attr] instanceof RelationOp) {
         changes[attr] = pending[attr].applyTo(undefined, this, attr);
-      } else if (!(attr in response)) {
+      } else if (!(attr in response) && !attr.includes('.')) {
         // Only SetOps and UnsetOps should not come back with results
         changes[attr] = pending[attr].applyTo(undefined);
       }
@@ -407,7 +407,13 @@ class ParseObject {
       } else if (attr === 'ACL') {
         changes[attr] = new ParseACL(response[attr]);
       } else if (attr !== 'objectId') {
-        changes[attr] = decode(response[attr]);
+        const val = decode(response[attr]);
+        if (val && Object.getPrototypeOf(val) === Object.prototype) {
+          // Update the object by merging in updates w/ old object
+          changes[attr] = { ...this.attributes[attr], ...val }
+        } else {
+          changes[attr] = val
+        }
         if (changes[attr] instanceof UnsetOp) {
           changes[attr] = undefined;
         }


Test patch:
diff --git a/integration/test/ParseObjectTest.js b/integration/test/ParseObjectTest.js
index 2428566..8bde7cd 100644
--- a/integration/test/ParseObjectTest.js
+++ b/integration/test/ParseObjectTest.js
@@ -255,6 +255,27 @@ describe('Parse Object', () => {
     assert.equal(result.get('objectField').number, 20);
   });
 
+  it('can increment nested field and retain full object', async () => {
+    const obj = new TestObject();
+    obj.set('objectField', { number: 5, letter: 'a' });
+    assert.equal(obj.get('objectField').number, 5);
+    assert.equal(obj.get('objectField').letter, 'a');
+    await obj.save();
+
+    obj.increment('objectField.number', 15);
+    assert.equal(obj.get('objectField').number, 20);
+    assert.equal(obj.get('objectField').letter, 'a');
+    await obj.save();
+
+    assert.equal(obj.get('objectField').number, 20);
+    assert.equal(obj.get('objectField').letter, 'a');
+
+    const query = new Parse.Query(TestObject);
+    const result = await query.get(obj.id);
+    assert.equal(result.get('objectField').number, 20);
+    assert.equal(result.get('objectField').letter, 'a');
+  });
+
   it('can increment non existing field', async () => {
     const obj = new TestObject();
     obj.set('objectField', { number: 5 });
diff --git a/src/__tests__/ObjectStateMutations-test.js b/src/__tests__/ObjectStateMutations-test.js
index 9f972d2..5162de9 100644
--- a/src/__tests__/ObjectStateMutations-test.js
+++ b/src/__tests__/ObjectStateMutations-test.js
@@ -120,17 +120,19 @@ describe('ObjectStateMutations', () => {
   });
 
   it('can estimate attributes for nested documents', () => {
-    const serverData = { objectField: { counter: 10 } };
+    const serverData = { objectField: { counter: 10, letter: 'a' } };
     let pendingOps = [{ 'objectField.counter': new ParseOps.IncrementOp(2) }];
     expect(ObjectStateMutations.estimateAttributes(serverData, pendingOps, 'someClass', 'someId')).toEqual({
       objectField: {
-        counter: 12
+        counter: 12,
+        letter: 'a'
       },
     });
     pendingOps = [{ 'objectField.counter': new ParseOps.SetOp(20) }];
     expect(ObjectStateMutations.estimateAttributes(serverData, pendingOps, 'someClass', 'someId')).toEqual({
       objectField: {
-        counter: 20
+        counter: 20,
+        letter: 'a'
       },
     });
   });
diff --git a/src/__tests__/ParseObject-test.js b/src/__tests__/ParseObject-test.js
index 9c40906..62250f8 100644
--- a/src/__tests__/ParseObject-test.js
+++ b/src/__tests__/ParseObject-test.js
@@ -610,20 +610,21 @@ describe('ParseObject', () => {
     o._finishFetch({
       objectId: 'setNested',
       objectField: {
-        number: 5
+        number: 5,
+        letter: 'a'
       },
       otherField: {},
     });
 
     expect(o.attributes).toEqual({
-      objectField: { number: 5 },
+      objectField: { number: 5, letter: 'a' },
       otherField: {},
     });
     o.set('otherField', { hello: 'world' });
     o.set('objectField.number', 20);
 
     expect(o.attributes).toEqual({
-      objectField: { number: 20 },
+      objectField: { number: 20, letter: 'a' },
       otherField: { hello: 'world' },
     });
     expect(o.op('objectField.number') instanceof SetOp).toBe(true);
@@ -634,6 +635,44 @@ describe('ParseObject', () => {
     });
   });
 
+  it('can increment a nested field', () => {
+    const o = new ParseObject('Person');
+    o._finishFetch({
+      objectId: 'incNested',
+      objectField: {
+        number: 5,
+        letter: 'a'
+      },
+    });
+
+    expect(o.attributes).toEqual({
+      objectField: { number: 5, letter: 'a' },
+    });
+    o.increment('objectField.number');
+
+    expect(o.attributes).toEqual({
+      objectField: { number: 6, letter: 'a' },
+    });
+    expect(o.op('objectField.number') instanceof IncrementOp).toBe(true);
+    expect(o.dirtyKeys()).toEqual(['objectField.number', 'objectField']);
+    expect(o._getSaveJSON()).toEqual({
+      'objectField.number': {
+        "__op": "Increment",
+        "amount": 1
+      },
+    });
+
+    // Nested objects only return values changed
+    o._handleSaveResponse({
+      objectId: 'incNested',
+      objectField: {
+        number: 6
+      }
+    })
+    expect(o.get('objectField').number).toEqual(6)
+    expect(o.get('objectField').letter).toEqual('a')
+  });
+
   it('ignore set nested field on new object', () => {
     const o = new ParseObject('Person');
     o.set('objectField.number', 20);