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

Syncing Issue #202

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Syncing Issue #202

wants to merge 3 commits into from

Conversation

beaucollins
Copy link
Contributor

@beaucollins beaucollins commented Sep 26, 2018

Failing test to reproduce #201 / Automattic/simplenote-android#560

We can probably make a better test using JSONDiff.transform directly now that we're close to the source of the problem.

This is where the simperium client attempts to rebase a change on top of the server's state.

If you think about it in terms of git the client has a local branch that is one commit ahead of what it knows to be the remote branch.

Meanwhile someone else pushed a commit to the remote branch before the client could. When it reconnects to the server it does effectively a git pull --rebase so it can apply its local changes on top of the changes made by the other client.

This happens in JSONDiff.transform:

public static JSONObject transform(String o_diff, String diff, String source)
throws JSONException {
JSONObject transformed = new JSONObject();
LinkedList<Patch> o_patches = dmp.patch_make(source, dmp.diff_fromDelta(source, o_diff));
LinkedList<Patch> patches = dmp.patch_make(source, dmp.diff_fromDelta(source, diff));
String text = (String) dmp.patch_apply(patches, source)[0];
String combined = (String) dmp.patch_apply(o_patches, text)[0];
if (text.equals(combined)) {
// text is the same, return empty diff
return transformed;
}
LinkedList<Diff> diffs = dmp.diff_main(text, combined);
if (diffs.size() > 2) {
dmp.diff_cleanupEfficiency(diffs);
}
if (diffs.size() == 0) {
// no diffs, text is the same, return empty diff
return transformed;
}
transformed.put(DIFF_OPERATION_KEY, OPERATION_DIFF);
transformed.put(DIFF_VALUE_KEY, dmp.diff_toDelta(diffs));
return transformed;
}

@beaucollins
Copy link
Contributor Author

beaucollins commented Sep 27, 2018

I'm finding inconsistencies across how the clients handle this situation. simperium-android and node-simperium exhibit similar issues while iOS and the GAE clients each have their unique ways of handling this case.

In each scenario, both clients end up with the same note content and same history.

Now the three different results I have seen:

  1. Mangled: The note content is messed up and history is missing content (what Data Loss Scenario with DiffMatchPatch #201 describes)
  2. Replaced: The note content is replaced by the content of the
  3. Merged: The note content is the correct combination of the two changes

The steps in each scenario:

  1. Open Local client and second client
  2. Add a note with contents:
Line 1
Line 2
Line to be deleted
  1. Disconnect the local client
  2. On local client change the note to:
Line 1
Line 2
Before
Line to be deleted
After
  1. On the remote client change the note to:
Line 1
Line 2
Deleted
  1. Reconnect the local client and let it sync

Note content scenarios:

a. Mangled history is missing change from local client, not always the same result

Line 1
Line 2
X
Before
Line to be delet
Aftered

b. Replaced (history shows the change from remote client)

Line 1
Line 2
Before
Line to be deleted
After

c. Merged (history shows the change from remote client)

Line 1
Line 2
Before
Deleted
After
Scenario iOS Android Electron Web
iOS ? Merged Merged Merged
Android Merged Merged Merged Merged
Electron Content Loss Content Loss ? ?
Web Content Loss ? ? ?

@roundhill
Copy link
Contributor

Adding a new scenario, I've encountered content loss using the React app and Android:

Line 1
Line 2
Before
Deleted

^ Notice After is gone.

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.

2 participants