Skip to content

Perform type validation for QueryMap and HeaderMap in Contract#573

Merged
codefromthecrypt merged 1 commit into
OpenFeign:masterfrom
EthanLozano:refactor/base-contract-upfront-check
Jul 6, 2017
Merged

Perform type validation for QueryMap and HeaderMap in Contract#573
codefromthecrypt merged 1 commit into
OpenFeign:masterfrom
EthanLozano:refactor/base-contract-upfront-check

Conversation

@EthanLozano

Copy link
Copy Markdown
Contributor

No description provided.

@EthanLozano

Copy link
Copy Markdown
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

@codefromthecrypt codefromthecrypt left a comment

Copy link
Copy Markdown

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

sunruh commented Aug 5, 2018

Copy link
Copy Markdown

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

kdavisk6 commented Aug 5, 2018

Copy link
Copy Markdown
Member

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

@sunruh

sunruh commented Aug 5, 2018

Copy link
Copy Markdown

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.

4 participants