You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2021/02/09 08:59:16 UTC
svn commit: r1886355 - in /jackrabbit/oak/trunk/oak-core/src:
main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java
test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java
Author: angela
Date: Tue Feb 9 08:59:16 2021
New Revision: 1886355
URL: http://svn.apache.org/viewvc?rev=1886355&view=rev
Log:
OAK-9344 : JcrLastModifiedConflictHandler prone to NPE
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java?rev=1886355&r1=1886354&r2=1886355&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandler.java Tue Feb 9 08:59:16 2021
@@ -47,39 +47,66 @@ public class JcrLastModifiedConflictHand
@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
@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) {
- if (a.before(b)) {
- return jcrCreated ? a : b;
+ /**
+ * Pick "a" or "b", depending on which one is earlier (if jcrCreated = true) or later (if jcrCreated = false).
+ *
+ * @param ours the property state from our change set to have a conflict resolved.
+ * @param theirs the property state from their change set to have a conflict resolved.
+ * @param jcrCreated if true and 'ours' is before 'theirs', 'ours' is returned, otherwise 'theirs'
+ * @return the calendar; either "ours" or "theirs". It will return {@code null} if both are {@code null}
+ */
+ @Nullable
+ private static Calendar pick(@Nullable Calendar ours, @Nullable Calendar theirs, boolean jcrCreated) {
+ if (ours == null) {
+ return theirs;
+ }
+ if (theirs == null) {
+ return ours;
+ }
+ if (ours.before(theirs)) {
+ return jcrCreated ? ours : theirs;
} else {
- return jcrCreated ? b : a;
+ return jcrCreated ? theirs : ours;
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java?rev=1886355&r1=1886354&r2=1886355&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/commit/JcrLastModifiedConflictHandlerTest.java Tue Feb 9 08:59:16 2021
@@ -21,6 +21,13 @@ package org.apache.jackrabbit.oak.plugin
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.Con
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(@NotNull 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(@NotNull 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(@NotNull 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 @@ public class JcrLastModifiedConflictHand
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);
}
- private static ContentRepository newRepo(ThreeWayConflictHandler handler) {
+ @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(@NotNull 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();