Permalink
Browse files

MODE-1552 Corrected how Node.save() works with descendant shared nodes

The Node.save() method is deprecated, but the behavior was incorrect when there were
shared nodes below the node being saved and where the shared nodes were in the same
shared set as nodes that were not descendants. This behavior was corrected while keeping
Node.save() relatively efficient, although it's still not as efficient as a regular
Session.save().

All TCK unit tests pass with these fixes!
  • Loading branch information...
rhauch committed Sep 7, 2012
1 parent 87045ce commit 105e5f83248f2093d01fb5394e51fc7e50cab7b1
@@ -0,0 +1,60 @@
+/*
+ * ModeShape (http://www.modeshape.org)
+ * See the COPYRIGHT.txt file distributed with this work for information
+ * regarding copyright ownership. Some portions may be licensed
+ * to Red Hat, Inc. under one or more contributor license agreements.
+ * See the AUTHORS.txt file in the distribution for a full listing of
+ * individual contributors.
+ *
+ * ModeShape is free software. Unless otherwise indicated, all code in ModeShape
+ * is licensed to you under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * ModeShape is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.modeshape.common.collection;
+
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import org.modeshape.common.annotation.NotThreadSafe;
+
+/**
+ * An {@link Iterator} that is used to iterate over a single, fixed value.
+ *
+ * @param <T> the value type
+ */
+@NotThreadSafe
+public class SingleIterator<T> implements Iterator<T> {
+ private T value;
+
+ public SingleIterator( T value ) {
+ this.value = value;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return value != null;
+ }
+
+ @Override
+ public T next() {
+ if (value == null) throw new NoSuchElementException();
+ T next = value;
+ value = null;
+ return next;
+ }
+
+ @Override
+ public void remove() {
+ throw new UnsupportedOperationException();
+ }
+}
View
@@ -79,6 +79,27 @@
</dependencies>
<build>
<plugins>
+ <plugin>
+ <!--Disable JAR packaging because there aren't any sources-->
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>test-jar</id>
+ <phase>non-existant</phase>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ <!--Used so that OSGI information can be added without changing the packaging type-->
+ <configuration>
+ <skipIfEmpty>true</skipIfEmpty>
+ <archive>
+ <manifestFile>${project.build.outputDirectory}/META-INF/MANIFEST.MF</manifestFile>
+ </archive>
+ </configuration>
+ </plugin>
<plugin>
<!--Disable bundle packaging because there aren't any sources-->
<groupId>org.apache.felix</groupId>
View
@@ -231,9 +231,11 @@
<property>
<name>known.issues</name>
<value>
- <!--// TODO author=Randall Hauch date=7/10/12 description=https://issues.apache.org/jira/browse/MODE-1552-->
+ <!--
+ List each test method that is not passing because of known issues, where the method is
+ of the form "package#method". For example:
org.apache.jackrabbit.test.api.ShareableNodeTest#testModifyDescendantAndSave
- org.apache.jackrabbit.test.api.ShareableNodeTest#testModifyDescendantAndRemoveShareAndSave
+ -->
</value>
</property>
</systemProperties>
@@ -40,6 +40,7 @@
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Pattern;
import javax.jcr.AccessDeniedException;
import javax.jcr.Binary;
@@ -3177,7 +3178,12 @@ public void accept( ItemVisitor visitor ) throws RepositoryException {
public void save()
throws AccessDeniedException, ItemExistsException, ConstraintViolationException, InvalidItemStateException,
ReferentialIntegrityException, VersionException, LockException, NoSuchNodeTypeException, RepositoryException {
- session.save(this);
+ if (this.isRoot()) {
+ // Just call save on the session (it's more efficient) ...
+ session.save();
+ } else {
+ session.save(this);
+ }
}
@Override
@@ -3301,15 +3307,19 @@ public String toString() {
* Determines whether this node, or any nodes below it, contain changes that depend on nodes that are outside of this node's
* hierarchy.
*
+ * @param affectedNodeKeys the reference that should be assigned to the set of node keys that are at or below this node; may
+ * be null if not needed
* @return true if this node's hierarchy has nodes with changes dependent on nodes from outside the hierarchy
* @throws InvalidItemStateException
* @throws ItemNotFoundException
* @throws RepositoryException
*/
- protected boolean containsChangesWithExternalDependencies() throws RepositoryException {
+ protected boolean containsChangesWithExternalDependencies( AtomicReference<Set<NodeKey>> affectedNodeKeys )
+ throws RepositoryException {
Set<NodeKey> allChanges = sessionCache().getChangedNodeKeys();
Set<NodeKey> changesAtOrBelowThis = sessionCache().getChangedNodeKeysAtOrBelow(this.node());
removeReferrerChanges(allChanges, changesAtOrBelowThis);
+ if (affectedNodeKeys != null) affectedNodeKeys.set(changesAtOrBelowThis);
return !changesAtOrBelowThis.containsAll(allChanges);
}
@@ -947,36 +947,43 @@ public void save()
*/
void save( AbstractJcrNode node ) throws RepositoryException {
// first check the node is valid from a cache perspective
- CachedNode cachedNode = null;
+ Set<NodeKey> keysToBeSaved = null;
try {
- cachedNode = node.node();
if (node.isNew()) {
// expected by TCK
throw new RepositoryException(JcrI18n.unableToSaveNodeThatWasCreatedSincePreviousSave.text(node.getPath(),
workspaceName()));
}
- if (node.containsChangesWithExternalDependencies()) {
+ AtomicReference<Set<NodeKey>> refToKeys = new AtomicReference<Set<NodeKey>>();
+ if (node.containsChangesWithExternalDependencies(refToKeys)) {
// expected by TCK
I18n msg = JcrI18n.unableToSaveBranchBecauseChangesDependOnChangesToNodesOutsideOfBranch;
throw new ConstraintViolationException(msg.text(node.path(), workspaceName()));
}
+ keysToBeSaved = refToKeys.get();
} catch (ItemNotFoundException e) {
throw new InvalidItemStateException(e);
} catch (NodeNotFoundException e) {
throw new InvalidItemStateException(e);
}
+ assert keysToBeSaved != null;
SessionCache sessionCache = cache();
+ if (sessionCache.getChangedNodeKeys().size() == keysToBeSaved.size()) {
+ // The node is above all the other changes, so go ahead and save the whole session ...
+ save();
+ return;
+ }
// Perform the save, using 'JcrPreSave' operations ...
SessionCache systemCache = createSystemCache(false);
SystemContent systemContent = new SystemContent(systemCache);
Map<NodeKey, NodeKey> baseVersionKeys = this.baseVersionKeys.get();
Map<NodeKey, NodeKey> originalVersionKeys = this.originalVersionKeys.get();
try {
- sessionCache.save(cachedNode, systemContent.cache(), new JcrPreSave(systemContent, baseVersionKeys,
- originalVersionKeys));
+ sessionCache.save(keysToBeSaved, systemContent.cache(), new JcrPreSave(systemContent, baseVersionKeys,
+ originalVersionKeys));
} catch (WrappedException e) {
Throwable cause = e.getCause();
throw (cause instanceof RepositoryException) ? (RepositoryException)cause : new RepositoryException(e.getCause());
@@ -0,0 +1,115 @@
+/*
+ * ModeShape (http://www.modeshape.org)
+ * See the COPYRIGHT.txt file distributed with this work for information
+ * regarding copyright ownership. Some portions may be licensed
+ * to Red Hat, Inc. under one or more contributor license agreements.
+ * See the AUTHORS.txt file in the distribution for a full listing of
+ * individual contributors.
+ *
+ * ModeShape is free software. Unless otherwise indicated, all code in ModeShape
+ * is licensed to you under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * ModeShape is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.modeshape.jcr.cache;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.modeshape.jcr.ExecutionContext;
+import org.modeshape.jcr.value.Path;
+import org.modeshape.jcr.value.Path.Segment;
+import org.modeshape.jcr.value.PathFactory;
+
+/**
+ * A simple cache of all valid paths for a given node, where each node may have 1 or more valid paths due to
+ * {@link CachedNode#getAdditionalParentKeys(NodeCache) additional parents}.
+ */
+public class AllPathsCache {
+ protected final NodeCache cache;
+ protected final NodeCache removedCache;
+ private final Map<NodeKey, List<Path>> paths = new HashMap<NodeKey, List<Path>>();
+ protected final PathFactory pathFactory;
+
+ public AllPathsCache( NodeCache cache,
+ NodeCache removedCache,
+ ExecutionContext context ) {
+ this.cache = cache;
+ this.removedCache = removedCache;
+ this.pathFactory = context.getValueFactories().getPathFactory();
+ }
+
+ public NodeCache getCache() {
+ return cache;
+ }
+
+ /**
+ * Get all of the paths through which the specified node is accessible, including all paths based upon the node's
+ * {@link CachedNode#getParentKey(NodeCache) parent} (which can potentially have multiple paths) and upon the node's
+ * {@link CachedNode#getAdditionalParentKeys(NodeCache) additional parents} (which each can potentially have multiple paths).
+ *
+ * @param node the node for which the paths are to be returned; may not be null
+ * @return the paths for the node; never null but possibly empty
+ */
+ public Iterable<Path> getPaths( CachedNode node ) {
+ NodeKey key = node.getKey();
+ List<Path> pathList = paths.get(key);
+ if (pathList == null) {
+ // Compute the node's path ...
+ Segment nodeSegment = node.getSegment(cache);
+ NodeKey parentKey = node.getParentKey(cache);
+ if (parentKey == null) {
+ // This is the root node ...
+ pathList = Collections.singletonList(node.getPath(cache));
+ } else {
+ // This is not the root node, so add a path for each of the parent's valid paths ...
+ CachedNode parent = cache.getNode(parentKey);
+ if (parent == null && removedCache != null) {
+ // This is a removed node, so check the removed cache ...
+ parent = removedCache.getNode(parentKey);
+ }
+ pathList = new LinkedList<Path>();
+ for (Path parentPath : getPaths(parent)) {
+ Path path = pathFactory.create(parentPath, nodeSegment);
+ pathList.add(path);
+ }
+ // Get the additional parents ...
+ Set<NodeKey> additionalParentKeys = getAdditionalParentKeys(node, cache);
+ // There is at least one additional parent ...
+ for (NodeKey additionalParentKey : additionalParentKeys) {
+ parent = cache.getNode(additionalParentKey);
+ for (Path parentPath : getPaths(parent)) {
+ Path path = pathFactory.create(parentPath, nodeSegment);
+ pathList.add(path);
+ }
+ }
+ }
+ assert pathList != null;
+ pathList = Collections.unmodifiableList(pathList);
+ paths.put(key, pathList);
+ }
+ return pathList;
+ }
+
+ public boolean removePath( NodeKey key ) {
+ return paths.remove(key) != null;
+ }
+
+ protected Set<NodeKey> getAdditionalParentKeys( CachedNode node,
+ NodeCache cache ) {
+ return node.getAdditionalParentKeys(cache);
+ }
+}
@@ -85,6 +85,18 @@
*/
Path getPath( NodeCache cache ) throws NodeNotFoundException;
+ /**
+ * Get the path to this node.
+ *
+ * @param pathCache the cache of paths that can be used to compute the path for any node; may not be null
+ * @return the node's path; never null with at least one segment for all nodes except the root node
+ * @throws NodeNotFoundInParentException if this node no longer exists inside the parent node (and perhaps in no other parent)
+ * @throws NodeNotFoundException if this node no longer exists
+ * @see #getName(NodeCache)
+ * @see #getSegment(NodeCache)
+ */
+ Path getPath( PathCache pathCache ) throws NodeNotFoundException;
+
/**
* Get the node key for this node's primary parent within this workspace.
*
@@ -38,11 +38,15 @@ public PathCache( NodeCache cache ) {
this.cache = cache;
}
+ public NodeCache getCache() {
+ return cache;
+ }
+
public Path getPath( CachedNode node ) {
NodeKey key = node.getKey();
Path path = paths.get(key);
if (path == null) {
- path = node.getPath(cache);
+ path = node.getPath(this);
paths.put(key, path); // even if null
}
return path;
@@ -54,8 +54,8 @@
/**
* The definition of a callback that can be implemented and passed to {@link SessionCache#save(SessionCache, PreSave)} and
- * {@link SessionCache#save(CachedNode, SessionCache, PreSave)}, allowing the caller to recieve a hook where they can
- * interrogate each of the changed nodes and perform additional logic prior to the actual persisting of the changes. Note that
+ * {@link SessionCache#save(Set, SessionCache, PreSave)}, allowing the caller to recieve a hook where they can interrogate
+ * each of the changed nodes and perform additional logic prior to the actual persisting of the changes. Note that
* implementations are free to make additional modifications to the supplied nodes, and even create additional nodes or change
* persistent but unchanged nodes, as long as these operations are done within the same calling thread.
*/
@@ -102,15 +102,15 @@ public void addContextData( String key,
* Saves all of this session's changes that were made at or below the specified path. Note that this is not terribly
* efficient, but is done to implement the deprecated {@link javax.jcr.Item#save()}.
*
- * @param node the node at or below which all changes should be saved; may not be null
+ * @param toBeSaved the set of keys identifying the nodes whose changes should be saved; may not be null
* @param otherSession another session whose changes should be saved with this session's changes; may not be null
* @param preSaveOperation the set of operations to run against the new and changed nodes prior to saving; may be null
* @throws LockFailureException if a requested lock could not be made
* @throws DocumentAlreadyExistsException if this session attempts to create a document that has the same key as an existing
* document
* @throws DocumentNotFoundException if one of the modified documents was removed by another session
*/
- public void save( CachedNode node,
+ public void save( Set<NodeKey> toBeSaved,
SessionCache otherSession,
PreSave preSaveOperation );
Oops, something went wrong.

0 comments on commit 105e5f8

Please sign in to comment.