Compare commits

...

4 Commits

Author SHA1 Message Date
Dylan Thacker-Smith
391a6eb92b Temporarily use liquid-c branch for its corresponding change 2020-08-20 10:12:28 -04:00
Dylan Thacker-Smith
89d206d8c6 Use symbols for filter names now that ruby has symbol GC 2020-08-20 10:06:17 -04:00
Dylan Thacker-Smith
5532df880f Handle disabled tags errors like other liquid errors (#1275) 2020-08-18 11:39:54 -04:00
Dylan Thacker-Smith
2b11efc3ae Fix performance regression from introduction of Template#disable_tags (#1274) 2020-08-18 11:25:51 -04:00
24 changed files with 212 additions and 230 deletions

View File

@@ -22,6 +22,6 @@ group :test do
gem 'rubocop-performance', require: false
platform :mri, :truffleruby do
gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'master'
gem 'liquid-c', github: 'Shopify/liquid-c', branch: 'symbol-filter-names'
end
end

View File

@@ -63,6 +63,8 @@ require 'liquid/expression'
require 'liquid/context'
require 'liquid/parser_switching'
require 'liquid/tag'
require 'liquid/tag/disabler'
require 'liquid/tag/disableable'
require 'liquid/block'
require 'liquid/block_body'
require 'liquid/document'
@@ -86,4 +88,3 @@ require 'liquid/template_factory'
# Load all the tags of the standard library
#
Dir["#{__dir__}/liquid/tags/*.rb"].each { |f| require f }
Dir["#{__dir__}/liquid/registers/*.rb"].each { |f| require f }

View File

@@ -171,13 +171,7 @@ module Liquid
private
def render_node(context, output, node)
if node.disabled?(context)
output << node.disabled_error_message
return
end
disable_tags(context, node.disabled_tags) do
node.render_to_output_buffer(context, output)
end
node.render_to_output_buffer(context, output)
rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e
context.handle_error(e, node.line_number)
rescue ::StandardError => e
@@ -185,11 +179,6 @@ module Liquid
output << context.handle_error(e, line_number)
end
def disable_tags(context, tags, &block)
return yield if tags.empty?
context.registers[:disabled_tags].disable(tags, &block)
end
def raise_if_resource_limits_reached(context, length)
context.resource_limits.render_length += length
return unless context.resource_limits.reached?

View File

@@ -44,6 +44,7 @@ module Liquid
@interrupts = []
@filters = []
@global_filter = nil
@disabled_tags = {}
end
# rubocop:enable Metrics/ParameterLists
@@ -144,6 +145,7 @@ module Liquid
subcontext.strainer = nil
subcontext.errors = errors
subcontext.warnings = warnings
subcontext.disabled_tags = @disabled_tags
end
end
@@ -208,9 +210,24 @@ module Liquid
end
end
def with_disabled_tags(tag_names)
tag_names.each do |name|
@disabled_tags[name] = @disabled_tags.fetch(name, 0) + 1
end
yield
ensure
tag_names.each do |name|
@disabled_tags[name] -= 1
end
end
def tag_disabled?(tag_name)
@disabled_tags.fetch(tag_name, 0) > 0
end
protected
attr_writer :base_scope_depth, :warnings, :errors, :strainer, :filters
attr_writer :base_scope_depth, :warnings, :errors, :strainer, :filters, :disabled_tags
private

View File

@@ -53,5 +53,6 @@ module Liquid
UndefinedDropMethod = Class.new(Error)
UndefinedFilter = Class.new(Error)
MethodOverrideError = Class.new(Error)
DisabledError = Class.new(Error)
InternalError = Class.new(Error)
end

View File

@@ -1,25 +1,21 @@
# frozen_string_literal: true
module Liquid
class BlockBody
def render_node_with_profiling(context, output, node)
module BlockBodyProfilingHook
def render_node(context, output, node)
Profiler.profile_node_render(node) do
render_node_without_profiling(context, output, node)
super
end
end
alias_method :render_node_without_profiling, :render_node
alias_method :render_node, :render_node_with_profiling
end
BlockBody.prepend(BlockBodyProfilingHook)
class Include < Tag
def render_to_output_buffer_with_profiling(context, output)
module IncludeProfilingHook
def render_to_output_buffer(context, output)
Profiler.profile_children(context.evaluate(@template_name_expr).to_s) do
render_to_output_buffer_without_profiling(context, output)
super
end
end
alias_method :render_to_output_buffer_without_profiling, :render_to_output_buffer
alias_method :render_to_output_buffer, :render_to_output_buffer_with_profiling
end
Include.prepend(IncludeProfilingHook)
end

View File

@@ -1,32 +0,0 @@
# frozen_string_literal: true
module Liquid
class DisabledTags < Register
def initialize
@disabled_tags = {}
end
def disabled?(tag)
@disabled_tags.key?(tag) && @disabled_tags[tag] > 0
end
def disable(tags)
tags.each(&method(:increment))
yield
ensure
tags.each(&method(:decrement))
end
private
def increment(tag)
@disabled_tags[tag] ||= 0
@disabled_tags[tag] += 1
end
def decrement(tag)
@disabled_tags[tag] -= 1
end
end
Template.add_register(:disabled_tags, DisabledTags.new)
end

View File

@@ -24,17 +24,17 @@ module Liquid
include(filter)
filter_methods.merge(filter.public_instance_methods.map(&:to_s))
filter.public_instance_methods.each { |name| filter_methods[name] = true }
end
def invokable?(method)
filter_methods.include?(method.to_s)
filter_methods.key?(method.to_sym)
end
private
def filter_methods
@filter_methods ||= Set.new
@filter_methods ||= {}
end
end

View File

@@ -13,15 +13,13 @@ module Liquid
tag
end
def disable_tags(*tags)
disabled_tags.push(*tags)
def disable_tags(*tag_names)
@disabled_tags ||= []
@disabled_tags.concat(tag_names)
prepend(Disabler)
end
private :new
def disabled_tags
@disabled_tags ||= []
end
end
def initialize(tag_name, markup, parse_context)
@@ -46,14 +44,6 @@ module Liquid
''
end
def disabled?(context)
context.registers[:disabled_tags].disabled?(tag_name)
end
def disabled_error_message
"#{tag_name} #{options[:locale].t('errors.disabled.tag')}"
end
# For backwards compatibility with custom tags. In a future release, the semantics
# of the `render_to_output_buffer` method will become the default and the `render`
# method will be removed.
@@ -65,9 +55,5 @@ module Liquid
def blank?
false
end
def disabled_tags
self.class.disabled_tags
end
end
end

View File

@@ -0,0 +1,22 @@
# frozen_string_literal: true
module Liquid
class Tag
module Disableable
def render_to_output_buffer(context, output)
if context.tag_disabled?(tag_name)
output << disabled_error(context)
return
end
super
end
def disabled_error(context)
# raise then rescue the exception so that the Context#exception_renderer can re-raise it
raise DisabledError, "#{tag_name} #{parse_context[:locale].t('errors.disabled.tag')}"
rescue DisabledError => exc
context.handle_error(exc, line_number)
end
end
end
end

View File

@@ -0,0 +1,21 @@
# frozen_string_literal: true
module Liquid
class Tag
module Disabler
module ClassMethods
attr_reader :disabled_tags
end
def self.prepended(base)
base.extend(ClassMethods)
end
def render_to_output_buffer(context, output)
context.with_disabled_tags(self.class.disabled_tags) do
super
end
end
end
end
end

View File

@@ -16,6 +16,8 @@ module Liquid
# {% include 'product' for products %}
#
class Include < Tag
prepend Tag::Disableable
SYNTAX = /(#{QuotedFragment}+)(\s+(?:with|for)\s+(#{QuotedFragment}+))?(\s+(?:as)\s+(#{VariableSegment}+))?/o
Syntax = SYNTAX

View File

@@ -80,14 +80,6 @@ module Liquid
tags[name.to_s] = klass
end
attr_accessor :registers
Template.registers = {}
private :registers=
def add_register(name, klass)
registers[name.to_sym] = klass
end
# Pass a module with filter methods which should be available
# to all liquid views. Good for registering the standard library
def register_filter(mod)
@@ -194,10 +186,6 @@ module Liquid
context.add_filters(args.pop)
end
Template.registers.each do |key, register|
context_register[key] = register unless context_register.key?(key)
end
# Retrying a render resets resource usage
context.resource_limits.reset

View File

@@ -123,7 +123,7 @@ module Liquid
filter_args << Expression.parse(a)
end
end
result = [filter_name, filter_args]
result = [filter_name.to_sym, filter_args]
result << keyword_args if keyword_args
result
end

View File

@@ -1,27 +0,0 @@
# frozen_string_literal: true
require 'test_helper'
class DisabledTagsTest < Minitest::Test
include Liquid
class DisableRaw < Block
disable_tags "raw"
end
class DisableRawEcho < Block
disable_tags "raw", "echo"
end
def test_disables_raw
with_custom_tag('disable', DisableRaw) do
assert_template_result 'raw usage is not allowed in this contextfoo', '{% disable %}{% raw %}Foobar{% endraw %}{% echo "foo" %}{% enddisable %}'
end
end
def test_disables_echo_and_raw
with_custom_tag('disable', DisableRawEcho) do
assert_template_result 'raw usage is not allowed in this contextecho usage is not allowed in this context', '{% disable %}{% raw %}Foobar{% endraw %}{% echo "foo" %}{% enddisable %}'
end
end
end

View File

@@ -43,15 +43,22 @@ class SecurityTest < Minitest::Test
assert_equal(expected, Template.parse(text).render!(@assigns, filters: SecurityFilter))
end
def test_does_not_add_filters_to_symbol_table
def test_does_not_permanently_add_filters_to_symbol_table
current_symbols = Symbol.all_symbols
test = %( {{ "some_string" | a_bad_filter }} )
# MRI imprecisely marks objects found on the C stack, which can result
# in uninitialized memory being marked. This can even result in the test failing
# deterministically for a given compilation of ruby. Using a separate thread will
# keep these writes of the symbol pointer on a separate stack that will be garbage
# collected after Thread#join.
Thread.new do
test = %( {{ "some_string" | a_bad_filter }} )
Template.parse(test).render!
nil
end.join
template = Template.parse(test)
assert_equal([], (Symbol.all_symbols - current_symbols))
GC.start
template.render!
assert_equal([], (Symbol.all_symbols - current_symbols))
end

View File

@@ -0,0 +1,51 @@
# frozen_string_literal: true
require 'test_helper'
class TagDisableableTest < Minitest::Test
include Liquid
class DisableRaw < Block
disable_tags "raw"
end
class DisableRawEcho < Block
disable_tags "raw", "echo"
end
class DisableableRaw < Liquid::Raw
prepend Liquid::Tag::Disableable
end
class DisableableEcho < Liquid::Echo
prepend Liquid::Tag::Disableable
end
def test_disables_raw
with_disableable_tags do
with_custom_tag('disable', DisableRaw) do
output = Template.parse('{% disable %}{% raw %}Foobar{% endraw %}{% echo "foo" %}{% enddisable %}').render
assert_equal('Liquid error: raw usage is not allowed in this contextfoo', output)
end
end
end
def test_disables_echo_and_raw
with_disableable_tags do
with_custom_tag('disable', DisableRawEcho) do
output = Template.parse('{% disable %}{% raw %}Foobar{% endraw %}{% echo "foo" %}{% enddisable %}').render
assert_equal('Liquid error: raw usage is not allowed in this contextLiquid error: echo usage is not allowed in this context', output)
end
end
end
private
def with_disableable_tags
with_custom_tag('raw', DisableableRaw) do
with_custom_tag('echo', DisableableEcho) do
yield
end
end
end
end

View File

@@ -127,7 +127,10 @@ class RenderTagTest < Minitest::Test
'test_include' => '{% include "foo" %}'
)
assert_template_result('include usage is not allowed in this context', '{% render "test_include" %}')
exc = assert_raises(Liquid::DisabledError) do
Liquid::Template.parse('{% render "test_include" %}').render!
end
assert_equal('Liquid error: include usage is not allowed in this context', exc.message)
end
def test_includes_will_not_render_inside_nested_sibling_tags
@@ -137,7 +140,8 @@ class RenderTagTest < Minitest::Test
'test_include' => '{% include "foo" %}'
)
assert_template_result('include usage is not allowed in this contextinclude usage is not allowed in this context', '{% render "nested_render_with_sibling_include" %}')
output = Liquid::Template.parse('{% render "nested_render_with_sibling_include" %}').render
assert_equal('Liquid error: include usage is not allowed in this contextLiquid error: include usage is not allowed in this context', output)
end
def test_render_tag_with

View File

@@ -361,48 +361,4 @@ class TemplateTest < Minitest::Test
result = t.render('x' => 1, 'y' => 5)
assert_equal('12345', result)
end
def test_render_uses_correct_disabled_tags_instance
Liquid::Template.file_system = StubFileSystem.new(
'foo' => 'bar',
'test_include' => '{% include "foo" %}'
)
disabled_tags = DisabledTags.new
context = Context.build(registers: { disabled_tags: disabled_tags })
source = "{% render 'test_include' %}"
parse_context = Liquid::ParseContext.new(line_numbers: true, error_mode: :strict)
document = Document.parse(Liquid::Tokenizer.new(source, true), parse_context)
assert_equal("include usage is not allowed in this context", document.render(context))
end
def test_render_sets_context_static_register_when_register_key_does_exist
disabled_tags_for_test = DisabledTags.new
Template.add_register(:disabled_tags, disabled_tags_for_test)
t = Template.parse("{% if true %} Test Template {% endif %}")
context = Context.new
refute(context.registers.key?(:disabled_tags))
t.render(context)
assert(context.registers.key?(:disabled_tags))
assert_equal(disabled_tags_for_test, context.registers[:disabled_tags])
end
def test_render_does_not_override_context_static_register_when_register_key_exists
context = Context.new
context.registers[:random_register] = nil
Template.add_register(:random_register, {})
t = Template.parse("{% if true %} Test Template {% endif %}")
t.render(context)
assert_nil(context.registers[:random_register])
assert(context.registers.key?(:random_register))
end
end

View File

@@ -98,10 +98,17 @@ module Minitest
end
def with_custom_tag(tag_name, tag_class)
Liquid::Template.register_tag(tag_name, tag_class)
yield
ensure
Liquid::Template.tags.delete(tag_name)
old_tag = Liquid::Template.tags[tag_name]
begin
Liquid::Template.register_tag(tag_name, tag_class)
yield
ensure
if old_tag
Liquid::Template.tags[tag_name] = old_tag
else
Liquid::Template.tags.delete(tag_name)
end
end
end
end
end

View File

@@ -563,6 +563,35 @@ class ContextUnitTest < Minitest::Test
assert_equal('my filter result', template.render(subcontext))
end
def test_disables_tag_specified
context = Context.new
context.with_disabled_tags(%w(foo bar)) do
assert_equal true, context.tag_disabled?("foo")
assert_equal true, context.tag_disabled?("bar")
assert_equal false, context.tag_disabled?("unknown")
end
end
def test_disables_nested_tags
context = Context.new
context.with_disabled_tags(["foo"]) do
context.with_disabled_tags(["foo"]) do
assert_equal true, context.tag_disabled?("foo")
assert_equal false, context.tag_disabled?("bar")
end
context.with_disabled_tags(["bar"]) do
assert_equal true, context.tag_disabled?("foo")
assert_equal true, context.tag_disabled?("bar")
context.with_disabled_tags(["foo"]) do
assert_equal true, context.tag_disabled?("foo")
assert_equal true, context.tag_disabled?("bar")
end
end
assert_equal true, context.tag_disabled?("foo")
assert_equal false, context.tag_disabled?("bar")
end
end
private
def assert_no_object_allocations

View File

@@ -1,36 +0,0 @@
# frozen_string_literal: true
require 'test_helper'
class DisabledTagsUnitTest < Minitest::Test
include Liquid
def test_disables_tag_specified
register = DisabledTags.new
register.disable(%w(foo bar)) do
assert_equal true, register.disabled?("foo")
assert_equal true, register.disabled?("bar")
assert_equal false, register.disabled?("unknown")
end
end
def test_disables_nested_tags
register = DisabledTags.new
register.disable(["foo"]) do
register.disable(["foo"]) do
assert_equal true, register.disabled?("foo")
assert_equal false, register.disabled?("bar")
end
register.disable(["bar"]) do
assert_equal true, register.disabled?("foo")
assert_equal true, register.disabled?("bar")
register.disable(["foo"]) do
assert_equal true, register.disabled?("foo")
assert_equal true, register.disabled?("bar")
end
end
assert_equal true, register.disabled?("foo")
assert_equal false, register.disabled?("bar")
end
end
end

View File

@@ -60,7 +60,7 @@ class StrainerTemplateUnitTest < Minitest::Test
strainer = Context.new.strainer
with_global_filter do
strainer.class.add_filter(PublicMethodOverrideFilter)
assert(strainer.class.send(:filter_methods).include?('public_filter'))
assert(strainer.class.send(:filter_methods).include?(:public_filter))
end
end

View File

@@ -13,75 +13,75 @@ class VariableUnitTest < Minitest::Test
def test_filters
var = create_variable('hello | textileze')
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['textileze', []]], var.filters)
assert_equal([[:textileze, []]], var.filters)
var = create_variable('hello | textileze | paragraph')
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['textileze', []], ['paragraph', []]], var.filters)
assert_equal([[:textileze, []], [:paragraph, []]], var.filters)
var = create_variable(%( hello | strftime: '%Y'))
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['strftime', ['%Y']]], var.filters)
assert_equal([[:strftime, ['%Y']]], var.filters)
var = create_variable(%( 'typo' | link_to: 'Typo', true ))
assert_equal('typo', var.name)
assert_equal([['link_to', ['Typo', true]]], var.filters)
assert_equal([[:link_to, ['Typo', true]]], var.filters)
var = create_variable(%( 'typo' | link_to: 'Typo', false ))
assert_equal('typo', var.name)
assert_equal([['link_to', ['Typo', false]]], var.filters)
assert_equal([[:link_to, ['Typo', false]]], var.filters)
var = create_variable(%( 'foo' | repeat: 3 ))
assert_equal('foo', var.name)
assert_equal([['repeat', [3]]], var.filters)
assert_equal([[:repeat, [3]]], var.filters)
var = create_variable(%( 'foo' | repeat: 3, 3 ))
assert_equal('foo', var.name)
assert_equal([['repeat', [3, 3]]], var.filters)
assert_equal([[:repeat, [3, 3]]], var.filters)
var = create_variable(%( 'foo' | repeat: 3, 3, 3 ))
assert_equal('foo', var.name)
assert_equal([['repeat', [3, 3, 3]]], var.filters)
assert_equal([[:repeat, [3, 3, 3]]], var.filters)
var = create_variable(%( hello | strftime: '%Y, okay?'))
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['strftime', ['%Y, okay?']]], var.filters)
assert_equal([[:strftime, ['%Y, okay?']]], var.filters)
var = create_variable(%( hello | things: "%Y, okay?", 'the other one'))
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['things', ['%Y, okay?', 'the other one']]], var.filters)
assert_equal([[:things, ['%Y, okay?', 'the other one']]], var.filters)
end
def test_filter_with_date_parameter
var = create_variable(%( '2006-06-06' | date: "%m/%d/%Y"))
assert_equal('2006-06-06', var.name)
assert_equal([['date', ['%m/%d/%Y']]], var.filters)
assert_equal([[:date, ['%m/%d/%Y']]], var.filters)
end
def test_filters_without_whitespace
var = create_variable('hello | textileze | paragraph')
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['textileze', []], ['paragraph', []]], var.filters)
assert_equal([[:textileze, []], [:paragraph, []]], var.filters)
var = create_variable('hello|textileze|paragraph')
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['textileze', []], ['paragraph', []]], var.filters)
assert_equal([[:textileze, []], [:paragraph, []]], var.filters)
var = create_variable("hello|replace:'foo','bar'|textileze")
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['replace', ['foo', 'bar']], ['textileze', []]], var.filters)
assert_equal([[:replace, ['foo', 'bar']], [:textileze, []]], var.filters)
end
def test_symbol
var = create_variable("http://disney.com/logo.gif | image: 'med' ", error_mode: :lax)
assert_equal(VariableLookup.new('http://disney.com/logo.gif'), var.name)
assert_equal([['image', ['med']]], var.filters)
assert_equal([[:image, ['med']]], var.filters)
end
def test_string_to_filter
var = create_variable("'http://disney.com/logo.gif' | image: 'med' ")
assert_equal('http://disney.com/logo.gif', var.name)
assert_equal([['image', ['med']]], var.filters)
assert_equal([[:image, ['med']]], var.filters)
end
def test_string_single_quoted
@@ -128,13 +128,13 @@ class VariableUnitTest < Minitest::Test
def test_filter_with_keyword_arguments
var = create_variable(%( hello | things: greeting: "world", farewell: 'goodbye'))
assert_equal(VariableLookup.new('hello'), var.name)
assert_equal([['things', [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters)
assert_equal([[:things, [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters)
end
def test_lax_filter_argument_parsing
var = create_variable(%( number_of_comments | pluralize: 'comment': 'comments' ), error_mode: :lax)
assert_equal(VariableLookup.new('number_of_comments'), var.name)
assert_equal([['pluralize', ['comment', 'comments']]], var.filters)
assert_equal([[:pluralize, ['comment', 'comments']]], var.filters)
end
def test_strict_filter_argument_parsing