From 7deac93c6113d946022bed3c96726b4e35c4330c Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 6 Sep 2026 13:52:39 +0200 Subject: [PATCH] Copy response headers with merge! instead of merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Grape::Util::Header.new.merge(headers)` appears twice on a request path, and `merge` is a `dup` plus a `merge!` — so the Header built on the same line is allocated only to be thrown away. `merge!` fills the one already in hand and returns it, halving the cost: * `Grape::API::Instance#call`, on every request when `cascade false` is set — the usual configuration for Grape mounted inside Rails. * `Grape::Middleware::Error#rack_response`, on every error response. Both copies stay. `headers` can come from a mounted Rack app, which is free to hand back a frozen or shared Hash, and both call sites go on to write into what they are given. The `cascade false` path is now pinned by a spec that mounts a Rack app returning a frozen Hash; without the copy it raises `FrozenError`. `merge!` behaves the same on both halves of the supported Rack range: Rack 2.2's `HeaderHash#merge!` is `other.each { |k, v| self[k] = v }`, and Rack 3 aliases `merge!` to `update`. Both route through the overridden `[]=`, so header names are still downcased. Note that the shorter-looking `Grape::Util::Header.new(headers)` is not equivalent: `Rack::Headers` subclasses `Hash`, so on Rack 3 the argument becomes the default value and the result is empty. Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 1 + lib/grape/api/instance.rb | 7 ++++++- lib/grape/middleware/error.rb | 2 +- spec/grape/api/instance_spec.rb | 25 +++++++++++++++++++++++++ 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2d9c2951..89766e950 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,7 @@ * [#2905](https://github.com/ruby-grape/grape/pull/2905): Nest any `route_param` requirement under the param name, not just a Regexp, and reject the `requirements` shapes that have no param to attach to where they are written rather than on the first request - [@ericproulx](https://github.com/ericproulx). * [#2908](https://github.com/ruby-grape/grape/pull/2908): Stop reassigning method parameters across `lib`, so a parameter keeps the value its caller passed for the whole method; the `oneof` collection in `Grape::Validations::ParamsScope` no longer writes into the Hash it was given - [@ericproulx](https://github.com/ericproulx). * [#2910](https://github.com/ruby-grape/grape/pull/2910): Restore `Grape::Middleware::Formatter`'s in-place content-type negotiation as `ensure_content_type!`, which #2908 had turned into a copy of the response headers on every response - [@ericproulx](https://github.com/ericproulx). +* [#2911](https://github.com/ruby-grape/grape/pull/2911): Copy response headers with `merge!` instead of `merge` in `Grape::API::Instance#call` and `Grape::Middleware::Error`, which allocated a `Grape::Util::Header` only to discard it - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 3.3.5 (2026-07-30) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 8a3e2f1e7..2b43d5a8e 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -118,7 +118,12 @@ def initialize def call(env) status, headers, response = @router.call(env) unless @cascade - headers = Grape::Util::Header.new.merge(headers) + # +merge!+, not +merge+: the latter is a `dup` plus a `merge!`, so the + # Header built on this line would be allocated only to be discarded. + # The copy stays because +headers+ can come from a mounted Rack app, + # which is free to hand back a frozen or shared Hash that the delete + # below must not reach into. + headers = Grape::Util::Header.new.merge!(headers) headers.delete('X-Cascade') end diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 0b43730c0..a2dd7c48a 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -68,7 +68,7 @@ def call!(env) def rack_response(status, headers, message) body = html_content_type?(headers[Rack::CONTENT_TYPE]) ? Rack::Utils.escape_html(message) : message - Rack::Response.new(Array.wrap(body), Rack::Utils.status_code(status), Grape::Util::Header.new.merge(headers)) + Rack::Response.new(Array.wrap(body), Rack::Utils.status_code(status), Grape::Util::Header.new.merge!(headers)) end # Escaping must key off the media type only, case-insensitively. Comparing diff --git a/spec/grape/api/instance_spec.rb b/spec/grape/api/instance_spec.rb index 2af3df688..e7deff512 100644 --- a/spec/grape/api/instance_spec.rb +++ b/spec/grape/api/instance_spec.rb @@ -117,4 +117,29 @@ def app expect(an_instance.compile!.cascade?).to be(true) end end + + describe '#call' do + context 'when cascade is false' do + let(:rack_headers) { { 'content-type' => 'text/plain', 'x-cascade' => 'pass' }.freeze } + let(:root_api) do + headers = rack_headers + Class.new(Grape::API::Instance) do + cascade false + mount ->(_env) { [200, headers, ['from rack']] } => '/rack' + end + end + + it 'strips X-Cascade from the response' do + get '/rack' + expect(last_response.headers).not_to have_key('x-cascade') + end + + # The mounted app owns the Hash it returned and is free to hand back a + # frozen or shared one, so removing X-Cascade has to happen on a copy. + it 'does not write into the headers the mounted app returned' do + get '/rack' + expect(rack_headers).to eq('content-type' => 'text/plain', 'x-cascade' => 'pass') + end + end + end end