fix: Setting and unsetting property without saving `Parse.Object` causes server error

[mortenmo]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: https://github.com/parse-community/Parse-SDK-JS/issues/1798

Approach

On the path of making my app work well offline, this was one more blocker.

The problem is that if you set a parent object o.set('data', {a: b, b: 3}) and then set or unset anything on data (example: o.unset('data.a') without saving between (or save failed for no connection), you get a server error on save(). 1798 indicates this needs to be fixed on client. What fails seems to be mongo doesn't support setting an object and then sub-properties in the same call which is what the server attempts to do in this case.

This fix merges Ops if a "parent" Op exists locally and is not changed and applies the new Op to that parent. In the end this means that when sent to server it wont create the issue above and succeed even if subproperties were set/unset/incremented as a part of local operation before save.

Small(?) change to ParseObject.set with calling validPendingParentOp that if the current Op attribute has dots (.) it will look for pending parent Operations. If one is found, applyOpToParent is called taking the new Op and applying the change to it locally. Put these new functions in ParseOp, but that might not be the best place for them (maybe ObjectStateController?).

Tasks

• [X] Add tests
• [x] Add test to validate server can save properly now

[parse-github-assistant[bot]]

Thanks for opening this pull request!

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (b50790a) to head (fdba10f). Report is 47 commits behind head on alpha.

Additional details and impacted files

@@            Coverage Diff             @@
##             alpha     #2117    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           61        64     +3     
  Lines         6186      6398   +212     
  Branches      1499      1543    +44     
==========================================
+ Hits          6186      6398   +212     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[dplewis]

Can you check code coverage? Looks like you are missing a test.

[mortenmo]

Another alternative solution that might be preferred is to put a similar fix into ParseObject._getSaveJSON() instead? That means the Ops array stays the same (which is actually what the tests should check I'll fix that, that's in the end what the server cares about). But the data is merged in the saveJSON only.

[dplewis]

@mortenmo Good Work! Can you add an integration test to make sure it's not crashing?

[mortenmo]

@mortenmo Good Work! Can you add an integration test to make sure it's not crashing?

Yes. Took a subset of related testcases and made versions that don't save between the object creation and setting/unsetting making sure the results are the same. Just had to figure out how to run them :)

[mortenmo]

@dplewis type check was failing I assume from the changing CoreManager to ts. I put in a fix here but have to back that out if you fixed it somewhere else as well.

[dplewis]

Yeah sorry for the confusion. Can you revert changes unrelated to the issue? This PR looks good to me. I just to test it myself. Luckily I’ve been looking into dot notation and the internals recently so it shouldn’t take too long

[dplewis]

@mortenmo Can you rebase from alpha? ParseObject was converted to typescript and is causing conflicts in this PR.

[mortenmo]

Made some git mistakes that my git skillz couldn't get me out of, but should be good now.

[mtrezza]

Isn't there more to this issue? In https://github.com/parse-community/Parse-SDK-JS/issues/1798 it's described that it causes an internal server error, which sounds like Parse Server needs a patch as well, to that doesn't crash on malformed request?

[mortenmo]

Isn't there more to this issue? In #1798 it's described that it causes an internal server error, which sounds like Parse Server needs a patch as well, to that doesn't crash on malformed request?

@mtrezza I don't think so. The reason the server "barfed" on the Ops is that Mongo (maybe also Postgres?) doesn't support chained updates when you changed the "parent". When you change/set a property (eg object) to an object and then do a set object.foo to something in the same save, Mongo threw an error. This PR merges those on the client before sending to server (whether it is set or unset), so the server doesn't get those Op commands anymore.

It is unclear to me if the server should support an operation to set object={ ... } and set object.name='foo' in the same save or if the clients should avoid it. It is only a problem if you set a parent and then a "child" property. If you think the server should, then yes there should be another issue opened on the server. But the JS SDK shouldn't trigger that problem anymore, but others might?

[mtrezza]

I mean it should not be possible to cause the server to crash with internal server error. Even if the Parse JS SDK doesn't send this anymore, it would still be possible to send this to Parse Server via the REST API and cause the crash, right?

[mortenmo]

When I tried it manually, server does respond with a 500, but to be clear doesn't completely crash. But yes, it is possible to send through and at a minimum a validation server side with a 400 response is probably better.

[mtrezza]

I believe a 500 error may actually be a process crash.