Add sending unsaved ParseObjects to Clients

[fastrde]

Converts the ParseObject _toFullJson when no id is set. So you are able to send unsaved ParseObjects to the client to unify the communication when you retrieved the data previously from third party and are not allowed to save the data in your Parse-DB.

see parse-community/parse-server#7004 for the complete discussion.

[ghost]

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by 🚫 dangerJS

[codecov]

Codecov Report

Merging #1256 (d1b9302) into master (49df2f5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1256   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files          57       57           
  Lines        5405     5412    +7     
  Branches     1199     1202    +3     
=======================================
+ Hits         5404     5411    +7     
  Misses          1        1           

Impacted Files	Coverage Δ
src/encode.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 49df2f5...d1b9302. Read the comment docs.

[davimacedo]

@dplewis any thoughts? I'm afraid of having counter effects with this change. @fastrde tests are also failing. Also, why don't you just use obj._toFullJSON(). @dplewis maybe we could think about removing the _ from this method or just create a new that call it to avoid breaking changes ad I'm pretty sure that a lot of people use it. Thoughts?

[fastrde]

@fastrde tests are also failing.
@davimacedo that's strange. That's the result for the tests on my machine. Can you tell me what's failing exactly?

$ npm test
...
PASS  src/__tests__/encode-test.js
...
Test Suites: 47 passed, 47 total
Tests:       957 passed, 957 total
Snapshots:   0 total
Time:        10.806s
Ran all test suites.

Also, why don't you just use obj._toFullJSON()

obj._toFullJSON() works for one Object but when you have a (unsaved) Parse.Object what contains more (unsaved) Parse.Objects in an attribute this Child-Objects are encoded with toPointer again and throws "Pointer from unsaved Object".

[davimacedo]

You can check here: https://travis-ci.com/github/parse-community/Parse-SDK-JS/jobs/442423259

There are lint issues and a failing test case.

Talking about the pointers, I believe that's the expected behavior for referenced objects. To have them as pointers but with all data available.

[fastrde]

Can't reproduce the failing test on my local machine. (used the same commands that shown in the travis console)
(Edit: looks like an temporary issue at travis-ci yesterday. Didn't change anything expect the trailing spaces and the test now passes also on travis-ci.)

Pending:

1) Geo Point minimum 3 points withinPolygon
  Temporarily disabled with xit

722 specs, 0 failures, 1 pending spec
Finished in 42.075 seconds

to the pointers: when the objects are not saved, where should point the pointers to? When the objects are saved they have an id so the toPointer method is called and the behavior stays the same. When they are unsaved they get converted _toFullJSON when send to the client. Because the id is undefined the client thinks the object is a local object when it calls _getId() and on the client side it looks like there was no server involved before (what is great for objects that didn't exist in the parse-DB).

Perhaps there are side effects I've overlooked. To make the change less scary it were a possibility to explicitly mark objects temporary (like the offline flag). So only when someone explicitly marked the objects they get converted _toFullJSON what then would work for child objects (recursivly) too, when they are also marked as temporary.

[dplewis]

Can you remove this?

[fastrde]

when i remove this line id is undefined and the new introduced if (value.id === undefined) matches and the test encode ParseObjects fails because the test expects a pointer. But this is exactly the case I want to treat differently. When there is no id toPointer should not be called and the object should be send as full json.

  ● encode › encodes ParseObjects
    Expected: "POINTER"
    Received: {"__type": "Object", "className": "Item"}

[dplewis]

Update the test by setting an id within the test. The MockObject should closely resemble a real ParseObject.

[dplewis]

I'm afraid of having counter effects with this change

toPointer behavior should stay the same. The first thing that comes to mind would be a recursive encoding. Is there any way that this could cause an infinite recursion issue?

Edit: Actually can you create an integration tests that shows toFullJSON you added will be called? (Testing this with cloud code might be difficult)

[fastrde]

@dplewis you are right. The recursive encoding with toFullJSON runs in an endless loop when Parent -> Child -> Parent relations exists. Is it ok to throw an error in encode.js in this case?

Now, after days of trying to get the tests working I know exactly what

(Testing this with cloud code might be difficult)

means...
The server that is starting depends on the published parse version. In my tries I patched this version with the one I build.
Something like this:

Parse._encode = myParse._encode;
Parse.Object = myParse.Object;

With this patches in place the encoding of temporary objects works and throws an error on circular encoding. The new integration test runs, but 4 other tests fail. This tests fail also when I patch the published parse version with the master branch of the sdk.

[dplewis]

That’s a cool workaround, we could add a way to inject the SDK instead of using the published version. I’ve been thinking about that for a while now. Currently we assume it works before merging it with the server.

@davimacedo what do you think?

[davimacedo]

Passing the Parse SDK to Parse Server via a configuration option makes sense to me. It looks that it could solve this problem and it would be helpful for other scenarios as well.

In addition, the cross dependency between the projects makes maintenance harder and I believe that we should pursue to put the two projects together in the same mono repository.

[dplewis]

@fastrde @davimacedo Sorry for lack of feedback. I didn't realize that you have a check for circular cycles. Coincidently we need to check for cycles in transaction PR #1090