Handling multipart requests #368

bazi · 2015-06-18T13:51:29Z

First of all let me note that this PR is not complete yet and needs community's feedback.

What this PR targets

Parameter injection into controller methods does not work yet with multipart/form-data requests. This issue was addressed in mail group here where @raphaelbauer proposed to make @Param("...") String param work with multipart requests and additionally introduce something like @File("...") File file that directly injects uploaded file into controller's method.
Besides, i found an old issue #88 that roughly says to hide commons-upload behind some Ninja API.
Well, this PR is all about those issues.

What's already done

Almost everything to handle multipart requests. There are new annotations @FileParam and @FileParams that can be applied to controller method's parameters. For instance, by adding @FileParam("myfile") File file into your controller method you have a ready java.io.File instance to read uploaded file contents from.
Additionally, every form parameter of a multipart request (i.e. those that are not attached files) can be injected by @Param annotation.

What's not done, issues

Cleanup of uploaded files

When multipart request are parsed, uploaded files are locally saved in temp directory. I need your feedback on how to clean up those files.

First thing that comes to my mind is to use finalize method of Context implementation(references to files are stored in Context).
Or there may be some aggregator of temporary files which deletes them after some time (thinking of a scheduler).

What are your suggestions? Can we leave cleanup responsibility to application developers or app administrators? I guess not :)

Commons-upload

Well there are no issues with the library ;) This is more related to issue #88.
Currently to handle file uploads we use Context.getFileItemIterator() which returns us an iterator of multipart items aka parts. This is the recommended way of handling parts, it is described in Streaming API. As its name implies parts are streamed and you have to handle them once. In this PR, multipart requests are parsed automatically (i.e. iterator is used) and all related parameters and files are available for injection. So Context.getFileItemIterator() becomes useless -- it won't anymore return an iterator that you can iterate through parts.
I thought to put @Deprecated on that method but deprecated methods shall have their old behavior for migration period but in our case that won't be the case. How hard would migration be if that method is removed?

buildhive · 2015-06-18T14:12:44Z

Ninja Web Framework » ninja #630 FAILURE
Looks like there's a problem with this pull request
(what's this?)

buildhive · 2015-06-18T16:55:30Z

Ninja Web Framework » ninja #631 SUCCESS
This pull request looks good
(what's this?)

raphaelbauer · 2015-06-19T10:18:12Z

ninja-servlet/src/main/java/ninja/servlet/ContextImpl.java

+                }
+            }
+        } catch (FileUploadException | IOException ex) {
+            logger.error("Failed to parse multipart request data", ex);


Hmm... should we throw the error? So we can fail fast - or just silently ignore that parsing went wrong and log... not 100% sure...

raphaelbauer · 2015-06-19T10:26:24Z

That looks very nice!

Obvious stuff missing:

Unit tests for the classes...
Documentation how stuff works
changelog.md ofc

Stuff that we still need to tackle:
I personally run Ninja on systems that do not allow to store uploaded files on the file system (no temp space). So I'd need an "in memory" mode to handle multipart requests. Can you think of a nice way how to disable the "automatically-store-multipart-files-on-temp-disk" thing?

Stuff you asked:
Regarding cleanup of temp files. finalize() sounds a bit too weak as we have no guarantee that finalize() is called at all. What about deleting the temp files once the result returns to ResultHandler? We could use a finally {} block to do that... We must then also add documentation - if someone wants to use the file after the controller method exists - she has to make sure that file got copied somewhere else...

Hope that helps :)

Your PR looks really nice. The uploading facilities were really not a nice part of Ninja. But you improved it very very nicely :) Good work!

…impl Context has two implementations now - the current one and one for multipart requests. Two binding are registered, impl for multipart contexts has additional binding annotation

buildhive · 2015-06-24T07:44:39Z

Ninja Web Framework » ninja #643 FAILURE
Looks like there's a problem with this pull request
(what's this?)

buildhive · 2015-06-24T07:44:44Z

Ninja Web Framework » ninja #644 FAILURE
Looks like there's a problem with this pull request
(what's this?)

buildhive · 2015-06-24T08:21:00Z

Ninja Web Framework » ninja #645 FAILURE
Looks like there's a problem with this pull request
(what's this?)

…art-params Conflicts: ninja-core/src/site/markdown/developer/changelog.md

bazi · 2015-06-24T08:30:00Z

Updates:

added full in-memory file upload handling (added commons-upload interface implementations), activated in application config
clean up of temporary files (handled in ResultHandler as @raphaelbauer suggested)
docs, tests, changelog, ...

P.S.
Context.getFileItemIterator() is still usable even if file item iterator is used to parse multipart request payload. Further calls are simulated by returning custom implementation of iterator.

buildhive · 2015-06-24T08:37:02Z

Ninja Web Framework » ninja #646 SUCCESS
This pull request looks good
(what's this?)

buildhive · 2015-06-24T08:48:41Z

Ninja Web Framework » ninja #647 SUCCESS
This pull request looks good
(what's this?)

raphaelbauer · 2015-06-30T08:05:09Z

This looks really good - unfortunately we are quite busy right now - so it may take some more days to review and merge :)

Conflicts: ninja-core/src/site/markdown/developer/changelog.md

buildhive · 2015-06-30T08:32:25Z

Ninja Web Framework » ninja #665 SUCCESS
This pull request looks good
(what's this?)

buildhive · 2015-06-30T08:43:43Z

Ninja Web Framework » ninja #666 SUCCESS
This pull request looks good
(what's this?)

raphaelbauer · 2015-07-13T07:54:58Z

Looking now into the PR. Quite a lot of code... So reviewing may take a while :)

raphaelbauer · 2015-07-13T08:10:33Z

ninja-servlet/src/main/java/ninja/servlet/MultipartContextImpl.java

+                    StringBuilder sb = new StringBuilder();
+                    try (InputStreamReader isr = new InputStreamReader(fileItemStream.openStream())) {
+                        int n;
+                        char[] buf = new char[128];


can be replaced by a simple Streams.asString(item.openStream());

raphaelbauer · 2015-07-13T08:13:11Z

before we continue a deeper discussion. Do you think it is possible to incorporate MultipartContextImpl into ContextImpl?

That you'd be the first thing I'd try to do - to remove a lot of code and also remove the bridge that checks whether to create a MultipartContextImpl or a ContextImpl.

Providing the InputStream feels handy to me... :)

buildhive · 2015-07-20T15:10:41Z

Ninja Web Framework » ninja #683 SUCCESS
This pull request looks good
(what's this?)

Conflicts: ninja-core/src/site/markdown/developer/changelog.md

buildhive · 2015-07-22T06:46:23Z

Ninja Web Framework » ninja #697 SUCCESS
This pull request looks good
(what's this?)

raphaelbauer · 2015-07-23T07:55:04Z

I don't care. We can merge this PR first and then @momiji can submit the improved version.
@momiji Ninja is released 12 times a year roughly.

@bazi I got some more remarks (tiny stuff I did not say until the big things were ironed out...) I'll add that now... Once that's incorporated we can merge it into master... :)

raphaelbauer · 2015-07-23T07:57:00Z

ninja-core/src/main/java/ninja/Context.java

@@ -41,6 +41,8 @@
    interface Impl extends Context {

        void setRoute(Route route);
+
+        void cleanup();


maybe we can rename that to cleanupAfterRequest() + add one line of javadoc...

raphaelbauer · 2015-07-23T08:08:27Z

Okay finished adding my smaller remarks. A lot of formatting and language things :) Don't take it personally - it's really just to make things better and keep things uniform across the codebase...

raphaelbauer · 2015-07-23T08:08:56Z

@bazi Let us know when you are finished - then we can merge the PR :)

momiji · 2015-07-23T09:51:29Z

I doubt you can merge my PR after, as it use a very different approach than this one. It will require to remove too many things from this PR, and should be considered as a drop-in replacement.

Making my own PR was more a challenge to see if the "file provider" was something that could be done, and I found that it was quite easy to implement. It also make me learn things about Github branching, Mockito tests, Guice injection, and so on.

The important thing is to have a way to handle files, whatever way is used to do it, and to do it in a way that let's the final developers do their job, with enough flexibility.

I would have only one request before merging this PR, please be sure you do not break getFileItemIterator() current behavior. It may already be used in some applications, and shod still work as before, meaning providing original streams to the developer.

I'm just saying that because we are already using it in several 3 different ways:

save file to disk
save file to disk with on-the-fly encryption
save file chunks to disk (for very big files) with on-the-fly encryption

I doubt to be able to do all that with the new way to handle uploads, as we have both in-memory and disk, but this is not a problem as long as we can still use the getFileItemIterator(), as actually done.

If not, then this should be mentioned that this is a breaking change, and we will switch to use the internal httpServletRequest and ServletFileUpload, as it was originally coded.

raphaelbauer · 2015-07-23T10:06:30Z

Ok - how should we proceed? I don't care how it is implemented as the way it is currently is very very low level (but works of course). Any improvement would be great.

The only constraint I have: I want to be able to control whether the file is stored in memory or on disk.

Anything else can be discussed :)

I guess it is about you two: @bazi @momiji - you did the great job of actually implementing great solutions. Can you agree on one? Or agree on how we want to proceed from those two PRs? Thanks :)

bazi · 2015-07-23T10:37:09Z

@momiji getFileItemIterator() is marked deprecated but it surely keeps its behavior.

Anyways, i made a decision. Let's abandon this PR and focus on the other one from @momiji, guess it is #383 but it seems closed. Please provide PR, I will be on the topic.

@raphaelbauer beg your pardon for your time spent on reviewing ;-)

raphaelbauer · 2015-07-24T07:50:16Z

Absolutely no problem! It's a pleasure to read code and improve things together! :)
Let me know when I can help on the "other" PR... As I said above - it would be great if you two could team up to implement the feature!

Cool effort!

I'll close this PR then...

bazi added 2 commits June 18, 2015 17:23

parse multipart request params and files

94c09ef

fix integration tests

d378344

remove java 8 elements to avoid buildhive failure

da7028d

raphaelbauer reviewed Jun 19, 2015
View reviewed changes

bazi added 6 commits June 19, 2015 17:35

use long names for local params, reformat to minimize diff

89a5005

refactor: seperate multipart context impl from non-multipart context …

cb3e7e6

…impl Context has two implementations now - the current one and one for multipart requests. Two binding are registered, impl for multipart contexts has additional binding annotation

add in-memory handler for file uploads

945605b

simulate file item iterator of commons-upload

ea18a74

add test cases

31bc8d3

add docs and update changelog

730e846

fix null check and tests

8778515

Merge branch 'develop' of github.com:ninjaframework/ninja into multip…

4fd43cb

…art-params Conflicts: ninja-core/src/site/markdown/developer/changelog.md

Merge remote-tracking branch 'upstream/develop' into multipart-params

585f59c

Conflicts: ninja-core/src/site/markdown/developer/changelog.md

Merge remote-tracking branch 'upstream/develop' into multipart-params

ecd953b

raphaelbauer reviewed Jul 13, 2015
View reviewed changes

bazi added 2 commits July 22, 2015 10:47

Merge remote-tracking branch 'upstream/develop' into multipart-params

a2401b5

Conflicts: ninja-core/src/site/markdown/developer/changelog.md

update docs

0fc2798

raphaelbauer reviewed Jul 23, 2015
View reviewed changes

raphaelbauer closed this Jul 24, 2015

This was referenced Jul 27, 2015

Cannot get form field parameters from multipart/form-data #388

Open

handle of uploaded files the same way as normal query and form fields #383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling multipart requests #368

Handling multipart requests #368

bazi commented Jun 18, 2015

buildhive commented Jun 18, 2015

buildhive commented Jun 18, 2015

raphaelbauer Jun 19, 2015

raphaelbauer commented Jun 19, 2015

buildhive commented Jun 24, 2015

buildhive commented Jun 24, 2015

buildhive commented Jun 24, 2015

bazi commented Jun 24, 2015

buildhive commented Jun 24, 2015

buildhive commented Jun 24, 2015

raphaelbauer commented Jun 30, 2015

buildhive commented Jun 30, 2015

buildhive commented Jun 30, 2015

raphaelbauer commented Jul 13, 2015

raphaelbauer Jul 13, 2015

raphaelbauer commented Jul 13, 2015

buildhive commented Jul 20, 2015

buildhive commented Jul 22, 2015

raphaelbauer commented Jul 23, 2015

raphaelbauer Jul 23, 2015

raphaelbauer commented Jul 23, 2015

raphaelbauer commented Jul 23, 2015

momiji commented Jul 23, 2015

raphaelbauer commented Jul 23, 2015

bazi commented Jul 23, 2015

raphaelbauer commented Jul 24, 2015

Handling multipart requests #368

Handling multipart requests #368

Conversation

bazi commented Jun 18, 2015

What this PR targets

What's already done

What's not done, issues

Cleanup of uploaded files

Commons-upload

buildhive commented Jun 18, 2015

buildhive commented Jun 18, 2015

raphaelbauer Jun 19, 2015

Choose a reason for hiding this comment

raphaelbauer commented Jun 19, 2015

buildhive commented Jun 24, 2015

buildhive commented Jun 24, 2015

buildhive commented Jun 24, 2015

bazi commented Jun 24, 2015

buildhive commented Jun 24, 2015

buildhive commented Jun 24, 2015

raphaelbauer commented Jun 30, 2015

buildhive commented Jun 30, 2015

buildhive commented Jun 30, 2015

raphaelbauer commented Jul 13, 2015

raphaelbauer Jul 13, 2015

Choose a reason for hiding this comment

raphaelbauer commented Jul 13, 2015

buildhive commented Jul 20, 2015

buildhive commented Jul 22, 2015

raphaelbauer commented Jul 23, 2015

raphaelbauer Jul 23, 2015

Choose a reason for hiding this comment

raphaelbauer commented Jul 23, 2015

raphaelbauer commented Jul 23, 2015

momiji commented Jul 23, 2015

raphaelbauer commented Jul 23, 2015

bazi commented Jul 23, 2015

raphaelbauer commented Jul 24, 2015