Index: jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/ConsistencyCheckerImplTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/ConsistencyCheckerImplTest.java (revision ) +++ jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/ConsistencyCheckerImplTest.java (revision ) @@ -0,0 +1,264 @@ +/* + * 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.core.data; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import javax.jcr.RepositoryException; + +import org.apache.jackrabbit.core.id.NodeId; +import org.apache.jackrabbit.core.persistence.bundle.AbstractBundlePersistenceManager; +import org.apache.jackrabbit.core.persistence.bundle.ConsistencyCheckerImpl; +import org.apache.jackrabbit.core.persistence.check.ConsistencyChecker; +import org.apache.jackrabbit.core.persistence.util.BLOBStore; +import org.apache.jackrabbit.core.persistence.util.NodePropBundle; +import org.apache.jackrabbit.core.state.ItemStateException; +import org.apache.jackrabbit.core.state.NoSuchItemStateException; +import org.apache.jackrabbit.core.state.NodeReferences; +import org.apache.jackrabbit.spi.NameFactory; +import org.apache.jackrabbit.spi.commons.name.NameConstants; +import org.apache.jackrabbit.spi.commons.name.NameFactoryImpl; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import junit.framework.TestCase; + +public class ConsistencyCheckerImplTest extends TestCase { + + private static final Logger log = LoggerFactory.getLogger(ConsistencyCheckerImplTest.class); + + private static final NameFactory nameFactory = NameFactoryImpl.getInstance(); + + // Abandoned nodes are nodes that have a link to a parent but that + // parent does not have a link back to the child + public void testFixAbandonedNode() throws RepositoryException { + NodePropBundle bundle1 = new NodePropBundle(new NodeId(0, 0)); + NodePropBundle bundle2 = new NodePropBundle(new NodeId(0, 1)); + + // node2 has a reference to node 1 as its parent, but node 1 doesn't have + // a corresponding child node entry + bundle2.setParentId(bundle1.getId()); + + MockPersistenceManager pm = new MockPersistenceManager(Arrays.asList(bundle1, bundle2)); + ConsistencyCheckerImpl checker = new ConsistencyCheckerImpl(pm, null); + + // run checker with fix = true + checker.check(null, false, true, null); + + // node1 should now have a child node entry for node2 + bundle1 = pm.loadBundle(bundle1.getId()); + assertEquals(1, bundle1.getChildNodeEntries().size()); + assertEquals(bundle2.getId(), bundle1.getChildNodeEntries().get(0).getId()); + } + + /* + * There was a bug where when there were multiple abandoned nodes by the same parent + * only one of them was fixed. Hence this separate test case for this scenario. + */ + public void testFixMultipleAbandonedNodesBySameParent() throws RepositoryException { + NodePropBundle bundle1 = new NodePropBundle(new NodeId(0, 0)); + NodePropBundle bundle2 = new NodePropBundle(new NodeId(0, 1)); + NodePropBundle bundle3 = new NodePropBundle(new NodeId(1, 0)); + + + // node2 and node3 have a reference to node1 as its parent, but node1 doesn't have + // corresponding child node entries + bundle2.setParentId(bundle1.getId()); + bundle3.setParentId(bundle1.getId()); + + MockPersistenceManager pm = new MockPersistenceManager(Arrays.asList(bundle1, bundle2, bundle3)); + ConsistencyCheckerImpl checker = new ConsistencyCheckerImpl(pm, null); + + // run checker with fix = true + checker.check(null, false, true, null); + + // node1 should now have child node entries for node2 and node3 + bundle1 = pm.loadBundle(bundle1.getId()); + assertEquals(2, bundle1.getChildNodeEntries().size()); + assertEquals(bundle2.getId(), bundle1.getChildNodeEntries().get(0).getId()); + assertEquals(bundle3.getId(), bundle1.getChildNodeEntries().get(1).getId()); + } + + // Orphaned nodes are those nodes who's parent does not exist + public void testAddOrphanedNodeToLostAndFound() throws RepositoryException { + NodePropBundle lostAndFound = new NodePropBundle(new NodeId(0, 0)); + // lost and found must be of type nt:unstructured + lostAndFound.setNodeTypeName(NameConstants.NT_UNSTRUCTURED); + + NodePropBundle orphaned = new NodePropBundle(new NodeId(0, 1)); + // set non-existent parent node id + orphaned.setParentId(new NodeId(1, 0)); + + MockPersistenceManager pm = new MockPersistenceManager(Arrays.asList(lostAndFound, orphaned)); + ConsistencyCheckerImpl checker = new ConsistencyCheckerImpl(pm, null); + + // run checker with fix = true + checker.check(null, false, true, lostAndFound.getId().toString()); + + // orphan should have been added to lost+found + lostAndFound = pm.loadBundle(lostAndFound.getId()); + assertEquals(1, lostAndFound.getChildNodeEntries().size()); + assertEquals(orphaned.getId(), lostAndFound.getChildNodeEntries().get(0).getId()); + + orphaned = pm.loadBundle(orphaned.getId()); + assertEquals(lostAndFound.getId(), orphaned.getParentId()); + } + + // Disconnected nodes are those nodes for which there are nodes + // that have the node as its child, but the node itself does not + // have those nodes as its parent + public void testFixDisconnectedNode() throws RepositoryException { + NodePropBundle bundle1 = new NodePropBundle(new NodeId(0, 0)); + NodePropBundle bundle2 = new NodePropBundle(new NodeId(0, 1)); + NodePropBundle bundle3 = new NodePropBundle(new NodeId(1, 0)); + + // node1 has child node3 + bundle1.addChildNodeEntry(nameFactory.create("", "test"), bundle3.getId()); + // node2 also has child node3 + bundle2.addChildNodeEntry(nameFactory.create("", "test"), bundle3.getId()); + // node3 has node2 as parent + bundle3.setParentId(bundle2.getId()); + + MockPersistenceManager pm = new MockPersistenceManager(Arrays.asList(bundle1, bundle2, bundle3)); + ConsistencyCheckerImpl checker = new ConsistencyCheckerImpl(pm, null); + + // run checker with fix = true + checker.check(null, false, true, null); + + bundle1 = pm.loadBundle(bundle1.getId()); + bundle2 = pm.loadBundle(bundle2.getId()); + bundle3 = pm.loadBundle(bundle3.getId()); + + // node3 should have been removed as child node entry of node1 + assertEquals(0, bundle1.getChildNodeEntries().size()); + + // node3 should still be a child of node2 + assertEquals(1, bundle2.getChildNodeEntries().size()); + assertEquals(bundle2.getId(), bundle3.getParentId()); + } + + // make sure we don't fix anything in check mode, we can't be careful enough + public void testDontFixInCheckMode() throws RepositoryException { + /** abandoned node, also see {@link #testFixAbandonedNode} */ + NodePropBundle bundle1 = new NodePropBundle(new NodeId(0, 0)); + NodePropBundle bundle2 = new NodePropBundle(new NodeId(0, 1)); + bundle2.setParentId(bundle1.getId()); + + /** orphaned node, also see {@link #testAddOrphanedNodeToLostAndFound} */ + NodePropBundle lostAndFound = new NodePropBundle(new NodeId(1, 0)); + lostAndFound.setNodeTypeName(NameConstants.NT_UNSTRUCTURED); + NodePropBundle orphaned = new NodePropBundle(new NodeId(1, 1)); + orphaned.setParentId(new NodeId(0, 2)); + + /** disconnected node, also see {@link #testFixDisconnectedNode} */ + NodePropBundle bundle3 = new NodePropBundle(new NodeId(1, 2)); + NodePropBundle bundle4 = new NodePropBundle(new NodeId(2, 2)); + NodePropBundle bundle5 = new NodePropBundle(new NodeId(0, 3)); + bundle3.addChildNodeEntry(nameFactory.create("", "test"), bundle5.getId()); + bundle4.addChildNodeEntry(nameFactory.create("", "test"), bundle5.getId()); + bundle5.setParentId(bundle4.getId()); + + MockPersistenceManager pm = new MockPersistenceManager(Arrays.asList(bundle1, bundle2, bundle3, bundle4, bundle5, lostAndFound, orphaned)); + ConsistencyCheckerImpl checker = new ConsistencyCheckerImpl(pm, null); + + // run checker with fix = false + checker.check(null, false, false, null); + + bundle1 = pm.loadBundle(bundle1.getId()); + lostAndFound = pm.loadBundle(lostAndFound.getId()); + bundle3 = pm.loadBundle(bundle3.getId()); + + // abandoned node should not have been un-abandoned + assertEquals(0, bundle1.getChildNodeEntries().size()); + + // orphan should not have been added to lost and found + assertEquals(0, lostAndFound.getChildNodeEntries().size()); + + // disconnected entries should not have been removed + assertEquals(1, bundle3.getChildNodeEntries().size()); + + } + + private static class MockPersistenceManager extends AbstractBundlePersistenceManager { + + private Map bundles = new LinkedHashMap(); + + private MockPersistenceManager(List bundles) { + for (NodePropBundle bundle : bundles) { + this.bundles.put(bundle.getId(), bundle); + } + } + + public List getAllNodeIds(final NodeId after, final int maxCount) throws ItemStateException, RepositoryException { + List allNodeIds = new ArrayList(); + boolean add = after == null; + for (NodeId nodeId : bundles.keySet()) { + if (add) { + allNodeIds.add(nodeId); + } + if (!add) { + add = nodeId.equals(after); + } + } + return allNodeIds; + } + + @Override + protected NodePropBundle loadBundle(final NodeId id) { + return bundles.get(id); + } + + @Override + protected void evictBundle(final NodeId id) { + } + + @Override + protected void storeBundle(final NodePropBundle bundle) throws ItemStateException { + bundles.put(bundle.getId(), bundle); + } + + @Override + protected void destroyBundle(final NodePropBundle bundle) throws ItemStateException { + bundles.remove(bundle.getId()); + } + + @Override + protected void destroy(final NodeReferences refs) throws ItemStateException { + } + + @Override + protected void store(final NodeReferences refs) throws ItemStateException { + } + + @Override + protected BLOBStore getBlobStore() { + return null; + } + + public NodeReferences loadReferencesTo(final NodeId id) throws NoSuchItemStateException, ItemStateException { + return null; + } + + public boolean existsReferencesTo(final NodeId targetId) throws ItemStateException { + return false; + } + } +} \ No newline at end of file Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/ConsistencyCheckerImpl.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/ConsistencyCheckerImpl.java (revision 1301887) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/ConsistencyCheckerImpl.java (revision ) @@ -76,8 +76,6 @@ boolean fix, Set reports, String lostNFoundId) throws RepositoryException { int count = 0; - Collection modifications = new ArrayList(); - Set orphaned = new HashSet(); NodeId lostNFound = null; if (fix && lostNFoundId != null) { @@ -116,9 +114,7 @@ error(id.toString(), "No bundle found for id '" + id + "'"); } else { - checkBundleConsistency(id, bundle, fix, - modifications, lostNFound, orphaned, - reports); + checkBundleConsistency(id, bundle, fix, lostNFound, reports); count++; if (count % 1000 == 0 && listener == null) { @@ -174,8 +170,7 @@ + id + "'"); } } else { - checkBundleConsistency(id, bundle, fix, modifications, - lostNFound, orphaned, reports); + checkBundleConsistency(id, bundle, fix, lostNFound, reports); if (recursive) { for (NodePropBundle.ChildNodeEntry entry : bundle @@ -197,49 +192,6 @@ } } - // repair collected broken bundles - if (fix && !modifications.isEmpty()) { - info(null, pm + ": Fixing " + modifications.size() - + " inconsistent bundle(s)..."); - for (NodePropBundle bundle : modifications) { - try { - info(bundle.getId().toString(), pm + ": Fixing bundle '" - + bundle.getId() + "'"); - bundle.markOld(); // use UPDATE instead of INSERT - pm.storeBundle(bundle); - pm.evictBundle(bundle.getId()); - } catch (ItemStateException e) { - error(bundle.getId().toString(), pm - + ": Error storing fixed bundle: " + e); - } - } - } - - if (fix && lostNFoundId != null && !orphaned.isEmpty()) { - // do we have things to add to "lost+found"? - try { - NodePropBundle lfBundle = pm.loadBundle(lostNFound); - if (lfBundle == null) { - error(lostNFoundId, "specified 'lost+found' node does not exist"); - } else if (!NameConstants.NT_UNSTRUCTURED.equals(lfBundle - .getNodeTypeName())) { - error(lostNFoundId, "specified 'lost+found' node is not of type nt:unstructered"); - } else { - lfBundle.markOld(); - for (NodeId orphan : orphaned) { - String nodeName = orphan + "-" - + System.currentTimeMillis(); - lfBundle.addChildNodeEntry(NF.create("", nodeName), - orphan); - } - pm.storeBundle(lfBundle); - pm.evictBundle(lfBundle.getId()); - } - } catch (Exception ex) { - error(null, "trying orphan adoption", ex); - } - } - log.info(pm + ": checked " + count + " bundles."); return count; @@ -255,13 +207,9 @@ * the bundle to check * @param fix * if true, repair things that can be repaired - * @param modifications - * if fix == true, collect the repaired - * {@linkplain NodePropBundle bundles} here */ private void checkBundleConsistency(NodeId id, NodePropBundle bundle, - boolean fix, Collection modifications, - NodeId lostNFoundId, Set orphaned, Set reports) { + boolean fix, NodeId lostNFoundId, Set reports) { // log.info(name + ": checking bundle '" + id + "'"); // skip all virtual nodes @@ -275,36 +223,80 @@ // look at the node's children Collection missingChildren = new ArrayList(); + Collection disconnectedChildren = new ArrayList(); for (NodePropBundle.ChildNodeEntry entry : bundle.getChildNodeEntries()) { + NodeId childNodeId = entry.getId(); + // skip check for system nodes (root, system root, version storage, // node types) - if (entry.getId().toString().endsWith("babecafebabe")) { + if (childNodeId.toString().endsWith("babecafebabe")) { continue; } try { // analyze child node bundles - NodePropBundle child = pm.loadBundle(entry.getId()); + NodePropBundle childBundle = pm.loadBundle(childNodeId); String message = null; - if (child == null) { + // does the child exist? + if (childBundle == null) { + // double check whether we still exist and the child entry is still there + bundle = pm.loadBundle(id); + + if (bundle != null) { + boolean stillThere = false; + for (NodePropBundle.ChildNodeEntry entryRetry : bundle.getChildNodeEntries()) { + if (entryRetry.getId().equals(childNodeId)) { + stillThere = true; + break; + } + } + if (stillThere) { - message = "NodeState '" + id - + "' references inexistent child" + " '" - + entry.getName() + "' with id " + "'" + message = "NodeState '" + id + + "' references inexistent child" + " '" + + entry.getName() + "' with id " + "'" - + entry.getId() + "'"; + + childNodeId + "'"; - log.error(message); - missingChildren.add(entry); + log.error(message); + missingChildren.add(entry); + } + } } else { - NodeId cp = child.getParentId(); - if (cp == null) { - message = "ChildNode has invalid parent id: "; + // if the child exists does it reference the current node as its parent? + NodeId cp = childBundle.getParentId(); + if (!id.equals(cp)) { + + // we can distinguish four cases here: + // 1. the child's parent id is of a non-existent node + // 2. the child's parent id is of an existent node and that node has a corresponding child node entry + // 3. the child's parent id is of an existent node but that node does not have a corresponding child node entry + // 4. the child's parent id is null + // + // In 1 the child is an orphan and will be added to lost+found when it is checked for consistency + // In 2 the current node has an invalid child node entry for the child + // In 3 the child is an abandoned node and will be added again to its parent when it is checked for consistency + // + // In all cases we should remove the child node entry from the current node + + // double check whether we still exist and the child entry is still there + bundle = pm.loadBundle(id); + if (bundle != null) { + boolean stillThere = false; + for (NodePropBundle.ChildNodeEntry entryRetry : bundle.getChildNodeEntries()) { + if (entryRetry.getId().equals(childNodeId)) { + // still there + stillThere = true; + break; + } + } + if (stillThere) { + // indeed we have a disconnected child + message = "ChildNode has invalid parent id: '" + cp + "' (instead of '" + id + "')"; - log.error(message); + log.error(message); - } else if (!cp.equals(id)) { - message = "ChildNode has invalid parent id: '" + cp - + "' (instead of '" + id + "')"; - log.error(message); + disconnectedChildren.add(entry); - } - } + } + } + } + } if (message != null) { addMessage(reports, id, message); } @@ -315,13 +307,17 @@ } } // remove child node entry (if fixing is enabled) - if (fix && !missingChildren.isEmpty()) { + if (fix && (!missingChildren.isEmpty() || (!disconnectedChildren.isEmpty()))) { for (NodePropBundle.ChildNodeEntry entry : missingChildren) { bundle.getChildNodeEntries().remove(entry); } - modifications.add(bundle); + for (NodePropBundle.ChildNodeEntry entry : disconnectedChildren) { + bundle.getChildNodeEntries().remove(entry); - } + } + fixBundle(bundle); + } + // check parent reference NodeId parentId = bundle.getParentId(); try { @@ -330,21 +326,34 @@ NodePropBundle parentBundle = pm.loadBundle(parentId); if (parentBundle == null) { + // double check whether we still exist and the parent is still the same + bundle = pm.loadBundle(id); + if (bundle != null) { + if (bundle.getParentId().equals(parentId)) { - String message = "NodeState '" + id - + "' references inexistent parent id '" + parentId - + "'"; - log.error(message); - addMessage(reports, id, message); + String message = "NodeState '" + id + + "' references inexistent parent id '" + parentId + + "'"; + log.error(message); + addMessage(reports, id, message); - orphaned.add(id); - if (lostNFoundId != null) { + if (fix && lostNFoundId != null) { + // add a child to lost+found + NodePropBundle lfBundle = pm.loadBundle(lostNFoundId); + lfBundle.markOld(); + String nodeName = id + "-" + System.currentTimeMillis(); + lfBundle.addChildNodeEntry(NF.create("", nodeName), id); + pm.storeBundle(lfBundle); + pm.evictBundle(lostNFoundId); + + // set lost+found parent - bundle.setParentId(lostNFoundId); + bundle.setParentId(lostNFoundId); - modifications.add(bundle); + fixBundle(bundle); - } + } + } + } } else { boolean found = false; - for (NodePropBundle.ChildNodeEntry entry : parentBundle - .getChildNodeEntries()) { + for (NodePropBundle.ChildNodeEntry entry : parentBundle.getChildNodeEntries()) { if (entry.getId().equals(id)) { found = true; break; @@ -352,31 +361,50 @@ } if (!found) { + // double check whether we still exist and the parent is still the same + bundle = pm.loadBundle(id); + if (bundle != null) { + if (bundle.getParentId().equals(parentId)) { - String message = "NodeState '" + id - + "' is not referenced by its parent node '" - + parentId + "'"; - log.error(message); - addMessage(reports, id, message); + String message = "NodeState '" + id + + "' is not referenced by its parent node '" + + parentId + "'"; + log.error(message); + addMessage(reports, id, message); + if (fix) { - int l = (int) System.currentTimeMillis(); - int r = new Random().nextInt(); - int n = l + r; - String nodeName = Integer.toHexString(n); - parentBundle.addChildNodeEntry( - NF.create("{}" + nodeName), id); - log.info("NodeState '" + id - + "' adds itself to its parent node '" + int l = (int) System.currentTimeMillis(); + int r = new Random().nextInt(); + int n = l + r; + String nodeName = Integer.toHexString(n); + parentBundle.addChildNodeEntry( + NF.create("{}" + nodeName), id); + log.info("NodeState '" + id + + "' adds itself to its parent node '" - + parentId + "' with a new name '" + nodeName - + "'"); - modifications.add(parentBundle); + + parentId + "' with a new name '" + nodeName + "'"); + fixBundle(parentBundle); - } - } - } + } + } + } + } + + } + } } catch (ItemStateException e) { String message = "Error reading node '" + parentId + "' (parent of '" + id + "'): " + e; log.error(message); addMessage(reports, id, message); + } + } + + private void fixBundle(NodePropBundle bundle) { + try { + log.info(pm + ": Fixing bundle '" + bundle.getId() + "'"); + bundle.markOld(); // use UPDATE instead of INSERT + pm.storeBundle(bundle); + pm.evictBundle(bundle.getId()); + } catch (ItemStateException e) { + log.error(pm + ": Error storing fixed bundle: " + e); } }