GH-4374: Eliminate commons-fileupload dependency #4375

erikgb · 2023-01-18T21:13:20Z

GitHub issue resolved: #4374

Briefly describe the changes proposed in this PR:

Removing the dependency to commons-fileupload is a pre-requirement to the javax -> jakarta migration proposed in #3559.

I did the simplest thing to remove the dependency, and IMO the affected code should be refactored more, but I am leaving this to a follow-up PR.

PR Author Checklist (see the contributor guidelines for more details):

my pull request is self-contained
I've added tests for the changes I made
I've applied code formatting (you can use mvn process-resources to format from the command line)
I've squashed my commits where necessary
every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

JervenBolleman

I think if the toLowerCase is removed it can be merged. But feel free to convince me that it should stay :)

JervenBolleman · 2023-01-19T22:03:21Z

tools/workbench/src/main/java/org/eclipse/rdf4j/workbench/util/WorkbenchRequest.java

+		if (contentType == null) {
+			return false;
+		}
+		if (contentType.toLowerCase(Locale.ENGLISH).startsWith("multipart/")) {


I think the multipart/ must always be lowercase when coming from a header so my gut feeling is that enforcing a lowercase is not needed and may even be a point of failure in the future. e.g. badly configured method filtering on a proxy assuming the lowercase rule being by passed by accepting uppercase headers.

I just in-lined the code from commons-fileupload when removing the dependency, to ensure we don't break anything. Is that a good enough argument? IMO better safe than sorry, even if I tend to agree with you. ;-)

tools/workbench/src/main/java/org/eclipse/rdf4j/workbench/base/TransformationServlet.java

abrokenjester · 2023-02-03T22:33:23Z

It took me a while longer than expected to get back to this (the docker ecosystem is even more of a hot mess than I remember from when I last had to reinstall this stuff - took me six attempts to install the correct versions of all the bits).

Unfortunately, when trying to upload an RDF file via the workbench, immediately ran into this:

java.lang.IllegalStateException: Unable to process parts as no multi-part configuration has been provided
	org.apache.catalina.connector.Request.parseParts(Request.java:2905)
	org.apache.catalina.connector.Request.getParts(Request.java:2873)
	org.apache.catalina.connector.RequestFacade.getParts(RequestFacade.java:1086)
	javax.servlet.http.HttpServletRequestWrapper.getParts(HttpServletRequestWrapper.java:349)
	javax.servlet.http.HttpServletRequestWrapper.getParts(HttpServletRequestWrapper.java:349)
	javax.servlet.http.HttpServletRequestWrapper.getParts(HttpServletRequestWrapper.java:349)
	javax.servlet.http.HttpServletRequestWrapper.getParts(HttpServletRequestWrapper.java:349)
	org.eclipse.rdf4j.workbench.util.WorkbenchRequest.getMultipartParameterMap(WorkbenchRequest.java:284)
	org.eclipse.rdf4j.workbench.util.WorkbenchRequest.<init>(WorkbenchRequest.java:74)
	org.eclipse.rdf4j.workbench.base.TransformationServlet.service(TransformationServlet.java:96)
	org.eclipse.rdf4j.workbench.base.AbstractServlet.service(AbstractServlet.java:129)
	org.eclipse.rdf4j.workbench.proxy.ProxyRepositoryServlet.service(ProxyRepositoryServlet.java:100)
	org.eclipse.rdf4j.workbench.proxy.WorkbenchServlet.service(WorkbenchServlet.java:215)
	org.eclipse.rdf4j.workbench.proxy.WorkbenchServlet.handleRequest(WorkbenchServlet.java:137)
	org.eclipse.rdf4j.workbench.proxy.WorkbenchServlet.service(WorkbenchServlet.java:112)
	org.eclipse.rdf4j.workbench.proxy.WorkbenchGateway.service(WorkbenchGateway.java:117)
	org.eclipse.rdf4j.workbench.base.AbstractServlet.service(AbstractServlet.java:129)
	org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
	org.apache.catalina.filters.SetCharacterEncodingFilter.doFilter(SetCharacterEncodingFilter.java:109)
	org.eclipse.rdf4j.workbench.proxy.CacheFilter.doFilter(CacheFilter.java:64)
	org.eclipse.rdf4j.workbench.proxy.CookieCacheControlFilter.doFilter(CookieCacheControlFilter.java:56)

I'll take a look if adding the annotation in AddServlet is enough to fix things.

abrokenjester · 2023-02-03T22:57:20Z

It does not seem to a case of simply adding this to AddServlet :( I'm afraid you're going to have to tweak/test things a bit more on your end @erikgb .

You can find some instructions on how to build and run a local docker image in docker/README_DEV.md. Or if you prefer not to use docker, you can also run mvn -Pquick clean install from the workbench dir and then manually deploy the created warfile in a local Tomcat installation.

erikgb · 2023-02-04T12:22:08Z

@abrokenjester No worries! I suspected this, and will take another look. Could you share your test setup, or is it documented somewhere?

erikgb · 2023-02-04T12:22:51Z

Sorry, I need to read before I write. 😂

erikgb · 2023-02-05T16:54:39Z

@abrokenjester I am having trouble getting this to work in my local environment. The first issue was docker-compse vs. docker compose (the modern variant), but that was easy to fix. I am considering filing a PR to change that. Have you had any discussions about that in the team?

Next issue I am still trying to resolve is that I cannot access the workbench app from my browser. My setup is a bit complex, so I am reaching out to a colleague.

I have now moved the multi-part annotation to all concrete subclasses - since the annotation is not inherited. If you can take another look now at smoke testing, it would be greatly appreciated! I will try to fix my local environment, but I have no idea how the workbench works. So I will probably have a hard time knowing how to test/verify if this is working now.

abrokenjester · 2023-02-05T22:49:55Z

@erikgb yeah good point, I had logged a ticket for the docker issues (#4399) but I haven't pushed changes through yet. I'll make that a priority because it saves a lot of hassle.

erikgb · 2023-02-25T14:07:10Z

After seeing this comment in WorkbenchGateway:

All requests are serviced by this Servlet, though it usually delegates to other Servlets.

And being able to run the workbench locally after #4447, I think this works now! 😅 It was simpler to change than expected.

Maybe you can take another look @abrokenjester?

abrokenjester

Tested locally and workbench appears to work fine. Tested file uploads, exports, queries, updates, and repo creation. 👍

In discussion current solution was considered acceptable.

erikgb · 2023-03-03T21:59:33Z

Tested locally and workbench appears to work fine. Tested file uploads, exports, queries, updates, and repo creation. 👍

Thanks for verifying, @abrokenjester! ❤️ Could we get this merged?

erikgb mentioned this pull request Jan 18, 2023

refactor: eliminate commons-fileupload dependency #4372

Closed

5 tasks

erikgb changed the title ~~GH-4374 Eliminate commons-fileupload dependencies~~ GH-4374 eliminate commons-fileupload dependencies Jan 18, 2023

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch from 1d1bb8d to b4608fe Compare January 18, 2023 22:34

hmottestad changed the base branch from main to develop January 19, 2023 06:51

abrokenjester linked an issue Jan 19, 2023 that may be closed by this pull request

Replace commons-fileupload with servlet 3.0 multipart support #4378

Closed

JervenBolleman self-requested a review January 19, 2023 21:54

JervenBolleman previously requested changes Jan 19, 2023

View reviewed changes

erikgb commented Jan 20, 2023

View reviewed changes

tools/workbench/src/main/java/org/eclipse/rdf4j/workbench/base/TransformationServlet.java Outdated Show resolved Hide resolved

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch from b4608fe to 4699e98 Compare January 20, 2023 11:31

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch 2 times, most recently from 2e9a29f to 894b01e Compare February 5, 2023 16:08

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch from 894b01e to c89c620 Compare February 13, 2023 15:44

erikgb changed the title ~~GH-4374 eliminate commons-fileupload dependencies~~ GH-4374: eliminate commons-fileupload dependencies Feb 16, 2023

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch 2 times, most recently from 9a8b9a7 to c77668b Compare February 20, 2023 09:07

erikgb requested review from JervenBolleman and abrokenjester and removed request for JervenBolleman February 20, 2023 09:34

erikgb changed the title ~~GH-4374: eliminate commons-fileupload dependencies~~ GH-4374: Eliminate commons-fileupload dependencies Feb 20, 2023

erikgb mentioned this pull request Feb 22, 2023

Replace tomcat7-maven-plugin with jetty-maven-plugin #4446

Closed

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch from c77668b to 8c72872 Compare February 25, 2023 10:15

eclipse-rdf4jGH-4374: Eliminate commons-fileupload dependency

19c7f3c

erikgb force-pushed the GH-4374-eliminate-commons-fileupload-dependencies branch from 8c72872 to 19c7f3c Compare February 25, 2023 14:03

erikgb changed the title ~~GH-4374: Eliminate commons-fileupload dependencies~~ GH-4374: Eliminate commons-fileupload dependency Feb 25, 2023

abrokenjester approved these changes Feb 25, 2023

View reviewed changes

abrokenjester requested a review from JervenBolleman February 25, 2023 22:05

abrokenjester merged commit 0e920e2 into eclipse-rdf4j:develop Mar 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-4374: Eliminate commons-fileupload dependency #4375

GH-4374: Eliminate commons-fileupload dependency #4375

erikgb commented Jan 18, 2023

JervenBolleman left a comment

JervenBolleman Jan 19, 2023

erikgb Jan 20, 2023

abrokenjester commented Feb 3, 2023 •

edited

Loading

abrokenjester commented Feb 3, 2023 •

edited

Loading

erikgb commented Feb 4, 2023

erikgb commented Feb 4, 2023

erikgb commented Feb 5, 2023

abrokenjester commented Feb 5, 2023

erikgb commented Feb 25, 2023

abrokenjester left a comment

erikgb commented Mar 3, 2023

GH-4374: Eliminate commons-fileupload dependency #4375

GH-4374: Eliminate commons-fileupload dependency #4375

Conversation

erikgb commented Jan 18, 2023

JervenBolleman left a comment

Choose a reason for hiding this comment

JervenBolleman Jan 19, 2023

Choose a reason for hiding this comment

erikgb Jan 20, 2023

Choose a reason for hiding this comment

abrokenjester commented Feb 3, 2023 • edited Loading

abrokenjester commented Feb 3, 2023 • edited Loading

erikgb commented Feb 4, 2023

erikgb commented Feb 4, 2023

erikgb commented Feb 5, 2023

abrokenjester commented Feb 5, 2023

erikgb commented Feb 25, 2023

abrokenjester left a comment

Choose a reason for hiding this comment

erikgb commented Mar 3, 2023

abrokenjester commented Feb 3, 2023 •

edited

Loading

abrokenjester commented Feb 3, 2023 •

edited

Loading