From 1b83001b8b60b958364cf744d1b30fade8fda24d Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Mon, 5 Nov 2018 10:03:30 -0800 Subject: [PATCH 1/5] When a merge fails, queue the object to be synced again --- Simperium/src/main/java/com/simperium/client/Bucket.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Simperium/src/main/java/com/simperium/client/Bucket.java b/Simperium/src/main/java/com/simperium/client/Bucket.java index 3c600ba1..0dfa5247 100644 --- a/Simperium/src/main/java/com/simperium/client/Bucket.java +++ b/Simperium/src/main/java/com/simperium/client/Bucket.java @@ -648,8 +648,10 @@ public void run() { object.setGhost(ghost); updateObject(object); } catch (JSONException | IllegalArgumentException e) { - // Could not apply patch, update from the ghost - updateObjectWithGhost(ghost); + // Could not apply patch, queue the local client to sync the object + // with the local client's modifications + // https://github.com/Simperium/simperium-android/pull/202 + mChannel.queueLocalChange(ghost.getSimperiumKey()); } } else { // Apply the new ghost to the unmodified local object From dabe159f3c221015cd94096c2afa654b43b0a8c5 Mon Sep 17 00:00:00 2001 From: Dan Roundhill Date: Tue, 6 Nov 2018 12:24:43 -0800 Subject: [PATCH 2/5] Throw a JSONException if diff match patch didn't apply the patches correctly. --- .../src/main/java/com/simperium/client/Bucket.java | 4 ++-- .../src/main/java/com/simperium/util/JSONDiff.java | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Simperium/src/main/java/com/simperium/client/Bucket.java b/Simperium/src/main/java/com/simperium/client/Bucket.java index 0dfa5247..e439378a 100644 --- a/Simperium/src/main/java/com/simperium/client/Bucket.java +++ b/Simperium/src/main/java/com/simperium/client/Bucket.java @@ -975,8 +975,8 @@ public Ghost applyRemoteChange(RemoteChange change) updatedProperties = JSONDiff.apply(updatedProperties, transformedDiff); } catch (JSONException | IllegalArgumentException e) { - // could not transform properties - // continue with updated properties + // TODO: Handle failed merge by sending local changes to Simperium + // TODO: Why does sync completely die if this is reached? } } diff --git a/Simperium/src/main/java/com/simperium/util/JSONDiff.java b/Simperium/src/main/java/com/simperium/util/JSONDiff.java index da0d8783..68f66910 100644 --- a/Simperium/src/main/java/com/simperium/util/JSONDiff.java +++ b/Simperium/src/main/java/com/simperium/util/JSONDiff.java @@ -43,8 +43,16 @@ public static JSONObject transform(String o_diff, String diff, String source) LinkedList 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]; + Object[] appliedPatch = dmp.patch_apply(o_patches, text); + boolean[] results = (boolean[]) appliedPatch[1]; + for (boolean result : results) { + if (!result) { + throw new JSONException("Could not cleanly transform patch."); + } + } + + String combined = (String)appliedPatch[0]; if (text.equals(combined)) { // text is the same, return empty diff return transformed; From 1cc28b8950df7e89dfc0e56e33fdde31997ae976 Mon Sep 17 00:00:00 2001 From: Dan Roundhill Date: Wed, 7 Nov 2018 10:33:57 -0800 Subject: [PATCH 3/5] Don't update the object when we encounter an error merging local modifications. --- .../src/main/java/com/simperium/client/Bucket.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Simperium/src/main/java/com/simperium/client/Bucket.java b/Simperium/src/main/java/com/simperium/client/Bucket.java index e439378a..fb096e2a 100644 --- a/Simperium/src/main/java/com/simperium/client/Bucket.java +++ b/Simperium/src/main/java/com/simperium/client/Bucket.java @@ -929,6 +929,7 @@ public Ghost applyRemoteChange(RemoteChange change) try { T object; Boolean isNew; + Boolean shouldUpdateObject = true; if (change.isAddOperation()) { object = newObject(change.getKey()); @@ -964,7 +965,6 @@ public Ghost applyRemoteChange(RemoteChange change) mSchema.updateWithDefaults(object, updatedProperties); addObject(object); } else { - if (localModifications != null && localModifications.length() > 0) { try { JSONObject incomingDiff = change.getPatch(); @@ -973,15 +973,17 @@ public Ghost applyRemoteChange(RemoteChange change) JSONObject transformedDiff = JSONDiff.transform(localDiff, incomingDiff, currentProperties); updatedProperties = JSONDiff.apply(updatedProperties, transformedDiff); - } catch (JSONException | IllegalArgumentException e) { - // TODO: Handle failed merge by sending local changes to Simperium - // TODO: Why does sync completely die if this is reached? + // We couldn't merge the local and remote changes. + // Hold off on updating the object so that the local change can sync + shouldUpdateObject = false; } } - mSchema.update(object, updatedProperties); - updateObject(object); + if (shouldUpdateObject) { + mSchema.update(object, updatedProperties); + updateObject(object); + } } } catch(SimperiumException e) { From 95d5e4e2bc57a1119486dfdeb38acd695023a9b4 Mon Sep 17 00:00:00 2001 From: Dan Roundhill Date: Wed, 7 Nov 2018 10:34:12 -0800 Subject: [PATCH 4/5] Revert "When a merge fails, queue the object to be synced again" This reverts commit 1b83001b8b60b958364cf744d1b30fade8fda24d. --- Simperium/src/main/java/com/simperium/client/Bucket.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Simperium/src/main/java/com/simperium/client/Bucket.java b/Simperium/src/main/java/com/simperium/client/Bucket.java index fb096e2a..1058071e 100644 --- a/Simperium/src/main/java/com/simperium/client/Bucket.java +++ b/Simperium/src/main/java/com/simperium/client/Bucket.java @@ -648,10 +648,8 @@ public void run() { object.setGhost(ghost); updateObject(object); } catch (JSONException | IllegalArgumentException e) { - // Could not apply patch, queue the local client to sync the object - // with the local client's modifications - // https://github.com/Simperium/simperium-android/pull/202 - mChannel.queueLocalChange(ghost.getSimperiumKey()); + // Could not apply patch, update from the ghost + updateObjectWithGhost(ghost); } } else { // Apply the new ghost to the unmodified local object From 08c303450359602823558ad77e9c8c7e8586fcd6 Mon Sep 17 00:00:00 2001 From: Dan Roundhill Date: Wed, 7 Nov 2018 11:24:45 -0800 Subject: [PATCH 5/5] Adding test for invalid transform exception. --- .../java/com/simperium/JSONDiffTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Simperium/src/androidTestSupport/java/com/simperium/JSONDiffTest.java b/Simperium/src/androidTestSupport/java/com/simperium/JSONDiffTest.java index 92d91331..d5b5477a 100644 --- a/Simperium/src/androidTestSupport/java/com/simperium/JSONDiffTest.java +++ b/Simperium/src/androidTestSupport/java/com/simperium/JSONDiffTest.java @@ -338,6 +338,24 @@ public void testTransformStringDiff() } + public void testInvalidStringTransformThrowsException() + throws Exception { + String origin = "Line 1\nLine 2\nReplace me"; + try + { + JSONObject transformed = JSONDiff.transform( + "=14\t+Before%0A\t=10\t+%0AAfter", + "=14\t-10\t+BYE", + origin + ); + fail("Patch transform should have failed."); + } + catch(Exception e) + { + // Test passed + } + } + public void testTransformObjectDiffChangeRemovedKey() throws Exception {