Skip to content

Conversation

sancho21
Copy link

Added new SimpleSortHandlerMethodArgumentResolver as an alternative to SortHandlerMethodArgumentResolver implementation.

This is for simplified sort parameter with sign. I have feeling that SortDefaultUnitTests.java should be independent of the SortHandlerMethodArgumentResolver implementation.

@odrotbohm
Copy link
Member

So what is the plan on integrating this? We can't just simply replace the current one as that would break existing applications.

Regarding the split of the unit tests for SortDefault — You now tied those tests to your implementation, which doesn't necessarily change the situation.

To be completely hones, I am not even sure I want to "solve" the issue, as we're basically just trading one way with the other and I am pretty sure, if we moved to the new format people that preferred the old format.

@sancho21
Copy link
Author

sancho21 commented Jun 28, 2016

This is not a replacement. It doesn't even touch the current functionality.

On https://jira.spring.io/browse/DATACMNS-531, you said that you want clear separation. So, I made a separate class for simple sort option. This way, users still can either choose the existing SortHandlerMethodArgumentResolver or the new SimpleSortHandlerMethodArgumentResolver as both classes are there for them to choose.

Or, is it better to just add another flag to the existing class like?

@jeffsegal
Copy link

jeffsegal commented Jul 29, 2016

+1 on this PR. It snaps nicely in and satisfies a very common pattern in APIs (+/- sort direction).

Update: Upon closer examination, I see that the code here is mostly a copy/paste of SortHandlerMethodArgumentResolver. Seems like the actual solution ought to be at least be a subclass of SortHandlerMethodArgumentResolver, but I think a new interface is actually needed here since Pageable's contain a Sort (and likewise PageableHandlerMethodArgumentResolver contains a SortHandlerMethodArgumentResolver).

This means that it becomes pretty hairy to use any API design for sort that differs from the Spring default in combination with @RequestMapping's that use Pageable. For now, I'm subclassing SortHandlerMethodArgumentResolver and then passing that to my PageableHandlerMethodArgumentResolver.

@pivotal-issuemaster
Copy link

@sancho21 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@mp911de
Copy link
Member

mp911de commented Mar 11, 2020

Thanks for your PR. Closing this one as per comment in the linked ticket as we do not intend to add additional/alternative sort formats. You can customize the format by providing your own SortHandlerMethodArgumentResolver bean.

@mp911de mp911de closed this Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants