Enable multipart/related on FileUpload #315

mufasa1976 · 2024-05-20T13:02:18Z

SOAP-Requests with MTOM enabled are sending the Requests with Content-Type multipart/related.
Because the Parts would not have content-disposition Headers they would not be recognized by the FileUploads as Parts.

Because this Project is used by Wiremock it would fix the Issue wiremock/wiremock#2176

… at javadoc

codecov-commenter · 2024-05-20T13:27:18Z

Codecov Report

Attention: Patch coverage is 21.42857% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 41.23%. Comparing base (ed002cb) to head (76a84fa).
Report is 340 commits behind head on master.

Files	Patch %	Lines
...ns/fileupload2/core/FileItemInputIteratorImpl.java	0.00%	11 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master     #315       +/-   ##
=============================================
- Coverage     74.50%   41.23%   -33.27%     
+ Complexity      265      173       -92     
=============================================
  Files            30       35        +5     
  Lines          1051     1072       +21     
  Branches        154      145        -9     
=============================================
- Hits            783      442      -341     
- Misses          200      596      +396     
+ Partials         68       34       -34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

garydgregory

Hello @mufasa1976
Please run 'mvn' locally before pushing and fix build issues.

mufasa1976 · 2024-05-20T15:32:03Z

I've fixed the Checkstyle Bugs. But now I got a failure from Jacoco.

[WARNING] Rule violated for bundle commons-fileupload2-core: lines covered ratio is 0.41, but expected minimum is 0.42

The Problem is that I added a Test for the FileItemInputIteratorImpl and AbstractFileUpload but these will not be execute by the module commons-fileupload2-core. Indeed the changed Lines will be covered by Tests of all other Modules because these will extend AbstractFileUploadTest located within the Module commons-fileupload2-core.

But this "Warning" will cause that the Code Coverage of the Module commons-fileupload2-core sinks beyound 0.42 (indeed it is now 0.41). Should I set the Value of the Code-Coverage to 0.41 within commons-fileupload2-core/pom.xml?

garydgregory

@mufasa1976
Thank you for your PR.
Please see my comments.

garydgregory · 2024-05-20T17:16:28Z

...leupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractRequestContext.java


 public abstract class AbstractRequestContext<T> implements RequestContext {
+    /**
+     * the Content-Type Pattern for multipart/related Requests.


Sentences start with a capital letter.

garydgregory · 2024-05-20T17:16:53Z

...leupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractRequestContext.java

@@ -79,4 +85,7 @@ public String toString() {
        return String.format("%s [ContentLength=%s, ContentType=%s]", getClass().getSimpleName(), getContentLength(), getContentType());
    }

+    protected boolean isMultipartRelated(final String contentType) {


New protected and public items should have a Javadoc and since tag.

...pload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java

commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/RequestContext.java

garydgregory · 2024-05-20T17:17:48Z

.../main/java/org/apache/commons/fileupload2/jakarta/servlet5/JakartaServletRequestContext.java

+     *
+     * @return the Request is of type <code>multipart/related</code>
+     */
+    @Override


Add since Javadoc tag.

garydgregory · 2024-05-20T17:17:56Z

.../main/java/org/apache/commons/fileupload2/jakarta/servlet6/JakartaServletRequestContext.java

+     * Is the Request of type <code>multipart/related</code>?
+     *
+     * @return the Request is of type <code>multipart/related</code>
+     */


Add since Javadoc tag.

garydgregory · 2024-05-20T17:18:01Z

...ad2-javax/src/main/java/org/apache/commons/fileupload2/javax/JavaxServletRequestContext.java

+     *
+     * @return the Request is of type <code>multipart/related</code>
+     */
+    @Override


Add since Javadoc tag.

garydgregory · 2024-05-20T17:18:07Z

...portlet/src/main/java/org/apache/commons/fileupload2/portlet/JavaxPortletRequestContext.java

+     * Is the Request of type <code>multipart/related</code>?
+     *
+     * @return the Request is of type <code>multipart/related</code>
+     */


Add since Javadoc tag.

garydgregory · 2024-05-20T17:24:25Z

I've fixed the Checkstyle Bugs. But now I got a failure from Jacoco.
[WARNING] Rule violated for bundle commons-fileupload2-core: lines covered ratio is 0.41, but expected minimum is 0.42
The Problem is that I added a Test for the FileItemInputIteratorImpl and AbstractFileUpload but these will not be execute by the module commons-fileupload2-core. Indeed the changed Lines will be covered by Tests of all other Modules because these will extend AbstractFileUploadTest located within the Module commons-fileupload2-core.

But this "Warning" will cause that the Code Coverage of the Module commons-fileupload2-core sinks beyound 0.42 (indeed it is now 0.41). Should I set the Value of the Code-Coverage to 0.41 within commons-fileupload2-core/pom.xml?

Author

The point of the Jacoco check is to avoid making things worse and, conversely, to improve the overall quality of the component. So I encourage you to improve the state of the code.

Run: mvn clean site (or mvn clean site -P jacoco) and look at the Jacoco report in target/site where you'll see a nice HTML report with code highlights of what's tested and what's not. This will let you make sure that the code you touch in a PR is indeed touched by tests, new or existing. It's not perfect, but we want to quality overall, to grow, not shrink ;-)

mufasa1976 · 2024-05-20T18:50:38Z

I've fixed the Checkstyle Bugs. But now I got a failure from Jacoco.
[WARNING] Rule violated for bundle commons-fileupload2-core: lines covered ratio is 0.41, but expected minimum is 0.42
The Problem is that I added a Test for the FileItemInputIteratorImpl and AbstractFileUpload but these will not be execute by the module commons-fileupload2-core. Indeed the changed Lines will be covered by Tests of all other Modules because these will extend AbstractFileUploadTest located within the Module commons-fileupload2-core.
But this "Warning" will cause that the Code Coverage of the Module commons-fileupload2-core sinks beyound 0.42 (indeed it is now 0.41). Should I set the Value of the Code-Coverage to 0.41 within commons-fileupload2-core/pom.xml?
Author

The point of the Jacoco check is to avoid making things worse and, conversely, to improve the overall quality of the component. So I encourage you to improve the state of the code.

Run: mvn clean site (or mvn clean site -P jacoco) and look at the Jacoco report in target/site where you'll see a nice HTML report with code highlights of what's tested and what's not. This will let you make sure that the code you touch in a PR is indeed touched by tests, new or existing. It's not perfect, but we want to quality overall, to grow, not shrink ;-)

I've ran mvn clean site (and also mvn clean site -P jacoco) but unfortunatly the Jacoco-Report of commons-fileupload2-core shows that NO Lines of AbstractFileUpload or FileItemInputIteratorImpl will be covered.

It seems that Jacoco doesn't take care of Project-internal Dependencies because common-fileupload2-core has been added as a compile Dependency to common-fileupload2-servlet5, common-fileupload2-servlet6, common-fileupload2-javax and common-fileupload2-portlet.

I've made a Code Coverage Run with IntelliJ on the Modules common-fileupload2-servlet5, common-fileupload2-servlet6, common-fileupload2-javax and common-fileupload2-portlet and I have seen that my Code will be covered by the Test I've written.

So unless there is a Chance to tell Jacoco to take some Dependencies in Content the only solution I see is to reduce the Line Complexity on common-fileupload2-core because some of these Classes (i.E. AbstractFileUpload or FileItemInputIteratorImpl) can't be covered by Tests on common-fileupload2-core

garydgregory · 2024-05-20T19:30:13Z

Hello @mufasa1976
Note that it's the lines covered ratio that is at issue, not the complexity.

You added runtime code like:
org.apache.commons.fileupload2.core.AbstractRequestContext.isMultipartRelated(String) and its regular expression constant.
Since there is no concrete subclass in this module. create a subclass in the tests themselves and make sure the method does the right thing with the regular expression added in the PR.
And so on. This should allow the core module to not drop coverage. It should not rely on other modules to test its new code and regular expression anyway.

mufasa1976 · 2024-05-20T20:25:23Z

Hello @mufasa1976 Note that it's the lines covered ratio that is at issue, not the complexity.

You added runtime code like: org.apache.commons.fileupload2.core.AbstractRequestContext.isMultipartRelated(String) and its regular expression constant. Since there is no concrete subclass in this module. create a subclass in the tests themselves and make sure the method does the right thing with the regular expression added in the PR. And so on. This should allow the core module to not drop coverage. It should not rely on other modules to test its new code and regular expression anyway.

You're absolutly right. I've added a new Test MockRequestContextTest where I added additional Tests for AbstractRequestContext. And there I got a Coverage-Ratio of 0.42 or above.

mufasa1976 · 2024-05-21T07:50:55Z

Thank you very much for your patience

mufasa1976 and others added 3 commits May 19, 2024 16:32

added ability to use content-type: multipart/related

fb332ce

Merge branch 'apache:master' into master

6e4e9eb

added entry to src/changes/changes.xml; removed unnecessary paragraph…

148bfaf

… at javadoc

mufasa1976 mentioned this pull request May 20, 2024

[1.x] Enable multipart/related on FileUpload #314

Merged

garydgregory requested changes May 20, 2024

View reviewed changes

fixing checkstyle failures

96e3573

garydgregory changed the title ~~enable multipart/related on FileUpload~~ Enable multipart/related on FileUpload May 20, 2024

garydgregory requested changes May 20, 2024

View reviewed changes

fixed findings of review

9945131

mufasa1976 added 2 commits May 20, 2024 22:19

added MockRequestContextTest

8cc58eb

added License Header

76a84fa

garydgregory merged commit d187c65 into apache:master May 20, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multipart/related on FileUpload #315

Enable multipart/related on FileUpload #315

mufasa1976 commented May 20, 2024

codecov-commenter commented May 20, 2024 •

edited

garydgregory left a comment

mufasa1976 commented May 20, 2024

garydgregory left a comment

garydgregory May 20, 2024

garydgregory May 20, 2024

garydgregory May 20, 2024

garydgregory May 20, 2024

garydgregory May 20, 2024

garydgregory May 20, 2024

garydgregory commented May 20, 2024 •

edited

mufasa1976 commented May 20, 2024

garydgregory commented May 20, 2024

mufasa1976 commented May 20, 2024

mufasa1976 commented May 21, 2024

Enable multipart/related on FileUpload #315

Enable multipart/related on FileUpload #315

Conversation

mufasa1976 commented May 20, 2024

codecov-commenter commented May 20, 2024 • edited

Codecov Report

garydgregory left a comment

Choose a reason for hiding this comment

mufasa1976 commented May 20, 2024

garydgregory left a comment

Choose a reason for hiding this comment

garydgregory May 20, 2024

Choose a reason for hiding this comment

garydgregory May 20, 2024

Choose a reason for hiding this comment

garydgregory May 20, 2024

Choose a reason for hiding this comment

garydgregory May 20, 2024

Choose a reason for hiding this comment

garydgregory May 20, 2024

Choose a reason for hiding this comment

garydgregory May 20, 2024

Choose a reason for hiding this comment

garydgregory commented May 20, 2024 • edited

mufasa1976 commented May 20, 2024

garydgregory commented May 20, 2024

mufasa1976 commented May 20, 2024

mufasa1976 commented May 21, 2024

codecov-commenter commented May 20, 2024 •

edited

garydgregory commented May 20, 2024 •

edited