[JENKINS-73136] Adapt HTTP Request tests for Jetty 12 (EE 8) #172

BorisYaoA · 2024-07-08T07:49:30Z

see https://issues.jenkins.io/browse/JENKINS-73136

The goal of this PR is to adapt http-request-plugin for jetty 12 (EE8).
There was some tests failures with jenkins.plugins.http_request.HttpRequestStepTest and jenkins.plugins.http_request.HttpRequestTest so I had to fix them.
The most of the work is about adapting SimpleHandler classes to use Handler.Abstract

Testing done

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

… container

…for_jetty12

pom.xml

The existing code doesn't use a static import for `StandardCharsets`, so preserve the existing convention to keep this diff focused on substance rather than unrelated stylistic changes.

…asynchronous one

basil

Very nice! I ran this locally and it all works quite nicely. I fixed a handful of trivial bugs and cleaned up the code a little bit. There is just one issue remaining before I am ready to approve this PR.

basil · 2024-08-07T20:38:51Z

src/test/java/jenkins/plugins/http_request/Registers.java

-					assertEquals(uploadFile.length(), part.getSize());
-					assertEquals(uploadFile.getName(), part.getSubmittedFileName());
-					assertEquals(MimeType.APPLICATION_ZIP.getValue(), part.getContentType());
+					if (multipartInputStream != null) {


Try setting a breakpoint here and running the test suite. In my tests multipartInputStream is always null, and the code inside the if statement is never executed. As such, this turns the test into a false negative—the assertions are never executed. Presumably the assertions were executed before, making this a regression.

Thank you for the catch, yes multipartInputStream is always null on my side too.
With what @olamy suggested here BorisYaoA#1, it is no more used and it is more clear.

And Just FTR I've run the same test in debug mode on the actual master branch and multipartInputStream is always null so it seems that the finally below isn't doing nothing

http-request-plugin/src/test/java/jenkins/plugins/http_request/Registers.java

Lines 364 to 371 in 018c44c

} finally {

String MULTIPART =

"org.eclipse.jetty.servlet.MultiPartFile.multiPartInputStream";

MultiPartFormInputStream multipartInputStream =

(MultiPartFormInputStream) request.getAttribute(MULTIPART);

if (multipartInputStream != null) {

multipartInputStream.deleteParts();

}

basil · 2024-08-07T20:39:05Z

src/test/java/jenkins/plugins/http_request/Registers.java

-					assertEquals(content,
-							IOUtils.toString(modelPart.getInputStream(), StandardCharsets.UTF_8));
-					assertEquals(MimeType.APPLICATION_JSON.getValue(), modelPart.getContentType());
+					if (multipartInputStream != null) {


…into Adapt_http_request_for_jetty12 # Conflicts: # src/test/java/jenkins/plugins/http_request/HttpRequestTestBase.java

Signed-off-by: Olivier Lamy <[email protected]>

olamy · 2024-08-09T05:35:20Z

regarding Jetty 12 core handller multipart feel to look at BorisYaoA#1
The secret is with those 2 lines of code ;)

String contentType = request.getHeaders().get(HttpHeader.CONTENT_TYPE);
MultiPartFormData.Parts parts = MultiPartFormData.from(request, request, contentType, new MultiPartConfig.Builder().build()).get();

BorisYaoA · 2024-08-09T07:06:23Z

regarding Jetty 12 core handller multipart feel to look at BorisYaoA#1 The secret is with those 2 lines of code ;)
String contentType = request.getHeaders().get(HttpHeader.CONTENT_TYPE);
MultiPartFormData.Parts parts = MultiPartFormData.from(request, request, contentType, new MultiPartConfig.Builder().build()).get();

Thank you @olamy, it is what I was missing for handling Parts

fix Jetty 12 core handler multipart

…into Adapt_http_request_for_jetty12

"If false is returned, then this method must not generate a response, nor complete the callback."

basil

Thanks for the PR!

BorisYaoA and others added 14 commits July 8, 2024 09:47

Update POM for jetty 12 EE 8

5fbce5a

remove usage of MultiPartFormInputStream as cleanup is managed by the…

ca5ac76

… container

revert

69ad01d

revert clean

9630a67

update pom

f6cf304

remove MultiPartFormInputStream usage

48b7573

Fix import

43fb80c

Fix multipartconfig attribut

71ceac4

adapt Handle methods

4707871

fix bugs

9e3afe3

fix bugs

777ca39

fix Part by using multipartInputStream

5444d1f

Refactoring

27e309a

Merge remote-tracking branch 'origin/master' into Adapt_http_request_…

84003ca

…for_jetty12

basil reviewed Aug 7, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

basil added 14 commits August 7, 2024 11:52

Apply suggestions from code review

f77e660

Avoid static import

77883ff

The existing code doesn't use a static import for `StandardCharsets`, so preserve the existing convention to keep this diff focused on substance rather than unrelated stylistic changes.

Consistent indentation

7f4297b

Remove unused imports

6b81635

Remove unnecessary import

5be3c57

Remove unnecessary use of IOException

115871b

A synchronous use case calls for a synchronous method rather than an …

1bb0d4f

…asynchronous one

Consistent variable naming

b56dee0

Minimize diff with default branch

a649e41

Programming error

7e38874

Fix bug

8a6cbce

Propagate return values

35b2135

Merge branch 'master' into Adapt_http_request_for_jetty12

3f371d8

Stop ignoring parameter

9d5da5b

basil requested changes Aug 7, 2024

View reviewed changes

basil added the tests label Aug 7, 2024

BorisYaoA and others added 2 commits August 8, 2024 10:42

Merge remote-tracking branch 'origin/Adapt_http_request_for_jetty12' …

c6b650d

…into Adapt_http_request_for_jetty12 # Conflicts: # src/test/java/jenkins/plugins/http_request/HttpRequestTestBase.java

fix Jetty 12 core handler multipart

f0b3e05

Signed-off-by: Olivier Lamy <[email protected]>

BorisYaoA and others added 3 commits August 9, 2024 09:08

Merge pull request #1 from olamy/fix-jetty-12-multiparts

9535e5d

fix Jetty 12 core handler multipart

Merge remote-tracking branch 'origin/Adapt_http_request_for_jetty12' …

396acb1

…into Adapt_http_request_for_jetty12

fix unused imports and unused method after handling Parts

50ae44e

BorisYaoA requested a review from basil August 9, 2024 07:54

BorisYaoA marked this pull request as ready for review August 9, 2024 09:48

BorisYaoA requested a review from a team as a code owner August 9, 2024 09:48

basil added 2 commits August 9, 2024 08:59

Code cleanup

841f057

Per the Javadoc:

5aaff3e

"If false is returned, then this method must not generate a response, nor complete the callback."

MarkEWaite added developer Pull requests that improve developer experience and removed tests labels Aug 9, 2024

MarkEWaite changed the title ~~[JENKINS-73136] Adapt HTTP Request for Jetty 12 (EE 8)~~ [JENKINS-73136] Adapt HTTP Request tests for Jetty 12 (EE 8) Aug 9, 2024

basil added 2 commits August 9, 2024 09:06

Return value cleanup

7f84d24

Use Response.writeError where possible

4986697

basil approved these changes Aug 9, 2024

View reviewed changes

basil merged commit fa04893 into jenkinsci:master Aug 9, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73136] Adapt HTTP Request tests for Jetty 12 (EE 8) #172

[JENKINS-73136] Adapt HTTP Request tests for Jetty 12 (EE 8) #172

BorisYaoA commented Jul 8, 2024 •

edited

Loading

basil left a comment

basil Aug 7, 2024

BorisYaoA Aug 9, 2024 •

edited

Loading

basil Aug 7, 2024

olamy commented Aug 9, 2024 •

edited

Loading

BorisYaoA commented Aug 9, 2024 •

edited

Loading

basil left a comment

	} finally {
	String MULTIPART =
	"org.eclipse.jetty.servlet.MultiPartFile.multiPartInputStream";
	MultiPartFormInputStream multipartInputStream =
	(MultiPartFormInputStream) request.getAttribute(MULTIPART);
	if (multipartInputStream != null) {
	multipartInputStream.deleteParts();
	}

[JENKINS-73136] Adapt HTTP Request tests for Jetty 12 (EE 8) #172

[JENKINS-73136] Adapt HTTP Request tests for Jetty 12 (EE 8) #172

Conversation

BorisYaoA commented Jul 8, 2024 • edited Loading

Testing done

Submitter checklist

basil left a comment

Choose a reason for hiding this comment

basil Aug 7, 2024

Choose a reason for hiding this comment

BorisYaoA Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

basil Aug 7, 2024

Choose a reason for hiding this comment

olamy commented Aug 9, 2024 • edited Loading

BorisYaoA commented Aug 9, 2024 • edited Loading

basil left a comment

Choose a reason for hiding this comment

BorisYaoA commented Jul 8, 2024 •

edited

Loading

BorisYaoA Aug 9, 2024 •

edited

Loading

olamy commented Aug 9, 2024 •

edited

Loading

BorisYaoA commented Aug 9, 2024 •

edited

Loading