Index: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/ConflictValidator.java =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/ConflictValidator.java (revision 1689931) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/ConflictValidator.java (working copy) @@ -16,23 +16,24 @@ */ package org.apache.jackrabbit.oak.plugins.commit; -import com.google.common.base.Joiner; +import static org.apache.jackrabbit.oak.api.Type.STRINGS; +import static org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants.MIX_REP_MERGE_CONFLICT; + import org.apache.jackrabbit.JcrConstants; import org.apache.jackrabbit.oak.api.CommitFailedException; import org.apache.jackrabbit.oak.api.PropertyState; -import org.apache.jackrabbit.oak.api.Tree; import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants; import org.apache.jackrabbit.oak.spi.commit.DefaultValidator; import org.apache.jackrabbit.oak.spi.commit.Validator; +import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry; import org.apache.jackrabbit.oak.spi.state.ConflictType; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static com.google.common.collect.Iterables.transform; -import static org.apache.jackrabbit.oak.api.Type.STRINGS; -import static org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants.MIX_REP_MERGE_CONFLICT; +import com.google.common.base.Joiner; /** * {@link Validator} which checks the presence of conflict markers @@ -43,10 +44,47 @@ public class ConflictValidator extends DefaultValidator { private static Logger log = LoggerFactory.getLogger(ConflictValidator.class); - private final Tree parentAfter; + /** + * Current processed path, or null if the debug log is not enabled at the + * beginning of the call. The null check will also be used to verify if a + * debug log will be needed or not + */ + private final String path; - public ConflictValidator(Tree parentAfter) { - this.parentAfter = parentAfter; + private NodeState after; + + private ConflictValidator() { + if (log.isDebugEnabled()) { + this.path = "/"; + } else { + this.path = null; + } + } + + private ConflictValidator(String path, String name) { + if (path != null) { + this.path = PathUtils.concat(path, name); + } else { + this.path = null; + } + } + + private boolean isLogEnabled() { + return path != null; + } + + @Override + public void enter(NodeState before, NodeState after) + throws CommitFailedException { + if (isLogEnabled()) { + this.after = after; + } + } + + @Override + public void leave(NodeState before, NodeState after) + throws CommitFailedException { + this.after = null; } @Override @@ -62,12 +100,13 @@ @Override public Validator childNodeAdded(String name, NodeState after) { - return new ConflictValidator(parentAfter.getChild(name)); + return new ConflictValidator(path, name); } @Override - public Validator childNodeChanged(String name, NodeState before, NodeState after) { - return new ConflictValidator(parentAfter.getChild(name)); + public Validator childNodeChanged(String name, NodeState before, + NodeState after) { + return new ConflictValidator(path, name); } @Override @@ -82,12 +121,12 @@ if (MIX_REP_MERGE_CONFLICT.equals(v)) { CommitFailedException ex = new CommitFailedException( - CommitFailedException.STATE, 1, "Unresolved conflicts in " + parentAfter.getPath()); + CommitFailedException.STATE, 1, "Unresolved conflicts in " + path); //Conflict details are not made part of ExceptionMessage instead they are //logged. This to avoid exposing property details to the caller as it might not have //permission to access it - if (log.isDebugEnabled()) { + if (isLogEnabled()) { log.debug(getConflictMessage(), ex); } throw ex; @@ -98,16 +137,17 @@ private String getConflictMessage() { StringBuilder sb = new StringBuilder("Commit failed due to unresolved conflicts in "); - sb.append(parentAfter.getPath()); + sb.append(path); sb.append(" = {"); - for (Tree conflict : parentAfter.getChild(NodeTypeConstants.REP_OURS).getChildren()) { - ConflictType ct = ConflictType.fromName(conflict.getName()); + for (ChildNodeEntry conflict : after.getChildNode(NodeTypeConstants.REP_OURS).getChildNodeEntries()) { + ConflictType ct = ConflictType.fromName(conflict.getName()); + NodeState node = conflict.getNodeState(); sb.append(ct.getName()).append(" = {"); if (ct.effectsNode()) { - sb.append(getChildNodeNamesAsString(conflict)); + sb.append(getChildNodeNamesAsString(node)); } else { - for (PropertyState ps : conflict.getProperties()) { + for (PropertyState ps : node.getProperties()) { PropertyState ours = null, theirs = null; switch (ct) { case DELETE_CHANGED_PROPERTY: @@ -117,7 +157,7 @@ case ADD_EXISTING_PROPERTY: case CHANGE_CHANGED_PROPERTY: ours = ps; - theirs = parentAfter.getProperty(ps.getName()); + theirs = after.getProperty(ps.getName()); break; case CHANGE_DELETED_PROPERTY: ours = ps; @@ -145,8 +185,8 @@ return sb.toString(); } - private static String getChildNodeNamesAsString(Tree t) { - return Joiner.on(',').join(transform(t.getChildren(), Tree.GET_NAME)); + private static String getChildNodeNamesAsString(NodeState ns) { + return Joiner.on(',').join(ns.getChildNodeNames()); } private static String toString(PropertyState ps) { @@ -154,7 +194,7 @@ return ""; } - final Type type = ps.getType(); + final Type type = ps.getType(); if (type.isArray()) { return ""; } Index: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/ConflictValidatorProvider.java =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/ConflictValidatorProvider.java (revision 1689931) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/ConflictValidatorProvider.java (working copy) @@ -18,7 +18,6 @@ import org.apache.felix.scr.annotations.Component; import org.apache.felix.scr.annotations.Service; -import org.apache.jackrabbit.oak.plugins.tree.TreeFactory; import org.apache.jackrabbit.oak.spi.commit.CommitInfo; import org.apache.jackrabbit.oak.spi.commit.EditorProvider; import org.apache.jackrabbit.oak.spi.commit.Validator; @@ -35,7 +34,7 @@ @Override public Validator getRootValidator( NodeState before, NodeState after, CommitInfo info) { - return new ConflictValidator(TreeFactory.createReadOnlyTree(after)); + return new ConflictValidator(); } } Index: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java (revision 1689931) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/commit/MergingNodeStateDiff.java (working copy) @@ -32,6 +32,7 @@ import com.google.common.collect.ImmutableMap; import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.json.JsopDiff; 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; @@ -112,6 +113,13 @@ NodeState theirs = parent.getChildNode(name); Resolution resolution = nodeConflictHandler.resolve(name, ours, theirs); applyResolution(resolution, conflictType, name, ours); + if (LOG.isDebugEnabled()) { + String diff = JsopDiff.diffToJsop(ours, theirs); + LOG.debug( + "{} resolved conflict of type {} with resolution {}, conflict trace {}", + nodeConflictHandler, conflictType, resolution, + diff); + } } } else { @@ -179,30 +187,55 @@ public Resolution resolve(PropertyState ours, PropertyState theirs) { return conflictHandler.addExistingProperty(target, ours, theirs); } + + @Override + public String toString() { + return "PropertyConflictHandler"; + } }, CHANGE_DELETED_PROPERTY, new PropertyConflictHandler() { @Override public Resolution resolve(PropertyState ours, PropertyState theirs) { return conflictHandler.changeDeletedProperty(target, ours); } + + @Override + public String toString() { + return "PropertyConflictHandler"; + } }, CHANGE_CHANGED_PROPERTY, new PropertyConflictHandler() { @Override public Resolution resolve(PropertyState ours, PropertyState theirs) { return conflictHandler.changeChangedProperty(target, ours, theirs); } + + @Override + public String toString() { + return "PropertyConflictHandler"; + } }, DELETE_DELETED_PROPERTY, new PropertyConflictHandler() { @Override public Resolution resolve(PropertyState ours, PropertyState theirs) { return conflictHandler.deleteDeletedProperty(target, ours); } + + @Override + public String toString() { + return "PropertyConflictHandler"; + } }, DELETE_CHANGED_PROPERTY, new PropertyConflictHandler() { @Override public Resolution resolve(PropertyState ours, PropertyState theirs) { return conflictHandler.deleteChangedProperty(target, theirs); } + + @Override + public String toString() { + return "PropertyConflictHandler"; + } } ); @@ -212,24 +245,44 @@ public Resolution resolve(String name, NodeState ours, NodeState theirs) { return conflictHandler.addExistingNode(target, name, ours, theirs); } + + @Override + public String toString() { + return "NodeConflictHandler"; + } }, CHANGE_DELETED_NODE, new NodeConflictHandler() { @Override public Resolution resolve(String name, NodeState ours, NodeState theirs) { return conflictHandler.changeDeletedNode(target, name, ours); } + + @Override + public String toString() { + return "NodeConflictHandler"; + } }, DELETE_CHANGED_NODE, new NodeConflictHandler() { @Override public Resolution resolve(String name, NodeState ours, NodeState theirs) { return conflictHandler.deleteChangedNode(target, name, theirs); } + + @Override + public String toString() { + return "NodeConflictHandler"; + } }, DELETE_DELETED_NODE, new NodeConflictHandler() { @Override public Resolution resolve(String name, NodeState ours, NodeState theirs) { return conflictHandler.deleteDeletedNode(target, name); } + + @Override + public String toString() { + return "NodeConflictHandler"; + } } ); Index: oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeStateUtils.java =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeStateUtils.java (revision 1689931) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeStateUtils.java (working copy) @@ -21,6 +21,7 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; +import org.apache.commons.io.IOUtils; import org.apache.jackrabbit.JcrConstants; import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.Type; @@ -80,4 +81,49 @@ return node; } + /** + * Provides a string representation of the given node state + * + * @param node + * node state + * @return a string representation of {@code node}. + */ + public static String toString(NodeState node) { + if (node == null) { + return "[null]"; + } + StringBuilder sb = new StringBuilder(); + sb.append(toString(node, 1, " ", "/")); + return sb.toString(); + } + + private static String toString(NodeState ns, int level, String prepend, + String name) { + StringBuilder node = new StringBuilder(); + node.append(prepend).append(name); + + StringBuilder props = new StringBuilder(); + boolean first = true; + for (PropertyState ps : ns.getProperties()) { + if (!first) { + props.append(", "); + } else { + first = false; + } + props.append(ps); + } + + if (props.length() > 0) { + node.append("{"); + node.append(props); + node.append("}"); + } + for (ChildNodeEntry c : ns.getChildNodeEntries()) { + node.append(IOUtils.LINE_SEPARATOR); + node.append(toString(c.getNodeState(), level++, prepend + prepend, + c.getName())); + } + return node.toString(); + } + } Index: oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ConflictResolutionTest.java =================================================================== --- oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ConflictResolutionTest.java (revision 0) +++ oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ConflictResolutionTest.java (revision 0) @@ -0,0 +1,71 @@ +/* + * 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.jcr; + +import static org.junit.Assert.fail; + +import javax.jcr.InvalidItemStateException; +import javax.jcr.RepositoryException; +import javax.jcr.Session; + +import org.junit.Test; + +public class ConflictResolutionTest extends AbstractRepositoryTest { + + // TODO add tests for all ConflictType types to observe generated logs + + public ConflictResolutionTest(NodeStoreFixture fixture) { + super(fixture); + } + + @Test + public void deleteChangedNode() throws RepositoryException { + getAdminSession().getRootNode().addNode("node").addNode("jcr:content") + .addNode("metadata"); + getAdminSession().save(); + + Session session1 = createAdminSession(); + Session session2 = createAdminSession(); + try { + session1.getNode("/node/jcr:content").remove(); + session2.getNode("/node/jcr:content/metadata").setProperty( + "updated", "myself"); + session2.save(); + try { + session1.save(); + fail("Expected InvalidItemStateException"); + } catch (InvalidItemStateException expected) { + // javax.jcr.InvalidItemStateException: OakState0001: Unresolved + // conflicts in /node + // + // ConflictValidator debug: Commit failed due to unresolved + // conflicts in /node = {deleteChangedNode = {jcr:content}} + // + // MergingNodeStateDif debug: [MergingNodeStateDiff] + // NodeConflictHandler resolved conflict of + // type DELETE_CHANGED_NODE with resolution THEIRS, conflict + // trace ^"/metadata/updated":"myself" + } + } finally { + session1.logout(); + session2.logout(); + } + } + +}