diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrConflictHandler.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrConflictHandler.java index 5282a88408..8179470cc8 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrConflictHandler.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/JcrConflictHandler.java @@ -18,9 +18,11 @@ */ package org.apache.jackrabbit.oak.plugins.commit; -import com.google.common.collect.ImmutableList; +import org.apache.jackrabbit.oak.security.user.RepMembersConflictHandler; import org.apache.jackrabbit.oak.spi.commit.CompositeConflictHandler; +import com.google.common.collect.ImmutableList; + /** * Utility class providing conflict handlers used for JCR. */ @@ -32,6 +34,7 @@ public final class JcrConflictHandler { */ public static CompositeConflictHandler createJcrConflictHandler() { return new CompositeConflictHandler(ImmutableList.of( + new RepMembersConflictHandler(), new JcrLastModifiedConflictHandler(), new ChildOrderConflictHandler(), new AnnotatingConflictHandler() diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java index 57cf46789c..45cf2d0121 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java @@ -37,7 +37,9 @@ import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; import org.apache.jackrabbit.oak.plugins.tree.impl.TreeConstants; import org.apache.jackrabbit.oak.spi.commit.ConflictHandler; import org.apache.jackrabbit.oak.spi.commit.PartialConflictHandler.Resolution; +import org.apache.jackrabbit.oak.spi.commit.ThreeWayConflictHandler; import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry; +import org.apache.jackrabbit.oak.spi.state.ConflictAnnotatingRebaseDiff; import org.apache.jackrabbit.oak.spi.state.ConflictType; import org.apache.jackrabbit.oak.spi.state.DefaultNodeStateDiff; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; @@ -99,12 +101,17 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { PropertyConflictHandler propertyConflictHandler = propertyConflictHandlers.get(conflictType); if (propertyConflictHandler != null) { for (PropertyState ours : conflictInfo.getProperties()) { + if (ours.getName().startsWith(ConflictAnnotatingRebaseDiff.BASE_PROPERTY)) { + continue; + } + + PropertyState base = conflictInfo + .getProperty(ConflictAnnotatingRebaseDiff.BASE_PROPERTY + ours.getName()); PropertyState theirs = parent.getProperty(ours.getName()); - Resolution resolution = propertyConflictHandler.resolve(ours, theirs); + Resolution resolution = propertyConflictHandler.resolve(ours, theirs, base); applyResolution(resolution, conflictType, ours); } - } - else { + } else { NodeConflictHandler nodeConflictHandler = nodeConflictHandlers.get(conflictType); if (nodeConflictHandler != null) { for (ChildNodeEntry oursCNE : conflictInfo.getChildNodeEntries()) { @@ -174,7 +181,7 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { } private interface PropertyConflictHandler { - Resolution resolve(PropertyState ours, PropertyState theirs); + Resolution resolve(PropertyState ours, PropertyState theirs, PropertyState base); } private interface NodeConflictHandler { @@ -184,7 +191,7 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { private final Map propertyConflictHandlers = ImmutableMap.of( ADD_EXISTING_PROPERTY, new PropertyConflictHandler() { @Override - public Resolution resolve(PropertyState ours, PropertyState theirs) { + public Resolution resolve(PropertyState ours, PropertyState theirs, PropertyState base) { return conflictHandler.addExistingProperty(target, ours, theirs); } @@ -195,7 +202,7 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { }, CHANGE_DELETED_PROPERTY, new PropertyConflictHandler() { @Override - public Resolution resolve(PropertyState ours, PropertyState theirs) { + public Resolution resolve(PropertyState ours, PropertyState theirs, PropertyState base) { return conflictHandler.changeDeletedProperty(target, ours); } @@ -206,8 +213,13 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { }, CHANGE_CHANGED_PROPERTY, new PropertyConflictHandler() { @Override - public Resolution resolve(PropertyState ours, PropertyState theirs) { - return conflictHandler.changeChangedProperty(target, ours, theirs); + public Resolution resolve(PropertyState ours, PropertyState theirs, PropertyState base) { + if (base != null && conflictHandler instanceof ThreeWayConflictHandler) { + return ((ThreeWayConflictHandler) conflictHandler).changeChangedProperty(target, ours, theirs, + base); + } else { + return conflictHandler.changeChangedProperty(target, ours, theirs); + } } @Override @@ -217,7 +229,7 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { }, DELETE_DELETED_PROPERTY, new PropertyConflictHandler() { @Override - public Resolution resolve(PropertyState ours, PropertyState theirs) { + public Resolution resolve(PropertyState ours, PropertyState theirs, PropertyState base) { return conflictHandler.deleteDeletedProperty(target, ours); } @@ -228,7 +240,7 @@ public final class MergingNodeStateDiff extends DefaultNodeStateDiff { }, DELETE_CHANGED_PROPERTY, new PropertyConflictHandler() { @Override - public Resolution resolve(PropertyState ours, PropertyState theirs) { + public Resolution resolve(PropertyState ours, PropertyState theirs, PropertyState base) { return conflictHandler.deleteChangedProperty(target, theirs); } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/RepMembersConflictHandler.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/RepMembersConflictHandler.java new file mode 100644 index 0000000000..7f3f3aeeee --- /dev/null +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/RepMembersConflictHandler.java @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.security.user; + +import com.google.common.collect.Sets; +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.commit.ThreeWayConflictHandler; +import org.apache.jackrabbit.oak.spi.security.user.UserConstants; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeState; + +import java.util.Set; + +/** + * The RepMembersConflictHandler takes care of merging the rep:members property + * during parallel updates. Change merges are done pessimistically, in that only members of both change sets + * are included in the result. + * + * The conflict handler deals with the following conflicts: + * + */ +public class RepMembersConflictHandler implements ThreeWayConflictHandler { + + @Override + public Resolution addExistingProperty(NodeBuilder parent, PropertyState ours, PropertyState theirs) { + if (isRepMembersProperty(theirs)) { + mergeAdd(parent, ours, theirs); + return Resolution.MERGED; + } else { + return null; + } + } + + @Override + public Resolution changeChangedProperty(NodeBuilder parent, PropertyState ours, PropertyState theirs) { + throw new IllegalStateException("this method should not be called"); + } + + @Override + public Resolution changeChangedProperty(NodeBuilder parent, PropertyState ours, PropertyState theirs, + PropertyState base) { + if (isRepMembersProperty(theirs)) { + mergeChange(parent, ours, theirs, base); + return Resolution.MERGED; + } else { + return null; + } + } + + @Override + public Resolution changeDeletedProperty(NodeBuilder parent, PropertyState ours) { + if (isRepMembersProperty(ours)) { + // removing the members property takes precedence + return Resolution.THEIRS; + } else { + return null; + } + } + + @Override + public Resolution deleteChangedProperty(NodeBuilder parent, PropertyState theirs) { + if (isRepMembersProperty(theirs)) { + // removing the members property takes precedence + return Resolution.OURS; + } else { + return null; + } + } + + @Override + public Resolution deleteDeletedProperty(NodeBuilder parent, PropertyState ours) { + if (isRepMembersProperty(ours)) { + // both are removing the members property, resolve + return Resolution.THEIRS; + } else { + return null; + } + } + + @Override + public Resolution addExistingNode(NodeBuilder parent, String name, NodeState ours, NodeState theirs) { + return null; + } + + @Override + public Resolution changeDeletedNode(NodeBuilder parent, String name, NodeState ours) { + return null; + } + + @Override + public Resolution deleteChangedNode(NodeBuilder parent, String name, NodeState theirs) { + return null; + } + + @Override + public Resolution deleteDeletedNode(NodeBuilder parent, String name) { + return null; + } + + //----------------------------< internal >---------------------------------- + + private static void mergeAdd(NodeBuilder parent, PropertyState ours, PropertyState theirs) { + PropertyBuilder merged = PropertyBuilder.array(Type.STRING); + merged.setName(UserConstants.REP_MEMBERS); + + Set theirMembers = Sets.newHashSet(theirs.getValue(Type.STRINGS)); + Set ourMembers = Sets.newHashSet(ours.getValue(Type.STRINGS)); + Set combined = Sets.union(theirMembers, ourMembers); + + merged.addValues(combined); + parent.setProperty(merged.getPropertyState()); + } + + private static void mergeChange(NodeBuilder parent, PropertyState ours, PropertyState theirs, PropertyState base) { + PropertyBuilder merged = PropertyBuilder.array(Type.STRING); + merged.setName(UserConstants.REP_MEMBERS); + + Set theirMembers = Sets.newHashSet(theirs.getValue(Type.STRINGS)); + Set ourMembers = Sets.newHashSet(ours.getValue(Type.STRINGS)); + Set baseMembers = Sets.newHashSet(base.getValue(Type.STRINGS)); + + // merge ours and theirs to a de-duplicated set + Set combined = Sets.newHashSet(Sets.intersection(ourMembers, theirMembers)); + for (String m : Sets.difference(ourMembers, theirMembers)) { + if (!baseMembers.contains(m)) { + combined.add(m); + } + } + for (String m : Sets.difference(theirMembers, ourMembers)) { + if (!baseMembers.contains(m)) { + combined.add(m); + } + } + merged.addValues(combined); + parent.setProperty(merged.getPropertyState()); + } + + private static boolean isRepMembersProperty(PropertyState p) { + return UserConstants.REP_MEMBERS.equals(p.getName()); + } + +} diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RepMembersConflictHandlerTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RepMembersConflictHandlerTest.java new file mode 100644 index 0000000000..720a40505d --- /dev/null +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/RepMembersConflictHandlerTest.java @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.user; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.apache.jackrabbit.api.security.user.Group; +import org.apache.jackrabbit.api.security.user.User; +import org.apache.jackrabbit.api.security.user.UserManager; +import org.apache.jackrabbit.oak.AbstractSecurityTest; +import org.apache.jackrabbit.oak.api.Root; +import org.junit.Test; + +public class RepMembersConflictHandlerTest extends AbstractSecurityTest { + + /** + * The id of the test group + */ + private static final String GROUP_ID = "test-groupId"; + + private Group group; + private User[] users; + + @Override + public void before() throws Exception { + super.before(); + UserManager um = getUserManager(root); + // create a group to receive users + group = um.createGroup(GROUP_ID); + + // create future members of the above group + User u1 = um.createUser("u1", "pass"); + User u2 = um.createUser("u2", "pass"); + User u3 = um.createUser("u3", "pass"); + User u4 = um.createUser("u4", "pass"); + User u5 = um.createUser("u5", "pass"); + root.commit(); + + users = new User[] { u1, u2, u3, u4, u5 }; + + } + + /** + * ADD-ADD test on an empty base + */ + @Test + public void testAddAddOnEmpty() throws Exception { + + Root r0 = login(getAdminCredentials()).getLatestRoot(); + Root r1 = login(getAdminCredentials()).getLatestRoot(); + + add(r0, users[1].getID()); + add(r1, users[2].getID()); + + // refresh to get the latest changes + root.refresh(); + + // verify users are now members (merged result) + assertTrue(group.isDeclaredMember(users[1])); + assertTrue(group.isDeclaredMember(users[2])); + } + + /** + * ADD-ADD test with a preexisting value + */ + @Test + public void testAddAdd() throws Exception { + + // pre-populate with an id + add(root, users[0].getID()); + + Root r0 = login(getAdminCredentials()).getLatestRoot(); + Root r1 = login(getAdminCredentials()).getLatestRoot(); + + add(r0, users[1].getID()); + add(r1, users[2].getID()); + + // refresh to get the latest changes + root.refresh(); + + // verify users are now members (merged result) + assertTrue(group.isDeclaredMember(users[0])); + assertTrue(group.isDeclaredMember(users[1])); + assertTrue(group.isDeclaredMember(users[2])); + } + + /** + * Remove-Remove test + */ + @Test + public void testRmRm() throws Exception { + + // pre-populate with values + add(root, users[0].getID(), users[1].getID(), users[2].getID()); + + Root r0 = login(getAdminCredentials()).getLatestRoot(); + Root r1 = login(getAdminCredentials()).getLatestRoot(); + + rm(r0, users[1].getID()); + rm(r1, users[2].getID()); + + // refresh to get the latest changes + root.refresh(); + + // verify users are now members (merged result) + assertTrue(group.isDeclaredMember(users[0])); + assertFalse(group.isDeclaredMember(users[1])); + assertFalse(group.isDeclaredMember(users[2])); + } + + /** + * Remove-Remove test + */ + @Test + public void testRmAdd() throws Exception { + + // pre-populate with values + add(root, users[0].getID(), users[1].getID()); + + Root r0 = login(getAdminCredentials()).getLatestRoot(); + Root r1 = login(getAdminCredentials()).getLatestRoot(); + + rm(r0, users[1].getID()); + add(r1, users[2].getID()); + + // refresh to get the latest changes + root.refresh(); + + // verify users are now members (merged result) + assertTrue(group.isDeclaredMember(users[0])); + assertFalse(group.isDeclaredMember(users[1])); + assertTrue(group.isDeclaredMember(users[2])); + } + + private void add(Root r, String... ids) throws Exception { + UserManager um = getUserManager(r); + Group g = (Group) um.getAuthorizable(GROUP_ID); + for (String id : ids) { + g.addMember(um.getAuthorizable(id)); + } + r.commit(); + } + + private void rm(Root r, String... ids) throws Exception { + UserManager um = getUserManager(r); + Group g = (Group) um.getAuthorizable(GROUP_ID); + for (String id : ids) { + g.removeMember(um.getAuthorizable(id)); + } + r.commit(); + } +} diff --git a/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/CompositeConflictHandler.java b/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/CompositeConflictHandler.java index cc76adfd9b..4c6af672a4 100644 --- a/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/CompositeConflictHandler.java +++ b/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/CompositeConflictHandler.java @@ -47,7 +47,7 @@ import org.apache.jackrabbit.oak.spi.state.NodeState; * conflict none of the backing handlers returns a valid resolution * this implementation throws an {@code IllegalStateException}. */ -public class CompositeConflictHandler implements ConflictHandler { +public class CompositeConflictHandler implements ThreeWayConflictHandler { private final LinkedList handlers; /** @@ -105,8 +105,20 @@ public class CompositeConflictHandler implements ConflictHandler { @Override public Resolution changeChangedProperty(NodeBuilder parent, PropertyState ours, PropertyState theirs) { + return changeChangedProperty(parent, ours, theirs, null); + } + + @Override + public Resolution changeChangedProperty(NodeBuilder parent, PropertyState ours, PropertyState theirs, + PropertyState base) { for (PartialConflictHandler handler : handlers) { - Resolution resolution = handler.changeChangedProperty(parent, ours, theirs); + Resolution resolution; + if (base != null && handler instanceof ThreeWayConflictHandler) { + ThreeWayConflictHandler twhandler = (ThreeWayConflictHandler) handler; + resolution = twhandler.changeChangedProperty(parent, ours, theirs, base); + } else { + resolution = handler.changeChangedProperty(parent, ours, theirs); + } if (resolution != null) { return resolution; } @@ -186,4 +198,5 @@ public class CompositeConflictHandler implements ConflictHandler { throw new IllegalStateException("No conflict handler for " + DELETE_DELETED_NODE + " conflict"); } + } diff --git a/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/ThreeWayConflictHandler.java b/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/ThreeWayConflictHandler.java new file mode 100644 index 0000000000..a344514820 --- /dev/null +++ b/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/commit/ThreeWayConflictHandler.java @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.spi.commit; + +import javax.annotation.Nonnull; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; + +/** + * A {@code ConflictHandler} with support for 3 way merges + * + * @see ConflictHandler + */ +public interface ThreeWayConflictHandler extends ConflictHandler { + + /** + * The property {@code ours} has been changed in {@code parent} while it was + * also changed to a different value ({@code theirs}) in the persistence store. + * + * @param parent root of the conflict + * @param ours our version of the property + * @param theirs their version of the property + * @param base the base version of the property + * @return {@link Resolution} of the conflict + */ + @Nonnull + Resolution changeChangedProperty(NodeBuilder parent, PropertyState ours, PropertyState theirs, PropertyState base); + +} diff --git a/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/state/ConflictAnnotatingRebaseDiff.java b/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/state/ConflictAnnotatingRebaseDiff.java index d846560d4a..e39f086a1d 100644 --- a/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/state/ConflictAnnotatingRebaseDiff.java +++ b/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/spi/state/ConflictAnnotatingRebaseDiff.java @@ -17,6 +17,7 @@ package org.apache.jackrabbit.oak.spi.state; import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; import static org.apache.jackrabbit.oak.spi.state.ConflictType.DELETE_CHANGED_PROPERTY; import static org.apache.jackrabbit.oak.spi.state.ConflictType.DELETE_CHANGED_NODE; @@ -37,6 +38,8 @@ import static org.apache.jackrabbit.oak.spi.state.ConflictType.DELETE_DELETED_NO public class ConflictAnnotatingRebaseDiff extends AbstractRebaseDiff { public static final String CONFLICT = ":conflict"; + public static final String BASE_PROPERTY = ":base:"; + public ConflictAnnotatingRebaseDiff(NodeBuilder builder) { super(builder); } @@ -58,7 +61,11 @@ public class ConflictAnnotatingRebaseDiff extends AbstractRebaseDiff { @Override protected void changeChangedProperty(NodeBuilder builder, PropertyState before, PropertyState after) { - conflictMarker(builder, CHANGE_CHANGED_PROPERTY).setProperty(after); + NodeBuilder cb = conflictMarker(builder, CHANGE_CHANGED_PROPERTY); + cb.setProperty(after); + PropertyBuilder base = PropertyBuilder.copy(before.getType().getBaseType(), before) + .setName(BASE_PROPERTY + before.getName()); + cb.setProperty(base.getPropertyState()); } @Override