Encode Pageable.Sort with percent encoded comma. Fixes gh-440. #467
Conversation
|
@KENNYSOFT Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@KENNYSOFT Thank you for signing the Contributor License Agreement! |
Codecov Report
@@ Coverage Diff @@
## master #467 +/- ##
=========================================
Coverage 76.91% 76.91%
Complexity 438 438
=========================================
Files 54 54
Lines 1633 1633
Branches 242 242
=========================================
Hits 1256 1256
Misses 256 256
Partials 121 121
|
|
Fixes gh-440. |
| assertThat(pageable.getPageSize()).isEqualTo(response.getBody().getSize()); | ||
| assertThat(response.getBody().getPageable().getSort()).hasSize(2); | ||
| List<Sort.Order> orderList = response.getBody().getPageable().getSort().toList(); | ||
| if (!orderList.isEmpty()) { |
| assertThat(pageable.getPageSize()).isEqualTo(response.getBody().getSize()); | ||
| assertThat(response.getBody().getPageable().getSort()).hasSize(1); | ||
| Optional<Sort.Order> optionalOrder = response.getBody().getPageable().getSort().get().findFirst(); | ||
| if (optionalOrder.isPresent()) { |
There was a problem hiding this comment.
I'd replace if with assert, something like
assertThat(optionalOrder.isPresent()).isTrue();
There was a problem hiding this comment.
Actually it's guaranteed because we asserted that ...getSort() has size 1.
There was a problem hiding this comment.
But yeah changed. 86785dc
There was a problem hiding this comment.
Since it's rebased, now in c08b873
There was a problem hiding this comment.
@KENNYSOFT Other than some minor formatting issues, the PR looks good. However, similarly as with the previous one, please submit it against 2.2.x so that the changes are included in the Hoxton release train as well.
| @@ -87,6 +89,66 @@ public void testPageable() { | |||
|
|
|||
| } | |||
|
|
|||
| @Test | |||
| public void testPageableWithDescDirection() { | |||
|
|
|||
There was a problem hiding this comment.
Please remove the unnecessary empty line.
There was a problem hiding this comment.
Resolved in 364be92
| Sort.Order order = optionalOrder.get(); | ||
| assertThat(order.getDirection()).isEqualTo(Sort.Direction.DESC); | ||
| assertThat(order.getProperty()).isEqualTo("sortProperty"); | ||
|
|
There was a problem hiding this comment.
Please remove the unnecessary empty line.
|
|
||
| @Test | ||
| public void testPageableWithMultipleSort() { | ||
|
|
There was a problem hiding this comment.
Please remove the unnecessary empty line.
There was a problem hiding this comment.
Resolved in 364be92
| Sort.Order secondOrder = orderList.get(1); | ||
| assertThat(secondOrder.getDirection()).isEqualTo(Sort.Direction.ASC); | ||
| assertThat(secondOrder.getProperty()).isEqualTo("sortProperty2"); | ||
|
|
There was a problem hiding this comment.
Please remove the unnecessary empty line.
|
Test fails as well but I saw the comments 🙃 |
I've modified https://github.com/spring-cloud/spring-cloud-openfeign/blob/faff6aef8acdf2367c1b5596a384cbe86e1200c5/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/PageableSpringEncoder.java as suggested in #440 along with corresponding tests.
spring-cloud-openfeign/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/PageableSpringEncoder.java
Line 119 in faff6ae
If you run tests which I add without the patch of
PageSpringEncoder.javaand you'll be able to see the test fails.