Bug/server 6899/embedded document increment

[MobileVet]

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

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]

typeof val === 'object' should fix the tests

[MobileVet]

@dplewis Thanks for taking a look. Unfortunately it does not.

Interestingly, the code as it is written only fails on the new tests added. However, changing it to typeof val === 'object' per your suggestion results in 2 failures in legacy unit tests.

 FAIL  src/__tests__/ParseObject-test.js
  ● ParseObject › commits changes to server data when saved

    expect(received).toEqual(expected) // deep equality

    - Expected
    + Received

      Object {
        "age": 24,
    -   "updatedAt": 2020-09-14T12:57:56.944Z,
    +   "updatedAt": Object {},
      }

      1010 |       updatedAt: { __type: 'Date', iso: updated.toISOString() }
      1011 |     });
    > 1012 |     expect(p._getServerData()).toEqual({
           |                                ^
      1013 |       updatedAt: updated,
      1014 |       age: 24
      1015 |     });

      at Object.it (src/__tests__/ParseObject-test.js:1012:32)

  ● ParseObject › interpolates delete operations

    expect(received).toBeUndefined()

    Received: {}

      1289 |     o.save({ key: 'value', deletedKey: 'keyToDelete' }).then(() => {
      1290 |       expect(o.get('key')).toBe('value');
    > 1291 |       expect(o.get('deletedKey')).toBeUndefined();
           |                                   ^
      1292 |       done();
      1293 |     });
      1294 |   });

      at o.save.then (src/__tests__/ParseObject-test.js:1291:35)

[dplewis]

At least your test cases passed. I can look into the other tests.

[dplewis]

type object can be many fields. These tests are failing for type Date and Unset. Look into decode.js for additional types.

Try

if (val && typeof val === 'object'
          && !(val instanceof Array)
          && !(val instanceof ParseObject)
          && !(val instanceof ParseFile)
          && !(val instanceof ParseRelation)
          && !(val instanceof Op)
          && !(Object.prototype.toString.call(val) === '[object Date]')) {

[dplewis]

Might have to add check for ParseGeoPoint and ParsePolygon and test cases for those.

[MobileVet]

Might have to add check for ParseGeoPoint and ParsePolygon and test cases for those.

@dplewis Ok, thanks... will check that out in a bit.

The other changes did result in passing unit tests. However, I am still failing the integration test I created. I really don't understand how considering that I have seen it work as expected when actually used in practice. The test is super simple... The issue is the local value of obj.objectField AFTER the save(). 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?

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');
  });

[dplewis]

Are you running npm run watch:node locally for testing? On the travis build looks like the tests you wrote passed. The failing test might have to do with the ParseGeopoint and ParsePolygon

[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 full object. Can you add a beforeSave to the tests (integration/test/main.js)?

[codecov]

Codecov Report

Merging #1219 (7bd287e) into master (e155079) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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	Coverage Δ
src/ParseObject.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e155079...7bd287e. Read the comment docs.

[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

parse-community/parse-server#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.

[dplewis]

LGTM!