-
Notifications
You must be signed in to change notification settings - Fork 137
Jakarta EE 9+ migration #266
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
base: master
Are you sure you want to change the base?
Conversation
…akarta packages as well
…dded few required override methods
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
👍 looks like a good start but the PR feels incomplete. my idea about supporting jakarta/javax.servlet at the same time would have been converging towards moving all .rb bits that script the javax.servlet API into .java and than potentially use some kind of a weaving approach or the "migration" tool than Tomcat has to release 2 artifacts from the same build... 🤷 |
Exciting! This will obviously need to be rebased on master due to recent updates there, but I'm glad you've attempted this work.
I would tend to lean toward a hard version move as a 2.0 release myself. The old javax.servlet users have to be dwindling pretty fast these days. If we continued to maintain a 1.x line for a while that supports javax.servlet and provided 2.x for jakarta, we'd avoid any complex build situation and everyone would still be able to keep working with what they have. We might start dropping some deprecation warnings into the 1.x line at some point. @kares Are you opposed to this clean break, branching off 1.2.x and releasing a 2.x that uses only jakarta packages going forward? |
Not at all, just shared my personal opinions, think some folks might still be stuck with older versions here and there due "legacy" (enterprise) reasons. Thus was thinking about ways to support both wout having extra work (such as backports). I am very happy to see any effort such as this one (a full API rewrite) to keep the jruby-rack project going. Regarding this PR my only concern is that some methods feel like stubs and should be revisited implementation wise according to the servlet specification. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Another option would be a new named gem that is jakarta-rack or something, but there may be transitive dependency issues getting RubyGems/Bundler to pick that up. |
… into jakarta-update
Thanks for all the comments. The branch is now synced with the latest master code.
I'm not an expert in java, so I don't know what the implementation should be. In most places I've just updated |
b46f2c8
to
3bb23a1
Compare
# Conflicts: # pom.xml # src/main/java/org/jruby/rack/jms/DefaultQueueManager.java # src/main/java/org/jruby/rack/jms/QueueContextListener.java # src/main/java/org/jruby/rack/jms/QueueManager.java # src/main/java/org/jruby/rack/servlet/ResponseCapture.java # src/main/java/org/jruby/rack/servlet/RewindableInputStream.java # src/main/java/org/jruby/rack/servlet/ServletRackIncludedResponse.java # src/main/ruby/jruby/rack/queues.rb # src/spec/java/org/jruby/rack/mock/DelegatingServletInputStream.java # src/spec/java/org/jruby/rack/mock/DelegatingServletOutputStream.java # src/spec/java/org/jruby/rack/mock/MockAsyncContext.java # src/spec/java/org/jruby/rack/mock/MockHttpServletRequest.java # src/spec/java/org/jruby/rack/mock/MockHttpServletResponse.java # src/spec/java/org/jruby/rack/mock/MockHttpSession.java # src/spec/java/org/jruby/rack/mock/MockRequestDispatcher.java # src/spec/java/org/jruby/rack/mock/MockServletConfig.java # src/spec/java/org/jruby/rack/mock/MockServletContext.java # src/spec/java/org/jruby/rack/mock/MockSessionCookieConfig.java # src/spec/java/org/jruby/rack/mock/WebUtils.java # src/spec/ruby/jruby/rack/servlet_ext_spec.rb # src/spec/ruby/rack/jms_spec.rb
3bb23a1
to
3182a7f
Compare
We're still some way away from being able to merge/release something like this given we haven't yet released
|
# Conflicts: # .github/workflows/maven.yml # README.md # src/main/java/org/jruby/rack/UnmappedRackFilter.java # src/main/java/org/jruby/rack/servlet/ResponseCapture.java # src/spec/ruby/spec_helper.rb
For now, let's propagate this here, to allow moving forward. Need to step back and decide if this is still needed.
Temporarily propagated the removed To get this over the line, we probably mainly need to
Ideally we'd also have some stronger Rails regression tests reinstated to bring more confidence in these bigger releases, but that's probably a nice-to-have. |
# Conflicts: # src/main/java/org/jruby/rack/embed/Filter.java
Is there a reason to require Java 17 minimum? See my comment in #234, but basically I know of at least one large jruby-rack user that still has to support Java 8 for some time. |
For compatibility reasons, I'd argue for sticking tracking the underlying Jakarta EE compat. I can't find a quick compatibility matrix online to reference, but I'm pretty sure that means:
This PR says Jakarta EE 9+, but looking at the pom.xml, it seems that we're targeting the Jakarta EE 11 packages. If that's the case, if this is meant to jump directly to Jakarta EE 11, then I believe Java 17 is required. |
Require, no, but because the library is tested with Spring framework they made a decision to require Java 17 to get JEE 9 support thus it cant be tested on lower Java versions anyway. It's not deliberately targeting JEE 11 packages. (As a side note, the Jakarta namespace move is one of the dumbest,.most painful wastes of time in the Java ecosystem for a decade) |
@headius For your "large jruby-rack user that still has to support Java 8 for some time", are they large enough to warrant an attempt at creating a separate Java 8/Jakarta EE 9/Rack ?? version? Or are their needs met with the current release? |
I did some more poking locally, and am caught up with what @chadlwilson was saying. Even if I lower the packages to their EE 9 variants and update the main code to restore Java 8 compatibility, the spring-test dependency is problematic. Spring Test 5.3.39 is using the old javax packages, and 6.0.23 is compiled with Java 17. There's a non-zero chance that I might poke further at getting the testing side of EE 9+SE 8 resolved but I wouldn't hold your breath. I'm content to agree it would be a non-trivial effort to support EE 9 on Java 8 in jruby-rack. If I wanted to be more immediately useful, where would folks recommend I direct some effort? I see this comment from a few days ago, but I'm not sure where the |
Thanks for your interest @ajuckel . To begin with, I'll dig back and see why I bumped the servlet API further than 5.0. Possibly I thought it didn't do any 'harm' to support the newer API definitions as nothing had been removed, only added. But the spring test usage is more restrictive. Perhaps we can relook at that. I haven't really dug into its importance outside using it for mock servlet requests etc, but it might be possible to go back to what was done before and copy their mock code into the library, even though I was trying to avoid that maintenance overhead/cruft. |
Right now
|
# Conflicts: # .github/workflows/maven.yml # pom.xml # src/main/java/org/jruby/rack/ext/Logger.java # src/spec/ruby/jruby/rack/integration_spec.rb
Interesting, have considered Spring to be conservative - that feels like quite the jump they made with JakartaEE 9.0. Spring was brought in simply because of having the On the other hand, if we bump to jruby-rack it's perfectly reasonable to bump the minimum Java as well, to an up-to-date version. If someone migrates from javax.servler -> jakarta they might as well bump Java from 8. 🤷 |
and before I forget due jruby-rack being both a Java library as well as a Ruby gem, assuming the 1.4.0 release is not |
I've pushed an experiment here to copy the Spring Test mock objects from 5.3 back across, move them to I couldn't find an alternate Mock implementation that is maintained and looks decent. :-( |
# Conflicts: # README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except one small package rename, this does look great 👍
do you want to merge into a branch or wait (keep the PR) till 1.3.0 is released on master?
@@ -279,7 +279,7 @@ def servlet_context | |||
@servlet_env.servlet_context # ServletRequest#getServletContext() | |||
else | |||
if @servlet_env.respond_to?(:context) && | |||
@servlet_env.context.is_a?(javax.servlet.ServletContext) | |||
@servlet_env.context.is_a?(jakarta.servlet.ServletContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't work, JRuby only provides some common names at top level such as java
, javax
, org
.
so you'll need to prefix e.g. Java::jakarta.servlet.ServletContext
.
p.s. Java::JakartaServlet::ServletContext
(above) is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting that no tests fail. Probably because above this line is
if @servlet_env.respond_to?(:servlet_context) # @since Servlet 3.0
@servlet_env.servlet_context # ServletRequest#getServletContext()
else
So this clause itself was a <= Servlet 2.5 backward compat. Maybe can be removed?
In an ideal world we'd be able to run tests across multiple servlet API versions to test compatibility more thoroughly...
In any case I should probably change all the references to use the same style, as there are too many combinations here.
# Conflicts: # README.md # src/main/java/org/jruby/rack/UnmappedRackFilter.java
# Conflicts: # src/spec/ruby/rack/handler/servlet_spec.rb
I suppose it doesn't matter too much which order we do things, but probably wait since we're probably not too far away on 1.3.0? For this PR, I'll do another sweep for unused mock code, fix the import ordering churn, and find dead servlet < 5.0 compat code (which definitely won't work after this PR since servlet 5.0 is obviously forward only due to the namespace change, whereas earlier 1.2.x and 1.3.x changes might still work with Servlet 2.5 since they didn't tend to remove anything from the impls.) |
Updated a few files with changes required for upgrade to Jakarta EE 9+. The jar is getting generated but test cases are not getting executed, will work on that.
Please let me know if there is something else that also needs to be checked. Also I did override a few required abstract method with empty/default value, would appreciate if you take a look at those.
Thanks.