Skip to content

Commit 1e3968e

Browse files
committed
Raise DoubleRenderError on head after rendering
Currently, calling `head` to set the response code silently removes a previously set response body [0]. This can lead to unnecessary and potentially expensive rendering, as shown in the following snippet: ```ruby def index render json: User.all.as_json # expensive rendering head :ok # Overwrites `response_body` to `` end ``` This behavior is inconsistent with calling `render` twice or calling `redirect_to` after `render`: both of which raise a `DoubleRenderError` [1, 2]. Since this change could break existing endpoints when upgrading Rails, it might make sense to introduce a configuration option would allow developers to choose whether this scenario raises an exception or a warning. [0] https://github.com/rails/rails/blob/b721c62de11c2c29fa2b8ef51b00d95dce163afc/actionpack/lib/action_controller/metal/head.rb#L50 [1] https://github.com/rails/rails/blob/b721c62de11c2c29fa2b8ef51b00d95dce163afc/actionpack/lib/action_controller/metal/redirecting.rb#L103-L105 [2] https://github.com/rails/rails/blob/b721c62de11c2c29fa2b8ef51b00d95dce163afc/actionpack/lib/action_controller/metal/rendering.rb#L171-L174
1 parent 348f9b1 commit 1e3968e

File tree

3 files changed

+38
-0
lines changed

3 files changed

+38
-0
lines changed

actionpack/CHANGELOG.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,18 @@
1+
* Raise `AbstractController::DoubleRenderError` if `head` is called after rendering.
2+
3+
After this change, invoking `head` will lead to an error if response body is already set:
4+
5+
```ruby
6+
class PostController < ApplicationController
7+
def index
8+
render locals: {}
9+
head :ok
10+
end
11+
end
12+
```
13+
14+
*Iaroslav Kurbatov*
15+
116
* The Cookie Serializer can now serialize an Active Support SafeBuffer when using message pack.
217

318
Such code would previously produce an error if an application was using messagepack as its cookie serializer.

actionpack/lib/action_controller/metal/head.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ def head(status, options = nil)
2525
raise ArgumentError, "#{status.inspect} is not a valid value for `status`."
2626
end
2727

28+
raise ::AbstractController::DoubleRenderError if response_body
29+
2830
status ||= :ok
2931

3032
if options

actionpack/test/controller/new_base/render_test.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,13 @@ def index
4949
end
5050
end
5151

52+
class DoubleRenderWithHeadController < ActionController::Base
53+
def index
54+
render plain: "hello"
55+
head :bad_request
56+
end
57+
end
58+
5259
class ChildRenderController < BlankRenderController
5360
append_view_path ActionView::FixtureResolver.new("render/child_render/overridden_with_own_view_paths_appended.html.erb" => "child content")
5461
prepend_view_path ActionView::FixtureResolver.new("render/child_render/overridden_with_own_view_paths_prepended.html.erb" => "child content")
@@ -83,6 +90,20 @@ class RenderTest < Rack::TestCase
8390
end
8491
end
8592
end
93+
94+
test "using head after rendering raises an exception" do
95+
with_routing do |set|
96+
set.draw do
97+
ActionDispatch.deprecator.silence do
98+
get ":controller", action: "index"
99+
end
100+
end
101+
102+
assert_raises(AbstractController::DoubleRenderError) do
103+
get "/render/double_render_with_head", headers: { "action_dispatch.show_exceptions" => :none }
104+
end
105+
end
106+
end
86107
end
87108

88109
class TestOnlyRenderPublicActions < Rack::TestCase

0 commit comments

Comments
 (0)