From b8958f626dc48f38bfe2d4fede09e78a2af403a1 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 8 Oct 2025 15:42:17 -0400 Subject: [PATCH] Stricter 1:1 refactor of strict_parse for Variable --- lib/liquid/parser_switching.rb | 10 ++++-- lib/liquid/variable.rb | 60 +++++++++++++++++++++++++++------ test/unit/variable_unit_test.rb | 48 +++++++++++++++++++++++--- 3 files changed, 101 insertions(+), 17 deletions(-) diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 799250a0..c6b09bc8 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -5,11 +5,17 @@ module Liquid # Do not use this. # # It's basically doing the same thing the {#parse_with_selected_parser}, - # except this will use the strict parser, instead of the rigid 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. # # @deprecated Use {#parse_with_selected_parser} instead. def strict_parse_with_error_mode_fallback(markup) - strict_parse_with_error_context(markup) + case parse_context.error_mode + when :rigid + rigid_parse_with_error_context(markup) + else + strict_parse_with_error_context(markup) + end rescue SyntaxError => e case parse_context.error_mode when :rigid diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index a3623bc5..f355d694 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -54,7 +54,7 @@ module Liquid next unless f =~ /\w+/ filtername = Regexp.last_match(0) filterargs = f.scan(FilterArgsRegex).flatten - @filters << parse_filter_expressions(filtername, filterargs) + @filters << lax_parse_filter_expressions(filtername, filterargs) end end end @@ -66,14 +66,14 @@ module Liquid return if p.look(:end_of_string) @name = parse_context.safe_parse_expression(p) - while p.consume?(:pipe) - filtername = p.consume(:id) - filterargs = p.consume?(:colon) ? parse_filterargs(p) : Const::EMPTY_ARRAY - @filters << parse_filter_expressions(filtername, filterargs, safe: true) - end + @filters << strict_parse_filter_expressions(p) while p.consume?(:pipe) p.consume(:end_of_string) end + def rigid_parse(markup) + strict_parse(markup) + end + def parse_filterargs(p) # first argument filterargs = [p.argument] @@ -122,15 +122,15 @@ module Liquid private - def parse_filter_expressions(filter_name, unparsed_args, safe: false) + def lax_parse_filter_expressions(filter_name, unparsed_args) filter_args = [] keyword_args = nil unparsed_args.each do |a| - if (matches = a.match(JustTagAttributes)) # we'll need to fix this + if (matches = a.match(JustTagAttributes)) keyword_args ||= {} - keyword_args[matches[1]] = parse_context.parse_expression(matches[2], safe: false) + keyword_args[matches[1]] = parse_context.parse_expression(matches[2]) else - filter_args << parse_context.parse_expression(a, safe: safe) + filter_args << parse_context.parse_expression(a) end end result = [filter_name, filter_args] @@ -138,6 +138,46 @@ module Liquid result end + # Surprisingly, positional and keyword arguments can be mixed. + # + # filter = filtername [":" filterargs?] + # filterargs = argument ("," argument)* + # argument = (positional_argument | keyword_argument) + # positional_argument = expression + # keyword_argument = id ":" expression + def strict_parse_filter_expressions(p) + filtername = p.consume(:id) + filter_args = [] + keyword_args = {} + + if p.consume?(:colon) + # Parse first argument (no leading comma) + argument(p, filter_args, keyword_args) unless end_of_arguments?(p) + + # Parse remaining arguments (with leading commas) and optional trailing comma + argument(p, filter_args, keyword_args) while p.consume?(:comma) && !end_of_arguments?(p) + end + + result = [filtername, filter_args] + result << keyword_args unless keyword_args.empty? + result + end + + def argument(p, positional_arguments, keyword_arguments) + if p.look(:id) && p.look(:colon, 1) + key = p.consume(:id) + p.consume(:colon) + value = parse_context.safe_parse_expression(p) + keyword_arguments[key] = value + else + positional_arguments << parse_context.safe_parse_expression(p) + end + end + + def end_of_arguments?(p) + p.look(:pipe) || p.look(:end_of_string) + end + def evaluate_filter_expressions(context, filter_args, filter_kwargs) parsed_args = filter_args.map { |expr| context.evaluate(expr) } if filter_kwargs diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 2cc42e7d..c8e572c9 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -135,14 +135,52 @@ class VariableUnitTest < Minitest::Test 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) + + # missing does not throws error + create_variable(%(n | f1: ,), error_mode: :lax) + create_variable(%(n | f1: ,| f2), error_mode: :lax) + + # arg does not require colon, but ignores args :O, also ignores first kwarg since it splits on ':' + var = create_variable(%(n | f1 1 | f2 k1: v1), error_mode: :lax) + assert_equal([['f1', []], ['f2', [VariableLookup.new('v1')]]], var.filters) + + # positional and kwargs parsing + var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2), error_mode: :lax) + assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) + + # positional and kwargs intermixed (pos1, key1: val1, pos2) + var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title"), error_mode: :lax) + assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters) end def test_strict_filter_argument_parsing - with_error_mode(:strict) do - assert_raises(SyntaxError) do - create_variable(%( number_of_comments | pluralize: 'comment': 'comments' )) - end - end + # optional colon + var = create_variable(%(n | f1 | f2:), error_mode: :strict) + assert_equal([['f1', []], ['f2', []]], var.filters) + + # missing argument throws error + assert_raises(SyntaxError) { create_variable(%(n | f1: ,), error_mode: :strict) } + assert_raises(SyntaxError) { create_variable(%(n | f1: ,| f2), error_mode: :strict) } + + # arg requires colon + assert_raises(SyntaxError) { create_variable(%(n | f1 1), error_mode: :strict) } + + # trailing comma doesn't throw + create_variable(%(n | f1: 1, 2, 3, | f2:), error_mode: :strict) + + # missing comma throws error + assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3), error_mode: :strict) } + + # positional and kwargs parsing + var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2), error_mode: :strict) + assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) + + # positional and kwargs intermixed (pos1, key1: val1, pos2) + var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title"), error_mode: :strict) + assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters) + + # string key throws + assert_raises(SyntaxError) { create_variable(%(n | pluralize: 'comment': 'comments'), error_mode: :strict) } end def test_output_raw_source_of_variable