Skip to content

Commit a26557e

Browse files
schauderschauder
committed
Introduce SqlSort.
SqlSort allows the specification of unsafe order-by-expressions. Order-by-expressions that are not declared unsafe are only accepted when they either match a property or consist only of digits, letters, underscore, dot, or parentheses. Closes #1507
1 parent 1598017 commit a26557e

File tree

7 files changed

+517
-10
lines changed

7 files changed

+517
-10
lines changed

‎spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.springframework.data.relational.core.query.CriteriaDefinition.Comparator;
4242
import org.springframework.data.relational.core.query.ValueFunction;
4343
import org.springframework.data.relational.core.sql.*;
44+
import org.springframework.data.relational.domain.SqlSort;
4445
import org.springframework.data.util.Pair;
4546
import org.springframework.data.util.TypeInformation;
4647
import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
@@ -92,13 +93,22 @@ public List<OrderByField> getMappedSort(Table table, Sort sort, @Nullable Relati
9293

9394
for (Sort.Order order : sort) {
9495

95-
Field field = createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
96-
OrderByField orderBy = OrderByField.from(table.column(field.getMappedColumnName()))
96+
OrderByField simpleOrderByField = createSimpleOrderByField(table, entity, order);
97+
OrderByField orderBy = simpleOrderByField
9798
.withNullHandling(order.getNullHandling());
9899
mappedOrder.add(order.isAscending() ? orderBy.asc() : orderBy.desc());
99100
}
100101

101102
return mappedOrder;
103+
104+
}
105+
106+
private OrderByField createSimpleOrderByField(Table table, RelationalPersistentEntity<?> entity, Sort.Order order) {
107+
108+
SqlSort.validate(order);
109+
110+
Field field = createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
111+
return OrderByField.from(table.column(field.getMappedColumnName()));
102112
}
103113

104114
/**

‎spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import org.springframework.data.jdbc.testing.EnabledOnFeature;
6363
import org.springframework.data.jdbc.testing.TestConfiguration;
6464
import org.springframework.data.jdbc.testing.TestDatabaseFeatures;
65+
import org.springframework.data.mapping.context.InvalidPersistentPropertyPath;
6566
import org.springframework.data.relational.core.conversion.DbActionExecutionException;
6667
import org.springframework.data.relational.core.mapping.Column;
6768
import org.springframework.data.relational.core.mapping.InsertOnlyProperty;
@@ -275,6 +276,17 @@ void saveAndLoadManyEntitiesWithReferencedEntitySortedWithNullPrecedence() {
275276
.containsExactly("Frozen", "Star", null);
276277
}
277278

279+
280+
@Test //
281+
@EnabledOnFeature({ SUPPORTS_QUOTED_IDS})
282+
void findByNonPropertySortFails() {
283+
284+
assertThatThrownBy(() -> template.findAll(LegoSet.class,
285+
Sort.by("somethingNotExistant"))).isInstanceOf(InvalidPersistentPropertyPath.class);
286+
287+
}
288+
289+
278290
@Test // DATAJDBC-112
279291
@EnabledOnFeature(SUPPORTS_QUOTED_IDS)
280292
void saveAndLoadManyEntitiesByIdWithReferencedEntity() {

‎spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/QueryMapperUnitTests.java‎

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,12 @@
2121

2222
import java.util.Collections;
2323
import java.util.List;
24+
import java.util.Objects;
2425

2526
import org.junit.jupiter.api.Test;
27+
import org.junit.jupiter.params.ParameterizedTest;
28+
import org.junit.jupiter.params.provider.ValueSource;
2629
import org.springframework.data.domain.Sort;
27-
import org.springframework.data.jdbc.core.convert.BasicJdbcConverter;
28-
import org.springframework.data.jdbc.core.convert.JdbcConverter;
29-
import org.springframework.data.jdbc.core.convert.QueryMapper;
30-
import org.springframework.data.jdbc.core.convert.RelationResolver;
3130
import org.springframework.data.jdbc.core.mapping.JdbcMappingContext;
3231
import org.springframework.data.relational.core.dialect.PostgresDialect;
3332
import org.springframework.data.relational.core.mapping.Column;
@@ -37,12 +36,14 @@
3736
import org.springframework.data.relational.core.sql.Functions;
3837
import org.springframework.data.relational.core.sql.OrderByField;
3938
import org.springframework.data.relational.core.sql.Table;
39+
import org.springframework.data.relational.domain.SqlSort;
4040
import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
4141

4242
/**
4343
* Unit tests for {@link QueryMapper}.
4444
*
4545
* @author Mark Paluch
46+
* @author Jens Schauder
4647
*/
4748
public class QueryMapperUnitTests {
4849

@@ -376,8 +377,60 @@ public void shouldMapSort() {
376377
List<OrderByField> fields = mapper.getMappedSort(Table.create("tbl"), sort,
377378
context.getRequiredPersistentEntity(Person.class));
378379

379-
assertThat(fields).hasSize(1);
380-
assertThat(fields.get(0)).hasToString("tbl.\"another_name\" DESC");
380+
assertThat(fields) //
381+
.extracting(Objects::toString) //
382+
.containsExactly("tbl.\"another_name\" DESC");
383+
}
384+
385+
@Test // GH-1507
386+
public void shouldMapSortWithUnknownField() {
387+
388+
Sort sort = Sort.by(desc("unknownField"));
389+
390+
List<OrderByField> fields = mapper.getMappedSort(Table.create("tbl"), sort,
391+
context.getRequiredPersistentEntity(Person.class));
392+
393+
assertThat(fields) //
394+
.extracting(Objects::toString) //
395+
.containsExactly("tbl.unknownField DESC");
396+
}
397+
398+
@Test // GH-1507
399+
public void shouldMapSortWithAllowedSpecialCharacters() {
400+
401+
Sort sort = Sort.by(desc("x(._)x"));
402+
403+
List<OrderByField> fields = mapper.getMappedSort(Table.create("tbl"), sort,
404+
context.getRequiredPersistentEntity(Person.class));
405+
406+
assertThat(fields) //
407+
.extracting(Objects::toString) //
408+
.containsExactly("tbl.x(._)x DESC");
409+
}
410+
411+
@ParameterizedTest // GH-1507
412+
@ValueSource(strings = { " ", ";", "--" })
413+
public void shouldNotMapSortWithIllegalExpression(String input) {
414+
415+
Sort sort = Sort.by(desc("unknown" + input + "Field"));
416+
417+
assertThatThrownBy(
418+
() -> mapper.getMappedSort(Table.create("tbl"), sort, context.getRequiredPersistentEntity(Person.class)))
419+
.isInstanceOf(IllegalArgumentException.class);
420+
}
421+
422+
@Test // GH-1507
423+
public void shouldMapSortWithUnsafeExpression() {
424+
425+
String unsafeExpression = "arbitrary expression that may include evil stuff like ; & --";
426+
Sort sort = SqlSort.unsafe(unsafeExpression);
427+
428+
List<OrderByField> fields = mapper.getMappedSort(Table.create("tbl"), sort,
429+
context.getRequiredPersistentEntity(Person.class));
430+
431+
assertThat(fields) //
432+
.extracting(Objects::toString) //
433+
.containsExactly("tbl." + unsafeExpression + " ASC");
381434
}
382435

383436
private Condition map(Criteria criteria) {

‎spring-data-r2dbc/src/main/java/org/springframework/data/r2dbc/query/QueryMapper.java‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import org.springframework.data.relational.core.query.CriteriaDefinition.Comparator;
3939
import org.springframework.data.relational.core.query.ValueFunction;
4040
import org.springframework.data.relational.core.sql.*;
41+
import org.springframework.data.relational.domain.SqlSort;
4142
import org.springframework.data.util.Pair;
4243
import org.springframework.data.util.TypeInformation;
4344
import org.springframework.lang.Nullable;
@@ -56,6 +57,7 @@
5657
* @author Mark Paluch
5758
* @author Roman Chigvintsev
5859
* @author Manousos Mathioudakis
60+
* @author Jens Schauder
5961
*/
6062
public class QueryMapper {
6163

@@ -111,6 +113,8 @@ public Sort getMappedObject(Sort sort, @Nullable RelationalPersistentEntity<?> e
111113

112114
for (Sort.Order order : sort) {
113115

116+
SqlSort.validate(order);
117+
114118
Field field = createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
115119
mappedOrder.add(
116120
Sort.Order.by(toSql(field.getMappedColumnName())).with(order.getNullHandling()).with(order.getDirection()));
@@ -133,13 +137,22 @@ public List<OrderByField> getMappedSort(Table table, Sort sort, @Nullable Relati
133137

134138
for (Sort.Order order : sort) {
135139

136-
Field field = createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
137-
OrderByField orderBy = OrderByField.from(table.column(field.getMappedColumnName()))
140+
OrderByField simpleOrderByField = createSimpleOrderByField(table, entity, order);
141+
OrderByField orderBy = simpleOrderByField
138142
.withNullHandling(order.getNullHandling());
139143
mappedOrder.add(order.isAscending() ? orderBy.asc() : orderBy.desc());
140144
}
141145

142146
return mappedOrder;
147+
148+
}
149+
150+
private OrderByField createSimpleOrderByField(Table table, RelationalPersistentEntity<?> entity, Sort.Order order) {
151+
152+
SqlSort.validate(order);
153+
154+
Field field = createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
155+
return OrderByField.from(table.column(field.getMappedColumnName()));
143156
}
144157

145158
/**

‎spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/query/QueryMapperUnitTests.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import static org.springframework.data.domain.Sort.Order.*;
2121

2222
import java.util.Collections;
23+
import java.util.List;
24+
import java.util.Objects;
2325

2426
import org.junit.jupiter.api.Test;
2527
import org.springframework.core.convert.converter.Converter;
@@ -35,6 +37,7 @@
3537
import org.springframework.data.relational.core.query.Criteria;
3638
import org.springframework.data.relational.core.sql.Expression;
3739
import org.springframework.data.relational.core.sql.Functions;
40+
import org.springframework.data.relational.core.sql.OrderByField;
3841
import org.springframework.data.relational.core.sql.Table;
3942
import org.springframework.r2dbc.core.Parameter;
4043
import org.springframework.r2dbc.core.binding.BindMarkersFactory;
@@ -47,6 +50,7 @@
4750
*
4851
* @author Mark Paluch
4952
* @author Mingyuan Wu
53+
* @author Jens Schauder
5054
*/
5155
class QueryMapperUnitTests {
5256

@@ -423,6 +427,42 @@ void mapSortForPropertyPathInPrimitiveShouldFallBackToColumnName() {
423427
assertThat(mapped.getOrderFor("alternative_name")).isEqualTo(desc("alternative_name"));
424428
}
425429

430+
@Test // GH-1507
431+
public void shouldMapSortWithUnknownField() {
432+
433+
Sort sort = Sort.by(desc("unknownField"));
434+
435+
List<OrderByField> fields = mapper.getMappedSort(Table.create("tbl"), sort,
436+
mapper.getMappingContext().getRequiredPersistentEntity(Person.class));
437+
438+
assertThat(fields) //
439+
.extracting(Objects::toString) //
440+
.containsExactly("tbl.unknownField DESC");
441+
}
442+
443+
@Test // GH-1507
444+
public void shouldMapSortWithAllowedSpecialCharacters() {
445+
446+
Sort sort = Sort.by(desc("x(._)x"));
447+
448+
List<OrderByField> fields = mapper.getMappedSort(Table.create("tbl"), sort,
449+
mapper.getMappingContext().getRequiredPersistentEntity(Person.class));
450+
451+
assertThat(fields) //
452+
.extracting(Objects::toString) //
453+
.containsExactly("tbl.x(._)x DESC");
454+
}
455+
456+
457+
@Test // GH-1507
458+
public void shouldNotMapSortWithIllegalExpression() {
459+
460+
Sort sort = Sort.by(desc("unknown Field"));
461+
462+
assertThatThrownBy(() -> mapper.getMappedSort(Table.create("tbl"), sort,
463+
mapper.getMappingContext().getRequiredPersistentEntity(Person.class))).isInstanceOf(IllegalArgumentException.class);
464+
}
465+
426466
@Test // gh-369
427467
void mapQueryForPropertyPathInPrimitiveShouldFallBackToColumnName() {
428468

0 commit comments

Comments
 (0)