Revise RepositoryInformation and RepositoryComposition caching.

We now use a refined strategy to cache RepositoryInformation and RepositoryComposition.

Previously, RepositoryComposition wasn't cached at all and store modules that e.g. contributed a Querydsl (or a different) fragment based on the interface declaration returned a new RepositoryComposition (and thus a different hashCode) each time RepositoryInformation was obtained leading to memory leaks caused by HashMap caching.

We now use Fragment's hashCode for the cache key resulting in RepositoryComposition being created only once for a given repository interface and input-fragments arrangement.

Closes #3252

Author: mp911de

src/main/java/org/springframework/data/repository/core/support/RepositoryComposition.java

@@ -541,6 +541,10 @@ private static Method findMethod(InvokedMethod invokedMethod, MethodLookup looku
 			return null;
 		}
 
+		List<RepositoryFragment<?>> getFragments() {
+			return fragments;
+		}
+
 		/**
 		 * Returns the number of {@link RepositoryFragment fragments} available.
 		 *
@@ -585,5 +589,6 @@ public int hashCode() {
 			result = (31 * result) + ObjectUtils.nullSafeHashCode(fragments);
 			return result;
 		}
+
 	}
 }

src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java

@@ -110,7 +110,7 @@ public abstract class RepositoryFactorySupport
 		CONVERSION_SERVICE.removeConvertible(Object.class, Object.class);
 	}
 
-	private final Map<RepositoryInformationCacheKey, RepositoryInformation> repositoryInformationCache;
+	private final Map<RepositoryInformationCacheKey, RepositorySub> repositoryInformationCache;
 	private final List<RepositoryProxyPostProcessor> postProcessors;
 
 	private @Nullable Class<?> repositoryBaseClass;
@@ -130,7 +130,7 @@ public abstract class RepositoryFactorySupport
 	@SuppressWarnings("null")
 	public RepositoryFactorySupport() {
 
-		this.repositoryInformationCache = new HashMap<>(16);
+		this.repositoryInformationCache = new HashMap<>(8);
 		this.postProcessors = new ArrayList<>();
 
 		this.namedQueries = PropertiesBasedNamedQueries.EMPTY;
@@ -294,16 +294,6 @@ protected RepositoryFragments getRepositoryFragments(RepositoryMetadata metadata
 		return RepositoryFragments.empty();
 	}
 
-	/**
-	 * Creates {@link RepositoryComposition} based on {@link RepositoryMetadata} for repository-specific method handling.
-	 *
-	 * @param metadata the repository metadata to use.
-	 * @return repository composition.
-	 */
-	private RepositoryComposition getRepositoryComposition(RepositoryMetadata metadata) {
-		return RepositoryComposition.fromMetadata(metadata);
-	}
-
 	/**
 	 * Returns a repository instance for the given interface.
 	 *
@@ -362,8 +352,9 @@ public <T> T getRepository(Class<T> repositoryInterface, RepositoryFragments fra
 				repositoryInterface);
 		repositoryCompositionStep.tag("fragment.count", String.valueOf(fragments.size()));
 
-		RepositoryComposition composition = getRepositoryComposition(metadata, fragments);
-		RepositoryInformation information = getRepositoryInformation(metadata, composition);
+		RepositorySub stub = getRepositoryStub(metadata, fragments);
+		RepositoryComposition composition = stub.composition();
+		RepositoryInformation information = stub.information();
 
 		repositoryCompositionStep.tag("fragments", () -> {
 
@@ -493,47 +484,35 @@ protected RepositoryMetadata getRepositoryMetadata(Class<?> repositoryInterface)
 	 * @return will never be {@literal null}.
 	 */
 	protected RepositoryInformation getRepositoryInformation(RepositoryMetadata metadata, RepositoryFragments fragments) {
-		return getRepositoryInformation(metadata, getRepositoryComposition(metadata, fragments));
+		return getRepositoryStub(metadata, fragments).information();
 	}
 
 	/**
-	 * Returns the {@link RepositoryComposition} for the given {@link RepositoryMetadata} and extra
-	 * {@link RepositoryFragments}.
-	 *
-	 * @param metadata must not be {@literal null}.
-	 * @param fragments must not be {@literal null}.
-	 * @return will never be {@literal null}.
-	 */
-	private RepositoryComposition getRepositoryComposition(RepositoryMetadata metadata, RepositoryFragments fragments) {
-
-		Assert.notNull(metadata, "RepositoryMetadata must not be null");
-		Assert.notNull(fragments, "RepositoryFragments must not be null");
-
-		RepositoryComposition composition = getRepositoryComposition(metadata);
-		RepositoryFragments repositoryAspects = getRepositoryFragments(metadata);
-
-		return composition.append(fragments).append(repositoryAspects);
-	}
-
-	/**
-	 * Returns the {@link RepositoryInformation} for the given repository interface.
+	 * Returns the cached {@link RepositorySub} for the given repository and composition. {@link RepositoryMetadata} is a
+	 * strong cache key while {@link RepositoryFragments} contributes a light-weight caching component by using only the
+	 * fragments hash code. In a typical Spring scenario, that shouldn't impose issues as one repository factory produces
+	 * only a single repository instance for one repository interface. Things might be different when using various
+	 * fragments for the same repository interface.
 	 *
 	 * @param metadata
-	 * @param composition
+	 * @param fragments
 	 * @return
 	 */
-	private RepositoryInformation getRepositoryInformation(RepositoryMetadata metadata,
-			RepositoryComposition composition) {
+	private RepositorySub getRepositoryStub(RepositoryMetadata metadata, RepositoryFragments fragments) {
 
-		RepositoryInformationCacheKey cacheKey = new RepositoryInformationCacheKey(metadata, composition);
+		RepositoryInformationCacheKey cacheKey = new RepositoryInformationCacheKey(metadata, fragments);
 
 		synchronized (repositoryInformationCache) {
 
 			return repositoryInformationCache.computeIfAbsent(cacheKey, key -> {
 
+				RepositoryComposition composition = RepositoryComposition.fromMetadata(metadata);
+				RepositoryFragments repositoryAspects = getRepositoryFragments(metadata);
+				composition = composition.append(fragments).append(repositoryAspects);
+
 				Class<?> baseClass = repositoryBaseClass != null ? repositoryBaseClass : getRepositoryBaseClass(metadata);
 
-				return new DefaultRepositoryInformation(metadata, baseClass, composition);
+				return new RepositorySub(new DefaultRepositoryInformation(metadata, baseClass, composition), composition);
 			});
 		}
 	}
@@ -816,6 +795,18 @@ public List<QueryMethod> getQueryMethods() {
 		}
 	}
 
+	/**
+	 * Repository stub holding {@link RepositoryInformation} and {@link RepositoryComposition}.
+	 *
+	 * @param information
+	 * @param composition
+	 * @author Mark Paluch
+	 * @since 3.4.4
+	 */
+	record RepositorySub(RepositoryInformation information, RepositoryComposition composition) {
+
+	}
+
 	/**
 	 * Simple value object to build up keys to cache {@link RepositoryInformation} instances.
 	 *
@@ -825,31 +816,26 @@ public List<QueryMethod> getQueryMethods() {
 	private static final class RepositoryInformationCacheKey {
 
 		private final String repositoryInterfaceName;
-		private final long compositionHash;
+		private final long fragmentsHash;
 
 		/**
-		 * Creates a new {@link RepositoryInformationCacheKey} for the given {@link RepositoryMetadata} and composition.
+		 * Creates a new {@link RepositoryInformationCacheKey} for the given {@link RepositoryMetadata} and fragments.
 		 *
 		 * @param metadata must not be {@literal null}.
-		 * @param composition must not be {@literal null}.
+		 * @param fragments must not be {@literal null}.
 		 */
-		public RepositoryInformationCacheKey(RepositoryMetadata metadata, RepositoryComposition composition) {
+		public RepositoryInformationCacheKey(RepositoryMetadata metadata, RepositoryFragments fragments) {
 
 			this.repositoryInterfaceName = metadata.getRepositoryInterface().getName();
-			this.compositionHash = composition.hashCode();
-		}
-
-		public RepositoryInformationCacheKey(String repositoryInterfaceName, long compositionHash) {
-			this.repositoryInterfaceName = repositoryInterfaceName;
-			this.compositionHash = compositionHash;
+			this.fragmentsHash = fragments.getFragments().hashCode();
 		}
 
 		public String getRepositoryInterfaceName() {
 			return this.repositoryInterfaceName;
 		}
 
-		public long getCompositionHash() {
-			return this.compositionHash;
+		public long getFragmentsHash() {
+			return this.fragmentsHash;
 		}
 
 		@Override
@@ -860,7 +846,7 @@ public boolean equals(Object o) {
 			if (!(o instanceof RepositoryInformationCacheKey that)) {
 				return false;
 			}
-			if (compositionHash != that.compositionHash) {
+			if (fragmentsHash != that.fragmentsHash) {
 				return false;
 			}
 			return ObjectUtils.nullSafeEquals(repositoryInterfaceName, that.repositoryInterfaceName);
@@ -869,14 +855,14 @@ public boolean equals(Object o) {
 		@Override
 		public int hashCode() {
 			int result = ObjectUtils.nullSafeHashCode(repositoryInterfaceName);
-			result = 31 * result + (int) (compositionHash ^ (compositionHash >>> 32));
+			result = 31 * result + Long.hashCode(fragmentsHash);
 			return result;
 		}
 
 		@Override
 		public String toString() {
 			return "RepositoryFactorySupport.RepositoryInformationCacheKey(repositoryInterfaceName="
-					+ this.getRepositoryInterfaceName() + ", compositionHash=" + this.getCompositionHash() + ")";
+					+ this.getRepositoryInterfaceName() + ", fragmentsHash=" + this.getFragmentsHash() + ")";
 		}
 	}
 

src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java

@@ -23,6 +23,7 @@
 
 import org.mockito.ArgumentMatchers;
 import org.mockito.Mockito;
+
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.core.metrics.ApplicationStartup;
 import org.springframework.core.metrics.StartupStep;
@@ -103,7 +104,7 @@ protected RepositoryFragments getRepositoryFragments(RepositoryMetadata metadata
 		var fragments = super.getRepositoryFragments(metadata);
 
 		return QuerydslPredicateExecutor.class.isAssignableFrom(metadata.getRepositoryInterface()) //
-				? fragments.append(RepositoryFragments.just(querydsl)) //
+				? fragments.append(RepositoryFragments.just(querydsl, new Object())) //
 				: fragments;
 	}
 

src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java

@@ -24,6 +24,7 @@
 import java.lang.reflect.Method;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
@@ -293,6 +294,26 @@ record Metadata(RepositoryMethodContext context, MethodInvocation methodInvocati
 		assertThat(metadata.methodInvocation().getMethod().getName()).isEqualTo("findMetadataByLastname");
 	}
 
+	@Test
+	void cachesRepositoryInformation() {
+
+		var repository1 = factory.getRepository(ObjectAndQuerydslRepository.class, backingRepo);
+		var repository2 = factory.getRepository(ObjectAndQuerydslRepository.class, backingRepo);
+		repository1.findByFoo();
+		repository2.deleteAll();
+
+		for (int i = 0; i < 10; i++) {
+			RepositoryFragments fragments = RepositoryFragments.just(backingRepo);
+			RepositoryMetadata metadata = factory.getRepositoryMetadata(ObjectAndQuerydslRepository.class);
+			factory.getRepositoryInformation(metadata, fragments);
+		}
+
+		Map<Object, RepositoryInformation> cache = (Map) ReflectionTestUtils.getField(factory,
+				"repositoryInformationCache");
+
+		assertThat(cache).hasSize(1);
+	}
+
 	@Test // DATACMNS-509, DATACMNS-1764
 	void convertsWithSameElementType() {
 
@@ -544,6 +565,10 @@ private void expect(Future<?> future, Object value) throws Exception {
 
 	interface SimpleRepository extends Repository<Object, Serializable> {}
 
+	interface ObjectAndQuerydslRepository extends ObjectRepository, QuerydslPredicateExecutor<Object> {
+
+	}
+
 	interface ObjectRepository extends Repository<Object, Object>, ObjectRepositoryCustom {
 
 		@Nullable