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

Improve tests and Javadoc on binding to a property of type javax.servlet.Part #27830

Closed
wants to merge 4 commits into from
Closed

Improve tests and Javadoc on binding to a property of type javax.servlet.Part #27830

wants to merge 4 commits into from

Conversation

binchoo
Copy link
Contributor

@binchoo binchoo commented Dec 17, 2021

When a controller obtains files from multipart requests,
there is an inconsistency when jakarta.servlet.http.Part type is an attribute of an object or an argument of a handler method.

If a method has an object argument, and expects files to be saved to attributes of that argument, the attribute type should not be Part, but it would rather be MultipartFile.

This is because StandardMultipartHttpServletRequest wraps its Parts, whose filename exists, with StandardMultipartFile.
StandardMultipartFile is an private class that implements MultipartFile class. So Part type attributes generate conversion errors.

In contrast, if the method has an argument type of Part, it can normally receive multipart files from the framework.

My idea is simply modifying StandardMultipartFile class to implement both MultipartFile and Part, so preventing conversion issues. Likewise I added new MockStandardMultipartFile class that replaces MockMultipartFile class.

Issues: #27819

@pivotal-cla
Copy link

@binchoo Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 17, 2021
@pivotal-cla
Copy link

@binchoo Thank you for signing the Contributor License Agreement!

@rstoyanchev
Copy link
Contributor

In the description, you refer to scenario 7 from #15220, but that issue and those scenarios specifically were addressed with PR #370. There is a test class WebRequestDataBinderIntegrationTests for those scenarios. I'm wondering what is the difference between that and what you're running into?

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) status: waiting-for-feedback We need additional information before we can continue labels Jan 6, 2022
@binchoo
Copy link
Contributor Author

binchoo commented Jan 13, 2022

@rstoyanchev, thanks for your review and comment.
The absence of integration test before issuing misled me about the range of this issue. I'm sorry for confusing you.

Scenario 7 doesn't seem to work, specifically, when testing the controller with MockMVC and MockMultipartHttpServletRequestBuilder,
while WebRequestDataBinderIntegrationTests demonstrates scenario 7 in integration context.

Please view a failing mockMVC test below.

@Test
public void multipartRequestWithServletPartsForPartAttribute() throws Exception {
    byte[] fileContent = "bar".getBytes(StandardCharsets.UTF_8);
    MockPart filePart = new MockPart("file", "orig", fileContent);

    byte[] json = "{\"name\":\"yeeeah\"}".getBytes(StandardCharsets.UTF_8);
    MockPart jsonPart = new MockPart("json", json);
    jsonPart.getHeaders().setContentType(MediaType.APPLICATION_JSON);

    standaloneSetup(new MultipartController()).build()
        .perform(multipart("/partattr").part(filePart).part(jsonPart))
        .andExpect(status().isFound()) // expect: 302, actual: 400
        .andExpect(model().attribute("fileContent", fileContent))
        .andExpect(model().attribute("jsonContent", Collections.singletonMap("name", "yeeeah")));
}

@Controller
private static class MultipartController {

    @RequestMapping(value = "/partattr")
    public String processPartAttribute(PartForm form,
                                       @RequestPart(required = false) Map<String, String> json, Model model) throws IOException {

        if (form != null) {
            Part part = form.getFile();
            if (0 != part.getSize()) {
                byte[] fileContent = StreamUtils.copyToByteArray(part.getInputStream());
                model.addAttribute("fileContent", fileContent);
            }
        }
        if (json != null) {
            model.addAttribute("jsonContent", json);
        }

        return "redirect:/index";
    }
}

private static class PartForm {

    private Part file;

    public PartForm(Part file) {
        this.file = file;
    }

    public Part getFile() {
        return file;
    }
}

Due to type conversion issue described in #27830 (comment),
Line 158: I want MockStandardMultipartFile to be registered to the mock request, so that this can be delivered to Part type attribute of an object normally.

protected final MockHttpServletRequest createServletRequest(ServletContext servletContext) {
MockMultipartHttpServletRequest request = new MockMultipartHttpServletRequest(servletContext);
Charset defaultCharset = (request.getCharacterEncoding() != null ?
Charset.forName(request.getCharacterEncoding()) : StandardCharsets.UTF_8);
this.files.forEach(request::addFile);
this.parts.values().stream().flatMap(Collection::stream).forEach(part -> {
request.addPart(part);
try {
String name = part.getName();
String filename = part.getSubmittedFileName();
InputStream is = part.getInputStream();
if (filename != null) {
request.addFile(new MockStandardMultipartFile(part, filename));
}
else {

@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 Jan 13, 2022
@binchoo binchoo changed the title Support multipart file binding to attributes whose type is jakarta.servlet.http.Part Replace MockMultipartFile with StandardMockMultipartFile to avoid conversion error Jan 13, 2022
@binchoo binchoo changed the title Replace MockMultipartFile with StandardMockMultipartFile to avoid conversion error Replace MockMultipartFile with MockStandardMultipartFile to avoid conversion error Jan 13, 2022
@rstoyanchev
Copy link
Contributor

Thanks for elaborating.

We do support Part for data binding, but only in fallback mode, when there is no MultipartResolver (this is in ServletRequestDataBinder). If you don't want to use MultipartFile you need to make sure that a MultipartResolver is not declared.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 25, 2022
@binchoo
Copy link
Contributor Author

binchoo commented Apr 26, 2022

@rstoyanchev
Thanks! Your comment gave me a lot of hints to deep-dive into multipart bindings.

As I reviewed,

  • DispatcherServlet::checkMultipart
  • ServletRequestDataBinder::bind

when a request is a MultipartRequest that has been resolved by MultipartResolver, then

  • Part in this request can be resolved for arguments type of Part and MultipartFile.
  • Part in this request can be bound to attributes that are MultipartFile only.

So I concluded that MockMultipartHttpServletRequestBuilder is doing ok because it is a mock for MultipartRequest and follows the rules above.

If you don't want to use MultipartFile you need to make sure that a MultipartResolver is not declared.

Yes I could test this by registering a multipart resolver that purposely returns false in isMultiPart().

@Bean
MyMultipartResolver multipartResolver() {
    return new MyMultipartResolver();
}

class MyMultipartResolver extends StandardServletMultipartResolver {
    @Override
    public boolean isMultipart(HttpServletRequest request) {
        return false;
    }
}

@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 Apr 26, 2022
@binchoo
Copy link
Contributor Author

binchoo commented Apr 27, 2022

7919996 Added tests covering these behaviors.

When a request is a MultipartRequest that has been resolved by MultipartResolver, then

  • Part in this request can be resolved for arguments type of Part and MultipartFile.
    • multipartRequestWithParts_resolvesMultipartFileArguments
    • multipartRequestWithParts_resolvesPartArguments
  • Part in this request can be bound to attributes that are MultipartFile only.
    • multipartRequestWithParts_resolvesMultipartFileProperties
    • multipartRequestWithParts_cannotResolvePartProperties

@rstoyanchev rstoyanchev self-assigned this Apr 28, 2022
@rstoyanchev rstoyanchev added this to the 5.3.20 milestone Apr 28, 2022
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Apr 28, 2022
@rstoyanchev rstoyanchev changed the title Replace MockMultipartFile with MockStandardMultipartFile to avoid conversion error Improve tests and Javadoc on binding to a property of type javax.servlet.Part Apr 28, 2022
@rstoyanchev
Copy link
Contributor

Thanks for the updates. I'll process this.

Note that if you're in Boot application you can use spring.servlet.multipart.enabled=false to turn off the MultipartResolver config and then declare a bean of type javax.servlet.MultipartConfigElement which enables multipart support at the Servlet container level.

rstoyanchev pushed a commit that referenced this pull request Apr 28, 2022
@binchoo binchoo deleted the develop/support-part-attribute branch April 28, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants