Re-save installation is allowed if LDS is enabled

[hermanliang]

Following PR #579 and resolving Issue #605

[rogerhu]

Separate question. have you considered just setting applicationId to be different for dev/prod? Mixing cached data in general across prod/dev is going to cause you lots of problems in general.

[rogerhu]

So is it fairly easy to null out your objectId() on a failure and then cause multiple install id's per app?

[hermanliang]

Only the ParseException.OBJECT_NOT_FOUND (line 148) can trigger the re-creating process.
If just null out the objectId and save installation will not create another installation data. (It's weird, I'm still figuring out why)
BTW, I think setObjectId() should not be a public method, package private may be more appropriate.

[rogerhu]

Gotcha line 148 is indeed key.

[natario1]

btw this line can be removed, there is instanceof below

[hermanliang]

@rogerhu Thanks for your suggestion.
Yes, you're right, different appId can prevent problems from mixing cached data. But in my case, I need to retrieve the local stored data (such as SharedPreference, sqlite, ...) when dev/prod switching. If using different appId, it would be hard to get these data from another app.
Another reason is that in this case, Parse only serves as a push server. Just use the Installation to save the push related information. It's easier to handle the problem, just delete the installation on the server and let app to re-create it and update the cached data as well. I believe if including other Classes (table) in this case will be very painful to handle the mixing cached data problem, and using different appId will be considered.

[rogerhu]

I think you really shouldn't be mixing dev/prod. :) Even your push infra should silo the two environments as well (separate GCM keys).

Your PR seems fine for the problem it tries to rectify...given you pointed out the iOS SDK tries to do something similar does it do the same thing?

[hermanliang]

@rogerhu The iOS SDK is NOT allowed this operation if LDS is enabled.
I also figure out why setObjectId() is not permitted when using LDS.
Avoid inappropriate pin() into datastore would be the major concern.
For example, if setObject() is allowed when LDS is enabled

ParseObject object = new ParseObject("Test");
object.put("key", "value");
object.save(); // add to server
object.pin(); // add to LDS
object.setObjectId(null); // for replication purpose
object.save(); // add to server
object.pin(); // will REPLACE previous object to LDS
ParseQuery.getQuery("Test").count(); // count: 2 (server)
ParseQuery.getQuery("Test").fromPin().count(); // count: 1 (datastore)

In ParseInstallation, following operation will clear/replace the cached installation's objectId (shouldn't be allowed, fixed in PR #611)

// if LDS disabled
ParseInstallation installation = ParseInstallation.getCurrentInstallation();
installation.setObjectId(null);
installation.save(); // save task will succeed, but cached objectId will be cleared

// If LDS enabled
ParseInstallation installation = ParseInstallation.getCurrentInstallation();
installation.setObjectId(null);
installation.pin(); // objectId in datastore will be cleared

If setObjectId() is not allowed in ParseInstallation (PR #611), I think the above case will not happen.
And I think there is no other safety issue in this PR.

[rogerhu]

Does this change then how the Android version does it to ios then?

[rogerhu]

Thanks @hermanliang - this is on a backlog, and appreciate you doing the work on both platforms.

Can anyone take a look and provide any feedback? I think it addresses a weird corner case of mixing dev/prod instances data. But @hermanliang has done a ton of work and would love help taking a look.

[hermanliang]

Thanks @rogerhu
Yeah, we always want to save the installation successfully.
Here's what I'm going to update in this PR.

1. I think Not allowing setObjectId() in ParseInstallation #611 can be integrated into this PR because they are relevant.
2. There is another edge case stated in error:135 deviceType must be specified in this operation Parse v 1.14.1 #627 (comment). I think we may add this logic in this PR as well.

[codecov]

Codecov Report

Merging #607 into master will increase coverage by 0.28%.
The diff coverage is 40%.

@@             Coverage Diff              @@
##             master     #607      +/-   ##
============================================
+ Coverage     51.27%   51.56%   +0.28%     
- Complexity     1536     1546      +10     
============================================
  Files           129      129              
  Lines          9835     9837       +2     
  Branches       1327     1327              
============================================
+ Hits           5043     5072      +29     
+ Misses         4384     4354      -30     
- Partials        408      411       +3

Impacted Files	Coverage Δ	Complexity Δ
...rse/src/main/java/com/parse/ParseInstallation.java	76.47% <0%> (+0.74%)	39 <0> (ø)	⬇️
Parse/src/main/java/com/parse/OfflineStore.java	4.27% <50%> (+2.84%)	5 <0> (+4)	⬆️
Parse/src/main/java/com/parse/ParseObject.java	44.79% <0%> (+0.14%)	168% <0%> (+1%)	⬆️
...c/main/java/com/parse/OfflineSQLiteOpenHelper.java	16.66% <0%> (+16.66%)	1% <0%> (+1%)	⬆️
...src/main/java/com/parse/ParseSQLiteOpenHelper.java	21.42% <0%> (+21.42%)	1% <0%> (+1%)	⬆️
...arse/src/main/java/com/parse/WeakValueHashMap.java	56.25% <0%> (+37.5%)	4% <0%> (+3%)	⬆️

---

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 0579589...6748ecb. Read the comment docs.

[codecov]

Codecov Report

Merging #607 into master will increase coverage by 1.06%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master     #607      +/-   ##
============================================
+ Coverage     52.56%   53.62%   +1.06%     
- Complexity     1664     1688      +24     
============================================
  Files           131      131              
  Lines         10138    10142       +4     
  Branches       1407     1408       +1     
============================================
+ Hits           5329     5439     +110     
+ Misses         4369     4245     -124     
- Partials        440      458      +18

Impacted Files	Coverage Δ	Complexity Δ
Parse/src/main/java/com/parse/ParseException.java	100% <ø> (ø)	4 <0> (ø)	⬇️
Parse/src/main/java/com/parse/OfflineStore.java	8.91% <50%> (+7.47%)	9 <0> (+8)	⬆️
...rse/src/main/java/com/parse/ParseInstallation.java	76.19% <50%> (+0.22%)	40 <0> (ø)	⬇️
Parse/src/main/java/com/parse/ParseObject.java	51.12% <0%> (+0.82%)	202% <0%> (+2%)	⬆️
...e/src/main/java/com/parse/ParseSQLiteDatabase.java	26.22% <0%> (+24.59%)	5% <0%> (+4%)	⬆️
...arse/src/main/java/com/parse/WeakValueHashMap.java	56.25% <0%> (+37.5%)	4% <0%> (+3%)	⬆️
...src/main/java/com/parse/ParseSQLiteOpenHelper.java	71.42% <0%> (+71.42%)	4% <0%> (+4%)	⬆️
...c/main/java/com/parse/OfflineSQLiteOpenHelper.java	75% <0%> (+75%)	3% <0%> (+3%)	⬆️

---

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 2659e7d...6a2abc2. Read the comment docs.

[rogerhu]

rebase

[rogerhu]

rebase

[hermanliang]

rebased

[rogerhu]

This build failed actually. Can you fix?

https://travis-ci.org/parse-community/Parse-SDK-Android

[hermanliang]

@rogerhu fixed by #642
plz check
Thanks