Merge pull request #23 from apache/SLING-10011
SLING-10011 : Use JackrabbitSession.getParentOrNull when resolving parent JCR node in JcrResourceProvider#getParent
diff --git a/pom.xml b/pom.xml
index 8aeb840..fd803b0 100644
--- a/pom.xml
+++ b/pom.xml
@@ -44,7 +44,9 @@
<properties>
<site.jira.version.id>12314286</site.jira.version.id>
<site.javadoc.exclude>**.internal.**</site.javadoc.exclude>
- <oak.version>1.10.0</oak.version><!-- first version compatible with Jackrabbit API 2.18 (https://issues.apache.org/jira/browse/OAK-7943) -->
+ <!-- aok 1.10.0 was first version compatible with Jackrabbit API 2.18 (https://issues.apache.org/jira/browse/OAK-7943) -->
+ <!-- aok 1.42.0 first version to include JackrabbitSession.getParentOrNull in oak-jackrabbit-api (https://issues.apache.org/jira/browse/SLING-10011) -->
+ <oak.version>1.42.0</oak.version>
<jackrabbit.version>2.18.0</jackrabbit.version><!-- required for direct binary access, https://issues.apache.org/jira/browse/JCR-4335 -->
<project.build.outputTimestamp>1</project.build.outputTimestamp>
</properties>
@@ -112,8 +114,8 @@
<!-- Jackrabbit / Oak -->
<dependency>
<groupId>org.apache.jackrabbit</groupId>
- <artifactId>jackrabbit-api</artifactId>
- <version>${jackrabbit.version}</version>
+ <artifactId>oak-jackrabbit-api</artifactId>
+ <version>${oak.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java
index 22b210e..6858f19 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactory.java
@@ -35,6 +35,8 @@
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.jcr.resource.internal.HelperData;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -197,4 +199,21 @@
return item;
}
+ @Nullable Node getParentOrNull(@NotNull Item child, @NotNull String parentPath) {
+ Node parent = null;
+ try {
+ // Use fast getParentOrNull if session is a JackrabbitSession
+ if (this.isJackrabbit) {
+ parent = ((JackrabbitSession) session).getParentOrNull(child);
+ } else if (session.nodeExists(parentPath)) {
+ // Fallback to slower nodeExists & getNode pattern
+ parent = session.getNode(parentPath);
+ }
+ } catch (RepositoryException e) {
+ log.debug("Unable to access node at {}", parentPath, e);
+ }
+
+ return parent;
+ }
+
}
diff --git a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java
index bdb7790..f28774f 100644
--- a/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java
+++ b/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java
@@ -57,7 +57,6 @@
import org.apache.sling.jcr.resource.internal.JcrListenerBaseConfig;
import org.apache.sling.jcr.resource.internal.JcrModifiableValueMap;
import org.apache.sling.jcr.resource.internal.JcrResourceListener;
-import org.apache.sling.jcr.resource.internal.NodeUtil;
import org.apache.sling.spi.resource.provider.ObserverConfiguration;
import org.apache.sling.spi.resource.provider.ProviderContext;
import org.apache.sling.spi.resource.provider.QueryLanguageProvider;
@@ -358,28 +357,21 @@
@Override
public @Nullable Resource getParent(final @NotNull ResolveContext<JcrProviderState> ctx, final @NotNull Resource child) {
if (child instanceof JcrItemResource<?>) {
- try {
- String version = null;
- if (child.getResourceMetadata().getParameterMap() != null) {
- version = child.getResourceMetadata().getParameterMap().get("v");
- }
- if (version == null) {
- String parentPath = ResourceUtil.getParent(child.getPath());
- if (parentPath != null) {
- Item parentItem = ctx.getProviderState().getResourceFactory()
- .getItemOrNull(parentPath);
- if (parentItem != null && parentItem.isNode()) {
- return new JcrNodeResource(ctx.getResourceResolver(),
- parentPath, null, (Node)parentItem,
- ctx.getProviderState().getHelperData());
- }
- }
- return null;
- }
- } catch (RepositoryException e) {
- logger.warn("Can't get parent for {}", child, e);
- return null;
+ String version = null;
+ if (child.getResourceMetadata().getParameterMap() != null) {
+ version = child.getResourceMetadata().getParameterMap().get("v");
}
+ if (version == null) {
+ String parentPath = ResourceUtil.getParent(child.getPath());
+ if (parentPath != null) {
+ Item childItem = ((JcrItemResource) child).getItem();
+ Node parentNode = ctx.getProviderState().getResourceFactory().getParentOrNull(childItem, parentPath);
+ if (parentNode != null) {
+ return new JcrNodeResource(ctx.getResourceResolver(), parentPath, null, parentNode, ctx.getProviderState().getHelperData());
+ }
+ }
+ }
+ return null;
}
return super.getParent(ctx, child);
}
diff --git a/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactoryTest.java b/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactoryTest.java
index 3d65ebd..a1c4dc9 100644
--- a/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactoryTest.java
+++ b/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrItemResourceFactoryTest.java
@@ -18,20 +18,28 @@
*/
package org.apache.sling.jcr.resource.internal.helper.jcr;
+import org.apache.jackrabbit.JcrConstants;
+import org.apache.jackrabbit.commons.JcrUtils;
+import org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils;
+import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
+import org.apache.sling.api.resource.external.URIProvider;
+import org.apache.sling.commons.classloader.DynamicClassLoaderManager;
+import org.apache.sling.jcr.resource.internal.HelperData;
+
+import javax.jcr.GuestCredentials;
+import javax.jcr.Item;
+import javax.jcr.Node;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.security.Privilege;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.concurrent.atomic.AtomicReference;
-import javax.jcr.Item;
-import javax.jcr.Node;
-import javax.jcr.RepositoryException;
-import javax.jcr.Session;
-
-import org.apache.jackrabbit.commons.JcrUtils;
-import org.apache.sling.api.resource.external.URIProvider;
-import org.apache.sling.commons.classloader.DynamicClassLoaderManager;
-import org.apache.sling.jcr.resource.internal.HelperData;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
public class JcrItemResourceFactoryTest extends SlingRepositoryTestBase {
@@ -41,7 +49,7 @@
private Node node;
private Session nonJackrabbitSession;
-
+
@Override
protected void setUp() throws Exception {
super.setUp();
@@ -58,6 +66,9 @@
return method.invoke(session, args);
}
});
+
+ AccessControlUtils.allow(node, EveryonePrincipal.NAME, Privilege.JCR_READ);
+ session.save();
}
@Override
@@ -100,5 +111,53 @@
assertEquals(expectedPath, item2.getPath());
}
}
+
+ public void testGetParentOrNullExistingNode() throws RepositoryException {
+ compareGetParentOrNull(session, EXISTING_NODE_PATH, false);
+ compareGetParentOrNull(nonJackrabbitSession, EXISTING_NODE_PATH, false);
+ }
+ public void testGetParentOrNullExistingProperty() throws RepositoryException {
+ compareGetParentOrNull(session,EXISTING_NODE_PATH + "/" + JcrConstants.JCR_PRIMARYTYPE, false);
+ compareGetParentOrNull(nonJackrabbitSession,EXISTING_NODE_PATH + "/" + JcrConstants.JCR_PRIMARYTYPE, false);
+ }
+
+ public void testGetParentOrNullNonAccessibleParent() throws RepositoryException {
+ Session guestSession = null;
+ try {
+ guestSession = session.getRepository().login(new GuestCredentials());
+ compareGetParentOrNull(guestSession, EXISTING_NODE_PATH, true);
+ } finally {
+ if (guestSession != null) {
+ guestSession.logout();
+ }
+ }
+ }
+
+ public void testGetParentOrNullNonExistingParentNode() throws RepositoryException {
+ Session s = mock(Session.class);
+ when(s.getItem(EXISTING_NODE_PATH)).thenReturn(nonJackrabbitSession.getItem(EXISTING_NODE_PATH));
+ when(s.nodeExists(PathUtils.getParentPath(EXISTING_NODE_PATH))).thenReturn(false);
+ compareGetParentOrNull(s, EXISTING_NODE_PATH, true);
+ }
+
+ public void testGetParentOrNullWithException() throws RepositoryException {
+ Session s = mock(Session.class);
+ when(s.getItem(EXISTING_NODE_PATH)).thenReturn(nonJackrabbitSession.getItem(EXISTING_NODE_PATH));
+ when(s.nodeExists(PathUtils.getParentPath(EXISTING_NODE_PATH))).thenThrow(new RepositoryException());
+ compareGetParentOrNull(s, EXISTING_NODE_PATH, true);
+ }
+
+ private void compareGetParentOrNull(Session s, String path, boolean nullExpected) throws RepositoryException {
+ HelperData helper = new HelperData(new AtomicReference<DynamicClassLoaderManager>(), new AtomicReference<URIProvider[]>());
+
+ String parentPath = PathUtils.getParentPath(path);
+ Node parent = new JcrItemResourceFactory(s, helper).getParentOrNull(s.getItem(path), parentPath);
+ if (nullExpected) {
+ assertNull(parent);
+ } else {
+ assertNotNull(parent);
+ assertEquals(parentPath, parent.getPath());
+ }
+ }
}
diff --git a/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderTest.java b/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderTest.java
index 684ce80..1c97445 100644
--- a/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderTest.java
+++ b/src/test/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderTest.java
@@ -19,6 +19,7 @@
package org.apache.sling.jcr.resource.internal.helper.jcr;
import java.security.Principal;
+import java.util.concurrent.atomic.AtomicReference;
import javax.jcr.Node;
import javax.jcr.Repository;
@@ -26,16 +27,24 @@
import javax.jcr.Session;
import javax.jcr.nodetype.NodeType;
-import org.apache.jackrabbit.commons.JcrUtils;
+import org.apache.jackrabbit.oak.commons.PathUtils;
import org.apache.sling.api.resource.PersistenceException;
import org.apache.sling.api.resource.Resource;
+import org.apache.sling.api.resource.external.URIProvider;
+import org.apache.sling.commons.classloader.DynamicClassLoaderManager;
+import org.apache.sling.jcr.resource.internal.HelperData;
import org.apache.sling.spi.resource.provider.ResolveContext;
import org.apache.sling.spi.resource.provider.ResourceContext;
+import org.jetbrains.annotations.NotNull;
import org.junit.Assert;
import org.mockito.Mockito;
import org.osgi.framework.ServiceReference;
import org.osgi.service.component.ComponentContext;
+import static javax.jcr.nodetype.NodeType.NT_UNSTRUCTURED;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
public class JcrResourceProviderTest extends SlingRepositoryTestBase {
JcrResourceProvider jcrResourceProvider;
@@ -47,34 +56,48 @@
// create the session
session = getSession();
Repository repo = getRepository();
- ComponentContext ctx = Mockito.mock(ComponentContext.class);
- Mockito.when(ctx.locateService(Mockito.anyString(), Mockito.any(ServiceReference.class))).thenReturn(repo);
+ ComponentContext ctx = mock(ComponentContext.class);
+ when(ctx.locateService(Mockito.anyString(), Mockito.any(ServiceReference.class))).thenReturn(repo);
jcrResourceProvider = new JcrResourceProvider();
jcrResourceProvider.activate(ctx);
}
@Override
protected void tearDown() throws Exception {
- jcrResourceProvider.deactivate();
- super.tearDown();
+ try {
+ if (session.nodeExists("/parent")) {
+ session.removeItem("/parent");
+ session.save();
+ }
+ } finally {
+ jcrResourceProvider.deactivate();
+ super.tearDown();
+ }
+ }
+
+ private @NotNull JcrProviderState createProviderState() {
+ return new JcrProviderState(session, new HelperData(new AtomicReference<>(), new AtomicReference<>()), false);
+ }
+
+ private @NotNull ResolveContext mockResolveContext() {
+ ResolveContext ctx = mock(ResolveContext.class);
+ when(ctx.getProviderState()).thenReturn(createProviderState());
+ return ctx;
}
public void testAdaptTo_Principal() {
- ResolveContext ctx = Mockito.mock(ResolveContext.class);
- Mockito.when(ctx.getProviderState()).thenReturn(new JcrProviderState(session, null, false));
+ ResolveContext ctx = mockResolveContext();
Assert.assertNotNull(jcrResourceProvider.adaptTo(ctx, Principal.class));
}
public void testOrderBefore() throws RepositoryException, PersistenceException {
- Node parentNode = session.getRootNode().addNode("parent", NodeType.NT_UNSTRUCTURED);
- parentNode.addNode("child1", NodeType.NT_UNSTRUCTURED);
- parentNode.addNode("child2", NodeType.NT_UNSTRUCTURED);
- parentNode.addNode("child3", NodeType.NT_UNSTRUCTURED);
+ Node parentNode = session.getRootNode().addNode("parent", NT_UNSTRUCTURED);
+ parentNode.addNode("child1", NT_UNSTRUCTURED);
+ parentNode.addNode("child2", NT_UNSTRUCTURED);
+ parentNode.addNode("child3", NT_UNSTRUCTURED);
session.save();
- ResolveContext ctx = Mockito.mock(ResolveContext.class);
- JcrProviderState state = new JcrProviderState(session, null, false);
- Mockito.when(ctx.getProviderState()).thenReturn(state);
+ ResolveContext ctx = mockResolveContext();
Resource parent = jcrResourceProvider.getResource(ctx, "/parent", ResourceContext.EMPTY_CONTEXT, null);
Assert.assertNotNull(parent);
// order with invalid names
@@ -100,6 +123,45 @@
Assert.assertTrue(jcrResourceProvider.orderBefore(ctx, parent, "child2", null));
}
+
+ public void testGetParent() throws Exception {
+ Node parentNode = session.getRootNode().addNode("parent", NT_UNSTRUCTURED);
+ Node child = parentNode.addNode("child", NT_UNSTRUCTURED);
+ Node grandchild = child.addNode("grandchild", NT_UNSTRUCTURED);
+ session.save();
+
+ ResolveContext ctx = mockResolveContext();
+ Resource rootResource = jcrResourceProvider.getResource(ctx, PathUtils.ROOT_PATH, ResourceContext.EMPTY_CONTEXT, null);
+ Resource parentResource = jcrResourceProvider.getResource(ctx, parentNode.getPath(), ResourceContext.EMPTY_CONTEXT, rootResource);
+ Resource childResource = jcrResourceProvider.getResource(ctx, child.getPath(), ResourceContext.EMPTY_CONTEXT, parentResource);
+ Resource grandChildResource = jcrResourceProvider.getResource(ctx, grandchild.getPath(), ResourceContext.EMPTY_CONTEXT, childResource);
+ assertResources(rootResource, parentResource, childResource, grandChildResource);
+
+ assertParent(jcrResourceProvider.getParent(ctx, grandChildResource), child.getPath());
+ assertParent(jcrResourceProvider.getParent(ctx, childResource), parentNode.getPath());
+ assertParent(jcrResourceProvider.getParent(ctx, parentResource), PathUtils.ROOT_PATH);
+ assertNull(jcrResourceProvider.getParent(ctx, rootResource));
+ }
+
+ public void testGetParentDifferentResource() {
+ Resource r = mock(Resource.class);
+ when(r.getPath()).thenReturn("/test/path");
+ Resource parent = jcrResourceProvider.getParent(mockResolveContext(), r);
+ assertFalse(parent instanceof JcrNodeResource);
+ }
+
+ private static void assertResources(Resource... resources) {
+ for (Resource r : resources) {
+ assertNotNull(r);
+ assertTrue(r instanceof JcrNodeResource);
+ }
+ }
+
+ private static void assertParent(Resource parent, String expectedPath) {
+ assertNotNull(parent);
+ assertTrue(parent instanceof JcrNodeResource);
+ assertEquals(expectedPath, parent.getPath());
+ }
}
