From fc0db35fb1bbabecf8283475149d21c3cc7762bf Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 2 Aug 2022 12:35:09 +0200 Subject: [PATCH] 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`. --- actionmailer/test/caching_test.rb | 27 +----- actionpack/test/controller/caching_test.rb | 27 +----- actionview/lib/action_view/buffers.rb | 83 ++++++++++++++--- .../lib/action_view/helpers/cache_helper.rb | 13 +-- .../action_view/template/handlers/builder.rb | 7 +- actionview/test/buffers_test.rb | 89 +++++++++++++++++++ actionview/test/output_buffer_test.rb | 31 ------- 7 files changed, 168 insertions(+), 109 deletions(-) create mode 100644 actionview/test/buffers_test.rb delete mode 100644 actionview/test/output_buffer_test.rb diff --git a/actionmailer/test/caching_test.rb b/actionmailer/test/caching_test.rb index c3a3e418d097d..3a6e7867a5186 100644 --- a/actionmailer/test/caching_test.rb +++ b/actionmailer/test/caching_test.rb @@ -221,31 +221,8 @@ def self.output_buffer=; end cache_helper.stub :controller, controller do cache_helper.stub :output_buffer, output_buffer do - assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do - assert_nothing_raised do - cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil } - end - end - end - end - end - - def test_safe_buffer - output_buffer = ActiveSupport::SafeBuffer.new - controller = MockController.new - cache_helper = Class.new do - def self.controller; end - def self.output_buffer; end - def self.output_buffer=; end - end - cache_helper.extend(ActionView::Helpers::CacheHelper) - - cache_helper.stub :controller, controller do - cache_helper.stub :output_buffer, output_buffer do - assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do - assert_nothing_raised do - cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil } - end + assert_nothing_raised do + cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil } end end end diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index dbda943aacb5a..42d88811bd186 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -344,31 +344,8 @@ def self.output_buffer=; end cache_helper.stub :controller, controller do cache_helper.stub :output_buffer, output_buffer do - assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do - assert_nothing_raised do - cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil } - end - end - end - end - end - - def test_safe_buffer - output_buffer = ActiveSupport::SafeBuffer.new - controller = MockController.new - cache_helper = Class.new do - def self.controller; end - def self.output_buffer; end - def self.output_buffer=; end - end - cache_helper.extend(ActionView::Helpers::CacheHelper) - - cache_helper.stub :controller, controller do - cache_helper.stub :output_buffer, output_buffer do - assert_called_with cache_helper, :output_buffer=, [output_buffer.class.new(output_buffer)] do - assert_nothing_raised do - cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil } - end + assert_nothing_raised do + cache_helper.send :fragment_for, "Test fragment name", "Test fragment", &Proc.new { nil } end end end diff --git a/actionview/lib/action_view/buffers.rb b/actionview/lib/action_view/buffers.rb index be644a4748b0a..a3f0761e7f0de 100644 --- a/actionview/lib/action_view/buffers.rb +++ b/actionview/lib/action_view/buffers.rb @@ -20,19 +20,19 @@ module ActionView # class OutputBuffer # :nodoc: def initialize(buffer = "") - @buffer = String.new(buffer) - @buffer.encode! + @raw_buffer = String.new(buffer) + @raw_buffer.encode! end - delegate :length, :blank?, :encoding, :encode!, :force_encoding, to: :@buffer + delegate :length, :blank?, :encoding, :encode!, :force_encoding, to: :@raw_buffer def to_s - @buffer.html_safe + @raw_buffer.html_safe end alias_method :html_safe, :to_s def to_str - @buffer.dup + @raw_buffer.dup end def html_safe? @@ -42,7 +42,7 @@ def html_safe? def <<(value) unless value.nil? value = value.to_s - @buffer << if value.html_safe? + @raw_buffer << if value.html_safe? value else CGI.escapeHTML(value) @@ -53,28 +53,54 @@ def <<(value) alias :append= :<< def safe_concat(value) - @buffer << value + @raw_buffer << value self end alias :safe_append= :safe_concat def safe_expr_append=(val) return self if val.nil? - @buffer << val.to_s + @raw_buffer << val.to_s self end def initialize_copy(other) - @buffer = other.to_str + @raw_buffer = other.to_str end - # Don't use this - def slice!(range) - @buffer.slice!(range) + def capture + new_buffer = +"" + old_buffer, @raw_buffer = @raw_buffer, new_buffer + yield + new_buffer.html_safe + ensure + @raw_buffer = old_buffer end def ==(other) - other.class == self.class && @buffer == other.to_str + other.class == self.class && @raw_buffer == other.to_str + end + + def raw + RawOutputBuffer.new(self) + end + + attr_reader :raw_buffer + end + + class RawOutputBuffer # :nodoc: + def initialize(buffer) + @buffer = buffer + end + + def <<(value) + unless value.nil? + @buffer.raw_buffer << value.to_s + end + end + + def raw + self end end @@ -96,6 +122,15 @@ def safe_concat(value) end alias :safe_append= :safe_concat + def capture + buffer = +"" + old_block, @block = @block, ->(value) { buffer << value } + yield + buffer.html_safe + ensure + @block = old_block + end + def html_safe? true end @@ -103,5 +138,27 @@ def html_safe? def html_safe self end + + def raw + RawStreamingBuffer.new(self) + end + + attr_reader :block + end + + class RawStreamingBuffer # :nodoc: + def initialize(buffer) + @buffer = buffer + end + + def <<(value) + unless value.nil? + @buffer.block.call(value.to_s) + end + end + + def raw + self + end end end diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb index be3689c93b470..67bd76a07c4ac 100644 --- a/actionview/lib/action_view/helpers/cache_helper.rb +++ b/actionview/lib/action_view/helpers/cache_helper.rb @@ -281,17 +281,8 @@ def read_fragment_for(name, options) controller.read_fragment(name, options) end - def write_fragment_for(name, options) - pos = output_buffer.length - yield - output_safe = output_buffer.html_safe? - # We need to modify the buffer in place to be deal with the view handlers - # like `builder` that don't access the buffer through `@output_buffer` but - # keep the initial reference. - fragment = output_buffer.slice!(pos..-1) - if output_safe - self.output_buffer = output_buffer.class.new(output_buffer) - end + def write_fragment_for(name, options, &block) + fragment = output_buffer.capture(&block) controller.write_fragment(name, fragment, options) end diff --git a/actionview/lib/action_view/template/handlers/builder.rb b/actionview/lib/action_view/template/handlers/builder.rb index e5413cd2a0d50..2e1371c015fb5 100644 --- a/actionview/lib/action_view/template/handlers/builder.rb +++ b/actionview/lib/action_view/template/handlers/builder.rb @@ -7,10 +7,9 @@ class Builder def call(template, source) require_engine - "xml = ::Builder::XmlMarkup.new(:indent => 2);" \ - "self.output_buffer = xml.target!;" + - source + - ";xml.target!;" + "xml = ::Builder::XmlMarkup.new(indent: 2, target: output_buffer.raw);" \ + "#{source};" \ + "output_buffer.to_s" end private diff --git a/actionview/test/buffers_test.rb b/actionview/test/buffers_test.rb new file mode 100644 index 0000000000000..6d1c08a25211e --- /dev/null +++ b/actionview/test/buffers_test.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require "abstract_unit" + +module SharedBufferTests + def self.included(test_case) + test_case.test "#<< maintains HTML safety" do + @buffer << "" + assert_predicate @buffer, :html_safe? + assert_predicate output, :html_safe? + assert_equal "<script>alert('pwned!')</script>", output + end + + test_case.test "#safe_append= bypasses HTML safety" do + @buffer.safe_append = "

This is fine

" + assert_predicate @buffer, :html_safe? + assert_predicate output, :html_safe? + assert_equal "

This is fine

", output + end + + test_case.test "#raw allow to bypass HTML escaping" do + raw_buffer = @buffer.raw + raw_buffer << "" + assert_predicate @buffer, :html_safe? + assert_predicate output, :html_safe? + assert_equal "", output + end + + test_case.test "#capture allow to intercept writes" do + @buffer << "Hello" + result = @buffer.capture do + @buffer << "George!" + end + assert_equal "George!", result + assert_predicate result, :html_safe? + + @buffer << " World!" + assert_equal "Hello World!", output + end + + test_case.test "#raw respects #capture" do + @buffer << "Hello" + raw_buffer = @buffer.raw + result = @buffer.capture do + raw_buffer << "George!" + end + assert_equal "George!", result + assert_predicate result, :html_safe? + + @buffer << " World!" + assert_equal "Hello World!", output + end + end +end + +class TestOutputBuffer < ActiveSupport::TestCase + include SharedBufferTests + + setup do + @buffer = ActionView::OutputBuffer.new + end + + test "can be duped" do + @buffer << "Hello" + copy = @buffer.dup + copy << " World!" + assert_equal "Hello World!", copy.to_s + assert_equal "Hello", output + end + + private + def output + @buffer.to_s + end +end + +class TestStreamingBuffer < ActiveSupport::TestCase + include SharedBufferTests + + setup do + @raw_buffer = +"" + @buffer = ActionView::StreamingBuffer.new(@raw_buffer.method(:<<)) + end + + private + def output + @raw_buffer.html_safe + end +end diff --git a/actionview/test/output_buffer_test.rb b/actionview/test/output_buffer_test.rb deleted file mode 100644 index c3d051bf90f9b..0000000000000 --- a/actionview/test/output_buffer_test.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -require "abstract_unit" - -class TestOutputBuffer < ActiveSupport::TestCase - setup do - @buffer = ActionView::OutputBuffer.new - end - - test "#<< maintains HTML safety" do - @buffer << "" - assert_predicate @buffer, :html_safe? - assert_predicate @buffer.to_s, :html_safe? - assert_equal "<script>alert('pwned!')</script>", @buffer.to_s - end - - test "#safe_append= bypasses HTML safety" do - @buffer.safe_append = "

This is fine

" - assert_predicate @buffer, :html_safe? - assert_predicate @buffer.to_s, :html_safe? - assert_equal "

This is fine

", @buffer.to_s - end - - test "can be duped" do - @buffer << "Hello" - copy = @buffer.dup - copy << " World!" - assert_equal "Hello World!", copy.to_s - assert_equal "Hello", @buffer.to_s - end -end