From 02e9680de960c0d8428f549ee9e1c7c45459b539 Mon Sep 17 00:00:00 2001 From: Jukka Zitting Date: Thu, 21 Mar 2013 12:59:00 +0200 Subject: [PATCH] OAK-709: Consider moving permission evaluation to the node state level Rought draft for making NodeState.getChildNode() @Nonnull --- .../apache/jackrabbit/oak/core/ImmutableTree.java | 2 +- .../apache/jackrabbit/oak/core/ReadOnlyTree.java | 2 +- .../org/apache/jackrabbit/oak/core/TreeImpl.java | 13 ++-- .../jackrabbit/oak/kernel/KernelNodeState.java | 13 +++- .../oak/kernel/KernelNodeStoreBranch.java | 14 ++-- .../jackrabbit/oak/plugins/index/IndexUtils.java | 2 +- .../oak/plugins/index/p2/Property2IndexLookup.java | 2 +- .../p2/strategy/ContentMirrorStoreStrategy.java | 4 +- .../index/property/PropertyIndexLookup.java | 12 ++-- .../oak/plugins/memory/EmptyNodeState.java | 34 ++++++--- .../oak/plugins/memory/MemoryNodeBuilder.java | 55 +++++++------- .../oak/plugins/memory/MemoryNodeState.java | 24 ++++++- .../oak/plugins/memory/ModifiedNodeState.java | 13 +++- .../oak/plugins/segment/SegmentNodeState.java | 12 +++- .../jackrabbit/oak/plugins/segment/Template.java | 7 +- .../oak/spi/state/AbstractNodeState.java | 2 +- .../apache/jackrabbit/oak/spi/state/NodeState.java | 84 +++++++++++++++------- .../jackrabbit/oak/spi/state/ReadOnlyBuilder.java | 2 +- .../jackrabbit/oak/kernel/KernelNodeStateTest.java | 10 +-- .../jackrabbit/oak/kernel/KernelNodeStoreTest.java | 33 ++++----- .../oak/kernel/LargeKernelNodeStateTest.java | 10 +-- .../plugins/index/IndexHookManagerDiffTest.java | 2 +- .../oak/plugins/index/IndexHookManagerTest.java | 2 +- .../oak/plugins/index/lucene/LuceneIndex.java | 5 +- 24 files changed, 223 insertions(+), 136 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ImmutableTree.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ImmutableTree.java index 35fa7a0..fd382c2 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ImmutableTree.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ImmutableTree.java @@ -117,7 +117,7 @@ public final class ImmutableTree extends ReadOnlyTree { @Override public ImmutableTree getChild(@Nonnull String name) { NodeState child = getNodeState().getChildNode(name); - if (child != null) { + if (child.exists()) { return new ImmutableTree(this, name, child); } else { return null; diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ReadOnlyTree.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ReadOnlyTree.java index da856bd..4f80544 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ReadOnlyTree.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/ReadOnlyTree.java @@ -131,7 +131,7 @@ public class ReadOnlyTree implements Tree { @Override public ReadOnlyTree getChild(@Nonnull String name) { NodeState child = state.getChildNode(name); - if (child != null) { + if (child.exists()) { return new ReadOnlyTree(this, name, child); } else { return null; diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java index 4055073..79d34d5 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/TreeImpl.java @@ -64,7 +64,7 @@ public class TreeImpl implements Tree { private final RootImpl root; /** - * The node state this tree is based on. {@code null} if this is a newly added tree. + * The node state this tree is based on, possibly non-existent. */ private final NodeState baseState; @@ -100,12 +100,7 @@ public class TreeImpl implements Tree { this.name = checkNotNull(name); this.nodeBuilder = parent.getNodeBuilder().child(name); this.pendingMoves = checkNotNull(pendingMoves); - - if (parent.baseState == null) { - this.baseState = null; - } else { - this.baseState = parent.baseState.getChildNode(name); - } + this.baseState = parent.baseState.getChildNode(name); } @Override @@ -164,7 +159,7 @@ public class TreeImpl implements Tree { } NodeState parentBase = getBaseState(); - PropertyState base = parentBase == null ? null : parentBase.getProperty(name); + PropertyState base = parentBase.getProperty(name); if (head == null) { return (base == null) ? null : Status.DISCONNECTED; } else { @@ -428,7 +423,7 @@ public class TreeImpl implements Tree { return nodeBuilder; } - @CheckForNull + @Nonnull NodeState getBaseState() { return baseState; } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java index faf1ff1..e751f2b 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java @@ -59,8 +59,10 @@ import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.spi.state.NodeStateDiff; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE; +import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.MISSING_NODE; import static org.apache.jackrabbit.oak.plugins.memory.PropertyStates.createProperty; /** @@ -219,6 +221,11 @@ public final class KernelNodeState extends AbstractNodeState { } @Override + public boolean exists() { + return true; + } + + @Override public long getPropertyCount() { init(); return properties.size(); @@ -244,7 +251,7 @@ public final class KernelNodeState extends AbstractNodeState { @Override public NodeState getChildNode(String name) { - checkNotNull(name); + checkArgument(!checkNotNull(name).isEmpty()); init(); String childPath = null; if (childNames.contains(name)) { @@ -258,7 +265,7 @@ public final class KernelNodeState extends AbstractNodeState { if (state != NULL) { return state; } else { - return null; + return MISSING_NODE; } } // not able to tell from cache if node exists @@ -270,7 +277,7 @@ public final class KernelNodeState extends AbstractNodeState { } } if (childPath == null) { - return null; + return MISSING_NODE; } try { return cache.get(revision + childPath); diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java index 57fdba8..e39ad40 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreBranch.java @@ -108,7 +108,7 @@ class KernelNodeStoreBranch implements NodeStoreBranch { // parent of destination does not exist return false; } - if (destParent.getChildNode(getName(target)) != null) { + if (destParent.getChildNode(getName(target)).exists()) { // destination exists already return false; } @@ -129,7 +129,7 @@ class KernelNodeStoreBranch implements NodeStoreBranch { // parent of destination does not exist return false; } - if (destParent.getChildNode(getName(target)) != null) { + if (destParent.getChildNode(getName(target)).exists()) { // destination exists already return false; } @@ -213,12 +213,12 @@ class KernelNodeStoreBranch implements NodeStoreBranch { NodeState node = getHead(); for (String name : elements(path)) { node = node.getChildNode(name); - if (node == null) { - break; - } } - - return node; + if (node.exists()) { + return node; + } else { + return null; + } } private void commit(String jsop) { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java index 5c6a987..c0be8f8 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java @@ -118,7 +118,7 @@ public class IndexUtils implements IndexConstants { public static List buildIndexDefinitions(NodeState state, String indexConfigPath, String typeFilter) { NodeState definitions = state.getChildNode(INDEX_DEFINITIONS_NAME); - if (definitions == null) { + if (!definitions.exists()) { return Collections.emptyList(); } indexConfigPath = concat(indexConfigPath, INDEX_DEFINITIONS_NAME); diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/Property2IndexLookup.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/Property2IndexLookup.java index f849dc6..65bb56a 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/Property2IndexLookup.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/Property2IndexLookup.java @@ -121,7 +121,7 @@ public class Property2IndexLookup { @Nullable private static NodeState getIndexDataNode(NodeState node, String propertyName, Filter filter) { NodeState state = node.getChildNode(INDEX_DEFINITIONS_NAME); - if (state == null) { + if (!state.exists()) { return null; } String filterNodeType = null; diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/strategy/ContentMirrorStoreStrategy.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/strategy/ContentMirrorStoreStrategy.java index 9edfb56..0d3b259 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/strategy/ContentMirrorStoreStrategy.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/p2/strategy/ContentMirrorStoreStrategy.java @@ -149,7 +149,7 @@ public class ContentMirrorStoreStrategy implements IndexStoreStrategy { } else { for (String p : values) { NodeState property = index.getChildNode(p); - if (property != null) { + if (property.exists()) { // we have an entry for this value, so use it it.enqueue(Iterators.singletonIterator( new MemoryChildNodeEntry("", property))); @@ -181,7 +181,7 @@ public class ContentMirrorStoreStrategy implements IndexStoreStrategy { break; } NodeState s = index.getChildNode(p); - if (s != null) { + if (s.exists()) { CountingNodeVisitor v = new CountingNodeVisitor(max); v.visit(s); count += v.getEstimatedCount(); diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java index ae3e399..63ef6a9 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java @@ -114,9 +114,8 @@ public class PropertyIndexLookup { Set paths = Sets.newHashSet(); PropertyState property; - NodeState state = getIndexDefinitionNode(name); - if (state != null && state.getChildNode(":index") != null) { - state = state.getChildNode(":index"); + NodeState state = getIndexDefinitionNode(name).getChildNode(":index"); + if (state.exists()) { for (String p : PropertyIndex.encode(value)) { property = state.getProperty(p); if (property != null) { @@ -166,9 +165,8 @@ public class PropertyIndexLookup { public double getCost(String name, PropertyValue value) { double cost = 0.0; - NodeState state = getIndexDefinitionNode(name); - if (state != null && state.getChildNode(":index") != null) { - state = state.getChildNode(":index"); + NodeState state = getIndexDefinitionNode(name).getChildNode(":index"); + if (state.exists()) { for (String p : PropertyIndex.encode(value)) { PropertyState property = state.getProperty(p); if (property != null) { @@ -191,7 +189,7 @@ public class PropertyIndexLookup { @Nullable private NodeState getIndexDefinitionNode(String name) { NodeState state = root.getChildNode(INDEX_DEFINITIONS_NAME); - if (state != null) { + if (state.exists()) { for (ChildNodeEntry entry : state.getChildNodeEntries()) { PropertyState type = entry.getNodeState().getProperty(IndexConstants.TYPE_PROPERTY_NAME); if(type == null || type.isArray() || !PropertyIndex.TYPE.equals(type.getValue(Type.STRING))){ diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/EmptyNodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/EmptyNodeState.java index d4776a0..dbd59c8 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/EmptyNodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/EmptyNodeState.java @@ -16,6 +16,9 @@ */ package org.apache.jackrabbit.oak.plugins.memory; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; + import java.util.Collections; import javax.annotation.CheckForNull; @@ -28,14 +31,24 @@ import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.spi.state.NodeStateDiff; /** - * Singleton instance of an empty node state, i.e. one with neither - * properties nor child nodes. + * Singleton instances of empty and non-existent node states, i.e. ones + * with neither properties nor child nodes. */ public final class EmptyNodeState implements NodeState { - public static final NodeState EMPTY_NODE = new EmptyNodeState(); + public static final NodeState EMPTY_NODE = new EmptyNodeState(true); + + public static final NodeState MISSING_NODE = new EmptyNodeState(false); + + private final boolean exists; + + private EmptyNodeState(boolean exists) { + this.exists = exists; + } - private EmptyNodeState() { + @Override + public boolean exists() { + return exists; } @Override @@ -60,12 +73,14 @@ public final class EmptyNodeState implements NodeState { @Override public boolean hasChildNode(@Nonnull String name) { + checkArgument(!checkNotNull(name).isEmpty()); return false; } - @Override @CheckForNull + @Override @Nonnull public NodeState getChildNode(@Nonnull String name) { - return null; + checkArgument(!checkNotNull(name).isEmpty()); + return MISSING_NODE; } @Override @@ -85,7 +100,7 @@ public final class EmptyNodeState implements NodeState { @Override public void compareAgainstBaseState(NodeState base, NodeStateDiff diff) { - if (base != EMPTY_NODE) { + if (base != EMPTY_NODE && base.exists()) { for (PropertyState before : base.getProperties()) { diff.propertyDeleted(before); } @@ -97,7 +112,7 @@ public final class EmptyNodeState implements NodeState { public static void compareAgainstEmptyState( NodeState state, NodeStateDiff diff) { - if (state != EMPTY_NODE) { + if (state != EMPTY_NODE && state.exists()) { for (PropertyState after : state.getProperties()) { diff.propertyAdded(after); } @@ -114,7 +129,7 @@ public final class EmptyNodeState implements NodeState { } public boolean equals(Object object) { - if (object == EMPTY_NODE) { + if (object == EMPTY_NODE || object == MISSING_NODE) { return true; } else if (object instanceof NodeState) { NodeState that = (NodeState) object; @@ -128,4 +143,5 @@ public final class EmptyNodeState implements NodeState { public int hashCode() { return 0; } + } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java index 58ba6c2..22273a1 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java @@ -31,6 +31,7 @@ import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.spi.state.NodeStateDiff; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE; @@ -96,7 +97,7 @@ public class MemoryNodeBuilder implements NodeBuilder { private long revision; /** - * The base state of this builder, or {@code null} if this builder + * The base state of this builder, possibly non-existent if this builder * represents a new node that didn't yet exist in the base content tree. */ private NodeState baseState; @@ -120,7 +121,7 @@ public class MemoryNodeBuilder implements NodeBuilder { this.root = parent.root; this.revision = -1; - this.baseState = null; + this.baseState = parent.baseState.getChildNode(name); this.writeState = null; } @@ -143,7 +144,7 @@ public class MemoryNodeBuilder implements NodeBuilder { private boolean classInvariants() { boolean rootHasNoParent = isRoot() == (parent == null); boolean rootHasWriteState = root.writeState != null; - boolean baseStateOrWriteStateNotNull = baseState != null || writeState != null; + boolean baseStateOrWriteStateNotNull = baseState.exists() || writeState != null; return rootHasNoParent && rootHasWriteState && baseStateOrWriteStateNotNull; } @@ -158,11 +159,7 @@ public class MemoryNodeBuilder implements NodeBuilder { * @return base state of the child */ private NodeState getBaseState(String name) { - if (baseState != null) { - return baseState.getChildNode(name); - } else { - return null; - } + return baseState.getChildNode(name); } /** @@ -172,7 +169,7 @@ public class MemoryNodeBuilder implements NodeBuilder { * @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 baseState.hasChildNode(name); } /** @@ -196,11 +193,9 @@ public class MemoryNodeBuilder implements NodeBuilder { private boolean exists() { if (isRoot()) { return true; - } - else if (parent.writeState == null) { - return parent.baseState != null && parent.baseState.hasChildNode(name); - } - else { + } else if (parent.writeState == null) { + return parent.baseState.hasChildNode(name); + } else { return parent.writeState.hasChildNode(name); } } @@ -226,7 +221,7 @@ public class MemoryNodeBuilder implements NodeBuilder { return false; } - return writeState != null || baseState != null; + return writeState != null || baseState.exists(); } @Nonnull @@ -258,7 +253,7 @@ public class MemoryNodeBuilder implements NodeBuilder { writeState = parent.getWriteState(name); if (writeState == null) { if (exists()) { - assert baseState != null; + assert baseState.exists(); writeState = new MutableNodeState(baseState); } else { @@ -340,7 +335,8 @@ public class MemoryNodeBuilder implements NodeBuilder { if (pState == null) { return true; } - if (baseState == null || !pState.equals(baseState.getProperty(p.getKey()))) { + if (!baseState.exists() + || !pState.equals(baseState.getProperty(p.getKey()))) { return true; } } @@ -354,7 +350,7 @@ public class MemoryNodeBuilder implements NodeBuilder { if (writeState != null) { return writeState.snapshot(); } else { - assert baseState != null; + assert baseState.exists(); return baseState; } } @@ -505,15 +501,6 @@ public class MemoryNodeBuilder implements NodeBuilder { private final Map 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; @@ -563,6 +550,11 @@ public class MemoryNodeBuilder implements NodeBuilder { //-----------------------------------------------------< NodeState >-- @Override + public boolean exists() { + return true; + } + + @Override public long getPropertyCount() { return withProperties(base, properties).getPropertyCount(); } @@ -586,13 +578,20 @@ public class MemoryNodeBuilder implements NodeBuilder { @Override public boolean hasChildNode(String name) { checkNotNull(name); + checkArgument(!name.isEmpty()); return withNodes(base, nodes).hasChildNode(name); } @Override public NodeState getChildNode(String name) { checkNotNull(name); - return withNodes(base, nodes).getChildNode(name); // mutable + checkArgument(!name.isEmpty()); + NodeState state = withNodes(base, nodes).getChildNode(name); // mutable + if (state != null) { + return state; + } else { + return EmptyNodeState.MISSING_NODE; + } } @Override @Nonnull diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeState.java index 780ed92..1ddc321 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeState.java @@ -16,7 +16,10 @@ */ package org.apache.jackrabbit.oak.plugins.memory; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE; +import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.MISSING_NODE; import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.spi.state.AbstractNodeState; @@ -56,6 +59,11 @@ class MemoryNodeState extends AbstractNodeState { } @Override + public boolean exists() { + return true; + } + + @Override public PropertyState getProperty(String name) { return properties.get(name); } @@ -72,14 +80,19 @@ class MemoryNodeState extends AbstractNodeState { @Override public boolean hasChildNode(String name) { - checkNotNull(name); + checkArgument(!checkNotNull(name).isEmpty()); return nodes.containsKey(name); } @Override public NodeState getChildNode(String name) { - checkNotNull(name); - return nodes.get(name); + checkArgument(!checkNotNull(name).isEmpty()); + NodeState state = nodes.get(name); + if (state != null) { + return state; + } else { + return MISSING_NODE; + } } @Override @@ -104,6 +117,11 @@ class MemoryNodeState extends AbstractNodeState { */ @Override public void compareAgainstBaseState(NodeState base, NodeStateDiff diff) { + if (base == EMPTY_NODE || !base.exists()) { + EmptyNodeState.compareAgainstEmptyState(this, diff); + return; + } + Map newProperties = new HashMap(properties); for (PropertyState before : base.getProperties()) { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/ModifiedNodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/ModifiedNodeState.java index 7c8ae46..22021f6 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/ModifiedNodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/ModifiedNodeState.java @@ -16,6 +16,7 @@ */ package org.apache.jackrabbit.oak.plugins.memory; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Predicates.in; import static com.google.common.base.Predicates.not; @@ -24,6 +25,7 @@ import static com.google.common.collect.Collections2.filter; import static com.google.common.collect.Iterables.concat; import static com.google.common.collect.Iterables.filter; import static com.google.common.collect.Maps.filterValues; +import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.MISSING_NODE; import static org.apache.jackrabbit.oak.plugins.memory.MemoryChildNodeEntry.iterable; import java.util.Map; @@ -142,6 +144,11 @@ public class ModifiedNodeState extends AbstractNodeState { } @Override + public boolean exists() { + return true; + } + + @Override public long getPropertyCount() { long count = base.getPropertyCount(); @@ -204,7 +211,7 @@ public class ModifiedNodeState extends AbstractNodeState { @Override public boolean hasChildNode(String name) { - checkNotNull(name); + checkArgument(!checkNotNull(name).isEmpty()); NodeState child = nodes.get(name); if (child != null) { return true; @@ -217,12 +224,12 @@ public class ModifiedNodeState extends AbstractNodeState { @Override public NodeState getChildNode(String name) { - checkNotNull(name); + checkArgument(!checkNotNull(name).isEmpty()); NodeState child = nodes.get(name); if (child != null) { return child; } else if (nodes.containsKey(name)) { - return null; // removed + return MISSING_NODE; // removed } else { return base.getChildNode(name); } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeState.java index 033e35e..df0bc4c 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentNodeState.java @@ -16,6 +16,7 @@ */ package org.apache.jackrabbit.oak.plugins.segment; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE; @@ -69,6 +70,11 @@ class SegmentNodeState extends AbstractNodeState { } @Override + public boolean exists() { + return true; + } + + @Override public long getPropertyCount() { return getTemplate().getPropertyCount(); } @@ -91,12 +97,14 @@ class SegmentNodeState extends AbstractNodeState { @Override public boolean hasChildNode(String name) { - return getTemplate().hasChildNode(checkNotNull(name), store, recordId); + checkArgument(!checkNotNull(name).isEmpty()); + return getTemplate().hasChildNode(name, store, recordId); } @Override @CheckForNull public NodeState getChildNode(String name) { - return getTemplate().getChildNode(checkNotNull(name), store, recordId); + checkArgument(!checkNotNull(name).isEmpty()); + return getTemplate().getChildNode(name, store, recordId); } @Override diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/Template.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/Template.java index 10fc6d1..795da63 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/Template.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/Template.java @@ -19,6 +19,7 @@ package org.apache.jackrabbit.oak.plugins.segment; import static com.google.common.base.Preconditions.checkElementIndex; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.MISSING_NODE; import static org.apache.jackrabbit.oak.plugins.segment.Segment.RECORD_ID_BYTES; import java.util.Arrays; @@ -274,14 +275,14 @@ class Template { public NodeState getChildNode( String name, SegmentStore store, RecordId recordId) { if (hasNoChildNodes()) { - return null; + return MISSING_NODE; } else if (hasManyChildNodes()) { MapRecord map = getChildNodeMap(store, recordId); RecordId childNodeId = map.getEntry(name); if (childNodeId != null) { return new SegmentNodeState(store, childNodeId); } else { - return null; + return MISSING_NODE; } } else if (name.equals(childName)) { int offset = recordId.getOffset() + RECORD_ID_BYTES; @@ -289,7 +290,7 @@ class Template { RecordId childNodeId = segment.readRecordId(offset); return new SegmentNodeState(store, childNodeId); } else { - return null; + return MISSING_NODE; } } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/AbstractNodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/AbstractNodeState.java index 861bac9..6317838 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/AbstractNodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/AbstractNodeState.java @@ -60,7 +60,7 @@ public abstract class AbstractNodeState implements NodeState { @Override public boolean hasChildNode(String name) { - return getChildNode(name) != null; + return getChildNode(name).exists(); } @Override diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeState.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeState.java index 375ac00..15e2710 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeState.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/NodeState.java @@ -64,11 +64,36 @@ import org.apache.jackrabbit.oak.api.PropertyState; * as {@link RuntimeException unchecked exceptions} that higher level code * is not expected to be able to recover from. *

- * Since this interface exposes no higher level constructs like access - * controls, locking, node types or even path parsing, there's no way - * for content access to fail because of such concerns. Such functionality - * and related checked exceptions or other control flow constructs should - * be implemented on a higher level above this interface. + * Since this interface exposes no higher level constructs like locking, + * node types or even path parsing, there's no way for content access to + * fail because of such concerns. Such functionality and related checked + * exceptions or other control flow constructs should be implemented on + * a higher level above this interface. On the other hand read access + * controls can be implemented below this interface, in which + * case some content that would otherwise be accessible might not show + * up through such an implementation. + * + *

Existence and iterability of node states

+ *

+ * The {@link #getChildNode(String)} method is special in that it + * never returns a {@code null} value, even if the named child + * node does not exist. Instead a client should use the {@link #exists()} + * method to check whether a particular node exists. The purpose of this + * separation of concerns is to allow an implementation to lazily load + * content only when it's actually read instead of just traversed. It also + * simplifies client code by reducing the number of required {@code null} + * checks. All iterable child nodes, i.e. ones that are returned + * from or implied by methods like {@link #getChildNodeEntries()}, are + * guaranteed to exist. + *

+ * The other methods of a non-existent node state behave as if the node + * was empty. In other words, a non-existent node contains no properties + * and no iterable child nodes. It is even possible to construct a builder + * from or do comparisons with a non-existent node state. + *

+ * As a special case it is possible for a node not to "exist" even if one + * or more of it's descendants do exist. Such scenarios are typically the + * result of access control restrictions. * *

Decoration and virtual content

*

@@ -77,7 +102,7 @@ import org.apache.jackrabbit.oak.api.PropertyState; * like for example the aggregate size of the entire subtree as an * extra virtual property. A virtualization, sharding or caching layer * could provide a composite view over multiple underlying trees. - * Or a basic access control layer could decide to hide certain content + * Or a an access control layer could decide to hide certain content * based on specific rules. All such features need to be implemented * according to the API contract of this interface. A separate higher level * interface needs to be used if an implementation can't for example @@ -86,7 +111,7 @@ import org.apache.jackrabbit.oak.api.PropertyState; *

Equality and hash codes

*

* Two node states are considered equal if and only if their properties and - * child nodes match, regardless of ordering. The + * iterable child nodes match, regardless of ordering. The * {@link Object#equals(Object)} method needs to be implemented so that it * complies with this definition. And while node states are not meant for * use as hash keys, the {@link Object#hashCode()} method should still be @@ -95,13 +120,21 @@ import org.apache.jackrabbit.oak.api.PropertyState; public interface NodeState { /** + * Checks whether this node exists. See the above discussion about + * the existence of node states. + * + * @return {@code true} if this node exists, {@code false} if not + */ + boolean exists(); + + /** * Returns the named property. The name is an opaque string and * is not parsed or otherwise interpreted by this method. *

* The namespace of properties and child nodes is shared, so if * this method returns a non-{@code null} value for a given * name, then {@link #getChildNode(String)} is guaranteed to return - * {@code null} for the same name. + * a non-existing {@link NodeState} for the same name. * * @param name name of the property to return * @return named property, or {@code null} if not found @@ -128,7 +161,8 @@ public interface NodeState { Iterable getProperties(); /** - * Checks whether the named child node exists. + * Checks whether the named child node exists. The implementation + * is equivalent to {@code getChildNode(name).exists()}. * * @param name name of the child node * @return {@code true} if the named child node exists, @@ -137,40 +171,42 @@ public interface NodeState { boolean hasChildNode(@Nonnull String name); /** - * Returns the named child node. The name is an opaque string and - * is not parsed or otherwise interpreted by this method. + * Returns the named, possibly non-existent, child node. The name is an + * opaque string and is not parsed or otherwise interpreted by this method. + * Use the {@link #exists()} method on the returned child node to + * determine whether the node exists or not. *

* The namespace of properties and child nodes is shared, so if - * this method returns a non-{@code null} value for a given - * name, then {@link #getProperty(String)} is guaranteed to return - * {@code null} for the same name. + * this method returns an existing {@link NodeState} for + * a given name, then {@link #getProperty(String)} is guaranteed + * to return {@code null} for the same name. * * @param name name of the child node to return - * @return named child node, or {@code null} if not found + * @return named child node */ - @CheckForNull + @Nonnull NodeState getChildNode(@Nonnull String name); /** - * Returns the number of child nodes of this node. + * Returns the number of iterable child nodes of this node. * - * @return number of child nodes + * @return number of iterable child nodes */ long getChildNodeCount(); /** - * Returns the names of all child nodes. + * Returns the names of all iterable child nodes. * * @return child node names in some stable order */ Iterable getChildNodeNames(); /** - * Returns an iterable of the child node entries of this instance. Multiple - * iterations are guaranteed to return the child nodes in the same order, - * but the specific order used is implementation dependent and may change - * across different states of the same node. - *

+ * Returns the iterable child node entries of this instance. + * Multiple iterations are guaranteed to return the child nodes in + * the same order, but the specific order used is implementation + * dependent and may change across different states of the same node. + *

* Note on cost and performance: while it is possible to iterate over * all child {@code NodeState}s with the two methods {@link * #getChildNodeNames()} and {@link #getChildNode(String)}, this method is diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/ReadOnlyBuilder.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/ReadOnlyBuilder.java index 493f1ee..e2e0f09 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/ReadOnlyBuilder.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/spi/state/ReadOnlyBuilder.java @@ -130,7 +130,7 @@ public class ReadOnlyBuilder implements NodeBuilder { @Override public ReadOnlyBuilder child(String name) { NodeState child = state.getChildNode(name); - if (child != null) { + if (child.exists()) { return new ReadOnlyBuilder(child); } else { throw unsupported(); diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStateTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStateTest.java index 3ac8d29..f3d3d24 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStateTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStateTest.java @@ -37,8 +37,10 @@ import org.junit.Before; import org.junit.Test; import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertNotNull; import static junit.framework.Assert.assertNull; +import static junit.framework.Assert.assertTrue; import static org.apache.jackrabbit.oak.api.Type.LONG; public class KernelNodeStateTest { @@ -104,10 +106,10 @@ public class KernelNodeStateTest { @Test public void testGetChildNode() { - assertNotNull(state.getChildNode("x")); - assertNotNull(state.getChildNode("y")); - assertNotNull(state.getChildNode("z")); - assertNull(state.getChildNode("a")); + assertTrue(state.getChildNode("x").exists()); + assertTrue(state.getChildNode("y").exists()); + assertTrue(state.getChildNode("z").exists()); + assertFalse(state.getChildNode("a").exists()); } @Test diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreTest.java index 1a96f33..8583d99 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/KernelNodeStoreTest.java @@ -37,6 +37,7 @@ import org.apache.jackrabbit.oak.spi.state.NodeStoreBranch; import org.junit.Before; import org.junit.Test; +import static junit.framework.Assert.assertFalse; import static org.apache.jackrabbit.oak.api.Type.LONG; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -88,34 +89,34 @@ public class KernelNodeStoreTest { // Assert changes are present in the builder NodeState testState = rootBuilder.getNodeState().getChildNode("test"); - assertNotNull(testState.getChildNode("newNode")); - assertNull(testState.getChildNode("x")); + assertTrue(testState.getChildNode("newNode").exists()); + assertFalse(testState.getChildNode("x").exists()); assertEquals(42, (long) testState.getChildNode("newNode").getProperty("n").getValue(LONG)); // Assert changes are not yet present in the branch testState = branch.getHead().getChildNode("test"); - assertNull(testState.getChildNode("newNode")); - assertNotNull(testState.getChildNode("x")); + assertFalse(testState.getChildNode("newNode").exists()); + assertTrue(testState.getChildNode("x").exists()); branch.setRoot(rootBuilder.getNodeState()); // Assert changes are present in the branch testState = branch.getHead().getChildNode("test"); - assertNotNull(testState.getChildNode("newNode")); - assertNull(testState.getChildNode("x")); + assertTrue(testState.getChildNode("newNode").exists()); + assertFalse(testState.getChildNode("x").exists()); assertEquals(42, (long) testState.getChildNode("newNode").getProperty("n").getValue(LONG)); // Assert changes are not yet present in the trunk testState = store.getRoot().getChildNode("test"); - assertNull(testState.getChildNode("newNode")); - assertNotNull(testState.getChildNode("x")); + assertFalse(testState.getChildNode("newNode").exists()); + assertTrue(testState.getChildNode("x").exists()); branch.merge(EmptyHook.INSTANCE); // Assert changes are present in the trunk testState = store.getRoot().getChildNode("test"); - assertNotNull(testState.getChildNode("newNode")); - assertNull(testState.getChildNode("x")); + assertTrue(testState.getChildNode("newNode").exists()); + assertFalse(testState.getChildNode("x").exists()); assertEquals(42, (long) testState.getChildNode("newNode").getProperty("n").getValue(LONG)); } @@ -151,9 +152,9 @@ public class KernelNodeStoreTest { assertNotNull(before); assertNotNull(after); - assertNull(before.getChildNode("test").getChildNode("newNode")); - assertNotNull(after.getChildNode("test").getChildNode("newNode")); - assertNull(after.getChildNode("test").getChildNode("a")); + assertFalse(before.getChildNode("test").getChildNode("newNode").exists()); + assertTrue(after.getChildNode("test").getChildNode("newNode").exists()); + assertFalse(after.getChildNode("test").getChildNode("a").exists()); assertEquals(42, (long) after.getChildNode("test").getChildNode("newNode").getProperty("n").getValue(LONG)); assertEquals(newRoot, after); } @@ -184,9 +185,9 @@ public class KernelNodeStoreTest { }); NodeState test = store.getRoot().getChildNode("test"); - assertNotNull(test.getChildNode("newNode")); - assertNotNull(test.getChildNode("fromHook")); - assertNull(test.getChildNode("a")); + assertTrue(test.getChildNode("newNode").exists()); + assertTrue(test.getChildNode("fromHook").exists()); + assertFalse(test.getChildNode("a").exists()); assertEquals(42, (long) test.getChildNode("newNode").getProperty("n").getValue(LONG)); assertEquals(test, store.getRoot().getChildNode("test")); } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/LargeKernelNodeStateTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/LargeKernelNodeStateTest.java index 0f786a4..9adf4d1 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/LargeKernelNodeStateTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/kernel/LargeKernelNodeStateTest.java @@ -31,8 +31,10 @@ import org.junit.Before; import org.junit.Test; import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertNotNull; import static junit.framework.Assert.assertNull; +import static junit.framework.Assert.assertTrue; public class LargeKernelNodeStateTest { @@ -67,10 +69,10 @@ public class LargeKernelNodeStateTest { @Test public void testGetChildNode() { - assertNotNull(state.getChildNode("x0")); - assertNotNull(state.getChildNode("x1")); - assertNotNull(state.getChildNode("x" + N)); - assertNull(state.getChildNode("x" + (N + 1))); + assertTrue(state.getChildNode("x0").exists()); + assertTrue(state.getChildNode("x1").exists()); + assertTrue(state.getChildNode("x" + N).exists()); + assertFalse(state.getChildNode("x" + (N + 1)).exists()); } @Test diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerDiffTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerDiffTest.java index 117ac47..d4a67d3 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerDiffTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerDiffTest.java @@ -112,8 +112,8 @@ public class IndexHookManagerDiffTest { private static NodeState checkPathExists(NodeState state, String... verify) { NodeState c = state; for (String p : verify) { - assertTrue(c.hasChildNode(p)); c = c.getChildNode(p); + assertTrue(c.exists()); } return c; } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerTest.java index 63694e8..3c93bd7 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexHookManagerTest.java @@ -231,8 +231,8 @@ public class IndexHookManagerTest { private static NodeState checkPathExists(NodeState state, String... verify) { NodeState c = state; for (String p : verify) { - assertTrue(c.hasChildNode(p)); c = c.getChildNode(p); + assertTrue(c.exists()); } return c; } diff --git a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java index 861f301..b75fbc2 100644 --- a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java +++ b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java @@ -322,12 +322,9 @@ public class LuceneIndex implements QueryIndex, LuceneIndexConstants { return; // shortcut } NodeState system = root.getChildNode(NodeTypeConstants.JCR_SYSTEM); - if (system == null) { - return; - } final NodeState types = system.getChildNode(NodeTypeConstants.JCR_NODE_TYPES); - if (types == null) { + if (!types.exists()) { return; } -- 1.8.0.msysgit.0