refactor: eliminate commons-fileupload dependency #4372
Conversation
| @@ -31,6 +32,7 @@ | |||
| import org.slf4j.Logger; | |||
| import org.slf4j.LoggerFactory; | |||
|
|
|||
| @MultipartConfig | |||
There was a problem hiding this comment.
I suspect this annotation is not inherited, and would have to be added to all subclasses. But hoping tests and/or maintainers can verify if this works.
There was a problem hiding this comment.
I'm not familiar with this annotation. What does it do?
There was a problem hiding this comment.
It adds multipart request support to the servlet. Can also be configured in web.xml. I am happy to do that instead, but I need to know which servlets need it - as this is configured per servlet. See https://docs.oracle.com/javaee/7/tutorial/servlets011.htm for additional details.
erikgb
force-pushed
the
no-commons-fileupload
branch
from
6ae8394
to
257d227
Compare
|
@erikgb can you can create an issue specifically for this please, and link your PR to it? We base our release notes on our issue backlog so it's kinda important that changes are represented by issues. Please have a look at the contributor guidelines for details on how to create your issues and PRs. |
There was a problem hiding this comment.
Looks good to me! I haven't been able to test it yet (Can you perhaps add a few test to cover this behavior? I'm aware test coverage on the workbench is currently poor), but the idea looks solid enough. And eliminating an unneeded/obsolete dependency is a good idea.
| @@ -31,6 +32,7 @@ | |||
| import org.slf4j.Logger; | |||
| import org.slf4j.LoggerFactory; | |||
|
|
|||
| @MultipartConfig | |||
There was a problem hiding this comment.
I'm not familiar with this annotation. What does it do?
Thanks. Apart from verifying the new multipart setup, I have tried to do a safe refactoring, so I do not expect any issues when multipart request works at all. Sorry, I probably don't have the bandwidth to add additional tests now. According to @hmottestad there are some tests that could be enabled in CI, ref. #4370 (comment) |
|
Superseeded by #4375, closing. |
and replace it with multipart support added in Servlet 3.0.
GitHub issue resolved: #
Briefly describe the changes proposed in this PR:
commons-fileupload hasn't had any release since 2018, and is not really required - as Java EE added support for multipart handling in Servlet 3.0, ref. https://docs.oracle.com/javaee/7/tutorial/servlets011.htm
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):
mvn process-resourcesto format from the command line)