Add support of Jetty 12 #2593

reta · 2024-01-27T16:32:10Z

Add support of Jetty 12

References

jetty 12 / springboot 3.2 #2395

Road block (so far):

~~org.eclipse.jetty.server.MultiPartInputStreamParser is gone for good~~ replaced with MultiPartFormData.Parser but it does not have any special treatment to Base64 (as Jetty 11 had)
Jetty 12 does not currently support cross context dispatch (workaround - use AdminXxxContext / MockXxxContext explicitly)

Jetty 12 limitations to document:

when serving resource directory, always sends the redirect (if the path does not end with the /)
does not allow to set reason / message along with set response status (HTTP 1.1 at least)
does not decode Base64 (and other encodings) when using multipart form data

TODOs (from @tomakehurst):

Perhaps it might be worth considering a separate JAR after all. I've had a couple of ideas that might make it easier to live with:

Still detect the Jetty version in the core JAR and if it's 12 exit with a message indicating that the user should add the jetty12 JAR to their dependencies.
Use Java service loading to delegate to the appropriate server factory so e.g. when there's no service loader impl available we default to the built-in Jetty 11 factory, but if one is present we use this. Then the Jetty 12 JAR would register its factory as a service implementation and this would get loaded automatically when the JAR was present on the classpath.

Submitter checklist

Recommended: Join WireMock Slack to get any help in #help-contributing or a project-specific channel like #wiremock-java
The PR request is well described and justified, including the body and the references
The PR title represents the desired changelog entry
The repository's code style is followed (see the contributing guide)
Test coverage that demonstrates that the change works as expected
For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

reta · 2024-01-27T16:48:23Z

@tomakehurst would love to hear your opinion before committing more time, the are 2 issues that I run into with implementing the support of both Jetty 11 and Jetty 12:

Jetty 12 needs JDK-17: easy to address by switching the build to JDK-17 but keeping JDK-11 baseline
Jetty 12 has backward incompatible changes with Jetty 11 for existing APIs (fe HandlerCollection, etc.): this is more difficult to deal with since we are having conflicting artifacts / classes but different Jetty versions

The way I was thinking to make it work is that (it could be implemented more or less cleanly):

Jetty 11 stays as the default version (no change here)
build requires JDK-17 but keeps JDK-11 baseline (no user-visible changes here)
use multi-release JAR (Jetty 12 goes to java17), not all build tools deals with that well enough sadly
use dynamic Jetty container detection at runtime

With multi-release JAR we should be able to replace Jetty 11 with Jetty 12 and compile the integration properly. The other option (without the multi-release jar) would be to have some kind of "stubs" at compile time to fill up the API discrepancies (haven't tried that yet but looks a bit ugly).

Would appreciate the hints what direction you would prefer to steer towards, thank you.

tomakehurst · 2024-01-29T14:33:47Z

I broadly agree with your proposed approach, but I think a multi-release JAR might not be such a great idea as it's the Jetty version rather than Java that we're switching on and we want to support the case of Java 17 + Jetty 11 (someone out there will definitely be doing this!).

What I suggest is that we try to find a way to put all of stuff where the Jetty API varies under an abstraction in JettyHttpServer, create jetty11 and jetty12 packages with implementations subclassing this and a default implementation that selects and delegates to the appropriate one after querying Jetty.VERSION.

reta · 2024-01-29T14:40:26Z

Thanks a lot @tomakehurst

I broadly agree with your proposed approach, but I think a multi-release JAR might not be such a great idea as it's the Jetty version rather than Java that we're switching on and we want to support the case of Java 17 + Jetty 11 (someone out there will definitely be doing this!).

My apologies for confusion - the Jetty 12 won't be replacing Jetty 11, Jetty 11 would be default version to try independently of the JDK, the Jetty 12 would only be available on JDK-17+ (as an option)

What I suggest is that we try to find a way to put all of stuff where the Jetty API varies under an abstraction in JettyHttpServer, create jetty11 and jetty12 packages with implementations subclassing this and a default implementation that selects and delegates to the appropriate one after querying Jetty.VERSION.

I will surely try that first, thank you.

tomakehurst · 2024-02-05T08:30:00Z

I suggest we keep all sources under src/main/java and have jetty11 and jetty12 packages rather than having separate source trees for different Java versions. It should be fine to have a load of classes that depend on Jetty 12 provided they're only loaded when Jetty 12 is on the classpath.

reta · 2024-02-05T12:56:04Z

I suggest we keep all sources under src/main/java and have jetty11 and jetty12 packages rather than having separate source trees for different Java versions.

Thanks @tomakehurst , I will try that as well, but I have not figured out yet the way to compile both Jetty 11 and Jetty 12 within same source tree (PS: in all the cases, we have jetty11 and jetty12 packages, it is just jetty12 package comes from different source tree).

tomakehurst · 2024-02-05T20:06:40Z

Yeah, I guess that could be tricky. I wonder if we could put the compatibility tests in a sub-project that has Jetty 12 as its dependency, and keep Jetty 11 in the main project.

reta · 2024-02-05T20:13:45Z

Yeah, I guess that could be tricky. I wonder if we could put the compatibility tests in a sub-project that has Jetty 12 as its dependency, and keep Jetty 11 in the main project.

Exactly, I think we could do that by I haven't yet get to this point. If we find the way to phase off multi-release JAR, that would be easier, otherwise we would need to run Jetty 12 test suite against JAR (if nothing changed recently, none of build tools support running tests for multi-release artifacts if they are not packaged as JARs).

tomakehurst · 2024-02-05T20:16:41Z

The Gradle team only seems to support multi-release very reluctantly.

And I really think we shouldn't be switching it on the Java version.

reta · 2024-02-06T02:48:23Z

And I really think we shouldn't be switching it on the Java version.

I was exploring the hosting the jetty11 and jetty12 under same source tree, but I haven't been able to pull off the compilation: both jetty11 and jetty12 have many identical artifacts (fe org.eclipse.jetty:jetty-server, org.eclipse.jetty:jetty-proxy, version differs for sure) but absolutely incompatible APIs. So we may need to stick to separate source sets.

However what I am exploring now, if we could merge the classes from both source sets (main + jetty12) once the compilation of both succeeds. In this case we don't need the multi-release JAR but single one. How does it sound to you?

tomakehurst · 2024-02-06T08:52:17Z

Actually maybe having src/jetty11 and src/jetty12 source trees might be the solution. You'd comment out one of them while developing but then keep both, in addition to src/java when building so that both sets of classes end up in the JAR.

tomakehurst · 2024-02-08T08:45:57Z

src/main/java/com/github/tomakehurst/wiremock/jetty/JettyHttpServerFactory.java

+  private static final MethodHandle SERVER_CONSTRUCTOR = getServerConstructor();
+
+  @SuppressWarnings("unchecked")
+  private static MethodHandle getServerConstructor() {


Can we avoid using reflection by querying Jetty.VERSION and then delegating to e.g. Jetty11HttpServerFactory or Jetty12HttpServerFactory appropriately?

This would be great from a performance perspective as using reflection this way has a big impact on cold startup time.

Can we avoid using reflection by querying Jetty.VERSION and then delegating to e.g. Jetty11HttpServerFactory or Jetty12HttpServerFactory appropriately?

Absolutely, I haven't thought about that by I remember you suggesting it previously, I will work on that part, thank you

tomakehurst · 2024-02-13T12:33:59Z

Looking at your latest changes I'm starting to wonder if this is going to be a bit too painful to develop and maintain going forward.

Perhaps it might be worth considering a separate JAR after all. I've had a couple of ideas that might make it easier to live with:

Still detect the Jetty version in the core JAR and if it's 12 exit with a message indicating that the user should add the jetty12 JAR to their dependencies.
Use Java service loading to delegate to the appropriate server factory so e.g. when there's no service loader impl available we default to the built-in Jetty 11 factory, but if one is present we use this. Then the Jetty 12 JAR would register its factory as a service implementation and this would get loaded automatically when the JAR was present on the classpath.

We're planning to release WireMock on Thursday, although I'm guessing getting this in is a stretch at the moment.
Happy to assist with this if that would help.

BTW I appreciate you humouring me and trying to make this work the way I previously suggested!

reta · 2024-02-13T13:12:51Z

Thanks for looking @tomakehurst, really appreciate it

Perhaps it might be worth considering a separate JAR after all. I've had a couple of ideas that might make it easier to live with:

This would work (following the steps 1 and 2) but we have 2 choices here:
a) move project to multi-module (so jetty 11 & jetty 12 would be separate modules with delegation to core)
b) have 2 jars (probably with qualifier?) out of the single build

We're planning to release WireMock on Thursday, although I'm guessing getting this in is a stretch at the moment.
Happy to assist with this if that would help.

We are down to ~20 failing tests but large chunk of them is blocked by 2 issues (I also listed them on top but repeat it here for convinience):

Jetty 12 does not allow to set reason / message along with set response status (HTTP 1.1 at least), it goes up to the raw response where reason / message is not pushed down (even if present)
org.eclipse.jetty.server.MultiPartInputStreamParser is gone for good

The 1st one is really difficult to get back - Jetty 12 literally phased out the status propagation from everywhere (if you need code hints, happy to share). For 2nd one we would need a replacement but I have not looked into it yet, if you have time to take it, would be awesome.

tomakehurst · 2024-02-14T19:32:18Z

It looks like we can use MultiPartFormData.Parser in place of MultiPartInputStreamParser albeit with some work as the API isn't the same.

Agree status setting is tricky. I've been Googling a little bit to try to find out where and why they've done this, but without much luck so far so please share any links you've got to explanations of this.

reta · 2024-02-14T23:54:45Z

It looks like we can use MultiPartFormData.Parser in place of MultiPartInputStreamParser albeit with some work as the API isn't the same.

👍 (saw it, thank you)

Agree status setting is tricky. I've been Googling a little bit to try to find out where and why they've done this, but without much luck so far so please share any links you've got to explanations of this.

I believe the context for it is Servlet 6 spec changes where setStatus could set only the status code. Under the hood, the MetaData.Response class is being used to carry over the status + reason BUT the HttpChannelState class, which prepares the response before sending it over the wire, passed null as the reason, always [1].

[1] https://github.com/jetty/jetty.project/blob/645e775114884b44c62a3df21088cef802254b33/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java#L1396

tomakehurst · 2024-02-15T08:53:47Z

I suggest for now we prevent the reason setting tests from running when the Jetty version is 12.

We can document the fact it doesn't work with Jetty 12, and either drop support for it in WireMock 4.0 completely or see if we can persuade the Jetty team to restore this capability.

… when Jetty11 isn't available

reta · 2024-03-01T12:45:21Z

Are you in the Slack community BTW @reta ?

Thanks a lot @tomakehurst , I am on vacation this week, should be back on March 7th (if you need me to take a look but please don't wait for me, I could review post merge), my apologies for inconvenience.

I will join Slack shortly

tomakehurst · 2024-03-01T13:48:30Z

I think it might be in a releasable state. It'd be good to get a 2nd pair of eyes on it, but I can get one of my team to take a look.

The main thing is I wanted to make sure you get properly credited by GitHub for all the work you've put in, which I'm not sure it'll do if I merge my branch.

reta · 2024-03-08T23:31:13Z

The main thing is I wanted to make sure you get properly credited by GitHub for all the work you've put in, which I'm not sure it'll do if I merge my branch.

I am back from vacation (apologies for delay), I like your approach @tomakehurst (I was a bit afraid to introduce subproject), it introduces much cleaner distribution and development models. Please feel free to go ahead, I have no issue with merging your branch, thanks a lot for working with me on that!

tomakehurst · 2024-03-09T21:39:55Z

How about you pull my branch into this PR, then I'll squash and merge? That way you'll get properly credited by the OSS gods.

reta · 2024-03-09T22:08:57Z

How about you pull my branch into this PR, then I'll squash and merge? That way you'll get properly credited by the OSS gods.

Sure! No objections either, on it, thanks a lot @tomakehurst !

Signed-off-by: Andriy Redko <drreta@gmail.com>

reta · 2024-03-10T17:10:13Z

🟢 , we should be all set (the documentation update would be a separate pull request), I had to remove JDK-11 from GA actions since we cannot build Jetty 12 subproject anymore with it

tomakehurst · 2024-03-10T22:05:58Z

I think we should create separate GA workflows for the main project and Jetty12. Ensuring 11 support is too important to drop it completely from the matrix.

reta · 2024-03-11T02:07:31Z

I think we should create separate GA workflows for the main project and Jetty12. Ensuring 11 support is too important to drop it completely from the matrix.

Got it, will do that shortly, thanks @tomakehurst !

…etty12 inclusion (based on JDK version) Signed-off-by: Andriy Redko <drreta@gmail.com>

reta · 2024-03-12T01:06:40Z

.github/workflows/release.yml

@@ -14,7 +14,7 @@ jobs:
      - name: Set up Java
        uses: actions/setup-java@v3
        with:
-          java-version: '11'
+          java-version: '17'


Releasing with JDK-17 so to have wiremock-jetty12 included

reta · 2024-03-12T01:07:41Z

settings.gradle

@@ -1 +1,4 @@
 rootProject.name = 'wiremock'
+if (JavaVersion.current() >= JavaVersion.VERSION_17) {


@tomakehurst making wiremock-jetty12 subproject conditional (based on JDK version)

tomakehurst · 2024-03-12T12:14:47Z

This is looking good. Is it ready to merge as far as you're concerned?

I'll probably tweak the release workflow a bit after merging so don't worry about getting that perfect.

reta · 2024-03-12T13:00:48Z

This is looking good. Is it ready to merge as far as you're concerned?

No concerns, thanks a lot @tomakehurst ! (resolved the conflicts)

Signed-off-by: Andriy Redko <drreta@gmail.com>

reta · 2024-03-26T00:42:43Z

@tomakehurst wondering if you have something in mind for this pull request that holds it off, thank you.

tomakehurst · 2024-03-26T07:39:42Z

It's ready I think. Going to try to merge and release it today, along with a few other things.

tomakehurst self-assigned this Jan 30, 2024

reta force-pushed the issue-2395 branch 2 times, most recently from d0e792d to b42f781 Compare February 4, 2024 18:19

ibodrov mentioned this pull request Feb 4, 2024

project: upgrade dependencies walmartlabs/concord#861

Merged

2 tasks

reta force-pushed the issue-2395 branch 2 times, most recently from da28c16 to d68606e Compare February 8, 2024 00:30

tomakehurst reviewed Feb 8, 2024

View reviewed changes

reta force-pushed the issue-2395 branch 5 times, most recently from 06f6a23 to 75874bd Compare February 11, 2024 16:30

reta mentioned this pull request Feb 12, 2024

jetty 12 / springboot 3.2 #2395

Closed

reta force-pushed the issue-2395 branch from 75874bd to 3657b40 Compare February 13, 2024 03:40

reta force-pushed the issue-2395 branch from 967c4dd to 5294b3c Compare February 16, 2024 00:34

tomakehurst added 5 commits February 29, 2024 15:53

Fixed accidentally bumped version

c05a632

Fixed releasing of wiremock-jetty12

527b1b4

Moved fault tests to own class

cfcb4a9

Build file formatting tweak

1487c0f

Made WireMock scan for HttpServerFactory even if scanning is disabled…

47fc485

… when Jetty11 isn't available

reta force-pushed the issue-2395 branch 3 times, most recently from 59b8425 to a62a686 Compare March 10, 2024 16:35

Merge remote-tracking branch 'origin/master' into issue-2395

6ec605a

Signed-off-by: Andriy Redko <drreta@gmail.com>

reta force-pushed the issue-2395 branch from a62a686 to 6ec605a Compare March 10, 2024 16:55

Restore JDK-11 Github Action workflows and add conditional wiremock-j…

2234602

…etty12 inclusion (based on JDK version) Signed-off-by: Andriy Redko <drreta@gmail.com>

reta commented Mar 12, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into issue-2395

9cfbfad

Signed-off-by: Andriy Redko <drreta@gmail.com>

reta force-pushed the issue-2395 branch from 54f33f9 to 9cfbfad Compare March 12, 2024 14:26

reta added 2 commits March 21, 2024 21:55

Merge remote-tracking branch 'origin/master' into issue-2395

ebf91d0

Merge remote-tracking branch 'origin/master' into issue-2395

f157f05

tomakehurst merged commit 406be86 into wiremock:master Mar 26, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of Jetty 12 #2593

Add support of Jetty 12 #2593

reta commented Jan 27, 2024 •

edited

reta commented Jan 27, 2024 •

edited

tomakehurst commented Jan 29, 2024

reta commented Jan 29, 2024 •

edited

tomakehurst commented Feb 5, 2024

reta commented Feb 5, 2024

tomakehurst commented Feb 5, 2024

reta commented Feb 5, 2024

tomakehurst commented Feb 5, 2024

reta commented Feb 6, 2024 •

edited

tomakehurst commented Feb 6, 2024

tomakehurst Feb 8, 2024

reta Feb 8, 2024

tomakehurst commented Feb 13, 2024

reta commented Feb 13, 2024 •

edited

tomakehurst commented Feb 14, 2024

reta commented Feb 14, 2024

tomakehurst commented Feb 15, 2024

reta commented Mar 1, 2024 •

edited

tomakehurst commented Mar 1, 2024

reta commented Mar 8, 2024 •

edited

tomakehurst commented Mar 9, 2024

reta commented Mar 9, 2024

reta commented Mar 10, 2024

tomakehurst commented Mar 10, 2024

reta commented Mar 11, 2024

reta Mar 12, 2024

reta Mar 12, 2024

tomakehurst commented Mar 12, 2024

reta commented Mar 12, 2024

reta commented Mar 26, 2024

tomakehurst commented Mar 26, 2024

		@@ -1 +1,4 @@
		rootProject.name = 'wiremock'
		if (JavaVersion.current() >= JavaVersion.VERSION_17) {

Add support of Jetty 12 #2593

Add support of Jetty 12 #2593

Conversation

reta commented Jan 27, 2024 • edited

References

Submitter checklist

reta commented Jan 27, 2024 • edited

tomakehurst commented Jan 29, 2024

reta commented Jan 29, 2024 • edited

tomakehurst commented Feb 5, 2024

reta commented Feb 5, 2024

tomakehurst commented Feb 5, 2024

reta commented Feb 5, 2024

tomakehurst commented Feb 5, 2024

reta commented Feb 6, 2024 • edited

tomakehurst commented Feb 6, 2024

tomakehurst Feb 8, 2024

Choose a reason for hiding this comment

reta Feb 8, 2024

Choose a reason for hiding this comment

tomakehurst commented Feb 13, 2024

reta commented Feb 13, 2024 • edited

tomakehurst commented Feb 14, 2024

reta commented Feb 14, 2024

tomakehurst commented Feb 15, 2024

reta commented Mar 1, 2024 • edited

tomakehurst commented Mar 1, 2024

reta commented Mar 8, 2024 • edited

tomakehurst commented Mar 9, 2024

reta commented Mar 9, 2024

reta commented Mar 10, 2024

tomakehurst commented Mar 10, 2024

reta commented Mar 11, 2024

reta Mar 12, 2024

Choose a reason for hiding this comment

reta Mar 12, 2024

Choose a reason for hiding this comment

tomakehurst commented Mar 12, 2024

reta commented Mar 12, 2024

reta commented Mar 26, 2024

tomakehurst commented Mar 26, 2024

reta commented Jan 27, 2024 •

edited

reta commented Jan 27, 2024 •

edited

reta commented Jan 29, 2024 •

edited

reta commented Feb 6, 2024 •

edited

reta commented Feb 13, 2024 •

edited

reta commented Mar 1, 2024 •

edited

reta commented Mar 8, 2024 •

edited