SLING-12279 - Use ResourceResolver#getPropertyMap() for resource/resolver adaptables

pom.xml

@@ -99,7 +99,7 @@
         <dependency>
             <groupId>org.apache.sling</groupId>
             <artifactId>org.apache.sling.api</artifactId>
-            <version>2.21.0</version>
+            <version>2.24.0</version>
             <scope>provided</scope>
         </dependency>
         <dependency>

src/main/java/org/apache/sling/models/impl/AdapterCacheHolder.java

@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.models.impl;
+
+import java.lang.ref.SoftReference;
+import java.util.Collections;
+import java.util.Map;
+import java.util.WeakHashMap;
+
+import org.apache.sling.api.SlingHttpServletRequest;
+import org.jetbrains.annotations.NotNull;
+
+class AdapterCacheHolder implements AutoCloseable {
+    public static final String ADAPTABLE_ADAPTER_CACHE_KEY =
+            ModelAdapterFactory.class.getName() + ".AdapterCacheHolder";
+
+    /*
+     * This is to handle that all requests, including wrappers, utilize the same cache. If it is desired
+     */
+    private static final String SHARED_REQUEST_CACHE = "SHARED_REQUEST_CACHE";
+
+    // Map<Adaptable, Map<AdapterType, SoftReference<AdaptationResult>>
+    private final Map<Object, Map<Class<?>, SoftReference<Object>>> cacheMap;
+
+    private final boolean sync;
+
+    /**
+     * Create a cache, setting the sync and the Map fields. This constructor is primarily used to make testing easier.
+     * @param sync true if maps should be synchronized,
+     * @param cacheMap the map
+     */
+    AdapterCacheHolder(boolean sync, Map<Object, Map<Class<?>, SoftReference<Object>>> cacheMap) {
+        this.sync = sync;
+        this.cacheMap = cacheMap;
+    }
+
+    /**
+     * Create a cache, optionally using synchronized maps.
+     * @param sync true if maps should be synchronized,
+     */
+    public AdapterCacheHolder(boolean sync) {
+        this(sync, newMap(sync));
+    }
+
+    /**
+     * Gets the cache for a given adaptable.
+     * @param adaptable the adaptalbe
+     * @return the cache object
+     */
+    public @NotNull Map<Class<?>, SoftReference<Object>> getCacheMapForAdaptable(@NotNull Object adaptable) {
+        Object adaptableOrSharedKey = adaptable;
+
+        // All request wrappers share the cache of the original request
+        if (adaptable instanceof SlingHttpServletRequest) {
+            adaptableOrSharedKey = SHARED_REQUEST_CACHE;
+        }
+
+        return cacheMap.computeIfAbsent(adaptableOrSharedKey, key -> newMap(sync));
+    }
+
+    /**
+     * Clears the cache. Useful for caches that are stored in a {@link org.apache.sling.api.resource.ResourceResolver}
+     * or a {@link SlingHttpServletRequest}.
+     */
+    public void close() {
+        cacheMap.values().forEach(Map::clear);
+        cacheMap.clear();
+    }
+
+    /**
+     * Create a new map, either synchronized or not.
+     * @param sync true if it should use synchronized maps
+     * @return a new map
+     * @param <K> the key type
+     * @param <V> the value type
+     */
+    static <K, V> @NotNull Map<K, V> newMap(boolean sync) {
+        Map<K, V> theMap = new WeakHashMap<>();
+
+        if (sync) {
+            theMap = Collections.synchronizedMap(theMap);
+        }
+
+        return theMap;
+    }
+}

src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java

@@ -42,7 +42,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.WeakHashMap;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -52,6 +51,7 @@
 import org.apache.sling.api.adapter.AdapterFactory;
 import org.apache.sling.api.adapter.AdapterManager;
 import org.apache.sling.api.resource.Resource;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.commons.osgi.RankedServices;
 import org.apache.sling.models.annotations.Model;
 import org.apache.sling.models.annotations.ValidationStrategy;
@@ -126,8 +126,6 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
 
     private static final String REQUEST_MARKER_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".RealRequest";
 
-    private static final String REQUEST_CACHE_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".AdapterCache";
-
     private static class DisposalCallbackRegistryImpl implements DisposalCallbackRegistry, Disposable {
 
         private List<DisposalCallback> callbacks = new ArrayList<>();
@@ -222,7 +220,7 @@ private void clearDisposalCallbackRegistryQueue() {
     // dependencies)
     private ThreadLocal<ThreadInvocationCounter> invocationCountThreadLocal;
 
-    private Map<Object, Map<Class<?>, SoftReference<Object>>> adapterCache;
+    private AdapterCacheHolder adapterCacheHolder;
 
     private SlingModelsScriptEngineFactory scriptEngineFactory;
 
@@ -331,21 +329,31 @@ private <ModelType> ModelClass<ModelType> getImplementationTypeForAdapterType(
                 + " from adaptable " + adaptable.getClass());
     }
 
-    @SuppressWarnings("unchecked")
     private Map<Class<?>, SoftReference<Object>> getOrCreateCache(final Object adaptable) {
-        Map<Class<?>, SoftReference<Object>> adaptableCache;
+        AdapterCacheHolder cache;
         if (adaptable instanceof ServletRequest) {
             ServletRequest request = (ServletRequest) adaptable;
-            adaptableCache = (Map<Class<?>, SoftReference<Object>>) request.getAttribute(REQUEST_CACHE_ATTRIBUTE);
-            if (adaptableCache == null) {
-                adaptableCache = Collections.synchronizedMap(new WeakHashMap<>());
-                request.setAttribute(REQUEST_CACHE_ATTRIBUTE, adaptableCache);
+            cache = (AdapterCacheHolder) request.getAttribute(AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY);
+            if (cache == null) {
+                // This cache does not need to be synchronized, since it is just for a single request
+                cache = new AdapterCacheHolder(false);
+                request.setAttribute(AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY, cache);
+            }
+        } else if (adaptable instanceof Resource || adaptable instanceof ResourceResolver) {
+            ResourceResolver rr;
+            if (adaptable instanceof Resource) {
+                rr = ((Resource) adaptable).getResourceResolver();
+            } else {
+                rr = (ResourceResolver) adaptable;
             }
+            // This cache does not need to be synchronized, since it is just for a single resource or resource resolver
+            cache = (AdapterCacheHolder) rr.getPropertyMap()
+                    .computeIfAbsent(
+                            AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY, k -> new AdapterCacheHolder(false));
         } else {
-            adaptableCache =
-                    adapterCache.computeIfAbsent(adaptable, k -> Collections.synchronizedMap(new WeakHashMap<>()));
+            cache = adapterCacheHolder;
         }
-        return adaptableCache;
+        return cache.getCacheMapForAdaptable(adaptable);
     }
 
     @SuppressWarnings("unchecked")
@@ -1174,8 +1182,8 @@ protected ThreadInvocationCounter initialValue() {
             }
         };
 
-        this.adapterCache =
-                Collections.synchronizedMap(new WeakHashMap<Object, Map<Class<?>, SoftReference<Object>>>());
+        // This map needs to be synchronized, since it is shared
+        this.adapterCacheHolder = new AdapterCacheHolder(true);
 
         BundleContext bundleContext = ctx.getBundleContext();
         this.queue = new ReferenceQueue<>();
@@ -1210,7 +1218,8 @@ protected ThreadInvocationCounter initialValue() {
 
     @Deactivate
     protected void deactivate() {
-        this.adapterCache = null;
+        this.adapterCacheHolder.close();
+        this.adapterCacheHolder = null;
         this.clearDisposalCallbackRegistryQueue();
         this.listener.unregisterAll();
         this.adapterImplementations.removeAll();
@@ -1419,9 +1428,10 @@ public <T> T getModelFromWrappedRequest(
 
     @Override
     public void requestDestroyed(final ServletRequestEvent sre) {
-        final Object list = sre.getServletRequest().getAttribute(REQUEST_MARKER_ATTRIBUTE);
+        ServletRequest request = sre.getServletRequest();
+        final Object list = request.getAttribute(REQUEST_MARKER_ATTRIBUTE);
         if (list != null) {
-            sre.getServletRequest().removeAttribute(REQUEST_MARKER_ATTRIBUTE);
+            request.removeAttribute(REQUEST_MARKER_ATTRIBUTE);
             if (list instanceof List) {
                 final List<?> callbackList = (List<?>) list;
                 for (final Object disposable : callbackList) {
@@ -1432,6 +1442,9 @@ public void requestDestroyed(final ServletRequestEvent sre) {
                 callbackList.clear();
             }
         }
+        Optional.ofNullable(request.getAttribute(AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY))
+                .map(AdapterCacheHolder.class::cast)
+                .ifPresent(AdapterCacheHolder::close);
     }
 
     @Override

src/test/java/org/apache/sling/models/impl/AdapterCacheHolderTest.java

@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.models.impl;
+
+import java.lang.ref.SoftReference;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.WeakHashMap;
+
+import org.apache.sling.api.SlingHttpServletRequest;
+import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper;
+import org.apache.sling.servlethelpers.MockSlingHttpServletRequest;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AdapterCacheHolderTest {
+
+    @Test
+    public void testGetMapNonServletRequestAdaptable() {
+        Map<Object, Map<Class<?>, SoftReference<Object>>> outerMap = spy(new HashMap<>());
+        Object adaptable1 = new Object();
+        Object adaptable2 = new Object();
+
+        try (AdapterCacheHolder adapterCacheHolder = new AdapterCacheHolder(false, outerMap)) {
+            Map<Class<?>, SoftReference<Object>> map1 = adapterCacheHolder.getCacheMapForAdaptable(adaptable1);
+            Map<Class<?>, SoftReference<Object>> map2 = adapterCacheHolder.getCacheMapForAdaptable(adaptable1);
+            Map<Class<?>, SoftReference<Object>> map3 = adapterCacheHolder.getCacheMapForAdaptable(adaptable2);
+
+            assertSame(map1, map2);
+            assertNotSame(map1, map3);
+        }
+    }
+
+    @Test
+    public void testGetMapServletRequestAdaptable() {
+        Map<Object, Map<Class<?>, SoftReference<Object>>> outerMap = spy(new HashMap<>());
+        SlingHttpServletRequest adaptable1 = new MockSlingHttpServletRequest(null);
+        SlingHttpServletRequest adaptable2 = new SlingHttpServletRequestWrapper(adaptable1);
+        SlingHttpServletRequest adaptable3 = new MockSlingHttpServletRequest(null);
+
+        try (AdapterCacheHolder adapterCacheHolder = new AdapterCacheHolder(false, outerMap)) {
+            Map<Class<?>, SoftReference<Object>> map1 = adapterCacheHolder.getCacheMapForAdaptable(adaptable1);
+            Map<Class<?>, SoftReference<Object>> map2 = adapterCacheHolder.getCacheMapForAdaptable(adaptable2);
+            Map<Class<?>, SoftReference<Object>> map3 = adapterCacheHolder.getCacheMapForAdaptable(adaptable3);
+
+            assertSame(map1, map2);
+            assertSame(map1, map3);
+        }
+    }
+
+    @Test
+    public void testClose() {
+        Object adaptable = new Object();
+        Object result = new Object();
+        SoftReference<Object> ref = new SoftReference<>(result);
+
+        Map<Class<?>, SoftReference<Object>> innerMap = spy(new HashMap<>());
+        innerMap.put(Object.class, ref);
+
+        Map<Object, Map<Class<?>, SoftReference<Object>>> outerMap = spy(new HashMap<>());
+        outerMap.put(adaptable, innerMap);
+
+        try (AdapterCacheHolder adapterCacheHolder = new AdapterCacheHolder(false, outerMap)) {
+            // intentionally empty
+        }
+
+        verify(outerMap).values();
+        verify(outerMap).clear();
+        verify(innerMap).clear();
+    }
+
+    @Test
+    public void testNewMapSyncMap() {
+        // This is a very weak test, but it should be sufficient to at least confirm that we are not just using a
+        //  WeakHashMap, in case someone changes it without knowing why.
+        Map<Object, Object> nonSyncMap = AdapterCacheHolder.newMap(false);
+        Map<Object, Object> syncMap = AdapterCacheHolder.newMap(true);
+
+        try {
+            WeakHashMap<Object, Object> weakMap = (WeakHashMap<Object, Object>) nonSyncMap;
+        } catch (ClassCastException e) {
+            fail("Expected a WeakHashMap. Actual: " + nonSyncMap.getClass().getName());
+        }
+
+        try {
+            WeakHashMap<Object, Object> weakMap = (WeakHashMap<Object, Object>) syncMap;
+            fail("Expected a map other than WeakHashMap.");
+        } catch (ClassCastException e) {
+            // Expected to throw
+        }
+    }
+}

src/test/java/org/apache/sling/models/impl/CachingTest.java

@@ -20,17 +20,16 @@
 
 import java.util.Collections;
 import java.util.Enumeration;
+import java.util.HashMap;
 
 import org.apache.sling.api.resource.Resource;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.api.resource.ValueMap;
 import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper;
 import org.apache.sling.api.wrappers.ValueMapDecorator;
 import org.apache.sling.models.impl.injectors.RequestAttributeInjector;
 import org.apache.sling.models.impl.injectors.ValueMapInjector;
-import org.apache.sling.models.testmodels.classes.CachedModel;
-import org.apache.sling.models.testmodels.classes.CachedModelWithAdapterTypes12;
-import org.apache.sling.models.testmodels.classes.CachedModelWithAdapterTypes23;
-import org.apache.sling.models.testmodels.classes.UncachedModel;
+import org.apache.sling.models.testmodels.classes.*;
 import org.apache.sling.models.testmodels.interfaces.AdapterType1;
 import org.apache.sling.models.testmodels.interfaces.AdapterType2;
 import org.apache.sling.models.testmodels.interfaces.AdapterType3;
@@ -60,6 +59,9 @@ public class CachingTest {
     @Mock
     private Resource resource;
 
+    @Mock
+    private ResourceResolver resourceResolver;
+
     private ModelAdapterFactory factory;
 
     @Before
@@ -88,6 +90,9 @@ public void setup() {
 
         ValueMap vm = new ValueMapDecorator(Collections.singletonMap("testValue", "test"));
         when(resource.adaptTo(ValueMap.class)).thenReturn(vm);
+        when(resource.getResourceResolver()).thenReturn(resourceResolver);
+
+        when(resourceResolver.getPropertyMap()).thenReturn(new HashMap<>());
     }
 
     @Test