Return-Path: X-Original-To: apmail-jackrabbit-oak-commits-archive@minotaur.apache.org Delivered-To: apmail-jackrabbit-oak-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 156BFD3A6 for ; Thu, 1 Nov 2012 15:42:21 +0000 (UTC) Received: (qmail 6315 invoked by uid 500); 1 Nov 2012 15:42:20 -0000 Delivered-To: apmail-jackrabbit-oak-commits-archive@jackrabbit.apache.org Received: (qmail 6191 invoked by uid 500); 1 Nov 2012 15:42:16 -0000 Mailing-List: contact oak-commits-help@jackrabbit.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: oak-dev@jackrabbit.apache.org Delivered-To: mailing list oak-commits@jackrabbit.apache.org Received: (qmail 6121 invoked by uid 99); 1 Nov 2012 15:42:14 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 01 Nov 2012 15:42:14 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 01 Nov 2012 15:42:12 +0000 Received: from eris.apache.org (localhost [127.0.0.1]) by eris.apache.org (Postfix) with ESMTP id 42F28238896F; Thu, 1 Nov 2012 15:41:17 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1404646 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/ oak-core/src/test/java/org/apache/jackrabbit/oak/core/ oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/ oak-jcr/src/test/java/org/apache/jackrab... Date: Thu, 01 Nov 2012 15:41:16 -0000 To: oak-commits@jackrabbit.apache.org From: mduerig@apache.org X-Mailer: svnmailer-1.0.8-patched Message-Id: <20121101154117.42F28238896F@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: mduerig Date: Thu Nov 1 15:41:16 2012 New Revision: 1404646 URL: http://svn.apache.org/viewvc?rev=1404646&view=rev Log: OAK-391: Avoid weak references in TreeImpl OAK-14: Identify and document changes in behaviour wrt. Jackrabbit 2 - remove weak reference cache from TreeImpl - adapt test expectations wrt. moved nodes - add backward (in)compatibility tests Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/core/RootImplTest.java jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/ItemDelegate.java jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/CompatibilityIssuesTest.java jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java?rev=1404646&r1=1404645&r2=1404646&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java (original) +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java Thu Nov 1 15:41:16 2012 @@ -20,13 +20,16 @@ package org.apache.jackrabbit.oak.core; import java.util.Collections; import java.util.Iterator; -import java.util.Map; import java.util.Set; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import com.google.common.base.Function; +import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.Tree; import org.apache.jackrabbit.oak.api.TreeLocation; @@ -41,12 +44,6 @@ import org.apache.jackrabbit.oak.spi.sta import org.apache.jackrabbit.oak.spi.state.NodeStateUtils; import org.apache.jackrabbit.oak.spi.state.PropertyBuilder; -import com.google.common.base.Function; -import com.google.common.base.Predicate; -import com.google.common.cache.CacheBuilder; -import com.google.common.collect.Iterables; -import com.google.common.collect.Sets; - import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static org.apache.jackrabbit.oak.commons.PathUtils.elements; @@ -71,12 +68,6 @@ public class TreeImpl implements Tree, P /** Lazily initialised {@code NodeBuilder} for the underlying node state */ NodeBuilder nodeBuilder; - /** - * Cache for child trees that have been accessed before. - */ - private final Map children = - CacheBuilder.newBuilder().weakValues().build().asMap(); - private TreeImpl(RootImpl root, TreeImpl parent, String name) { this.root = checkNotNull(root); this.parent = parent; @@ -154,7 +145,7 @@ public class TreeImpl implements Tree, P root.checkLive(); Status nodeStatus = getStatus(); if (nodeStatus == Status.NEW) { - return (hasProperty(name)) ? Status.NEW : null; + return (hasProperty(name)) ? Status.NEW : null; } else if (nodeStatus == Status.REMOVED) { return Status.REMOVED; // FIXME not correct if no property existed with that name } else { @@ -264,12 +255,7 @@ public class TreeImpl implements Tree, P new Function() { @Override public Tree apply(String input) { - TreeImpl child = children.get(input); - if (child == null) { - child = new TreeImpl(root, TreeImpl.this, input); - children.put(input, child); - } - return child; + return new TreeImpl(root, TreeImpl.this, input); } }), new Predicate() { @@ -309,7 +295,6 @@ public class TreeImpl implements Tree, P if (!isRoot() && parent.hasChild(name)) { NodeBuilder builder = parent.getNodeBuilder(); builder.removeNode(name); - parent.children.remove(name); removed = true; if (parent.hasOrderableChildren()) { builder.setProperty( @@ -456,9 +441,6 @@ public class TreeImpl implements Tree, P throw new IllegalStateException("Cannot move removed tree"); } - parent.children.remove(name); - destParent.children.put(destName, this); - name = destName; parent = destParent; } @@ -515,12 +497,9 @@ public class TreeImpl implements Tree, P //------------------------------------------------------------< private >--- private TreeImpl internalGetChild(String childName) { - TreeImpl child = children.get(childName); - if (child == null && getNodeBuilder().hasChildNode(childName)) { - child = new TreeImpl(root, this, childName); - children.put(childName, child); - } - return child; + return getNodeBuilder().hasChildNode(childName) + ? new TreeImpl(root, this, childName) + : null; } private PropertyState internalGetProperty(String propertyName) { Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/core/RootImplTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/core/RootImplTest.java?rev=1404646&r1=1404645&r2=1404646&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/core/RootImplTest.java (original) +++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/core/RootImplTest.java Thu Nov 1 15:41:16 2012 @@ -110,20 +110,6 @@ public class RootImplTest { assertTrue(tree.getChild("y").hasChild("xx")); } - @Test - public void move2() { - Root root = session.getLatestRoot(); - Tree r = root.getTree("/"); - Tree x = r.getChild("x"); - Tree y = r.getChild("y"); - - assertFalse(y.hasChild("x")); - assertEquals("", x.getParent().getName()); - root.move("/x", "/y/x"); - assertTrue(y.hasChild("x")); - assertEquals("y", x.getParent().getName()); - } - /** * Regression test for OAK-208 */ Modified: jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/ItemDelegate.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/ItemDelegate.java?rev=1404646&r1=1404645&r2=1404646&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/ItemDelegate.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/ItemDelegate.java Thu Nov 1 15:41:16 2012 @@ -133,7 +133,12 @@ public abstract class ItemDelegate { } private static boolean isStale(TreeLocation location) { - return location.getStatus() == Status.REMOVED || location.getPath() == null; + try { + return location.getStatus() == Status.REMOVED || location.getPath() == null; + } + catch (IllegalStateException e) { + return true; // FIXME left over from OAK-391. Find better way to determine staleness + } } } Modified: jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/CompatibilityIssuesTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/CompatibilityIssuesTest.java?rev=1404646&r1=1404645&r2=1404646&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/CompatibilityIssuesTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/CompatibilityIssuesTest.java Thu Nov 1 15:41:16 2012 @@ -20,8 +20,16 @@ import javax.jcr.Node; import javax.jcr.RepositoryException; import javax.jcr.Session; +import org.apache.jackrabbit.oak.Oak; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.ContentSession; +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; /** @@ -60,12 +68,12 @@ public class CompatibilityIssuesTest ext session1.save(); session2.getNode("/testNode").setProperty("p2", -1); - check(session2); // Throws on JR2, not on JR3 + check(session2); // Throws on JR2, not on Oak session2.save(); Session session3 = createAnonymousSession(); try { - check(session3); // Throws on JR3 + check(session3); // Throws on Oak fail(); } catch (AssertionError e) { // expected @@ -91,4 +99,40 @@ public class CompatibilityIssuesTest ext } } + @Test + public void move() throws RepositoryException { + Session session = getAdminSession(); + + Node node = session.getNode("/"); + node.addNode("source").addNode("node"); + node.addNode("target"); + session.save(); + + session.refresh(true); + Node sourceNode = session.getNode("/source/node"); + session.move("/source/node", "/target/moved"); + // assertEquals("/target/moved", sourceNode.getPath()); // passes on JR2, fails on Oak + assertEquals("/source/node", sourceNode.getPath()); // fails on JR2, passed on Oak + } + + @Test + public void move2() throws CommitFailedException { + ContentSession session = new Oak().createContentSession(); + Root root = session.getLatestRoot(); + root.getTree("/").addChild("x"); + root.getTree("/").addChild("y"); + root.commit(); + + Tree r = root.getTree("/"); + Tree x = r.getChild("x"); + Tree y = r.getChild("y"); + + assertFalse(y.hasChild("x")); + assertEquals("", x.getParent().getName()); + root.move("/x", "/y/x"); + assertTrue(y.hasChild("x")); + // assertEquals("y", x.getParent().getName()); // passed on JR2, fails on Oak + assertEquals("", x.getParent().getName()); // fails on JR2, passes on Oak + } + } Modified: jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java?rev=1404646&r1=1404645&r2=1404646&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java (original) +++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java Thu Nov 1 15:41:16 2012 @@ -1356,10 +1356,8 @@ public class RepositoryTest extends Abst node.addNode("target"); session.save(); - Node sourceNode = session.getNode(TEST_PATH + "/source/node"); session.refresh(true); session.move(TEST_PATH + "/source/node", TEST_PATH + "/target/moved"); - assertEquals("/test_node/target/moved", sourceNode.getPath()); assertFalse(node.hasNode("source/node")); assertTrue(node.hasNode("source")); @@ -1381,10 +1379,8 @@ public class RepositoryTest extends Abst node.addNode("target"); session.save(); - Node sourceNode = session.getNode(TEST_PATH + "/source/node"); session.refresh(true); session.move(TEST_PATH + "/source/node", TEST_PATH + "/target/moved"); - assertEquals("/test_node/target/moved", sourceNode.getPath()); assertFalse(node.hasNode("source/node")); assertTrue(node.hasNode("source"));