-
Notifications
You must be signed in to change notification settings - Fork 9
Fix issue with submitting multipart data #33
Conversation
Does not fail tests if 8080 is already taken
Wraps a ServletInputStream in a BufferedInputStream to allow reuse
Allows the Servlet stream to be reset and reused
Need to load the plugin before core so the stream is not already read
// the getInputStream method. Also tried reading one byte, works on | ||
// first request, fails on all subsequent ones. Got no idea why at | ||
// the moment | ||
IOUtils.toString(usedStream, 'UTF-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is concerning to me because this ultimately will require all payloads to be read into memory for the duration of the request. This doesn't allow authors to efficiently pipe streams together, and can potentially cause large memory requirements, depending on the expected payload sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Bud.
Thanks for your review. This fix immediately unblocked me for using multipart requests in jersey, though I understand the problem. Will have a think on how to improve this
How did you get it to work for issue 21?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Bud.
I think I found a better solution which involves overwriting the MultiPartReaderServerSide to support the Grails requests. My question is, you think it is ok if jersey-multipart will be added as a dependency into the grails-jaxrs-jersey1 plugin? I see no reason not to.
Will push the new fix soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look at these PR's. Did this ever get the multipart library added?
Fixed by allowing multiple reads on the HttpRequest input stream.
Relates to issue #32