Rack 3 support

[kyleplump]

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

• hanami
• cli
• controller
• devtools

[cllns]

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]

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]

• 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]

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!

[kyleplump]

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

please let me know what you think. thanks!

[timriley]

Alright! I think we're finally good to go here. Thanks for your patience while I found the time to come back to this.

Everything from our previous conversations on this PR has been nicely resolved now.

Today I've made one final change, which is to allow the version of Rack to determine whether or not a Content-Length header is put on the responses. Content-Length being included by default in Rack 2 responses was a side effect of an implementation detail that has since been removed with Rack 3.

The point of Hanami Router is to provide routing capabilities on top of Rack, nothing more. I think that a router forcing a Content-Length header is overstepping the mark, and it isn't so strictly needed as the other compatibility shim we added, the rewindable input, which needs to be in place because of our params handling.

I'm actually pretty sure that Content-Length will still end up appearing in responses delivered by full Hanami apps due to other parts of our stack, which I'll test after all of this is merged.

[wuarmin]

Thank you for this great work here! Love it 😍

[cramt]

🙌🙌🙌🙌🙌

[Andsbf]

@timriley , will a tag 🏷️ be released with this change?

[timriley]

@Andsbf Yes, we'll release this as soon as we can. Rack 3 support will be the main feature of an upcoming Hanami 2.3 release. It's not quite ready yet, there are still some incompatibilities I need to iron out on the main branch of hanami-controller.