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

Perform type validation for QueryMap and HeaderMap in Contract #573

Conversation

EthanLozano
Copy link
Contributor

No description provided.

@EthanLozano
Copy link
Contributor Author

#567 concluded that more checks should be done upfront in the Contract. This PR aims to perform both the Map validation AND the Map's String key validation in the contract. This reduces the number of checks that happen at "runtime" within the ReflectiveFeign class

@EthanLozano EthanLozano force-pushed the refactor/base-contract-upfront-check branch from 08b414d to 3016dd3 Compare July 5, 2017 20:08
@EthanLozano EthanLozano force-pushed the refactor/base-contract-upfront-check branch from 3016dd3 to 45139d0 Compare July 5, 2017 20:12
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely

@codefromthecrypt codefromthecrypt merged commit a029afd into OpenFeign:master Jul 6, 2017
@sunruh
Copy link

sunruh commented Aug 5, 2018

This change breaks clients which use a method parameter as HeaderMap which is non-generic (like Spring's HttpHeaders).

The following used to work before the PR (when using spring-cloud-openfeign):

@RequestMapping(method = RequestMethod.POST, path = "/path")
void method(@RequestHeader HttpHeaders headers);

It now fails with java.lang.ClassCastException: java.lang.Class cannot be cast to java.lang.reflect.ParameterizedType as HttpHeaders is non-generic but implements MultiValueMap<String, String> which in turn implements Map<String, List<String>>.

As a workaround HttpHeaders can be changed to MultiValueMap<String, List<String>>.

@kdavisk6
Copy link
Member

kdavisk6 commented Aug 5, 2018

@sunruh this bug was corrected in Feign 9.7 via #689

@sunruh
Copy link

sunruh commented Aug 5, 2018

I must have missed that PR looking for fixes. Thanks for the quick feedback.

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.

None yet

4 participants