From 59909d8f469bba4305ccacb704444a322bf6910e Mon Sep 17 00:00:00 2001 From: LordKay-sudo Date: Mon, 29 Jun 2026 15:36:51 +0200 Subject: [PATCH] Cache MetadataReader by class name in CachingMetadataReaderFactory Avoid retaining duplicate annotation metadata when the same .class file is discovered through different Resource representations during classpath scanning (for example, ClassPathResource vs file URL paths). Adds regression tests that reproduce gh-36737. Closes gh-36737 Signed-off-by: LordKay-sudo --- .../CachingMetadataReaderFactory.java | 205 ++++++++++++++++-- .../CachingMetadataReaderFactoryTests.java | 80 +++++++ 2 files changed, 266 insertions(+), 19 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/type/classreading/CachingMetadataReaderFactory.java b/spring-core/src/main/java/org/springframework/core/type/classreading/CachingMetadataReaderFactory.java index 7e5e423fb15b..e5899df6e4a0 100644 --- a/spring-core/src/main/java/org/springframework/core/type/classreading/CachingMetadataReaderFactory.java +++ b/spring-core/src/main/java/org/springframework/core/type/classreading/CachingMetadataReaderFactory.java @@ -17,20 +17,27 @@ package org.springframework.core.type.classreading; import java.io.IOException; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import org.jspecify.annotations.Nullable; +import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.DefaultResourceLoader; import org.springframework.core.io.Resource; import org.springframework.core.io.ResourceLoader; +import org.springframework.util.ClassUtils; +import org.springframework.util.ResourceUtils; /** * Caching implementation of the {@link MetadataReaderFactory} interface, - * caching a {@link MetadataReader} instance per Spring {@link Resource} handle - * (i.e. per ".class" file). + * caching a {@link MetadataReader} instance per class name. + *

Metadata is also associated with the originating {@link Resource} handle + * in the shared {@link DefaultResourceLoader} resource cache when available. * * @author Juergen Hoeller * @author Costin Leau @@ -42,8 +49,15 @@ public class CachingMetadataReaderFactory extends AbstractMetadataReaderFactory /** Default maximum number of entries for a local MetadataReader cache: 256. */ public static final int DEFAULT_CACHE_LIMIT = 256; + private static final String CLASSES_DIR = "/classes/"; + + private static final String TEST_CLASSES_DIR = "/test-classes/"; + private final MetadataReaderFactory delegate; + /** MetadataReader cache keyed by class name. */ + private @Nullable Map classNameMetadataReaderCache; + /** MetadataReader cache: either local or shared at the ResourceLoader level. */ private @Nullable Map metadataReaderCache; @@ -81,6 +95,7 @@ public CachingMetadataReaderFactory(@Nullable ResourceLoader resourceLoader) { this.delegate = delegate; if (getResourceLoader() instanceof DefaultResourceLoader defaultResourceLoader) { this.metadataReaderCache = defaultResourceLoader.getResourceCache(MetadataReader.class); + this.classNameMetadataReaderCache = new ConcurrentHashMap<>(); } else { setCacheLimit(DEFAULT_CACHE_LIMIT); @@ -97,12 +112,17 @@ public CachingMetadataReaderFactory(@Nullable ResourceLoader resourceLoader) { public void setCacheLimit(int cacheLimit) { if (cacheLimit <= 0) { this.metadataReaderCache = null; + this.classNameMetadataReaderCache = null; } else if (this.metadataReaderCache instanceof LocalResourceCache localResourceCache) { localResourceCache.setCacheLimit(cacheLimit); + if (this.classNameMetadataReaderCache instanceof LocalClassNameCache localClassNameCache) { + localClassNameCache.setCacheLimit(cacheLimit); + } } else { this.metadataReaderCache = new LocalResourceCache(cacheLimit); + this.classNameMetadataReaderCache = new LocalClassNameCache(cacheLimit); } } @@ -120,34 +140,49 @@ public int getCacheLimit() { @Override public MetadataReader getMetadataReader(Resource resource) throws IOException { - if (this.metadataReaderCache instanceof ConcurrentMap) { - // No synchronization necessary... - MetadataReader metadataReader = this.metadataReaderCache.get(resource); - if (metadataReader == null) { - metadataReader = this.delegate.getMetadataReader(resource); - this.metadataReaderCache.put(resource, metadataReader); + @Nullable String className = resolveClassName(resource); + if (className != null) { + MetadataReader metadataReader = getFromClassNameCache(className); + if (metadataReader != null) { + cacheMetadataReader(resource, metadataReader); + return metadataReader; } - return metadataReader; } - else if (this.metadataReaderCache != null) { - synchronized (this.metadataReaderCache) { - MetadataReader metadataReader = this.metadataReaderCache.get(resource); - if (metadataReader == null) { - metadataReader = this.delegate.getMetadataReader(resource); - this.metadataReaderCache.put(resource, metadataReader); - } - return metadataReader; + + MetadataReader metadataReader = getFromResourceCache(resource); + if (metadataReader != null) { + if (className != null) { + cacheMetadataReaderByClassName(className, metadataReader); } + return metadataReader; } - else { - return this.delegate.getMetadataReader(resource); + + metadataReader = this.delegate.getMetadataReader(resource); + if (className == null) { + className = metadataReader.getClassMetadata().getClassName(); } + MetadataReader existing = getFromClassNameCache(className); + if (existing != null) { + cacheMetadataReader(resource, existing); + return existing; + } + cacheMetadataReader(resource, metadataReader); + cacheMetadataReaderByClassName(className, metadataReader); + return metadataReader; } /** * Clear the local MetadataReader cache, if any, removing all cached class metadata. */ public void clearCache() { + if (this.classNameMetadataReaderCache instanceof LocalClassNameCache) { + synchronized (this.classNameMetadataReaderCache) { + this.classNameMetadataReaderCache.clear(); + } + } + else if (this.classNameMetadataReaderCache != null) { + this.classNameMetadataReaderCache.clear(); + } if (this.metadataReaderCache instanceof LocalResourceCache) { synchronized (this.metadataReaderCache) { this.metadataReaderCache.clear(); @@ -159,6 +194,114 @@ else if (this.metadataReaderCache != null) { } } + private @Nullable MetadataReader getFromClassNameCache(String className) { + if (this.classNameMetadataReaderCache instanceof ConcurrentMap concurrentMap) { + return concurrentMap.get(className); + } + else if (this.classNameMetadataReaderCache != null) { + synchronized (this.classNameMetadataReaderCache) { + return this.classNameMetadataReaderCache.get(className); + } + } + return null; + } + + private @Nullable MetadataReader getFromResourceCache(Resource resource) { + if (this.metadataReaderCache instanceof ConcurrentMap concurrentMap) { + return concurrentMap.get(resource); + } + else if (this.metadataReaderCache != null) { + synchronized (this.metadataReaderCache) { + return this.metadataReaderCache.get(resource); + } + } + return null; + } + + private void cacheMetadataReader(Resource resource, MetadataReader metadataReader) { + if (this.metadataReaderCache instanceof ConcurrentMap concurrentMap) { + concurrentMap.putIfAbsent(resource, metadataReader); + } + else if (this.metadataReaderCache != null) { + synchronized (this.metadataReaderCache) { + this.metadataReaderCache.putIfAbsent(resource, metadataReader); + } + } + } + + private void cacheMetadataReaderByClassName(MetadataReader metadataReader) { + cacheMetadataReaderByClassName(metadataReader.getClassMetadata().getClassName(), metadataReader); + } + + private void cacheMetadataReaderByClassName(String className, MetadataReader metadataReader) { + if (this.classNameMetadataReaderCache instanceof ConcurrentMap concurrentMap) { + concurrentMap.putIfAbsent(className, metadataReader); + } + else if (this.classNameMetadataReaderCache != null) { + synchronized (this.classNameMetadataReaderCache) { + this.classNameMetadataReaderCache.putIfAbsent(className, metadataReader); + } + } + } + + static @Nullable String resolveClassName(Resource resource) { + if (resource instanceof ClassPathResource classPathResource) { + return toClassName(classPathResource.getPath()); + } + try { + String urlString = resource.getURL().toString(); + int separatorIndex = urlString.indexOf(ResourceUtils.JAR_URL_SEPARATOR); + String path = (separatorIndex >= 0 ? + urlString.substring(separatorIndex + ResourceUtils.JAR_URL_SEPARATOR.length()) : + resource.getURL().getPath()); + path = URLDecoder.decode(path, StandardCharsets.UTF_8); + if (path.startsWith("/")) { + path = path.substring(1); + } + return toClassName(extractClassFileResourcePath(path)); + } + catch (IOException ex) { + return null; + } + } + + private static final String[] CLASS_FILE_ROOT_MARKERS = { + "/classes/java/test/", "/classes/java/main/", + "/classes/kotlin/test/", "/classes/kotlin/main/", + CLASSES_DIR, TEST_CLASSES_DIR + }; + + private static String extractClassFileResourcePath(String path) { + int classFileIndex = path.indexOf(ClassUtils.CLASS_FILE_SUFFIX); + if (classFileIndex < 0) { + return path; + } + String beforeSuffix = path.substring(0, classFileIndex); + int bestStart = -1; + for (String marker : CLASS_FILE_ROOT_MARKERS) { + int index = beforeSuffix.indexOf(marker); + if (index >= 0) { + int start = index + marker.length(); + if (start > bestStart) { + bestStart = start; + } + } + } + if (bestStart >= 0) { + return path.substring(bestStart, classFileIndex + ClassUtils.CLASS_FILE_SUFFIX.length()); + } + return path; + } + + private static @Nullable String toClassName(String resourcePath) { + if (!resourcePath.endsWith(ClassUtils.CLASS_FILE_SUFFIX)) { + return null; + } + String pathWithoutSuffix = resourcePath.substring(0, + resourcePath.length() - ClassUtils.CLASS_FILE_SUFFIX.length()); + return ClassUtils.convertResourcePathToClassName(pathWithoutSuffix); + } + @SuppressWarnings("serial") private static class LocalResourceCache extends LinkedHashMap { @@ -184,4 +327,28 @@ protected boolean removeEldestEntry(Map.Entry eldest) } } + @SuppressWarnings("serial") + private static class LocalClassNameCache extends LinkedHashMap { + + private volatile int cacheLimit; + + public LocalClassNameCache(int cacheLimit) { + super(cacheLimit, 0.75f, true); + this.cacheLimit = cacheLimit; + } + + public void setCacheLimit(int cacheLimit) { + this.cacheLimit = cacheLimit; + } + + public int getCacheLimit() { + return this.cacheLimit; + } + + @Override + protected boolean removeEldestEntry(Map.Entry eldest) { + return size() > this.cacheLimit; + } + } + } diff --git a/spring-core/src/test/java/org/springframework/core/type/classreading/CachingMetadataReaderFactoryTests.java b/spring-core/src/test/java/org/springframework/core/type/classreading/CachingMetadataReaderFactoryTests.java index e0a36c2c00a1..260843644587 100644 --- a/spring-core/src/test/java/org/springframework/core/type/classreading/CachingMetadataReaderFactoryTests.java +++ b/spring-core/src/test/java/org/springframework/core/type/classreading/CachingMetadataReaderFactoryTests.java @@ -16,10 +16,19 @@ package org.springframework.core.type.classreading; +import java.net.URL; +import java.util.Map; + import org.junit.jupiter.api.Test; +import org.springframework.core.io.ClassPathResource; +import org.springframework.core.io.DefaultResourceLoader; import org.springframework.core.io.Resource; +import org.springframework.core.io.UrlResource; +import org.springframework.core.type.ClassMetadata; +import org.springframework.util.ClassUtils; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -43,6 +52,77 @@ void shouldCacheClassNameCalls() throws Exception { verify(delegate, times(1)).getMetadataReader(any(Resource.class)); } + /** + * Reproduces gh-36737: the same class loaded via ClassPathResource and UrlResource + * must not create duplicate MetadataReader instances. + */ + @Test + void shouldNotDuplicateMetadataForSameClassFromDifferentResources() throws Exception { + MetadataReaderFactory delegate = mock(MetadataReaderFactory.class); + when(delegate.getMetadataReader(any(Resource.class))).thenAnswer(invocation -> { + Resource resource = invocation.getArgument(0); + MetadataReader reader = mock(MetadataReader.class); + ClassMetadata classMetadata = mock(ClassMetadata.class); + when(classMetadata.getClassName()).thenReturn(TestClass.class.getName()); + when(reader.getClassMetadata()).thenReturn(classMetadata); + when(reader.getResource()).thenReturn(resource); + return reader; + }); + + CachingMetadataReaderFactory readerFactory = new CachingMetadataReaderFactory(delegate); + + String classFilePath = ClassUtils.convertClassNameToResourcePath(TestClass.class.getName()) + + ClassUtils.CLASS_FILE_SUFFIX; + Resource classpathResource = new ClassPathResource(classFilePath); + URL url = TestClass.class.getResource('/' + classFilePath); + assertThat(url).isNotNull(); + Resource urlResource = new UrlResource(url); + + readerFactory.getMetadataReader(classpathResource); + readerFactory.getMetadataReader(urlResource); + + verify(delegate, times(1)).getMetadataReader(any(Resource.class)); + } + + @Test + void shouldResolveClassNameFromDifferentResourceRepresentations() throws Exception { + String classFilePath = ClassUtils.convertClassNameToResourcePath(TestClass.class.getName()) + + ClassUtils.CLASS_FILE_SUFFIX; + Resource classpathResource = new ClassPathResource(classFilePath); + URL url = TestClass.class.getResource('/' + classFilePath); + assertThat(url).isNotNull(); + Resource urlResource = new UrlResource(url); + + assertThat(CachingMetadataReaderFactory.resolveClassName(classpathResource)) + .isEqualTo(TestClass.class.getName()); + assertThat(CachingMetadataReaderFactory.resolveClassName(urlResource)) + .isEqualTo(TestClass.class.getName()); + } + + @Test + void shouldReuseMetadataReaderForSameClassFromDifferentResources() throws Exception { + String classFilePath = ClassUtils.convertClassNameToResourcePath(TestClass.class.getName()) + + ClassUtils.CLASS_FILE_SUFFIX; + Resource classpathResource = new ClassPathResource(classFilePath); + URL url = TestClass.class.getResource('/' + classFilePath); + assertThat(url).isNotNull(); + Resource urlResource = new UrlResource(url); + + DefaultResourceLoader resourceLoader = new DefaultResourceLoader(); + CachingMetadataReaderFactory readerFactory = new CachingMetadataReaderFactory(resourceLoader); + MetadataReader fromClasspath = readerFactory.getMetadataReader(classpathResource); + MetadataReader fromUrl = readerFactory.getMetadataReader(urlResource); + + assertThat(fromUrl).isSameAs(fromClasspath); + + Map sharedCache = resourceLoader.getResourceCache(MetadataReader.class); + long distinctReadersForSameClass = sharedCache.values().stream() + .filter(reader -> TestClass.class.getName().equals(reader.getClassMetadata().getClassName())) + .distinct() + .count(); + assertThat(distinctReadersForSameClass).isOne(); + } + public static class TestClass { }