You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@struts.apache.org by "Rene Gielen (JIRA)" <ji...@apache.org> on 2012/12/18 10:58:14 UTC
[jira] [Comment Edited] (WW-3924) refactor file upload framework
[ https://issues.apache.org/jira/browse/WW-3924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13534786#comment-13534786 ]
Rene Gielen edited comment on WW-3924 at 12/18/12 9:56 AM:
-----------------------------------------------------------
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. 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.
was (Author: rgielen):
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