Skip to content
This repository was archived by the owner on Nov 26, 2024. It is now read-only.

SER-13233 Add support for sorting on multiple columns using CollectinOptions sortby. #680

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

SER-13233 Add support for sorting on multiple columns using CollectinOptions sortby. #680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d7a52ab
SER-13233 Add support for sorting on multiple columns using Collectio…
ulfhagard Jan 17, 2024
6e8b328
SER-13233 Add support for sorting on multiple columns using Collectio…
ulfhagard Jan 19, 2024
188ec2e
SER-13233 Add support for sorting on multiple columns using Collectio…
ulfhagard Jan 21, 2024
7abd4f9
Merge branch 'master' into SER-13233-add-sort-on-multiple-columns
YouWillNeverWalkAl1 Jan 23, 2024
7493e75
Merge branch 'master' into SER-13233-add-sort-on-multiple-columns
YouWillNeverWalkAl1 Feb 1, 2024
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
37 changes: 32 additions & 5 deletions dao/src/main/java/se/fortnox/reactivewizard/db/query/parts/CollectionOptionsQueryPart.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

import java.lang.reflect.Method;
import java.sql.SQLException;
import java.util.Arrays;
import java.util.Collections;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static com.google.common.collect.Iterables.indexOf;
import static java.util.Arrays.asList;
Expand All @@ -17,6 +21,7 @@ public class CollectionOptionsQueryPart implements QueryPart {
private final int collectionOptionsArgIndex;
private final Query queryAnnotation;
private final boolean isMono;
private static final Pattern SORT_BY_PART = Pattern.compile("(?<sortby>[a-zåäö0-9_]+)(\\s+(?<order>asc|desc))?");
Copy link
Member

Choose a reason for hiding this comment

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

Do we need åäö?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know. We are a swedish company...

Copy link
Member

Choose a reason for hiding this comment

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

I want to think that åäö is not being used even though Fortnox is a swedish company :). Regardless, it is probably ok to leave it as is.


public CollectionOptionsQueryPart(Method method) {
collectionOptionsArgIndex = indexOf(asList(method.getParameterTypes()), CollectionOptions.class::isAssignableFrom);
Expand All @@ -36,11 +41,33 @@ public void visit(StringBuilder sql, Object[] args) {
if (collectionOptions != null) {
if (collectionOptions.getSortBy() != null) {
String sortBy = CamelSnakeConverter.camelToSnake(collectionOptions.getSortBy());
for (String allowed : queryAnnotation.allowedSortColumns()) {
if (allowed.equals(sortBy)) {
collectionOptionsHasOrderBy = true;
addOrderBy(sql, buildOrderBy(collectionOptions.getOrder(), allowed));
break;
var sortByParts = sortBy.split(",");
Copy link
Member

@softqwewasd softqwewasd Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could be good to have a check for max length for the sortBy unless Netty's query decoder already checks that?

Copy link
Author

@YouWillNeverWalkAl1 YouWillNeverWalkAl1 Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not know. sortBy is however not a new parameter and that change request is not addressing this change. How long should it be?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but we are now splitting it and running a regex for each column when sorting by multiple columns compared to only sorting by a single column with an equals comparison? I'd say 256 bytes is a reasonable limit, but we can put it at 1024 to be on the safe side, doesn't make that much of a difference performance wise.

if (sortByParts.length == 1) {
for (String allowed : queryAnnotation.allowedSortColumns()) {
if (allowed.equals(sortBy)) {
collectionOptionsHasOrderBy = true;
addOrderBy(sql, buildOrderBy(collectionOptions.getOrder(), allowed));
break;
}
}
} else {
Collections.reverse(Arrays.asList(sortByParts));
for (var sortByPart: sortByParts) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to filter out the columns that have already been appended to the ordering. For instance, sortBy="name asc, id desc, name asc, id desc, ..." should not become "ORDER BY name ASC, id DESC, name ASC, id DESC, ..." but rather "ORDER BY name ASC, id DESC".

Copy link
Author

Choose a reason for hiding this comment

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

Would make the code more complicated and would not change the result. It is also a fault made by the user of RW and should not occur.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it would make the code more complicated and would not change the output. I am not sure I understand what you mean that it is a fault made by the user of RW and should not occur. Do you mean that the responsibility of dealing with "sortby=id asc, name desc, id asc, name desc, ..." falls on the application developer?

Even though it would make the code more complicated and doesn't change the output (as well as probably a slight performance impact), I prefer to see the filtering because I find it unreasonable that we would allow an end user to craft a query that looks like that. Furthermore, while it may not be a security concern related to postgres, I have other security concerns that we can discuss in private.

Matcher matcher = SORT_BY_PART.matcher(sortByPart);
if (matcher.find()) {
var sortByColumn = matcher.group("sortby");
if (sortByColumn != null) {
var order = matcher.group("order");
for (String allowed : queryAnnotation.allowedSortColumns()) {
if (allowed.equals(sortByColumn)) {
collectionOptionsHasOrderBy = true;
var sortOrder = order != null ? CollectionOptions.SortOrder.valueOf(order.toUpperCase()) : collectionOptions.getOrder();
addOrderBy(sql, buildOrderBy(sortOrder, allowed));
break;
}
}
}
}
}
}
}
Expand Down
17 changes: 17 additions & 0 deletions dao/src/test/java/se/fortnox/reactivewizard/db/CollectionOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,20 @@ void shouldAddDefaultSortBeforeQueryOrderBy() throws SQLException {
mockDb.verifySelect("select * from table order by text desc, id LIMIT 101");
}

@Test
void shouldAddMultipleSort() throws SQLException {
CollectionOptions collectionOptions = new CollectionOptions("name asc, id desc", CollectionOptions.SortOrder.ASC);
collectionOptionsDao.selectWithDefaultSortingAllowMultiple(collectionOptions).blockFirst();
mockDb.verifySelect("select * from table order by name ASC, id DESC, id LIMIT 101");
}

@Test
void shouldFallbackToCollectionOptionSortOrderMultipleSort() throws SQLException {
CollectionOptions collectionOptions = new CollectionOptions("name, id", CollectionOptions.SortOrder.DESC);
collectionOptionsDao.selectWithDefaultSortingAllowMultiple(collectionOptions).blockFirst();
mockDb.verifySelect("select * from table order by name DESC, id DESC, id LIMIT 101");
}

@Test
void shouldAddOrderByBeforeQueryOrderByWithoutDefaultSort() throws Exception {
CollectionOptions collectionOptions = new CollectionOptions("name", CollectionOptions.SortOrder.ASC);
Expand Down Expand Up @@ -201,6 +215,9 @@ interface CollectionOptionsDao {
@Query(value = "select * from table order by id", allowedSortColumns = {"name"})
Flux<String> selectWithDefaultSorting(CollectionOptions collectionOptions);

@Query(value = "select * from table order by id", allowedSortColumns = {"name","id"})
Flux<String> selectWithDefaultSortingAllowMultiple(CollectionOptions collectionOptions);

@Query(value = "select * from table order by id", allowedSortColumns = {"name"}, defaultSort = "text desc")
Flux<String> selectWithDefaultSortingInQueryAndOptions(CollectionOptions collectionOptions);

Expand Down