fix: reject invalid order parameter values DHIS2-16935

[teleivo]

Fixes issue introduced in #15803. Also fixes issues we had before that. https://dhis2.atlassian.net/browse/DHIS2-16935 has a table with pre/post #15803 behaviors.

We now support

1. comma-separated values in a single order parameter like in order=createdAt:desc,event:asc
2. multiple order parameters with a value each like in order=createdAt:desc&order=event:asc

and reject

• mixing the above like in order=createdAt:desc,event:asc&order=completedAt:asc

Same is true for UID.

Tests

• Adapt tests to use the target types instead of asserting using string contains
• Add missing cases

Changes

To OrderCriteria

• Rename OrderCriteria factory to valueOf so it's used by Springs conversion system
• Remove OrderCriteriaParamEditor as it's not needed if we have a constructor/factory for String -> target type. In fact its even causing the issue that "order=createdAt:desc,event:asc" is not split by Spring and passed as is to the PropertyEditor
• Throw an IllegalArgumentException in case conversion failed as suggested by Spring. We returned null which lead to successful conversions with null elements. The null element lead to the HTTP 500 in the end
• Remove empty fields due to extra commas like in order=createdAt,,scheduledAt:desc to prevent any issues downstream
• Handle ConversionException and try to identify the mixed case to report a meaningful error

To UID

• Remove PropertyEditor for UID as it has a constructor taking a String to UID

Error cases

1. Invalid sort direction
   (GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/events?fields=event&order=createdAt:desc&order=event:wrong)

{
  "httpStatus": "Bad Request",
  "httpStatusCode": 400,
  "status": "ERROR",
  "message": "Value 'event:wrong' is not valid for parameter order. 'wrong' is not a valid sort direction. Valid values are: [ASC, DESC]."
}

2. Missing field name
   (GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/events?fields=event&order=createdAt:desc&order=:desc)

{
  "httpStatus": "Bad Request",
  "httpStatusCode": 400,
  "status": "ERROR",
  "message": "Value ':desc' is not valid for parameter order. Missing field name in order property: ':desc'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'."
}

3. Mixing repeated order request parameter with comma-separated values within at least one leads to
   (GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/events?fields=event&order=createdAt:desc,event:asc&order=completedAt:asc)

{
  "httpStatus": "Bad Request",
  "httpStatusCode": 400,
  "status": "ERROR",
  "message": "You likely repeated request parameter 'order' and used multiple comma-separated values within at least one of its values. Choose one of these approaches. Invalid order property: 'createdAt:desc,event:asc'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'."
}

4. to many : separators
   (GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/events?fields=event&order=createdAt:desc&order=event:asc:some)

{
  "httpStatus": "Bad Request",
  "httpStatusCode": 400,
  "status": "ERROR",
  "message": "Value 'event:asc:some' is not valid for parameter order. Invalid order property: 'event:asc:some'. Valid formats are 'field:direction' or 'field'. Valid directions are 'asc' or 'desc'. Direction defaults to 'asc'."
}

Spring Type Conversion

This describes how it worked after #15803. The behavior before was different as we used a converter for String to List<OrderCriteria>. This was also not without its flaws as you can see in https://dhis2.atlassian.net/browse/DHIS2-16935.

A request parameter like order=createdAt,event:desc that has comma separated values and is not repeated will be split by

https://github.com/spring-projects/spring-framework/blob/d81619addd6d9060c9d8dc454c359054e517ed98/spring-core/src/main/java/org/springframework/core/convert/support/StringToCollectionConverter.java#L68

Each element will then be converted by Springs' conversion service.

Since Spring knows the target class of OrderCriteria it looks for a constructor or factory mapping a String to the target type OrderCriteria.

Here are the factories it is looking for
https://github.com/spring-projects/spring-framework/blob/d81619addd6d9060c9d8dc454c359054e517ed98/spring-core/src/main/java/org/springframework/core/convert/support/ObjectToObjectConverter.java#L191-L197

If there is no such method or if instantiation fails it will call a registered PropertyEditor for the target type.

Since OrderCriteria did not have a suitable method for String -> OrderCriteria Spring called OrderCriteriaParamEditor with the entire String createdAt,event:desc.

OrderCriteriaParamEditor calls a factory for String -> OrderCriteria which can only handle a single order value. AFAIK a PropertyEditor is also expected to turn a String into one instance of a type.

https://github.com/dhis2/dhis2-core/pull/15803/files#diff-d6b509407da006d686ce32cd61fc18a99968ccb8f248bc3b3413cd9de29b9954L41-R36

Repeated parameters are treated differently by Spring. order=FV4JCI73wO2,oosW4rSH7CM&order=LKQXmteRtvQ,sxPParLSimC would be represented as String[] {"FV4JCI73wO2,oosW4rSH7CM", "LKQXmteRtvQ,sxPParLSimC"}. ArrayToCollectionConverter converts the String[] of into List/Set/... of the target type. Here Strings with comma-separated values will not be split!

Related Spring issues

• Percent encoded commas are treated as commas
  Why escaped comma is the delimiter for @RequestParam List<String>? spring-projects/spring-framework#23820
  Why escaped comma is the delimiter for @RequestParam List<String>? spring-projects/spring-framework#23820 (comment)
• Suggestion https://stackoverflow.com/questions/67593558/different-behaviors-observed-for-a-requestparam-in-spring-boot
• Inconsistent Behaviour for @RequestParam MultiValueMap<T, T> and @RequestParam Collection<T> with Delimiter String spring-projects/spring-framework#24641

[codecov]

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 66.92%. Comparing base (9eb5407) to head (6b479be).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #16673   +/-   ##
=========================================
  Coverage     66.92%   66.92%           
- Complexity    32035    32036    +1     
=========================================
  Files          3552     3550    -2     
  Lines        131537   131554   +17     
  Branches      15315    15321    +6     
=========================================
+ Hits          88031    88043   +12     
- Misses        36346    36348    +2     
- Partials       7160     7163    +3     

Flag	Coverage Δ
integration	50.20% <32.35%> (-0.01%)	⬇️
integration-h2	33.75% <8.82%> (-0.01%)	⬇️
unit	30.73% <85.29%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...api/controller/event/webrequest/OrderCriteria.java	94.44% <100.00%> (+1.11%)	⬆️
...p/dhis/webapi/controller/CrudControllerAdvice.java	67.39% <79.16%> (+0.72%)	⬆️

... and 5 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9eb5407...6b479be. Read the comment docs.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.6% Duplication on New Code

See analysis details on SonarCloud