Rack 3 support #277

kyleplump · 2024-12-31T15:22:25Z

multiple gems are impacted and updated by this change. this gem is the most significantly changed. i'll link to the open PR's of the affected gems below. please let me know if you'd like to see any changes, thanks!

highlights

changelog for Rack 3: https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md

response headers are now required to be lower case
rack.input is now no longer rewindable
- this impacts processing multiform data as well
some specs needed updating, on account that the content-length response header no longer a part of the implementation of Rack::Response. this change happened in v2.1, and while not directly related to Rack 3, was included here to get all green

impacted gems

cllns · 2024-12-31T17:22:06Z

Excellent! Thanks so much for doing this. It'll be a huge win when we can make a release that supports Rack 3.

Looks like there are some Rubocop errors, can you fix those?

I have a few questions (for you and others):

Do we want to require rack 3 or simply support it, if people want to upgrade themselves?
- If we require rack 3, then we should add support for people who have mixed-cased headers, so it's not a breaking change. It seems like Rack::Headers does this.
- If we don't require rack 3, then we might have to add conditionals based on rack version. Hopefully we can avoid this or at least minimize it.
Do we need our input to be rewindable with Rack::RewindableInput::Middleware at all? What if we just consume the headers / form data as part of our parsing, add it to the env/params, then never need to rewind? Is that an option? I guess it's more performant.
Similarly, for Content-Length, do we want add Rack::ContentLength middleware to all hanami-router apps, by default?
Along those lines, if we need rewindable input for normal non-streaming HTTP data, then can we let people opt out so they can use Rack 3's streaming support? This could definitely be a separate PR that comes later.

kyleplump · 2024-12-31T18:56:47Z

thanks for checking this out @cllns !

fixed the rubocop offenses

this is an excellent point. i feel as though the better DX would be to support and not require rack 3. the number of code changes isn't so significant that i think we can get away with only a few conditionals, though I'll defer to your judgement here
i'm not sure we need rewindable inputs. i think your suggestion of putting it into the env/params would certainly work (and already has some precedent, re: Router#_params). I kept the rewindable input just to match what was already there, but would be happy to do away with it if we want to avoid bringing in another middleware
i think having the Rack::ContentLength middleware is a great idea, and can add this in
another excellent point. I'd love to provide more 'rack 3+ specific' features, and will definitely start thinking about how to include that into the existing gem. including them here, or in a separate PR, i'll once again defer to your excellent judgement 😄

cllns · 2025-01-01T02:39:41Z

this is an excellent point. i feel as though the better DX would be to support and not require rack 3. the number of code changes isn't so significant that i think we can get away with only a few conditionals, though I'll defer to your judgement here

Sweet! If it's possible, I think that would be best, so we can keep this a minor version release.

We'll have to add rack-version to our CI matrix, but that's not a problem. It makes sense for a router.

Now that I think about it, even if Rack 3 is optional, we don't want users who are setting mixed-case headers to break their apps, so we should use Rack::Headers regardless.

i'm not sure we need rewindable inputs. i think your suggestion of putting it into the env/params would certainly work (and already has some precedent, re: Router#_params). I kept the rewindable input just to match what was already there, but would be happy to do away with it if we want to avoid bringing in another middleware

Outside of whether we need it, I feel like there might be reasons we want rewindable input... I guess it's more performant to keep the same input & rewind it instead of copying everything we need over. Plus users may be rewinding already (e.g. in middleware) and we don't want that to break.

Curious how others feel about this. I think it might make sense to use rewindable input by default for Rack 3 (maybe based on content type?) but hopefully there's a way to automatically allow non-rewindable inputs as well, for streams. If it can't be automatic then we should try to make it simple to opt-out of rewindable inputs.

I wish I knew more about Rack. I've read the UPGRADE_GUIDE, and started 'watching' the repo a couple weeks ago, so hopefully I can get a better grasp on it.

i think having the Rack::ContentLength middleware is a great idea, and can add this in

Looking into it, the Rack::ContentLength middleware only applies to responses that don't have the header already set. What if we just set it in hanami/hanami and leave it out of the router? Then we can document how users can add it. This keeps the router as small and flexible as possible. I think it's probably better to set it ourselves in the framework than rely on the middleware (which seems like more of a fallback).

another excellent point. I'd love to provide more 'rack 3+ specific' features, and will definitely start thinking about how to include that into the existing gem. including them here, or in a separate PR, i'll once again defer to your excellent judgement 😄

This is already a big change (across several repos, thank you!), so I think it'd be wise to keep the scope of this as small as we can. Then we can work on Rack 3+ features, and ship those independently.

So I think at this point:

Relax requirement to Rack > 2
Add testing Rack 2 and Rack 3 to CI matrix for this gem (and hanami/hanami?)
Make sure we support both, using RewindableInput
Un-do the changes for updating the specs to lowercase headers, to ensure we don't break people using mixed case headers.

Then we can get these merged, and work on non-rewindable inputs in separate PR's. We could ship rewindable-only Rack 3 support or wait until we actually support non-rewindable input to ship a new versions of the gems.

Curious to get @timriley's feedback :)

kyleplump · 2025-01-23T21:55:43Z

hi @cllns 👋 (sorry its been 10 years)

i just committed some changes. some thoughts:

relaxed the rack requirement
allowed handling of mixed case headers
injected content-length when necessary, not relying on the middleware

headers

this was the trickiest part for me, and would be very curious to hear your thoughts on it. i ran into a few different roadblocks, but think i have a solution:

all of the specs had hard coded, mixed case headers to assert against. in the previous iteration of this PR, i just lower cased them to match Rack3 spec. in this new iteration, I left the headers untouched but wrapped them in a new spec helper which either passes them through as is (for < rack 3), or relies on Rack::Headers for > rack 3.
http headers that the router automatically injects (e.g., sometimes location, sometimes allow), are both 'checking' rack version (via a check for Rack::Headers), and responding appropriately. Casing is incompatible between versions: Allow breaks Rack 3, but is needed for < Rack 3, etc.

these changes have achieved our own passing tests, as well as allowing the router to be able to respond correctly to multiple, technically incompatible, versions of Rack with just a few conditionals

content-length

I'm honestly slightly confused by what you mean here. i feel like automatically calculating and injecting the content-length header is the perfect work for the router? I added that behavior here, but would be happy to pull it out if you feel its better placed elsewhere.

i'd love to keep cracking at this and want to make sure its right, so let me know any changes you'd like to see. and thank you again!

timriley

Thank you so much for doing this, @kyleplump, and thank you @cllns for the wonderful feedback on this PR so far! 😍

I agree that we should be supporting both Rack 2 and 3 for the time being. Hanami 3 (whenever that comes) would be the right time to drop Rack 2 support.

If I might attempt summarise the changes here, adding Rack 3 compatibility boils down to:

Loosening the gem dependency (of course)
Deciding that we'll keep rewindable input objects as a first step of this transition, since that's what we've been expecting with Rack 2 to date
Updating our tests to expect the lower-cased headers that Rack 3 gives us

That's basically it, right?

If so, that's a really nice and simple adjustment. Easy to communicate, easy (hopefully) for others to understand. And it means the only thing we have to focus on for a full Rack 3 switch is a decision about rewindable input.

Love your work!

As for this PR, I've made a range of small suggestions via inline comments.

Biggest one is about moving the work of converting the input to rewindable up into Router#call, since I think that is an important behaviour, and keeping it buried in Router#_params makes it harder for us and our future contributors to find and understand.

Have a read of it all and please let know if there's anything else you'd like to discuss!

timriley · 2025-03-27T02:35:58Z

lib/hanami/middleware/body_parser.rb

@@ -39,10 +39,10 @@ def initialize(app, parsers)
      end

      def call(env)
-        body = env[RACK_INPUT].read
-        return @app.call(env) if body.empty?
+        body = env[RACK_INPUT]&.read


What required this &. change?

I'm sure you put it there for good reason, I just want to understand why. 😄 Especially because there's nothing in the rack 3 upgrade guide about env["rack.input"] potentially being nil.

its actually to fix a failing spec

i figure if its failing in our existing test suite, that its chance of occurring for others is non-negligible and would be an extra safe guard. happy to remove and rework the spec if you'd rather take that path!

lib/hanami/middleware/body_parser.rb

timriley · 2025-03-27T02:40:18Z

lib/hanami/middleware/body_parser/form_parser.rb

@@ -30,6 +30,7 @@ def self.mime_types
        # @since 2.0.1
        # @api private
        def parse(*, env)
+          env[::Rack::RACK_INPUT].rewind if env[::Rack::RACK_INPUT].respond_to?(:rewind)


Given the Rack 2 input was rewindable, I wonder why we weren't rewinding previously to this? Was it simply that no one actually read the stream before this middleware was hit, in reality?

I'm not sure - although what you're saying could very well be the case. Rack 3's more stringent requirements around this could have unearthed a future issue!

lib/hanami/router/rack_utils.rb

timriley · 2025-03-27T02:48:37Z

lib/hanami/router.rb


-      if !env.key?(ROUTER_PARSED_BODY) && (input = env[::Rack::RACK_INPUT]) and input.rewind
+      if !env.key?(ROUTER_PARSED_BODY) && input


There's a behavioural change here: we're no longer rewinding the input—see the and input.rewind in previous version of this line—before we parse the input in the line below.

Is there any reason we shouldn't be keeping that here to preserve our existing behaviour?

nope - this was a mistake and why PR's exist!

timriley · 2025-03-27T02:51:05Z

lib/hanami/router/block.rb

+          # explicitly set 'content-length' header
+          headers[::Rack::CONTENT_LENGTH] = body.size.to_s


The comment here is a but redundant. We can see from the line of code below that we're explicitly setting the header. What I'd like to see the comment explain is why we're doing it here. We want to make the why clear to the reader.

Speaking of which, why are we doing this here? 😅

After re-reading the conversation with @cllns so far, I think we've added this in because of this suggestion from Sean?

Similarly, for Content-Length, do we want add Rack::ContentLength middleware to all hanami-router apps, by default?

If we're wanting to keep this PR (and everything else to do with the Rack 3 compatibility changes) as minimal as possible, then I think it might be better to consider and make this change (or not) as a separate matter?

Sorry, I missed the detail about Content-Length from rack/rack#1472 (comment). Now I'm now caught up.

I think we should leave Content-Length out. People on the latest version of Rack 2 are already missing this header anyway.

If we wanted to restore it, I think we could consider adding the Rack::ContentLength middleware as part of the default middleware stack in Hanami apps.

People using the router directly can choose to do the same, too. I think it's better to keep the router as lean as practical.

My understanding is its actually the opposite: Rack 2 were the only people getting the header on the rack responses. The inclusion here was for green specs, but am super willing to either remove the explicit header checks from the specs, or go the middleware route

timriley · 2025-03-27T04:05:01Z

lib/hanami/router.rb

@@ -916,15 +922,18 @@ def _redirect(to, code)
    def _params(env, params)
      params ||= {}
      env[PARAMS] ||= {}
+      input = Rack::RewindableInput.new(env[::Rack::RACK_INPUT]) if env[::Rack::RACK_INPUT]


I wonder if we're "burying the lede" a little bit by doing this conversion of the input to a rewindable input down here in this _params.

The job of the _params method is to extract params, and by adding this line, I feel like we're asking it to do more than that, which isn't a good design change.

If we're deciding that we want to force a rack 3 input to be rewindable as the first part of our rack 3 transition, couldn't we do this conversion right up in Router#call?

Router#call is is the main entry point for the router, and as such, it feels like a more appropriate place to do something like this. It would make the rewindable conversion more prominent, and give us a clearer place to comment why we're doing it (this is important, IMO).

Can you please have a go at moving it there?

And as part of moving it there, I wonder if we actually want to wrap it in the rack_3? conditional, e.g.

# Explanatory comment goes here if rack_3? && (input = env[::Rack::RACK_INPUT]) env[::Rack::RACK_INPUT] = Rack::RewindableInput.new(input) end

Having this lead with the rack_3? conditional would also make it clearer that it's part of our Rack 3 transition strategy.

yes, agree with all these points! will make the update

spec/support/http.rb

spec/unit/hanami/router/redirect_spec.rb

Co-authored-by: Tim Riley <[email protected]>

kyleplump

thanks for the review @timriley! i at least responded to all your comments / committed the suggestions. I have a few slightly larger updates on my end and will ping you back!

kyleplump · 2025-03-28T20:52:14Z

lib/hanami/middleware/body_parser/form_parser.rb

@@ -30,6 +30,7 @@ def self.mime_types
        # @since 2.0.1
        # @api private
        def parse(*, env)
+          env[::Rack::RACK_INPUT].rewind if env[::Rack::RACK_INPUT].respond_to?(:rewind)


I'm not sure - although what you're saying could very well be the case. Rack 3's more stringent requirements around this could have unearthed a future issue!

kyleplump · 2025-03-28T20:55:41Z

lib/hanami/router.rb

@@ -916,15 +922,18 @@ def _redirect(to, code)
    def _params(env, params)
      params ||= {}
      env[PARAMS] ||= {}
+      input = Rack::RewindableInput.new(env[::Rack::RACK_INPUT]) if env[::Rack::RACK_INPUT]


yes, agree with all these points! will make the update

kyleplump · 2025-03-28T20:59:04Z

lib/hanami/router.rb


-      if !env.key?(ROUTER_PARSED_BODY) && (input = env[::Rack::RACK_INPUT]) and input.rewind
+      if !env.key?(ROUTER_PARSED_BODY) && input


nope - this was a mistake and why PR's exist!

kyleplump · 2025-03-28T21:02:43Z

lib/hanami/router/block.rb

+          # explicitly set 'content-length' header
+          headers[::Rack::CONTENT_LENGTH] = body.size.to_s


My understanding is its actually the opposite: Rack 2 were the only people getting the header on the rack responses. The inclusion here was for green specs, but am super willing to either remove the explicit header checks from the specs, or go the middleware route

spec/support/http.rb

spec/unit/hanami/router/redirect_spec.rb

kyleplump · 2025-04-10T14:07:16Z

@timriley pinging you back. made a bunch of requested updates, and left a few comments 🙂

please let me know what you think. thanks!

Kyle Plump and others added 6 commits December 16, 2024 17:15

bump rack

dd92970

.1

e163e78

rack-test bump to 2

4ae223a

update support for rack 3

ff4936a

resolved all non-back compat changes

7cb1e54

passing tests

becab3e

This was referenced Dec 31, 2024

rack 3 support hanami/hanami#1493

Open

adds rack 3 support hanami/cli#283

Closed

rack 3 support hanami/controller#460

Open

rack 3 support hanami/devtools#90

Open

rubocop

da9f8c9

kyleplump and others added 4 commits January 22, 2025 20:41

wip

f407b21

green tests for both rack 2 and 3

53b3da8

content-length

fafdf0d

rm .ruby-version

135b50e

Kyle Plump and others added 2 commits January 24, 2025 13:43

rack_utils

5409f28

reset scripts, rubocop

e313e78

kyleplump mentioned this pull request Feb 20, 2025

Add support for Rack 3 hanami/cli#289

Open

timriley reviewed Mar 27, 2025

View reviewed changes

kyleplump and others added 4 commits March 28, 2025 16:44

Update lib/hanami/middleware/body_parser.rb

6507dad

Co-authored-by: Tim Riley <[email protected]>

Update lib/hanami/router/rack_utils.rb

625dc4c

Co-authored-by: Tim Riley <[email protected]>

Update spec/support/http.rb

0a291c3

Co-authored-by: Tim Riley <[email protected]>

Update spec/unit/hanami/router/redirect_spec.rb

399c1dc

Co-authored-by: Tim Riley <[email protected]>

kyleplump commented Mar 28, 2025

View reviewed changes

kyleplump added 2 commits April 10, 2025 09:36

requested changes

1caf601

rubocop

6a84888

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rack 3 support #277

Rack 3 support #277

kyleplump commented Dec 31, 2024 •

edited

Loading

cllns commented Dec 31, 2024

kyleplump commented Dec 31, 2024

cllns commented Jan 1, 2025

kyleplump commented Jan 23, 2025

timriley left a comment •

edited

Loading

timriley Mar 27, 2025

kyleplump Mar 28, 2025

timriley Mar 27, 2025

kyleplump Mar 28, 2025

timriley Mar 27, 2025

kyleplump Mar 28, 2025

timriley Mar 27, 2025

timriley Mar 27, 2025

timriley Mar 27, 2025

kyleplump Mar 28, 2025

timriley Mar 27, 2025

kyleplump Mar 28, 2025

kyleplump left a comment

kyleplump Mar 28, 2025

kyleplump Mar 28, 2025

kyleplump Mar 28, 2025

kyleplump Mar 28, 2025

kyleplump commented Apr 10, 2025


		if !env.key?(ROUTER_PARSED_BODY) && (input = env[::Rack::RACK_INPUT]) and input.rewind
		if !env.key?(ROUTER_PARSED_BODY) && input

		# explicitly set 'content-length' header
		headers[::Rack::CONTENT_LENGTH] = body.size.to_s

Rack 3 support #277

Are you sure you want to change the base?

Rack 3 support #277

Conversation

kyleplump commented Dec 31, 2024 • edited Loading

highlights

impacted gems

cllns commented Dec 31, 2024

kyleplump commented Dec 31, 2024

cllns commented Jan 1, 2025

kyleplump commented Jan 23, 2025

headers

content-length

timriley left a comment • edited Loading

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

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

kyleplump left a 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

kyleplump commented Apr 10, 2025

kyleplump commented Dec 31, 2024 •

edited

Loading

timriley left a comment •

edited

Loading