Skip to content

Commit aadbb66

Browse files
mp911deschauder
mp911de
authored and
schauder
committed
DATAJDBC-318 - Extend query method validation.
We now reject criteria predicates for collections, maps and references. The select list selects all columns until DATAJDBC-523 is solved. Original pull request: #209.
1 parent fd98e16 commit aadbb66

File tree

4 files changed

+125
-55
lines changed

4 files changed

+125
-55
lines changed

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

Lines changed: 65 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,21 @@
1515
*/
1616
package org.springframework.data.jdbc.repository.query;
1717

18-
import java.util.ArrayList;
19-
import java.util.Collection;
18+
import java.util.Collections;
19+
import java.util.List;
2020

2121
import org.springframework.data.domain.Pageable;
2222
import org.springframework.data.domain.Sort;
2323
import org.springframework.data.jdbc.core.convert.JdbcConverter;
24+
import org.springframework.data.mapping.PersistentPropertyPath;
2425
import org.springframework.data.mapping.context.MappingContext;
2526
import org.springframework.data.relational.core.dialect.Dialect;
2627
import org.springframework.data.relational.core.dialect.RenderContextFactory;
28+
import org.springframework.data.relational.core.mapping.PersistentPropertyPathExtension;
2729
import org.springframework.data.relational.core.mapping.RelationalPersistentEntity;
2830
import org.springframework.data.relational.core.mapping.RelationalPersistentProperty;
2931
import org.springframework.data.relational.core.query.Criteria;
32+
import org.springframework.data.relational.core.sql.Expression;
3033
import org.springframework.data.relational.core.sql.Select;
3134
import org.springframework.data.relational.core.sql.SelectBuilder;
3235
import org.springframework.data.relational.core.sql.SqlIdentifier;
@@ -35,6 +38,8 @@
3538
import org.springframework.data.relational.repository.query.RelationalEntityMetadata;
3639
import org.springframework.data.relational.repository.query.RelationalParameterAccessor;
3740
import org.springframework.data.relational.repository.query.RelationalQueryCreator;
41+
import org.springframework.data.repository.query.Parameters;
42+
import org.springframework.data.repository.query.parser.Part;
3843
import org.springframework.data.repository.query.parser.PartTree;
3944
import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
4045
import org.springframework.util.Assert;
@@ -50,8 +55,6 @@ class JdbcQueryCreator extends RelationalQueryCreator<ParametrizedQuery> {
5055
private final PartTree tree;
5156
private final RelationalParameterAccessor accessor;
5257
private final QueryMapper queryMapper;
53-
54-
private final MappingContext<RelationalPersistentEntity<?>, RelationalPersistentProperty> mappingContext;
5558
private final RelationalEntityMetadata<?> entityMetadata;
5659
private final RenderContextFactory renderContextFactory;
5760

@@ -65,8 +68,8 @@ class JdbcQueryCreator extends RelationalQueryCreator<ParametrizedQuery> {
6568
* @param entityMetadata relational entity metadata, must not be {@literal null}.
6669
* @param accessor parameter metadata provider, must not be {@literal null}.
6770
*/
68-
JdbcQueryCreator(PartTree tree, JdbcConverter converter, Dialect dialect,
69-
RelationalEntityMetadata<?> entityMetadata, RelationalParameterAccessor accessor) {
71+
JdbcQueryCreator(PartTree tree, JdbcConverter converter, Dialect dialect, RelationalEntityMetadata<?> entityMetadata,
72+
RelationalParameterAccessor accessor) {
7073
super(tree, accessor);
7174

7275
Assert.notNull(converter, "JdbcConverter must not be null");
@@ -76,12 +79,60 @@ class JdbcQueryCreator extends RelationalQueryCreator<ParametrizedQuery> {
7679
this.tree = tree;
7780
this.accessor = accessor;
7881

79-
this.mappingContext = (MappingContext) converter.getMappingContext();
8082
this.entityMetadata = entityMetadata;
8183
this.queryMapper = new QueryMapper(dialect, converter);
8284
this.renderContextFactory = new RenderContextFactory(dialect);
8385
}
8486

87+
/**
88+
* Validate parameters for the derived query. Specifically checking that the query method defines scalar parameters
89+
* and collection parameters where required and that invalid parameter declarations are rejected.
90+
*
91+
* @param tree
92+
* @param parameters
93+
*/
94+
public static void validate(PartTree tree, Parameters<?, ?> parameters,
95+
MappingContext<? extends RelationalPersistentEntity<?>, ? extends RelationalPersistentProperty> context) {
96+
97+
RelationalQueryCreator.validate(tree, parameters);
98+
99+
for (PartTree.OrPart parts : tree) {
100+
for (Part part : parts) {
101+
102+
PersistentPropertyPath<? extends RelationalPersistentProperty> propertyPath = context
103+
.getPersistentPropertyPath(part.getProperty());
104+
PersistentPropertyPathExtension path = new PersistentPropertyPathExtension(context, propertyPath);
105+
106+
for (PersistentPropertyPathExtension pathToValidate = path; path.getLength() > 0; path = path.getParentPath()) {
107+
validateProperty(pathToValidate);
108+
}
109+
}
110+
}
111+
}
112+
113+
private static void validateProperty(PersistentPropertyPathExtension path) {
114+
115+
if (!path.getParentPath().isEmbedded() && path.getLength() > 1) {
116+
throw new IllegalArgumentException(
117+
String.format("Cannot query by nested property: %s", path.getRequiredPersistentPropertyPath().toDotPath()));
118+
}
119+
120+
if (path.isMultiValued() || path.isMap()) {
121+
throw new IllegalArgumentException(String.format("Cannot query by multi-valued property: %s",
122+
path.getRequiredPersistentPropertyPath().getLeafProperty().getName()));
123+
}
124+
125+
if (!path.isEmbedded() && path.isEntity()) {
126+
throw new IllegalArgumentException(
127+
String.format("Cannot query by nested entity: %s", path.getRequiredPersistentPropertyPath().toDotPath()));
128+
}
129+
130+
if (path.getRequiredPersistentPropertyPath().getLeafProperty().isReference()) {
131+
throw new IllegalArgumentException(
132+
String.format("Cannot query by reference: %s", path.getRequiredPersistentPropertyPath().toDotPath()));
133+
}
134+
}
135+
85136
/**
86137
* Creates {@link ParametrizedQuery} applying the given {@link Criteria} and {@link Sort} definition.
87138
*
@@ -96,7 +147,12 @@ protected ParametrizedQuery complete(Criteria criteria, Sort sort) {
96147
Table table = Table.create(entityMetadata.getTableName());
97148
MapSqlParameterSource parameterSource = new MapSqlParameterSource();
98149

99-
SelectBuilder.SelectFromAndJoin builder = Select.builder().select(table.columns(getSelectProjection())).from(table);
150+
List<? extends Expression> columns = table.columns(getSelectProjection());
151+
if (columns.isEmpty()) {
152+
columns = Collections.singletonList(table.asterisk());
153+
}
154+
155+
SelectBuilder.SelectFromAndJoin builder = Select.builder().select(columns).from(table);
100156

101157
if (tree.isExistsProjection()) {
102158
builder = builder.limit(1);
@@ -132,27 +188,6 @@ private SqlIdentifier[] getSelectProjection() {
132188
return new SqlIdentifier[] { tableEntity.getIdColumn() };
133189
}
134190

135-
Collection<SqlIdentifier> columnNames = unwrapColumnNames("", tableEntity);
136-
137-
return columnNames.toArray(new SqlIdentifier[0]);
138-
}
139-
140-
private Collection<SqlIdentifier> unwrapColumnNames(String prefix, RelationalPersistentEntity<?> persistentEntity) {
141-
142-
Collection<SqlIdentifier> columnNames = new ArrayList<>();
143-
144-
for (RelationalPersistentProperty property : persistentEntity) {
145-
146-
if (property.isEmbedded()) {
147-
columnNames.addAll(
148-
unwrapColumnNames(prefix + property.getEmbeddedPrefix(), mappingContext.getPersistentEntity(property)));
149-
}
150-
151-
else {
152-
columnNames.add(property.getColumnName().transform(prefix::concat));
153-
}
154-
}
155-
156-
return columnNames;
191+
return new SqlIdentifier[0];
157192
}
158193
}

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,8 @@ public PartTreeJdbcQuery(JdbcQueryMethod queryMethod, Dialect dialect, JdbcConve
6363
this.dialect = dialect;
6464
this.converter = converter;
6565

66-
try {
67-
68-
this.tree = new PartTree(queryMethod.getName(), queryMethod.getEntityInformation().getJavaType());
69-
JdbcQueryCreator.validate(this.tree, this.parameters);
70-
} catch (RuntimeException e) {
71-
72-
throw new IllegalArgumentException(
73-
String.format("Failed to create query for method %s! %s", queryMethod, e.getMessage()), e);
74-
}
66+
this.tree = new PartTree(queryMethod.getName(), queryMethod.getEntityInformation().getJavaType());
67+
JdbcQueryCreator.validate(this.tree, this.parameters, this.converter.getMappingContext());
7568

7669
this.execution = getQueryExecution(queryMethod, null, rowMapper);
7770
}

‎spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/support/JdbcQueryLookupStrategy.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.springframework.data.relational.core.mapping.event.AfterLoadEvent;
3636
import org.springframework.data.repository.core.NamedQueries;
3737
import org.springframework.data.repository.core.RepositoryMetadata;
38+
import org.springframework.data.repository.query.QueryCreationException;
3839
import org.springframework.data.repository.query.QueryLookupStrategy;
3940
import org.springframework.data.repository.query.RepositoryQuery;
4041
import org.springframework.jdbc.core.RowMapper;
@@ -94,12 +95,16 @@ public RepositoryQuery resolveQuery(Method method, RepositoryMetadata repository
9495
JdbcQueryMethod queryMethod = new JdbcQueryMethod(method, repositoryMetadata, projectionFactory, namedQueries,
9596
context);
9697

97-
if (namedQueries.hasQuery(queryMethod.getNamedQueryName()) || queryMethod.hasAnnotatedQuery()) {
98+
try {
99+
if (namedQueries.hasQuery(queryMethod.getNamedQueryName()) || queryMethod.hasAnnotatedQuery()) {
98100

99-
RowMapper<?> mapper = queryMethod.isModifyingQuery() ? null : createMapper(queryMethod);
100-
return new StringBasedJdbcQuery(queryMethod, operations, mapper, converter);
101-
} else {
102-
return new PartTreeJdbcQuery(queryMethod, dialect, converter, operations, createMapper(queryMethod));
101+
RowMapper<?> mapper = queryMethod.isModifyingQuery() ? null : createMapper(queryMethod);
102+
return new StringBasedJdbcQuery(queryMethod, operations, mapper, converter);
103+
} else {
104+
return new PartTreeJdbcQuery(queryMethod, dialect, converter, operations, createMapper(queryMethod));
105+
}
106+
} catch (Exception e) {
107+
throw QueryCreationException.create(queryMethod, e.getMessage());
103108
}
104109
}
105110

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

Lines changed: 48 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,20 @@
3030
import org.junit.Test;
3131
import org.junit.runner.RunWith;
3232
import org.mockito.junit.MockitoJUnitRunner;
33+
3334
import org.springframework.data.annotation.Id;
3435
import org.springframework.data.jdbc.core.convert.BasicJdbcConverter;
3536
import org.springframework.data.jdbc.core.convert.JdbcConverter;
3637
import org.springframework.data.jdbc.core.convert.RelationResolver;
38+
import org.springframework.data.jdbc.core.mapping.AggregateReference;
3739
import org.springframework.data.jdbc.core.mapping.JdbcMappingContext;
3840
import org.springframework.data.projection.SpelAwareProxyProjectionFactory;
3941
import org.springframework.data.relational.core.dialect.H2Dialect;
4042
import org.springframework.data.relational.core.mapping.Embedded;
43+
import org.springframework.data.relational.core.mapping.MappedCollection;
4144
import org.springframework.data.relational.core.mapping.Table;
4245
import org.springframework.data.relational.repository.query.RelationalParametersParameterAccessor;
46+
import org.springframework.data.repository.NoRepositoryBean;
4347
import org.springframework.data.repository.Repository;
4448
import org.springframework.data.repository.core.support.DefaultRepositoryMetadata;
4549
import org.springframework.data.repository.core.support.PropertiesBasedNamedQueries;
@@ -51,34 +55,50 @@
5155
*
5256
* @author Roman Chigvintsev
5357
* @author Mark Paluch
58+
* @author Jens Schauder
5459
*/
5560
@RunWith(MockitoJUnitRunner.class)
5661
public class PartTreeJdbcQueryUnitTests {
5762

5863
private static final String TABLE = "\"users\"";
59-
private static final String ALL_FIELDS = "\"users\".\"ID\", \"users\".\"FIRST_NAME\", \"users\".\"LAST_NAME\", \"users\".\"DATE_OF_BIRTH\", \"users\".\"AGE\", \"users\".\"ACTIVE\", \"users\".\"USER_STREET\", \"users\".\"USER_CITY\"";
64+
private static final String ALL_FIELDS = "\"users\".*";
6065

6166
JdbcMappingContext mappingContext = new JdbcMappingContext();
6267
JdbcConverter converter = new BasicJdbcConverter(mappingContext, mock(RelationResolver.class));
6368

6469
@Test // DATAJDBC-318
65-
public void selectContainsColumnsForOneToOneReference() throws Exception {
70+
public void shouldFailForQueryByReference() throws Exception {
6671

67-
JdbcQueryMethod queryMethod = getQueryMethod("findAllByFirstName", String.class);
68-
PartTreeJdbcQuery jdbcQuery = createQuery(queryMethod);
69-
ParametrizedQuery query = jdbcQuery.createQuery(getAccessor(queryMethod, new Object[] { "John" }));
72+
JdbcQueryMethod queryMethod = getQueryMethod("findAllByHated", Hobby.class);
73+
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
74+
}
75+
76+
@Test // DATAJDBC-318
77+
public void shouldFailForQueryByAggregateReference() throws Exception {
7078

71-
assertThat(query.getQuery()).contains("hated.\"name\" AS \"hated_name\"");
79+
JdbcQueryMethod queryMethod = getQueryMethod("findAllByHobbyReference", Hobby.class);
80+
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
7281
}
7382

7483
@Test // DATAJDBC-318
75-
public void doesNotContainsColumnsForOneToManyReference() throws Exception{
84+
public void shouldFailForQueryByList() throws Exception {
7685

77-
JdbcQueryMethod queryMethod = getQueryMethod("findAllByFirstName", String.class);
78-
PartTreeJdbcQuery jdbcQuery = createQuery(queryMethod);
79-
ParametrizedQuery query = jdbcQuery.createQuery(getAccessor(queryMethod, new Object[] { "John" }));
86+
JdbcQueryMethod queryMethod = getQueryMethod("findAllByHobbies", Object.class);
87+
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
88+
}
89+
90+
@Test // DATAJDBC-318
91+
public void shouldFailForQueryByEmbeddedList() throws Exception {
8092

81-
assertThat(query.getQuery().toLowerCase()).doesNotContain("hobbies");
93+
JdbcQueryMethod queryMethod = getQueryMethod("findByAnotherEmbeddedList", Object.class);
94+
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
95+
}
96+
97+
@Test // DATAJDBC-318
98+
public void shouldFailForAggregateReference() throws Exception {
99+
100+
JdbcQueryMethod queryMethod = getQueryMethod("findByAnotherEmbeddedList", Object.class);
101+
assertThatIllegalArgumentException().isThrownBy(() -> createQuery(queryMethod));
82102
}
83103

84104
@Test // DATAJDBC-318
@@ -570,10 +590,17 @@ private RelationalParametersParameterAccessor getAccessor(JdbcQueryMethod queryM
570590
return new RelationalParametersParameterAccessor(queryMethod, values);
571591
}
572592

593+
@NoRepositoryBean
573594
interface UserRepository extends Repository<User, Long> {
574595

575596
List<User> findAllByFirstName(String firstName);
576597

598+
List<User> findAllByHated(Hobby hobby);
599+
600+
List<User> findAllByHobbies(Object hobbies);
601+
602+
List<User> findAllByHobbyReference(Hobby hobby);
603+
577604
List<User> findAllByLastNameAndFirstName(String lastName, String firstName);
578605

579606
List<User> findAllByLastNameOrFirstName(String lastName, String firstName);
@@ -637,6 +664,8 @@ interface UserRepository extends Repository<User, Long> {
637664
User findByAddress(Address address);
638665

639666
User findByAddressStreet(String street);
667+
668+
User findByAnotherEmbeddedList(Object list);
640669
}
641670

642671
@Table("users")
@@ -650,9 +679,12 @@ static class User {
650679
Boolean active;
651680

652681
@Embedded(prefix = "user_", onEmpty = Embedded.OnEmpty.USE_NULL) Address address;
682+
@Embedded.Nullable AnotherEmbedded anotherEmbedded;
653683

654684
List<Hobby> hobbies;
655685
Hobby hated;
686+
687+
AggregateReference<Hobby, String> hobbyReference;
656688
}
657689

658690
@AllArgsConstructor
@@ -661,6 +693,11 @@ static class Address {
661693
String city;
662694
}
663695

696+
@AllArgsConstructor
697+
static class AnotherEmbedded {
698+
@MappedCollection(idColumn = "ID", keyColumn = "ORDER_KEY") List<Hobby> list;
699+
}
700+
664701
static class Hobby {
665702
String name;
666703
}

0 commit comments

Comments
 (0)