From 738540a601c5b012af2bb594076be224bab1811e Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 11:48:12 +0200 Subject: [PATCH] Update infrastructure that handles parsing switching: * Remove development helpers from parse context * Simplify strict_parse_with_error_mode_fallback and update documentation * Add unit tests for `Liquid::Expression` and `Liquid::ParseContext` * Update test helpers to work better with the `:rigid` mode --- lib/liquid/parse_context.rb | 11 ++- lib/liquid/parser_switching.rb | 12 ++- test/integration/expression_test.rb | 33 ++++++- test/test_helper.rb | 8 +- test/unit/parse_context_unit_test.rb | 123 +++++++++++++++++++++++++++ 5 files changed, 170 insertions(+), 17 deletions(-) create mode 100644 test/unit/parse_context_unit_test.rb diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 1b192d8c..47612339 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -55,10 +55,15 @@ module Liquid end def parse_expression(markup, safe: false) - # todo(guilherme): remove this once rigid mode is fully using safe_parse_expression if !safe && @error_mode == :rigid - # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" - puts("🚨 parse_expression used in rigid mode") + # parse_expression is a widely used API. To maintain backward + # compatibility while raising awareness about rigid parser standards, + # the safe flag supports API users make a deliberate decision. + # + # In rigid mode, markup MUST come from a string returned by the parser + # (e.g., parser.expression). We're not calling the parser here to + # prevent redundant parser overhead. + raise Liquid::InternalError, "unsafe parse_expression cannot be used in rigid mode" end Expression.parse(markup, @string_scanner, @expression_cache) diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index c6b09bc8..7c662e92 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -6,16 +6,14 @@ module Liquid # # It's basically doing the same thing the {#parse_with_selected_parser}, # except this will try the strict parser regardless of the error mode, - # and fall back to the lax parser if the error mode is lax or warn. + # and fall back to the lax parser if the error mode is lax or warn, + # except when in rigid mode where it uses the rigid parser. # # @deprecated Use {#parse_with_selected_parser} instead. def strict_parse_with_error_mode_fallback(markup) - case parse_context.error_mode - when :rigid - rigid_parse_with_error_context(markup) - else - strict_parse_with_error_context(markup) - end + return rigid_parse_with_error_context(markup) if rigid_mode? + + strict_parse_with_error_context(markup) rescue SyntaxError => e case parse_context.error_mode when :rigid diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 71969900..fe54bd99 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -71,7 +71,6 @@ class ExpressionTest < Minitest::Test def test_expression_cache skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled - skip("Rigid mode does not use Expression caching") if Liquid::Environment.default.error_mode == :rigid cache = {} template = <<~LIQUID @@ -93,7 +92,6 @@ class ExpressionTest < Minitest::Test def test_expression_cache_with_true_boolean skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled - skip("Rigid mode does not use Expression caching") if Liquid::Environment.default.error_mode == :rigid template = <<~LIQUID {% assign x = 1 %} @@ -118,7 +116,6 @@ class ExpressionTest < Minitest::Test def test_expression_cache_with_lru_redux skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled - skip("Rigid mode does not use Expression caching") if Liquid::Environment.default.error_mode == :rigid cache = LruRedux::Cache.new(10) template = <<~LIQUID @@ -140,7 +137,6 @@ class ExpressionTest < Minitest::Test def test_disable_expression_cache skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled - skip("Rigid mode does not use Expression caching") if Liquid::Environment.default.error_mode == :rigid template = <<~LIQUID {% assign x = 1 %} @@ -156,6 +152,35 @@ class ExpressionTest < Minitest::Test assert(parse_context.instance_variable_get(:@expression_cache).nil?) end + def test_safe_parse_with_variable_lookup + parse_context = Liquid::ParseContext.new + parser = parse_context.new_parser('product.title') + result = Liquid::Expression.safe_parse(parser) + + assert_instance_of(Liquid::VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + + def test_safe_parse_with_number + parse_context = Liquid::ParseContext.new + parser = parse_context.new_parser('42') + result = Liquid::Expression.safe_parse(parser) + + assert_equal(42, result) + end + + def test_safe_parse_raises_syntax_error_for_invalid_expression + parse_context = Liquid::ParseContext.new + parser = parse_context.new_parser('') + + error = assert_raises(Liquid::SyntaxError) do + Liquid::Expression.safe_parse(parser) + end + + assert_match(/is not a valid expression/, error.message) + end + private def assert_expression_result(expect, markup, **assigns) diff --git a/test/test_helper.rb b/test/test_helper.rb index 69172d47..a09f2277 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -82,10 +82,12 @@ module Minitest Environment.dangerously_override(environment, &blk) end - def with_error_mode(mode) + def with_error_mode(*modes) old_mode = Liquid::Environment.default.error_mode - Liquid::Environment.default.error_mode = mode - yield + modes.each do |mode| + Liquid::Environment.default.error_mode = mode + yield + end ensure Liquid::Environment.default.error_mode = old_mode end diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb new file mode 100644 index 00000000..b1c8fe47 --- /dev/null +++ b/test/unit/parse_context_unit_test.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ParseContextUnitTest < Minitest::Test + include Liquid + + def test_safe_parse_expression_with_variable_lookup + parser_strict = strict_parse_context.new_parser('product.title') + result_strict = strict_parse_context.safe_parse_expression(parser_strict) + + parser_rigid = rigid_parse_context.new_parser('product.title') + result_rigid = rigid_parse_context.safe_parse_expression(parser_rigid) + + assert_instance_of(VariableLookup, result_strict) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + assert_instance_of(VariableLookup, result_rigid) + assert_equal('product', result_rigid.name) + assert_equal(['title'], result_rigid.lookups) + end + + def test_safe_parse_expression_raises_syntax_error_for_invalid_expression + parser_strict = strict_parse_context.new_parser('') + parser_rigid = rigid_parse_context.new_parser('') + + error_strict = assert_raises(Liquid::SyntaxError) do + strict_parse_context.safe_parse_expression(parser_strict) + end + assert_match(/is not a valid expression/, error_strict.message) + + error_rigid = assert_raises(Liquid::SyntaxError) do + rigid_parse_context.safe_parse_expression(parser_rigid) + end + + assert_match(/is not a valid expression/, error_rigid.message) + end + + def test_parse_expression_with_variable_lookup + result_strict = strict_parse_context.parse_expression('product.title') + + assert_instance_of(VariableLookup, result_strict) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + error = assert_raises(Liquid::InternalError) do + rigid_parse_context.parse_expression('product.title') + end + + assert_match(/unsafe parse_expression cannot be used in rigid mode/, error.message) + end + + def test_parse_expression_with_safe_true + result_strict = strict_parse_context.parse_expression('product.title', safe: true) + + assert_instance_of(VariableLookup, result_strict) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + result_rigid = rigid_parse_context.parse_expression('product.title', safe: true) + + assert_instance_of(VariableLookup, result_rigid) + assert_equal('product', result_rigid.name) + assert_equal(['title'], result_rigid.lookups) + end + + def test_parse_expression_with_empty_string + result_strict = strict_parse_context.parse_expression('') + assert_nil(result_strict) + + error = assert_raises(Liquid::InternalError) do + rigid_parse_context.parse_expression('') + end + + assert_match(/unsafe parse_expression cannot be used in rigid mode/, error.message) + end + + def test_parse_expression_with_empty_string_and_safe_true + result_strict = strict_parse_context.parse_expression('', safe: true) + assert_nil(result_strict) + + result_rigid = rigid_parse_context.parse_expression('', safe: true) + assert_nil(result_rigid) + end + + def test_safe_parse_expression_advances_parser_pointer + parser = rigid_parse_context.new_parser('foo, bar') + + # safe_parse_expression consumes "foo" + first_result = rigid_parse_context.safe_parse_expression(parser) + assert_instance_of(VariableLookup, first_result) + assert_equal('foo', first_result.name) + + parser.consume(:comma) + + # safe_parse_expression consumes "bar" + second_result = rigid_parse_context.safe_parse_expression(parser) + assert_instance_of(VariableLookup, second_result) + assert_equal('bar', second_result.name) + + parser.consume(:end_of_string) + end + + def test_parse_expression_with_whitespace_in_rigid_mode + result = rigid_parse_context.parse_expression(' ', safe: true) + assert_nil(result) + end + + private + + def strict_parse_context + @strict_parse_context ||= ParseContext.new( + environment: Environment.build(error_mode: :strict), + ) + end + + def rigid_parse_context + @rigid_parse_context ||= ParseContext.new( + environment: Environment.build(error_mode: :rigid), + ) + end +end