Parcelable object #625

natario1 · 2017-04-04T22:51:34Z

This is a working stub that implements Parcelable interface into ParseObject ( #122 ).

Implements Parcelable into ParseACL
Implements Parcelable into ParseRelation
Implements Parcelable into ParseObject. It parcels both server data and dirty data. Internally this is done by parceling the ParseObject.State for server data, and the ParseOperationSets attached to the object. These sets are squashed into a single ParseOperationSet, as outlined by grantland in ParseObject implements Parcelable #122 , and the merged set is saved to parcel.
Creates ParseParcelEncoder and ParseParcelDecoder, utility classes that help parceling when we host untyped lists (e.g. Map<String, Object>). They work by saving a type string into the parcel and checking that when unparceling. Eventually these classes will be extended to work for ParseQuery as well.
Introduces (untested) smooth parceling for ParseObject subclasses. They do not need to provide a CREATOR static field. Instead, we have two new protected methods in ParseObject which should be familiar to any developer, onSaveInstanceState(Bundle) and onRestoreInstanceState(Bundle). For example, they are used in ParseUser to keep the isCurrentUser boolean. We pass a Bundle because that’s much safer than a Parcel.
Should work out of the box for internal subclasses: ParsePin, ParseInstallation, ParseSession, ParseRole, EventuallyPin and finally ParseUser
Special treatment for ParseUser since it has an internal State class and own fields (isNew, isCurrentUser).
Warns the developer (with Log.w) if the object about to be parceled has ongoing save tasks / scheduled saveEventually tasks, to make him aware of the implications outlined by grantland in ParseObject implements Parcelable #122 .
Warns the developer (with Log.w) if the object has ongoing delete tasks / scheduled deleteEventually tasks, as above

Approach

My approach was to never give Parcelable to inner classes (e.g. ParseObject.State, ParseACL.Permissions) even if they need to be parceled. That is because unparceling requires the CREATOR field to be publicly accessible, and we don’t want to make ParseObject.State public.

Instead, simple methods like writeToParcel or constructor(Parcel) are implemented to inner classes, and are called from the outer public class. Internally this ensures also inheritance when needed (e.g. ParseUser.State that extends ParseObject.State). We use Parcelable only when the developer would benefit from it.

Consistency

I built this trying to keep consistency with JSON encoding / decoding that is already built in into classes. For example

ParseParcelDecoder / Encoder works just like ParseDecoder / Encoder for JSON
ParseOperationSet has toRest(ParseEncoder) and fromRest(JSONObject, ParseDecoder). Added toParcel(Parcel, ParseParcelEncoder) and fromParcel(Parcel, ParseParcelDecoder)
ParseFieldOperation interface has encode(ParseEncoder) and now encode(Parcel, ParseParcelEncoder)
Same for ParseFieldOperationFactory for decoding

Recursiveness

This now avoids StackOverflowErrors caused by nested ParseObject in the object tree. It’s pretty common for a ParseObject to be self referenced by one of its keys. This can be explicit (e.g. object.put(“itself”, object) or more obscure (e.g. ParseRelation is parceled when the parent object is parceled, but the relation itself holds a reference to the parent...).

This is solved by using stateful encoders / decoders called ParseObjectParcelEncoder and ParseObjectParcelDecoder. When the encoder traverses the objects / keys tree,

it holds a reference to parceled objects
when asked to parcel an object that was already parceled, it is parceled as a pointer (just class name and id)

Later, when the decoder parses the parcel,

it holds a reference to unparceled objects
when it finds a pointer, and an object with that id was already unparceled, it uses the same instance

This is pretty much what KnownParseObjectDecoder is already doing for JSON decoding, so there is still consistency with JSON stuff.
The required encoder / decoder instance is passed to every class so it can safely keep trace of actions.

Ongoing tasks and LDS

By default this follows what was told in #122, that is, if there are ongoing save / save eventually tasks and we parcel, the tasks might succeed or fail but the unparceled object will act as if they failed.
The operation set queue is squashed to a single ParseOperationSet, that is parceled and restored, and contains the dirty changes from the last successful save at the moment of parceling.

If local datastore is enabled and we manage to get the object from LDS memory when unparceling, we can have better behavior because the same instance is restored. In this case the original operation set queue is kept safe, and the unparceled object will be internally updated when save / delete tasks end.

What do you think of this approach?
I think there’s not much missing anymore.

natario1 · 2017-04-04T23:09:09Z

An issue I can see (off the top of my head) is that in special occasions we might stack overflow, for example object.put(“itself”, object) when parceled will go on forever, right..?

However if this is an issue, it is already addressed by the SDK, because this will be true also with JSON encoding / decoding that the SDK already does for caching. So we should just see how it’s addressed in that case, and emulate the behavior. I think it’s about subclassing ParseParcelableEncoder to not encode ParseObject in some occasions... Or to encode as a pointer... Anyway, this can be addressed once all the TODOs are fixed.

rogerhu · 2017-04-04T23:21:42Z

Parse/src/main/java/com/parse/ParseObject.java

@@ -94,6 +98,14 @@ private static ParseObjectSubclassingController getSubclassingController() {
      return new Builder(className);
    }

+    /* package */ static State createFromParcel(Parcel source) {
+      String className = source.readString();
+      if ("_User".equals(className)) {


do we have this constant defined somewhere?

I think it’s left as _User everywhere, e.g. a few lines above.

rogerhu · 2017-04-04T23:21:58Z

Parse/src/main/java/com/parse/ParseUser.java

+  protected void onSaveInstanceState(Bundle outState) {
+    super.onSaveInstanceState(outState);
+    synchronized (mutex) {
+      outState.putBoolean("_isCurrentUser", isCurrentUser);


same here, should be a constant

Yeah, I’ll fix this

rogerhu · 2017-04-04T23:28:56Z

Are you talking about some type of recursive put? Yeah there should be a check to make sure you're not putting an object against yourself. I don't think Parcelable changes that fact (or might exacerbate it?)

In general I think the challenge with supporting this functionality is going to be all the inflight operations that occur. I think the relative mechanics of implementing a parcelable interface is a lot of plumbing work. We just have to make sure we have in the docs about best practices with this stuff.

natario1 · 2017-04-04T23:40:39Z

Well not a recursive put, but recursive parceling because we parcel all dirty data for all objects, even child objects. The recursion might also be more hidden, e.g.

user.setLastPost(post);
post.put(“author”, user);
// Then parcel post...

I wrote a similar test and it throws StackOverflowError when parceling. Any of you know how this is managed when JSON encoding ParseObjects?

facebook-github-bot · 2017-04-05T03:04:20Z

@natario1 updated the pull request - view changes

facebook-github-bot · 2017-04-05T14:19:48Z

@natario1 updated the pull request - view changes

facebook-github-bot · 2017-04-05T20:39:37Z

@natario1 updated the pull request - view changes

codecov · 2017-04-05T21:29:01Z

Codecov Report

Merging #625 into master will increase coverage by 1.35%.
The diff coverage is 72.96%.

@@             Coverage Diff             @@
##             master    #625      +/-   ##
===========================================
+ Coverage     51.05%   52.4%   +1.35%     
- Complexity     1524    1647     +123     
===========================================
  Files           127     131       +4     
  Lines          9699   10096     +397     
  Branches       1310    1404      +94     
===========================================
+ Hits           4952    5291     +339     
- Misses         4345    4364      +19     
- Partials        402     441      +39

Impacted Files	Coverage Δ	Complexity Δ
...rse/src/main/java/com/parse/ParseAddOperation.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../src/main/java/com/parse/ParseRemoveOperation.java	`11.53% <0%> (-1.51%)`	`3 <0> (ø)`
.../src/main/java/com/parse/ParseDeleteOperation.java	`54.54% <0%> (-12.13%)`	`5 <0> (ø)`
...c/main/java/com/parse/ParseAddUniqueOperation.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
Parse/src/main/java/com/parse/ParseUser.java	`77.09% <100%> (+0.88%)`	`107 <2> (+3)`	⬆️
.../main/java/com/parse/ParseObjectParcelEncoder.java	`100% <100%> (ø)`	`5 <5> (?)`
...rse/src/main/java/com/parse/ParseSetOperation.java	`100% <100%> (+14.28%)`	`6 <1> (+2)`	⬆️
...c/main/java/com/parse/ParseIncrementOperation.java	`60.71% <100%> (+24.71%)`	`7 <1> (+3)`	⬆️
...rc/main/java/com/parse/ParseRelationOperation.java	`46.26% <30%> (+0.11%)`	`22 <2> (+3)`	⬆️
...e/src/main/java/com/parse/ParseFieldOperation.java	`33.33% <41.3%> (-3.88%)`	`0 <0> (ø)`
... and 19 more

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 53b05e9...e06280a. Read the comment docs.

rogerhu · 2017-04-06T17:37:35Z

Parse/src/main/java/com/parse/ParseObject.java

+      boolean saving = hasOutstandingOperations();
+      boolean deleting = isDeleting || isDeletingEventually > 0;
+      if (saving) {
+        Log.w(TAG, "About to parcel a ParseObject while a save / saveEventually operation is " +


Will it be easy to trigger these issues if people are navigating between activities or you have a lot of rotation-type changes?

I guess depends on developer habits, but generally this should be rare (a configuration change while the object is in the middle of a save).

I agree the message is too long, but I could not make it shorter... Maybe if we merge this and update docs, we can replace with About to parcel a ParseObject with ongoing tasks. This might be dangerous. Read <link to docs>?

We can do that later. Would rather have a really big warning msg when this thing is first being used rather than a one-liner so we get some feedback about whether it happens a lot or not a ton.

natario1 · 2017-04-07T01:21:10Z

Now that this has been refined, it would be great to get the opinion of one of the original authors, beyond what was said in #122 .. Maybe we can summon @inlined ?

rogerhu · 2017-04-12T05:49:18Z

@wangmengyan95 @grantland any thoughts?

grantland · 2017-04-17T18:03:19Z

I haven't had time to go into the code in-depth, but the description seems like this has been well thought out. I haven't touched Parse code in about a year now, but the only thing that jumped out to me originally was the single instance use case for LDS which seems to have been covered.

natario1 · 2017-04-17T18:31:28Z

Glad to hear that @grantland , thank you for your time.

rogerhu · 2017-04-18T06:18:34Z

rebase =)

natario1 · 2017-04-18T10:08:04Z

@rogerhu what do you think? I think we can merge this first, then I will just add a couple lines to #619 and #624

rogerhu · 2017-04-19T20:48:45Z

rebase again

natario1 · 2017-04-19T20:52:25Z

@rogerhu done now

facebook-github-bot added GH Review: review-needed labels Apr 4, 2017

rogerhu reviewed Apr 4, 2017

View reviewed changes

rogerhu reviewed Apr 6, 2017

View reviewed changes

natario1 force-pushed the parcelable-object branch from 94945f0 to b0d07c2 Compare April 18, 2017 09:59

natario1 force-pushed the parcelable-object branch 2 times, most recently from 7ab3f11 to a16971a Compare April 19, 2017 19:49

natario1 added 8 commits April 19, 2017 22:50

Parcelable to ParseACL

b8e1714

Parcelable to ParseObject and subclasses

5912b05

Parceling ParseFieldOperations instead of estimatedData

df6fc0e

Name constants for operations

0b3a8c4

Tests for ParseObject, ParseUser, ParseACL

24f5cc3

Ability to pass encoders/decoders down the object tree

e1a5bab

Encode circular references as pointers. Support for ParseRelation

e68a2da

Test parcel while saving, tests for ParseRelation

bf55908

natario1 added 2 commits April 19, 2017 22:50

Warn with delete operations ongoing. Support for LDS

9e1240d

Fix docs and tests

e06280a

natario1 force-pushed the parcelable-object branch from a16971a to e06280a Compare April 19, 2017 20:51

rogerhu approved these changes Apr 19, 2017

View reviewed changes

rogerhu merged commit aaf740a into parse-community:master Apr 19, 2017

natario1 mentioned this pull request Apr 19, 2017

Parcelable documentation for Android parse-community/docs#428

Merged

natario1 deleted the parcelable-object branch May 1, 2017 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parcelable object #625

Parcelable object #625

natario1 commented Apr 4, 2017 •

edited

Loading

natario1 commented Apr 4, 2017

rogerhu Apr 4, 2017

natario1 Apr 4, 2017

rogerhu Apr 4, 2017

natario1 Apr 4, 2017

rogerhu commented Apr 4, 2017

natario1 commented Apr 4, 2017

facebook-github-bot commented Apr 5, 2017

facebook-github-bot commented Apr 5, 2017

facebook-github-bot commented Apr 5, 2017

codecov bot commented Apr 5, 2017 •

edited

Loading

rogerhu Apr 6, 2017

natario1 Apr 6, 2017

rogerhu Apr 6, 2017

natario1 commented Apr 7, 2017

rogerhu commented Apr 12, 2017

grantland commented Apr 17, 2017

natario1 commented Apr 17, 2017

rogerhu commented Apr 18, 2017

natario1 commented Apr 18, 2017

rogerhu commented Apr 19, 2017

natario1 commented Apr 19, 2017

Parcelable object #625

Parcelable object #625

Conversation

natario1 commented Apr 4, 2017 • edited Loading

Approach

Consistency

Recursiveness

Ongoing tasks and LDS

natario1 commented Apr 4, 2017

rogerhu Apr 4, 2017

Choose a reason for hiding this comment

natario1 Apr 4, 2017

Choose a reason for hiding this comment

rogerhu Apr 4, 2017

Choose a reason for hiding this comment

natario1 Apr 4, 2017

Choose a reason for hiding this comment

rogerhu commented Apr 4, 2017

natario1 commented Apr 4, 2017

facebook-github-bot commented Apr 5, 2017

facebook-github-bot commented Apr 5, 2017

facebook-github-bot commented Apr 5, 2017

codecov bot commented Apr 5, 2017 • edited Loading

Codecov Report

rogerhu Apr 6, 2017

Choose a reason for hiding this comment

natario1 Apr 6, 2017

Choose a reason for hiding this comment

rogerhu Apr 6, 2017

Choose a reason for hiding this comment

natario1 commented Apr 7, 2017

rogerhu commented Apr 12, 2017

grantland commented Apr 17, 2017

natario1 commented Apr 17, 2017

rogerhu commented Apr 18, 2017

natario1 commented Apr 18, 2017

rogerhu commented Apr 19, 2017

natario1 commented Apr 19, 2017

natario1 commented Apr 4, 2017 •

edited

Loading

codecov bot commented Apr 5, 2017 •

edited

Loading