This draft PR is a first Multipart support implementation here for high-level feedbacks and discussion.
It does not include RequestMapping level support, integration tests do not work yet with Reactor and RxNetty and NioMultipartResolver need to be rewritten using Flux.create() instead of ReplayProcessor.
Comment From: sdeleuze
I have updated this PR witgh the following changes:
- Tests now run fine with Reactor Netty and RxNetty
- getAllParts() now caches the resulting mono to allow multiple invocations
- There is a single Part object suitable usable for file and non file parts
- We now use Flux.create() instead of ReplayProcessor
- NioPart#transferTo() now uses zero copy when possible (when a temporary file has been previously created)
@rstoyanchev Could you please make a high-level review of this commit?
Comment From: rstoyanchev
@sdeleuze, apologies for suggesting it but ServerHttpRequest doesn't seem like the right level for exposing multipart support. Having MultipartResolver take ServerHttpRequest as input is already an indication that it's a higher level concern. Then MultipartResolver has to be injected into every request implementation, which is rather repetitive and artificial since it doesn't benefit from being in every request implementation. It's also a cyclical dependency even at the package level since the http package is logically at a level below the web package and should not depend on anything from it.
As it looks currently it's a better fit exposing it in ServerWebExchange. To avoid a circular dependency there once again, MultipartResolver and Part should be in the server package with maybe a server.multipart sub-package for implementations. For initialization MultipartResolver could be a field of HttpWebHandlerAdapter and then passed into ServerWebExchange instances. Very similar to WebSessionManager and WebSession.
For the caching, the cache operator should really be called at construction time or otherwise concurrent calls to getParts can each create a separate pipeline. See how DefaultWebExchange does it for WebSession and also note that DefaultWebSession uses the defer operator to ensure it doesn't do anything at the time of declaring the pipeline.
When the request is not multipart could we be more lenient and return an empty stream from getParts and a Mono with empty map from getAllParts perhaps? It flips the model from having to call isMultipartRequest first to being able to call getParts and getAllParts without error or side effect and optionally checking isMultipartRequest first if you have to.
Do we need Part.getContentType if we have getHeaders? Just thinking there are plenty of methods on part already?
I'm still not entirely sure how a JSON multipart would work out? Would it be a call to getValue() or getContent()? The underlying library seems to split the callbacks that way, but I'm not sure what it would do if there is no filename but there is a non-text/plain content-type.
I haven't looked into the implementation library but I presume it is non-blocking indeed? It seems to be returning java.io.InputStream.
Comment From: sdeleuze
No worry indeed, I agree that ServerHttpRequest is not the right level to implement that for the reasons you mentioned. I also agree with your remarks on caching and being more lenient if the request is not multipart.
We can remove Part.getContentType() since this is just a shortcut to Part.getHeaders().getContentType() (with maybe a Javadoc hint to help users for file parts).
Currently I think JSON parts will be natively treated by the underlying library like a part with a String based value since it has no filename header. Even if in NioPart I allowed to get the content both as a String or as a Publisher<DataBuffer>, that could be not optimal.
Not sure if that makes sens, but should we add some HttpMessageReader based capabilities with some <T> Flux<T> getContentAsStream(Class<T> type) and <T> Mono<T> getContentAs(Class<T> type) methods available on Part ? Or should we delegate that part to a composite MultipartHttpMessageReader that could be initialized with a List<HttpMessageReader> like we do on the writing side for SSE?
About the NIO Multipart library, it seems to me that the request parsing is done in a non blocking way but sadly we can only access to data as String or InputStream depending on the part type, so it may be blocking I suppose. I have implemented an optimisation to be able to perform a zero-copy transfer if the InputStream is a FileInputStream.
I can take care of the changes discussed here if you are ok.
I have created an issue on their GitHub to ask them more insight about the blocking behavior implied by InputStream and that fact that non-file part content is only accessible as a String.
Comment From: rstoyanchev
Okay, I'm not too sure what you mean with the HttpMessageReader based capabilities. I would think that Part should only expose basic ways of getting to the data, but once you've updated the PR maybe it will be more clear.
Comment From: sdeleuze
I have pushed an updated PR, and asked a new question to NIO Multipart project lead about how to perform non-blocking processing with their current InputStream / OutputStream based API.
Comment From: sdeleuze
@rstoyanchev Could you please review the last version of this PR?
Comment From: sdeleuze
@rstoyanchev I took in account your feedbacks and:
- Changed the package to org.springframework.http.codec.multipart
- Polished the Javadoc
- Renamed getAllParts() to getFormParts()
- Renamed getParts() to getFormPartsAsFlux()
- Integrated the MultipartHttpMessageWriter commit
Comment From: rstoyanchev
This is now in master.