From c9be96b58d9de731450d4ee16ef43fd380dcef41 Mon Sep 17 00:00:00 2001 From: Alan Tan Date: Thu, 12 Dec 2019 16:21:09 +0800 Subject: [PATCH] Improve error message of `Liquid::MemoryLimit`. The existing error message is too generic since there are three types of limits in place. It is useful to know which limit was reached to make it easier to debug the error. --- History.md | 1 + lib/liquid/block_body.rb | 14 +++++++++++-- lib/liquid/errors.rb | 20 ++++++++++++------ lib/liquid/resource_limits.rb | 14 +++++++++---- test/integration/template_test.rb | 34 +++++++++++++++---------------- 5 files changed, 54 insertions(+), 29 deletions(-) diff --git a/History.md b/History.md index 4781b23..9570fc0 100644 --- a/History.md +++ b/History.md @@ -4,6 +4,7 @@ * Split Strainer class as a factory and a template (#1208) [Thierry Joyal] * Remove handling of a nil context in the Strainer class (#1218) [Thierry Joyal] +* Change `Liquid::MemoryError` message to be more specific about which limit was reached. (#1206) [Alan Tan] ## 4.0.3 / 2019-03-12 diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 467bbdd..d2b7e07 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -192,8 +192,18 @@ module Liquid def raise_if_resource_limits_reached(context, length) context.resource_limits.render_length += length - return unless context.resource_limits.reached? - raise MemoryError, "Memory limits exceeded" + + error_message = + if context.resource_limits.render_length_reached? + MemoryError::RENDER_LENGTH_ERROR_MESSAGE + elsif context.resource_limits.render_score_reached? + MemoryError::RENDER_SCORE_ERROR_MESSAGE + elsif context.resource_limits.assign_score_reached? + MemoryError::ASSIGN_SCORE_ERROR_MESSAGE + end + + return unless error_message + raise MemoryError, error_message end def create_variable(token, parse_context) diff --git a/lib/liquid/errors.rb b/lib/liquid/errors.rb index 4937da3..aee3ec6 100644 --- a/lib/liquid/errors.rb +++ b/lib/liquid/errors.rb @@ -23,11 +23,14 @@ module Liquid def message_prefix str = +"" - str << if is_a?(SyntaxError) - "Liquid syntax error" - else - "Liquid error" - end + str << + if is_a?(SyntaxError) + "Liquid syntax error" + elsif is_a?(MemoryError) + "Liquid memory limit error" + else + "Liquid error" + end if line_number str << " (" @@ -40,6 +43,12 @@ module Liquid end end + class MemoryError < Error + RENDER_LENGTH_ERROR_MESSAGE = 'Too many bytes rendered.' + RENDER_SCORE_ERROR_MESSAGE = 'Too many tags rendered.' + ASSIGN_SCORE_ERROR_MESSAGE = 'Too many bytes assigned to variables.' + end + ArgumentError = Class.new(Error) ContextError = Class.new(Error) FileSystemError = Class.new(Error) @@ -47,7 +56,6 @@ module Liquid SyntaxError = Class.new(Error) StackLevelError = Class.new(Error) TaintedError = Class.new(Error) - MemoryError = Class.new(Error) ZeroDivisionError = Class.new(Error) FloatDomainError = Class.new(Error) UndefinedVariable = Class.new(Error) diff --git a/lib/liquid/resource_limits.rb b/lib/liquid/resource_limits.rb index 9c898f0..46f3d16 100644 --- a/lib/liquid/resource_limits.rb +++ b/lib/liquid/resource_limits.rb @@ -12,10 +12,16 @@ module Liquid reset end - def reached? - (@render_length_limit && @render_length > @render_length_limit) || - (@render_score_limit && @render_score > @render_score_limit) || - (@assign_score_limit && @assign_score > @assign_score_limit) + def render_length_reached? + @render_length_limit && @render_length > @render_length_limit + end + + def render_score_reached? + @render_score_limit && @render_score > @render_score_limit + end + + def assign_score_reached? + @assign_score_limit && @assign_score > @assign_score_limit end def reset diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index f5588ed..4a11775 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -112,8 +112,8 @@ class TemplateTest < Minitest::Test def test_resource_limits_render_length t = Template.parse("0123456789") t.resource_limits.render_length_limit = 5 - assert_equal("Liquid error: Memory limits exceeded", t.render) - assert(t.resource_limits.reached?) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_LENGTH_ERROR_MESSAGE}", t.render) + assert(t.resource_limits.render_length_reached?) t.resource_limits.render_length_limit = 10 assert_equal("0123456789", t.render!) @@ -123,13 +123,13 @@ class TemplateTest < Minitest::Test def test_resource_limits_render_score t = Template.parse("{% for a in (1..10) %} {% for a in (1..10) %} foo {% endfor %} {% endfor %}") t.resource_limits.render_score_limit = 50 - assert_equal("Liquid error: Memory limits exceeded", t.render) - assert(t.resource_limits.reached?) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_SCORE_ERROR_MESSAGE}", t.render) + assert(t.resource_limits.render_score_reached?) t = Template.parse("{% for a in (1..100) %} foo {% endfor %}") t.resource_limits.render_score_limit = 50 - assert_equal("Liquid error: Memory limits exceeded", t.render) - assert(t.resource_limits.reached?) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_SCORE_ERROR_MESSAGE}", t.render) + assert(t.resource_limits.render_score_reached?) t.resource_limits.render_score_limit = 200 assert_equal((" foo " * 100), t.render!) @@ -139,8 +139,8 @@ class TemplateTest < Minitest::Test def test_resource_limits_assign_score t = Template.parse("{% assign foo = 42 %}{% assign bar = 23 %}") t.resource_limits.assign_score_limit = 1 - assert_equal("Liquid error: Memory limits exceeded", t.render) - assert(t.resource_limits.reached?) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::ASSIGN_SCORE_ERROR_MESSAGE}", t.render) + assert(t.resource_limits.assign_score_reached?) t.resource_limits.assign_score_limit = 2 assert_equal("", t.render!) @@ -161,8 +161,8 @@ class TemplateTest < Minitest::Test t = Template.parse("{% assign foo = 'aaaa' | reverse %}") t.resource_limits.assign_score_limit = 3 - assert_equal("Liquid error: Memory limits exceeded", t.render) - assert(t.resource_limits.reached?) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::ASSIGN_SCORE_ERROR_MESSAGE}", t.render) + assert(t.resource_limits.assign_score_reached?) t.resource_limits.assign_score_limit = 5 assert_equal("", t.render!) @@ -171,8 +171,8 @@ class TemplateTest < Minitest::Test def test_resource_limits_aborts_rendering_after_first_error t = Template.parse("{% for a in (1..100) %} foo1 {% endfor %} bar {% for a in (1..100) %} foo2 {% endfor %}") t.resource_limits.render_score_limit = 50 - assert_equal("Liquid error: Memory limits exceeded", t.render) - assert(t.resource_limits.reached?) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_SCORE_ERROR_MESSAGE}", t.render) + assert(t.resource_limits.render_score_reached?) end def test_resource_limits_hash_in_template_gets_updated_even_if_no_limits_are_set @@ -186,21 +186,21 @@ class TemplateTest < Minitest::Test def test_render_length_persists_between_blocks t = Template.parse("{% if true %}aaaa{% endif %}") t.resource_limits.render_length_limit = 7 - assert_equal("Liquid error: Memory limits exceeded", t.render) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_LENGTH_ERROR_MESSAGE}", t.render) t.resource_limits.render_length_limit = 8 assert_equal("aaaa", t.render) t = Template.parse("{% if true %}aaaa{% endif %}{% if true %}bbb{% endif %}") t.resource_limits.render_length_limit = 13 - assert_equal("Liquid error: Memory limits exceeded", t.render) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_LENGTH_ERROR_MESSAGE}", t.render) t.resource_limits.render_length_limit = 14 assert_equal("aaaabbb", t.render) t = Template.parse("{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}") t.resource_limits.render_length_limit = 5 - assert_equal("Liquid error: Memory limits exceeded", t.render) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_LENGTH_ERROR_MESSAGE}", t.render) t.resource_limits.render_length_limit = 11 - assert_equal("Liquid error: Memory limits exceeded", t.render) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_LENGTH_ERROR_MESSAGE}", t.render) t.resource_limits.render_length_limit = 12 assert_equal("ababab", t.render) end @@ -208,7 +208,7 @@ class TemplateTest < Minitest::Test def test_render_length_uses_number_of_bytes_not_characters t = Template.parse("{% if true %}すごい{% endif %}") t.resource_limits.render_length_limit = 10 - assert_equal("Liquid error: Memory limits exceeded", t.render) + assert_equal("Liquid memory limit error: #{Liquid::MemoryError::RENDER_LENGTH_ERROR_MESSAGE}", t.render) t.resource_limits.render_length_limit = 18 assert_equal("すごい", t.render) end