Set response headers based on Rack version #2355

schinery · 2023-10-16T07:52:01Z

#2354

dblock · 2023-10-16T13:13:38Z

I like this better than #2356. ~~Same question as in that issue, let's dig up why this was changed.~~

rack/rack#1592 is clear that this is changing to lowercase in Rack 3.

One option would be to major-version increment Grape as part of this PR and do the same as in Rack.

schinery · 2023-10-16T13:51:47Z

One option would be to major-version increment Grape as part of this PR and do the same as in Rack

Do you need something adding to this PR for this?

I also pushed fixing the integration spec paths and updated changelog.

dblock · 2023-10-16T20:40:57Z

Let's see if other maintainers have an opinion. If we major increment the version here we can make the backwards incompatible change. I am 50/50 on what's best.

dblock · 2023-10-16T20:42:06Z

@ioquatix would like your eyes and opinion here if you have a moment?

dblock · 2023-10-16T20:42:49Z

@schinery looks like we may need to do extra work to support Rack 1.x.

schinery · 2023-10-17T08:49:29Z

@schinery looks like we may need to do extra work to support Rack 1.x.

Ah, seems that Rack::RELEASE was added after 1.x., but there is Rack.release in all 3 versions (1.x., 2.x. and 3.x.) so switched to use that for the version check.

dblock · 2023-10-17T18:30:29Z

I will merge this unless I hear from other maintainers not to.

@schinery Let's increment the version to 1.9 as part of this PR since at the very least it's a big change, and add a section to UPGRADING?

schinery · 2023-10-18T08:08:52Z

@schinery Let's increment the version to 1.9 as part of this PR since at the very least it's a big change, and add a section to UPGRADING?

@dblock like this? cd71b14

CHANGELOG.md

README.md

UPGRADING.md

schinery · 2023-10-19T22:00:46Z

@dblock hopefully done all the PR feedback correctly, with the exception of "I think we need more text explaining what happens in what code. Give an example of an API that sets one of these headers, or explain when they are set?" as not 100% sure what you want for this.

Do you have an example in mind?

schinery · 2023-10-20T06:04:03Z

Also, just going through the README, and wondering if the following needs updating:

There is also https://github.com/ruby-grape/grape#header-case-handling, which this PR hasn’t altered as it has only changed the two hardcoded headers to fix error cascading. Should this be handled in separate PR as this is what #2254 is on about?

schinery · 2023-10-21T15:00:59Z

@dblock just trying to get a sense of the other changes needed to resolve #2254 to see if we do it in this PR, so far I have:

1: Update #transform_header in https://github.com/ruby-grape/grape/blob/master/lib/grape/request.rb#L49 to be Rack specific, so capitalize for Rack < 2, lowercase for Rack >= 3. Think this is fairly straightforward as long as we're ok to use the same Rack check used in the headers class?

2: Fix the specs, like https://github.com/ruby-grape/grape/blob/master/spec/grape/request_spec.rb#L121, so they are Rack version specific. Am guessing this would mean that they would go in the Rack integration specs like we've added in this PR?

3: Update the README, such as https://github.com/ruby-grape/grape#header-case-handling, so that it has changes for both header casings.

4: Don't think we need to do anything for the #header method defined in https://github.com/ruby-grape/grape/blob/master/lib/grape/dsl/headers.rb#L11, as this is down to whatever people use for the header key.

Not sure what else, partly as this is my first time truly digging around the code. Anything else you think is needed?

schinery · 2023-10-21T15:06:32Z

Does also make me wonder... would it be an idea to leave version 1 of Grape (1.x branch off of master) supporting Rack < 3 and create a version 2 of Grape (current master) that supports Rack >= 3.

There would be more overhead for maintaining etc depending how long you want to maintain version 1, but the code would be cleaner without the Rack version detection code in it.

Happy to continue with #2355 though, let me know your thoughts.

schinery · 2023-10-21T16:33:00Z

@dblock more for #2355 (comment)

I threw this PR together quick just to look at what might need changing (minus the README changes), especially which specs would need updating. There were 5 specs that need the header in the right case based on what Rack version is used in the test run.

dblock · 2023-10-21T21:25:14Z

Does also make me wonder... would it be an idea to leave version 1 of Grape (1.x branch off of master) supporting Rack < 3 and create a version 2 of Grape (current master) that supports Rack >= 3.

I would be totally fine with incrementing major to 2.0 as part of this PR, and lock it at Rack >= 3, instead of trying to do both if you prefer?

I don't love the idea of also maintaining a 1.x. Are you interested in doing so? It's just a 1.x branch and releasing.

schinery · 2023-10-22T07:00:56Z

I would be totally fine with incrementing major to 2.0 as part of this PR, and lock it at Rack >= 3, instead of trying to do both if you prefer?

It just feels a bit cleaner. As you saw in schinery#1 (which again was more to highlight than a solution) some of the issues are in the testing, plus I guess you also have to have update the docs to support both Rack versions.

I don't love the idea of also maintaining a 1.x. Are you interested in doing so? It's just a 1.x branch and releasing.

Not really interested either, but I guess then you have something if you need to patch 1.x. in the future, and I guess then you could then just release a 1.8.1 or 1.9.0 release with an updated README etc, with any new features etc going in the 2.x version.

If it is decided that this is approach to take then is this what needs to happen?

Update this PR to:
- Bump Grape to 2.0.0
- Lock Rack to >= 3
- Update the transform_headers to downcase only and update the specs where needed
- Update the README, CHANGELOG and UPGRADING docs
You'd create a 1.x branch and then we'd need a PR for that branch that:
- Bumped Grape to 1.8.1 or 1.9.0
- Lock Rack < 3
- Update the test matrix to not include rack_3_0
- Update the README, CHANGELOG and UPGRADING docs

Does it feel like we need a second opinion on this too?

schinery · 2023-10-22T17:31:37Z

One thing I was wrong about... it wasn't Rails 7.1 that bumped Rack to 3, as the deps in the Gemfile.lock are as follows:

    actionpack (7.1.1)
      actionview (= 7.1.1)
      activesupport (= 7.1.1)
      nokogiri (>= 1.8.5)
      rack (>= 2.2.4)
      rack-session (>= 1.0.1)
      rack-test (>= 0.6.3)
      rails-dom-testing (~> 2.2)
      rails-html-sanitizer (~> 1.6)

However, I've just been playing about with locking to Rack 3 in a branch and I get this error using rails_7_0.gemfile.

And because actionpack >= 7.0.5, < 7.1.0 depends on rack >= 2.2.4, < 3.A
  and every version of grape depends on rack >= 3.0.0,
  every version of grape is incompatible with rails >= 7.0.0, < 7.1.0.beta1.
So, because rails_7_0.gemfile depends on rails ~> 7.0.0
  and rails_7_0.gemfile depends on grape >= 0,
  version solving has failed.

So, if Grape was locked to Rack 3 it looks like potentially that version might only work with Rails 7.1.

Which means I'm now flip flopping between whether Rack should be locked in Grape or not. Suspect not...

schinery · 2023-10-22T17:35:22Z

I've updated schinery#1 with the additional changes I think are needed to support both Rack versions in Grape. I haven't addressed your additional comments in that PR, partly as my brain is tied in knots.

dblock · 2023-10-23T14:30:42Z

I feel like my brain is also tied in knots over this, so let's try to merge one thing! Let's make the changes as you proposed originally, support both Rack 2.x and 3.x where in Rack 3.x all headers are lowercase. Take your changes in schinery#1 into this PR.

Can the code be simplified by making headers a case-insensitive structure (hash with invariant access) that lets you access :x_foo_bar, x-foo-bar and X-Foo-Bar alike in Rack 3? This would make it backwards compatible for most applications and comply with the new spec.

schinery · 2023-10-23T15:18:05Z

Merged schinery#1 in.

Will ponder the "a case-insensitive structure" for the request headers, would be ace if there was a way to do it but there is nothing screaming out at me at the moment.

dblock · 2023-10-23T20:23:16Z

Will ponder the "a case-insensitive structure" for the request headers, would be ace if there was a way to do it but there is nothing screaming out at me at the moment.

In https://github.com/schinery/grape/blob/51b081cef9d4d91cb75f80dd33222805612c7b95/lib/grape/request.rb#L38

This builds a Hash. You could try with ActiveSupport::HashWithIndifferentAccess, I don't remember if it's also case-insensitive.

dblock

Getting there ...

UPGRADING.md

dblock · 2023-10-23T20:27:46Z

lib/grape/request.rb

@@ -47,7 +47,11 @@ def build_headers
    end

    def transform_header(header)
-      -header[5..].split('_').each(&:capitalize!).join('-')
+      if Gem::Version.new(Rack.release) < Gem::Version.new('3')


This executes in a tight loop, which is bad for performance. You want this check outside. We also have it in a few places. It should look like this:

if Grape::Rack.rack3? def transform_header(header) -header[5..].split('_').map(&:capitalize).join('-') end else def transform_header(header) -header[5..].tr('_', '-').downcase end end

Added .rack3? method and done this change, although I've added it to Grape rather than Grape::Rack otherwise there was a load of Rack errors raised because we would need to change all the Rack references to ::Rack.

Can do this though if really needs to be Grape::Rack

dblock · 2023-10-23T20:29:27Z

spec/grape/api_spec.rb

@@ -689,7 +689,7 @@ class DummyFormatClass
        'example'
      end
      put '/example'
-      expect(last_response.headers['Content-Type']).to eql 'text/plain'
+      expect(last_response.headers[rack_versioned_headers[:content_type]]).to eql 'text/plain'


Can we get rid of rack_versioned_headers and instead do Grape::HTTP::Headers::CONTENT_TYPE?

dblock · 2023-10-23T20:30:03Z

spec/support/headers_helpers.rb

+module Spec
+  module Support
+    module Helpers
+      def rack_versioned_headers


Get rid of this class, use Grape::HTTP::Headers::CACHE_CONTROL and friends.

The only issue with this is what to do in the specs that are testing with non Grape::HTTP::Headers headers, such as Grape-Likes-Symbolic or X-Access-Token, as these need to be Rack versioned in the specs.

I've removed the headers helper though and put these inline, so they are defined in whatever specs needed them

schinery · 2023-10-24T05:45:10Z

Will ponder the "a case-insensitive structure" for the request headers, would be ace if there was a way to do it but there is nothing screaming out at me at the moment.

In https://github.com/schinery/grape/blob/51b081cef9d4d91cb75f80dd33222805612c7b95/lib/grape/request.rb#L38

This builds a Hash. You could try with ActiveSupport::HashWithIndifferentAccess, I don't remember if it's also case-insensitive.

Yeah, it doesn't...

irb(main):001:0> hash = ActiveSupport::HashWithIndifferentAccess.new(a: 1)
=> {"a"=>1}
irb(main):002:0> hash["A"]
=> nil
irb(main):003:0>

Whether something like this would work. I'm not sure what code changes would be required though.

dblock

I'm going to merge and let it sit.

dblock · 2023-10-24T13:19:01Z

spec/grape/api/custom_validations_spec.rb

          # check if user is admin or not
          # as an example get a token from request and check if it's admin or not
-          raise Grape::Exceptions::Validation.new(params: @attrs, message: 'Can not set Admin only field.') unless request.headers['X-Access-Token'] == 'admin'
+          raise Grape::Exceptions::Validation.new(params: @attrs, message: 'Can not set Admin only field.') unless request.headers[access_header] == 'admin'


Could use x_access_token_header here.

Yeah, tried that, but got...

NameError: undefined local variable or method `x_access_token_header' for #<#<Class:0x0000ffff8f8f9b18>:0x0000ffff928d97e8 @attrs=[:admin_field], @option=true, @required=false, @scope=#<Grape::Validations::ParamsScope:0x0000ffff8f8f96b8 @element=nil, @element_renamed=nil, @parent=nil, @api=#<Class:0x0000ffff8f8f9898>, @optional=false, @type=Hash, @group=nil, @dependent_on=nil, @declared_params=nil, @index=nil>, @fail_fast=false, @allow_blank=nil>

which was annoying, as don't like the defining it twice either.

ioquatix

This generally looks okay to me.

The biggest issue is, people trying to support both Rack 2 and Rack 3 will need to be careful about the assertions. Direct assertions against the hash can be problematic. It's important to have a proper normalization step in assertions IMHO, as the expectation should be "A header named X has a value Y", but the case of X can depend on the HTTP version, server, etc. In Rails, there were many instances of header key and value case which had ossified to whatever Rack (the gem) was generating, even thought that was only a subset of valid possible results.

I know this was a pretty annoying change in some regards, but overall it will simplify things and aligns the code with the HTTP specification more closely in a way which makes it easier to know that code is doing the right thing. e.g. rack/rack-test#229

ioquatix · 2023-10-24T20:44:50Z

UPGRADING.md

+
+#### Headers
+
+As per [rack/rack#1592](https://github.com/rack/rack/issues/1592) Rack 3.0 is enforcing the HTTP/2 semantics, and thus treats all headers as lowercase. Starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.


Rack 3.0 is enforcing the HTTP/2 semantics, and thus treats all headers as lowercase. Starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.

Rack 3 is following the HTTP/2+ semantics which require header names to be lower case. To avoid compatibility issues, starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.

ioquatix · 2023-10-24T20:47:50Z

lib/grape.rb

@@ -42,6 +42,10 @@ def self.deprecator
    @deprecator ||= ActiveSupport::Deprecation.new('2.0', 'Grape')
  end

+  def self.rack3?
+    Gem::Version.new(::Rack.release) >= Gem::Version.new('3')


If you are just trying to detect whether to use lower case headers or not, I suggest a slightly different approach:

LOWER_CASE_HEADERS = Rack::CONTENT_TYPE == "content-type"

You could then use that directly or in a method.

ioquatix · 2023-10-24T20:48:12Z

lib/grape/dsl/inside_route.rb

-        header 'Content-Length', nil
-        header 'Transfer-Encoding', nil
-        header 'Cache-Control', 'no-cache' # Skips ETag generation (reading the response up front)
+        header Grape::Http::Headers::CONTENT_LENGTH, nil


You can use Rack::CONTENT_LENGTH if that suits your use case.

dblock · 2023-10-24T21:01:26Z

Thanks @ioquatix ! @schinery will you extract the comments into another PR please?

schinery · 2023-10-24T21:30:15Z

Additional comments in #2362

schinery force-pushed the detect-rack-version-for-headers branch 2 times, most recently from cd40b2f to 138a771 Compare October 16, 2023 08:57

schinery mentioned this pull request Oct 16, 2023

X-Cascade header Rack 3 issue #2354

Closed

schinery force-pushed the detect-rack-version-for-headers branch 2 times, most recently from 4511cf6 to 97c2348 Compare October 16, 2023 13:32

schinery changed the title ~~Set headers based on Rack version~~ Set response headers based on Rack version Oct 16, 2023

Set response headers based on Rack version

608bd18

schinery force-pushed the detect-rack-version-for-headers branch from 97c2348 to 608bd18 Compare October 16, 2023 13:36

schinery mentioned this pull request Oct 16, 2023

Lowercase content-type and x-cascade headers #2356

Closed

schinery marked this pull request as ready for review October 16, 2023 13:47

schinery force-pushed the detect-rack-version-for-headers branch from 8c10933 to 72ec447 Compare October 18, 2023 08:11

schinery added 2 commits October 18, 2023 09:14

Use Rack.release instead of Rack::RELEASE

1b6b7a4

Bumped to 1.9.0 and updated UPGRADING.md

cd71b14

schinery force-pushed the detect-rack-version-for-headers branch from 72ec447 to cd71b14 Compare October 18, 2023 08:14

dblock reviewed Oct 19, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

dblock reviewed Oct 19, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

dblock reviewed Oct 19, 2023

View reviewed changes

UPGRADING.md Show resolved Hide resolved

dblock reviewed Oct 19, 2023

View reviewed changes

UPGRADING.md Outdated Show resolved Hide resolved

PR Feedback

6bfc616

schinery requested a review from dblock October 19, 2023 22:01

Merge branch 'master' into detect-rack-version-for-headers

ff4c6b5

schinery and others added 3 commits October 23, 2023 16:06

Other changes needed to support both Rack 2 and 3 (#1)

5ecb7c7

Updated custom validations spec

32ca429

Added other headers to integration specs

2d87076

dblock requested changes Oct 23, 2023

View reviewed changes

Added .rack3? method

0a88e94

schinery force-pushed the detect-rack-version-for-headers branch from 820e1ab to 0a88e94 Compare October 24, 2023 04:10

schinery added 3 commits October 24, 2023 05:36

Removed headers_helper

7b51e0f

Fixed alignment

d0850d5

Updated README and UPGRADING

ba8c0e3

schinery requested a review from dblock October 24, 2023 05:46

dblock approved these changes Oct 24, 2023

View reviewed changes

dblock mentioned this pull request Oct 24, 2023

Make response headers a hash and enforce lower-case keys. rack/rack#1592

Closed

dblock merged commit 2f62365 into ruby-grape:master Oct 24, 2023
31 checks passed

schinery deleted the detect-rack-version-for-headers branch October 24, 2023 15:15

ioquatix reviewed Oct 24, 2023

View reviewed changes

schinery mentioned this pull request Oct 24, 2023

Additional changes for setting Rack versioned headers #2362

Merged


		#### Headers

		As per [rack/rack#1592](https://github.com/rack/rack/issues/1592) Rack 3.0 is enforcing the HTTP/2 semantics, and thus treats all headers as lowercase. Starting with Grape 1.9.0, headers will be cased based on what version of Rack you are using.

Set response headers based on Rack version #2355

Set response headers based on Rack version #2355

Conversation

schinery commented Oct 16, 2023 • edited by dblock Loading

dblock commented Oct 16, 2023 • edited Loading

schinery commented Oct 16, 2023

dblock commented Oct 16, 2023

dblock commented Oct 16, 2023

dblock commented Oct 16, 2023

schinery commented Oct 17, 2023

dblock commented Oct 17, 2023 • edited Loading

schinery commented Oct 18, 2023 • edited Loading

schinery commented Oct 19, 2023

schinery commented Oct 20, 2023 • edited Loading

schinery commented Oct 21, 2023 • edited Loading

schinery commented Oct 21, 2023 • edited Loading

schinery commented Oct 21, 2023

dblock commented Oct 21, 2023

schinery commented Oct 22, 2023 • edited Loading

schinery commented Oct 22, 2023

schinery commented Oct 22, 2023

dblock commented Oct 23, 2023

schinery commented Oct 23, 2023

dblock commented Oct 23, 2023

dblock 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

Choose a reason for hiding this comment

schinery commented Oct 24, 2023

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioquatix 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

dblock commented Oct 24, 2023

schinery commented Oct 24, 2023 • edited Loading

schinery commented Oct 16, 2023 •

edited by dblock

Loading

dblock commented Oct 16, 2023 •

edited

Loading

dblock commented Oct 17, 2023 •

edited

Loading

schinery commented Oct 18, 2023 •

edited

Loading

schinery commented Oct 20, 2023 •

edited

Loading

schinery commented Oct 21, 2023 •

edited

Loading

schinery commented Oct 21, 2023 •

edited

Loading

schinery commented Oct 22, 2023 •

edited

Loading

schinery commented Oct 24, 2023 •

edited

Loading