From dadd9b4dd22fa1701d5a2199c2061709c49676dd Mon Sep 17 00:00:00 2001 From: Ivan Kuznetsov Date: Wed, 3 Feb 2016 10:49:33 +0700 Subject: [PATCH] Add strict_variables/strict_filters render options to check for undefined variables and filters --- README.md | 31 ++++++++++++++ lib/liquid/block_body.rb | 3 ++ lib/liquid/context.rb | 4 +- lib/liquid/drop.rb | 5 ++- lib/liquid/errors.rb | 3 ++ lib/liquid/strainer.rb | 4 +- lib/liquid/template.rb | 15 ++++--- lib/liquid/variable_lookup.rb | 6 ++- test/integration/template_test.rb | 70 +++++++++++++++++++++++++++++++ test/unit/strainer_unit_test.rb | 10 +++++ 10 files changed, 139 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index cf8fcd3..877fdf6 100644 --- a/README.md +++ b/README.md @@ -73,3 +73,34 @@ This is useful for doing things like enabling strict mode only in the theme edit It is recommended that you enable `:strict` or `:warn` mode on new apps to stop invalid templates from being created. It is also recommended that you use it in the template editors of existing apps to give editors better error messages. + +### Undefined variables and filters + +By default, the renderer doesn't raise or in any other way notify you if some variables or filters are missing, i.e. not passed to the `render` method. +You can improve this situation by passing `strict_variables: true` and/or `strict_filters: true` options to the `render` method. +When one of these options is set to true, all errors about undefined variables and undefined filters will be stored in `errors` array of a `Liquid::Template` instance. +Here are some examples: + +```ruby +template = Liquid::Template.parse("{{x}} {{y}} {{z.a}} {{z.b}}") +template.render({ 'x' => 1, 'z' => { 'a' => 2 } }, { strict_variables: true }) +#=> '1 2 ' # when a variable is undefined, it's rendered as nil +template.errors +#=> [#, #] +``` + +```ruby +template = Liquid::Template.parse("{{x | filter1 | upcase}}") +template.render({ 'x' => 'foo' }, { strict_filters: true }) +#=> '' # when at least one filter in the filter chain is undefined, a whole expression is rendered as nil +template.errors +#=> [#] +``` + +If you want to raise on a first exception instead of pushing all of them in `errors`, you can use `render!` method: + +```ruby +template = Liquid::Template.parse("{{x}} {{y}}") +template.render!({ 'x' => 1}, { strict_variables: true }) +#=> Liquid::UndefinedVariable: Liquid error: undefined variable y +``` diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 60b28b5..c375347 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -76,6 +76,9 @@ module Liquid end rescue MemoryError => e raise e + rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e + context.handle_error(e, token.line_number) + output << nil rescue ::StandardError => e output << context.handle_error(e, token.line_number) end diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 225eec5..bab5008 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -13,7 +13,7 @@ module Liquid # context['bob'] #=> nil class Context class Context attr_reader :scopes, :errors, :registers, :environments, :resource_limits - attr_accessor :exception_handler, :template_name, :partial, :global_filter + attr_accessor :exception_handler, :template_name, :partial, :global_filter, :strict_variables, :strict_filters def initialize(environments = {}, outer_scope = {}, registers = {}, rethrow_errors = false, resource_limits = nil) @environments = [environments].flatten @@ -207,6 +207,8 @@ module Liquid def lookup_and_evaluate(obj, key) if (value = obj[key]).is_a?(Proc) && obj.respond_to?(:[]=) obj[key] = (value.arity == 0) ? value.call : value.call(self) + elsif strict_variables && obj.respond_to?(:key?) && !obj.key?(key) + raise Liquid::UndefinedVariable, "undefined variable #{key}" else value end diff --git a/lib/liquid/drop.rb b/lib/liquid/drop.rb index 0814532..57a17e0 100644 --- a/lib/liquid/drop.rb +++ b/lib/liquid/drop.rb @@ -24,8 +24,9 @@ module Liquid attr_writer :context # Catch all for the method - def liquid_method_missing(_method) - nil + def liquid_method_missing(method) + return nil unless @context.strict_variables + raise Liquid::UndefinedDropMethod, "undefined method #{method}" end # called by liquid to invoke a drop diff --git a/lib/liquid/errors.rb b/lib/liquid/errors.rb index 9ee3a66..762828c 100644 --- a/lib/liquid/errors.rb +++ b/lib/liquid/errors.rb @@ -56,4 +56,7 @@ module Liquid MemoryError = Class.new(Error) ZeroDivisionError = Class.new(Error) FloatDomainError = Class.new(Error) + UndefinedVariable = Class.new(Error) + UndefinedDropMethod = Class.new(Error) + UndefinedFilter = Class.new(Error) end diff --git a/lib/liquid/strainer.rb b/lib/liquid/strainer.rb index a902825..c79a586 100644 --- a/lib/liquid/strainer.rb +++ b/lib/liquid/strainer.rb @@ -26,7 +26,7 @@ module Liquid end def self.add_filter(filter) - raise ArgumentError, "Expected module but got: #{f.class}" unless filter.is_a?(Module) + raise ArgumentError, "Expected module but got: #{filter.class}" unless filter.is_a?(Module) unless self.class.include?(filter) send(:include, filter) @filter_methods.merge(filter.public_instance_methods.map(&:to_s)) @@ -48,6 +48,8 @@ module Liquid def invoke(method, *args) if self.class.invokable?(method) send(method, *args) + elsif @context && @context.strict_filters + raise Liquid::UndefinedFilter, "undefined filter #{method}" else args.first end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index b875e0b..130d17d 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -181,12 +181,7 @@ module Liquid registers.merge!(options[:registers]) if options[:registers].is_a?(Hash) - context.add_filters(options[:filters]) if options[:filters] - - context.global_filter = options[:global_filter] if options[:global_filter] - - context.exception_handler = options[:exception_handler] if options[:exception_handler] - + apply_options_to_context(context, options) when Module, Array context.add_filters(args.pop) end @@ -235,5 +230,13 @@ module Liquid yield end end + + def apply_options_to_context(context, options) + context.add_filters(options[:filters]) if options[:filters] + context.global_filter = options[:global_filter] if options[:global_filter] + context.exception_handler = options[:exception_handler] if options[:exception_handler] + context.strict_variables = options[:strict_variables] if options[:strict_variables] + context.strict_filters = options[:strict_filters] if options[:strict_filters] + end end end diff --git a/lib/liquid/variable_lookup.rb b/lib/liquid/variable_lookup.rb index 62d753a..3ed4e4a 100644 --- a/lib/liquid/variable_lookup.rb +++ b/lib/liquid/variable_lookup.rb @@ -55,9 +55,11 @@ module Liquid object = object.send(key).to_liquid # No key was present with the desired value and it wasn't one of the directly supported - # keywords either. The only thing we got left is to return nil + # keywords either. The only thing we got left is to return nil or + # raise an exception if `strict_variables` option is set to true else - return nil + return nil unless context.strict_variables + raise Liquid::UndefinedVariable, "undefined variable #{key}" end # If we are dealing with a drop here we have to diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index c1e2ef3..aea8669 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -27,6 +27,12 @@ class ErroneousDrop < Liquid::Drop end end +class DropWithUndefinedMethod < Liquid::Drop + def foo + 'foo' + end +end + class TemplateTest < Minitest::Test include Liquid @@ -225,4 +231,68 @@ class TemplateTest < Minitest::Test assert_equal 'BOB filtered', rendered_template end + + def test_undefined_variables + t = Template.parse("{{x}} {{y}} {{z.a}} {{z.b}} {{z.c.d}}") + result = t.render({ 'x' => 33, 'z' => { 'a' => 32, 'c' => { 'e' => 31 } } }, { strict_variables: true }) + + assert_equal '33 32 ', result + assert_equal 3, t.errors.count + assert_instance_of Liquid::UndefinedVariable, t.errors[0] + assert_equal 'Liquid error: undefined variable y', t.errors[0].message + assert_instance_of Liquid::UndefinedVariable, t.errors[1] + assert_equal 'Liquid error: undefined variable b', t.errors[1].message + assert_instance_of Liquid::UndefinedVariable, t.errors[2] + assert_equal 'Liquid error: undefined variable d', t.errors[2].message + end + + def test_undefined_variables_raise + t = Template.parse("{{x}} {{y}} {{z.a}} {{z.b}} {{z.c.d}}") + + assert_raises UndefinedVariable do + t.render!({ 'x' => 33, 'z' => { 'a' => 32, 'c' => { 'e' => 31 } } }, { strict_variables: true }) + end + end + + def test_undefined_drop_methods + d = DropWithUndefinedMethod.new + t = Template.new.parse('{{ foo }} {{ woot }}') + result = t.render(d, { strict_variables: true }) + + assert_equal 'foo ', result + assert_equal 1, t.errors.count + assert_instance_of Liquid::UndefinedDropMethod, t.errors[0] + end + + def test_undefined_drop_methods_raise + d = DropWithUndefinedMethod.new + t = Template.new.parse('{{ foo }} {{ woot }}') + + assert_raises UndefinedDropMethod do + t.render!(d, { strict_variables: true }) + end + end + + def test_undefined_filters + t = Template.parse("{{a}} {{x | upcase | somefilter1 | somefilter2 | somefilter3}}") + filters = Module.new do + def somefilter3(v) + "-#{v}-" + end + end + result = t.render({ 'a' => 123, 'x' => 'foo' }, { filters: [filters], strict_filters: true }) + + assert_equal '123 ', result + assert_equal 1, t.errors.count + assert_instance_of Liquid::UndefinedFilter, t.errors[0] + assert_equal 'Liquid error: undefined filter somefilter1', t.errors[0].message + end + + def test_undefined_filters_raise + t = Template.parse("{{x | somefilter1 | upcase | somefilter2}}") + + assert_raises UndefinedFilter do + t.render!({ 'x' => 'foo' }, { strict_filters: true }) + end + end end diff --git a/test/unit/strainer_unit_test.rb b/test/unit/strainer_unit_test.rb index 2048a39..d06d30a 100644 --- a/test/unit/strainer_unit_test.rb +++ b/test/unit/strainer_unit_test.rb @@ -77,4 +77,14 @@ class StrainerUnitTest < Minitest::Test assert_kind_of b, strainer assert_kind_of Liquid::StandardFilters, strainer end + + def test_add_filter_when_wrong_filter_class + c = Context.new + s = c.strainer + wrong_filter = ->(v) { v.reverse } + + assert_raises ArgumentError do + s.class.add_filter(wrong_filter) + end + end end # StrainerTest