[
https://issues.apache.org/jira/browse/WW-3924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13534786#comment-13534786
]
Rene Gielen commented on WW-3924:
---------------------------------
Uhm - sorry guys, but so far this looks not like what we discussed in the list.
AFACIS we talked about providing non breaking API extensions to support
multipart handler plugins a plugin. It was explicitely not about changing core
to use some new library, even not if it is marked optional.
Some more concrete stuff regarding the patch
- Please provide a pure patch. Roughly two third of this patch is reformatted
original code. Please keep your IDE from automatic reformatting. A patch /
commit is either functional or reformatting the code, but not both
- please tidy up your code. Things like IDEA standard file template comments or
commented out code should not go into a patch. Best example is
FastUploadMultiPartHandler, which includes a wrong JavaDoc-comment and code in
comments.
- Again FastUploadMultiPartHandler: Please use one file for one class, as long
you are not creating inner classes. This java-File contains two toplevel classes
- Please be sure to review your code for general quality. As I understood, you
are using IntelliJ IDEA. Reviewing warnings would easily have revealed that in
NgFileUploadInterceptor:182 there is a senseless null check due to NPE about to
happen before in line 176ff. Also you might want to review
Dispatcher#cleanUpRequest, just to name another one
- DispatcherTest#testConfigurationManager - what is this URL, split into two
lines, one now marking a Java label?
That was just a quick first review. A cleaner patch helps reviewing by far...
Now regarding the API changes
- NG as naming component is a bit unfortunate. What comes in two years, when we
have even better ideas? NNG? :)
- As I see it, a new API for MultiPart should either be extending the old or
providing a full new implementation if you flip the switch. Full means full
alternative, that is: it should provide an implementation of the former
functionality. Only then it is possible to deprecate the old API and declare
the new stuff as API to use. What I see here is having just two APIs, one for
commons-fu etc and one for fastupload.
An API change proposal should
- check wether the old API can be reused, starting with factoring out more
general interfaces and more abstract classes in the hierarchy. Did that happen?
- if extending the old doesn't work, provide a complete implemetation for the
former functionality. That is, implement a commons-fu Provider
For the integration of fastupload itself:
This library and related implementations will not make it into core any time
soon, as it would not for other "new" libs. You are providing an implementation
alternative. Fine. That's what plugis are for. So if a new MultiPart API makes
it into the distribution, feel free to provide a plugin, maybe as part of your
library distribution.
A small tip: even if we would want to include the lib into our distro, we
couldn't. You don't provide any license. To make it into any Apache related
distro, a library must be released under a APL compatible license. To get
adoption at all, your lib should generally be released under an accepted OSS
license.
> refactor file upload framework
> ------------------------------
>
> Key: WW-3924
> URL: https://issues.apache.org/jira/browse/WW-3924
> Project: Struts 2
> Issue Type: Improvement
> Components: "New" API, Integration
> Affects Versions: 2.3.7
> Environment: HTTP, RFC 1867, form-based upload.
> Reporter: Link Qian
> Labels: features, patch
> Fix For: 2.3.9
>
> Attachments: fileupload-patch.txt
>
> Original Estimate: 504h
> Remaining Estimate: 504h
>
> refactor current file upload framework in Struts2, the goals are:
> 1, compatible with current file upload API in Struts2.
> 2, enable file upload framework to integration with form-based upload
> components easily.
> 3, enable user to use stream/memory parsing model beyond temporary file
> parsing model.
> 4, testing
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
