Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade from javax.servlet-api-3.1 to jakarta.servlet-api:4.0 and ban javax.servlet:javax.servlet-api #693

Merged
merged 9 commits into from Apr 28, 2023

Conversation

jeromepochat
Copy link
Contributor

When running tests based on JenkinsRule, multiple servlet APIs are available from tests classpath, then causing some issues.

For example :

javax.servlet.http.HttpSessionBindingListener implementation class from Jetty is compiled using 4.0 (with default) and test execution is using 3.1 (without default), then causing:

java.lang.AbstractMethodError: Receiver class org.eclipse.jetty.security.authentication.SessionAuthentication does not define or inherit an implementation of the resolved method abstract valueBound(Ljavax/servlet/http/HttpSessionBindingEvent;)V of interface javax.servlet.http.HttpSessionBindingListener.
	at org.eclipse.jetty.server.session.Session.bindValue(Session.java:357)

pom.xml Outdated Show resolved Hide resolved
@jglick
Copy link
Member

jglick commented Feb 16, 2023

I am not sure offhand if this is safe. Does it even work—is there a reproducible test case for the observed error? See https://github.com/jenkinsci/plugin-pom/tree/master/src/it

Since #478 we require 2.361.x or newer. Core is using this dep in jenkinsci/jenkins#6801 which is only in 3.363. In current versions of core it seems the only 3.x Servlet dep comes via the old Jetty 9 WebSocket adapter, which as in jenkinsci/jenkins#7101 (comment) I think could now be deleted since jenkinsci/jenkins-test-harness#453 is widely adopted.

@jeromepochat
Copy link
Contributor Author

Test case added 7b2d225

@jeromepochat
Copy link
Contributor Author

I tried locally to remove Jetty 9 WebSocket adpater from core, but this does not fix plugin issue as servlet-api-3.1 dependency is still active from plugin-pom.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not entirely sure I understand the subtleties here but seems right.

*/
@Test
public void involveHttpSessionBindingListener() throws Exception {
Jenkins.get().setSecurityRealm(new LegacySecurityRealm());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the error is not reproducible after

Suggested change
Jenkins.get().setSecurityRealm(new LegacySecurityRealm());
Jenkins.get().setSecurityRealm(rule.createDummySecurityRealm());

I point this out because LegacySecurityRealm is almost never used (it was superseded in 2007 and should perhaps be dropped altogether along with support for non-Winstone containers), and if a routine usage of HtmlUnit were broken after some core or tooling update we would have noticed that in lots of popular plugins by now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix @jglick !

BTW it this PR is still revelant as similar issue could occurs as having multiple versions of APIs in same classloader (at least from plugin tests).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially yes. I suspect the conflict just rarely matters, but using LegacySecurityRealm in a test activates a code path in the servlet container which is otherwise rarely encountered.

src/it/servlet-api/src/test/java/test/ServletAPITest.java Outdated Show resolved Hide resolved
@jglick jglick requested a review from basil February 17, 2023 13:35
@jglick jglick added the bug label Feb 17, 2023
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
@basil basil removed their request for review February 27, 2023 16:08
@olamy
Copy link
Member

olamy commented Apr 27, 2023

maybe a bit nit :)
I wonder maybe now we should add a ban of javax.servlet:javax.servlet-api?
just to be sure nobody add it.
doesn't look to be used so much: https://github.com/search?q=org%3Ajenkinsci+%3CartifactId%3Ejavax.servlet-api%3C%2FartifactId%3E&type=code&p=1

@olamy olamy changed the title Upgrade from javax.servlet-api-3.1 to jakarta.servlet-api:4.0 Upgrade from javax.servlet-api-3.1 to jakarta.servlet-api:4.0 and ban javax.servlet:javax.servlet-api Apr 28, 2023
@olamy olamy merged commit 9502105 into jenkinsci:master Apr 28, 2023
1 check passed
@jglick
Copy link
Member

jglick commented May 1, 2023

#751 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants