[JENKINS-73258] Migrate Stapler from EE 8 to EE 9 #482

basil · 2023-08-02T18:46:54Z

Context

Goes with three other pull requests:

Problem

Stapler only supports EE 8, precluding an upgrade to Spring Security 6.x in Jenkins core (which requires Spring Framework 6.x, which requires EE 9).

Solution

Update dependencies in pom.xml to align with Jetty 12 (EE 9)
Migrate imports
Add compatibility layer for EE 8 to support Jenkins plugins

At a high level, we add StaplerRequest2 and StaplerResponse2 classes, the Jakarta equivalents of the old versions (now deprecated). The RequestImpl and ResponseImpl implementations are now based on EE 9 rather than EE 8, but support for plugins is maintained by converting to and from the interfaces as needed.

Testing done

Full PCT and ATH (Jenkins public version and CloudBees internal version)

basil · 2024-08-27T22:47:20Z

core/pom.xml

+    <dependency>
+      <groupId>org.eclipse.jetty.toolchain</groupId>
+      <artifactId>jetty-servlet-api</artifactId>
+      <version>4.0.6</version>
+    </dependency>


A dependency of the compatibility layer. Not part of the servlet container (Jetty) classpath, but part of the web application classpath in order to handle calls made from Jenkins plugins that were compiled against EE 8.

core/src/main/java/io/jenkins/servlet/AsyncContextWrapper.java

basil · 2024-08-27T22:51:00Z

core/src/main/java/io/jenkins/servlet/ServletRequestWrapper.java

+public class ServletRequestWrapper {
+    public static jakarta.servlet.ServletRequest toJakartaServletRequest(ServletRequest from) {
+        if (from instanceof JavaxServletRequestWrapper javax) {
+            return javax.toJakartaServletRequest();


Here and elsewhere, prevent infinite recursion by returning the original request.

basil · 2024-08-27T22:52:11Z

core/src/main/java/io/jenkins/servlet/http/CookieWrapper.java

+import java.util.Objects;
+import javax.servlet.http.Cookie;
+
+public class CookieWrapper {


Does a deep copy rather than wrapping the Cookie object (which is a POJO rather than an interface), but an audit of usages shows that this should not be a problem for existing callers.

basil · 2024-08-27T22:54:23Z

core/src/main/java/org/kohsuke/stapler/CompatibleFilter.java

+import jakarta.servlet.ServletResponse;
+import java.io.IOException;
+
+public interface CompatibleFilter extends Filter {


A convenience class that allows existing Filter subclasses in Stapler and Jenkins core to be used as-is with either EE 8 (i.e., old plugins) or EE 9 (modern Stapler and core). I sprinkled this liberally wherever a class extends Filter in Stapler or core. Perhaps not all of them are necessary, and this can be simplified in the future when usages of Filter in plugins have been migrated to EE 9.

basil · 2024-08-27T22:59:31Z

core/src/main/java/org/kohsuke/stapler/ReflectionUtils.java

+     * @throws IllegalArgumentException When {@code derived} does not derive from {@code base}, or when {@code base}
+     *                                  does not contain the specified method.
+     */
+    public static boolean isOverridden(


Copypasta of the corresponding utilities from core. Unfortunately I can't easily mark them as @Restricted(DoNotUse.class) at present because this repository isn't set up for lib-access-modifier. Perhaps someone else will find the energy to add both lib-access-modifier and the annotation. Unfortunately I can't make it package-private either, as it needs to be used in a lot of places in Stapler.

basil · 2024-08-27T23:00:16Z

core/src/main/java/org/kohsuke/stapler/RequestImpl.java

 *
 * @author Kohsuke Kawaguchi
 */
-public class RequestImpl extends HttpServletRequestWrapper implements StaplerRequest {
+public class RequestImpl extends HttpServletRequestWrapper implements StaplerRequest2 {


This implementation class is changing incompatibly to implement StaplerRequest2. With the exception of a single proprietary plugin, no plugins care about the implementation class, just the interface.

basil · 2024-08-27T23:01:41Z

core/src/main/java/org/kohsuke/stapler/StaplerRequest2.java

+ * @see Stapler#getCurrentRequest2()
+ * @author Kohsuke Kawaguchi
+ */
+public interface StaplerRequest2 extends HttpServletRequest {


The Jakarta version of StaplerRequest (which is now deprecated).

basil · 2024-08-27T23:04:26Z

core/src/main/java/org/kohsuke/stapler/jsr269/QueryParameterAnnotationProcessor.java

+                String paramNames = m.getParameters().stream()
+                        .map(VariableElement::getSimpleName)
+                        .collect(Collectors.joining(","));
+                String existing = output.get(m.getSimpleName().toString());
+                /*
+                 * Allow multiple methods to have the same name but different argument types as long as the arguments
+                 * have the same names. This allows deprecated StaplerRequest/StaplerResponse methods to coexist
+                 * alongside non-deprecated StaplerRequest2/StaplerResponse2 methods.
+                 */
+                if (existing == null || !existing.equals(paramNames)) {
+                    write(paramNames, m);
+                    output.put(m.getSimpleName().toString(), paramNames);
+                }


Needed to fix a compilation error in core. Comes with an associated unit test that demonstrates the problem.

basil · 2024-08-27T23:06:57Z

jelly/src/main/java/org/kohsuke/stapler/jelly/DefaultScriptInvoker.java

+        context.setVariable("request", StaplerRequest.fromStaplerRequest2(req));
+        context.setVariable("response", StaplerResponse.fromStaplerResponse2(rsp));
+        context.setVariable("request2", req);
+        context.setVariable("response2", rsp);


I had briefly considered changing request and response into Jakarta versions, but this broke plugins with Jelly views that called plugin methods whose parameters were requests or responses.

timja

Thanks!

vwagh-dev · 2024-08-28T11:53:12Z

core/src/main/java/io/jenkins/servlet/AsyncContextWrapper.java

+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+public class AsyncContextWrapper {


@basil Just curious why don't we have the documentation for any of the class here. At least 1-2 liner description would have given the clarity that the purpose of that class.

This would not hurt, but the most important aspect of Javadoc in this PR is linking to non-deprecated versions of newly-deprecated functionality, which I have done consistently.

vwagh-dev

LGTM

BorisYaoA

LGTM

Kevin-CB

Looks fine, security-wise

daniel-beck

Looks good as far as I can evaluate it 👍

Some *Wrapper methods seem to not handle documented null values well, but I'm not sure how relevant that really is.

StaplerResponse*#getCompressed* methods were accidentally undeprecated.

I don't think any of my comments need to be addressed for the weekly.

core/src/main/java/org/kohsuke/stapler/DataBoundResolvable.java

core/src/main/java/org/kohsuke/stapler/StaplerResponse2Wrapper.java

core/src/main/java/io/jenkins/servlet/http/HttpSessionEventWrapper.java

core/src/main/java/org/kohsuke/stapler/StaplerRequest.java

jelly/src/main/java/org/kohsuke/stapler/jelly/DefaultScriptInvoker.java

core/src/main/java/io/jenkins/servlet/ServletContextWrapper.java

Co-authored-by: Daniel Beck <[email protected]>

basil added the work-in-progress label Aug 2, 2023

basil force-pushed the jakarta branch 3 times, most recently from 9818bb9 to 28b6ce2 Compare May 15, 2024 21:25

basil changed the title ~~Jakarta EE 9 transition~~ Jetty 12 EE 8 to Jetty 12 EE 9 May 16, 2024

basil changed the title ~~Jetty 12 EE 8 to Jetty 12 EE 9~~ Upgrade Jetty from 10.0.20 to 12.0.9 (EE 8) May 16, 2024

basil force-pushed the jakarta branch 2 times, most recently from 4996efc to 3f566c2 Compare May 16, 2024 00:11

basil changed the title ~~Upgrade Jetty from 10.0.20 to 12.0.9 (EE 8)~~ EE 8 to EE 9 May 16, 2024

basil force-pushed the jakarta branch 13 times, most recently from 7dd5d97 to 4721447 Compare May 23, 2024 16:07

basil force-pushed the jakarta branch 6 times, most recently from b2a0128 to 7ab2633 Compare May 29, 2024 01:09

basil force-pushed the jakarta branch 2 times, most recently from 8d92a23 to c2378c0 Compare June 3, 2024 18:48

basil force-pushed the jakarta branch 2 times, most recently from 6299e57 to 2d50895 Compare August 27, 2024 19:40

[JENKINS-73258] Migrate Stapler from EE 8 to EE 9

6830dd3

basil force-pushed the jakarta branch from 2d50895 to 6830dd3 Compare August 27, 2024 20:34

basil commented Aug 27, 2024

View reviewed changes

basil marked this pull request as ready for review August 27, 2024 23:08

This was referenced Aug 28, 2024

[JENKINS-73259] Migrate Winstone from EE 8 to EE 9 jenkinsci/winstone#396

Merged

[JENKINS-73256] Migrate Jelly from EE 8 to EE 9 jenkinsci/jelly#114

Merged

[JENKINS-73278] Migrate core from EE 8 to EE 9 jenkinsci/jenkins#9672

Merged

timja approved these changes Aug 28, 2024

View reviewed changes

vwagh-dev reviewed Aug 28, 2024

View reviewed changes

vwagh-dev approved these changes Aug 28, 2024

View reviewed changes

MarkEWaite approved these changes Aug 29, 2024

View reviewed changes

daniel-beck mentioned this pull request Aug 30, 2024

Add support for StaplerRequest2/StaplerResponse2 jenkins-infra/jenkins-codeql#45

Open

BorisYaoA approved these changes Aug 30, 2024

View reviewed changes

Merge branch 'master' into jakarta

c3c8b63

Kevin-CB approved these changes Sep 2, 2024

View reviewed changes

daniel-beck approved these changes Sep 3, 2024

View reviewed changes

basil and others added 9 commits September 3, 2024 08:32

Merge branch 'master' into jakarta

6aad5e3

Use released version

a8c6e02

https://github.com/jenkinsci/stapler/pull/482#discussion_r1738459565

3de077e

https://github.com/jenkinsci/stapler/pull/482#discussion_r1732709697

78cf874

Null checks

8160998

Add null checks

1e2df1e

Update core/src/main/java/org/kohsuke/stapler/StaplerResponse2.java

0287073

Co-authored-by: Daniel Beck <[email protected]>

Update core/src/main/java/org/kohsuke/stapler/StaplerResponse2.java

40f6ad8

Co-authored-by: Daniel Beck <[email protected]>

Spotless

499af8d

basil merged commit 994adb3 into master Sep 3, 2024
3 of 5 checks passed

basil deleted the jakarta branch September 3, 2024 16:27

jglick mentioned this pull request Oct 31, 2024

Clarify SECURITY-3315 error code on client side jenkinsci/jenkins#9930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73258] Migrate Stapler from EE 8 to EE 9 #482

[JENKINS-73258] Migrate Stapler from EE 8 to EE 9 #482

basil commented Aug 2, 2023 •

edited

Loading

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

basil Aug 27, 2024

timja left a comment

vwagh-dev Aug 28, 2024

basil Aug 28, 2024

vwagh-dev left a comment

BorisYaoA left a comment

Kevin-CB left a comment

daniel-beck left a comment

[JENKINS-73258] Migrate Stapler from EE 8 to EE 9 #482

[JENKINS-73258] Migrate Stapler from EE 8 to EE 9 #482

Conversation

basil commented Aug 2, 2023 • edited Loading

Context

Problem

Solution

Testing done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vwagh-dev left a comment

Choose a reason for hiding this comment

BorisYaoA left a comment

Choose a reason for hiding this comment

Kevin-CB left a comment

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

basil commented Aug 2, 2023 •

edited

Loading