Visitar URL original
Add OutputBuffer#raw and #capture to reduce the need to swap the buffer · rails/rails@fc0db35 · GitHub
Skip to content

Commit fc0db35

Browse files
byrootbyroot
committed
Add OutputBuffer#raw and #capture to reduce the need to swap the buffer
Right now many helpers have to deal with two modes of operation to capture view output. The main one is to swap the `@output_buffer` variable with a new buffer. But since some view implementations such as `builder` keep a reference on the buffer they were initialized with, this doesn't always work. So additionally, the various capturing helpers also record the buffer length prior to executing the block, and then `slice!` the buffer back to its original size. This is wasteful and make the code rather unclear. Now that `OutputBuffer` is a delegator, I'd like to refactor all this so that: - @output_buffer is no longer re-assigned - A single OutputBuffer instance is used for the entire response rendering - Instead capturing is done through `OutputBuffer#capture` Once the above is achieved, it should allow us to enabled Erubi's `:chain_appends` option and get some reduced template size and some performance. Not re-assigning `@output_buffer` will also allow template to access the local variable instead of an instance variable, which is cheaper. But more importantly, that should make the code easier to understand and easier to be compatible with `StreamingBuffer`.
1 parent ee754ba commit fc0db35

7 files changed

Lines changed: 168 additions & 109 deletions

File tree

‎actionmailer/test/caching_test.rb‎

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -221,31 +221,8 @@ def self.output_buffer=; end
221221

222222
cache_helper.stub :controller, controller do
223223
cache_helper.stub :output_buffer, output_buffer do
224-
assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do
225-
assert_nothing_raised do
226-
cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil }
227-
end
228-
end
229-
end
230-
end
231-
end
232-
233-
def test_safe_buffer
234-
output_buffer = ActiveSupport::SafeBuffer.new
235-
controller = MockController.new
236-
cache_helper = Class.new do
237-
def self.controller; end
238-
def self.output_buffer; end
239-
def self.output_buffer=; end
240-
end
241-
cache_helper.extend(ActionView::Helpers::CacheHelper)
242-
243-
cache_helper.stub :controller, controller do
244-
cache_helper.stub :output_buffer, output_buffer do
245-
assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do
246-
assert_nothing_raised do
247-
cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil }
248-
end
224+
assert_nothing_raised do
225+
cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil }
249226
end
250227
end
251228
end

‎actionpack/test/controller/caching_test.rb‎

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -344,31 +344,8 @@ def self.output_buffer=; end
344344

345345
cache_helper.stub :controller, controller do
346346
cache_helper.stub :output_buffer, output_buffer do
347-
assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do
348-
assert_nothing_raised do
349-
cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil }
350-
end
351-
end
352-
end
353-
end
354-
end
355-
356-
def test_safe_buffer
357-
output_buffer = ActiveSupport::SafeBuffer.new
358-
controller = MockController.new
359-
cache_helper = Class.new do
360-
def self.controller; end
361-
def self.output_buffer; end
362-
def self.output_buffer=; end
363-
end
364-
cache_helper.extend(ActionView::Helpers::CacheHelper)
365-
366-
cache_helper.stub :controller, controller do
367-
cache_helper.stub :output_buffer, output_buffer do
368-
assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do
369-
assert_nothing_raised do
370-
cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil }
371-
end
347+
assert_nothing_raised do
348+
cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil }
372349
end
373350
end
374351
end

‎actionview/lib/action_view/buffers.rb‎

Lines changed: 70 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,19 @@ module ActionView
2020
#
2121
class OutputBuffer # :nodoc:
2222
def initialize(buffer = "")
23-
@buffer = String.new(buffer)
24-
@buffer.encode!
23+
@raw_buffer = String.new(buffer)
24+
@raw_buffer.encode!
2525
end
2626

27-
delegate :length, :blank?, :encoding, :encode!, :force_encoding, to: :@buffer
27+
delegate :length, :blank?, :encoding, :encode!, :force_encoding, to: :@raw_buffer
2828

2929
def to_s
30-
@buffer.html_safe
30+
@raw_buffer.html_safe
3131
end
3232
alias_method :html_safe, :to_s
3333

3434
def to_str
35-
@buffer.dup
35+
@raw_buffer.dup
3636
end
3737

3838
def html_safe?
@@ -42,7 +42,7 @@ def html_safe?
4242
def <<(value)
4343
unless value.nil?
4444
value = value.to_s
45-
@buffer << if value.html_safe?
45+
@raw_buffer << if value.html_safe?
4646
value
4747
else
4848
CGI.escapeHTML(value)
@@ -53,28 +53,54 @@ def <<(value)
5353
alias :append= :<<
5454

5555
def safe_concat(value)
56-
@buffer << value
56+
@raw_buffer << value
5757
self
5858
end
5959
alias :safe_append= :safe_concat
6060

6161
def safe_expr_append=(val)
6262
return self if val.nil?
63-
@buffer << val.to_s
63+
@raw_buffer << val.to_s
6464
self
6565
end
6666

6767
def initialize_copy(other)
68-
@buffer = other.to_str
68+
@raw_buffer = other.to_str
6969
end
7070

71-
# Don't use this
72-
def slice!(range)
73-
@buffer.slice!(range)
71+
def capture
72+
new_buffer = +""
73+
old_buffer, @raw_buffer = @raw_buffer, new_buffer
74+
yield
75+
new_buffer.html_safe
76+
ensure
77+
@raw_buffer = old_buffer
7478
end
7579

7680
def ==(other)
77-
other.class == self.class && @buffer == other.to_str
81+
other.class == self.class && @raw_buffer == other.to_str
82+
end
83+
84+
def raw
85+
RawOutputBuffer.new(self)
86+
end
87+
88+
attr_reader :raw_buffer
89+
end
90+
91+
class RawOutputBuffer # :nodoc:
92+
def initialize(buffer)
93+
@buffer = buffer
94+
end
95+
96+
def <<(value)
97+
unless value.nil?
98+
@buffer.raw_buffer << value.to_s
99+
end
100+
end
101+
102+
def raw
103+
self
78104
end
79105
end
80106

@@ -96,12 +122,43 @@ def safe_concat(value)
96122
end
97123
alias :safe_append= :safe_concat
98124

125+
def capture
126+
buffer = +""
127+
old_block, @block = @block, ->(value) { buffer << value }
128+
yield
129+
buffer.html_safe
130+
ensure
131+
@block = old_block
132+
end
133+
99134
def html_safe?
100135
true
101136
end
102137

103138
def html_safe
104139
self
105140
end
141+
142+
def raw
143+
RawStreamingBuffer.new(self)
144+
end
145+
146+
attr_reader :block
147+
end
148+
149+
class RawStreamingBuffer # :nodoc:
150+
def initialize(buffer)
151+
@buffer = buffer
152+
end
153+
154+
def <<(value)
155+
unless value.nil?
156+
@buffer.block.call(value.to_s)
157+
end
158+
end
159+
160+
def raw
161+
self
162+
end
106163
end
107164
end

‎actionview/lib/action_view/helpers/cache_helper.rb‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -281,17 +281,8 @@ def read_fragment_for(name, options)
281281
controller.read_fragment(name, options)
282282
end
283283

284-
def write_fragment_for(name, options)
285-
pos = output_buffer.length
286-
yield
287-
output_safe = output_buffer.html_safe?
288-
# We need to modify the buffer in place to be deal with the view handlers
289-
# like `builder` that don't access the buffer through `@output_buffer` but
290-
# keep the initial reference.
291-
fragment = output_buffer.slice!(pos..-1)
292-
if output_safe
293-
self.output_buffer = output_buffer.class.new(output_buffer)
294-
end
284+
def write_fragment_for(name, options, &block)
285+
fragment = output_buffer.capture(&block)
295286
controller.write_fragment(name, fragment, options)
296287
end
297288

‎actionview/lib/action_view/template/handlers/builder.rb‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,9 @@ class Builder
77

88
def call(template, source)
99
require_engine
10-
"xml = ::Builder::XmlMarkup.new(:indent => 2);" \
11-
"self.output_buffer = xml.target!;" +
12-
source +
13-
";xml.target!;"
10+
"xml = ::Builder::XmlMarkup.new(indent: 2, target: output_buffer.raw);" \
11+
"#{source};" \
12+
"output_buffer.to_s"
1413
end
1514

1615
private

‎actionview/test/buffers_test.rb‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
# frozen_string_literal: true
2+
3+
require "abstract_unit"
4+
5+
module SharedBufferTests
6+
def self.included(test_case)
7+
test_case.test "#<< maintains HTML safety" do
8+
@buffer << "<script>alert('pwned!')</script>"
9+
assert_predicate @buffer, :html_safe?
10+
assert_predicate output, :html_safe?
11+
assert_equal "&lt;script&gt;alert(&#39;pwned!&#39;)&lt;/script&gt;", output
12+
end
13+
14+
test_case.test "#safe_append= bypasses HTML safety" do
15+
@buffer.safe_append = "<p>This is fine</p>"
16+
assert_predicate @buffer, :html_safe?
17+
assert_predicate output, :html_safe?
18+
assert_equal "<p>This is fine</p>", output
19+
end
20+
21+
test_case.test "#raw allow to bypass HTML escaping" do
22+
raw_buffer = @buffer.raw
23+
raw_buffer << "<script>alert('pwned!')</script>"
24+
assert_predicate @buffer, :html_safe?
25+
assert_predicate output, :html_safe?
26+
assert_equal "<script>alert('pwned!')</script>", output
27+
end
28+
29+
test_case.test "#capture allow to intercept writes" do
30+
@buffer << "Hello"
31+
result = @buffer.capture do
32+
@buffer << "George!"
33+
end
34+
assert_equal "George!", result
35+
assert_predicate result, :html_safe?
36+
37+
@buffer << " World!"
38+
assert_equal "Hello World!", output
39+
end
40+
41+
test_case.test "#raw respects #capture" do
42+
@buffer << "Hello"
43+
raw_buffer = @buffer.raw
44+
result = @buffer.capture do
45+
raw_buffer << "George!"
46+
end
47+
assert_equal "George!", result
48+
assert_predicate result, :html_safe?
49+
50+
@buffer << " World!"
51+
assert_equal "Hello World!", output
52+
end
53+
end
54+
end
55+
56+
class TestOutputBuffer < ActiveSupport::TestCase
57+
include SharedBufferTests
58+
59+
setup do
60+
@buffer = ActionView::OutputBuffer.new
61+
end
62+
63+
test "can be duped" do
64+
@buffer << "Hello"
65+
copy = @buffer.dup
66+
copy << " World!"
67+
assert_equal "Hello World!", copy.to_s
68+
assert_equal "Hello", output
69+
end
70+
71+
private
72+
def output
73+
@buffer.to_s
74+
end
75+
end
76+
77+
class TestStreamingBuffer < ActiveSupport::TestCase
78+
include SharedBufferTests
79+
80+
setup do
81+
@raw_buffer = +""
82+
@buffer = ActionView::StreamingBuffer.new(@raw_buffer.method(:<<))
83+
end
84+
85+
private
86+
def output
87+
@raw_buffer.html_safe
88+
end
89+
end

‎actionview/test/output_buffer_test.rb‎

Lines changed: 0 additions & 31 deletions
This file was deleted.

0 commit comments

Comments
 (0)