Skip to content
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.

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

HateoasSortHandlerMethodArgumentResolver should append * to the 'sort' parameter template variable. #2531

Closed
dayre opened this issue Jan 15, 2022 · 1 comment
Assignees
Labels
type: bug A general bug

Comments

@dayre
Copy link

dayre commented Jan 15, 2022

The Spring docs (see Table 1) specify that multiple sort parameters should be present in the request params for Pageable instances when sorting by multiple fields. (e.g. sort=field1,asc&sort=field2,desc).

Spring HATEOAS via the data commons HateoasSortHandlerMethodArgumentResolver class generates a URI template variable for the 'sort' param without the URI template explode modifier (*).

http://localhost:8080/api/clients{?page,size,sort,projection}

For client side libraries performing URI expansion based on rfc6570, in conditions where there are multiple sort parameters, the template provided by Spring without the modifier will generate URLs of the following format:

// using https://www.npmjs.com/package/uri-template based on rfc6570
const templ = uriTemplate.parse("http://localhost:8080/api/clients{?page,size,sort,projection}");
const expanded = templ.expand({sort : ["id,asc", "lastModifiedDate,desc"]});

// BAD -> http://localhost:8080/api/clients?sort=id%2Casc,lastModifiedDate%2Cdesc

This URL isn't in the correct format outlined in the Spring docs (see Table 1), and a format not expected by the handlers resulting in incorrect parsing (results in two order by's, but direction is DESC for both).

The explode modifier added to sort would allow rfc6570 compliant libraries to generate the correct URLs expected by Spring and allow correct sorting criteria to be generated:

const templ = uriTemplate.parse("http://localhost:8080/api/clients{?page,size,sort*,projection}");
const expanded = templ.expand({sort : ["id,asc", "lastModifiedDate,desc"]});

// GOOD -> http://localhost:8080/api/clients?sort=id%2Casc&sort=lastModifiedDate%2Cdesc

There is an existing related issue #1242, which deals with multiple values and generated query method templates... which i can see needs addressing deeper in the HATEOAS codebase, but to fix sorting properly by anything Pageable, it seems like a simple fix would be to append "*" to the sort param generated by this class.

org.springframework.data.web.HateoasSortHandlerMethodArgumentResolver#getSortTemplateVariables

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 15, 2022
@christophstrobl christophstrobl added type: enhancement A general enhancement theme: 3.0 and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 17, 2022
odrotbohm pushed a commit that referenced this issue Dec 1, 2023
HateoasSortHandlerMethodArgumentResolver now renders the sort template variable as composite to properly indicate that it can be submitted multiple times.

Fixes GH-2531.
Original pull request GH-2945.
@odrotbohm odrotbohm added type: bug A general bug and removed type: enhancement A general enhancement theme: 3.1 labels Dec 1, 2023
@odrotbohm odrotbohm added this to the 3.1.7 (2023.0.7) milestone Dec 1, 2023
odrotbohm pushed a commit that referenced this issue Dec 1, 2023
HateoasSortHandlerMethodArgumentResolver now renders the sort template variable as composite to properly indicate that it can be submitted multiple times.

Fixes GH-2531.
Original pull request GH-2945.
odrotbohm added a commit that referenced this issue Dec 12, 2023
Since the introduction of 6e22ffd, the sort parameter has been rendered as composite parameter which will cause the comma used to delimit property from direction potentially be encoded by clients following the URI template RFC. Thus, we need to defensively decode the source parameter value before parsing.

Related ticket: GH-2531.
odrotbohm added a commit that referenced this issue Dec 12, 2023
This reverts commit 40edfdf as the change has too many side effects to be included in a bugfix version.

Related ticket: GH-2531.
odrotbohm added a commit that referenced this issue Dec 12, 2023
This reverts commit fa9d7bd as the change has too many side effects to be included in a bugfix version.

Related ticket: GH-2531.
@odrotbohm
Copy link
Member

I've reverted the back ports as the change has too many side effects to the URI handling to introduce it in bugfix releases. The switch to a composite parameter causes clients adhering to the URI template RFC to now encode the comma, which effectively breaks the API. I've put measures in place in SortHandlerMethodArgumentResolver to deal with that case but we cannot ship that change in behavior in bugfix releases.

odrotbohm added a commit that referenced this issue Dec 12, 2023
Since the introduction of 6e22ffd, the sort parameter has been rendered as composite parameter which will cause the comma used to delimit property from direction potentially be encoded by clients following the URI template RFC. Thus, we need to defensively decode the source parameter value before parsing.

Related ticket: GH-2531.
mp911de added a commit that referenced this issue Dec 13, 2023
Fix Javadoc reference. Reorder tags.

See #2531
mp911de added a commit that referenced this issue Dec 13, 2023
Fix Javadoc reference. Reorder tags.

See #2531
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Dec 13, 2023
christophstrobl added a commit to christophstrobl/spring-aot-smoke-tests that referenced this issue Mar 27, 2024
wilkinsona pushed a commit to spring-projects/spring-aot-smoke-tests that referenced this issue Mar 27, 2024
Adapt the affected smoke test to the changes made for
spring-projects/spring-data-commons#2531.

See gh-211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants