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

Spring Data Pageable Sort in Feign Client #146

Closed
sebastian3456 opened this issue Mar 13, 2019 · 19 comments
Closed

Spring Data Pageable Sort in Feign Client #146

sebastian3456 opened this issue Mar 13, 2019 · 19 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@sebastian3456
Copy link

The new pageable support in version 2.1.1 can't handle the sorting in the pageable.

Here is my code:
@RequestMapping(method = RequestMethod.GET, produces = MediaType.APPLICATION_JSON_VALUE) Page<T> findAll(@RequestParam("parameters") Map<String, Collection<String>> parameters, @RequestBody Pageable page);

When I call this method size and page parameters are mapped fine, sorting is not working.
Let's expect I have a pageable with a sort on id,asc.
So the request url should look like example.com/api/test?page=0&size=10&sort=id,ASC
But the actual request url looks like example.com/api/test?page=0&size=10&sort=id&sort=ASC

@ryanjbaxter
Copy link
Contributor

Can you provide a complete, minimal, verifiable sample that reproduces the problem? It should be available as a GitHub (or similar) project or attached to this issue as a zip file.

@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@jbrmg
Copy link

jbrmg commented Apr 1, 2019

@ryanjbaxter I created a sample: https://github.com/jbrmg/openfeign-issue-146 . Please see the class Issue146Test for the executable test code. The test fails as expected.

As stated by @sebastian3456, the problem is that the sort query param is split into two parameters in the request url. This however does not get parsed by other spring boot services. This seems to be caused by the CollectionFormat for the RequestTemplate being CollectionFormat.EXPLODED, which performs a split by , on the original query param set by the PageableQueryEncoder.

@colinzhy
Copy link

colinzhy commented Apr 9, 2019

Also, encounter this issue, any suggestion?

My workaround is extending the PageableSpringEncoder, override the encode method and update the collectionFormat to CollectionFormat.CSV

  @Override
  public void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException {
    template.collectionFormat(CollectionFormat.CSV);
    super.encode(object, bodyType, template);
  }

Any better solution? I know we can set collectionFormat in @RequestLine , how about to use Spring?

@nciriglianoupgrade
Copy link

@sebastian3456 the issue was solved here: OpenFeign/feign#930
please update io.github.openfeign dependencies to a higher version than 10.2.0 (last version with this bug)

@ryanjbaxter ryanjbaxter added this to the 2.2.0.M1 milestone Jun 21, 2019
@spencergibb spencergibb removed this from the 2.2.0.M1 milestone Jul 3, 2019
@ryanjbaxter
Copy link
Contributor

We updated to OpenFeign 10.4.0 in this fix eecc51e

Please reopen if this did not fix the issue

@golartes
Copy link

Is that really works now? I use spring-cloud-starter-openfeign 2.2.2.RELEASE with feign-core 10.7.4. And I have the same problem and only WA by @colinzhy helps me now.

@rvervaek
Copy link
Contributor

I can verify @golartes comment. I'm too using spring-cloud-starter-openfeign 2.2.2.RELEASE which uses feign-core 10.7.4.

I'm experiencing the same issue. The sort queryparameter is being expanded by the QueryTemplate class in feign-core, which is not the desired behavior since the documentation states that sort parameters values should/can be provided in CSV format: https://docs.spring.io/spring-data/commons/docs/current/reference/html/#core.web.basic.paging-and-sorting

@karbi
Copy link

karbi commented May 11, 2020

Issue should be reopened. There was change in Feign that reintroduced the issue: OpenFeign/feign#1138

Proper way to pass collection parameters is separating by commas and specifing collection format. When using raw Feign library this could be accomplished by passing collectionFormat parameter in @RequestLine annotation.

Possible solution for Spring Cloud integration could be creating annotation for specifing collection format and hadling in SpringMvcContract.

Here is my workaround.

@Configuration
public class CsvFormatConfiguration {

    @Autowired(required = false)
    private List<AnnotatedParameterProcessor> parameterProcessors = new ArrayList<>();

    @Bean
    public Contract feignContract(final ConversionService feignConversionService) {
        return new CsvContract(this.parameterProcessors, feignConversionService);
    }

    public static class CsvContract extends SpringMvcContract {

        public CsvContract(final List<AnnotatedParameterProcessor> annotatedParameterProcessors, final ConversionService conversionService) {
            super(annotatedParameterProcessors, conversionService);
        }

        @Override
        protected void processAnnotationOnMethod(final MethodMetadata data, final Annotation methodAnnotation, final Method method) {
            super.processAnnotationOnMethod(data, methodAnnotation, method);

            data.template().collectionFormat(CollectionFormat.CSV);
        }
    }
}

@nickcaballero-mm
Copy link

nickcaballero-mm commented May 19, 2020

Seeing the same issue as mentioned by @karbi when passing Pageable. A custom encoder can also be used as a workaround, temporarily changing the default collection format:

public class CustomPageableEncoder extends PageableSpringEncoder {

    public CustomPageableEncoder(Encoder delegate) {
        super(delegate);
    }

    @Override
    public void encode(Object object, Type bodyType, RequestTemplate template) {
        CollectionFormat previousFormat = template.collectionFormat();
        template.collectionFormat(CollectionFormat.CSV);
        super.encode(object, bodyType, template);
        template.collectionFormat(previousFormat);
    }
}

Update: After testing the above, it only really works for a single sort parameter. Multiple sort parameters still breaks. As a workaround, I've resorted to encoding the comma to %2C.

@OlgaMaciaszek
Copy link
Collaborator

Will verify.

@OlgaMaciaszek OlgaMaciaszek reopened this Jun 4, 2020
@OlgaMaciaszek OlgaMaciaszek self-assigned this Jun 4, 2020
@OlgaMaciaszek
Copy link
Collaborator

Was able to reproduce it. We'll need to introduce support for CollectionFormat in order to make it work.

@OlgaMaciaszek OlgaMaciaszek removed their assignment Jun 5, 2020
@OlgaMaciaszek OlgaMaciaszek added bug Something isn't working and removed in progress labels Jun 5, 2020
@OlgaMaciaszek OlgaMaciaszek self-assigned this Jun 5, 2020
@OlgaMaciaszek OlgaMaciaszek added this to the 2.2.4.RELEASE milestone Jun 5, 2020
@jonseah
Copy link

jonseah commented Jun 29, 2020

Hi, is it possible to make this change for Greenwich.SRX?

@spencergibb
Copy link
Member

We are no longer adding features to Greenwich

@royremi
Copy link

royremi commented Aug 12, 2020

I don't know if it is related, but we upgraded from Hoxton.SR2 to Hoxton.SR7 and we started to have an issue with sorting.

When I look at the request I see something like that

Hoxton.SR2 = /api/v1/driver/?statuses=Active&page=0&size=10&sort=driver_filename,ASC]
Hoxton.SR7 = /api/v1/driver/?statuses=Active&page=0&size=10&sort=driver_filename&sort=ASC]

if you look you have 2 times the same request parameter. sort=driver_filename&sort=ASC.

Is something change in config I need to fix or its a bug on your side?

Thank you!

@royremi
Copy link

royremi commented Aug 12, 2020

I guess Ill open a new bug, this one is close...

@rezanachmad
Copy link

@royremi I also got the same issue in Hoxton.SR7. It is solved after I add @CollectionFormat(feign.CollectionFormat.CSV) in @GetMapping as shown in the documentation

        @FeignClient(name = "demo")
	protected interface PageableFeignClient {

		@CollectionFormat(feign.CollectionFormat.CSV)
		@GetMapping(path = "/page")
		ResponseEntity performRequest(Pageable page);
	}

@alabotski
Copy link

alabotski commented Apr 7, 2021

I have similar problem....

My feign client and has this annotation CollectionFormat (feign.CollectionFormat.CSV)
But it does not help me = (

@FeignClient(name = "BaseCaseFeignClient", url = "${feign.services.host-backend.url}")
public interface BaseCaseFeignClient {
    @PostMapping("/api/cases/base/search")
    @CollectionFormat (feign.CollectionFormat.CSV)
    Page<FoundRecordDto> searchCases(@RequestBody FilterSpecification filter, @SpringQueryMap Pageable pageable);
}

If I build a project without SpringQueryMap, then I get the following error

Page<FoundRecordDto> searchCases(@RequestBody FilterSpecification filter, Pageable pageable);

Caused by: java.lang.IllegalStateException: Method has too many Body parameters: public abstract org.springframework.data.domain.Page ru..BaseCaseFeignClient.searchCases(ru.ilter.shared.basecase.search.FilterSpecification,org.springframework.data.domain.Pageable)
Warnings:

@alabotski
Copy link

Updated spring-cloud-version to version 3.0.2 helped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests