Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.3.7
    • Fix Version/s: 2.5.12
    • Component/s: "New" API, Integration
    • Labels:
    • Environment:

      HTTP, RFC 1867, form-based upload.

    • Flags:
      Patch

      Description

      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

        Activity

        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Could you attach a patch ?

        Show
        lukaszlenart Lukasz Lenart added a comment - Could you attach a patch ?
        Hide
        lqian Link Qian added a comment -

        patch will be ready in future.

        Show
        lqian Link Qian added a comment - patch will be ready in future.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Perfect!

        Show
        lukaszlenart Lukasz Lenart added a comment - Perfect!
        Hide
        lqian Link Qian added a comment -

        1, add fastupload dependency in struts core pom.xml
        2, add struts.multipart.ngSupport configurable item
        3, modify Dispatcher.wrapRequest(... ) mothod
        4, modify Dispatcher.cleanUpRequest() method
        5, add new classes, MultiPart.java, MultiPartHandler.java NgMultiPartRequestWrapper.java, FastUploadMultiPartHandler.java in package org.apache.struts2.dispatcher.multipart.ng
        6, add a MultiPartHandler implementation config in struts-default.xml
        7. add testing.

        Show
        lqian Link Qian added a comment - 1, add fastupload dependency in struts core pom.xml 2, add struts.multipart.ngSupport configurable item 3, modify Dispatcher.wrapRequest(... ) mothod 4, modify Dispatcher.cleanUpRequest() method 5, add new classes, MultiPart.java, MultiPartHandler.java NgMultiPartRequestWrapper.java, FastUploadMultiPartHandler.java in package org.apache.struts2.dispatcher.multipart.ng 6, add a MultiPartHandler implementation config in struts-default.xml 7. add testing.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        As I see, Fastupload, library is already in Maven Central Repo so I think we can implement the change.

        Show
        lukaszlenart Lukasz Lenart added a comment - As I see, Fastupload, library is already in Maven Central Repo so I think we can implement the change.
        Hide
        lqian Link Qian added a comment -

        should I implement the change into code SVN repository?

        Show
        lqian Link Qian added a comment - should I implement the change into code SVN repository?
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        You're not allowed to do it It will be introduced sooner or later

        Show
        lukaszlenart Lukasz Lenart added a comment - You're not allowed to do it It will be introduced sooner or later
        Hide
        lqian Link Qian added a comment -

        when shall struts 2.3.8 release?

        Show
        lqian Link Qian added a comment - when shall struts 2.3.8 release?
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        No specific date right now, but probably soon

        Show
        lukaszlenart Lukasz Lenart added a comment - No specific date right now, but probably soon
        Hide
        lqian Link Qian added a comment -

        does the patch merge into struts 2.3.8?

        Show
        lqian Link Qian added a comment - does the patch merge into struts 2.3.8?
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Nope

        Show
        lukaszlenart Lukasz Lenart added a comment - Nope
        Hide
        lqian Link Qian added a comment -

        why nope

        Show
        lqian Link Qian added a comment - why nope
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Because the change was huge and I didn't want to introduce such big change as the main goal of 2.3.8 was to improve performance - which was broken.
        It's simple matter of what is more important.

        Show
        lukaszlenart Lukasz Lenart added a comment - Because the change was huge and I didn't want to introduce such big change as the main goal of 2.3.8 was to improve performance - which was broken. It's simple matter of what is more important.
        Hide
        lqian Link Qian added a comment -

        all code is tested, you're safe to introduce current release

        Show
        lqian Link Qian added a comment - all code is tested, you're safe to introduce current release
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        But it must be reviewed by a committer and that takes time.

        Show
        lukaszlenart Lukasz Lenart added a comment - But it must be reviewed by a committer and that takes time.
        Hide
        lqian Link Qian added a comment -

        sure, when will you review it.

        Show
        lqian Link Qian added a comment - sure, when will you review it.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        When I'll have a time

        Show
        lukaszlenart Lukasz Lenart added a comment - When I'll have a time
        Hide
        lqian Link Qian added a comment -

        hence the patch is based struts 2.3.7. if you find any issue when patch it into struts 2.3.8 codebase. please let me know.

        Show
        lqian Link Qian added a comment - hence the patch is based struts 2.3.7. if you find any issue when patch it into struts 2.3.8 codebase. please let me know.
        Hide
        rgielen Rene Gielen added a comment - - edited

        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.

        Show
        rgielen Rene Gielen added a comment - - edited 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.
        Hide
        lqian Link Qian added a comment -

        Not exactly. Struts1, Struts2 file upload framework limits Action to use java.io.File to reference a uploading multipart content from http request because FileUploadIntercetor and MultiPartResolver only expose File type to others class needs. It's hard to extend and low performance fact which discuss more thread more time. Thus, it's necessary refactor the old file upload processing framework. meanwhile, we need to protect user's program work in new struts release including the refactor code as well. So the Ng (new generation) file upload process framework is not constraint, instead of an alternative for experimental framework aim to easy extension and high performance usage. It is easy to switch and is off in default. As Lukasz said, it's a big change with a bit change core code.

        The patch provides a solution that easy to write plugin, whatever commons-fu, fastupload and others. what use commons-fu API implements NgMultiPartResolver and wrap MultiPart is easy.

        also I would like to say sorry for the codestyle issue. include license, comments etc.

        Show
        lqian Link Qian added a comment - Not exactly. Struts1, Struts2 file upload framework limits Action to use java.io.File to reference a uploading multipart content from http request because FileUploadIntercetor and MultiPartResolver only expose File type to others class needs. It's hard to extend and low performance fact which discuss more thread more time. Thus, it's necessary refactor the old file upload processing framework. meanwhile, we need to protect user's program work in new struts release including the refactor code as well. So the Ng (new generation) file upload process framework is not constraint, instead of an alternative for experimental framework aim to easy extension and high performance usage. It is easy to switch and is off in default. As Lukasz said, it's a big change with a bit change core code. The patch provides a solution that easy to write plugin, whatever commons-fu, fastupload and others. what use commons-fu API implements NgMultiPartResolver and wrap MultiPart is easy. also I would like to say sorry for the codestyle issue. include license, comments etc.
        Hide
        lqian Link Qian added a comment -

        Lukasz, sorry to update the ticket until today as I has been busy as bee recenty. I re-create a patch for the ticket and attach it.

        Here are brief:
        1, instead of Apache commons fileupload stream API to implements Multipart and MultipartHandler.
        2, add 3 test for NgFileUploadInterceptorTest
        3, make sure all file has ASF license 2.0
        4, add struts.multipart.ngSupport config entry in default.properties
        5, inject struts.multipart.ngSupport config entry to Dispatcher.java and modify the wrapRequest(...) and cleanUp() method.
        6, add bean config of MultipartHandler in struts-default.xml

        Show
        lqian Link Qian added a comment - Lukasz, sorry to update the ticket until today as I has been busy as bee recenty. I re-create a patch for the ticket and attach it. Here are brief: 1, instead of Apache commons fileupload stream API to implements Multipart and MultipartHandler. 2, add 3 test for NgFileUploadInterceptorTest 3, make sure all file has ASF license 2.0 4, add struts.multipart.ngSupport config entry in default.properties 5, inject struts.multipart.ngSupport config entry to Dispatcher.java and modify the wrapRequest(...) and cleanUp() method. 6, add bean config of MultipartHandler in struts-default.xml
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        I was trying to apply this patch but a lot of things has changed and it isn't possible to apply this patch. Also it introduces a new hierarchy of classes which isn't the best idea - this will take ages for users to migrate.

        Another thing that there is already support for Jakarta Stream added with WW-3025 and currently we are using a UploadedFile abstraction instead of the java.io.File - it isn't so powerful as your MultiPart but is good enough

        Sorry that it took so long to give you the feedback

        Show
        lukaszlenart Lukasz Lenart added a comment - I was trying to apply this patch but a lot of things has changed and it isn't possible to apply this patch. Also it introduces a new hierarchy of classes which isn't the best idea - this will take ages for users to migrate. Another thing that there is already support for Jakarta Stream added with WW-3025 and currently we are using a UploadedFile abstraction instead of the java.io.File - it isn't so powerful as your MultiPart but is good enough Sorry that it took so long to give you the feedback

          People

          • Assignee:
            lukaszlenart Lukasz Lenart
            Reporter:
            lqian Link Qian
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 504h
              504h
              Remaining:
              Remaining Estimate - 504h
              504h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development