From 7abb339fcd4c9a7a90ff779e19c77ebbe2d37f3d Mon Sep 17 00:00:00 2001 From: Hugo Parente Lima Date: Wed, 18 Jan 2023 15:45:40 -0300 Subject: [PATCH 1/2] Some love to error handling. - Silent emit nil and do not raise `IndexError` on array out of bounds errors (like shopify liquid). - Only raise exceptions under `LiquidException` hierarchy. - Store exceptions instead of strings on `Context#errors`, so we can later add mode info on exception objects. - Do not raise exceptions on undefined variables, even on Strict mode (like shopify liquid). - Render the filter error on `FilterArgumentException` instead of raising (like shopify liquid), but raises on Strict mode, unlike Shopify liquid that does it only if rendering with `render!` - Fix some filters raising `LiquidException` instead of `FilterArgumentException`. - Add some documentation. --- README.md | 12 +++-- spec/expression_spec.cr | 2 - spec/integration/integration_spec.cr | 4 +- spec/liquid_spec.cr | 20 +++++---- spec/template_spec.cr | 1 + src/liquid/context.cr | 40 ++++++++++++----- src/liquid/exceptions.cr | 32 +++++++++++-- src/liquid/expression.cr | 67 +++++++++++++--------------- src/liquid/filters/abs.cr | 2 +- src/liquid/filters/append.cr | 4 +- src/liquid/filters/capitalize.cr | 2 +- src/liquid/filters/ceil.cr | 2 +- src/liquid/filters/floor.cr | 2 +- 13 files changed, 113 insertions(+), 77 deletions(-) diff --git a/README.md b/README.md index 8e4afab..e383155 100644 --- a/README.md +++ b/README.md @@ -161,21 +161,19 @@ Or on an existing Context: ctx.error_mode = :strict ``` -Raises `Liquid::InvalidExpression` on missing keys or `IndexError` on array out of bounds errors instead of silently emitting `nil`. +Raises `Liquid::InvalidExpression` on missing keys and silently emit `nil` on array out of bounds. Append `?` to emit nil in strict mode (very simplistic, just checks for `?` at the end of the identifier) ```crystal -ctx = Liquid::Context.new(strict: true) +ctx = Liquid::Context.new(:strict) ctx["obj"] = { something: "something" } ``` ```liquid -{{ missing }} -> KeyError -{{ missing? }} -> nil -{{ obj.missing }} -> KeyError -{{ obj.missing? }} -> nil -{{ missing.missing? }} -> nil +{{ missing }} -> nil, but generates a UndefinedVariable errors if not in Lax mode. +{{ obj.missing }} -> InvalidExpression +{{ missing.missing? }} -> generates a UndefinedVariable error, evaluates `missing` to nil then raises a InvalidExpression due to `nil.missing` call. ``` ## Contributing diff --git a/spec/expression_spec.cr b/spec/expression_spec.cr index b6ddf8d..d968e40 100644 --- a/spec/expression_spec.cr +++ b/spec/expression_spec.cr @@ -51,8 +51,6 @@ private def it_raises(exception, message : String, expr : String, ctx : Context, end describe Expression do - it_raises(InvalidExpression, "Variable \"bar\" not found", "bar", Context.new(:strict)) - it_evaluates("foo", Context{"foo" => 42}, 42) it_evaluates("foo.blank?", Context{"foo" => ""}, true) it_evaluates("foo.size", Context{"foo" => Any.new("123")}, 3) diff --git a/spec/integration/integration_spec.cr b/spec/integration/integration_spec.cr index 4cb43e3..8f5833e 100644 --- a/spec/integration/integration_spec.cr +++ b/spec/integration/integration_spec.cr @@ -16,7 +16,9 @@ class GoldenTest vars = @context.as_h? raise "Bad context: #{@context.to_s}" if vars.nil? - ctx = Context.new(@strict ? Context::ErrorMode::Strict : Context::ErrorMode::Lax) + # Golden liquid run ruby tests with `render!`, that raises an exception on first error, this is the strict behavior + # of liquid crystal. + ctx = Context.new(@strict || @error ? Context::ErrorMode::Strict : Context::ErrorMode::Lax) vars.each do |key, value| ctx.set(key.as_s, yaml_any_to_liquid_any(value)) end diff --git a/spec/liquid_spec.cr b/spec/liquid_spec.cr index ec5c3dd..47e39af 100644 --- a/spec/liquid_spec.cr +++ b/spec/liquid_spec.cr @@ -18,21 +18,23 @@ describe Liquid::Context do ctx["missing"]?.should be_nil end - it "raises on missing key in strict mode" do - ctx = Context.new(:strict) - ctx["obj"] = Any{"something" => "something"} - expect_raises(InvalidExpression) { ctx.get("missing") } - expect_raises(InvalidExpression) { ctx.get("obj.missing") } + it "returns nil for undefined variables on Lax mode" do + ctx = Context.new(:lax) + ctx.get("missing").raw.should eq(nil) + ctx.errors.should be_empty end - it "returns nil for missing key on Lax mode" do - ctx = Context.new(:lax) + it "does not raise for undefined variables on strict mode" do + ctx = Context.new(:strict) ctx.get("missing").raw.should eq(nil) + ctx.errors.map(&.message).should eq([%(Liquid error: Variable "missing" not found.)]) + ctx.errors.map(&.class).should eq([Liquid::UndefinedVariable]) end - it "returns nil for missing key on Warn mode" do + it "store errors for undefined variables in warn mode" do ctx = Context.new(:warn) ctx.get("missing").raw.should eq(nil) - ctx.errors.should eq([%(Variable "missing" not found.)]) + ctx.errors.map(&.message).should eq([%(Liquid error: Variable "missing" not found.)]) + ctx.errors.map(&.class).should eq([Liquid::UndefinedVariable]) end end diff --git a/spec/template_spec.cr b/spec/template_spec.cr index d235289..f550b7b 100644 --- a/spec/template_spec.cr +++ b/spec/template_spec.cr @@ -12,6 +12,7 @@ describe Template do it_renders("{% if 'a' != blank %}noblank{% endif %}", "noblank") it_renders("{% if '' > blank %}blank{% endif %}", "") it_renders("{% assign a = '' | split %}{% if a == blank %}blank{% endif %}", "blank") + it_renders("{{ a[100] }}", "", Context.new(:strict, {"a" => Any{1, 2}})) it "should render raw text" do tpl = Parser.parse("raw text") diff --git a/src/liquid/context.cr b/src/liquid/context.cr index 9cded80..182631a 100644 --- a/src/liquid/context.cr +++ b/src/liquid/context.cr @@ -3,23 +3,29 @@ require "./blank" module Liquid class Context + # Context error mode. enum ErrorMode + # Raise exceptions on any error but `UndefinedVariable`, that can be accessed later by `Context#errors`. Strict + # Like `Lax` mode, but the errors can be accessed later by `Context#errors`. Warn + # Do the best to render something without raising any exceptions. Lax end - @data = Hash(String, Any).new + @data : Hash(String, Any) # :nodoc: # These values are used/reused when calling filters in a expression using this context. protected getter filter_args = Array(Any).new + # :nodoc: protected getter filter_options = Hash(String, Any).new(Any.new) property error_mode : ErrorMode - getter errors = Array(String).new + # List of errors found when rendering using this context. + getter errors = Array(LiquidException).new - def initialize(@error_mode = :lax) + def initialize(@error_mode = :lax, @data = Hash(String, Any).new) add_builtins end @@ -29,6 +35,7 @@ module Liquid @[Deprecated("Use `initialize(ErrorMode)` instead.")] def initialize(strict : Bool) + @data = Hash(String, Any).new @error_mode = strict ? ErrorMode::Strict : ErrorMode::Lax add_builtins end @@ -54,24 +61,35 @@ module Liquid value end + protected def add_error(error : LiquidException) : Any + raise error if @error_mode.strict? + + @errors << error if @error_mode.warn? + Any.new(nil) + end + + protected def add_error(error : UndefinedVariable) : Any + @errors << error if @error_mode.warn? || @error_mode.strict? + Any.new(nil) + end + + # Fetch a variable from context, add `UndefinedVariable` error if the variable isn't found and behave according the + # error mode. def get(var : String) : Any value = @data[var]? return value if value - if !@error_mode.lax? - error_message = "Variable \"#{var}\" not found." - raise InvalidExpression.new(error_message) if @error_mode.strict? - - @errors << error_message if @error_mode.warn? - end - - Any.new(nil) + add_error(UndefinedVariable.new(var)) end + # Alias for `#get` def [](var : String) : Any get(var) end + # Fetch a variable from context and return nil if the variable isn't found. + # + # This doesn't trigger any exceptions or store any errors if the variable doesn't exists. def []?(var : String) : Any? @data[var]? end diff --git a/src/liquid/exceptions.cr b/src/liquid/exceptions.cr index c2b7fdc..d735cf9 100644 --- a/src/liquid/exceptions.cr +++ b/src/liquid/exceptions.cr @@ -1,19 +1,43 @@ -# exceptions.cr - module Liquid + # Base class for all exceptions raised by Liquid.cr shard. class LiquidException < Exception end + # Exception raised on syntax errors. class SyntaxError < LiquidException + # Line number where the syntax error was found. property line_number : Int32 = -1 end + class InvalidStatement < LiquidException + end + + # Exception used for any non-fatal errors that can happen while rendering a liquid template. class InvalidExpression < LiquidException + def initialize(message : String) + super("Liquid error: #{message}") + end end - class InvalidStatement < LiquidException + # Error generated when a variable used in a template doesn't exists in the current context. + # + # This exception is never raised whatever the context error mode, to access it check `Context#errors`. + class UndefinedVariable < InvalidExpression + def initialize(var_name : String) + super("Variable \"#{var_name}\" not found.") + end + end + + # Error generated when a filter used in a template doesn't exists. + # + # This exception is only raised in `Context::ErrorMode::Strict` error mode. + class UndefinedFilter < InvalidExpression + def initialize(filter_name : String) + super("Undefined filter: #{filter_name}") + end end - class FilterArgumentException < LiquidException + # Exception raised by filters if something went wrong. + class FilterArgumentException < InvalidExpression end end diff --git a/src/liquid/expression.cr b/src/liquid/expression.cr index 73e9a81..c4b016b 100644 --- a/src/liquid/expression.cr +++ b/src/liquid/expression.cr @@ -96,7 +96,7 @@ module Liquid stack.push(Operator::Filter) stack.push(opcode.value) in .filter_option? - return raise?(ctx) { "Unexpected filter option: #{opcode.value}." } if filter_stack.nil? + return expression_error(ctx, "Unexpected filter option: #{opcode.value}.") if filter_stack.nil? stack.push(Operator::FilterOption) stack.push(opcode.value) @@ -108,9 +108,9 @@ module Liquid index = stack.pop var = stack.pop if index.is_a?(Operator) - return raise?(ctx) { "Unexpected operator: #{index}." } + return expression_error(ctx, "Unexpected operator: #{index}.") elsif var.is_a?(Operator) - return raise?(ctx) { "Unexpected operator: #{var}." } + return expression_error(ctx, "Unexpected operator: #{var}.") end retval = call_index(ctx, var, index.as(Any)) @@ -118,8 +118,8 @@ module Liquid end end - return raise?(ctx) { "Empty expression." } if code_stack.empty? - return raise?(ctx) { "Bad values left on stack." } if code_stack.size == 2 || !code_stack.first.is_a?(Any) + return expression_error(ctx, "Empty expression.") if code_stack.empty? + return expression_error(ctx, "Bad values left on stack.") if code_stack.size == 2 || !code_stack.first.is_a?(Any) result = code_stack.size == 1 ? code_stack.first.as(Any) : apply_operators(ctx, code_stack) apply_filters(ctx, result, filter_stack) @@ -130,10 +130,10 @@ module Liquid result = apply_binary_operator(ctx, stack) stack << result end - return raise?(ctx) { "Bad values left on stack." } if stack.size != 1 + return expression_error(ctx, "Bad values left on stack.") if stack.size != 1 result = stack.first.as?(Any) - return raise?(ctx) { "Bad values left on stack." } if result.nil? + return expression_error(ctx, "Bad values left on stack.") if result.nil? result end @@ -142,14 +142,14 @@ module Liquid right_operand = stack.pop operator = stack.pop left_operand = stack.pop - return raise?(ctx) { "Unexpected operator: #{operator}." } unless operator.is_a?(Operator) - return raise?(ctx) { "Unexpected left operand: #{left_operand}." } unless left_operand.is_a?(Any) - return raise?(ctx) { "Unexpected right operand: #{right_operand}." } unless right_operand.is_a?(Any) + return expression_error(ctx, "Unexpected operator: #{operator}.") unless operator.is_a?(Operator) + return expression_error(ctx, "Unexpected left operand: #{left_operand}.") unless left_operand.is_a?(Any) + return expression_error(ctx, "Unexpected right operand: #{right_operand}.") unless right_operand.is_a?(Any) result = operator.eval(left_operand, right_operand) Any.new(result) rescue e : InvalidExpression - raise?(ctx, e) + ctx.add_error(e) end private def apply_filters(ctx : Context, operand : Any, filter_stack : Stack?) : Any @@ -157,20 +157,25 @@ module Liquid while filter_stack.any? item = filter_stack.shift - return raise?(ctx) { "Expected a filter, got #{item}." } if !item.is_a?(Operator) || !item.filter? + return expression_error(ctx, "Expected a filter, got #{item}.") if !item.is_a?(Operator) || !item.filter? item = filter_stack.shift? - return raise?(ctx) { "Expected a filter name, got #{item}." } unless item.is_a?(Any) + return expression_error(ctx, "Expected a filter name, got #{item}.") unless item.is_a?(Any) filter_name = item.as_s filter = Filters::FilterRegister.get(filter_name) - return raise?(ctx) { "Unknown filter: #{filter_name}." } if filter.nil? + return ctx.add_error(UndefinedFilter.new(filter_name)) if filter.nil? setup_context_filter_args_and_options(ctx, filter_stack) operand = filter.filter(operand, ctx.filter_args, ctx.filter_options) end operand + rescue e : Liquid::FilterArgumentException + ctx.add_error(e) + + # Shopify liquid returns the filter error message instead of nil, so do we. + Any.new(e.message) end private def setup_context_filter_args_and_options(ctx : Context, stack : Stack) : Nil @@ -189,7 +194,7 @@ module Liquid elsif item.is_a?(Operator) name = stack.shift?.as?(Any) value = stack.shift?.as?(Any) - return raise?(ctx) { "Missing filter option name or value." } if name.nil? || value.nil? + return expression_error(ctx, "Missing filter option name or value.") if name.nil? || value.nil? filter_options[name.to_s] = value end @@ -201,12 +206,12 @@ module Liquid return call_hash_method(ctx, raw, index.as_s) if raw.is_a?(Hash) return call_drop_method(ctx, raw, index.as_s) if raw.is_a?(Drop) - return raise?(ctx) { "Tried to index a non-array object with \"#{index}\"." } unless raw.is_a?(Array) + return expression_error(ctx, "Tried to index a non-array object with \"#{index}\".") unless raw.is_a?(Array) i = index.as_i? - return raise?(ctx) { "Tried to index an array object with a #{raw.class.name}." } if i.nil? + return expression_error(ctx, "Tried to index an array object with a #{raw.class.name}.") if i.nil? - raw[i]? || raise?(ctx) { "\"Index out of bounds: #{i}." } + raw[i]? || Any.new(nil) end private def call_method(ctx : Context, obj : Any, method : String) : Any @@ -215,7 +220,7 @@ module Liquid return call_hash_method(ctx, raw, method) if raw.is_a?(Hash) return call_nil_method(ctx, method) if raw.is_a?(Nil) - return raise?(ctx) { "Tried to call ##{method} on a #{raw.class.name}." } if !raw.responds_to?(:size) + return expression_error(ctx, "Tried to call ##{method} on a #{raw.class.name}.") if !raw.responds_to?(:size) case method when "present", "present?" @@ -225,14 +230,14 @@ module Liquid when "size" Any.new(raw.size) else - raise?(ctx) { "Tried to access property \"#{method}\" of a non-hash/non-drop object." } + expression_error(ctx, "Tried to access property \"#{method}\" of a non-hash/non-drop object.") end end private def call_drop_method(ctx : Context, drop : Drop, method : String) : Any drop.call(method) rescue e : Liquid::InvalidExpression - raise?(ctx, e) + ctx.add_error(e) end private def call_hash_method(ctx : Context, hash : Hash, method : String) : Any @@ -245,7 +250,7 @@ module Liquid Any.new(hash.size) else value = hash[method]? - return raise?(ctx) { "Method \"#{method}\" not found." } if value.nil? + return expression_error(ctx, "Method \"#{method}\" not found.") if value.nil? value end @@ -258,24 +263,12 @@ module Liquid when "blank", "blank?" Any.new(true) else - raise?(ctx) { "Method \"#{method}\" not found for Nil." } + expression_error(ctx, "Method \"#{method}\" not found for Nil.") end end - private def raise?(ctx : Context) : Any - case ctx.error_mode - when .strict? then raise InvalidExpression.new(yield) - when .warn? then ctx.errors << yield - end - Any.new(nil) - end - - private def raise?(ctx : Context, exception : LiquidException) : Any - case ctx.error_mode - when .strict? then raise exception - when .warn? then ctx.errors << exception.message.to_s - end - Any.new(nil) + def expression_error(ctx : Context, message : String) + ctx.add_error(InvalidExpression.new(message)) end def to_s(io : IO) diff --git a/src/liquid/filters/abs.cr b/src/liquid/filters/abs.cr index b4c0512..f007673 100644 --- a/src/liquid/filters/abs.cr +++ b/src/liquid/filters/abs.cr @@ -16,7 +16,7 @@ module Liquid::Filters extend Filter def self.filter(data : Any, args : Array(Any), options : Hash(String, Any)) : Any - raise LiquidException.new("Unexpected argument for abs filter") if args && args.any? + raise FilterArgumentException.new("Unexpected argument for abs filter") if args && args.any? if data.raw.is_a? Number Any.new data.raw.as(Number).abs diff --git a/src/liquid/filters/append.cr b/src/liquid/filters/append.cr index 46bae9a..e930f51 100644 --- a/src/liquid/filters/append.cr +++ b/src/liquid/filters/append.cr @@ -23,8 +23,8 @@ module Liquid::Filters extend Filter def self.filter(data : Any, args : Array(Any), options : Hash(String, Any)) : Any - raise LiquidException.new("Missing argument for append filter") if args.nil? || args.empty? - raise LiquidException.new("Too many arguments for append filter") if args.size > 1 + raise FilterArgumentException.new("Missing argument for append filter") if args.nil? || args.empty? + raise FilterArgumentException.new("Too many arguments for append filter") if args.size > 1 Any.new(data.to_s + args.first.to_s) end diff --git a/src/liquid/filters/capitalize.cr b/src/liquid/filters/capitalize.cr index 616e2b8..3e5168b 100644 --- a/src/liquid/filters/capitalize.cr +++ b/src/liquid/filters/capitalize.cr @@ -20,7 +20,7 @@ module Liquid::Filters extend Filter def self.filter(data : Any, args : Array(Any), options : Hash(String, Any)) : Any - raise LiquidException.new("Unexpected argument for capitalize filter") if args && args.any? + raise FilterArgumentException.new("Unexpected argument for capitalize filter") if args && args.any? raw = data.raw if raw.is_a?(String) diff --git a/src/liquid/filters/ceil.cr b/src/liquid/filters/ceil.cr index e1f5694..ca81a3c 100644 --- a/src/liquid/filters/ceil.cr +++ b/src/liquid/filters/ceil.cr @@ -31,7 +31,7 @@ module Liquid::Filters extend Filter def self.filter(data : Any, args : Array(Any), options : Hash(String, Any)) : Any - raise LiquidException.new("Unexpected argument for ceil filter") if args && args.any? + raise FilterArgumentException.new("Unexpected argument for ceil filter") if args && args.any? if (raw = data.raw) && raw.is_a? Number Any.new(raw.ceil.to_i) diff --git a/src/liquid/filters/floor.cr b/src/liquid/filters/floor.cr index 2492c2d..37544f8 100644 --- a/src/liquid/filters/floor.cr +++ b/src/liquid/filters/floor.cr @@ -14,7 +14,7 @@ module Liquid::Filters extend Filter def self.filter(data : Any, args : Array(Any), options : Hash(String, Any)) : Any - raise LiquidException.new("Unexpected argument for floor filter") if args && args.any? + raise FilterArgumentException.new("Unexpected argument for floor filter") if args && args.any? if (raw = data.raw) && raw.is_a? Number Any.new(raw.floor.to_i) From 6bc8f4e23f3536f6b2944c5056fc3eb49cb70988 Mon Sep 17 00:00:00 2001 From: Hugo Parente Lima Date: Thu, 19 Jan 2023 11:28:47 -0300 Subject: [PATCH 2/2] Change text for UndefinedVariable exception. --- spec/liquid_spec.cr | 4 ++-- src/liquid/exceptions.cr | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/liquid_spec.cr b/spec/liquid_spec.cr index 47e39af..9dbfa26 100644 --- a/spec/liquid_spec.cr +++ b/spec/liquid_spec.cr @@ -27,14 +27,14 @@ describe Liquid::Context do it "does not raise for undefined variables on strict mode" do ctx = Context.new(:strict) ctx.get("missing").raw.should eq(nil) - ctx.errors.map(&.message).should eq([%(Liquid error: Variable "missing" not found.)]) + ctx.errors.map(&.message).should eq([%(Liquid error: Undefined variable: "missing".)]) ctx.errors.map(&.class).should eq([Liquid::UndefinedVariable]) end it "store errors for undefined variables in warn mode" do ctx = Context.new(:warn) ctx.get("missing").raw.should eq(nil) - ctx.errors.map(&.message).should eq([%(Liquid error: Variable "missing" not found.)]) + ctx.errors.map(&.message).should eq([%(Liquid error: Undefined variable: "missing".)]) ctx.errors.map(&.class).should eq([Liquid::UndefinedVariable]) end end diff --git a/src/liquid/exceptions.cr b/src/liquid/exceptions.cr index d735cf9..2f6f45f 100644 --- a/src/liquid/exceptions.cr +++ b/src/liquid/exceptions.cr @@ -24,7 +24,7 @@ module Liquid # This exception is never raised whatever the context error mode, to access it check `Context#errors`. class UndefinedVariable < InvalidExpression def initialize(var_name : String) - super("Variable \"#{var_name}\" not found.") + super("Undefined variable: \"#{var_name}\".") end end