[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #542
Conversation
basil
changed the title
| */ | ||
| void setHeaders(FileItemHeaders headers); | ||
|
|
||
| default org.apache.commons.fileupload2.core.FileItem toFileUpload2FileItem() { |
| }; | ||
| } | ||
|
|
||
| static FileItem fromFileUpload2FileItem(org.apache.commons.fileupload2.core.FileItem from) { |
|
|
||
| @Override | ||
| public org.apache.commons.fileupload2.core.FileItem toFileUpload2FileItem() { | ||
| return from; |
There was a problem hiding this comment.
Prevent infinite recursion
| */ | ||
| Iterator<String> getHeaderNames(); | ||
|
|
||
| default org.apache.commons.fileupload2.core.FileItemHeaders toFileUpload2FileItemHeaders() { |
| }; | ||
| } | ||
|
|
||
| static FileItemHeaders fromFileUpload2FileItemHeaders(org.apache.commons.fileupload2.core.FileItemHeaders from) { |
| * C library, it might create a file named "foo.exe", as the NUL | ||
| * character is the string terminator in C. | ||
| */ | ||
| public class InvalidFileNameException extends RuntimeException { |
There was a problem hiding this comment.
Part of the public API and therefore included for compatibility purposes
| File tmpDir; | ||
| try { | ||
| tmpDir = Files.createTempDirectory("jenkins-stapler-uploads").toFile(); | ||
| } catch (IOException e) { | ||
| throw new ServletException("Error creating temporary directory", e); | ||
| } | ||
| tmpDir.deleteOnExit(); | ||
| upload = new ServletFileUpload(new DiskFileItemFactory(DiskFileItemFactory.DEFAULT_SIZE_THRESHOLD, tmpDir)); | ||
| upload = new JavaxServletDiskFileUpload(DiskFileItemFactory.builder().setFile(tmpDir).get()); |
There was a problem hiding this comment.
The new builder API allows us to omit the default size threshold without any change in behavior. I verified the builder's default is the same number as the old default that we were explicitly passing in.
| @@ -1158,6 +1166,7 @@ private boolean isMultipart() { | |||
| } | |||
|
|
|||
| @Override | |||
| @WithBridgeMethods(value = org.apache.commons.fileupload.FileItem.class, adapterMethod = "fromFileUpload2FileItem") | |||
There was a problem hiding this comment.
The Java compiler will generate a getFileItem(String) method that returns the new FileUpload 2.x type. The bridge method injector will call the abovementioned method, then call fromFileUpload2FileItem to generate the old type. Thus compatibility is retained.
| CONVERT_UTILS.register(new Converter() { | ||
| @Override | ||
| public org.apache.commons.fileupload.FileItem convert(Class type, Object value) { | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| return org.apache.commons.fileupload.FileItem.fromFileUpload2FileItem(Stapler.getCurrentRequest().getFileItem(value.toString())); | ||
| } catch (ServletException | IOException e) { | ||
| throw new ConversionException(e); | ||
| } | ||
| } | ||
| }, org.apache.commons.fileupload.FileItem.class); |
There was a problem hiding this comment.
I honestly can't remember why I had to do this, but some test failed somewhere along the line back in August 2023 and I added this to fix it. If someone really wants to know the justification, I can try removing this and re-running hours worth of test suites to remember the original motivation, but I'm hoping nobody cares enough to ask this question.
There was a problem hiding this comment.
If I were to be interested in the answer, what test suites are you referring to? Core and/or BOM PCT?
There was a problem hiding this comment.
Yes, it was some PCT or ATH test if I remember correctly.
| @@ -497,6 +498,7 @@ public interface StaplerRequest extends HttpServletRequest { | |||
| * This includes the case where the name corresponds to a simple | |||
| * form field (like textbox, checkbox, etc.) | |||
| */ | |||
| @WithBridgeMethods(org.apache.commons.fileupload.FileItem.class) | |||
There was a problem hiding this comment.
Bridge methods need to be added to both interface and implementation. See the unit tests for bridge-method-injector.
| @@ -1158,6 +1166,7 @@ private boolean isMultipart() { | |||
| } | |||
|
|
|||
| @Override | |||
| @WithBridgeMethods(value = org.apache.commons.fileupload.FileItem.class, adapterMethod = "fromFileUpload2FileItem") | |||
| public FileItem getFileItem(String name) throws ServletException, IOException { | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
bridge methods work at runtime but there is still a compilation error. Since I don't want to force plugins to adopt the new API when bumping their core baseline, simpler to avoid bridge methods here.
See jenkinsci/jenkins#9259 for the PR description.
Testing done
See jenkinsci/jenkins#9259.