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

Allow @RequestPart user-defined POJOs #314

Merged
merged 28 commits into from
Apr 14, 2020

Conversation

darrenfoong
Copy link
Contributor

I'm using Spring Cloud OpenFeign, and I have a use case where I need to send multiple files and user-defined POJOs as multipart/form-data, for example:

@PostMapping("/items")
SomeResponse uploadItems(@RequestPart List<SomePojo> items, @RequestPart List<MultipartFile> itemImages);

There was a recent fix in #258 that allowed the use of multiple @RequestParts, but it was only for MultipartFile and boxed primitive types. This PR builds on the abovementioned PR by:

  • Updating SpringEncoder to (optionally) use a new SpringPojoFormEncoder that comes with a PojoFormWriter, which serializes user-defined POJOs.
  • PojoFormWriter is abstract; developers can implement missing methods to serialize POJOs to any string format. In this PR, I implemented JsonPojoFormWriter, which uses Jackson.
  • Update FeignClientsConfiguration to optionally autowire a PojoFormWriter that will be used with SpringEncoder upon application start-up.
  • Update SpringMvcContract.processAnnotationsOnParameter to not expand parameters when the Content-Type is multipart/form-data. This is because a simple, single-field POJO like FeignClientTests.Hello would have been converted to a String.
  • Add unit tests.

I am not sure how best to fix the issue in processAnnotationsOnParameter and I don't have a good understand of what it does exactly. I'd like to know a better way to prevent expansion of simple POJOs like FeignClientTests.Hello.

I had earlier created a pull request on feign-form (OpenFeign/feign-form#89), but considering that the project is not regularly maintained, I decided to make a PR here instead.

I had also noticed that the recommended usage on feign-form's documentation is to create a new SpringFormEncoder(new SpringEncoder(...)), but because SpringEncoder itself has an internal instance of SpringFormEncoder, I thought it would be cleaner to make a PR here.

@codecov
Copy link

codecov bot commented Mar 29, 2020

Codecov Report

Merging #314 into 2.2.x will increase coverage by 0.17%.
The diff coverage is 83.67%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.2.x     #314      +/-   ##
============================================
+ Coverage     78.84%   79.01%   +0.17%     
- Complexity      396      411      +15     
============================================
  Files            52       54       +2     
  Lines          1541     1587      +46     
  Branches        223      230       +7     
============================================
+ Hits           1215     1254      +39     
- Misses          233      237       +4     
- Partials         93       96       +3     
Impacted Files Coverage Δ Complexity Δ
...rk/cloud/openfeign/support/AbstractFormWriter.java 72.72% <72.72%> (ø) 6.00 <6.00> (?)
...ork/cloud/openfeign/support/SpringMvcContract.java 88.82% <87.50%> (-0.13%) 56.00 <2.00> (+3.00) ⬇️
...ork/cloud/openfeign/FeignClientsConfiguration.java 97.22% <90.90%> (+1.06%) 13.00 <2.00> (+2.00)
...mework/cloud/openfeign/support/JsonFormWriter.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
...amework/cloud/openfeign/support/SpringEncoder.java 90.27% <100.00%> (+0.57%) 18.00 <1.00> (+1.00)

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

I'll let @OlgaMaciaszek comment on actual functionality as opposed to just structure and naming.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

@darrenfoong Thanks for submitting this. The logic looks good to me, but I've added some comments for you to address.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

LGTM

@OlgaMaciaszek OlgaMaciaszek merged commit 59ba6b1 into spring-cloud:2.2.x Apr 14, 2020
@OlgaMaciaszek OlgaMaciaszek added this to the 2.2.3.RELEASE milestone Apr 14, 2020
@darrenfoong
Copy link
Contributor Author

Thanks @spencergibb and @OlgaMaciaszek for reviewing and merging this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants