Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Discarded Remote Changes (when defaults values are set in core data model) #546

Closed

Conversation

larryryu
Copy link
Contributor

@larryryu larryryu commented May 13, 2016

No longer overwriting diff generated from remote change with diff between a ManagedObject and it's ghostData

More details here: #547

…ppen when an entity has a default value on a field and the ghost diff is missing that field. In previous implementation this causes remote diff to be discarded and possibly lost if the client sends the incorrect value back to the server.
… proceed and acknowledge the currently value and assume previous value was nil, rather than return with no diff.
…t per key) before applying combined diff to the object. This also fixes failing test due to discarding local changes over remote changes.
@jleandroperez
Copy link
Contributor

Hello there @larryryu!

By design, local data is always preferred, on top of remote data. Reason for doing so is super simple: prevent data loss.

This sounds as if the real problem is the Core Data model's default value. How about if, instead:

  • The default value gets set by code instead (SPManagedObject.awakeFromLocalInsert)
  • The default value gets removed entirely?

@larryryu
Copy link
Contributor Author

Indeed default value is problematic and can be side stepped if it is removed from the core data model. However this is set automatically when creating a numeric attribute in core data model. One would have to know before hand to disable it because it could cause sync problems.

If v1 of app had default value and v2 removes it. It causes this issue to occur between app versions. This problem can be avoided if ever client assigns a default value, which is the work around we are currently using.

Just thought it would be better if the sdk handled the problem directly as it is a core data feature. I think it's fine to choose local data over remote data, but it is currently doing so on an object basis rather than per field. If there is a conflict in 1/3 fields everything from remote client is discarded (I would expect that the remaining 2 fields get the right changes applied from the remote client.). Moreover the local data would force its values next time sync occurs and overwrites values on server.

@gwwar gwwar requested review from gwwar, dmsnell and sirbrillig August 2, 2017 17:00
@gwwar
Copy link

gwwar commented Aug 2, 2017

@jleandroperez did you have recommendations for next steps here?

@jleandroperez
Copy link
Contributor

@gwwar nothing to add to what was mentioned here!

@gwwar
Copy link

gwwar commented Aug 2, 2017

thanks @jleandroperez !

@jleandroperez
Copy link
Contributor

Thank you @gwwar!

@gwwar
Copy link

gwwar commented Aug 2, 2017

So I'm pretty unfamiliar with this project, but at a high level, we can't land this as is due to other client needs like Simplenote that depend on the current behavior to prevent data loss.

I do think that we could perhaps make this a configurable option, where the current behavior is the default. I'm not sure how reasonable this is with my current understanding, but let me know what folks think.

@larryryu
Copy link
Contributor Author

larryryu commented Aug 2, 2017

Not sure i understand the current implementation really... I would think that a remote change should be applied at an attribute level as long as there are no conflicts with local change.

Currently, If there is a local change to field A (only), and a remote change that only contained an update to field B comes by... That change to field B is discarded (not applied to the local object). Eventually the client that discarded field B sends a change that updates field B to an older value (which can appear as data loss as well).

I think it would be great for me to understand the issue that bit of the code is trying to solve.
We can create a fork and apply this change, just wanted to brainstorm with the experts on potential issues we would run into.

The commit below may be a better alternative, since it only acts on a newly added object. So the only local data should be the default values which gets added to the ghost data as well.
larryryu@fc973b1

@dmsnell
Copy link

dmsnell commented Aug 3, 2017

To me it sounds like this isn't so much a problem of local data taking preference over remote data but as @larryryu suggested is that we have different granularities in the iOS library compared to what Simperium itself uses.

Because I'm not familiar with the iOS codebase or this issue, I'm going to try and rephrase with some examples. If they represent the problem then I believe we should be able to think about trying to reproduce with the other libraries and confirm first if the behavior is different and second if this fix would cause any trouble elsewhere.

// HTTP client A creates entity
{ text: 'fromHttpA', v: 1 }

// iOS client M gets entity, CoreData adds a new property
// but for some reason this default field doesn't sync with
// simperium yet so we have a ghost of v1 plus a local copy
// of the altered note
{ text: 'fromHttpA', v: 1 }
{ text: 'fromHttpA', counter: 0, v: 1* }

// HTTP client B adds field
{ text: 'fromHttpA', encoding: 'utf8', v: 2 }

// iOS client M gets update from server
// end up missing new field
{ text: 'fromHttpA', counter: 0, v: 2 }

If that sequence isn't too far off, and from reading the patch, it seems like what may be happening is that we update the ghost bucket in iOS with the update first, then perform a new diff from the local changes to the update in the ghost bucket which is like [ add counter, remove encoding ]

@larryryu can you help me understand the specific steps and mechanism here?

either way I think that if this is happening where unrelated fields are conflicting then that just shouldn't be that way. am I missing something big @jleandroperez?

@larryryu
Copy link
Contributor Author

larryryu commented Aug 3, 2017

@dmsnell the example you provide describes the problem.

Here is another example using the SimpleTodo sample code associated with this repo. Although this is a conflict on the same field "title". But can also easily reproduce by following your example.

  • run SimpletodoFinal project file (insert appropriate SIMPERIUM_APP_ID and SIMPERIUM_API_KEY in Appdelegate). Sign in and keep app running.

  • run curl command in terminal
    curl -H 'X-Simperium-Token: ’
    https://api.simperium.com/1/<app_id>/todo/i/ -d '{"title" : “test 1”}’

  • NOTE that the app inserts new item with "test 1" title

  • update title
    curl -H 'X-Simperium-Token: ’
    https://api.simperium.com/1/<app_id>/todo/i/ -d '{"title" : “test 2”}’

  • NOTE that the app is not updated

  • update title again
    curl -H 'X-Simperium-Token: ’
    https://api.simperium.com/1/<app_id>/todo/i/ -d '{"title" : “test 3”}’

  • NOTE that the app is not updated

  • fetch todo
    curl -H 'X-Simperium-Token: '
    https://api.simperium.com/1/<app_id>/todo/i/

  • Note that response is {"num": 0, "order": 0, "title": "test 1"}

  • Subsequent updates will now work as expected.

@jleandroperez
Copy link
Contributor

Closing this one since it's been a few years without further development.

Thanks everyone!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants