Skip to content

GH-1643: Replace commons-fileload#1645

Merged
afs merged 3 commits intoapache:mainfrom
afs:file-uploader
Jan 3, 2023
Merged

GH-1643: Replace commons-fileload#1645
afs merged 3 commits intoapache:mainfrom
afs:file-uploader

Conversation

@afs
Copy link
Copy Markdown
Member

@afs afs commented Nov 30, 2022

GitHub issue resolved #1643

Pull request Description:
See #1643

Tested for Jetty 10.

It uses Servlet API 3.0 which Tomcat has supported since version 7.

This PR removes the commons-fileupload dependency which is no longer used.

https://issues.apache.org/jira/browse/FILEUPLOAD-309


  • Tests are included. Existing tests cover usage.
  • Commits have been squashed to remove intermediate development commit messages.
  • Key commit messages start with the issue number (GH-xxxx or JENA-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.


InputStream input = fileStream.openStream();
for ( Part part : request.getParts() ) {
InputStream input = part.getInputStream();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙆‍♂️ that's some nice simplification!


// Jetty requires a setting of this annotation object as a request attribute.
private static MultipartConfigElement multipartConfigElement = new MultipartConfigElement("");
private static String multipartAttributeName = org.eclipse.jetty.server.Request.__MULTIPART_CONFIG_ELEMENT;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if this __MULTIPART_CONFIG_ELEMENT attribute was created this way to discourage external use?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if this __MULTIPART_CONFIG_ELEMENT attribute was created this way to discourage external use?

I'll check some more. Using the annotation @MultipartConfig on FusekiFilter does not work (it's not a servlet).

The name is Jetty specific which is why it is protected by the getAttribute. Jetty source code likes leading _'s. See org.eclipse.jetty.server.Request.

I'm unsure if Tomcat needs anything special. I haven't managed to find a mention one way or the other.

PS: https://stackoverflow.com/questions/8047173/how-to-use-httpservletrequestgetparts-in-a-servlet-filter-running-on-tomcat

got me to:

https://tomcat.apache.org/tomcat-7.0-doc/config/context.html#Common_Attributes

Set allowCasualMultipartParsing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no rush to complete this.

@kinow - thanks for catching the issues - I hadn't noticed that the constant was Jetty-specific.

With various other things doing on, clarifying the situation for Tomcat may not get done for 4.7.0.

There may be a way to automatically programtically set allowCasualMultipartParsing in Tomcat, otherwise the user needs to set it in their context.xml.

Marked as "draft" to avoid accidentally merging!

@afs afs marked this pull request as draft December 1, 2022 09:20
@afs
Copy link
Copy Markdown
Member Author

afs commented Dec 11, 2022

Not for 4.7.0.

Until the Tomcat situation is cleared up, this can wait.
It would be nice not to require uses to modify the tomcat deployment configuration.

(On the complete migration to "jakarta", the war file wil only run in Tomcat10, whereas no it only runs in Tomcat <=9 - ther is a converter tool.)

@afs afs force-pushed the file-uploader branch 6 times, most recently from 9ca04ba to 1b8fc93 Compare December 27, 2022 13:46
@afs
Copy link
Copy Markdown
Member Author

afs commented Dec 28, 2022

Now supports Tomcat 8 and 9 as well as Jetty 10.

Tomcat 10 uses packages from jakarta.* so is incompatible with Fuseki at the moment.

@afs afs marked this pull request as ready for review December 28, 2022 14:40
@afs afs self-assigned this Dec 30, 2022
@afs afs merged commit decdbe7 into apache:main Jan 3, 2023
@afs afs deleted the file-uploader branch January 3, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace Apache Commons FileUpload with Servlet MultipartConfig

2 participants