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

Inconsistencies in MockMvc's file() and part() handling and their impact on functional endpoints #34197

Open
Lee-WonJun opened this issue Jan 6, 2025 · 0 comments
Labels
in: test Issues in the test module status: waiting-for-triage An issue we've not yet triaged or decided on

Comments

@Lee-WonJun
Copy link

Lee-WonJun commented Jan 6, 2025

Spring REST Docs was the first to identify this issue: spring-projects/spring-restdocs#953

Current Situation

As evident in this issue, using the part() method instead of file() for multipart uploads in Spring REST Docs results in duplicate file entries within the generated documentation.

While I've submitted an issue and a proposed ad-hoc solution to Spring REST Docs, I believe the root cause lies in the somewhat ambiguous behavior of MockHttpServletRequest or MockMultipartHttpServletRequestBuilder.

Here's a breakdown of the sequence of events when uploading multipart data using MockMvc:

MockMultipartHttpServletRequest internally manages multipartFiles as separate file types. When using MockMultipartHttpServletRequestBuilder to create a mock multipart request, files can be added using either the part method to add a MockPart or the file method to add a MockMultipartFile.

this.mockMvc.perform(multipart("/upload") 
        .file(new MockMultipartFile("file", "file.pdf", "application/pdf", "sample content".getBytes())) 
        .part(new MockPart("file", "test.pdf", "contents".getBytes(), MediaType.APPLICATION_PDF)) 
); 

The actual servlet creation process is implemented as follows:

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 MockMultipartFile(name", filename, part.getContentType(), is)); 
            } else { 
                InputStreamReader reader = new InputStreamReader(is, getCharsetOrDefault(part, defaultCharset)); 
                String value = FileCopyUtils.copyToString(reader); 
                request.addParameter(part.getName(), value); 
            } 
        } catch (IOException ex) { 
             throw new IllegalStateException("Failed to read content for part " + part.getName(), ex); 
         } 
     }); 
 } 

As shown in the code, files added using the file method are only processed using addFile. However, files added using the part method are processed using both addPart and addFile, resulting in duplicate data being included in the servlet creation.

Multipart in Spring

I believe there are primarily two ways to handle multipart data on Spring servers:

  1. @RequestParam("file") MultipartFile file: This is the more traditional approach, commonly used in standard controller methods.
  2. request.multipartData().getFirst("file"): This method is primarily used in functional endpoints, providing a more flexible but manual way to handle multipart data. (As far as I know, traditional controllers also use this manual approach to handle file uploads.)

An example using functional endpoints is shown below:

@RestController
class FileUploadController {
    @PostMapping("/upload")
    public ReturnObject handleFileUpload(@RequestParam("file") MultipartFile file) {
        System.out.println("Received file: " + file.getOriginalFilename());
        return new ReturnObject("File uploaded successfully");
    }
}

@Component
class FileUploadFunctionalEndpoint {

    @Bean
    public RouterFunction<ServerResponse> fileUploadRoute() {
        return RouterFunctions.route(
                POST("/upload"),
                request -> {
                    var filePart = request.multipartData().getFirst("file");
                    var file = (Part) filePart;
                    System.out.println("Received file: " + file.getSubmittedFileName());
                    return ServerResponse.ok().body(new ReturnObject("File uploaded successfully"));
                }
        );
    }
}

Problems

When using MockMvc to simulate these requests, a couple of issues arise:

  1. Missing Part Data for .file(): When creating mock multipart data using the file() method, the corresponding part data is not populated. This means that methods like request.multipartData().getFirst("file") cannot retrieve the file using the part mechanism. This is a limitation inherent to MockMvc.

  2. Duplicate Entries in REST Docs: RestDocs, when generating documentation, attempts to handle this discrepancy by checking if the request is of type MockMultipartHttpServletRequest. If it is, REST Docs extracts file information from the files collection.
    This appears to be an ad-hoc workaround implemented in rest-docs to handle the scenario where no file info in parts collection.
    However, when using the part() method to create a MockMultipartHttpServletRequest, the same file is extracted twice, once from the parts collection and once from the files collection. This results in duplicate entries in the generated documentation.

Currently, the clearest solution I see is:

  1. Ensure that the part in the Mock behaves consistently with the file.
    1.2. To achieve consistency in the operation pipeline between @RequestParam("file") MultipartFile file and request.multipartData().getFirst("file"), we might need to modify not only the Mock but also the servlet structure.
  2. After to finish 1, remove the dependency on MockMultipartHttpServletRequest in rest-docs.

However, I don't have a clear solution to address this situation comprehensively, as I'm not familiar with all the dependencies within the Spring Framework.

Therefore, I've created this issue to discuss it further before making any modifications.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 6, 2025
@sbrannen sbrannen added in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jan 7, 2025
@sbrannen sbrannen changed the title Inconsistencies in MockMvc's .file() and .part() Usage and Their Impact on Functional Endpoints and RestDocs Inconsistencies in MockMvc's file() and part() handling and their impact on functional endpoints Jan 7, 2025
@snicoll snicoll removed the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 8, 2025
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 status: waiting-for-triage An issue we've not yet triaged or decided on
Projects
None yet
Development

No branches or pull requests

4 participants