This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-12217-cache-implementation-lookups in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-models-impl.git
commit db2a88e99d098d77fad5b0648d5ee6fa3dee1d9d Author: Joerg Hoh <[email protected]> AuthorDate: Sun Jun 28 16:28:19 2026 +0200 SLING-12217 Cache model implementation class lookups per ResourceResolver - ModelAdapterFactory: cache the resolved ModelClass (or a negative-result sentinel) in the backing ResourceResolver.getPropertyMap(), as a bounded LRU map to avoid unbounded growth - AdapterImplementations: add requiresImplementationPickerLookup() to limit caching to the picker-based path. - ModelAdapterFactoryConfiguration: add cache_implementation_lookups (default true) and implementation_lookup_cache_size (default 100). - add test cases and run relevant existing tests with and without this feature enabled --- pom.xml | 5 + .../sling/models/impl/AdapterImplementations.java | 22 ++ .../sling/models/impl/ModelAdapterFactory.java | 126 ++++++++++- .../impl/ModelAdapterFactoryConfiguration.java | 17 ++ .../sling/models/impl/AdapterFactoryTest.java | 10 + .../org/apache/sling/models/impl/CachingTest.java | 12 +- .../impl/ImplementationLookupCachingTest.java | 245 +++++++++++++++++++++ .../sling/models/impl/ImplementsExtendsTest.java | 12 +- ...apterFactory_ImplementationPickerOrderTest.java | 15 +- 9 files changed, 460 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index bd2ed9c..66a7c50 100644 --- a/pom.xml +++ b/pom.xml @@ -175,6 +175,11 @@ <artifactId>junit-jupiter-api</artifactId> <scope>test</scope> </dependency> + <dependency> + <groupId>org.junit.jupiter</groupId> + <artifactId>junit-jupiter-params</artifactId> + <scope>test</scope> + </dependency> <dependency> <groupId>org.junit.platform</groupId> <artifactId>junit-platform-engine</artifactId> diff --git a/src/main/java/org/apache/sling/models/impl/AdapterImplementations.java b/src/main/java/org/apache/sling/models/impl/AdapterImplementations.java index 956cabc..367d0e2 100644 --- a/src/main/java/org/apache/sling/models/impl/AdapterImplementations.java +++ b/src/main/java/org/apache/sling/models/impl/AdapterImplementations.java @@ -227,6 +227,25 @@ final class AdapterImplementations { return null; } + /** + * Checks whether resolving the implementation for the given adapter type requires consulting the + * {@link ImplementationPicker}s. This is only the case when the adapter type is not directly registered as its own + * model (the cheap, deterministic fast path in {@link #lookup}) but has one or more implementations registered with + * a different adapter class. Only that path can cause repository access (resource-type based picking) and is + * therefore worth caching, see SLING-12217. + * + * @param adapterType the adapter type to check + * @return {@code true} if a picker has to be consulted to resolve the implementation + */ + public boolean requiresImplementationPickerLookup(Class<?> adapterType) { + String key = adapterType.getName(); + if (modelClasses.containsKey(key)) { + return false; + } + ConcurrentNavigableMap<String, ModelClass<?>> implementations = adapterImplementations.get(key); + return implementations != null && !implementations.isEmpty(); + } + /** * @param adapterType the type to check * @return {@code true} in case the given type is a model (may be with a different adapter class) @@ -319,6 +338,9 @@ final class AdapterImplementations { return null; } ResourceResolver resolver = resource.getResourceResolver(); + if (resolver == null) { + return null; + } final String originalResourceType = resource.getResourceType(); Class<?> modelClass = getClassFromResourceTypeMap(originalResourceType, map, resolver); if (modelClass != null) { diff --git a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java index 5b7fb8e..6252608 100644 --- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java +++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java @@ -36,6 +36,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Hashtable; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -54,6 +55,7 @@ import org.apache.sling.api.adapter.Adaptable; 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.models.annotations.Model; import org.apache.sling.models.annotations.ValidationStrategy; import org.apache.sling.models.annotations.ViaProviderType; @@ -129,6 +131,30 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto private static final String REQUEST_CACHE_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".AdapterCache"; + // Implementation lookup cache: + // Resolving the implementation class for an adapter type is purely a function of the requested adapter type, the + // type of the adaptable and the resource type of the (possibly wrapped) resource. Only the picker-based path walks + // the resource super type hierarchy (repository access), so caching is limited to that path (see + // AdapterImplementations#requiresImplementationPickerLookup) and is scoped per ResourceResolver via its property + // map. This removes redundant lookups for the canCreateFromAdaptable/createModel pair and across components sharing + // a resource type, while leaving the cheap fast path untouched. The cache is a bounded LRU map (see + // implementationLookupCacheSize) so it cannot grow unbounded for long-lived resolvers or large numbers of distinct + // resource types. The cache key is "adaptableClass|adapterType|resourceType"; the concrete adaptable type is + // included so custom adaptables (e.g. request wrappers) do not share entries with unrelated ones. + + /** + * Key under which the (bounded) implementation lookup cache is stored in the backing + * {@link ResourceResolver#getPropertyMap()} (see {@link #getImplementationTypeForAdapterType(Class, Object)}). + */ + private static final String IMPLEMENTATION_LOOKUP_CACHE_KEY = + ModelAdapterFactory.class.getName() + ".ImplementationLookup"; + + /** + * Sentinel stored in the implementation lookup cache to memoize a negative result (no model implementation found + * for the given adaptable and resource type), so that repeated probing does not trigger the lookup again. + */ + private static final Object NO_MODEL_IMPLEMENTATION = new Object(); + private final Logger log = LoggerFactory.getLogger(ModelAdapterFactory.class); private ReferenceQueue<Object> queue; @@ -212,6 +238,12 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto private Map<Object, Map<Class<?>, SoftReference<Object>>> adapterCache; + /** Whether to cache model implementation class lookups per ResourceResolver, see SLING-12217. */ + private boolean cacheImplementationLookups; + + /** Maximum number of entries in the per-ResourceResolver implementation lookup cache, see SLING-12217. */ + private int implementationLookupCacheSize; + private SlingModelsScriptEngineFactory scriptEngineFactory; @Override @@ -324,22 +356,111 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto * href="http://sling.apache.org/documentation/bundles/models.html#specifying-an-alternate-adapter-class-since-sling-models-110";>Specifying * an Alternate Adapter Class</a> */ + @SuppressWarnings("unchecked") private <ModelType> ModelClass<ModelType> getImplementationTypeForAdapterType( Class<ModelType> requestedType, Object adaptable) { + // only the (repository-accessing) picker-based path is cached + final boolean cacheable = + cacheImplementationLookups && adapterImplementations.requiresImplementationPickerLookup(requestedType); + final Map<String, Object> cache = cacheable ? getImplementationLookupCache(adaptable) : null; + final String cacheKey = cache != null ? getImplementationLookupCacheKey(requestedType, adaptable) : null; + if (cache != null && cacheKey != null) { + final Object cached = cache.get(cacheKey); + if (cached == NO_MODEL_IMPLEMENTATION) { + throw newNoImplementationException(requestedType, adaptable); + } else if (cached != null) { + return (ModelClass<ModelType>) cached; + } + } + // lookup ModelClass wrapper for implementation type // additionally check if a different implementation class was registered for this adapter type // the adapter implementation is initially filled by the ModelPackageBundleList ModelClass<ModelType> modelClass = this.adapterImplementations.lookup(requestedType, adaptable, this.implementationPickers); + + if (cache != null && cacheKey != null) { + cache.put(cacheKey, modelClass != null ? modelClass : NO_MODEL_IMPLEMENTATION); + } + if (modelClass != null) { log.debug("Using implementation type {} for requested adapter type {}", modelClass, requestedType); return modelClass; } // throw exception here - throw new ModelClassException("Could not yet find an adapter factory for the model " + requestedType + throw newNoImplementationException(requestedType, adaptable); + } + + private ModelClassException newNoImplementationException(Class<?> requestedType, Object adaptable) { + return new ModelClassException("Could not yet find an adapter factory for the model " + requestedType + " from adaptable " + adaptable.getClass()); } + /** + * Returns the (resolver scoped) cache for implementation lookups, lazily creating it in the + * {@link ResourceResolver#getPropertyMap() property map} of the resolver backing the given adaptable. Returns + * {@code null} if no resource (and hence no resolver) can be derived from the adaptable. + * + * <p>The cache is a bounded LRU map: even for long-lived (e.g. service) resolvers or pathological numbers of + * distinct resource types, it never holds more than {@link #implementationLookupCacheSize} entries, evicting the + * least recently used ones. + */ + @SuppressWarnings("unchecked") + private @Nullable Map<String, Object> getImplementationLookupCache(Object adaptable) { + final Resource resource = getResourceFromAdaptable(adaptable); + if (resource == null) { + return null; + } + final ResourceResolver resolver = resource.getResourceResolver(); + if (resolver == null) { + return null; + } + final Map<String, Object> propertyMap = resolver.getPropertyMap(); + Map<String, Object> cache = (Map<String, Object>) propertyMap.get(IMPLEMENTATION_LOOKUP_CACHE_KEY); + if (cache == null) { + cache = createBoundedLookupCache(implementationLookupCacheSize); + propertyMap.put(IMPLEMENTATION_LOOKUP_CACHE_KEY, cache); + } + return cache; + } + + private static Map<String, Object> createBoundedLookupCache(final int maxSize) { + return Collections.synchronizedMap(new LinkedHashMap<String, Object>(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry<String, Object> eldest) { + return size() > maxSize; + } + }); + } + + /** + * Builds the cache key for an implementation lookup, or {@code null} if no resource type is available, in which case + * the lookup must not be cached. + */ + private @Nullable String getImplementationLookupCacheKey(Class<?> requestedType, Object adaptable) { + final Resource resource = getResourceFromAdaptable(adaptable); + if (resource == null) { + return null; + } + final String resourceType = resource.getResourceType(); + if (resourceType == null) { + return null; + } + return adaptable.getClass().getName() + '|' + requestedType.getName() + '|' + resourceType; + } + + @SuppressWarnings("deprecation") + private @Nullable Resource getResourceFromAdaptable(Object adaptable) { + if (adaptable instanceof Resource resource) { + return resource; + } else if (adaptable instanceof SlingJakartaHttpServletRequest jakartaRequest) { + return jakartaRequest.getResource(); + } else if (adaptable instanceof SlingHttpServletRequest javaxRequest) { + return javaxRequest.getResource(); + } + return null; + } + @SuppressWarnings("unchecked") private Map<Class<?>, SoftReference<Object>> getOrCreateCache(final Object adaptable) { Map<Class<?>, SoftReference<Object>> adaptableCache; @@ -1186,6 +1307,9 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto this.adapterCache = Collections.synchronizedMap(new WeakHashMap<Object, Map<Class<?>, SoftReference<Object>>>()); + this.cacheImplementationLookups = configuration.cache_implementation_lookups(); + this.implementationLookupCacheSize = Math.max(1, configuration.implementation_lookup_cache_size()); + BundleContext bundleContext = ctx.getBundleContext(); this.queue = new ReferenceQueue<>(); this.disposalCallbacks = new ConcurrentHashMap<>(); diff --git a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java index 86b25e1..e2c9f59 100644 --- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java +++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java @@ -30,4 +30,21 @@ public @interface ModelAdapterFactoryConfiguration { name = "Cleanup Job Period", description = "Period in seconds at which OSGi service references from ThreadLocals will be cleaned up.") long cleanup_job_period() default 30l; + + @AttributeDefinition( + name = "Cache Implementation Lookups", + description = "Cache the resolution of a model implementation class per adaptable type and resource type " + + "within a ResourceResolver. This avoids redundant repository lookups when the same model is " + + "resolved repeatedly (e.g. once for canCreateFromAdaptable and once for createModel, or across " + + "many components sharing a resource type). Disable this if a custom ImplementationPicker selects " + + "an implementation based on more than the resource type (e.g. selectors or request attributes).") + boolean cache_implementation_lookups() default true; + + @AttributeDefinition( + name = "Implementation Lookup Cache Size", + description = "Maximum number of entries in the per-ResourceResolver implementation lookup cache (see " + + "'Cache Implementation Lookups'). Once exceeded, the least recently used entries are evicted, so " + + "that the cache cannot grow unbounded for long-lived resolvers or large numbers of distinct " + + "resource types. Must be greater than 0.") + int implementation_lookup_cache_size() default 100; } diff --git a/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java b/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java index 003dd5f..1da4e48 100644 --- a/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java +++ b/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java @@ -80,13 +80,23 @@ public class AdapterFactoryTest { return createModelAdapterFactory(bundleContext); } + public static ModelAdapterFactory createModelAdapterFactory(boolean cacheImplementationLookups) { + return createModelAdapterFactory(Mockito.mock(BundleContext.class), cacheImplementationLookups); + } + public static ModelAdapterFactory createModelAdapterFactory(BundleContext bundleContext) { + return createModelAdapterFactory(bundleContext, true); + } + + public static ModelAdapterFactory createModelAdapterFactory( + BundleContext bundleContext, boolean cacheImplementationLookups) { ComponentContext componentCtx = Mockito.mock(ComponentContext.class); when(componentCtx.getBundleContext()).thenReturn(bundleContext); ModelAdapterFactory factory = new ModelAdapterFactory(); Converter c = Converters.standardConverter(); Map<String, String> map = new HashMap<>(); + map.put("cache.implementation.lookups", Boolean.toString(cacheImplementationLookups)); ModelAdapterFactoryConfiguration config = c.convert(map).to(ModelAdapterFactoryConfiguration.class); factory.activate(componentCtx, config); factory.injectAnnotationProcessorFactories = Collections.emptyList(); diff --git a/src/test/java/org/apache/sling/models/impl/CachingTest.java b/src/test/java/org/apache/sling/models/impl/CachingTest.java index b9217f3..8c46a60 100644 --- a/src/test/java/org/apache/sling/models/impl/CachingTest.java +++ b/src/test/java/org/apache/sling/models/impl/CachingTest.java @@ -39,6 +39,9 @@ import org.apache.sling.servlethelpers.MockSlingJakartaHttpServletRequest; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.Parameter; +import org.junit.jupiter.params.ParameterizedClass; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; @@ -50,9 +53,16 @@ import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +// run with the implementation-lookup cache (SLING-12217) both enabled and disabled - the model instance caching +// behavior asserted here must be identical regardless of that optimization +@ParameterizedClass +@ValueSource(booleans = {true, false}) @ExtendWith(MockitoExtension.class) class CachingTest { + @Parameter + private boolean cacheImplementationLookups; + @Spy private MockSlingJakartaHttpServletRequest request = new MockSlingJakartaHttpServletRequest(null); @@ -65,7 +75,7 @@ class CachingTest { @BeforeEach void setup() { - factory = AdapterFactoryTest.createModelAdapterFactory(); + factory = AdapterFactoryTest.createModelAdapterFactory(cacheImplementationLookups); factory.injectors = Arrays.asList(new RequestAttributeInjector(), new ValueMapInjector()); factory.adapterImplementations.addClassesAsAdapterAndImplementation( CachedModel.class, diff --git a/src/test/java/org/apache/sling/models/impl/ImplementationLookupCachingTest.java b/src/test/java/org/apache/sling/models/impl/ImplementationLookupCachingTest.java new file mode 100644 index 0000000..519c215 --- /dev/null +++ b/src/test/java/org/apache/sling/models/impl/ImplementationLookupCachingTest.java @@ -0,0 +1,245 @@ +/* + * 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.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.models.annotations.Model; +import org.apache.sling.models.spi.ImplementationPicker; +import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +import org.osgi.framework.BundleContext; +import org.osgi.service.component.ComponentContext; +import org.osgi.util.converter.Converters; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Tests for the per-{@link ResourceResolver} caching of model implementation class lookups (SLING-12217). + * + * <p>The lookup of the implementation class is the operation that - for resource-type-based adaptation - walks the + * resource super type hierarchy and therefore causes repository access. By counting how often the + * {@link ImplementationPicker} is consulted, these tests verify that the lookup is performed at most once per + * {@code (adaptable type, requested type, resource type)} within a single resolver. + */ +@ExtendWith(MockitoExtension.class) +class ImplementationLookupCachingTest { + + /** The single resolver shared by all resources, providing the (resolver-scoped) cache via its property map. */ + private ResourceResolver resolver; + + private final Map<String, Object> propertyMap = new HashMap<>(); + + /** Counts how often the implementation lookup actually consults the picker. */ + private final AtomicInteger pickCount = new AtomicInteger(); + + private final ImplementationPicker countingPicker = new ImplementationPicker() { + @Override + public Class<?> pick( + @NotNull Class<?> adapterType, Class<?> @NotNull [] implementationsTypes, @NotNull Object adaptable) { + pickCount.incrementAndGet(); + if (adapterType == TestAdapter.class) { + return TestModel.class; + } + // UnmatchedAdapter has registered implementations but the picker selects none -> negative result + return null; + } + }; + + @BeforeEach + void setUp() { + resolver = mock(ResourceResolver.class); + lenient().when(resolver.getPropertyMap()).thenReturn(propertyMap); + } + + private ModelAdapterFactory createFactory(boolean cacheLookups) { + return createFactory(cacheLookups, 100); + } + + private ModelAdapterFactory createFactory(boolean cacheLookups, int cacheSize) { + ComponentContext componentCtx = mock(ComponentContext.class); + BundleContext bundleContext = mock(BundleContext.class); + when(componentCtx.getBundleContext()).thenReturn(bundleContext); + + ModelAdapterFactory factory = new ModelAdapterFactory(); + Map<String, Object> config = new HashMap<>(); + config.put("cache.implementation.lookups", cacheLookups); + config.put("implementation.lookup.cache.size", cacheSize); + factory.activate( + componentCtx, + Converters.standardConverter().convert(config).to(ModelAdapterFactoryConfiguration.class)); + factory.injectAnnotationProcessorFactories = Collections.emptyList(); + factory.injectAnnotationProcessorFactories2 = Collections.emptyList(); + factory.injectors = Collections.emptyList(); + factory.implementationPickers = Collections.singletonList(countingPicker); + // register with an explicit adapter type so that the picker (and hence the lookup) is consulted + factory.adapterImplementations.addAll(TestModel.class, TestAdapter.class); + factory.adapterImplementations.addAll(UnmatchedModel.class, UnmatchedAdapter.class); + return factory; + } + + /** Size of the nested (bounded) lookup cache stored in the resolver property map, or 0 if none was created yet. */ + @SuppressWarnings("unchecked") + private int nestedCacheSize() { + if (propertyMap.isEmpty()) { + return 0; + } + return ((Map<String, Object>) propertyMap.values().iterator().next()).size(); + } + + private Resource resourceOfType(String resourceType) { + Resource resource = mock(Resource.class); + lenient().when(resource.getResourceType()).thenReturn(resourceType); + lenient().when(resource.getResourceResolver()).thenReturn(resolver); + return resource; + } + + @Test + void lookupIsCachedAcrossComponentsWithSameResourceType() { + ModelAdapterFactory factory = createFactory(true); + + TestAdapter first = factory.getAdapter(resourceOfType("my/type"), TestAdapter.class); + TestAdapter second = factory.getAdapter(resourceOfType("my/type"), TestAdapter.class); + + assertNotNull(first); + assertNotNull(second); + assertEquals(1, pickCount.get(), "the implementation lookup must run only once for a given resource type"); + } + + @Test + void canCreateAndCreateShareTheCachedLookup() { + ModelAdapterFactory factory = createFactory(true); + Resource resource = resourceOfType("my/type"); + + // this is the JavaUseProvider pattern: guard with canCreateFromAdaptable, then createModel + assertTrue(factory.canCreateFromAdaptable(resource, TestAdapter.class)); + TestAdapter model = factory.createModel(resource, TestAdapter.class); + + assertNotNull(model); + assertEquals(1, pickCount.get(), "canCreateFromAdaptable and createModel must not look up the type twice"); + } + + @Test + void differentResourceTypesAreNotSharedAndResolveCorrectly() { + ModelAdapterFactory factory = createFactory(true); + + TestAdapter a = factory.getAdapter(resourceOfType("type/a"), TestAdapter.class); + TestAdapter b = factory.getAdapter(resourceOfType("type/b"), TestAdapter.class); + + assertNotNull(a); + assertNotNull(b); + assertEquals(2, pickCount.get(), "different resource types must each trigger their own lookup"); + } + + @Test + void negativeResultIsCached() { + ModelAdapterFactory factory = createFactory(true); + + // UnmatchedAdapter has registered implementations but the picker returns none -> no model + assertFalse(factory.canCreateFromAdaptable(resourceOfType("my/type"), UnmatchedAdapter.class)); + assertFalse(factory.canCreateFromAdaptable(resourceOfType("my/type"), UnmatchedAdapter.class)); + + assertEquals(1, pickCount.get(), "a negative lookup result must be memoized as well"); + } + + @Test + void freshResolverDoesNotShareTheCache() { + ModelAdapterFactory factory = createFactory(true); + + factory.getAdapter(resourceOfType("my/type"), TestAdapter.class); + + // a second resolver has its own property map -> the cache does not carry over + ResourceResolver otherResolver = mock(ResourceResolver.class); + when(otherResolver.getPropertyMap()).thenReturn(new HashMap<>()); + Resource otherResource = mock(Resource.class); + when(otherResource.getResourceType()).thenReturn("my/type"); + when(otherResource.getResourceResolver()).thenReturn(otherResolver); + + factory.getAdapter(otherResource, TestAdapter.class); + + assertEquals(2, pickCount.get(), "a different resolver must not reuse the cached lookup"); + } + + @Test + void leastRecentlyUsedEntryIsEvictedBeyondTheCap() { + ModelAdapterFactory factory = createFactory(true, 2); + + factory.getAdapter(resourceOfType("type/a"), TestAdapter.class); // miss -> cache [a] + factory.getAdapter(resourceOfType("type/b"), TestAdapter.class); // miss -> cache [a, b] + factory.getAdapter(resourceOfType("type/c"), TestAdapter.class); // miss -> evicts a -> cache [b, c] + assertEquals(3, pickCount.get()); + + // type/a was evicted, so it must be looked up again + factory.getAdapter(resourceOfType("type/a"), TestAdapter.class); // miss -> evicts b -> cache [c, a] + assertEquals(4, pickCount.get(), "evicted entries must be recomputed"); + + // type/c is still cached and was made most-recently-used above, so no new lookup + factory.getAdapter(resourceOfType("type/c"), TestAdapter.class); + assertEquals(4, pickCount.get(), "a still-cached entry must not be looked up again"); + + assertEquals(2, nestedCacheSize(), "the cache must never exceed its configured size"); + } + + @Test + void longLivedResolverCacheStaysBounded() { + ModelAdapterFactory factory = createFactory(true, 3); + + for (int i = 0; i < 50; i++) { + factory.getAdapter(resourceOfType("type/" + i), TestAdapter.class); + } + + assertEquals(50, pickCount.get(), "every distinct resource type triggers a lookup once"); + assertEquals(3, nestedCacheSize(), "the cache must stay bounded regardless of resolver lifetime"); + } + + @Test + void cachingCanBeDisabled() { + ModelAdapterFactory factory = createFactory(false); + + factory.getAdapter(resourceOfType("my/type"), TestAdapter.class); + factory.getAdapter(resourceOfType("my/type"), TestAdapter.class); + + assertEquals(2, pickCount.get(), "with caching disabled the lookup runs on every adaptation"); + assertTrue(propertyMap.isEmpty(), "no cache entries must be written when caching is disabled"); + } + + public interface TestAdapter {} + + @Model(adaptables = Resource.class, adapters = TestAdapter.class) + public static class TestModel implements TestAdapter {} + + public interface UnmatchedAdapter {} + + @Model(adaptables = Resource.class, adapters = UnmatchedAdapter.class) + public static class UnmatchedModel implements UnmatchedAdapter {} +} diff --git a/src/test/java/org/apache/sling/models/impl/ImplementsExtendsTest.java b/src/test/java/org/apache/sling/models/impl/ImplementsExtendsTest.java index fe70e0a..6d6110b 100644 --- a/src/test/java/org/apache/sling/models/impl/ImplementsExtendsTest.java +++ b/src/test/java/org/apache/sling/models/impl/ImplementsExtendsTest.java @@ -47,6 +47,9 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.Parameter; +import org.junit.jupiter.params.ParameterizedClass; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.jupiter.MockitoExtension; @@ -67,9 +70,16 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; +// run with the implementation-lookup cache (SLING-12217) both enabled and disabled - resolving the alternate +// implementation class via the picker is exactly the path that gets cached, so its result must not change +@ParameterizedClass +@ValueSource(booleans = {true, false}) @ExtendWith(MockitoExtension.class) class ImplementsExtendsTest { + @Parameter + private boolean cacheImplementationLookups; + @Mock private BundleContext bundleContext; @@ -109,7 +119,7 @@ class ImplementsExtendsTest { } }); - factory = AdapterFactoryTest.createModelAdapterFactory(bundleContext); + factory = AdapterFactoryTest.createModelAdapterFactory(bundleContext, cacheImplementationLookups); factory.injectors = Collections.singletonList(new ValueMapInjector()); factory.implementationPickers = Collections.singletonList(firstImplementationPicker); diff --git a/src/test/java/org/apache/sling/models/impl/ModelAdapterFactory_ImplementationPickerOrderTest.java b/src/test/java/org/apache/sling/models/impl/ModelAdapterFactory_ImplementationPickerOrderTest.java index 991aca1..1200934 100644 --- a/src/test/java/org/apache/sling/models/impl/ModelAdapterFactory_ImplementationPickerOrderTest.java +++ b/src/test/java/org/apache/sling/models/impl/ModelAdapterFactory_ImplementationPickerOrderTest.java @@ -18,6 +18,7 @@ */ package org.apache.sling.models.impl; +import java.util.Map; import java.util.function.IntSupplier; import org.apache.sling.api.SlingJakartaHttpServletRequest; @@ -32,6 +33,9 @@ import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.Parameter; +import org.junit.jupiter.params.ParameterizedClass; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -41,10 +45,18 @@ import static org.osgi.framework.Constants.SERVICE_RANKING; /** * Tests in which order the implementation pickers are handled depending on service ranking. * For historic/backwards compatibility reasons, higher ranking value means lower priority (inverse to DS behavior). + * + * <p>Run with the implementation-lookup cache (SLING-12217) both enabled and disabled, as consulting the pickers is + * exactly the operation that gets cached - the picked implementation must be the same in both cases. */ +@ParameterizedClass +@ValueSource(booleans = {true, false}) @ExtendWith({OsgiContextExtension.class, MockitoExtension.class}) class ModelAdapterFactory_ImplementationPickerOrderTest { + @Parameter + boolean cacheImplementationLookups; + final OsgiContext context = new OsgiContext(); @Mock @@ -62,7 +74,8 @@ class ModelAdapterFactory_ImplementationPickerOrderTest { void setUp() { context.registerService(BindingsValuesProvidersByContext.class, bindingsValuesProvidersByContext); context.registerService(AdapterManager.class, adapterManager); - factory = context.registerInjectActivateService(ModelAdapterFactory.class); + factory = context.registerInjectActivateService( + ModelAdapterFactory.class, Map.of("cache.implementation.lookups", cacheImplementationLookups)); ModelAdapterFactoryUtil.addModelsForPackage(context.bundleContext(), Model1.class, Model2.class); }
