SER-13233 Add support for sorting on multiple columns using CollectinOptions sortby. #680
Conversation
There was a problem hiding this comment.
Nice feature!
Some things to consider though:
- If there is even a slight risk for sql-injection, I don't think it is worth the risk. I currently see such a risk with this PR.
- Readability of the code is important, especially to assess the risk of the code. I think the readability and structure of the code can be improved.
- Performance is key in code that runs often. If the input looks like the old normal case, you could make an early exit without running regex and other slow stuff. Perhaps queryAnnotation.allowedSortColumns() could be a Set in order to avoid nested loops?
For me the security part is really important. This is one of the few places where an injection could happen. I would like the code to be very easy to read and that the query is only built from strings that are defined by the application, using lookups based on the input, rather than using the input and propagating it to the sql.
…nOptions sortby. Review changes.
|
I've added that if sortby contains only one column it will continue as before (without using regexp). |
…nOptions sortby. Some improvements.
JonasHallFnx
requested review from
flowertwig,
splitfeed,
jepp3 and
softqwewasd
| @@ -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))?"); | |||
There was a problem hiding this comment.
I don't know. We are a swedish company...
There was a problem hiding this comment.
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.
| } | ||
| } else { | ||
| Collections.reverse(Arrays.asList(sortByParts)); | ||
| for (var sortByPart: sortByParts) { |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| collectionOptionsHasOrderBy = true; | ||
| addOrderBy(sql, buildOrderBy(collectionOptions.getOrder(), allowed)); | ||
| break; | ||
| var sortByParts = sortBy.split(","); |
There was a problem hiding this comment.
Could be good to have a check for max length for the sortBy unless Netty's query decoder already checks that?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Added support for specifying multiple sort by columns in the CollectionOptions sortBy string separated by commas. Each column needs to be defined in allowedSortColumns in order to be used in the sort.
The sortBy string could e.g. contain the string "name asc, id desc" which would result in the corresponding sortby clause.