Attempt to re-save installation if Installation was deleted on the server

[hermanliang]

Reference: Parse-SDK-iOS-OSX/PFInstallation.m#saveAsync

[rogerhu]

Besides this PR matching iOS, do you see this issue for yourself?

[hermanliang]

@rogerhu
The issue would be happended in the following steps (but doesn't happend on iOS device):

1. App with the following code

ParseInstallation installation = ParseInstallation.getCurrentInstallation();
// put some parameters here
...
installation.saveInBackground();

2. Install & launch the app
3. installation added to server successfully
4. Delete the added installation on the server
5. Terminate the app and re-launch again
6. The installation not re-created to the server

The following is my scenario:
I have two environments for the app. One is Sandbox using Database-A (db-A), the other is Production using Database-B (db-B).
The released app might be with Sandbox (for testing/debugging purpose) or Production settings. Both settings are with the same applicationId, so I can easily replace the settings and keep the app data intact.

1. The device install the app with Sandbox settings first
2. Launch the app, the installation data would be added to the db-A
3. Replace the app with Production settings on the same device
4. Launch the app, the installation data would be not added to the db-B unless I clear app data from Settings App.

If the device with the above installation scenario, the push notification won't be received in production environment due to no installation data in db-B.
But the above scenario doesn't happened on iOS devices.

[rogerhu]

Great thanks for the explaination. Is there a way to add a test for this?

[facebook-github-bot]

@hermanliang updated the pull request - view changes

[hermanliang]

@rogerhu
I added a test. Thanks!

[rogerhu]

minor optimization -- this can be verify(controller).getAsync();

http://static.javadoc.io/org.mockito/mockito-core/2.7.16/org/mockito/Mockito.html#verify(T)

verify(mock).someMethod("some arg");
 
Above is equivalent to:
verify(mock, times(1)).someMethod("some arg");
 

[hermanliang]

update it by 94da911
Thanks

[rogerhu]

@natario1 will any changes be necessary for safeKeys() if we merge this in?

[natario1]

@rogerhu no, it’s OK as it is now.

[rogerhu]

I think code coverage will be higher after you rebase. :)

[rogerhu]

rebased / force pushed to your branch

[facebook-github-bot]

@hermanliang updated the pull request - view changes

[facebook-github-bot]

@hermanliang updated the pull request - view changes

[rogerhu]

Lgtm thanks

[rogerhu]

lgtm

[natario1]

@hermanliang @rogerhu isn’t your method ParseObject.markAllFieldsDirty doing the same of ParseObject.rebuildEstimatedData (ref)?

I think you could have used that instead.

[hermanliang]

@natario1 I think the two methods call is not the same and also for different purposes. ParseObject.rebuildEstimatedData does not force all fields to get dirty because it uses estimatedData.put(key, value) to rebuild estimatedData and it does not change the operations, but performPut(key, value) will update the operations.

[rogerhu]

Can we use markallDirty inside the rebuildEstimatedData function? Seems like it's repeated?

Also it should be plural markAllFields

[hermanliang]

@rogerhu They're different.
rebuildEstimatedData vs. markAllFieldDirty

1. performPut(key, value) also updates the operations.
2. rebuildEstimatedData will be called before/after update to the server (ex. handleSaveResultAsync -> setState)

So if use markAllDirty inside the rebuildEstimatedData, the transmission size might be increased.

1. All fields will be transferred to the server instead of modified (dirty) fields only.
2. After updating to the server, the data is dirty again.

[rogerhu]

Gotcha. They are different.

Looking at https://github.com/ParsePlatform/Parse-SDK-iOS-OSX/blob/2504c19f03698b4f7d89a22708133a410b17fba9/Parse/PFInstallation.m#L73-L80 though markAllFields (should be plural) puts the estimatedData. Yours seems to clear and then write the current state instead of the cached data?

[hermanliang]

Yes, the cached installation needs to be rebuilt by new installation after being saved to the server.
To do this, the existing installation needs to be saved as a new installation.

1. Clear objectID (assign null).
2. Mark all fields as needed for updates.

After the re-saving process, cached installation file will be overwritten by new one.

The above operation seems to not allow when the local datastore is enabled due to objectID is unchangeable after it's created. (But I think it should be allowed in this case.)

BTW, thanks for your correction. I created a PR (#602) for renaming the function (markAllFieldDirty -> markAllFieldsDirty).

[rogerhu]

great thanks for the explanation.

I guess my question is that markAllFieldsDirty() in the iOS SDK (https://github.com/ParsePlatform/Parse-SDK-iOS-OSX/blob/2504c19f03698b4f7d89a22708133a410b17fba9/Parse/PFInstallation.m#L73-L80) seems to mark all the cached data entries as dirty.

it seems like you want to clear the cache, reset it to the original state, and clear object ID.

I guess my question is why there's a difference?

[hermanliang]

I know your question now. 😄
Yes, they're different and iOS's markAllFieldsDirty is right.
This line should be removed. No needs to clear the estimatedData before reset it to the original state.
I'll submit another PR to fix it.
Thanks!