Skip to content

Commit f86c19c

Browse files
christophstroblchristophstrobl
committed
Consider null values when caching queries.
1 parent 09a99df commit f86c19c

File tree

7 files changed

+330
-44
lines changed

7 files changed

+330
-44
lines changed

‎spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/PartTreeJpaQuery.java

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -210,12 +210,7 @@ private static boolean expectsCollection(Type type) {
210210
*/
211211
private class QueryPreparer {
212212

213-
private final Map<Sort, JpqlQueryCreator> cache = new LinkedHashMap<Sort, JpqlQueryCreator>() {
214-
@Override
215-
protected boolean removeEldestEntry(Map.Entry<Sort, JpqlQueryCreator> eldest) {
216-
return size() > 256;
217-
}
218-
};
213+
private final PartTreeQueryCache cache = new PartTreeQueryCache();
219214

220215
/**
221216
* Creates a new {@link Query} for the given parameter values.
@@ -279,7 +274,7 @@ private Query restrictMaxResultsIfNecessary(Query query, @Nullable ScrollPositio
279274
protected JpqlQueryCreator createCreator(Sort sort, JpaParametersParameterAccessor accessor) {
280275

281276
synchronized (cache) {
282-
JpqlQueryCreator jpqlQueryCreator = cache.get(sort);
277+
JpqlQueryCreator jpqlQueryCreator = cache.get(sort, accessor); // this caching thingy is broken due to IS NULL rendering for simple properties
283278
if (jpqlQueryCreator != null) {
284279
return jpqlQueryCreator;
285280
}
@@ -304,7 +299,7 @@ protected JpqlQueryCreator createCreator(Sort sort, JpaParametersParameterAccess
304299
}
305300

306301
synchronized (cache) {
307-
cache.put(sort, creator);
302+
cache.put(sort, accessor, creator);
308303
}
309304

310305
return creator;

‎spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/PartTreeQueryCache.java

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
/*
2+
* Copyright 2024 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.springframework.data.jpa.repository.query;
17+
18+
import java.util.HashMap;
19+
import java.util.LinkedHashMap;
20+
import java.util.Map;
21+
import java.util.Objects;
22+
23+
import org.springframework.data.domain.Sort;
24+
import org.springframework.lang.Nullable;
25+
import org.springframework.util.ObjectUtils;
26+
27+
/**
28+
* @author Christoph Strobl
29+
*/
30+
class PartTreeQueryCache {
31+
32+
private final Map<CacheKey, JpqlQueryCreator> cache = new LinkedHashMap<CacheKey, JpqlQueryCreator>() {
33+
@Override
34+
protected boolean removeEldestEntry(Map.Entry<CacheKey, JpqlQueryCreator> eldest) {
35+
return size() > 256;
36+
}
37+
};
38+
39+
@Nullable
40+
JpqlQueryCreator get(Sort sort, JpaParametersParameterAccessor accessor) {
41+
return cache.get(CacheKey.of(sort, accessor));
42+
}
43+
44+
@Nullable
45+
JpqlQueryCreator put(Sort sort, JpaParametersParameterAccessor accessor, JpqlQueryCreator creator) {
46+
return cache.put(CacheKey.of(sort, accessor), creator);
47+
}
48+
49+
static class CacheKey {
50+
51+
private final Sort sort;
52+
private final Map<Integer, Nulled> params;
53+
54+
public CacheKey(Sort sort, Map<Integer, Nulled> params) {
55+
this.sort = sort;
56+
this.params = params;
57+
}
58+
59+
static CacheKey of(Sort sort, JpaParametersParameterAccessor accessor) {
60+
61+
Object[] values = accessor.getValues();
62+
if (ObjectUtils.isEmpty(values)) {
63+
return new CacheKey(sort, Map.of());
64+
}
65+
66+
return new CacheKey(sort, toNullableMap(values));
67+
}
68+
69+
static Map<Integer, Nulled> toNullableMap(Object[] args) {
70+
71+
Map<Integer, Nulled> paramMap = new HashMap<>(args.length);
72+
for (int i = 0; i < args.length; i++) {
73+
paramMap.put(i, args[i] != null ? Nulled.NO : Nulled.YES);
74+
}
75+
return paramMap;
76+
}
77+
78+
@Override
79+
public boolean equals(Object o) {
80+
if (o == this) {
81+
return true;
82+
}
83+
if (o == null || getClass() != o.getClass()) {
84+
return false;
85+
}
86+
CacheKey cacheKey = (CacheKey) o;
87+
return sort.equals(cacheKey.sort) && params.equals(cacheKey.params);
88+
}
89+
90+
@Override
91+
public int hashCode() {
92+
return Objects.hash(sort, params);
93+
}
94+
}
95+
96+
enum Nulled {
97+
YES, NO
98+
}
99+
100+
}

‎spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryFinderTests.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,4 +386,13 @@ public void selectProjectionWithSubselect() {
386386
assertThat(dtos).flatExtracting(UserRepository.NameOnly::getLastname) //
387387
.containsExactly("Matthews", "Beauford", "Matthews");
388388
}
389+
390+
@Test
391+
void findBySimplePropertyUsingMixedNullNonNullArgument() {
392+
393+
List<User> result = userRepository.findUserByLastname(null);
394+
assertThat(result).isEmpty();
395+
result = userRepository.findUserByLastname(carter.getLastname());
396+
assertThat(result).containsExactly(carter);
397+
}
389398
}

‎spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryCreatorTests.java

Lines changed: 7 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import jakarta.persistence.Tuple;
2828
import jakarta.persistence.metamodel.Metamodel;
2929

30-
import java.util.ArrayList;
3130
import java.util.Date;
3231
import java.util.Iterator;
3332
import java.util.List;
@@ -39,12 +38,9 @@
3938
import org.junit.jupiter.params.ParameterizedTest;
4039
import org.junit.jupiter.params.provider.FieldSource;
4140
import org.junit.jupiter.params.provider.ValueSource;
42-
import org.mockito.Mockito;
43-
import org.springframework.core.MethodParameter;
4441
import org.springframework.data.domain.Pageable;
4542
import org.springframework.data.domain.ScrollPosition;
4643
import org.springframework.data.domain.Sort;
47-
import org.springframework.data.jpa.repository.query.JpaParameters.JpaParameter;
4844
import org.springframework.data.jpa.repository.support.JpqlQueryTemplates;
4945
import org.springframework.data.jpa.util.TestMetaModel;
5046
import org.springframework.data.projection.ProjectionFactory;
@@ -53,7 +49,6 @@
5349
import org.springframework.data.repository.query.ReturnedType;
5450
import org.springframework.data.repository.query.parser.PartTree;
5551
import org.springframework.data.util.Lazy;
56-
import org.springframework.data.util.TypeInformation;
5752
import org.springframework.lang.Nullable;
5853

5954
/**
@@ -725,16 +720,16 @@ JpaQueryCreator queryCreator(PartTree tree, ReturnedType returnedType, Metamodel
725720
JpaQueryCreator queryCreator(PartTree tree, ReturnedType returnedType, Metamodel metamodel,
726721
JpqlQueryTemplates templates, Object... arguments) {
727722

728-
ParameterMetadataProvider parameterMetadataProvider = new ParameterMetadataProvider(accessor(arguments),
729-
EscapeCharacter.DEFAULT, templates);
723+
ParameterMetadataProvider parameterMetadataProvider = new ParameterMetadataProvider(
724+
StubJpaParameterParameterAccessor.accessor(arguments), EscapeCharacter.DEFAULT, templates);
730725
return queryCreator(tree, returnedType, metamodel, templates, parameterMetadataProvider);
731726
}
732727

733728
JpaQueryCreator queryCreator(PartTree tree, ReturnedType returnedType, Metamodel metamodel,
734729
JpqlQueryTemplates templates, Class<?>... argumentTypes) {
735730

736-
ParameterMetadataProvider parameterMetadataProvider = new ParameterMetadataProvider(accessor(argumentTypes),
737-
EscapeCharacter.DEFAULT, templates);
731+
ParameterMetadataProvider parameterMetadataProvider = new ParameterMetadataProvider(
732+
StubJpaParameterParameterAccessor.accessor(argumentTypes), EscapeCharacter.DEFAULT, templates);
738733
return queryCreator(tree, returnedType, metamodel, templates, parameterMetadataProvider);
739734
}
740735

@@ -749,34 +744,10 @@ JpaQueryCreator queryCreator(PartTree tree, ReturnedType returnedType, Metamodel
749744
return new JpaQueryCreator(tree, returnedType, parameterMetadataProvider, templates, entityManager);
750745
}
751746

752-
@SuppressWarnings({ "rawtypes", "unchecked" })
753-
private JpaParametersParameterAccessor accessor(Object... parameters) {
754-
755-
List<JpaParameter> parametersList = new ArrayList<>(parameters.length);
756-
for (Object param : parameters) {
757-
758-
MethodParameter mock = Mockito.mock(MethodParameter.class);
759-
when(mock.getParameterType()).thenReturn((Class) param.getClass());
760-
JpaParameter parameter = new JpaParameter(mock, TypeInformation.of(param.getClass()));
761-
parametersList.add(parameter);
762-
}
763-
return new JpaParametersParameterAccessor(new JpaParameters(parametersList), parameters);
764-
}
765-
766747
@SuppressWarnings({ "rawtypes", "unchecked" })
767748
private JpaParametersParameterAccessor accessor(Class<?>... argumentTypes) {
768749

769-
List<JpaParameter> parametersList = new ArrayList<>(argumentTypes.length);
770-
for (Class<?> type : argumentTypes) {
771-
772-
MethodParameter mock = Mockito.mock(MethodParameter.class);
773-
when(mock.getParameterType()).thenReturn((Class) type);
774-
JpaParameter parameter = new JpaParameter(mock, TypeInformation.of(type));
775-
parametersList.add(parameter);
776-
}
777-
778-
Object[] parameters = new Object[argumentTypes.length];
779-
return new JpaParametersParameterAccessor(new JpaParameters(parametersList), parameters);
750+
return StubJpaParameterParameterAccessor.accessor(argumentTypes);
780751
}
781752

782753
@jakarta.persistence.Entity
@@ -1052,9 +1023,9 @@ public Iterator<Object> iterator() {
10521023
JpaParametersParameterAccessor initParameterAccessor() {
10531024

10541025
if (arguments.length > 0 || argumentTypes == null) {
1055-
return accessor(arguments);
1026+
return StubJpaParameterParameterAccessor.accessor(arguments);
10561027
}
1057-
return accessor(argumentTypes);
1028+
return StubJpaParameterParameterAccessor.accessor(argumentTypes);
10581029
}
10591030

10601031
JpaQueryCreator initJpaQueryCreator() {

‎spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/PartTreeQueryCacheUnitTests.java

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* Copyright 2024 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.springframework.data.jpa.repository.query;
17+
18+
import static org.assertj.core.api.Assertions.assertThat;
19+
20+
import java.util.function.Supplier;
21+
import java.util.stream.Stream;
22+
23+
import org.junit.jupiter.api.BeforeEach;
24+
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.params.ParameterizedTest;
26+
import org.junit.jupiter.params.provider.Arguments;
27+
import org.junit.jupiter.params.provider.FieldSource;
28+
import org.mockito.Mockito;
29+
import org.springframework.data.domain.Sort;
30+
import org.springframework.data.domain.Sort.Direction;
31+
32+
/**
33+
* @author Christoph Strobl
34+
*/
35+
public class PartTreeQueryCacheUnitTests {
36+
37+
PartTreeQueryCache cache;
38+
39+
static Supplier<Stream<Arguments>> cacheInput = () -> Stream.of(
40+
Arguments.arguments(Sort.unsorted(), StubJpaParameterParameterAccessor.accessor()), //
41+
Arguments.arguments(Sort.by(Direction.ASC, "one"), StubJpaParameterParameterAccessor.accessor()), //
42+
Arguments.arguments(Sort.by(Direction.DESC, "one"), StubJpaParameterParameterAccessor.accessor()), //
43+
Arguments.arguments(Sort.unsorted(),
44+
StubJpaParameterParameterAccessor.accessorFor(String.class).withValues("value")), //
45+
Arguments.arguments(Sort.unsorted(),
46+
StubJpaParameterParameterAccessor.accessorFor(String.class).withValues(new Object[] { null })), //
47+
Arguments.arguments(Sort.by(Direction.ASC, "one"),
48+
StubJpaParameterParameterAccessor.accessorFor(String.class).withValues("value")), //
49+
Arguments.arguments(Sort.by(Direction.ASC, "one"),
50+
StubJpaParameterParameterAccessor.accessorFor(String.class).withValues(new Object[] { null })));
51+
52+
@BeforeEach
53+
void beforeEach() {
54+
cache = new PartTreeQueryCache();
55+
}
56+
57+
@ParameterizedTest
58+
@FieldSource("cacheInput")
59+
void getReturnsNullForEmptyCache(Sort sort, JpaParametersParameterAccessor accessor) {
60+
assertThat(cache.get(sort, accessor)).isNull();
61+
}
62+
63+
@ParameterizedTest
64+
@FieldSource("cacheInput")
65+
void getReturnsCachedInstance(Sort sort, JpaParametersParameterAccessor accessor) {
66+
67+
JpaQueryCreator queryCreator = Mockito.mock(JpaQueryCreator.class);
68+
69+
assertThat(cache.put(sort, accessor, queryCreator)).isNull();
70+
assertThat(cache.get(sort, accessor)).isSameAs(queryCreator);
71+
}
72+
73+
@ParameterizedTest
74+
@FieldSource("cacheInput")
75+
void cacheGetWithSort(Sort sort, JpaParametersParameterAccessor accessor) {
76+
77+
JpaQueryCreator queryCreator = Mockito.mock(JpaQueryCreator.class);
78+
assertThat(cache.put(Sort.by("not-in-cache"), accessor, queryCreator)).isNull();
79+
80+
assertThat(cache.get(sort, accessor)).isNull();
81+
}
82+
83+
@ParameterizedTest
84+
@FieldSource("cacheInput")
85+
void cacheGetWithccessor(Sort sort, JpaParametersParameterAccessor accessor) {
86+
87+
JpaQueryCreator queryCreator = Mockito.mock(JpaQueryCreator.class);
88+
assertThat(cache.put(sort, StubJpaParameterParameterAccessor.accessor("spring", "data"), queryCreator)).isNull();
89+
90+
assertThat(cache.get(sort, accessor)).isNull();
91+
}
92+
93+
@Test
94+
void cachesOnNullableNotArgumentType() {
95+
96+
JpaQueryCreator queryCreator = Mockito.mock(JpaQueryCreator.class);
97+
Sort sort = Sort.unsorted();
98+
assertThat(cache.put(sort, StubJpaParameterParameterAccessor.accessor("spring", "data"), queryCreator)).isNull();
99+
100+
assertThat(cache.get(sort,
101+
StubJpaParameterParameterAccessor.accessor(new Class[] { String.class, String.class }, "spring", null)))
102+
.isNull();
103+
104+
assertThat(cache.get(sort,
105+
StubJpaParameterParameterAccessor.accessor(new Class[] { String.class, String.class }, null, "data"))).isNull();
106+
107+
assertThat(cache.get(sort,
108+
StubJpaParameterParameterAccessor.accessor(new Class[] { String.class, String.class }, "data", "spring")))
109+
.isSameAs(queryCreator);
110+
111+
assertThat(cache.get(Sort.by("not-in-cache"),
112+
StubJpaParameterParameterAccessor.accessor(new Class[] { String.class, String.class }, "data", "spring")))
113+
.isNull();
114+
}
115+
116+
}

0 commit comments

Comments
 (0)