This is an automated email from the ASF dual-hosted git repository.
joerghoh pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
The following commit(s) were added to refs/heads/master by this push:
new f57cea1a SLING-13326 Fix caching for getParentResourceType (#236)
f57cea1a is described below
commit f57cea1a979c3bfecbe398b5958fa63d292c9d8a
Author: Jörg Hoh <[email protected]>
AuthorDate: Mon Oct 12 18:53:13 2026 +0200
SLING-13326 Fix caching for getParentResourceType (#236)
Use the resource type as the cache key for the parentResourceTypeMap,
rather than the resource path. This handles cases where a ResourceWrapper
exposes an existing resource with a different resource type while retaining the
same path.
---------
Co-authored-by: Julian Sedding <[email protected]>
---
.../impl/ResourceResolverImpl.java | 34 ++--
.../impl/ResourceResolverImplTest.java | 183 +++++++++++++++++----
2 files changed, 172 insertions(+), 45 deletions(-)
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
index 9f7b0248..699f125a 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverImpl.java
@@ -96,7 +96,7 @@ public class ResourceResolverImpl extends SlingAdaptable
implements ResourceReso
protected final Map<ResourceTypeInformation, Boolean>
resourceTypeLookupCache = new ConcurrentHashMap<>();
- // Store the resourceSupertype mapping (supertype can be null)
+ // Cache parent type lookups by resource type, including null results.
protected final Map<String, Optional<String>> parentResourceTypeMap = new
ConcurrentHashMap<>();
private Map<String, Object> propertyMap;
@@ -1148,22 +1148,10 @@ public class ResourceResolverImpl extends
SlingAdaptable implements ResourceReso
@Override
public String getParentResourceType(final Resource resource) {
checkClosed();
- String resourceSuperType = null;
- if (resource != null) {
- if (parentResourceTypeMap.containsKey(resource.getPath())) {
- resourceSuperType =
-
parentResourceTypeMap.get(resource.getPath()).orElse(null);
- } else {
- resourceSuperType = getParentResourceTypeInternal(resource);
- parentResourceTypeMap.put(resource.getPath(),
Optional.ofNullable(resourceSuperType));
- }
+ if (resource == null) {
+ return null;
}
- return resourceSuperType;
- }
-
- String getParentResourceTypeInternal(final Resource resource) {
- String resourceSuperType;
- resourceSuperType = resource.getResourceSuperType();
+ String resourceSuperType = resource.getResourceSuperType();
if (resourceSuperType == null) {
resourceSuperType =
this.getParentResourceType(resource.getResourceType());
}
@@ -1176,7 +1164,19 @@ public class ResourceResolverImpl extends SlingAdaptable
implements ResourceReso
@Override
public String getParentResourceType(final String resourceType) {
checkClosed();
- return this.control.getParentResourceType(this.factory, this,
resourceType);
+ if (resourceType == null) {
+ return null;
+ }
+ Optional<String> resourceSuperType =
parentResourceTypeMap.get(resourceType);
+ if (resourceSuperType == null) {
+ resourceSuperType =
+
Optional.ofNullable(this.control.getParentResourceType(this.factory, this,
resourceType));
+ // computeIfAbsent() might fail here, because type resolution can
re-enter this cache
+ // through providers or resource wrappers.
+ // In case of a race, the worst case is that multiple threads
compute the same super type.
+ parentResourceTypeMap.putIfAbsent(resourceType, resourceSuperType);
+ }
+ return resourceSuperType.orElse(null);
}
/**
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverImplTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverImplTest.java
index abdb2427..cfadf4aa 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverImplTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverImplTest.java
@@ -39,6 +39,7 @@ import org.apache.sling.api.resource.PersistenceException;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.resource.ResourceResolverFactory;
+import org.apache.sling.api.resource.ResourceWrapper;
import org.apache.sling.api.resource.SyntheticResource;
import org.apache.sling.api.security.ResourceAccessSecurity;
import
org.apache.sling.resourceresolver.impl.providers.ResourceProviderHandler;
@@ -51,7 +52,6 @@ import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.Before;
import org.junit.Test;
-import org.mockito.Mockito;
import org.osgi.framework.Bundle;
import static java.util.Arrays.asList;
@@ -63,10 +63,11 @@ import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
public class ResourceResolverImplTest {
@@ -563,7 +564,7 @@ public class ResourceResolverImplTest {
// correct
}
// same RP supporting and allowing orderBefore
- when(ras.canOrderChildren(Mockito.any())).thenReturn(true);
+ when(ras.canOrderChildren(any())).thenReturn(true);
assertTrue(resResolver.orderBefore(root, "test", "test2"));
}
@@ -596,19 +597,19 @@ public class ResourceResolverImplTest {
@Test
public void testIsResourceTypeCached() throws Exception {
- final PathBasedResourceResolverImpl resolver =
Mockito.spy(getPathBasedResourceResolver());
+ final PathBasedResourceResolverImpl resolver =
spy(getPathBasedResourceResolver());
final Resource r1 = resolver.add(new SyntheticResource(resolver, "/a",
"a:b"));
final Resource r2 = resolver.add(new
SyntheticResourceWithSupertype(resolver, "/b", "a:b", "c:d"));
// 1st lookup needs to get through, 2nd will be taken from cache
assertTrue(resolver.isResourceType(r1, "a:b"));
- Mockito.verify(resolver,
Mockito.times(1)).isResourceTypeInternal(eq(r1), eq("a:b"));
+ verify(resolver, times(1)).isResourceTypeInternal(r1, "a:b");
assertTrue(resolver.isResourceType(r1, "a:b"));
- Mockito.verify(resolver,
Mockito.times(1)).isResourceTypeInternal(eq(r1), eq("a:b"));
+ verify(resolver, times(1)).isResourceTypeInternal(r1, "a:b");
resolver.refresh();
assertTrue(resolver.isResourceType(r1, "a:b"));
- Mockito.verify(resolver,
Mockito.times(2)).isResourceTypeInternal(eq(r1), eq("a:b"));
+ verify(resolver, times(2)).isResourceTypeInternal(r1, "a:b");
// make sure that resources with the same resourceType but different
resourceSuperType are
// treated differently
@@ -737,18 +738,11 @@ public class ResourceResolverImplTest {
// use the propertyMap
resolver = getPathBasedResourceResolver();
Object value1 = new String("value1");
- Closeable value2 = Mockito.spy(new Closeable() {
- @Override
- public void close() {
- // do nothing
- }
- });
- Closeable valueWithException = Mockito.spy(new Closeable() {
- @Override
- public void close() {
- throw new RuntimeException("RuntimeExceptions in close must be
handled");
- }
- });
+ Closeable value2 = mock(Closeable.class);
+ Closeable valueWithException = mock(Closeable.class);
+ doThrow(new RuntimeException("RuntimeExceptions in close must be
handled"))
+ .when(valueWithException)
+ .close();
assertNotNull(resolver.getPropertyMap());
resolver.getPropertyMap().put("key1", value1);
resolver.getPropertyMap().put("key2", value2);
@@ -757,23 +751,156 @@ public class ResourceResolverImplTest {
resolver.close();
assertNotNull(resolver.getPropertyMap());
assertTrue(resolver.getPropertyMap().isEmpty());
- Mockito.verify(value2, Mockito.times(1)).close();
- Mockito.verify(valueWithException, Mockito.times(1)).close();
+ verify(value2, times(1)).close();
+ verify(valueWithException, times(1)).close();
}
@Test
public void testGetParentResourceType() {
- final PathBasedResourceResolverImpl resolver =
Mockito.spy(getPathBasedResourceResolver());
+ final PathBasedResourceResolverImpl resolver =
getPathBasedResourceResolver();
+ final Resource type = resolver.add(
+ spy(new SyntheticResourceWithSupertype(resolver, "/types/1",
"/types/component", "/types/2")));
+ final Resource resource =
+ resolver.add(new SyntheticResourceWithSupertype(resolver,
"/content/type1", "/types/1", null));
+ final Resource otherResource =
+ resolver.add(new SyntheticResourceWithSupertype(resolver,
"/content/type2", "/types/1", null));
+
+ assertEquals("/types/2", resolver.getParentResourceType(type));
+ assertTrue(resolver.parentResourceTypeMap.isEmpty());
+ verify(type, times(1)).getResourceSuperType();
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ assertEquals("/types/2",
resolver.getParentResourceType(otherResource));
+ assertEquals("/types/2", resolver.getParentResourceType("/types/1"));
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ verify(type, times(2)).getResourceSuperType();
+ assertEquals(1, resolver.parentResourceTypeMap.size());
+ }
+
+ /**
+ * @see <a
href="https://issues.apache.org/jira/browse/SLING-13326";>SLING-13326</a>
+ */
+ @Test
+ public void testGetParentResourceTypeWithOverriddenResourceType() {
+ final PathBasedResourceResolverImpl resolver =
getPathBasedResourceResolver();
+
+ resolver.add(new SyntheticResourceWithSupertype(
+ resolver, "/foo/pages/special", "types/component",
"foo/pages/base"));
+ resolver.add(new SyntheticResourceWithSupertype(
+ resolver, "/foo/pages/base", "types/component",
"generic/pages/page"));
+ resolver.add(new SyntheticResource(resolver, "/generic/pages/page",
"types/component"));
+ final Resource resource = resolver.add(new SyntheticResource(resolver,
"/content/foo", "foo/pages/special"));
+ final Resource decorated = new ResourceWrapper(resource) {
+ @Override
+ public String getResourceType() {
+ return "foo/pages/base";
+ }
+ };
+
+ assertEquals(resource.getPath(), decorated.getPath());
+ assertEquals("foo/pages/base",
resolver.getParentResourceType(resource));
+ assertEquals(
+ "The cached parent type must reflect the wrapper's overridden
resource type",
+ "generic/pages/page",
+ resolver.getParentResourceType(decorated));
+ assertEquals("foo/pages/base",
resolver.getParentResourceType(resource));
+ }
+
+ @Test
+ public void testGetParentResourceTypeWithNestedLookup() {
+ final PathBasedResourceResolverImpl resolver =
getPathBasedResourceResolver();
+ // These distinct type names collide, exercising nested updates to the
same cache bin.
+ assertEquals("Aa".hashCode(), "BB".hashCode());
+ final Resource outerType = resolver.add(spy(new
SyntheticResource(resolver, "/Aa", "types/component") {
+ @Override
+ public String getResourceSuperType() {
+ return resolver.getParentResourceType("BB");
+ }
+ }));
+ final Resource innerType = resolver.add(
+ spy(new SyntheticResourceWithSupertype(resolver, "/BB",
"types/component", "types/parent")));
+
+ assertEquals("types/parent", resolver.getParentResourceType("Aa"));
+ assertEquals("types/parent", resolver.getParentResourceType("BB"));
+ assertEquals("types/parent", resolver.getParentResourceType("Aa"));
+ verify(outerType, times(1)).getResourceSuperType();
+ verify(innerType, times(1)).getResourceSuperType();
+ assertEquals(2, resolver.parentResourceTypeMap.size());
+ }
+
+ @Test
+ public void testGetParentResourceTypeWithOverriddenResourceSuperType() {
+ final PathBasedResourceResolverImpl resolver =
getPathBasedResourceResolver();
+ resolver.add(new SyntheticResourceWithSupertype(resolver, "/types/1",
"/types/component", "/types/2"));
+ final Resource resource = resolver.add(new SyntheticResource(resolver,
"/content/type1", "/types/1"));
+ final Resource decorated = spy(new ResourceWrapper(resource));
+ when(decorated.getResourceSuperType()).thenReturn("/types/3",
"/types/4", null);
+
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ assertEquals("/types/3", resolver.getParentResourceType(decorated));
+ assertEquals("/types/4", resolver.getParentResourceType(decorated));
+ assertEquals("/types/2", resolver.getParentResourceType(decorated));
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ assertEquals(1, resolver.parentResourceTypeMap.size());
+ }
+
+ @Test
+ public void testGetParentResourceTypeWithCachedNull() {
+ final PathBasedResourceResolverImpl resolver =
getPathBasedResourceResolver();
resolver.add(new SyntheticResourceWithSupertype(resolver, "/types/1",
"/types/component", "/types/2"));
- resolver.add(new SyntheticResourceWithSupertype(resolver,
"/content/type1", "/types/1", null));
+ final Resource type = resolver.add(spy(new SyntheticResource(resolver,
"/types/unknown", "/types/component")));
+ final Resource resource = resolver.add(new SyntheticResource(resolver,
"/content/type1", "/types/unknown"));
+ final Resource decorated = new ResourceWrapper(resource) {
+ @Override
+ public String getResourceType() {
+ return "/types/1";
+ }
+ };
- assertEquals("/types/2",
resolver.getParentResourceType(resolver.getResource("/types/1")));
- assertEquals("/types/2",
resolver.getParentResourceType(resolver.getResource("/content/type1")));
+ assertNull(resolver.getParentResourceType((Resource) null));
+ assertNull(resolver.getParentResourceType((String) null));
+ assertNull(resolver.getParentResourceType(mock(Resource.class)));
+ assertTrue(resolver.parentResourceTypeMap.isEmpty());
+ assertNull(resolver.getParentResourceType(resource));
+ assertNull(resolver.getParentResourceType(resource));
+ assertNull(resolver.getParentResourceType("/types/unknown"));
+ verify(type, times(1)).getResourceSuperType();
+ assertEquals("/types/2", resolver.getParentResourceType(decorated));
+ assertNull(resolver.getParentResourceType(resource));
+ verify(type, times(1)).getResourceSuperType();
+ }
- // Ensure that the next call will be served from the cache
- resolver.getParentResourceType(resolver.getResource("/types/1"));
- Mockito.verify(resolver,
times(2)).getParentResourceTypeInternal(any(Resource.class));
+ @Test
+ public void testGetParentResourceTypeCacheClearedOnRefresh() throws
LoginException {
+ final PathBasedResourceResolverImpl typeResolver =
getPathBasedResourceResolver();
+ final PathBasedResourceResolverImpl resolver =
+ new PathBasedResourceResolverImpl(asList(typeResolver),
resourceProviderTracker, new String[] {""});
+ final Resource type = typeResolver.add(
+ spy(new SyntheticResourceWithSupertype(resolver, "/types/1",
"/types/component", "/types/2")));
+ final Resource unknownType =
+ typeResolver.add(spy(new SyntheticResource(resolver,
"/types/unknown", "/types/component")));
+ final Resource resource = resolver.add(new SyntheticResource(resolver,
"/content/type1", "/types/1"));
+
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ assertNull(resolver.getParentResourceType("/types/unknown"));
+ verify(type, times(1)).getResourceSuperType();
+ verify(unknownType, times(1)).getResourceSuperType();
+
+ final Resource updatedType = typeResolver.add(
+ spy(new SyntheticResourceWithSupertype(resolver, "/types/1",
"/types/component", "/types/3")));
+ final Resource updatedUnknownType = typeResolver.add(
+ spy(new SyntheticResourceWithSupertype(resolver,
"/types/unknown", "/types/component", "/types/4")));
+ assertEquals("/types/2", resolver.getParentResourceType(resource));
+ assertNull(resolver.getParentResourceType("/types/unknown"));
+ verify(updatedType, times(0)).getResourceSuperType();
+ verify(updatedUnknownType, times(0)).getResourceSuperType();
+ resolver.refresh();
+ assertTrue(resolver.parentResourceTypeMap.isEmpty());
+ assertEquals("/types/3", resolver.getParentResourceType(resource));
+ assertEquals("/types/4",
resolver.getParentResourceType("/types/unknown"));
+ verify(updatedType, times(1)).getResourceSuperType();
+ verify(updatedUnknownType, times(1)).getResourceSuperType();
}
private PathBasedResourceResolverImpl getPathBasedResourceResolver() {
