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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java forms bind `multipart/form-data` file uploads #8898

Merged
merged 8 commits into from Jan 4, 2019

Conversation

Projects
None yet
5 participants
@mkurz
Copy link
Member

mkurz commented Dec 21, 2018

Just have a look at the notes I added to the 2.7 release highlights.
Now a Java form also mimics an html multipart-form 馃槈

We could now add validators for uploaded files (like @MinFileSize, @AllowedContenttypes, etc.). Nice, I like that. No more validation of file uploads in the controller...

BTW: I uncovered a spring-beans bug which also effects previous Play versions. However it doesn't hold back this pull request (this pull request didn't cause the bug, it exists since a long time). (I will fix the bug in spring).

This pull request is/should be backwards compatible and no problem to include in the next Play 2.7 RC.

mkurz added some commits Dec 21, 2018

@mkurz mkurz added this to the Play 2.7.0 milestone Dec 21, 2018

@mkurz mkurz added the type:feature label Dec 21, 2018

@mkurz mkurz requested a review from marcospereira Dec 21, 2018

// TODO: This tests below are buggy. As you can see the closing bracket ] is missing everywhere. This is a bug in spring-beans.
// When parsing e.g. "data[attachments[0]]" spring just looks for the first occurrence of ] when and uses it for the the end position in substring
// See here, `keyEnd` is wrong:
// https://github.com/spring-projects/spring-framework/blob/v5.1.3.RELEASE/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java#L941

This comment has been minimized.

@mkurz

mkurz Dec 21, 2018

Author Member

Here is the spring-beans bug I was talking about. Have a look at the lines below (missing ]).

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Dec 21, 2018

Oh yes and fixes #8323

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Dec 21, 2018

Build will turn green when finished (only Java 11 tests currently left) 馃憤
@marcospereira Looking forward to your feedback so we get this shipped with the next RC, thanks!

@wsargent

This comment has been minimized.

Copy link
Member

wsargent commented Dec 22, 2018

Also see #8323

Doc fix suggested by wsargent
Co-Authored-By: mkurz <m.kurz@irregular.at>
@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Dec 22, 2018

Closed/re-opened because there were problems getting the build status from Travis...

@marcospereira
Copy link
Member

marcospereira left a comment

LGTM.

A suggestion is to add some documentation in JavaForms with examples of how to use this.

mkurz added some commits Jan 4, 2019

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jan 4, 2019

Thanks for reviewing @marcospereira. Added the docs, please have a look again, thank you very much!

@marcospereira
Copy link
Member

marcospereira left a comment

LGTM.

@mkurz mkurz merged commit c9f3d74 into playframework:master Jan 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@mkurz mkurz deleted the mkurz:java-form-bind-files branch Jan 4, 2019

@eximius313

This comment has been minimized.

Copy link

eximius313 commented Jan 7, 2019

@mkurz could you link to the bug in Spring here (so we can watch it)?

@mkurz

This comment has been minimized.

Copy link
Member Author

mkurz commented Jan 7, 2019

@eximius313 I don't think there is an open bug report in the Spring project yet. I will open one and provide a fix for it to Spring as soon as I have more time (maybe this week...)

@renatocaval

This comment has been minimized.

Copy link
Contributor

renatocaval commented Jan 7, 2019

Backported to 2.7.x (e81c665)

renatocaval added a commit that referenced this pull request Jan 7, 2019

Java forms bind `multipart/form-data` file uploads (#8898)
* Also bind files in Form and DynamicForm

* Add tests

* Add feature to highlights

* Fix CompileTimeDependencyInjectionFormSpec

* Doc fix suggested by wsargent

Co-Authored-By: mkurz <m.kurz@irregular.at>

* Correct year in header

* Add example to JavaForms docs
@eximius313

This comment has been minimized.

Copy link

eximius313 commented Jan 9, 2019

@mkurz If you just open this bug report, we will see what people from Spring say about it. Maybe it's already known, maybe it's "feature not a bug" - who knows ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment