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

MockPart in request is not recieved in RequestPart in controller #26501

Closed
LickIt opened this issue Feb 3, 2021 · 7 comments
Closed

MockPart in request is not recieved in RequestPart in controller #26501

LickIt opened this issue Feb 3, 2021 · 7 comments
Assignees
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@LickIt
Copy link

LickIt commented Feb 3, 2021

Affects version: 5.3.3
Related to: #26261

I have a controller method that accepts multipart/form-data requests with 2 parameters: a file and a @RequestPart String label.
Previously in my tests I was creating a MockPart for the label and it was working fine: new MockPart("label", "mainImage".getBytes(UTF_8)).

With the latest changes in 5.3.3 the MockPart is converted to a request parameter instead of a multipart boundary and I don't receive anything for the RequestPart in the controller thus breaking the tests.

Why is it necessary to convert MockParts to parameters in a multipart request?
I'm not sure if this is strictly related to the value being a parameter of if there is another underlying issue. I thought that I should receive the value by using either RequestPart or RequestParam just using different converters.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 3, 2021
@rstoyanchev
Copy link
Contributor

This is likely a duplicate of #26400. Could you check if it works 5.3.4-SNAPSHOT?

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Feb 3, 2021
@LickIt
Copy link
Author

LickIt commented Feb 3, 2021

Yes, I already tried running it with spring-test:5.3.4-SNAPSHOT but the result is still a "null" value in the controller.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 3, 2021
@LickIt
Copy link
Author

LickIt commented Feb 3, 2021

I extracted a simple snippet for testing: https://github.com/LickIt/spring-mockpart-issue
The test is passing with spring-test 5.3.2 but not with 5.3.3 and 5.3.4-SNAPSHOT

@Test
public void testUpload() throws Exception {
    MockMultipartFile file = new MockMultipartFile("file", "file.png", "application/octet-stream", new byte[0]);
    MockPart label = new MockPart("label", "mainImage".getBytes(UTF_8));

    mockMvc.perform(multipart("/test").file(file).part(label))
            .andExpect(content().string("mainImage"));
}

Here is the controller method:

@PostMapping(path = "/test", consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
public String uploadFile(@RequestParam("file") MultipartFile file,
                         @RequestPart(value = "label", required = false) String label) {
    return label;
}

@rstoyanchev
Copy link
Contributor

Thanks for the sample.

The latest code adds both a Part and a request parameter, this is not the issue. The root cause appears to be in the way StandardMultipartHttpServletRequest and MockMultipartHttpServletRequest implement getMultipartHeaders(String). The former always returns headers (possibly empty) for a part that is present. The latter returns null if the part doesn't have a Content-Type. Then RequestPartServletServerHttpRequest takes this to mean there is no such part at all. In effect, currently, a MockPart has to have a Content-Type to work with @RequestPart. The issue is temporarily masked in 5.3.2 due to the use of StandardMultipartHttpServletRequest but in previous versions, including 5.2.x the same issue is present.

From MultipartHttpServletRequest#getMultipartHeaders:

 * Return the headers associated with the specified part of the multipart request.
 * <p>If the underlying implementation supports access to headers, then all headers are returned.
 * Otherwise, the returned headers will include a 'Content-Type' header at the very least.

From RFC 7578 says:

   Each part MAY have an (optional) "Content-Type" header field, which
   defaults to "text/plain".  If the contents of a file are to be sent,
   the file data SHOULD be labeled with an appropriate media type, if
   known, or "application/octet-stream".

I think the Javadoc for MultipartHttpServletRequest#getMultipartHeaders is too strict since a Content-Type may legitimately not be present. We can align MockMultipartHttpServletRequest to work like StandardMultipartHttpServletRequest by returning the available headers, possibly empty. That has a better chance of decoding correctly based on the target Class.

What do you think @jhoeller ?

@rstoyanchev rstoyanchev self-assigned this Feb 3, 2021
@rstoyanchev rstoyanchev added in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 3, 2021
@rstoyanchev rstoyanchev added this to the 5.3.4 milestone Feb 3, 2021
@rstoyanchev
Copy link
Contributor

This should be fixed in the latest snapshots. If you could, please give it another try.

@LickIt
Copy link
Author

LickIt commented Feb 4, 2021

@rstoyanchev Thank you for the quick response and resolution!
I can confirm this is fixed in the latest snapshot.

@rstoyanchev
Copy link
Contributor

Thanks for checking.

This was referenced Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants