jackrabbit-oak-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mdue...@apache.org
Subject svn commit: r1411619 - in /jackrabbit/oak/trunk/oak-core/src: main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java test/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilderTest.java
Date Tue, 20 Nov 2012 11:20:31 GMT
Author: mduerig
Date: Tue Nov 20 11:20:30 2012
New Revision: 1411619

URL: http://svn.apache.org/viewvc?rev=1411619&view=rev
Log:
OAK-452: MemoryNodeBuilder improvements
changes from https://github.com/mduerig/jackrabbit-oak/commits/nodebuilder-cleanup

Modified:
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilderTest.java

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java?rev=1411619&r1=1411618&r2=1411619&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java
(original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java
Tue Nov 20 11:20:30 2012
@@ -22,7 +22,6 @@ import java.util.Map.Entry;
 
 import javax.annotation.Nonnull;
 
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import org.apache.jackrabbit.oak.api.PropertyState;
 import org.apache.jackrabbit.oak.api.Type;
@@ -32,6 +31,7 @@ import org.apache.jackrabbit.oak.spi.sta
 import org.apache.jackrabbit.oak.spi.state.NodeStateDiff;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static org.apache.jackrabbit.oak.plugins.memory.ModifiedNodeState.with;
 import static org.apache.jackrabbit.oak.plugins.memory.ModifiedNodeState.withNodes;
 import static org.apache.jackrabbit.oak.plugins.memory.ModifiedNodeState.withProperties;
@@ -69,10 +69,6 @@ import static org.apache.jackrabbit.oak.
  */
 public class MemoryNodeBuilder implements NodeBuilder {
 
-    private static final NodeState NULL_STATE = new MemoryNodeState(
-            ImmutableMap.<String, PropertyState>of(),
-            ImmutableMap.<String, NodeState>of());
-
     /**
      * Parent builder, or {@code null} for a root builder.
      */
@@ -142,78 +138,126 @@ public class MemoryNodeBuilder implement
         this.writeState = new MutableNodeState(baseState);
     }
 
+    private boolean classInvariants() {
+        boolean rootHasNoParent = isRoot() == (parent == null);
+        boolean rootHasWriteState = root.writeState != null;
+        boolean baseStateOrWriteStateNotNull = baseState != null || writeState != null;
+
+        return rootHasNoParent && rootHasWriteState && baseStateOrWriteStateNotNull;
+    }
+
+    private boolean isRoot() {
+        return this == root;
+    }
+
+    /**
+     * Return the base state of the named child. Assumes {@code read()} needs not be called.
+     * @param name  name of the child
+     * @return  base state of the child
+     */
+    private NodeState getBaseState(String name) {
+        if (baseState != null) {
+            return baseState.getChildNode(name);
+        } else {
+            return null;
+        }
+    }
+
+    /**
+     * Determine whether the base state has a child of the given name.
+     * Assumes {@code read()} needs not be called.
+     * @param name  name of the child
+     * @return  {@code true} iff the base state has a child of the given name.
+     */
+    private boolean hasBaseState(String name) {
+        return baseState != null && baseState.hasChildNode(name);
+    }
+
+    /**
+     * Return the write state of the named child. Assumes {@code read()}, {@code write()}
needs not be called.
+     * @param name  name of the child
+     * @return  base state of the child
+     */
+    private MutableNodeState getWriteState(String name) {
+        if (writeState != null) {
+            return writeState.nodes.get(name);
+        }
+        else {
+            return null;
+        }
+    }
+
+    /**
+     * Determine whether the named child has been removed. This is the
+     * case when the write state has a corresponding {@code null} entry.
+     * Assumes {@code read()}, {@code write()} needs not be called.
+     * @param name  name of the child
+     * @return  {@code true} iff a child with the given name has been removed
+     */
+    private boolean removed(String name) {
+        return writeState != null && writeState.isRemoved(name);
+    }
+
+    @Nonnull
     private NodeState read() {
         if (revision != root.revision) {
-            assert(parent != null); // root never gets here
+            assert(!isRoot()); // root never gets here since revision == root.revision
+            checkState(!parent.removed(name));
             parent.read();
 
             // The builder could have been reset, need to re-get base state
-            if (parent.baseState != null) {
-                baseState = parent.baseState.getChildNode(name);
-            } else {
-                baseState = null;
-            }
+            baseState = parent.getBaseState(name);
 
             // ... same for the write state
-            if (parent.writeState != null) {
-                writeState = parent.writeState.nodes.get(name);
-                if (writeState == null
-                        && parent.writeState.nodes.containsKey(name)) {
-                    throw new IllegalStateException(
-                            "This node has been removed");
-                }
-            } else {
-                writeState = null;
-            }
+            writeState = parent.getWriteState(name);
 
             revision = root.revision;
         }
+
+        assert classInvariants();
+
         if (writeState != null) {
             return writeState;
-        } else if (baseState != null) {
-            return baseState;
         } else {
-            return NULL_STATE;
+            return baseState;
         }
     }
 
+    @Nonnull
     private MutableNodeState write() {
-        return write(root.revision + 1);
+        return write(root.revision + 1, false);
     }
 
-    private MutableNodeState write(long newRevision) {
+    @Nonnull
+    private MutableNodeState write(long newRevision, boolean skipRemovedCheck) {
+        // make sure that all revision numbers up to the root gets updated
+        if (!isRoot()) {
+            checkState(skipRemovedCheck || !parent.removed(name));
+            parent.write(newRevision, skipRemovedCheck);
+        }
+
         if (writeState == null || revision != root.revision) {
-            assert(parent != null); // root never gets here
-            parent.write(newRevision);
+            assert(!isRoot()); // root never gets here since revision == root.revision
 
             // The builder could have been reset, need to re-get base state
-            if (parent.baseState != null) {
-                baseState = parent.baseState.getChildNode(name);
-            } else {
-                baseState = null;
-            }
+            baseState = parent.getBaseState(name);
 
-            assert parent.writeState != null; // we just called parent.write()
-            writeState = parent.writeState.nodes.get(name);
+            writeState = parent.getWriteState(name);
             if (writeState == null) {
-                if (parent.writeState.nodes.containsKey(name)) {
-                    throw new IllegalStateException(
-                            "This node has been removed");
-                } else {
-                    // need to make this node writable
-                    NodeState base = baseState;
-                    if (base == null) {
-                        base = NULL_STATE;
-                    }
-                    writeState = new MutableNodeState(base);
-                    parent.writeState.nodes.put(name, writeState);
+                if (parent.removed(name)) {
+                    writeState = new MutableNodeState(null);
+                }
+                else {
+                    writeState = new MutableNodeState(baseState);
                 }
+                assert parent.writeState != null; // guaranteed by called parent.write()
+                parent.writeState.nodes.put(name, writeState);
             }
-        } else if (parent != null) {
-            // make sure that all revision numbers up to the root gets updated
-            parent.write(newRevision);
         }
+
         revision = newRevision;
+        assert classInvariants();
+        assert writeState != null;
         return writeState;
     }
 
@@ -249,25 +293,27 @@ public class MemoryNodeBuilder implement
 
     @Override
     public boolean isNew() {
-        return this != root && parent.isNew(name);
+        return !isRoot() && parent.isNew(name);
     }
 
     private boolean isNew(String name) {
-        return (getBaseState() == null || !getBaseState().hasChildNode(name)) &&
hasChildNode(name);
+        read();
+        return !hasBaseState(name) && hasChildNode(name);
     }
 
     @Override
     public boolean isRemoved() {
-        return this != root && (parent.isRemoved() || parent.isRemoved(name));
+        return !isRoot() && (parent.isRemoved() || parent.isRemoved(name));
     }
 
     private boolean isRemoved(String name) {
-        return getBaseState() != null && getBaseState().hasChildNode(name) &&
!hasChildNode(name);
+        read();
+        return hasBaseState(name) && !hasChildNode(name);
     }
 
     @Override
     public boolean isModified() {
-        NodeState baseState = getBaseState();
+        read();
         if (writeState == null) {
             return false;
         }
@@ -276,7 +322,7 @@ public class MemoryNodeBuilder implement
                 if (n.getValue() == null) {
                     return true;
                 }
-                if (baseState == null || !baseState.hasChildNode(n.getKey())) {
+                if (!(hasBaseState(n.getKey()))) {
                     return true;
                 }
             }
@@ -299,8 +345,7 @@ public class MemoryNodeBuilder implement
         if (writeState != null) {
             return writeState.snapshot();
         } else {
-            // FIXME this assertion might fail when getNodeState() is called on a removed
node.
-            assert baseState != null; // guaranteed by read()
+            assert baseState != null;
             return baseState;
         }
     }
@@ -313,13 +358,10 @@ public class MemoryNodeBuilder implement
 
     @Override
     public void reset(NodeState newBase) {
-        if (this == root) {
-            baseState = checkNotNull(newBase);
-            writeState = new MutableNodeState(baseState);
-            revision++;
-        } else {
-            throw new IllegalStateException("Cannot reset a non-root builder");
-        }
+        checkState(isRoot(), "Cannot reset a non-root builder");
+        baseState = checkNotNull(newBase);
+        writeState = new MutableNodeState(baseState);
+        revision++;
     }
 
     @Override
@@ -341,7 +383,7 @@ public class MemoryNodeBuilder implement
     public NodeBuilder setNode(String name, NodeState state) {
         write();
 
-        MutableNodeState childState = writeState.nodes.get(name);
+        MutableNodeState childState = getWriteState(name);
         if (childState == null) {
             writeState.nodes.remove(name);
             childState = createChildBuilder(name).write();
@@ -354,12 +396,12 @@ public class MemoryNodeBuilder implement
 
     @Override @Nonnull
     public NodeBuilder removeNode(String name) {
-        MutableNodeState mstate = write();
+        write();
 
-        if (mstate.base.getChildNode(name) != null) {
-            mstate.nodes.put(name, null);
+        if (writeState.base.getChildNode(name) != null) {
+            writeState.nodes.put(name, null);
         } else {
-            mstate.nodes.remove(name);
+            writeState.nodes.remove(name);
         }
 
         updated();
@@ -376,7 +418,6 @@ public class MemoryNodeBuilder implement
         return read().getProperties();
     }
 
-
     @Override
     public PropertyState getProperty(String name) {
         return read().getProperty(name);
@@ -384,12 +425,12 @@ public class MemoryNodeBuilder implement
 
     @Override @Nonnull
     public NodeBuilder removeProperty(String name) {
-        MutableNodeState mstate = write();
+        write();
 
-        if (mstate.base.getProperty(name) != null) {
-            mstate.properties.put(name, null);
+        if (writeState.base.getProperty(name) != null) {
+            writeState.properties.put(name, null);
         } else {
-            mstate.properties.remove(name);
+            writeState.properties.remove(name);
         }
 
         updated();
@@ -398,8 +439,8 @@ public class MemoryNodeBuilder implement
 
     @Override
     public NodeBuilder setProperty(PropertyState property) {
-        MutableNodeState mstate = write();
-        mstate.properties.put(property.getName(), property);
+        write();
+        writeState.properties.put(property.getName(), property);
         updated();
         return this;
     }
@@ -418,36 +459,13 @@ public class MemoryNodeBuilder implement
 
     @Override
     public NodeBuilder child(String name) {
-        read(); // shortcut when dealing with a read-only child node
-        if (baseState != null
-                && baseState.hasChildNode(name)
-                && (writeState == null
-                    || (writeState.base == baseState
-                        && !writeState.nodes.containsKey(name)))) {
-            return createChildBuilder(name);
-        }
-
-        // no read-only child node found, switch to write mode
-        write();
-        assert writeState != null; // guaranteed by write()
-
-        NodeState childBase = null;
-        if (baseState != null) {
-            childBase = baseState.getChildNode(name);
-        }
+        read();
+        MemoryNodeBuilder builder = createChildBuilder(name);
 
-        if (writeState.nodes.get(name) == null) {
-            if (writeState.nodes.containsKey(name)) {
-                // The child node was removed earlier and we're creating
-                // a new child with the same name. Use the null state to
-                // prevent the previous child state from re-surfacing.
-                childBase = null;
-            }
-            writeState.nodes.put(name, new MutableNodeState(childBase));
+        boolean modified = writeState != null && (writeState.base != baseState ||
writeState.nodes.containsKey(name));
+        if (!hasBaseState(name) || modified) {
+            builder.write(root.revision + 1, true);
         }
-
-        MemoryNodeBuilder builder = createChildBuilder(name);
-        builder.write();
         return builder;
     }
 
@@ -478,11 +496,20 @@ public class MemoryNodeBuilder implement
         private final Map<String, MutableNodeState> nodes =
                 Maps.newHashMap();
 
+        /**
+         * Determine whether the a node with the given name was removed
+         * @param name  name of the node
+         * @return  {@code true}  iff a node with the given name was removed
+         */
+        private boolean isRemoved(String name) {
+            return nodes.containsKey(name) && nodes.get(name) == null;
+        }
+
         public MutableNodeState(NodeState base) {
             if (base != null) {
                 this.base = base;
             } else {
-                this.base = MemoryNodeBuilder.NULL_STATE;
+                this.base = MemoryNodeState.EMPTY_NODE;
             }
         }
 

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilderTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilderTest.java?rev=1411619&r1=1411618&r2=1411619&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilderTest.java
(original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilderTest.java
Tue Nov 20 11:20:30 2012
@@ -22,7 +22,6 @@ import com.google.common.collect.Immutab
 import org.apache.jackrabbit.oak.api.PropertyState;
 import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
 import org.apache.jackrabbit.oak.spi.state.NodeState;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import static junit.framework.Assert.assertEquals;
@@ -103,7 +102,7 @@ public class MemoryNodeBuilderTest {
     }
 
     @Test
-    public void testConnectOnRemoveNode() {
+    public void testReadOnRemoveNode() {
         NodeBuilder root = new MemoryNodeBuilder(BASE);
         NodeBuilder child = root.child("x");
 
@@ -120,6 +119,23 @@ public class MemoryNodeBuilderTest {
     }
 
     @Test
+    public void testWriteOnRemoveNode() {
+        NodeBuilder root = new MemoryNodeBuilder(BASE);
+        NodeBuilder child = root.child("x");
+
+        root.removeNode("x");
+        try {
+            child.setProperty("q", "w");
+            fail();
+        } catch (IllegalStateException e) {
+            // expected
+        }
+
+        root.child("x");
+        assertEquals(0, child.getChildNodeCount()); // reconnect!
+    }
+
+    @Test
     public void testAddRemovedNodeAgain() {
         NodeBuilder root = new MemoryNodeBuilder(BASE);
 
@@ -148,7 +164,7 @@ public class MemoryNodeBuilderTest {
     public void testReset2() {
         NodeBuilder root = new MemoryNodeBuilder(BASE);
         NodeBuilder x = root.child("x");
-        NodeBuilder y = x.child("y");
+        x.child("y");
 
         root.reset(BASE);
         assertTrue(root.hasChildNode("x"));



Mime
View raw message