Skip to content

Draft: accounting for ignore case in sorting #2102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Draft: accounting for ignore case in sorting #2102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,23 +89,29 @@ public List<OrderByField> getMappedSort(Table table, Sort sort, @Nullable Relati

SqlSort.validate(order);

OrderByField simpleOrderByField = createSimpleOrderByField(table, entity, order);
OrderByField orderBy = simpleOrderByField.withNullHandling(order.getNullHandling());
mappedOrder.add(order.isAscending() ? orderBy.asc() : orderBy.desc());
if (order instanceof SqlSort.SqlOrder sqlOrder && sqlOrder.isUnsafe()) {
mappedOrder.add(OrderByField.from(Expressions.just(sqlOrder.getProperty())));
continue;
}

Field field = createField(entity, order);

OrderByField orderByField = OrderByField.from( //
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, Order By items accept an expression instead of being configured for case-insensitivity. Case-insensitive ordering requires some sort of normalization (upper or lower-case transformation) and that requires configuration as databases may have upper case or lower case indexes (e.g. spring-projects/spring-data-jpa#2420)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this concern, and I agree. The case sensitivity is configured via Order#ignoreCase. I think we might consider to add a strategy interface, something like:

interface CaseSensitiveOrderExpressionMapper {

     // What is the contract here? What is the type of an argument?
     // Is it okay to just return String? Maybe we can go for Expression 
     String fromOrder(Order order);
}

This is a global strategy, we can make it a bit more fine-grained to be able to override its behavior on the method level via potentially new annotation

public @interface @OrderNormaliztion {
   
   String orderingClause();

   // Do we need an enum here? It is limiting the possible options. The alternative is to go for a String    
   enum NormalizationFunction {
         LOWER,
         UPPER
   }
}

The abstraction names etc. can vary of course, I want to communicate the overall idea.

As far as I understood this Spring Data JPA 2420 ticket, there is a need for normalization function to be configurable. So I think we can add both the annotation and the strategy interface.

What are you thoughts on this, @mp911de

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have still too little insight for a full overview. In SQL, you can use additionally collations (ORDER BY <column> COLLATE <collation>, https://learn.microsoft.com/de-de/sql/t-sql/statements/collations?view=sql-server-ver17, https://www.postgresql.org/docs/current/collation.html#ICU-TAILORING-RULES) meaning that we can use either normalization or collations.

I started in JPA introducing JpqlQueryTemplates but that seems insufficient. Collations between the JVM and the database (e.g. upper in the database, upper on the JVM) might be different yielding different results.

The simplemost approach could be to use lowercase everywhere, a more sophisticated one to use configurable normalization and the most elaborate variant would be considering collations, maybe even on a per-table (domain type) and per-column (persistent property) basis. Let's discuss this once @schauder is back.

table.column(field.getMappedColumnName()), //
order.isAscending() ? Sort.Direction.ASC : Sort.Direction.DESC, //
order.getNullHandling(), //
order.isIgnoreCase() //
);

mappedOrder.add(orderByField);
}

return mappedOrder;

}

private OrderByField createSimpleOrderByField(Table table, RelationalPersistentEntity<?> entity, Sort.Order order) {

if (order instanceof SqlSort.SqlOrder sqlOrder && sqlOrder.isUnsafe()) {
return OrderByField.from(Expressions.just(sqlOrder.getProperty()));
}

Field field = createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
return OrderByField.from(table.column(field.getMappedColumnName()));
private Field createField(RelationalPersistentEntity<?> entity, Sort.Order order) {
return createPropertyField(entity, SqlIdentifier.unquoted(order.getProperty()), this.mappingContext);
}

/**
Expand Down
13 changes: 5 additions & 8 deletions ...src/main/java/org/springframework/data/relational/core/dialect/OrderByNullPrecedence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,11 @@ class SqlStandardOrderByNullPrecedence implements OrderByNullPrecedence {

@Override
public String evaluate(Sort.NullHandling nullHandling) {

switch (nullHandling) {
case NULLS_FIRST: return NULLS_FIRST;
case NULLS_LAST: return NULLS_LAST;
case NATIVE: return UNSPECIFIED;
default:
throw new UnsupportedOperationException("Sort.NullHandling " + nullHandling + " not supported");
}
return switch (nullHandling) {
case NULLS_FIRST -> NULLS_FIRST;
case NULLS_LAST -> NULLS_LAST;
case NATIVE -> UNSPECIFIED;
};
}
}
}
34 changes: 25 additions & 9 deletions ...a-relational/src/main/java/org/springframework/data/relational/core/sql/OrderByField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,25 @@
*
* @author Mark Paluch
* @author Milan Milanov
* @author Mikhail Polivakha
* @since 1.1
*/
public class OrderByField extends AbstractSegment {

private final Expression expression;
private final @Nullable Sort.Direction direction;
private final Sort.NullHandling nullHandling;
private final boolean caseInsensitive;

private OrderByField(Expression expression, @Nullable Direction direction, NullHandling nullHandling) {
private OrderByField(Expression expression, @Nullable Direction direction, NullHandling nullHandling, boolean caseInsensitive) {

super(expression);
Assert.notNull(expression, "Order by expression must not be null");
Assert.notNull(nullHandling, "NullHandling by expression must not be null");

this.expression = expression;
this.direction = direction;
this.caseInsensitive = caseInsensitive;
this.nullHandling = nullHandling;
}

Expand All @@ -52,38 +55,51 @@ private OrderByField(Expression expression, @Nullable Direction direction, NullH
* @return the {@link OrderByField}.
*/
public static OrderByField from(Expression expression) {
return new OrderByField(expression, null, NullHandling.NATIVE);
return new OrderByField(expression, null, NullHandling.NATIVE, false);
}

/**
* Creates a new {@link OrderByField} from an {@link Expression} applying a given ordering.
*
* @param expression must not be {@literal null}.
* @param direction order direction
* @param direction order direction.
* @return the {@link OrderByField}.
*/
public static OrderByField from(Expression expression, Direction direction) {
return new OrderByField(expression, direction, NullHandling.NATIVE);
return new OrderByField(expression, direction, NullHandling.NATIVE, false);
}

/**
* Creates a new {@link OrderByField} from a the current one using ascending sorting.
* Creates a new {@link OrderByField} from an {@link Expression} applying a given ordering and {@link NullHandling}.
*
* @param expression must not be {@literal null}.
* @param direction order direction.
* @param nullHandling the null handling strategy.
* @param caseInsensitive whether the ordering is case-insensitive.
* @return the {@link OrderByField}.
*/
public static OrderByField from(Expression expression, Direction direction, NullHandling nullHandling, boolean caseInsensitive) {
return new OrderByField(expression, direction, nullHandling, caseInsensitive);
}

/**
* Creates a new {@link OrderByField} from a current one using ascending sorting.
*
* @return the new {@link OrderByField} with ascending sorting.
* @see #desc()
*/
public OrderByField asc() {
return new OrderByField(expression, Direction.ASC, nullHandling);
return new OrderByField(expression, Direction.ASC, nullHandling, caseInsensitive);
}

/**
* Creates a new {@link OrderByField} from a the current one using descending sorting.
* Creates a new {@link OrderByField} from a current one using descending sorting.
*
* @return the new {@link OrderByField} with descending sorting.
* @see #asc()
*/
public OrderByField desc() {
return new OrderByField(expression, Direction.DESC, nullHandling);
return new OrderByField(expression, Direction.DESC, nullHandling, caseInsensitive);
}

/**
Expand All @@ -93,7 +109,7 @@ public OrderByField desc() {
* @return the new {@link OrderByField} with {@link NullHandling} applied.
*/
public OrderByField withNullHandling(NullHandling nullHandling) {
return new OrderByField(expression, direction, nullHandling);
return new OrderByField(expression, direction, nullHandling, caseInsensitive);
}

public Expression getExpression() {
Expand Down
9 changes: 7 additions & 2 deletions ...ional/src/main/java/org/springframework/data/relational/core/sql/render/NameRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,13 @@ static CharSequence fullyQualifiedReference(RenderContext context, Column column
return render(context, namingStrategy.getReferenceName(column));
}

return render(context, SqlIdentifier.from(namingStrategy.getReferenceName(column.getTable()),
namingStrategy.getReferenceName(column)));
return render( //
context, //
SqlIdentifier.from( //
namingStrategy.getReferenceName(column.getTable()), //
namingStrategy.getReferenceName(column) //
)
);
}

/**
Expand Down