You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2022/11/30 21:13:04 UTC
[GitHub] [jena] afs opened a new pull request, #1645: GH-1643: Replace commons-fileload
afs opened a new pull request, #1645:
URL: https://github.com/apache/jena/pull/1645
GitHub issue resolved #1643
Pull request Description:
See #1643
Test 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.
----
- [x] Tests are included. Existing tests cover usage.
- [x] Commits have been squashed to remove intermediate development commit messages.
- [x] 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](https://www.apache.org/licenses/contributor-agreements.html).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a diff in pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
afs commented on code in PR #1645:
URL: https://github.com/apache/jena/pull/1645#discussion_r1036492716
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/system/DataUploader.java:
##########
@@ -102,6 +102,10 @@ public static UploadDetails incomingData(HttpAction action, StreamRDF dest) {
}
}
+ // 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;
Review Comment:
> 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a diff in pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
afs commented on code in PR #1645:
URL: https://github.com/apache/jena/pull/1645#discussion_r1036868799
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/system/DataUploader.java:
##########
@@ -102,6 +102,10 @@ public static UploadDetails incomingData(HttpAction action, StreamRDF dest) {
}
}
+ // 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;
Review Comment:
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] kinow commented on a diff in pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #1645:
URL: https://github.com/apache/jena/pull/1645#discussion_r1036466549
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/system/DataUploader.java:
##########
@@ -102,6 +102,10 @@ public static UploadDetails incomingData(HttpAction action, StreamRDF dest) {
}
}
+ // 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;
Review Comment:
Hm, can't find any note saying that, so probably just a different way to name constants I guess. https://github.com/eclipse/jetty.project/blob/410e9db837c964bfcb27ff01693eb77a45dcd60f/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1645:
URL: https://github.com/apache/jena/pull/1645#issuecomment-1345528476
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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] kinow commented on a diff in pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #1645:
URL: https://github.com/apache/jena/pull/1645#discussion_r1036464108
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/system/DataUploader.java:
##########
@@ -113,29 +117,16 @@ public static UploadDetails incomingData(HttpAction action, StreamRDF dest) {
* This function assumes it is inside a transaction.
*/
private static UploadDetails fileUploadMultipart(HttpAction action, StreamRDF dest) {
- String base = ActionLib.wholeRequestURL(action.getRequest());
- ServletFileUpload upload = new ServletFileUpload();
+ HttpServletRequest request = action.getRequest();
+ String base = ActionLib.wholeRequestURL(request);
StreamRDFCounting countingDest = StreamRDFLib.count(dest);
-
+ if ( request.getAttribute(multipartAttributeName) == null )
+ request.setAttribute(multipartAttributeName, multipartConfigElement);
try {
- FileItemIterator iter = upload.getItemIterator(action.getRequest());
- while (iter.hasNext()) {
- FileItemStream fileStream = iter.next();
- if (fileStream.isFormField()) {
- // Form field - this code only supports multipart file upload.
- String fieldName = fileStream.getFieldName();
- InputStream stream = fileStream.openStream();
- String value = Streams.asString(stream, "UTF-8");
- // This code is currently used to put multiple files into a single destination.
- // Additional field/values do not make sense.
- ServletOps.errorBadRequest(format("Only files accepted in multipart file upload (got %s=%s)", fieldName, value));
- // errorBadRequest does not return.
- return null;
- }
-
- InputStream input = fileStream.openStream();
+ for ( Part part : request.getParts() ) {
+ InputStream input = part.getInputStream();
Review Comment:
:ok_man: that's some nice simplification!
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/system/DataUploader.java:
##########
@@ -102,6 +102,10 @@ public static UploadDetails incomingData(HttpAction action, StreamRDF dest) {
}
}
+ // 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;
Review Comment:
Do you know if this `__MULTIPART_CONFIG_ELEMENT` attribute was created this way to discourage external use?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1645:
URL: https://github.com/apache/jena/pull/1645#issuecomment-1366700752
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs merged pull request #1645: GH-1643: Replace commons-fileload
Posted by GitBox <gi...@apache.org>.
afs merged PR #1645:
URL: https://github.com/apache/jena/pull/1645
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org