[MAGNOLIA-540] Replacing default multipart filter implementation (problematic com.oreilly.servlet package license terms) Created: 06/Sep/05 Updated: 26/Jun/19 Resolved: 25/Nov/13 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | core |
| Affects Version/s: | 2.1 Final |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major |
| Reporter: | Andreas Brenk | Assignee: | Fabrizio Giustina |
| Resolution: | Outdated | Votes: | 0 |
| Labels: | None | ||
| Σ Remaining Estimate: | Not Specified | Remaining Estimate: | Not Specified |
| Σ Time Spent: | Not Specified | Time Spent: | Not Specified |
| Σ Original Estimate: | Not Specified | Original Estimate: | Not Specified |
| Attachments: |
|
||||||||||||||||
| Issue Links: |
|
||||||||||||||||
| Sub-Tasks: |
|
||||||||||||||||
| Template: |
|
||||||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||||||
| Task DoR: |
Empty
|
||||||||||||||||
| Date of First Response: | |||||||||||||||||
| Description |
|
The WAR file download contains Jason Hunter's com.oreilly.servlet package in the WEB-INF/lib directory. Its license terms (see http://servlets.com/cos/license.html) are problematic I fear. Especially items 5 and 6 are currently violated: "5. The distribution includes copyright notice as follows: "The source code, object code, and documentation in the com.oreilly.servlet package is copyright and owned by Jason Hunter." in the documentation and/or other materials provided with the distribution. 6. You reproduce the above copyright notice, this list of conditions, and the following disclaimer in the documentation and/or other materials provided with the distribution." Both "license.txt" and "release notes.txt" contained in the WAR file download do not mention anything demanded by the terms cited above. A problematic second point is the license for commercial usage and redistribution as every developer must own a copy of Jason's book or negotiate a seperate license. Jakarta Commons FileUpload (http://jakarta.apache.org/commons/fileupload/) is a possible replacement at least for some functionality. |
| Comments |
| Comment by Boris Kraft [ 08/Sep/05 ] |
|
Andreas, thanks for pointing this out - for us it was not problematic because we own a copy of the book Do you have time to check if the alternatives would suffice, and provide a patch? |
| Comment by Andreas Brenk [ 09/Sep/05 ] |
|
Okay, I replaced the MultipartRequestFilter with a new implementation using commons-fileupload. You have to remove the dependency on cos and a one on commons-fileupload to project.xml: <dependency> Warning: I haven't tested the new code, yet. I'm currently using the WAR distribution and have to complete my Magnolia build environment first... |
| Comment by Fabrizio Giustina [ 12/Sep/05 ] |
|
Just a comment: I already tried replacing cos with commons-fileupload months ago, during the dependencies cleanup work, but my implementation failed in 2 points:
said that, I'm all for migrating to commons-fileupload and I will try to help with the implementation: we could start adding the dependency and committing a new multipart filter along with the existing one for tests (leaving the old one configured by default while testing the new one)... I'll do that |
| Comment by Andreas Brenk [ 12/Sep/05 ] |
|
A first go at a JUnit test case. |
| Comment by Andreas Brenk [ 12/Sep/05 ] |
|
I forgot: the test case uses commons-httpclient 3.0-rc3. So you have to update the dependency. Multi-value properties are not yet tested, feel free to enhance the test case. The problem you had with multi-value properties probably was that commons-fileupload's FileItem only has access methods for single-value String parameters. You have to get the multi-value ones from the ServletRequest yourself. Regarding UTF-8: The test case uses the UTF-8 encoded log4j.xml from src/test-resources for its multipart request without problems. But maybe this isn't comprehensive enough. |
| Comment by Fabrizio Giustina [ 24/Sep/05 ] |
|
The new filter is now in svn, but it still can't replace the COS based one because it actually doesn't support multi-value parameters. |
| Comment by Philipp Bracher [ 17/May/06 ] |
|
We should fix this in the 3.0 relese |
| Comment by Philipp Bracher [ 13/Jun/06 ] |
|
postponed again |
| Comment by Fabrizio Giustina [ 26/Sep/07 ] |
|
Setting release version and moving to an higher priority, cos license terms could be problematic and this issue is here from too much time... After my recent fixes the new commons-fileupload implementation looks good, and someway also better than the cos based one (there is an additional request wrapper which makes easier to deal with multipart forms). I'd like to see this becoming the default in 3.1-m4, by renaming the old cos filter to CosMultipartRequestFilter and CommonsFileUploadMultipartRequestFilter to MultipartRequestFilter. If no issues are found in this milestone we can easily drop the cos dependency (along with the filter implementation) for the first RC. |
| Comment by Sameer Charles [ 26/Sep/07 ] |
|
that's great, but just wanted to let you guys know that magnolia has a permission to redistribute cos library without any restrictions. |
| Comment by Fabrizio Giustina [ 30/Sep/07 ] |
|
> that's great, but just wanted to let you guys know that magnolia has a permission to redistribute cos library without any restrictions. well, that's great (unfortunately looks like nobody was aware of that), but anyway switching to commons-fileupload is probably still an improvement. Anyway we now have the alternative implementation with commons-fileupload fully working, and commons-fileupload is apache2, it couldn't be better. |
| Comment by Magnolia International [ 02/Nov/07 ] |
|
I'm sorry but I'm going to have to reopen this. At least temporarily, I'll switch back the default to the CosMultipartRequestFilter (not renaming classes). The problem is that with the commons-upload-based implementation, request parameters are lost. I haven't examined the problem deeply, and it might be that it's only because in my case, they were part of the query string (ie params in the URLs were lost, but the file was uploaded properly) I'm not sure that's spec compliant (having both url and post params) but in the case of a file upload through the fck editor, it breaks everything and we can't afford that just now. Another problem (which I'll try to fix now) is that the (commons-upload) MultipartRequestFilter is swallowing all exceptions that happen down the filter chain, which is just plain wrong (I'll fix that). Maybe we could introduce an AbstractMRF, because in the end all those filters (should) do is wrap the request and pass it on. This would ensure consistent behavior betweem the two. |
| Comment by Fabrizio Giustina [ 02/Nov/07 ] |
|
> The problem is that with the commons-upload-based implementation, request parameters are lost. mh, not sure if mixing query string with a multipart form parameters could be a standard behavior, but if request.getParameter() actually returns query string parameters also in case of multiparts this can be easily fixed in the commons-fileupload filter (just adding to the parameters from the query string to the ones in the multipart form). |
| Comment by Fabrizio Giustina [ 16/Nov/07 ] |
|
Grégory, can you explain exactly how to reproduce the problem you saw? Can we put together a testcase for it? |
| Comment by Magnolia International [ 16/Nov/07 ] |
|
Hmm, right now I don't remember exactly which one, but I had the issue with some dialog. I don't think it even had any upload, but it's <form> action parameter add URL parameters, and its method was POST. The URLs parameters were lost along the way. I'll try to reproduce... after the weekend |
| Comment by Jan Haderka [ 04/Sep/08 ] |
|
Since we have license to redistribute, it is not system critical and we have some time to solve all the issues with commons-fileupload before making the switch. |