Skip to content

Commit 285821a

Browse files
mp911demp911de
committed
Polishing.
Refine query creation exception. Reorder checks in validation to check the initial case and then the condition that must be met. Add integration test. Fix contributor name. See #2059 Original pull request: #2249
1 parent 3bc6527 commit 285821a

File tree

4 files changed

+92
-32
lines changed

4 files changed

+92
-32
lines changed

‎spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/JdbcQueryCreator.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@
2323
import org.springframework.data.jdbc.core.convert.JdbcConverter;
2424
import org.springframework.data.jdbc.core.convert.SqlGeneratorSource;
2525
import org.springframework.data.mapping.PersistentPropertyPath;
26-
import org.springframework.data.relational.core.dialect.Dialect;
2726
import org.springframework.data.relational.core.conversion.AbstractRelationalConverter;
27+
import org.springframework.data.relational.core.dialect.Dialect;
2828
import org.springframework.data.relational.core.mapping.AggregatePath;
2929
import org.springframework.data.relational.core.mapping.RelationalMappingContext;
3030
import org.springframework.data.relational.core.mapping.RelationalPersistentEntity;
@@ -48,7 +48,7 @@
4848
* @author Jens Schauder
4949
* @author Myeonghyeon Lee
5050
* @author Diego Krupitza
51-
* @author wonderfulrosemari
51+
* @author Jin Hyuk Cho
5252
* @since 2.0
5353
*/
5454
public class JdbcQueryCreator extends RelationalQueryCreator<ParametrizedQuery> {
@@ -124,6 +124,7 @@ public JdbcQueryCreator(PartTree tree, JdbcConverter converter, Dialect dialect,
124124
public JdbcQueryCreator(RelationalMappingContext context, PartTree tree, JdbcConverter converter, Dialect dialect,
125125
RelationalEntityMetadata<?> entityMetadata, RelationalParameterAccessor accessor, boolean isSliceQuery,
126126
ReturnedType returnedType, Optional<Lock> lockMode, SqlGeneratorSource sqlGeneratorSource) {
127+
127128
super(tree, accessor);
128129

129130
Assert.notNull(converter, "JdbcConverter must not be null");
@@ -148,7 +149,7 @@ StatementFactory getStatementFactory() {
148149
}
149150

150151
/**
151-
* Validate parameters for the derived query. Specifically checking that the query method defines scalar parameters
152+
* Validate parameters for the derived query. Specifically, checking that the query method defines scalar parameters
152153
* and collection parameters where required and that invalid parameter declarations are rejected.
153154
*
154155
* @param tree the tree structure defining the predicate of the query.
@@ -177,17 +178,17 @@ private static void validateProperty(AggregatePath path, JdbcConverter converter
177178
return;
178179
}
179180

180-
if (!path.getParentPath().isEmbedded() && path.getLength() > 2) {
181-
throw new IllegalArgumentException(String.format("Cannot query by nested property: %s", path.toDotPath()));
181+
if (path.getLength() > 2 && !path.getParentPath().isEmbedded()) {
182+
throw new IllegalStateException(String.format("Cannot query by nested property: %s", path.toDotPath()));
182183
}
183184

184185
if (path.isMultiValued() || path.isMap()) {
185-
throw new IllegalArgumentException(
186+
throw new IllegalStateException(
186187
String.format("Cannot query by multi-valued property: %s", path.getRequiredLeafProperty().getName()));
187188
}
188189

189-
if (!path.isEmbedded() && path.isEntity() && !hasCustomWriteTarget(path, converter)) {
190-
throw new IllegalArgumentException(String.format("Cannot query by nested entity: %s", path.toDotPath()));
190+
if (path.isEntity() && !path.isEmbedded() && !hasCustomWriteTarget(path, converter)) {
191+
throw new IllegalStateException(String.format("Cannot query by nested entity: %s", path.toDotPath()));
191192
}
192193
}
193194

‎spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/PartTreeJdbcQuery.java‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.springframework.data.relational.repository.query.RelationalParameterAccessor;
4141
import org.springframework.data.relational.repository.query.RelationalParametersParameterAccessor;
4242
import org.springframework.data.repository.query.Parameters;
43+
import org.springframework.data.repository.query.QueryCreationException;
4344
import org.springframework.data.repository.query.ResultProcessor;
4445
import org.springframework.data.repository.query.ReturnedType;
4546
import org.springframework.data.repository.query.parser.PartTree;
@@ -61,7 +62,7 @@
6162
* @author Mikhail Polivakha
6263
* @author Yunyoung LEE
6364
* @author Nikita Konev
64-
* @author wonderfulrosemari
65+
* @author Jin Hyuk Cho
6566
* @since 2.0
6667
*/
6768
public class PartTreeJdbcQuery extends AbstractJdbcQuery {
@@ -145,11 +146,16 @@ public PartTreeJdbcQuery(RelationalMappingContext context, JdbcQueryMethod query
145146
this.dialect = dialect;
146147
this.converter = converter;
147148

148-
this.tree = new PartTree(queryMethod.getName(), queryMethod.getResultProcessor().getReturnedType().getDomainType());
149-
JdbcQueryCreator.validate(this.tree, this.parameters, this.converter.getMappingContext(), this.converter);
149+
try {
150+
this.tree = new PartTree(queryMethod.getName(),
151+
queryMethod.getResultProcessor().getReturnedType().getDomainType());
152+
JdbcQueryCreator.validate(this.tree, this.parameters, this.converter.getMappingContext(), this.converter);
150153

151-
this.cachedRowMapperFactory = new CachedRowMapperFactory(tree, rowMapperFactory, converter,
152-
queryMethod.getResultProcessor());
154+
this.cachedRowMapperFactory = new CachedRowMapperFactory(tree, rowMapperFactory, converter,
155+
queryMethod.getResultProcessor());
156+
} catch (RuntimeException e) {
157+
throw QueryCreationException.create(queryMethod, e);
158+
}
153159
}
154160

155161
private Sort getDynamicSort(RelationalParameterAccessor accessor) {
@@ -164,8 +170,8 @@ public Object execute(Object[] values) {
164170
values);
165171

166172
if (tree.isDelete()) {
167-
JdbcQueryExecution<?> execution = createModifyingQueryExecutor();
168173

174+
JdbcQueryExecution<?> execution = createModifyingQueryExecutor();
169175
List<ParametrizedQuery> queries = createDeleteQueries(accessor);
170176
Object result = null;
171177
for (ParametrizedQuery query : queries) {

‎spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIntegrationTests.java‎

Lines changed: 61 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@
9090
import org.springframework.data.relational.repository.Lock;
9191
import org.springframework.data.repository.CrudRepository;
9292
import org.springframework.data.repository.ListCrudRepository;
93+
import org.springframework.data.repository.NoRepositoryBean;
9394
import org.springframework.data.repository.core.NamedQueries;
9495
import org.springframework.data.repository.core.support.PropertiesBasedNamedQueries;
9596
import org.springframework.data.repository.core.support.RepositoryFactoryCustomizer;
@@ -271,12 +272,28 @@ void findAllByIdWithIdConverter() {
271272
DummyEntity two = this.repository.save(createEntity("two"));
272273
DummyEntity three = this.repository.save(createEntity("three"));
273274

274-
List<WithConvertedIdentifier> result = withIdentifierConversion.findAllById(
275+
List<WithConversions> result = withIdentifierConversion.findAllById(
275276
List.of(new LongIdentifier(one.idProp), new LongIdentifier(two.idProp), new LongIdentifier(three.idProp)));
276277

277278
assertThat(result).hasSize(3);
278279
}
279280

281+
@Test // GH-2225
282+
void findAllByConverter() {
283+
284+
this.repository.save(createEntity("one"));
285+
this.repository.save(createEntity("two"));
286+
this.repository.save(createEntity("three"));
287+
288+
List<WithConversions> result = withIdentifierConversion.findByName(new Name("one"));
289+
290+
assertThat(result).hasSize(1);
291+
292+
result = withIdentifierConversion.queryByName(new Name("one"));
293+
294+
assertThat(result).hasSize(1);
295+
}
296+
280297
@Test // GH-2225
281298
void deleteAllByIdWithIdConverter() {
282299

@@ -297,7 +314,7 @@ void deleteAllWithIdConverter() {
297314
DummyEntity two = this.repository.save(createEntity("two"));
298315
DummyEntity three = this.repository.save(createEntity("three"));
299316

300-
List<WithConvertedIdentifier> result = withIdentifierConversion.findAllById(
317+
List<WithConversions> result = withIdentifierConversion.findAllById(
301318
List.of(new LongIdentifier(one.idProp), new LongIdentifier(two.idProp), new LongIdentifier(three.idProp)));
302319
withIdentifierConversion.deleteAll(result);
303320

@@ -1743,7 +1760,8 @@ RootRepository rootRepository() {
17431760
@Primary
17441761
CustomConversions jdbcCustomConversions(Dialect dialect) {
17451762
return JdbcConfiguration.createCustomConversions((JdbcDialect) dialect,
1746-
List.of(LongIdentifierToLongConverter.INSTANCE, NumberToLongIdentifierConverter.INSTANCE));
1763+
List.of(LongIdentifierToLongConverter.INSTANCE, NumberToLongIdentifierConverter.INSTANCE,
1764+
NameToStringConverterConverter.INSTANCE, StringToNameConverter.INSTANCE));
17471765
}
17481766

17491767
@Bean
@@ -1797,7 +1815,7 @@ NamingStrategy namingStrategy() {
17971815
@Override
17981816
public String getTableName(Class<?> type) {
17991817

1800-
if (type == WithConvertedIdentifier.class) {
1818+
if (type == WithConversions.class) {
18011819
return super.getTableName(DummyEntity.class);
18021820
}
18031821

@@ -2038,10 +2056,32 @@ public LongIdentifier convert(Number source) {
20382056
}
20392057
}
20402058

2041-
public static class WithConvertedIdentifier {
2059+
@WritingConverter
2060+
enum NameToStringConverterConverter implements Converter<Name, String> {
2061+
2062+
INSTANCE;
2063+
2064+
@Override
2065+
public String convert(Name source) {
2066+
return source.name;
2067+
}
2068+
}
2069+
2070+
@ReadingConverter
2071+
enum StringToNameConverter implements Converter<String, Name> {
2072+
2073+
INSTANCE;
2074+
2075+
@Override
2076+
public Name convert(String source) {
2077+
return new Name(source);
2078+
}
2079+
}
2080+
2081+
public static class WithConversions {
20422082

20432083
@Id LongIdentifier idProp;
2044-
String name;
2084+
Name name;
20452085

20462086
public LongIdentifier getIdProp() {
20472087
return idProp;
@@ -2051,16 +2091,28 @@ public void setIdProp(LongIdentifier idProp) {
20512091
this.idProp = idProp;
20522092
}
20532093

2054-
public String getName() {
2094+
public Name getName() {
20552095
return name;
20562096
}
20572097

2058-
public void setName(String name) {
2098+
public void setName(Name name) {
20592099
this.name = name;
20602100
}
20612101
}
20622102

2063-
public interface WithIdentifierConversion extends ListCrudRepository<WithConvertedIdentifier, LongIdentifier> {}
2103+
record Name(String name) {
2104+
2105+
}
2106+
2107+
@NoRepositoryBean
2108+
public interface WithIdentifierConversion extends ListCrudRepository<WithConversions, LongIdentifier> {
2109+
2110+
List<WithConversions> findByName(Name name);
2111+
2112+
@Query("SELECT * FROM DUMMY_ENTITY WHERE name = :name")
2113+
List<WithConversions> queryByName(Name name);
2114+
2115+
}
20642116

20652117
public static class DummyEntity {
20662118

‎spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/query/PartTreeJdbcQueryUnitTests.java‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import org.springframework.data.repository.Repository;
6363
import org.springframework.data.repository.core.support.DefaultRepositoryMetadata;
6464
import org.springframework.data.repository.core.support.PropertiesBasedNamedQueries;
65+
import org.springframework.data.repository.query.QueryCreationException;
6566
import org.springframework.data.repository.query.ReturnedType;
6667
import org.springframework.jdbc.core.RowMapper;
6768
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
@@ -74,7 +75,7 @@
7475
* @author Jens Schauder
7576
* @author Myeonghyeon Lee
7677
* @author Diego Krupitza
77-
* @author wonderfulrosemari
78+
* @author Jin Hyuk Cho
7879
*/
7980
@ExtendWith(MockitoExtension.class)
8081
public class PartTreeJdbcQueryUnitTests {
@@ -119,7 +120,7 @@ void setUp() {
119120
void shouldFailForQueryByReference() throws Exception {
120121

121122
JdbcQueryMethod queryMethod = getQueryMethod("findAllByHated", Hobby.class);
122-
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
123+
assertThatExceptionOfType(QueryCreationException.class).isThrownBy(() -> createQuery(queryMethod));
123124
}
124125

125126
@Test // GH-922
@@ -179,14 +180,14 @@ void createQueryWithPessimisticReadLock() throws Exception {
179180
void shouldFailForQueryByList() throws Exception {
180181

181182
JdbcQueryMethod queryMethod = getQueryMethod("findAllByHobbies", Object.class);
182-
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
183+
assertThatExceptionOfType(QueryCreationException.class).isThrownBy(() -> createQuery(queryMethod));
183184
}
184185

185186
@Test // DATAJDBC-318
186187
void shouldFailForQueryByEmbeddedList() throws Exception {
187188

188189
JdbcQueryMethod queryMethod = getQueryMethod("findByAnotherEmbeddedList", Object.class);
189-
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
190+
assertThatExceptionOfType(QueryCreationException.class).isThrownBy(() -> createQuery(queryMethod));
190191
}
191192

192193
@Test // GH-922
@@ -756,12 +757,12 @@ void createsQueryForCountProjection() throws Exception {
756757
}
757758

758759
@Test // GH-2059
759-
void createsQueryBySimpleDomainPrimitiveWithCustomConverters() throws Exception {
760+
void considersConvertersForQueryArguments() throws Exception {
760761

761762
JdbcCustomConversions conversions = JdbcCustomConversions.create(JdbcPostgresDialect.INSTANCE, it -> {
762-
it.registerConverter(CustomerRefToStringConverter.INSTANCE);
763-
it.registerConverter(StringToCustomerRefConverter.INSTANCE);
763+
it.registerConverters(CustomerRefToStringConverter.INSTANCE, StringToCustomerRefConverter.INSTANCE);
764764
});
765+
765766
JdbcMappingContext localContext = new JdbcMappingContext();
766767
JdbcConverter localConverter = new MappingJdbcConverter(localContext, mock(RelationResolver.class), conversions,
767768
JdbcTypeFactory.unsupported());
@@ -793,8 +794,8 @@ private JdbcQueryMethod getQueryMethod(Class<?> repositoryType, String methodNam
793794
private JdbcQueryMethod getQueryMethod(Class<?> repositoryType, JdbcMappingContext mappingContext, String methodName,
794795
Class<?>... parameterTypes) throws Exception {
795796
Method method = repositoryType.getMethod(methodName, parameterTypes);
796-
return new JdbcQueryMethod(method, new DefaultRepositoryMetadata(repositoryType), new SpelAwareProxyProjectionFactory(),
797-
new PropertiesBasedNamedQueries(new Properties()), mappingContext);
797+
return new JdbcQueryMethod(method, new DefaultRepositoryMetadata(repositoryType),
798+
new SpelAwareProxyProjectionFactory(), new PropertiesBasedNamedQueries(new Properties()), mappingContext);
798799
}
799800

800801
private RelationalParametersParameterAccessor getAccessor(JdbcQueryMethod queryMethod, Object[] values) {

0 commit comments

Comments
 (0)