Fix compatibility with Rack 2.x on Rails 7.0+, reinstating Rails appraisal tests #271

chadlwilson · 2025-01-22T08:38:15Z

This PR makes a few important changes.

Requires Rack 2.x (as bundled), removing old Rack 1.x compatibility code which is very confusing to support (and likely doesn't work)
- this makes no practical difference for rails since rack 2.0 is required from rails 5 anyway.
- seems fine to me more widely given rack 1.6 was EOL 5 years ago.
fixes Session store @env is nil with Rails 7 #244 - using a Java servlet session store under Rails 7.0 was broken
- Rails 7 accidentally broke backward compat with older store implementations when using flash middleware by needing an enabled? function. https://github.com/rails/rails/blob/d4acb7aed7c48c3a884cb0ee52f6de6aa3607f3c/actionpack/lib/action_dispatch/middleware/flash.rb#L71-L72
- Rack 2.x also made changes to a number of functions jruby rack overrides (nesting @env within @req for sessions and changing what is passed to functions). rack/rack@c94e224
- The second problem obscured the first problem by breaking the missing method delegation to the Java servlet session.
Makes a few minor test fixes to the Rails "appraisal" tests to allow them to run with modern JRuby and modern Rails
- Works around an issue with concurrent-ruby and Rails < 7.1 for tests
- Corrects visibility on some mock methods
- Adds workaround for frozen string literal issues with NameError handling on older JRuby/Rails versions.
Adds the appraisals to GitHub Actions as a second job, including Rails 5.0 -> 7.2 for now.
Updates README and History.md to reflect 1.2.x releases (assumes this will be in 1.2.3)
- fixes [to-do] towards **1.2** #168 in a sense

Commits should be logically chunked so can be reviewed independently if necessary.

This isn't as scary to review as it looks - almost all of the change chunk #s are from removing and adding appraisal Gemfile.locks 😛

@Req

Rack 2.x wraps the @env within @Req so previous overrides were not working correctly in all cases.

…Rails 5.2/6.0

Only affects appraisals/rails tests right now it seems where the MockHttpSession is used.

… Rails versions

chadlwilson · 2025-01-22T14:22:27Z

Would appreciate review/input from @kares and @headius when you folks have time :-)

chadlwilson · 2025-01-22T14:25:41Z

Appraisals

+appraise "rails50" do
+  gem "rails", "~> 5.0.0"
 end

-appraise "rails40" do
-  gem "rails", "~> 4.0.13"
+appraise "rails52" do
+  gem "rails", "~> 5.2.0"
 end

-appraise "rails41" do
-  gem "rails", "~> 4.1.16"
+appraise "rails60" do
+  gem "rails", "~> 6.0.0"
 end


We could consider dropping tests for Rails 5.0 (EOL Apr 2018), 5.2 (EOL Jun 2022) and/or 6.0 (EOL Jun 2023) as well. I kept all versions that worked with JRuby 9.3+.

One reason to be a bit more relaxed with EOLs for JRuby stuff is some of the lag for JRuby itself to catch up with required Ruby versions. Dropping 5.0 is probably pretty reasonable now though.

src/main/ruby/jruby/rack/helpers.rb

chadlwilson · 2025-01-22T14:30:56Z

src/main/ruby/jruby/rack/session_store.rb

-        def current_session_id(env)
-          env[ENV_SESSION_OPTIONS_KEY][:id] # 1.5.0: env[ENV_SESSION_KEY].id
-        end if ::JRuby::Rack::Session::OptionsHash


OptionsHash is gone from Rack 1.5+ so no longer needed.

chadlwilson · 2025-01-22T14:32:47Z

src/spec/java/org/jruby/rack/mock/MockHttpSession.java

@@ -101,7 +101,7 @@ public String getId() {
 		return this.id;
 	}

-    void setId(String id) {
+    public void setId(String id) {


Not visible on modern JRuby otherwise.

chadlwilson · 2025-01-22T14:32:51Z

src/spec/java/org/jruby/rack/mock/MockHttpSession.java

@@ -256,7 +256,7 @@ public String toString() {
        return getClass().getName() + "@" + Integer.toHexString(hashCode()) + this.attributes;
    }

-    Map<String, Object> getAttributes() {
+    public Map<String, Object> getAttributes() {


Not visible on modern JRuby otherwise.

chadlwilson · 2025-01-22T14:37:30Z

src/spec/ruby/action_controller/session/java_servlet_store_spec.rb

-  it "should raise an error if the servlet request is not present" do
-    expect { @session_store.call({}) }.to raise_error(RuntimeError)
-  end
-


This test was too problematic to get to work reliably across all versions, and seemed to be relying on a lot of other code to get to the expected result from the entry point. Seems to be trying to get into the code below when there is no existing session. I made a call that I didn't think it was sufficiently important after trying to fix it and getting lost as to why it wasn't always raising an error.

jruby-rack/src/main/ruby/jruby/rack/session_store.rb

Lines 49 to 57 in 4fa7813

def get_servlet_session(env, create = false)

servlet_session = env[ENV_SERVLET_SESSION_KEY]

invalid = false

begin

if servlet_session.nil? ||

( create && ( invalid || servlet_session.getCreationTime.nil? ) )

unless servlet_request = env['java.servlet_request']

raise "JavaServletStore expects a servlet request at env['java.servlet_request']"

end

We are only supporting Rack 2.x onwards now anyway.

…fore railtie Not sure if this is us, strictly speaking but we seem to need to require active_support before the railties or the issue at rails/rails#49495 (comment) occurs

src/main/java/org/jruby/rack/servlet/ResponseCapture.java

chadlwilson added 5 commits January 22, 2025 13:38

Support only Rack 2.x with Rails, removing old 1.x rack/rails compat

48624ff

Rack 2.x wraps the @env within @Req so previous overrides were not working correctly in all cases.

Workaround NameError frozen string literal issues with JRuby 9.3 and …

d35b122

…Rails 5.2/6.0

Correct mock method visibility for newer JRuby tests

be1f629

Only affects appraisals/rails tests right now it seems where the MockHttpSession is used.

Workaround logger require issues with concurrent-ruby 1.3.5 and older…

78bc656

… Rails versions

Remove some deprecated rspec usage

e34b0f6

chadlwilson mentioned this pull request Jan 22, 2025

Merge 1.1 into master (redux - with all specs passing + bumped dependency minor versions) #262

Merged

chadlwilson force-pushed the appraisals-tests branch 2 times, most recently from 281b664 to 4fa7813 Compare January 22, 2025 11:19

chadlwilson mentioned this pull request Jan 22, 2025

Include logger silence from active support #270

Open

chadlwilson commented Jan 22, 2025

View reviewed changes

src/main/ruby/jruby/rack/helpers.rb Show resolved Hide resolved

chadlwilson commented Jan 22, 2025

View reviewed changes

chadlwilson force-pushed the appraisals-tests branch 3 times, most recently from 277cbb4 to 3eeb902 Compare January 23, 2025 03:44

chadlwilson added 3 commits January 23, 2025 11:58

Re-enable Rails appraisals to run on modern Rails versions

c27409f

Remove old Rack 1.x / Rails < 3 compatibility code

6bab581

We are only supporting Rack 2.x onwards now anyway.

Update README / history to reflect 1.2.x releases.

f52da2e

chadlwilson force-pushed the appraisals-tests branch from 3eeb902 to f52da2e Compare January 23, 2025 03:58

Fix Rails 7.1 compatibility by ensuring active_support is required be…

4dcb31c

…fore railtie Not sure if this is us, strictly speaking but we seem to need to require active_support before the railties or the issue at rails/rails#49495 (comment) occurs

chadlwilson force-pushed the appraisals-tests branch from 638c232 to 4dcb31c Compare January 23, 2025 07:02

kares reviewed Jan 24, 2025

View reviewed changes

src/main/java/org/jruby/rack/servlet/ResponseCapture.java Show resolved Hide resolved

kares approved these changes Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with Rack 2.x on Rails 7.0+, reinstating Rails appraisal tests #271

Fix compatibility with Rack 2.x on Rails 7.0+, reinstating Rails appraisal tests #271

chadlwilson commented Jan 22, 2025 •

edited

Loading

chadlwilson commented Jan 22, 2025

chadlwilson Jan 22, 2025

chadlwilson Jan 22, 2025

chadlwilson Jan 22, 2025

chadlwilson Jan 22, 2025

chadlwilson Jan 22, 2025

	def get_servlet_session(env, create = false)
	servlet_session = env[ENV_SERVLET_SESSION_KEY]
	invalid = false
	begin
	if servlet_session.nil? \|\|
	( create && ( invalid \|\| servlet_session.getCreationTime.nil? ) )
	unless servlet_request = env['java.servlet_request']
	raise "JavaServletStore expects a servlet request at env['java.servlet_request']"
	end

Fix compatibility with Rack 2.x on Rails 7.0+, reinstating Rails appraisal tests #271

Are you sure you want to change the base?

Fix compatibility with Rack 2.x on Rails 7.0+, reinstating Rails appraisal tests #271

Conversation

chadlwilson commented Jan 22, 2025 • edited Loading

chadlwilson commented Jan 22, 2025

chadlwilson Jan 22, 2025

Choose a reason for hiding this comment

chadlwilson Jan 22, 2025

Choose a reason for hiding this comment

chadlwilson Jan 22, 2025

Choose a reason for hiding this comment

chadlwilson Jan 22, 2025

Choose a reason for hiding this comment

chadlwilson Jan 22, 2025

Choose a reason for hiding this comment

chadlwilson commented Jan 22, 2025 •

edited

Loading