Fix GH-201: Multiple @RequestPart not Working #258
Conversation
|
@aaronjwhiteside Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@aaronjwhiteside Thank you for signing the Contributor License Agreement! |
Codecov Report
@@ Coverage Diff @@
## 2.2.x #258 +/- ##
============================================
+ Coverage 77.63% 78.21% +0.57%
- Complexity 386 390 +4
============================================
Files 51 52 +1
Lines 1525 1538 +13
Branches 221 222 +1
============================================
+ Hits 1184 1203 +19
+ Misses 246 239 -7
- Partials 95 96 +1
|
|
@aaronjwhiteside Thanks for the PR. Will review today. |
|
Fixes gh-201 |
|
Changing PR base branch to |
There was a problem hiding this comment.
Hi @aaronjwhiteside In general, LGTM. Great that there are thorough tests. Have requested two minor changes - please have a look. Also, please merge the recent changes from branch 2.2.x, resolve any conflicts and resubmit - will be able to merge it then.
...-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringEncoder.java
Show resolved
Hide resolved
| @@ -120,8 +120,6 @@ public void testMultipartFile1() { | |||
| MultipartFile multipartFile = new MockMultipartFile("test_multipart_file", | |||
| "hi".getBytes()); | |||
| encoder.encode(multipartFile, MultipartFile.class, request); | |||
|
|
|||
| assertThat(request.requestCharset()).as("request charset is not null").isNull(); | |||
There was a problem hiding this comment.
We want the tests to have assertions varifying the results that we are expecting to get. If the assertion becomes invalid, due to changes in code, the test should be reworked with correct assertions.
There was a problem hiding this comment.
I think I removed this because charset has no impact on multipart handling, leaving this here would give the false impression that it's needed or influences something.
There was a problem hiding this comment.
Ok. We would have to verify the commits and why this assertion was added there, to begin with.
There was a problem hiding this comment.
But since it's testing the exception let's leave it this way.
OlgaMaciaszek
added
waiting for feedback
bug
aaronjwhiteside
force-pushed
the
gh-201
branch
2 times, most recently
from
930e11c to
c8dcd4d
Compare
Summary of changes follows: - Delegate ALL requestBody encoding where Content-Type is multipart/form-data to the SpringFormEncoder. - Introduce RequestPartParameterProcessor to deal with @RequestPart annotations, adds parameters to MethodMetadata.formParams(). - Wrap HttpMessageConversionException in EncodeException. - Add tests to verify expected behaviour. However there still exists a gap in functionality where any user defined pojo will be serialized as a Map<String,String>, as there is currently no way for SpringFormEncoder to know about the Content-Type of the individual parts beyond MultipartFile, boxed primitive types (which are treated as text/plain) and other built-in types inherited from FormEncoder.
aaronjwhiteside
force-pushed
the
gh-201
branch
from
dc7c1d2 to
9437031
Compare
Summary of changes follows:
Content-Typeismultipart/form-datato theSpringFormEncoder.RequestPartParameterProcessorto deal with@RequestPartannotations, adds parameters toMethodMetadata.formParams().HttpMessageConversionExceptioninEncodeException.However there still exists a gap in functionality where any user defined pojo will be serialized as a
Map<String,String>, as there is currently no way forSpringFormEncoderto know about theContent-Typeof the individual parts beyondMultipartFile, boxed primitive types (which are treated astext/plain) and other built-in types inherited fromFormEncoder.