Index: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java (revision 1886301) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java (working copy) @@ -47,11 +47,11 @@ @NotNull @Override public Resolution addExistingProperty(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs) { - if (isModifiedOrCreated(ours.getName())) { - merge(parent, ours, theirs); + if (isModifiedOrCreated(ours.getName()) && merge(parent, ours, theirs)) { return Resolution.MERGED; + } else { + return Resolution.IGNORED; } - return Resolution.IGNORED; } @NotNull @@ -58,24 +58,51 @@ @Override public Resolution changeChangedProperty(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, @NotNull PropertyState base) { - if (isModifiedOrCreated(ours.getName())) { - merge(parent, ours, theirs); + if (isModifiedOrCreated(ours.getName()) && merge(parent, ours, theirs)) { return Resolution.MERGED; + } else { + return Resolution.IGNORED; } - return Resolution.IGNORED; } - private static void merge(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs) { + /** + * Tries to merge two properties. The respective property of the parent is + * set if merging is successful. The the earlier value is used if + * jcr:created is true; the later is used if it is not jcr:created. + * + * @param parent the parent node + * @param ours our value + * @param theirs their value + * @return if merging is successful + */ + private static boolean merge(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs) { Calendar o = parse(ours.getValue(Type.DATE)); Calendar t = parse(theirs.getValue(Type.DATE)); - if (JCR_CREATED.equals(ours.getName())) { - parent.setProperty(ours.getName(), pick(o, t, true)); + Calendar value = pick(o, t, JCR_CREATED.equals(ours.getName())); + if (value != null) { + parent.setProperty(ours.getName(), value); + return true; } else { - parent.setProperty(ours.getName(), pick(o, t, false)); + return false; } } - private static Calendar pick(Calendar a, Calendar b, boolean jcrCreated) { + /** + * Pick "a" or "b", depending on which one is earlier (if jcrCreated = true) or later (if jcrCreated = false). + * + * @param a the first value + * @param b the second value + * @param jcrCreated if true, the earlier is returned, other wise the later one + * @return the calendar; either "a" or "b". It will return null if both are null + */ + @Nullable + private static Calendar pick(@Nullable Calendar a, @Nullable Calendar b, boolean jcrCreated) { + if (a == null) { + return b; + } + if (b == null) { + return a; + } if (a.before(b)) { return jcrCreated ? a : b; } else { Index: oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java (revision 1886301) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java (working copy) @@ -21,6 +21,13 @@ import static org.apache.jackrabbit.JcrConstants.JCR_CREATED; import static org.apache.jackrabbit.JcrConstants.JCR_LASTMODIFIED; import static org.apache.jackrabbit.oak.api.Type.DATE; +import static org.apache.jackrabbit.util.ISO8601.parse; +import static org.junit.Assert.assertSame; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; import java.util.Calendar; import java.util.concurrent.TimeUnit; @@ -30,20 +37,113 @@ import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.Root; import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.api.Type; import org.apache.jackrabbit.oak.plugins.memory.PropertyStates; import org.apache.jackrabbit.oak.spi.commit.ThreeWayConflictHandler; import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.util.ISO8601; import org.jetbrains.annotations.NotNull; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; public class JcrLastModifiedConflictHandlerTest { + private JcrLastModifiedConflictHandler handler; + private NodeBuilder nb; + + @Before + public void before() { + handler = new JcrLastModifiedConflictHandler(); + nb = mock(NodeBuilder.class); + } + @Test + public void testIgnoredDate() throws Exception { + String propertyName = "ignored"; + PropertyState ours = mockProperty(propertyName, "invalidDateValue"); + PropertyState theirs = createDateProperty(propertyName); + ThreeWayConflictHandler.Resolution resolution = handler.addExistingProperty(nb, ours, theirs); + assertSame(ThreeWayConflictHandler.Resolution.IGNORED, resolution); + resolution = handler.changeChangedProperty(nb, ours, theirs, theirs); + assertSame(ThreeWayConflictHandler.Resolution.IGNORED, resolution); + verifyNoInteractions(nb); + } + + @Test + public void testOursNotValidDate() throws Exception { + testOursNotValidDate(JCR_CREATED); + testOursNotValidDate(JCR_LASTMODIFIED); + } + + private void testOursNotValidDate(String propertyName) { + PropertyState ours = mockProperty(propertyName, "invalidDateValue"); + PropertyState theirs = createDateProperty(propertyName); + ThreeWayConflictHandler.Resolution resolution = handler.addExistingProperty(nb, ours, theirs); + assertSame(ThreeWayConflictHandler.Resolution.MERGED, resolution); + verify(nb).setProperty(propertyName, parse(theirs.getValue(Type.DATE))); + } + + @Test + public void testTheirsNotValidDate() throws Exception { + testTheirsNotValidDate(JCR_CREATED); + testTheirsNotValidDate(JCR_LASTMODIFIED); + } + + private void testTheirsNotValidDate(String propertyName) throws Exception { + PropertyState ours = createDateProperty(propertyName); + PropertyState theirs = mockProperty(propertyName, "invalidDateValue"); + ThreeWayConflictHandler.Resolution resolution = handler.addExistingProperty(nb, ours, theirs); + assertSame(ThreeWayConflictHandler.Resolution.MERGED, resolution); + verify(nb).setProperty(propertyName, parse(ours.getValue(Type.DATE))); + } + + @Test + public void testBothNotValidDate() throws Exception { + testBothNotValidDate(JCR_CREATED); + testBothNotValidDate(JCR_LASTMODIFIED); + } + + private void testBothNotValidDate(String propertyName) throws Exception { + PropertyState ours = mockProperty(propertyName, "invalidDateValue"); + PropertyState theirs = mockProperty(propertyName, "invalidDateValue"); + ThreeWayConflictHandler.Resolution resolution = handler.addExistingProperty(nb, ours, theirs); + assertSame(ThreeWayConflictHandler.Resolution.IGNORED, resolution); + verifyNoInteractions(nb); + resolution = handler.changeChangedProperty(nb, ours, theirs, createDateProperty(propertyName)); + assertSame(ThreeWayConflictHandler.Resolution.IGNORED, resolution); + verifyNoInteractions(nb); + } + + @Test + public void testOursBeforeTheirsLastModified() throws Exception { + PropertyState ours = createDateProperty(JCR_LASTMODIFIED); + // enforce that the second is a bit later + Thread.sleep(1); + PropertyState theirs = createDateProperty(JCR_LASTMODIFIED); + + ThreeWayConflictHandler.Resolution resolution = handler.changeChangedProperty(nb, ours, theirs, createDateProperty(JCR_LASTMODIFIED)); + assertSame(ThreeWayConflictHandler.Resolution.MERGED, resolution); + verify(nb).setProperty(JCR_LASTMODIFIED, parse(theirs.getValue(Type.DATE))); + } + + @Test + public void testOursBeforeTheirsCreated() throws Exception { + PropertyState ours = createDateProperty(JCR_CREATED); + // enforce that the second is a bit later + Thread.sleep(1); + PropertyState theirs = createDateProperty(JCR_CREATED); + + ThreeWayConflictHandler.Resolution resolution = handler.changeChangedProperty(nb, ours, theirs, createDateProperty(JCR_LASTMODIFIED)); + assertSame(ThreeWayConflictHandler.Resolution.MERGED, resolution); + verify(nb).setProperty(JCR_CREATED, parse(ours.getValue(Type.DATE))); + } + + @Test public void updates() throws Exception { - ContentRepository repo = newRepo(new JcrLastModifiedConflictHandler()); + ContentRepository repo = newRepo(handler); Root root = login(repo); setup(root); @@ -78,20 +178,31 @@ Assert.assertEquals(p1, root.getTree("/c").getProperty(JCR_LASTMODIFIED)); } + @NotNull public static PropertyState createDateProperty(@NotNull String name) { String now = ISO8601.format(Calendar.getInstance()); return PropertyStates.createProperty(name, now, DATE); } + @SuppressWarnings("unchecked") + @NotNull + private static PropertyState mockProperty(@NotNull String name, @NotNull String value) { + PropertyState ps = when(mock(PropertyState.class).getName()).thenReturn(name).getMock(); + when(ps.getValue(any(Type.class))).thenReturn(value); + return ps; + } + + @NotNull private static ContentRepository newRepo(ThreeWayConflictHandler handler) { return new Oak().with(new OpenSecurityProvider()).with(handler).createContentRepository(); } + @NotNull private static Root login(ContentRepository repo) throws Exception { return repo.login(null, null).getLatestRoot(); } - private static void setup(Root root) throws Exception { + private static void setup(@NotNull Root root) throws Exception { Tree tree = root.getTree("/"); tree.addChild("c"); root.commit();