From beaea6ffa8aac9ad77e92445fc12fb04ac791634 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Tue, 30 Sep 2025 14:31:44 +0200 Subject: [PATCH 01/41] Introduce :rigid parsing mode --- README.md | 1 + Rakefile | 10 +- bin/example.liquid | 5 + bin/render | 30 +++ lib/liquid/environment.rb | 2 +- lib/liquid/parse_context.rb | 20 ++ lib/liquid/parser_switching.rb | 3 + test/unit/partial_cache_unit_test.rb | 4 +- test/unit/rigid_mode_unit_test.rb | 264 +++++++++++++++++++++++++++ 9 files changed, 335 insertions(+), 4 deletions(-) create mode 100644 bin/example.liquid create mode 100755 bin/render create mode 100644 test/unit/rigid_mode_unit_test.rb diff --git a/README.md b/README.md index 3734a290f..93a726a91 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,7 @@ Liquid::Environment.default.error_mode = :strict Liquid::Environment.default.error_mode = :strict # Raises a SyntaxError when invalid syntax is used Liquid::Environment.default.error_mode = :warn # Adds strict errors to template.errors but continues as normal Liquid::Environment.default.error_mode = :lax # The default mode, accepts almost anything. +Liquid::Environment.default.error_mode = :rigid # Uses Parser.new instead of Expression.parse for stricter parsing ``` If you want to set the error mode only on specific templates you can pass `:error_mode` as an option to `parse`: diff --git a/Rakefile b/Rakefile index 6ccd2c866..ae41567a3 100755 --- a/Rakefile +++ b/Rakefile @@ -33,7 +33,7 @@ task :rubocop do end end -desc('runs test suite with both strict and lax parsers') +desc('runs test suite with all parsers (lax, strict, and rigid)') task :test do ENV['LIQUID_PARSER_MODE'] = 'lax' Rake::Task['base_test'].invoke @@ -42,6 +42,10 @@ task :test do Rake::Task['base_test'].reenable Rake::Task['base_test'].invoke + ENV['LIQUID_PARSER_MODE'] = 'rigid' + Rake::Task['base_test'].reenable + Rake::Task['base_test'].invoke + if RUBY_ENGINE == 'ruby' || RUBY_ENGINE == 'truffleruby' ENV['LIQUID_PARSER_MODE'] = 'lax' Rake::Task['integration_test'].reenable @@ -50,6 +54,10 @@ task :test do ENV['LIQUID_PARSER_MODE'] = 'strict' Rake::Task['integration_test'].reenable Rake::Task['integration_test'].invoke + + ENV['LIQUID_PARSER_MODE'] = 'rigid' + Rake::Task['integration_test'].reenable + Rake::Task['integration_test'].invoke end end diff --git a/bin/example.liquid b/bin/example.liquid new file mode 100644 index 000000000..c4a93aa3d --- /dev/null +++ b/bin/example.liquid @@ -0,0 +1,5 @@ + + {% tablerow i in (1..10) limit: foo=>bar %} + {{ i }} + {% endtablerow %} +
diff --git a/bin/render b/bin/render new file mode 100755 index 000000000..004c103f0 --- /dev/null +++ b/bin/render @@ -0,0 +1,30 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +require 'bundler/setup' +require 'liquid' + +class VirtualFileSystem + def initialize + snippet_1 = '

{{ greating | default: "Hello" }}, {{ name | default: "world" }}!

' + snippet_2 = '{% for i in (1..5) %} > {{ i }}{% endfor %}' + + @templates = { + 'snippet_1' => snippet_1, + 'snippet_2' => snippet_2, + } + end + + def read_template_file(key) + @templates[key] || raise(Liquid::FileSystemError, "No such template '#{key}'") + end +end + +error_mode = :strict +# error_mode = :rigid +file = File.read(ARGV[0]) +template = Liquid::Template.parse(file, error_mode: error_mode) + +template.registers[:file_system] = VirtualFileSystem.new + +puts template.render diff --git a/lib/liquid/environment.rb b/lib/liquid/environment.rb index 31b17b234..987ec13f9 100644 --- a/lib/liquid/environment.rb +++ b/lib/liquid/environment.rb @@ -34,7 +34,7 @@ class << self # @param file_system The default file system that is used # to load templates from. # @param error_mode [Symbol] The default error mode for all templates - # (either :strict, :warn, or :lax). + # (either :strict, :warn, :lax, or :rigid). # @param exception_renderer [Proc] The exception renderer that is used to # render exceptions. # @yieldparam environment [Environment] The environment instance that is being built. diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 60cdf9e41..277d349e9 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -51,6 +51,26 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) end def parse_expression(markup) + if @error_mode == :rigid + parser = new_parser(markup) + + # Return nil immediately if the markup is empty or contains only + # whitespaces + return if parser.look(:end_of_string) + + expression_string = parser.expression + + # In rigid mode, verify that all tokens have been consumed + # + # Extra tokens remaining after the expression indicate invalid syntaxes, + # such as: "product title" (instead of "product.title") + parser.consume(:end_of_string) unless parser.look(:end_of_string) + + # Use Parser for strict token validation, but still return + # Expression objects for compatibility with the rendering pipeline. + markup = expression_string + end + Expression.parse(markup, @string_scanner, @expression_cache) end diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 78afd58a9..e10476329 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -8,6 +8,8 @@ def strict_parse_with_error_mode_fallback(markup) case parse_context.error_mode when :strict raise + when :rigid + raise when :warn parse_context.warnings << e end @@ -17,6 +19,7 @@ def strict_parse_with_error_mode_fallback(markup) def parse_with_selected_parser(markup) case parse_context.error_mode when :strict then strict_parse_with_error_context(markup) + when :rigid then strict_parse_with_error_context(markup) when :lax then lax_parse(markup) when :warn begin diff --git a/test/unit/partial_cache_unit_test.rb b/test/unit/partial_cache_unit_test.rb index b081afe52..31790b0d8 100644 --- a/test/unit/partial_cache_unit_test.rb +++ b/test/unit/partial_cache_unit_test.rb @@ -184,7 +184,7 @@ def test_includes_error_mode_into_template_cache }, ) - [:lax, :warn, :strict].each do |error_mode| + [:lax, :warn, :strict, :rigid].each do |error_mode| Liquid::PartialCache.load( 'my_partial', context: context, @@ -193,7 +193,7 @@ def test_includes_error_mode_into_template_cache end assert_equal( - ["my_partial:lax", "my_partial:warn", "my_partial:strict"], + ["my_partial:lax", "my_partial:warn", "my_partial:strict", "my_partial:rigid"], context.registers[:cached_partials].keys, ) end diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb new file mode 100644 index 000000000..d1cd615ed --- /dev/null +++ b/test/unit/rigid_mode_unit_test.rb @@ -0,0 +1,264 @@ +# frozen_string_literal: true + +require 'test_helper' + +class RigidModeUnitTest < Minitest::Test + include Liquid + + def test_direct_parse_expression_comparison + test_cases = [ + 'foo bar', + 'user.name first', + 'items[0] next', + 'products[0].name extra', + ] + + test_cases.each do |expr| + ctx_strict = ParseContext.new(environment: strict_env) + result = ctx_strict.parse_expression(expr) + refute_nil(result, "Strict mode should parse '#{expr}'") + + ctx_rigid = ParseContext.new(environment: rigid_env) + error = assert_raises(SyntaxError) do + ctx_rigid.parse_expression(expr) + end + + assert_match(/Expected end_of_string but found id/, error.message) + end + end + + def test_comparison_strict_vs_rigid_with_space_separated_lookups + expr = 'product title' + + ctx_lax = ParseContext.new(environment: lax_env) + result_lax = ctx_lax.parse_expression(expr) + assert_equal('product', result_lax.name) + assert_equal(['title'], result_lax.lookups) + + ctx_strict = ParseContext.new(environment: strict_env) + result_strict = ctx_strict.parse_expression(expr) + assert_equal('product', result_strict.name) + assert_equal(['title'], result_strict.lookups) + + ctx_rigid = ParseContext.new(environment: rigid_env) + assert_raises(SyntaxError) do + ctx_rigid.parse_expression(expr) + end + end + + def test_tablerow_limit_with_invalid_expression + template = <<~LIQUID + {% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %} + LIQUID + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_tablerow_offset_with_invalid_expression + template = <<~LIQUID + {% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %} + LIQUID + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_cycle_name_with_invalid_expression + template = <<~LIQUID + {% for i in (1..3) %} + {% cycle foo=>bar: "a", "b" %} + {% endfor %} + LIQUID + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_cycle_variable_with_invalid_expression + template = <<~LIQUID + {% for i in (1..3) %} + {% cycle foo=>bar, "a", "b" %} + {% endfor %} + LIQUID + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_case_with_invalid_expression + template = <<~LIQUID + {% case foo=>bar %} + {% when 1 %} + one + {% endcase %} + LIQUID + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_include_template_with_invalid_expression + template = "{% include foo=>bar %}" + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_include_with_invalid_expression + template = '{% include "snippet" with foo=>bar %}' + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_include_attribute_with_invalid_expression + template = '{% include "snippet", key: foo=>bar %}' + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_render_with_invalid_expression + template = '{% render "snippet" with foo=>bar %}' + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_render_attribute_with_invalid_expression + template = '{% render "snippet", key: foo=>bar %}' + + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + assert_match(/Unexpected character =/, error.message) + end + + def test_valid_expressions_work_in_rigid_mode + test_cases = { + '{{ foo }}' => { 'foo' => 'bar' }, + '{{ foo.bar }}' => { 'foo' => { 'bar' => 'baz' } }, + '{{ items[0] }}' => { 'items' => ['first', 'second'] }, + '{{ product.variants[0].title }}' => { 'product' => { 'variants' => [{ 'title' => 'Small' }] } }, + '{{ "hello" }}' => {}, + '{{ 42 }}' => {}, + '{{ 3.14 }}' => {}, + } + + test_cases.each do |template_str, data| + t = rigid_parse(template_str) + result = t.render(data) + assert(result.is_a?(String), "Should render successfully for '#{template_str}'") + end + end + + def test_rigid_mode_with_ranges + template = <<~LIQUID + {% for i in (1..3) %}{{ i }}{% endfor %} + LIQUID + + t = rigid_parse(template) + result = t.render + assert_equal("123\n", result) + end + + def test_rigid_mode_with_variable_ranges + template = <<~LIQUID + {% for i in (start..end) %}{{ i }}{% endfor %} + LIQUID + + t = rigid_parse(template) + result = t.render({ 'start' => 1, 'end' => 3 }) + assert_equal("123\n", result) + end + + def test_rigid_mode_valid_filters + template = <<~LIQUID + {{ "hello" | upcase | prepend: "Say: " }} + LIQUID + + t = rigid_parse(template) + result = t.render + assert_equal("Say: HELLO\n", result) + end + + def test_rigid_mode_valid_filter_with_correct_variable_args + template = <<~LIQUID + {{ "hello" | append: world.name }} + LIQUID + + t = rigid_parse(template) + result = t.render({ 'world' => { 'name' => ' world' } }) + assert_equal("hello world\n", result) + end + + def test_empty_expression_handling + ctx_rigid = ParseContext.new(environment: rigid_env) + result = ctx_rigid.parse_expression('') + assert_nil(result) + + result = ctx_rigid.parse_expression(' ') + assert_nil(result) + end + + private + + def rigid_parse(source) + Template.parse(source, environment: rigid_env) + end + + def strict_parse(source) + Template.parse(source, environment: strict_env) + end + + def lax_env + Environment.build(error_mode: :lax) + end + + def rigid_env + Environment.build(error_mode: :rigid) + end + + def strict_env + Environment.build(error_mode: :strict) + end +end From 69a807eee8959f9e05a115e1529d83e54a3dfd8a Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 30 Sep 2025 15:45:30 -0400 Subject: [PATCH 02/41] Add rigid_parse_with_error_context and clarifications --- lib/liquid/parser_switching.rb | 17 ++++++++++++++--- lib/liquid/template.rb | 1 + 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index e10476329..a978dc43a 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -2,14 +2,17 @@ module Liquid module ParserSwitching + # Do not use this. Use parse_with_selected_parser instead. + # It's basically doing the same thing, except this will use strict_parse regardless + # of the error mode and fallback only if strict throws. def strict_parse_with_error_mode_fallback(markup) strict_parse_with_error_context(markup) rescue SyntaxError => e case parse_context.error_mode - when :strict - raise when :rigid raise + when :strict + raise when :warn parse_context.warnings << e end @@ -18,8 +21,8 @@ def strict_parse_with_error_mode_fallback(markup) def parse_with_selected_parser(markup) case parse_context.error_mode + when :rigid then rigid_parse_with_error_context(markup) when :strict then strict_parse_with_error_context(markup) - when :rigid then strict_parse_with_error_context(markup) when :lax then lax_parse(markup) when :warn begin @@ -33,6 +36,14 @@ def parse_with_selected_parser(markup) private + def rigid_parse_with_error_context(markup) + respond_to?(:rigid_parse) ? rigid_parse(markup) : strict_parse(markup) + rescue SyntaxError => e + e.line_number = line_number + e.markup_context = markup_context(markup) + raise e + end + def strict_parse_with_error_context(markup) strict_parse(markup) rescue SyntaxError => e diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index a6d80e0ae..acda1e4db 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -25,6 +25,7 @@ class << self # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases. # :warn is the default and will give deprecation warnings when invalid syntax is used. # :strict will enforce correct syntax. + # :rigid is stricter even. def error_mode=(mode) Deprecations.warn("Template.error_mode=", "Environment#error_mode=") Environment.default.error_mode = mode From 06279ec3d07361ea083f60fd3122d58d4b98af77 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 30 Sep 2025 15:46:17 -0400 Subject: [PATCH 03/41] Add a rigid_parse method to `cycle` --- lib/liquid/tags/cycle.rb | 53 +++++++++++++++++++------ test/integration/tags/cycle_tag_test.rb | 15 +++++++ 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index c2d94d5f4..8f4d06890 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -22,18 +22,7 @@ class Cycle < Tag def initialize(tag_name, markup, options) super - case markup - when NamedSyntax - @variables = variables_from_string(Regexp.last_match(2)) - @name = parse_expression(Regexp.last_match(1)) - @is_named = true - when SimpleSyntax - @variables = variables_from_string(markup) - @name = @variables.to_s - @is_named = !@name.match?(/\w+:0x\h{8}/) - else - raise SyntaxError, options[:locale].t("errors.syntax.cycle") - end + parse_with_selected_parser(markup) end def named? @@ -65,6 +54,46 @@ def render_to_output_buffer(context, output) private + # cycle [name:] expression(, expression)* + def rigid_parse(markup) + $stderr.puts "using rigid" + p = @parse_context.new_parser(markup) + + if p.look(:id) && p.peek(1) == :colon + @name = p.consume(:id) + @is_named = true + p.consume(:colon) + end + + @variables = [] + while (var = p.expression) + @variables << var + break unless p.consume?(:comma) + end + + raise_syntax_error(options) if @variables.empty? + end + + # Temporarily until we migrate + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + case markup + when NamedSyntax + @variables = variables_from_string(Regexp.last_match(2)) + @name = parse_expression(Regexp.last_match(1)) + @is_named = true + when SimpleSyntax + @variables = variables_from_string(markup) + @name = @variables.to_s + @is_named = !@name.match?(/\w+:0x\h{8}/) + else + raise SyntaxError, options[:locale].t("errors.syntax.cycle") + end + end + def variables_from_string(markup) markup.split(',').collect do |var| var =~ /\s*(#{QuotedFragment})\s*/o diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index a034db17e..8cd6daac4 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -45,4 +45,19 @@ def test_cycle_tag_always_resets_cycle assert_template_result("11", template) end + + def test_cycle_tag_with_error_mode + # QuotedFragment is more permissive than what Parser#expression allows. + [:lax, :strict].each do |mode| + with_error_mode(mode) do + assert_template_result("a", "{% cycle .5: 'a', 'b' %}") + assert_template_result("b", "{% assign 5 = 'b' %}{% cycle .5, .4 %}") + end + end + + with_error_mode(:rigid) do + assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") } + assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") } + end + end end From 22232cc1dda466d8e9197789f09e4411c7657b9a Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 1 Oct 2025 08:45:32 -0400 Subject: [PATCH 04/41] Add rigid_parse to `render` --- lib/liquid/tags/render.rb | 75 +++++++++++++++++++----- test/integration/tags/render_tag_test.rb | 26 ++++++++ 2 files changed, 85 insertions(+), 16 deletions(-) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 26004d647..89c11063c 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -35,22 +35,7 @@ class Render < Tag def initialize(tag_name, markup, options) super - - raise SyntaxError, options[:locale].t("errors.syntax.render") unless markup =~ SYNTAX - - template_name = Regexp.last_match(1) - with_or_for = Regexp.last_match(3) - variable_name = Regexp.last_match(4) - - @alias_name = Regexp.last_match(6) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil - @template_name_expr = parse_expression(template_name) - @is_for_loop = (with_or_for == FOR) - - @attributes = {} - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end + parse_with_selected_parser(markup) end def for_loop? @@ -99,6 +84,64 @@ def render_tag(context, output) output end + # render (string) (with|for expression)? (as id)? (key: value)* + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + template_name = rigid_template_name(p) + with_or_for = p.id?("for") || p.id?("with") || nil + if with_or_for + variable_name = p.expression + end + + alias_name = nil + if p.consume?(:as) + alias_name = p.consume(:id) + end + + @template_name_expr = parse_expression(template_name) + @variable_name_expr = variable_name ? parse_expression(variable_name) : nil + @alias_name = alias_name + @is_for_loop = (with_or_for == FOR) + + # optional comma + p.consume?(:comma) + + @attributes = {} + while p.look(:id) + key = p.consume + p.consume(:colon) + @attributes[key] = parse_expression(p.expression) + p.consume?(:comma) # optional comma + end + end + + def rigid_template_name(p) + p.consume(:string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + raise SyntaxError, options[:locale].t("errors.syntax.render") unless markup =~ SYNTAX + + template_name = Regexp.last_match(1) + with_or_for = Regexp.last_match(3) + variable_name = Regexp.last_match(4) + + @alias_name = Regexp.last_match(6) + @variable_name_expr = variable_name ? parse_expression(variable_name) : nil + @template_name_expr = parse_expression(template_name) + @is_for_loop = (with_or_for == FOR) + + @attributes = {} + markup.scan(TagAttributes) do |key, value| + @attributes[key] = parse_expression(value) + end + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index eda80a040..b6cd5cfb6 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -105,6 +105,32 @@ def test_dynamically_choosen_templates_are_not_allowed assert_syntax_error("{% assign name = 'snippet' %}{% render name %}") end + def test_rigid_parsing_errors + [:lax, :strict].each do |mode| + with_error_mode(mode) do + assert_template_result( + 'hello value1 value2', + '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, + ) + end + end + + [:rigid].each do |mode| + assert_syntax_error( + '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + error_mode: mode, + ) + end + end + + def test_optional_commas + partials = { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' } + assert_template_result('hello value1 value2', '{% render "snippet", arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% render "snippet" arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% render "snippet" arg1: "value1" arg2: "value2" %}', partials: partials) + end + def test_include_tag_caches_second_read_of_same_partial file_system = StubFileSystem.new('snippet' => 'echo') assert_equal( From 40928e12940d1ba25271b83bb587bc4f437875f8 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 1 Oct 2025 08:58:14 -0400 Subject: [PATCH 05/41] Add rigid_parse method to `include` --- lib/liquid/tags/include.rb | 73 +++++++++++++++++------ test/integration/tags/include_tag_test.rb | 26 ++++++++ 2 files changed, 81 insertions(+), 18 deletions(-) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 1fefa16f4..6cdbfd6f2 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -27,24 +27,7 @@ class Include < Tag def initialize(tag_name, markup, options) super - - if markup =~ SYNTAX - - template_name = Regexp.last_match(1) - variable_name = Regexp.last_match(3) - - @alias_name = Regexp.last_match(5) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil - @template_name_expr = parse_expression(template_name) - @attributes = {} - - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end - - else - raise SyntaxError, options[:locale].t("errors.syntax.include") - end + parse_with_selected_parser(markup) end def parse(_tokens) @@ -101,6 +84,60 @@ def render_to_output_buffer(context, output) alias_method :parse_context, :options private :parse_context + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + template_name = p.expression + with_or_for = p.id?("for") || p.id?("with") || nil + if with_or_for + variable_name = p.expression + end + + alias_name = nil + if p.consume?(:as) + alias_name = p.consume(:id) + end + + @template_name_expr = parse_expression(template_name) + @variable_name_expr = variable_name ? parse_expression(variable_name) : nil + @alias_name = alias_name + + # optional comma + p.consume?(:comma) + + @attributes = {} + while p.look(:id) + key = p.consume + p.consume(:colon) + @attributes[key] = parse_expression(p.expression) + p.consume?(:comma) # optional comma + end + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + if markup =~ SYNTAX + + template_name = Regexp.last_match(1) + variable_name = Regexp.last_match(3) + + @alias_name = Regexp.last_match(5) + @variable_name_expr = variable_name ? parse_expression(variable_name) : nil + @template_name_expr = parse_expression(template_name) + @attributes = {} + + markup.scan(TagAttributes) do |key, value| + @attributes[key] = parse_expression(value) + end + + else + raise SyntaxError, options[:locale].t("errors.syntax.include") + end + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index 6e1649663..e8cd68576 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -204,6 +204,32 @@ def test_dynamically_choosen_template ) end + def test_rigid_parsing_errors + [:lax, :strict].each do |mode| + with_error_mode(mode) do + assert_template_result( + 'hello value1 value2', + '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, + ) + end + end + + [:rigid].each do |mode| + assert_syntax_error( + '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + error_mode: mode, + ) + end + end + + def test_optional_commas + partials = { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' } + assert_template_result('hello value1 value2', '{% include "snippet", arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% include "snippet" arg1: "value1", arg2: "value2" %}', partials: partials) + assert_template_result('hello value1 value2', '{% include "snippet" arg1: "value1" arg2: "value2" %}', partials: partials) + end + def test_include_tag_caches_second_read_of_same_partial file_system = CountingFileSystem.new environment = Liquid::Environment.build(file_system: file_system) From 8f5361b872af33cd628c4c4bfaf79b513f911322 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 1 Oct 2025 15:07:37 +0200 Subject: [PATCH 06/41] Add `ExpressionParser` and `ExpressionConsumer` --- lib/liquid.rb | 2 + lib/liquid/environment.rb | 2 +- lib/liquid/expression_consumer.rb | 182 +++++++++++ lib/liquid/expression_parser.rb | 51 +++ lib/liquid/parser.rb | 6 + test/unit/expression_consumer_test.rb | 429 ++++++++++++++++++++++++++ test/unit/expression_parser_test.rb | 102 ++++++ 7 files changed, 773 insertions(+), 1 deletion(-) create mode 100644 lib/liquid/expression_consumer.rb create mode 100644 lib/liquid/expression_parser.rb create mode 100644 test/unit/expression_consumer_test.rb create mode 100644 test/unit/expression_parser_test.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index 4d0a71a64..edd2c4974 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -80,6 +80,8 @@ module Liquid require 'liquid/range_lookup' require 'liquid/resource_limits' require 'liquid/expression' +require 'liquid/expression_consumer' +require 'liquid/expression_parser' require 'liquid/template' require 'liquid/condition' require 'liquid/utils' diff --git a/lib/liquid/environment.rb b/lib/liquid/environment.rb index 987ec13f9..59719c4ad 100644 --- a/lib/liquid/environment.rb +++ b/lib/liquid/environment.rb @@ -34,7 +34,7 @@ class << self # @param file_system The default file system that is used # to load templates from. # @param error_mode [Symbol] The default error mode for all templates - # (either :strict, :warn, :lax, or :rigid). + # (either :rigid, :strict, :warn, or :lax). # @param exception_renderer [Proc] The exception renderer that is used to # render exceptions. # @yieldparam environment [Environment] The environment instance that is being built. diff --git a/lib/liquid/expression_consumer.rb b/lib/liquid/expression_consumer.rb new file mode 100644 index 000000000..a0c76d9b8 --- /dev/null +++ b/lib/liquid/expression_consumer.rb @@ -0,0 +1,182 @@ +# frozen_string_literal: true + +module Liquid + module ExpressionConsumer + INTEGER_REGEX = /\A(-?\d+)\z/ + FLOAT_REGEX = /\A(-?\d+)\.\d+\z/ + + LITERALS = { + 'nil' => nil, + 'null' => nil, + 'true' => true, + 'false' => false, + 'blank' => '', + 'empty' => '', + }.freeze + + MINUS_VARIABLE_LOOKUP = VariableLookup.parse("-", nil).freeze + + class << self + # Consumes tokens from a Parser instance to build an Expression + # object. + # + # This method reads tokens from the current parser position, + # consuming exactly one complete expression. The parser position is + # advanced past the consumed tokens. + # + # Unlike ExpressionParser.parse, this method does NOT validate that + # all tokens are consumed. It stops after consuming a complete + # expression, leaving any remaining tokens for the caller to handle. + # + # This is the efficient low-level method used by tags that manage + # their own Parser instances and need to consume multiple + # expressions from a single token stream. + # + # Returns an Expression object appropriate for the token type: + # - Literals (nil, true, false, numbers, strings) + # - VariableLookup (variables with optional property/index access) + # - RangeLookup or Range (for range expressions like (1..10)) + # + # Raises SyntaxError if invalid token encountered. + # + # Examples: + # parser = parse_context.new_parser("product.title | upcase") + # expr = ExpressionConsumer.consume(parser, parse_context) + # #=> # + # # Parser is now positioned at the pipe token + # + # parser = parse_context.new_parser("42") + # ExpressionConsumer.consume(parser, parse_context) + # #=> 42 + # + # parser = parse_context.new_parser("items[0]") + # ExpressionConsumer.consume(parser, parse_context) + # #=> # + def consume(parser, parse_context) + token = parser.tokens[parser.point] + + case token[0] + when :string + str = parser.consume(:string) + parse_string(str) + when :number + num_str = parser.consume(:number) + parse_number(num_str) + when :id + parse_id(parser, parse_context) + when :open_square + # Bracket notation: [expression] + parser.consume(:open_square) + inner = consume(parser, parse_context) + parser.consume(:close_square) + lookups = parse_variable_lookups(parser, parse_context) + build_variable_lookup(inner, lookups) + when :open_round + # Range notation: (start..end) + parser.consume(:open_round) + start_obj = consume(parser, parse_context) + parser.consume(:dotdot) + end_obj = consume(parser, parse_context) + parser.consume(:close_round) + build_range_lookup(start_obj, end_obj) + else + raise SyntaxError, "#{token} is not a valid expression" + end + end + + private + + def parse_string(str) + str[1..-2] + end + + def parse_number(num_str) + case num_str + when INTEGER_REGEX then Integer(num_str, 10) + when FLOAT_REGEX then num_str.to_f + else + raise Liquid::SyntaxError, "Invalid expression type in number expression" + end + end + + def parse_id(parser, parse_context) + id_value = parser.consume(:id) + lookups = parse_variable_lookups(parser, parse_context) + + if LITERALS.key?(id_value) + # Case: nil + return LITERALS[id_value] if lookups.empty? + + # Case: nil.size + return build_variable_lookup(id_value, lookups) + end + + if id_value == '-' + # Case (backwards compatibility): - + return MINUS_VARIABLE_LOOKUP if lookups.empty? + + # Case: -var + return build_variable_lookup('-', lookups) + end + + # Case: var + build_variable_lookup(id_value, lookups) + end + + def parse_variable_lookups(parser, parse_context) + lookups = [] + + loop do + if parser.look(:open_square) + parser.consume(:open_square) + lookup = consume(parser, parse_context) + parser.consume(:close_square) + lookups << lookup + next + end + + if parser.look(:dot) + parser.consume(:dot) + id = parser.consume(:id) + lookups << id + next + end + + break + end + + lookups + end + + # todo(guilherme): avoid allocate, simplify this + def build_variable_lookup(name, lookups) + lookup = VariableLookup.allocate + lookup.instance_variable_set(:@name, name) + lookup.instance_variable_set(:@lookups, lookups) + + command_flags = 0 + lookups.each_with_index do |lookup_item, i| + if lookup_item.is_a?(String) && VariableLookup::COMMAND_METHODS.include?(lookup_item) + command_flags |= 1 << i + end + end + lookup.instance_variable_set(:@command_flags, command_flags) + + lookup + end + + # todo(guilherme): use RangeLookup.parse logic, simplify this + def build_range_lookup(start_obj, end_obj) + if !start_obj.respond_to?(:evaluate) && !end_obj.respond_to?(:evaluate) + begin + start_obj.to_i..end_obj.to_i + rescue NoMethodError + raise Liquid::SyntaxError, "Invalid expression type in range expression" + end + else + RangeLookup.new(start_obj, end_obj) + end + end + end + end +end diff --git a/lib/liquid/expression_parser.rb b/lib/liquid/expression_parser.rb new file mode 100644 index 000000000..8712274bc --- /dev/null +++ b/lib/liquid/expression_parser.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Liquid + module ExpressionParser + class << self + # Parses a Liquid expression string into an Expression object using + # strict token-based validation. + # + # This method tokenizes the markup, validates that the expression + # consumes all available tokens (no trailing garbage), and builds + # an appropriate Expression object (literal, VariableLookup, or + # RangeLookup). + # + # Returns nil if the markup is empty or contains only whitespace. + # + # Raises SyntaxError if: + # - Invalid syntax is encountered + # - Extra tokens remain after the expression + # (e.g., "product title" instead of "product.title") + # + # Examples: + # ExpressionParser.parse("product.title", ctx) + # #=> # + # + # ExpressionParser.parse("42", ctx) + # #=> 42 + # + # ExpressionParser.parse("(1..10)", ctx) + # #=> 1..10 + # + # ExpressionParser.parse("", ctx) + # #=> nil + # + # ExpressionParser.parse("product title", ctx) + # #=> raises SyntaxError (extra token "title") + def parse(markup, parse_context) + parser = parse_context.new_parser(markup) + + # Whitespaces only. + return if parser.look(:end_of_string) + + result = ExpressionConsumer.consume(parser, parse_context) + + # Extra tokens after the expression. + parser.consume(:end_of_string) unless parser.look(:end_of_string) + + result + end + end + end +end diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 645dfa3a1..24b2c5267 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -2,6 +2,8 @@ module Liquid class Parser + attr_reader :tokens + def initialize(input) ss = input.is_a?(StringScanner) ? input : StringScanner.new(input) @tokens = Lexer.tokenize(ss) @@ -82,6 +84,10 @@ def argument str end + def point + @p + end + def variable_lookups str = +"" loop do diff --git a/test/unit/expression_consumer_test.rb b/test/unit/expression_consumer_test.rb new file mode 100644 index 000000000..99921ec7a --- /dev/null +++ b/test/unit/expression_consumer_test.rb @@ -0,0 +1,429 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ExpressionConsumerTest < Minitest::Test + include Liquid + + def test_consume_string_literal_with_double_quotes + parser = parse_context.new_parser('"hello"') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('hello', result) + end + + def test_consume_string_literal_with_single_quotes + parser = parse_context.new_parser("'world'") + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('world', result) + end + + def test_consume_string_with_empty_content + parser = parse_context.new_parser('""') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('', result) + end + + def test_consume_single_quote_empty_string + parser = parse_context.new_parser("''") + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('', result) + end + + def test_consume_integer_literal + parser = parse_context.new_parser('42') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(42, result) + assert_kind_of(Integer, result) + end + + def test_consume_negative_integer_literal + parser = parse_context.new_parser('-42') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(-42, result) + end + + def test_consume_float_literal + parser = parse_context.new_parser('3.14') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(3.14, result) + assert_kind_of(Float, result) + end + + def test_consume_negative_float_literal + parser = parse_context.new_parser('-3.14') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(-3.14, result) + end + + def test_consume_zero_as_integer + parser = parse_context.new_parser('0') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(0, result) + assert_kind_of(Integer, result) + end + + def test_consume_zero_as_float + parser = parse_context.new_parser('0.0') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(0.0, result) + assert_kind_of(Float, result) + end + + def test_consume_nil_literal + parser = parse_context.new_parser('nil') + result = ExpressionConsumer.consume(parser, parse_context) + assert_nil(result) + end + + def test_consume_null_literal + parser = parse_context.new_parser('null') + result = ExpressionConsumer.consume(parser, parse_context) + assert_nil(result) + end + + def test_consume_true_literal + parser = parse_context.new_parser('true') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(true, result) + end + + def test_consume_false_literal + parser = parse_context.new_parser('false') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(false, result) + end + + def test_consume_blank_literal + parser = parse_context.new_parser('blank') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('', result) + end + + def test_consume_empty_literal + parser = parse_context.new_parser('empty') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('', result) + end + + def test_consume_nil_literal_with_lookups + parser = parse_context.new_parser('nil.size') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('nil', result.name) + assert_equal(['size'], result.lookups) + end + + def test_consume_true_literal_with_lookups + parser = parse_context.new_parser('true.size') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('true', result.name) + assert_equal(['size'], result.lookups) + end + + def test_consume_negative_number_parses_as_number + parser = parse_context.new_parser('-5') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(-5, result) + end + + def test_consume_simple_variable + parser = parse_context.new_parser('product') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal([], result.lookups) + end + + def test_consume_variable_with_dot_lookup + parser = parse_context.new_parser('product.title') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + + def test_consume_variable_with_multiple_dot_lookups + parser = parse_context.new_parser('product.variants.first') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['variants', 'first'], result.lookups) + end + + def test_consume_variable_with_bracket_lookup + parser = parse_context.new_parser('items[0]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('items', result.name) + assert_equal(0, result.lookups[0]) + end + + def test_consume_variable_with_bracket_string_lookup + parser = parse_context.new_parser('items["key"]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('items', result.name) + assert_equal('key', result.lookups[0]) + end + + def test_consume_variable_with_bracket_variable_lookup + parser = parse_context.new_parser('items[index]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('index', result.lookups[0].name) + end + + def test_consume_variable_with_mixed_lookups + parser = parse_context.new_parser('product.variants[0].title') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(3, result.lookups.length) + assert_equal('variants', result.lookups[0]) + assert_equal(0, result.lookups[1]) + assert_equal('title', result.lookups[2]) + end + + def test_consume_bracket_notation_without_variable + parser = parse_context.new_parser('[0]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal(0, result.name) + end + + def test_consume_bracket_notation_with_lookups + parser = parse_context.new_parser('[0].title') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal(0, result.name) + assert_equal(['title'], result.lookups) + end + + def test_consume_bracket_notation_with_bracket_lookups + parser = parse_context.new_parser('[0][1]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_equal(0, result.name) + assert_equal(1, result.lookups[0]) + end + + def test_consume_range_with_integer_literals + parser = parse_context.new_parser('(1..5)') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(Range, result) + assert_equal(1..5, result) + end + + def test_consume_range_with_negative_integers + parser = parse_context.new_parser('(-5..-1)') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(Range, result) + assert_equal(-5..-1, result) + end + + def test_consume_range_with_variable_start + parser = parse_context.new_parser('(start..10)') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(RangeLookup, result) + assert_kind_of(VariableLookup, result.start_obj) + assert_equal(10, result.end_obj) + end + + def test_consume_range_with_variable_end + parser = parse_context.new_parser('(1..end)') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(RangeLookup, result) + assert_equal(1, result.start_obj) + assert_kind_of(VariableLookup, result.end_obj) + end + + def test_consume_range_with_both_variables + parser = parse_context.new_parser('(start..end)') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(RangeLookup, result) + assert_kind_of(VariableLookup, result.start_obj) + assert_kind_of(VariableLookup, result.end_obj) + end + + def test_consume_range_with_variable_lookups + parser = parse_context.new_parser('(start.value..end.value)') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(RangeLookup, result) + assert_equal(['value'], result.start_obj.lookups) + end + + def test_consume_command_method_size_sets_flag + parser = parse_context.new_parser('items.size') + result = ExpressionConsumer.consume(parser, parse_context) + assert(result.lookup_command?(0)) + end + + def test_consume_command_method_first_sets_flag + parser = parse_context.new_parser('items.first') + result = ExpressionConsumer.consume(parser, parse_context) + assert(result.lookup_command?(0)) + end + + def test_consume_command_method_last_sets_flag + parser = parse_context.new_parser('items.last') + result = ExpressionConsumer.consume(parser, parse_context) + assert(result.lookup_command?(0)) + end + + def test_consume_non_command_method_does_not_set_flag + parser = parse_context.new_parser('items.title') + result = ExpressionConsumer.consume(parser, parse_context) + refute(result.lookup_command?(0)) + end + + def test_consume_advances_parser_position + parser = parse_context.new_parser('foo.bar') + ExpressionConsumer.consume(parser, parse_context) + assert(parser.look(:end_of_string)) + end + + def test_consume_stops_before_extra_tokens + parser = parse_context.new_parser('foo bar') + ExpressionConsumer.consume(parser, parse_context) + refute(parser.look(:end_of_string)) + end + + def test_consume_with_nested_brackets + parser = parse_context.new_parser('items[items[0]]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result) + assert_kind_of(VariableLookup, result.lookups[0]) + end + + def test_consume_bracket_with_range + parser = parse_context.new_parser('items[(1..3)]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(Range, result.lookups[0]) + assert_equal(1..3, result.lookups[0]) + end + + def test_consume_raises_on_invalid_token_type + parser = parse_context.new_parser('|') + error = assert_raises(SyntaxError) do + ExpressionConsumer.consume(parser, parse_context) + end + assert_match(/is not a valid expression/, error.message) + end + + def test_consume_range_with_string_literals + parser = parse_context.new_parser('("a".."z")') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(Range, result) + assert_equal(0..0, result) + end + + def test_consume_multiple_command_methods + parser = parse_context.new_parser('items.first.size.last') + result = ExpressionConsumer.consume(parser, parse_context) + assert(result.lookup_command?(0)) + assert(result.lookup_command?(1)) + assert(result.lookup_command?(2)) + end + + def test_consume_dot_after_bracket + parser = parse_context.new_parser('items[0].title') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(0, result.lookups[0]) + assert_equal('title', result.lookups[1]) + end + + def test_consume_bracket_after_dot + parser = parse_context.new_parser('product.variants[0]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal('variants', result.lookups[0]) + assert_equal(0, result.lookups[1]) + end + + def test_consume_multiple_brackets_with_different_types + parser = parse_context.new_parser('a[0]["key"][var]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(0, result.lookups[0]) + assert_equal('key', result.lookups[1]) + assert_kind_of(VariableLookup, result.lookups[2]) + end + + def test_consume_deep_nested_brackets + parser = parse_context.new_parser('a[b[c[d]]]') + result = ExpressionConsumer.consume(parser, parse_context) + inner1 = result.lookups[0] + assert_kind_of(VariableLookup, inner1) + inner2 = inner1.lookups[0] + assert_kind_of(VariableLookup, inner2) + end + + def test_consume_with_spaces_in_range + parser = parse_context.new_parser('( 1 .. 10 )') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(1..10, result) + end + + def test_consume_starting_with_bracket_then_dots + parser = parse_context.new_parser('[0].first.last') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(0, result.name) + assert(result.lookup_command?(0)) + assert(result.lookup_command?(1)) + end + + def test_consume_only_dot_lookups + parser = parse_context.new_parser('a.b.c.d') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(['b', 'c', 'd'], result.lookups) + end + + def test_consume_only_bracket_lookups + parser = parse_context.new_parser('a[0][1][2]') + result = ExpressionConsumer.consume(parser, parse_context) + assert_equal(3, result.lookups.length) + assert_equal(0, result.lookups[0]) + assert_equal(1, result.lookups[1]) + assert_equal(2, result.lookups[2]) + end + + def test_consume_complex_nested_expression + parser = parse_context.new_parser('product.variants[index].title') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(VariableLookup, result.lookups[1]) + assert_equal('index', result.lookups[1].name) + end + + def test_consume_range_with_bracketed_variables + parser = parse_context.new_parser('(items[0]..items[1])') + result = ExpressionConsumer.consume(parser, parse_context) + assert_kind_of(RangeLookup, result) + assert_kind_of(VariableLookup, result.start_obj) + end + + def test_consume_evaluates_correctly + parser = parse_context.new_parser('product') + result = ExpressionConsumer.consume(parser, parse_context) + context = Context.new({ 'product' => 'Test Product' }) + assert_equal('Test Product', context.evaluate(result)) + end + + def test_consume_with_lookups_evaluates_correctly + parser = parse_context.new_parser('product.title') + result = ExpressionConsumer.consume(parser, parse_context) + context = Context.new({ 'product' => { 'title' => 'My Title' } }) + assert_equal('My Title', context.evaluate(result)) + end + + def test_consume_range_evaluates_correctly + parser = parse_context.new_parser('(start..end)') + result = ExpressionConsumer.consume(parser, parse_context) + context = Context.new({ 'start' => 1, 'end' => 5 }) + assert_equal(1..5, context.evaluate(result)) + end + + private + + def parse_context + @parse_context ||= ParseContext.new(environment: Environment.build(error_mode: :rigid)) + end +end diff --git a/test/unit/expression_parser_test.rb b/test/unit/expression_parser_test.rb new file mode 100644 index 000000000..8cc1dd31d --- /dev/null +++ b/test/unit/expression_parser_test.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ExpressionParserTest < Minitest::Test + include Liquid + + def test_parse_returns_nil_for_empty_string + result = ExpressionParser.parse('', parse_context) + assert_nil(result) + end + + def test_parse_returns_nil_for_whitespace_only + result = ExpressionParser.parse(' ', parse_context) + assert_nil(result) + end + + def test_parse_raises_on_extra_tokens_after_expression + error = assert_raises(SyntaxError) do + ExpressionParser.parse('foo bar', parse_context) + end + assert_match(/Expected end_of_string but found id/, error.message) + end + + def test_parse_string_literal_with_double_quotes + result = ExpressionParser.parse('"hello"', parse_context) + assert_equal('hello', result) + end + + def test_parse_string_literal_with_single_quotes + result = ExpressionParser.parse("'world'", parse_context) + assert_equal('world', result) + end + + def test_parse_integer_literal + result = ExpressionParser.parse('42', parse_context) + assert_equal(42, result) + end + + def test_parse_float_literal + result = ExpressionParser.parse('3.14', parse_context) + assert_equal(3.14, result) + end + + def test_parse_nil_literal + result = ExpressionParser.parse('nil', parse_context) + assert_nil(result) + end + + def test_parse_true_literal + result = ExpressionParser.parse('true', parse_context) + assert_equal(true, result) + end + + def test_parse_simple_variable + result = ExpressionParser.parse('product', parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('product', result.name) + end + + def test_parse_variable_with_dot_lookup + result = ExpressionParser.parse('product.title', parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + + def test_parse_variable_with_bracket_lookup + result = ExpressionParser.parse('items[0]', parse_context) + assert_kind_of(VariableLookup, result) + assert_equal('items', result.name) + assert_equal(0, result.lookups[0]) + end + + def test_parse_range_with_integer_literals + result = ExpressionParser.parse('(1..5)', parse_context) + assert_kind_of(Range, result) + assert_equal(1..5, result) + end + + def test_parse_range_with_variables + result = ExpressionParser.parse('(start..end)', parse_context) + assert_kind_of(RangeLookup, result) + end + + def test_parse_validates_end_of_string + result = ExpressionParser.parse('foo', parse_context) + assert_kind_of(VariableLookup, result) + end + + def test_parse_evaluates_correctly + result = ExpressionParser.parse('product.title', parse_context) + context = Context.new({ 'product' => { 'title' => 'My Title' } }) + assert_equal('My Title', context.evaluate(result)) + end + + private + + def parse_context + @parse_context ||= ParseContext.new(environment: Environment.build(error_mode: :rigid)) + end +end From 7527f8adf8b2271c76aa29935572c9031246b555 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 1 Oct 2025 15:08:44 +0200 Subject: [PATCH 07/41] Use `ExpressionParser` in the `ParseContext` when parsing in `:rigid` mode --- lib/liquid/parse_context.rb | 31 ++++++++++++----------------- test/integration/expression_test.rb | 31 +++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 277d349e9..d314448c0 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -52,26 +52,21 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) def parse_expression(markup) if @error_mode == :rigid - parser = new_parser(markup) - - # Return nil immediately if the markup is empty or contains only - # whitespaces - return if parser.look(:end_of_string) - - expression_string = parser.expression - - # In rigid mode, verify that all tokens have been consumed + # ExpressionParser doesn't use @expression_cache because rigid mode + # must run Lexer and Parser validation on every call to ensure all + # tokens are valid and properly consumed. # - # Extra tokens remaining after the expression indicate invalid syntaxes, - # such as: "product title" (instead of "product.title") - parser.consume(:end_of_string) unless parser.look(:end_of_string) - - # Use Parser for strict token validation, but still return - # Expression objects for compatibility with the rendering pipeline. - markup = expression_string + # The expensive operations (tokenization and validation) cannot be + # cached, while the cheap operation (building Expression objects from + # validated tokens) provides minimal benefit from caching. + # + # Most importantly, caching would skip the validation step entirely, + # which defeats the core purpose of rigid mode: strict validation of + # every expression to catch syntax errors like "product title". + ExpressionParser.parse(markup, self) + else + Expression.parse(markup, @string_scanner, @expression_cache) end - - Expression.parse(markup, @string_scanner, @expression_cache) end def partial=(value) diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 29e456321..4ee164b84 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -42,14 +42,25 @@ def test_range assert_template_result("3..4", "{{ ( 3 .. 4 ) }}") assert_expression_result(1..2, "(1..2)") - assert_match_syntax_error( - "Liquid syntax error (line 1): Invalid expression type 'false' in range expression", - "{{ (false..true) }}", - ) - assert_match_syntax_error( - "Liquid syntax error (line 1): Invalid expression type '(1..2)' in range expression", - "{{ ((1..2)..3) }}", - ) + if Liquid::Environment.default.error_mode == :rigid + assert_match_syntax_error( + 'Invalid expression type in range expression in "{{ (false..true) }}"', + "{{ (false..true) }}", + ) + assert_match_syntax_error( + 'Liquid syntax error (line 1): Invalid expression type in range expression in "{{ ((1..2)..3) }}"', + "{{ ((1..2)..3) }}", + ) + else + assert_match_syntax_error( + "Liquid syntax error (line 1): Invalid expression type 'false' in range expression", + "{{ (false..true) }}", + ) + assert_match_syntax_error( + "Liquid syntax error (line 1): Invalid expression type '(1..2)' in range expression", + "{{ ((1..2)..3) }}", + ) + end end def test_quirky_negative_sign_expression_markup @@ -66,6 +77,7 @@ def test_quirky_negative_sign_expression_markup 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 @@ -87,6 +99,7 @@ def test_expression_cache 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 %} @@ -111,6 +124,7 @@ def test_expression_cache_with_true_boolean 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 @@ -132,6 +146,7 @@ def test_expression_cache_with_lru_redux 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 %} From d9caf26e881c573300631c62f906a4da8bfd1091 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 1 Oct 2025 19:53:20 +0200 Subject: [PATCH 08/41] Remove `ExpressionParser` in favor of `ParseContext#safe_parse` --- lib/liquid.rb | 2 - lib/liquid/expression.rb | 4 + lib/liquid/expression_consumer.rb | 182 ---------- lib/liquid/expression_parser.rb | 51 --- lib/liquid/parse_context.rb | 25 +- lib/liquid/parser.rb | 6 - lib/liquid/tag.rb | 4 + lib/liquid/tags/cycle.rb | 1 - test/integration/expression_test.rb | 27 +- test/integration/tags/cycle_tag_test.rb | 9 +- test/unit/expression_consumer_test.rb | 429 ------------------------ test/unit/expression_parser_test.rb | 102 ------ test/unit/rigid_mode_unit_test.rb | 4 + 13 files changed, 34 insertions(+), 812 deletions(-) delete mode 100644 lib/liquid/expression_consumer.rb delete mode 100644 lib/liquid/expression_parser.rb delete mode 100644 test/unit/expression_consumer_test.rb delete mode 100644 test/unit/expression_parser_test.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index edd2c4974..4d0a71a64 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -80,8 +80,6 @@ module Liquid require 'liquid/range_lookup' require 'liquid/resource_limits' require 'liquid/expression' -require 'liquid/expression_consumer' -require 'liquid/expression_parser' require 'liquid/template' require 'liquid/condition' require 'liquid/utils' diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index adf340f1f..2605c5576 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -28,6 +28,10 @@ class Expression FLOAT_REGEX = /\A(-?\d+)\.\d+\z/ class << self + def safe_parse(parser, ss = StringScanner.new(""), cache = nil) + parse(parser.expression, ss, cache) + end + def parse(markup, ss = StringScanner.new(""), cache = nil) return unless markup diff --git a/lib/liquid/expression_consumer.rb b/lib/liquid/expression_consumer.rb deleted file mode 100644 index a0c76d9b8..000000000 --- a/lib/liquid/expression_consumer.rb +++ /dev/null @@ -1,182 +0,0 @@ -# frozen_string_literal: true - -module Liquid - module ExpressionConsumer - INTEGER_REGEX = /\A(-?\d+)\z/ - FLOAT_REGEX = /\A(-?\d+)\.\d+\z/ - - LITERALS = { - 'nil' => nil, - 'null' => nil, - 'true' => true, - 'false' => false, - 'blank' => '', - 'empty' => '', - }.freeze - - MINUS_VARIABLE_LOOKUP = VariableLookup.parse("-", nil).freeze - - class << self - # Consumes tokens from a Parser instance to build an Expression - # object. - # - # This method reads tokens from the current parser position, - # consuming exactly one complete expression. The parser position is - # advanced past the consumed tokens. - # - # Unlike ExpressionParser.parse, this method does NOT validate that - # all tokens are consumed. It stops after consuming a complete - # expression, leaving any remaining tokens for the caller to handle. - # - # This is the efficient low-level method used by tags that manage - # their own Parser instances and need to consume multiple - # expressions from a single token stream. - # - # Returns an Expression object appropriate for the token type: - # - Literals (nil, true, false, numbers, strings) - # - VariableLookup (variables with optional property/index access) - # - RangeLookup or Range (for range expressions like (1..10)) - # - # Raises SyntaxError if invalid token encountered. - # - # Examples: - # parser = parse_context.new_parser("product.title | upcase") - # expr = ExpressionConsumer.consume(parser, parse_context) - # #=> # - # # Parser is now positioned at the pipe token - # - # parser = parse_context.new_parser("42") - # ExpressionConsumer.consume(parser, parse_context) - # #=> 42 - # - # parser = parse_context.new_parser("items[0]") - # ExpressionConsumer.consume(parser, parse_context) - # #=> # - def consume(parser, parse_context) - token = parser.tokens[parser.point] - - case token[0] - when :string - str = parser.consume(:string) - parse_string(str) - when :number - num_str = parser.consume(:number) - parse_number(num_str) - when :id - parse_id(parser, parse_context) - when :open_square - # Bracket notation: [expression] - parser.consume(:open_square) - inner = consume(parser, parse_context) - parser.consume(:close_square) - lookups = parse_variable_lookups(parser, parse_context) - build_variable_lookup(inner, lookups) - when :open_round - # Range notation: (start..end) - parser.consume(:open_round) - start_obj = consume(parser, parse_context) - parser.consume(:dotdot) - end_obj = consume(parser, parse_context) - parser.consume(:close_round) - build_range_lookup(start_obj, end_obj) - else - raise SyntaxError, "#{token} is not a valid expression" - end - end - - private - - def parse_string(str) - str[1..-2] - end - - def parse_number(num_str) - case num_str - when INTEGER_REGEX then Integer(num_str, 10) - when FLOAT_REGEX then num_str.to_f - else - raise Liquid::SyntaxError, "Invalid expression type in number expression" - end - end - - def parse_id(parser, parse_context) - id_value = parser.consume(:id) - lookups = parse_variable_lookups(parser, parse_context) - - if LITERALS.key?(id_value) - # Case: nil - return LITERALS[id_value] if lookups.empty? - - # Case: nil.size - return build_variable_lookup(id_value, lookups) - end - - if id_value == '-' - # Case (backwards compatibility): - - return MINUS_VARIABLE_LOOKUP if lookups.empty? - - # Case: -var - return build_variable_lookup('-', lookups) - end - - # Case: var - build_variable_lookup(id_value, lookups) - end - - def parse_variable_lookups(parser, parse_context) - lookups = [] - - loop do - if parser.look(:open_square) - parser.consume(:open_square) - lookup = consume(parser, parse_context) - parser.consume(:close_square) - lookups << lookup - next - end - - if parser.look(:dot) - parser.consume(:dot) - id = parser.consume(:id) - lookups << id - next - end - - break - end - - lookups - end - - # todo(guilherme): avoid allocate, simplify this - def build_variable_lookup(name, lookups) - lookup = VariableLookup.allocate - lookup.instance_variable_set(:@name, name) - lookup.instance_variable_set(:@lookups, lookups) - - command_flags = 0 - lookups.each_with_index do |lookup_item, i| - if lookup_item.is_a?(String) && VariableLookup::COMMAND_METHODS.include?(lookup_item) - command_flags |= 1 << i - end - end - lookup.instance_variable_set(:@command_flags, command_flags) - - lookup - end - - # todo(guilherme): use RangeLookup.parse logic, simplify this - def build_range_lookup(start_obj, end_obj) - if !start_obj.respond_to?(:evaluate) && !end_obj.respond_to?(:evaluate) - begin - start_obj.to_i..end_obj.to_i - rescue NoMethodError - raise Liquid::SyntaxError, "Invalid expression type in range expression" - end - else - RangeLookup.new(start_obj, end_obj) - end - end - end - end -end diff --git a/lib/liquid/expression_parser.rb b/lib/liquid/expression_parser.rb deleted file mode 100644 index 8712274bc..000000000 --- a/lib/liquid/expression_parser.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -module Liquid - module ExpressionParser - class << self - # Parses a Liquid expression string into an Expression object using - # strict token-based validation. - # - # This method tokenizes the markup, validates that the expression - # consumes all available tokens (no trailing garbage), and builds - # an appropriate Expression object (literal, VariableLookup, or - # RangeLookup). - # - # Returns nil if the markup is empty or contains only whitespace. - # - # Raises SyntaxError if: - # - Invalid syntax is encountered - # - Extra tokens remain after the expression - # (e.g., "product title" instead of "product.title") - # - # Examples: - # ExpressionParser.parse("product.title", ctx) - # #=> # - # - # ExpressionParser.parse("42", ctx) - # #=> 42 - # - # ExpressionParser.parse("(1..10)", ctx) - # #=> 1..10 - # - # ExpressionParser.parse("", ctx) - # #=> nil - # - # ExpressionParser.parse("product title", ctx) - # #=> raises SyntaxError (extra token "title") - def parse(markup, parse_context) - parser = parse_context.new_parser(markup) - - # Whitespaces only. - return if parser.look(:end_of_string) - - result = ExpressionConsumer.consume(parser, parse_context) - - # Extra tokens after the expression. - parser.consume(:end_of_string) unless parser.look(:end_of_string) - - result - end - end - end -end diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index d314448c0..1c59fe4a6 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -50,23 +50,16 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) ) end + def safe_parse_expression(parser) + Expression.safe_parse(parser) + end + def parse_expression(markup) - if @error_mode == :rigid - # ExpressionParser doesn't use @expression_cache because rigid mode - # must run Lexer and Parser validation on every call to ensure all - # tokens are valid and properly consumed. - # - # The expensive operations (tokenization and validation) cannot be - # cached, while the cheap operation (building Expression objects from - # validated tokens) provides minimal benefit from caching. - # - # Most importantly, caching would skip the validation step entirely, - # which defeats the core purpose of rigid mode: strict validation of - # every expression to catch syntax errors like "product title". - ExpressionParser.parse(markup, self) - else - Expression.parse(markup, @string_scanner, @expression_cache) - end + # todo(guilherme): remove this once rigid mode is fully using safe_parse_expression + # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" if @error_mode == :rigid + puts("🚨 parse_expression used in rigid mode") if @error_mode == :rigid + + Expression.parse(markup, @string_scanner, @expression_cache) end def partial=(value) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 24b2c5267..645dfa3a1 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -2,8 +2,6 @@ module Liquid class Parser - attr_reader :tokens - def initialize(input) ss = input.is_a?(StringScanner) ? input : StringScanner.new(input) @tokens = Lexer.tokenize(ss) @@ -84,10 +82,6 @@ def argument str end - def point - @p - end - def variable_lookups str = +"" loop do diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 9ca97d0f3..656d2e47c 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -68,6 +68,10 @@ def blank? private + def safe_parse_expression(parser) + parse_context.safe_parse_expression(parser) + end + def parse_expression(markup) parse_context.parse_expression(markup) end diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 8f4d06890..ad1058bd9 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -56,7 +56,6 @@ def render_to_output_buffer(context, output) # cycle [name:] expression(, expression)* def rigid_parse(markup) - $stderr.puts "using rigid" p = @parse_context.new_parser(markup) if p.look(:id) && p.peek(1) == :colon diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 4ee164b84..918f87c28 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -42,25 +42,14 @@ def test_range assert_template_result("3..4", "{{ ( 3 .. 4 ) }}") assert_expression_result(1..2, "(1..2)") - if Liquid::Environment.default.error_mode == :rigid - assert_match_syntax_error( - 'Invalid expression type in range expression in "{{ (false..true) }}"', - "{{ (false..true) }}", - ) - assert_match_syntax_error( - 'Liquid syntax error (line 1): Invalid expression type in range expression in "{{ ((1..2)..3) }}"', - "{{ ((1..2)..3) }}", - ) - else - assert_match_syntax_error( - "Liquid syntax error (line 1): Invalid expression type 'false' in range expression", - "{{ (false..true) }}", - ) - assert_match_syntax_error( - "Liquid syntax error (line 1): Invalid expression type '(1..2)' in range expression", - "{{ ((1..2)..3) }}", - ) - end + assert_match_syntax_error( + "Liquid syntax error (line 1): Invalid expression type 'false' in range expression", + "{{ (false..true) }}", + ) + assert_match_syntax_error( + "Liquid syntax error (line 1): Invalid expression type '(1..2)' in range expression", + "{{ ((1..2)..3) }}", + ) end def test_quirky_negative_sign_expression_markup diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index 8cd6daac4..8452c4678 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -55,9 +55,10 @@ def test_cycle_tag_with_error_mode end end - with_error_mode(:rigid) do - assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") } - assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") } - end + skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + # with_error_mode(:rigid) do + # assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") } + # assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") } + # end end end diff --git a/test/unit/expression_consumer_test.rb b/test/unit/expression_consumer_test.rb deleted file mode 100644 index 99921ec7a..000000000 --- a/test/unit/expression_consumer_test.rb +++ /dev/null @@ -1,429 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -class ExpressionConsumerTest < Minitest::Test - include Liquid - - def test_consume_string_literal_with_double_quotes - parser = parse_context.new_parser('"hello"') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('hello', result) - end - - def test_consume_string_literal_with_single_quotes - parser = parse_context.new_parser("'world'") - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('world', result) - end - - def test_consume_string_with_empty_content - parser = parse_context.new_parser('""') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('', result) - end - - def test_consume_single_quote_empty_string - parser = parse_context.new_parser("''") - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('', result) - end - - def test_consume_integer_literal - parser = parse_context.new_parser('42') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(42, result) - assert_kind_of(Integer, result) - end - - def test_consume_negative_integer_literal - parser = parse_context.new_parser('-42') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(-42, result) - end - - def test_consume_float_literal - parser = parse_context.new_parser('3.14') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(3.14, result) - assert_kind_of(Float, result) - end - - def test_consume_negative_float_literal - parser = parse_context.new_parser('-3.14') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(-3.14, result) - end - - def test_consume_zero_as_integer - parser = parse_context.new_parser('0') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(0, result) - assert_kind_of(Integer, result) - end - - def test_consume_zero_as_float - parser = parse_context.new_parser('0.0') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(0.0, result) - assert_kind_of(Float, result) - end - - def test_consume_nil_literal - parser = parse_context.new_parser('nil') - result = ExpressionConsumer.consume(parser, parse_context) - assert_nil(result) - end - - def test_consume_null_literal - parser = parse_context.new_parser('null') - result = ExpressionConsumer.consume(parser, parse_context) - assert_nil(result) - end - - def test_consume_true_literal - parser = parse_context.new_parser('true') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(true, result) - end - - def test_consume_false_literal - parser = parse_context.new_parser('false') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(false, result) - end - - def test_consume_blank_literal - parser = parse_context.new_parser('blank') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('', result) - end - - def test_consume_empty_literal - parser = parse_context.new_parser('empty') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('', result) - end - - def test_consume_nil_literal_with_lookups - parser = parse_context.new_parser('nil.size') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('nil', result.name) - assert_equal(['size'], result.lookups) - end - - def test_consume_true_literal_with_lookups - parser = parse_context.new_parser('true.size') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('true', result.name) - assert_equal(['size'], result.lookups) - end - - def test_consume_negative_number_parses_as_number - parser = parse_context.new_parser('-5') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(-5, result) - end - - def test_consume_simple_variable - parser = parse_context.new_parser('product') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('product', result.name) - assert_equal([], result.lookups) - end - - def test_consume_variable_with_dot_lookup - parser = parse_context.new_parser('product.title') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('product', result.name) - assert_equal(['title'], result.lookups) - end - - def test_consume_variable_with_multiple_dot_lookups - parser = parse_context.new_parser('product.variants.first') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('product', result.name) - assert_equal(['variants', 'first'], result.lookups) - end - - def test_consume_variable_with_bracket_lookup - parser = parse_context.new_parser('items[0]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('items', result.name) - assert_equal(0, result.lookups[0]) - end - - def test_consume_variable_with_bracket_string_lookup - parser = parse_context.new_parser('items["key"]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('items', result.name) - assert_equal('key', result.lookups[0]) - end - - def test_consume_variable_with_bracket_variable_lookup - parser = parse_context.new_parser('items[index]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('index', result.lookups[0].name) - end - - def test_consume_variable_with_mixed_lookups - parser = parse_context.new_parser('product.variants[0].title') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('product', result.name) - assert_equal(3, result.lookups.length) - assert_equal('variants', result.lookups[0]) - assert_equal(0, result.lookups[1]) - assert_equal('title', result.lookups[2]) - end - - def test_consume_bracket_notation_without_variable - parser = parse_context.new_parser('[0]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal(0, result.name) - end - - def test_consume_bracket_notation_with_lookups - parser = parse_context.new_parser('[0].title') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal(0, result.name) - assert_equal(['title'], result.lookups) - end - - def test_consume_bracket_notation_with_bracket_lookups - parser = parse_context.new_parser('[0][1]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_equal(0, result.name) - assert_equal(1, result.lookups[0]) - end - - def test_consume_range_with_integer_literals - parser = parse_context.new_parser('(1..5)') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(Range, result) - assert_equal(1..5, result) - end - - def test_consume_range_with_negative_integers - parser = parse_context.new_parser('(-5..-1)') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(Range, result) - assert_equal(-5..-1, result) - end - - def test_consume_range_with_variable_start - parser = parse_context.new_parser('(start..10)') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(RangeLookup, result) - assert_kind_of(VariableLookup, result.start_obj) - assert_equal(10, result.end_obj) - end - - def test_consume_range_with_variable_end - parser = parse_context.new_parser('(1..end)') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(RangeLookup, result) - assert_equal(1, result.start_obj) - assert_kind_of(VariableLookup, result.end_obj) - end - - def test_consume_range_with_both_variables - parser = parse_context.new_parser('(start..end)') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(RangeLookup, result) - assert_kind_of(VariableLookup, result.start_obj) - assert_kind_of(VariableLookup, result.end_obj) - end - - def test_consume_range_with_variable_lookups - parser = parse_context.new_parser('(start.value..end.value)') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(RangeLookup, result) - assert_equal(['value'], result.start_obj.lookups) - end - - def test_consume_command_method_size_sets_flag - parser = parse_context.new_parser('items.size') - result = ExpressionConsumer.consume(parser, parse_context) - assert(result.lookup_command?(0)) - end - - def test_consume_command_method_first_sets_flag - parser = parse_context.new_parser('items.first') - result = ExpressionConsumer.consume(parser, parse_context) - assert(result.lookup_command?(0)) - end - - def test_consume_command_method_last_sets_flag - parser = parse_context.new_parser('items.last') - result = ExpressionConsumer.consume(parser, parse_context) - assert(result.lookup_command?(0)) - end - - def test_consume_non_command_method_does_not_set_flag - parser = parse_context.new_parser('items.title') - result = ExpressionConsumer.consume(parser, parse_context) - refute(result.lookup_command?(0)) - end - - def test_consume_advances_parser_position - parser = parse_context.new_parser('foo.bar') - ExpressionConsumer.consume(parser, parse_context) - assert(parser.look(:end_of_string)) - end - - def test_consume_stops_before_extra_tokens - parser = parse_context.new_parser('foo bar') - ExpressionConsumer.consume(parser, parse_context) - refute(parser.look(:end_of_string)) - end - - def test_consume_with_nested_brackets - parser = parse_context.new_parser('items[items[0]]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result) - assert_kind_of(VariableLookup, result.lookups[0]) - end - - def test_consume_bracket_with_range - parser = parse_context.new_parser('items[(1..3)]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(Range, result.lookups[0]) - assert_equal(1..3, result.lookups[0]) - end - - def test_consume_raises_on_invalid_token_type - parser = parse_context.new_parser('|') - error = assert_raises(SyntaxError) do - ExpressionConsumer.consume(parser, parse_context) - end - assert_match(/is not a valid expression/, error.message) - end - - def test_consume_range_with_string_literals - parser = parse_context.new_parser('("a".."z")') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(Range, result) - assert_equal(0..0, result) - end - - def test_consume_multiple_command_methods - parser = parse_context.new_parser('items.first.size.last') - result = ExpressionConsumer.consume(parser, parse_context) - assert(result.lookup_command?(0)) - assert(result.lookup_command?(1)) - assert(result.lookup_command?(2)) - end - - def test_consume_dot_after_bracket - parser = parse_context.new_parser('items[0].title') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(0, result.lookups[0]) - assert_equal('title', result.lookups[1]) - end - - def test_consume_bracket_after_dot - parser = parse_context.new_parser('product.variants[0]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal('variants', result.lookups[0]) - assert_equal(0, result.lookups[1]) - end - - def test_consume_multiple_brackets_with_different_types - parser = parse_context.new_parser('a[0]["key"][var]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(0, result.lookups[0]) - assert_equal('key', result.lookups[1]) - assert_kind_of(VariableLookup, result.lookups[2]) - end - - def test_consume_deep_nested_brackets - parser = parse_context.new_parser('a[b[c[d]]]') - result = ExpressionConsumer.consume(parser, parse_context) - inner1 = result.lookups[0] - assert_kind_of(VariableLookup, inner1) - inner2 = inner1.lookups[0] - assert_kind_of(VariableLookup, inner2) - end - - def test_consume_with_spaces_in_range - parser = parse_context.new_parser('( 1 .. 10 )') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(1..10, result) - end - - def test_consume_starting_with_bracket_then_dots - parser = parse_context.new_parser('[0].first.last') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(0, result.name) - assert(result.lookup_command?(0)) - assert(result.lookup_command?(1)) - end - - def test_consume_only_dot_lookups - parser = parse_context.new_parser('a.b.c.d') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(['b', 'c', 'd'], result.lookups) - end - - def test_consume_only_bracket_lookups - parser = parse_context.new_parser('a[0][1][2]') - result = ExpressionConsumer.consume(parser, parse_context) - assert_equal(3, result.lookups.length) - assert_equal(0, result.lookups[0]) - assert_equal(1, result.lookups[1]) - assert_equal(2, result.lookups[2]) - end - - def test_consume_complex_nested_expression - parser = parse_context.new_parser('product.variants[index].title') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(VariableLookup, result.lookups[1]) - assert_equal('index', result.lookups[1].name) - end - - def test_consume_range_with_bracketed_variables - parser = parse_context.new_parser('(items[0]..items[1])') - result = ExpressionConsumer.consume(parser, parse_context) - assert_kind_of(RangeLookup, result) - assert_kind_of(VariableLookup, result.start_obj) - end - - def test_consume_evaluates_correctly - parser = parse_context.new_parser('product') - result = ExpressionConsumer.consume(parser, parse_context) - context = Context.new({ 'product' => 'Test Product' }) - assert_equal('Test Product', context.evaluate(result)) - end - - def test_consume_with_lookups_evaluates_correctly - parser = parse_context.new_parser('product.title') - result = ExpressionConsumer.consume(parser, parse_context) - context = Context.new({ 'product' => { 'title' => 'My Title' } }) - assert_equal('My Title', context.evaluate(result)) - end - - def test_consume_range_evaluates_correctly - parser = parse_context.new_parser('(start..end)') - result = ExpressionConsumer.consume(parser, parse_context) - context = Context.new({ 'start' => 1, 'end' => 5 }) - assert_equal(1..5, context.evaluate(result)) - end - - private - - def parse_context - @parse_context ||= ParseContext.new(environment: Environment.build(error_mode: :rigid)) - end -end diff --git a/test/unit/expression_parser_test.rb b/test/unit/expression_parser_test.rb deleted file mode 100644 index 8cc1dd31d..000000000 --- a/test/unit/expression_parser_test.rb +++ /dev/null @@ -1,102 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -class ExpressionParserTest < Minitest::Test - include Liquid - - def test_parse_returns_nil_for_empty_string - result = ExpressionParser.parse('', parse_context) - assert_nil(result) - end - - def test_parse_returns_nil_for_whitespace_only - result = ExpressionParser.parse(' ', parse_context) - assert_nil(result) - end - - def test_parse_raises_on_extra_tokens_after_expression - error = assert_raises(SyntaxError) do - ExpressionParser.parse('foo bar', parse_context) - end - assert_match(/Expected end_of_string but found id/, error.message) - end - - def test_parse_string_literal_with_double_quotes - result = ExpressionParser.parse('"hello"', parse_context) - assert_equal('hello', result) - end - - def test_parse_string_literal_with_single_quotes - result = ExpressionParser.parse("'world'", parse_context) - assert_equal('world', result) - end - - def test_parse_integer_literal - result = ExpressionParser.parse('42', parse_context) - assert_equal(42, result) - end - - def test_parse_float_literal - result = ExpressionParser.parse('3.14', parse_context) - assert_equal(3.14, result) - end - - def test_parse_nil_literal - result = ExpressionParser.parse('nil', parse_context) - assert_nil(result) - end - - def test_parse_true_literal - result = ExpressionParser.parse('true', parse_context) - assert_equal(true, result) - end - - def test_parse_simple_variable - result = ExpressionParser.parse('product', parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('product', result.name) - end - - def test_parse_variable_with_dot_lookup - result = ExpressionParser.parse('product.title', parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('product', result.name) - assert_equal(['title'], result.lookups) - end - - def test_parse_variable_with_bracket_lookup - result = ExpressionParser.parse('items[0]', parse_context) - assert_kind_of(VariableLookup, result) - assert_equal('items', result.name) - assert_equal(0, result.lookups[0]) - end - - def test_parse_range_with_integer_literals - result = ExpressionParser.parse('(1..5)', parse_context) - assert_kind_of(Range, result) - assert_equal(1..5, result) - end - - def test_parse_range_with_variables - result = ExpressionParser.parse('(start..end)', parse_context) - assert_kind_of(RangeLookup, result) - end - - def test_parse_validates_end_of_string - result = ExpressionParser.parse('foo', parse_context) - assert_kind_of(VariableLookup, result) - end - - def test_parse_evaluates_correctly - result = ExpressionParser.parse('product.title', parse_context) - context = Context.new({ 'product' => { 'title' => 'My Title' } }) - assert_equal('My Title', context.evaluate(result)) - end - - private - - def parse_context - @parse_context ||= ParseContext.new(environment: Environment.build(error_mode: :rigid)) - end -end diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb index d1cd615ed..595c6717c 100644 --- a/test/unit/rigid_mode_unit_test.rb +++ b/test/unit/rigid_mode_unit_test.rb @@ -5,6 +5,10 @@ class RigidModeUnitTest < Minitest::Test include Liquid + def setup + skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + end + def test_direct_parse_expression_comparison test_cases = [ 'foo bar', From 5625f6f2babf78db3519a1e852c4b2a592073f70 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 2 Oct 2025 10:24:32 +0200 Subject: [PATCH 09/41] Fix cycle tag - `respond_to?` was returning `false` in the parser switcher because `rigid_parse` was private It was working before because `parse_context` was doing the double-parsing thing, but when we removed that, this test fairly started breaking --- lib/liquid/tags/cycle.rb | 4 ++-- test/integration/tags/cycle_tag_test.rb | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index ad1058bd9..2df55bfba 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -52,8 +52,6 @@ def render_to_output_buffer(context, output) output end - private - # cycle [name:] expression(, expression)* def rigid_parse(markup) p = @parse_context.new_parser(markup) @@ -73,6 +71,8 @@ def rigid_parse(markup) raise_syntax_error(options) if @variables.empty? end + private + # Temporarily until we migrate def strict_parse(markup) lax_parse(markup) diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index 8452c4678..8cd6daac4 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -55,10 +55,9 @@ def test_cycle_tag_with_error_mode end end - skip("todo(guilherme): parse_context.safe_parse_expression in progress...") - # with_error_mode(:rigid) do - # assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") } - # assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") } - # end + with_error_mode(:rigid) do + assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") } + assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") } + end end end From 42fcbcc59f30be1d4283f73e1993c6b80c393c9f Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 2 Oct 2025 11:29:49 +0200 Subject: [PATCH 10/41] Fix an int the `cycle` tag, add extra unit tests, and updated parser switcher: - Fixed NoMethod error with .peek (using look instead) - Add friendlier error message when {% cycle %} --- lib/liquid/parser_switching.rb | 13 ++++++++----- lib/liquid/tags/cycle.rb | 15 +++++++++++---- lib/liquid/tags/for.rb | 4 ++++ lib/liquid/tags/if.rb | 4 ++++ test/integration/tags/cycle_tag_test.rb | 24 ++++++++++++++++++++---- 5 files changed, 47 insertions(+), 13 deletions(-) diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index a978dc43a..78b86b23d 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -2,9 +2,12 @@ module Liquid module ParserSwitching - # Do not use this. Use parse_with_selected_parser instead. - # It's basically doing the same thing, except this will use strict_parse regardless - # of the error mode and fallback only if strict throws. + # 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. + # + # @deprecated Use {#parse_with_selected_parser} instead. def strict_parse_with_error_mode_fallback(markup) strict_parse_with_error_context(markup) rescue SyntaxError => e @@ -26,7 +29,7 @@ def parse_with_selected_parser(markup) when :lax then lax_parse(markup) when :warn begin - strict_parse_with_error_context(markup) + rigid_parse_with_error_context(markup) rescue SyntaxError => e parse_context.warnings << e lax_parse(markup) @@ -37,7 +40,7 @@ def parse_with_selected_parser(markup) private def rigid_parse_with_error_context(markup) - respond_to?(:rigid_parse) ? rigid_parse(markup) : strict_parse(markup) + rigid_parse(markup) rescue SyntaxError => e e.line_number = line_number e.markup_context = markup_context(markup) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 2df55bfba..d4f7166aa 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -52,27 +52,34 @@ def render_to_output_buffer(context, output) output end + private + # cycle [name:] expression(, expression)* def rigid_parse(markup) p = @parse_context.new_parser(markup) - if p.look(:id) && p.peek(1) == :colon + if p.look(:id) && p.look(:colon, 1) @name = p.consume(:id) @is_named = true p.consume(:colon) end @variables = [] + + raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string) + while (var = p.expression) + var = parse_expression(var) @variables << var break unless p.consume?(:comma) end - raise_syntax_error(options) if @variables.empty? + unless @is_named + @name = @variables.to_s + @is_named = !@name.match?(/\w+:0x\h{8}/) + end end - private - # Temporarily until we migrate def strict_parse(markup) lax_parse(markup) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 6aa308f1c..c2be5db1d 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -111,6 +111,10 @@ def strict_parse(markup) private + def rigid_parse(markup) + strict_parse(markup) + end + def collection_segment(context) offsets = context.registers[:for] ||= {} diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 040fecb84..342374f1c 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -66,6 +66,10 @@ def render_to_output_buffer(context, output) private + def rigid_parse(markup) + strict_parse(markup) + end + def push_block(tag, markup) block = if tag == 'else' ElseCondition.new diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index 8cd6daac4..a40424796 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -46,18 +46,34 @@ def test_cycle_tag_always_resets_cycle assert_template_result("11", template) end + def test_cycle_tag_without_arguments + error = assert_raises(Liquid::SyntaxError) do + Template.parse("{% cycle %}") + end + + assert_match(/Syntax Error in 'cycle' - Valid syntax: cycle \[name :\] var/, error.message) + end + def test_cycle_tag_with_error_mode # QuotedFragment is more permissive than what Parser#expression allows. + temlate1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" + temlate2 = "{% cycle .5: 'a', 'b' %}" + [:lax, :strict].each do |mode| with_error_mode(mode) do - assert_template_result("a", "{% cycle .5: 'a', 'b' %}") - assert_template_result("b", "{% assign 5 = 'b' %}{% cycle .5, .4 %}") + assert_template_result("b", temlate1) + assert_template_result("a", temlate2) end end with_error_mode(:rigid) do - assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") } - assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") } + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate2) } + + expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/ + + assert_match(expected_error, error1.message) + assert_match(expected_error, error2.message) end end end From 0d7a7a514c57c03b2fc2996b1c99b0388f90225d Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 2 Oct 2025 12:38:03 +0200 Subject: [PATCH 11/41] Fail with trailing elements in the `cycle` tag --- lib/liquid/tags/cycle.rb | 2 ++ test/integration/tags/cycle_tag_test.rb | 36 +++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index d4f7166aa..745fd3536 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -74,6 +74,8 @@ def rigid_parse(markup) break unless p.consume?(:comma) end + p.consume(:end_of_string) + unless @is_named @name = @variables.to_s @is_named = !@name.match?(/\w+:0x\h{8}/) diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index a40424796..dde8a1bc1 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -76,4 +76,40 @@ def test_cycle_tag_with_error_mode assert_match(expected_error, error2.message) end end + + def test_cycle_with_trailing_elements + assignments = "{% assign a = 'A' %}{% assign n = 'N' %}" + + template1 = "#{assignments}{% cycle 'a' 'b', 'c' %}" + template2 = "#{assignments}{% cycle name: 'a' 'b', 'c' %}" + template3 = "#{assignments}{% cycle name: 'a', 'b' 'c' %}" + template4 = "#{assignments}{% cycle n e: 'a', 'b', 'c' %}" + template5 = "#{assignments}{% cycle n e 'a', 'b', 'c' %}" + + [:lax, :strict].each do |mode| + with_error_mode(mode) do + assert_template_result("a", template1) + assert_template_result("a", template2) + assert_template_result("a", template3) + assert_template_result("N", template4) + assert_template_result("N", template5) + end + end + + with_error_mode(:rigid) do + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } + error3 = assert_raises(Liquid::SyntaxError) { Template.parse(template3) } + error4 = assert_raises(Liquid::SyntaxError) { Template.parse(template4) } + error5 = assert_raises(Liquid::SyntaxError) { Template.parse(template5) } + + expected_error = /Expected end_of_string but found/ + + assert_match(expected_error, error1.message) + assert_match(expected_error, error2.message) + assert_match(expected_error, error3.message) + assert_match(expected_error, error4.message) + assert_match(expected_error, error5.message) + end + end end From 8e6faa43f5e5e68f46ecc234d0cd10d9c43dcf35 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 2 Oct 2025 12:51:42 +0200 Subject: [PATCH 12/41] Add rigid_parse to `case/when` --- lib/liquid/parser_switching.rb | 4 + lib/liquid/tags/case.rb | 49 +++++++++-- test/unit/tags/case_tag_unit_test.rb | 120 +++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 6 deletions(-) diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 78b86b23d..799250a06 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -37,6 +37,10 @@ def parse_with_selected_parser(markup) end end + def rigid_mode? + parse_context.error_mode == :rigid + end + private def rigid_parse_with_error_context(markup) diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 6b67601fe..bdf329a2b 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -31,12 +31,7 @@ class Case < Block def initialize(tag_name, markup, options) super @blocks = [] - - if markup =~ Syntax - @left = parse_expression(Regexp.last_match(1)) - else - raise SyntaxError, options[:locale].t("errors.syntax.case") - end + parse_with_selected_parser(markup) end def parse(tokens) @@ -91,9 +86,51 @@ def render_to_output_buffer(context, output) private + def rigid_parse(markup) + parser = @parse_context.new_parser(markup) + @left = safe_parse_expression(parser) + parser.consume(:end_of_string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) + if markup =~ Syntax + @left = parse_expression(Regexp.last_match(1)) + else + raise SyntaxError, options[:locale].t("errors.syntax.case") + end + end + def record_when_condition(markup) body = new_body + if rigid_mode? + parse_rigid_when(markup, body) + else + parse_lax_when(markup, body) + end + end + + def parse_rigid_when(markup, body) + parser = @parse_context.new_parser(markup) + + loop do + expr = safe_parse_expression(parser) + block = Condition.new(@left, '==', expr) + block.attach(body) + @blocks << block + + # Temporarily until support :or lexeme. + break unless parser.id?('or') || parser.consume?(:comma) + end + + parser.consume(:end_of_string) + end + + def parse_lax_when(markup, body) while markup unless markup =~ WhenSyntax raise SyntaxError, options[:locale].t("errors.syntax.case_invalid_when") diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index a94d167f5..687cf9d91 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -9,4 +9,124 @@ def test_case_nodelist template = Liquid::Template.parse('{% case var %}{% when true %}WHEN{% else %}ELSE{% endcase %}') assert_equal(['WHEN', 'ELSE'], template.root.nodelist[0].nodelist.map(&:nodelist).flatten) end + + def test_case_with_trailing_element + template = <<~LIQUID + {%- case 1 bar -%} + {%- when 1 -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + [:lax, :strict].each do |mode| + with_error_mode(mode) { assert_template_result("one", template) } + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Expected end_of_string but found/, error.message) + end + end + + def test_case_when_trailing_element + template = <<~LIQUID + {%- case 1 -%} + {%- when 1 bar -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + [:lax, :strict].each do |mode| + with_error_mode(mode) { assert_template_result("one", template) } + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Expected end_of_string but found/, error.message) + end + end + + def test_case_when_with_comma + template = <<~LIQUID + {%- case 1 -%} + {%- when 2, 1 -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + [:lax, :strict, :rigid].each do |mode| + with_error_mode(mode) { assert_template_result("one", template) } + end + end + + def test_case_when_with_or + template = <<~LIQUID + {%- case 1 -%} + {%- when 2 or 1 -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + + [:lax, :strict, :rigid].each do |mode| + with_error_mode(mode) { assert_template_result("one", template) } + end + end + + def test_case_with_invalid_expression + template = <<~LIQUID + {%- case foo=>bar -%} + {%- when 'baz' -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + assigns = { 'foo' => { 'bar' => 'baz' } } + + [:lax, :strict].each do |mode| + with_error_mode(mode) do + assert_template_result("one", template, assigns) + end + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Unexpected character =/, error.message) + end + end + + def test_case_when_with_invalid_expression + template = <<~LIQUID + {%- case 'baz' -%} + {%- when foo=>bar -%} + one + {%- else -%} + two + {%- endcase -%} + LIQUID + assigns = { 'foo' => { 'bar' => 'baz' } } + + [:lax, :strict].each do |mode| + with_error_mode(mode) do + assert_template_result("one", template, assigns) + end + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + + assert_match(/Unexpected character =/, error.message) + end + end end From a16b955910891aa1c443fb33a32519c9e3bb704a Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 2 Oct 2025 12:52:01 +0200 Subject: [PATCH 13/41] Remove unnecessary skips --- test/unit/rigid_mode_unit_test.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb index 595c6717c..1071d483f 100644 --- a/test/unit/rigid_mode_unit_test.rb +++ b/test/unit/rigid_mode_unit_test.rb @@ -5,11 +5,9 @@ class RigidModeUnitTest < Minitest::Test include Liquid - def setup + def test_direct_parse_expression_comparison skip("todo(guilherme): parse_context.safe_parse_expression in progress...") - end - def test_direct_parse_expression_comparison test_cases = [ 'foo bar', 'user.name first', @@ -32,6 +30,8 @@ def test_direct_parse_expression_comparison end def test_comparison_strict_vs_rigid_with_space_separated_lookups + skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + expr = 'product title' ctx_lax = ParseContext.new(environment: lax_env) @@ -51,6 +51,8 @@ def test_comparison_strict_vs_rigid_with_space_separated_lookups end def test_tablerow_limit_with_invalid_expression + skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + template = <<~LIQUID {% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %} LIQUID @@ -64,6 +66,8 @@ def test_tablerow_limit_with_invalid_expression end def test_tablerow_offset_with_invalid_expression + skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + template = <<~LIQUID {% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %} LIQUID From c36852de4a6fedee3dc69bb82ec0c080a6e05a53 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 2 Oct 2025 12:58:14 +0200 Subject: [PATCH 14/41] Use safe_parse_expression instead of parse_expression --- lib/liquid/tags/cycle.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 745fd3536..5a2048fca 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -68,8 +68,7 @@ def rigid_parse(markup) raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string) - while (var = p.expression) - var = parse_expression(var) + while (var = safe_parse_expression(p)) @variables << var break unless p.consume?(:comma) end From 533396bf4e794a81d942b201205d3f48a0b63ca8 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 7 Oct 2025 13:50:48 -0400 Subject: [PATCH 15/41] Make it possible to safe_parse subsets of expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit e.g. sometimes you want to only accept strings | lookups. {% render snippetName %} for example. snippetName is a string right now. We don't want safe_parse_expression because this would allow snippetName to be a number, a boolean, etc. But we still want to strict parse this. So what we'll do is use parse_expression(string, safe: true), this is an optional opt-in to say "I know what I'm doing". Usually that's because you're using the output of Parser#something as the input of parse_expression. It is true that Parser#expression is subset of Expression.parse, it is not true of the opposite (e.g. Expression.parse doesn't care about .5 and happily parses that as a global lookup of the variable named "5", Parser#expression throws for that.) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index e5c321dc..9ab350f0 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -48,8 +48,8 @@ module Liquid @@operators end - def self.parse_expression(parse_context, markup) - @@method_literals[markup] || parse_context.parse_expression(markup) + def self.parse_expression(parse_context, markup, safe: false) + @@method_literals[markup] || parse_context.parse_expression(markup, safe: safe) end attr_reader :attachment, :child_condition diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 1c59fe4a..82cf5768 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -51,13 +51,13 @@ module Liquid end def safe_parse_expression(parser) - Expression.safe_parse(parser) + Expression.safe_parse(parser, @string_scanner, @expression_cache) end - def parse_expression(markup) + def parse_expression(markup, safe: false) # todo(guilherme): remove this once rigid mode is fully using safe_parse_expression - # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" if @error_mode == :rigid - puts("🚨 parse_expression used in rigid mode") if @error_mode == :rigid + # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" if !safe && @error_mode == :rigid + puts("🚨 parse_expression used in rigid mode") if !safe && @error_mode == :rigid Expression.parse(markup, @string_scanner, @expression_cache) end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 656d2e47..374ee511 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -72,8 +72,8 @@ module Liquid parse_context.safe_parse_expression(parser) end - def parse_expression(markup) - parse_context.parse_expression(markup) + def parse_expression(markup, safe: false) + parse_context.parse_expression(markup, safe: safe) end end end diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index c2be5db1..3182983b 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -93,7 +93,7 @@ module Liquid raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') collection_name = p.expression - @collection_name = parse_expression(collection_name) + @collection_name = parse_expression(collection_name, safe: true) @name = "#{@variable_name}-#{collection_name}" @reversed = p.id?('reversed') diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 342374f1..e25d6250 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -81,8 +81,8 @@ module Liquid block.attach(new_body) end - def parse_expression(markup) - Condition.parse_expression(parse_context, markup) + def parse_expression(markup, safe: false) + Condition.parse_expression(parse_context, markup, safe: safe) end def lax_parse(markup) @@ -124,9 +124,9 @@ module Liquid end def parse_comparison(p) - a = parse_expression(p.expression) + a = parse_expression(p.expression, safe: true) if (op = p.consume?(:comparison)) - b = parse_expression(p.expression) + b = parse_expression(p.expression, safe: true) Condition.new(a, op, b) else Condition.new(a) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 6cdbfd6f..b72a235b 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -87,10 +87,11 @@ module Liquid def rigid_parse(markup) p = @parse_context.new_parser(markup) - template_name = p.expression + @template_name_expr = safe_parse_expression(p) with_or_for = p.id?("for") || p.id?("with") || nil + @variable_name_expr = nil if with_or_for - variable_name = p.expression + @variable_name_expr = parse_expression(p.consume(:id), safe: true) end alias_name = nil @@ -98,8 +99,6 @@ module Liquid alias_name = p.consume(:id) end - @template_name_expr = parse_expression(template_name) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil @alias_name = alias_name # optional comma @@ -109,7 +108,7 @@ module Liquid while p.look(:id) key = p.consume p.consume(:colon) - @attributes[key] = parse_expression(p.expression) + @attributes[key] = safe_parse_expression(p) p.consume?(:comma) # optional comma end end diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 89c11063..4f716b24 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -88,10 +88,11 @@ module Liquid def rigid_parse(markup) p = @parse_context.new_parser(markup) - template_name = rigid_template_name(p) + @template_name_expr = parse_expression(rigid_template_name(p), safe: true) + @variable_name_expr = nil with_or_for = p.id?("for") || p.id?("with") || nil if with_or_for - variable_name = p.expression + @variable_name_expr = safe_parse_expression(p) end alias_name = nil @@ -99,8 +100,6 @@ module Liquid alias_name = p.consume(:id) end - @template_name_expr = parse_expression(template_name) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil @alias_name = alias_name @is_for_loop = (with_or_for == FOR) @@ -111,7 +110,7 @@ module Liquid while p.look(:id) key = p.consume p.consume(:colon) - @attributes[key] = parse_expression(p.expression) + @attributes[key] = safe_parse_expression(p) p.consume?(:comma) # optional comma end end diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 20957065..a3623bc5 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -65,11 +65,11 @@ module Liquid return if p.look(:end_of_string) - @name = parse_context.parse_expression(p.expression) + @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) + @filters << parse_filter_expressions(filtername, filterargs, safe: true) end p.consume(:end_of_string) end @@ -122,15 +122,15 @@ module Liquid private - def parse_filter_expressions(filter_name, unparsed_args) + def parse_filter_expressions(filter_name, unparsed_args, safe: false) filter_args = [] keyword_args = nil unparsed_args.each do |a| - if (matches = a.match(JustTagAttributes)) + if (matches = a.match(JustTagAttributes)) # we'll need to fix this keyword_args ||= {} - keyword_args[matches[1]] = parse_context.parse_expression(matches[2]) + keyword_args[matches[1]] = parse_context.parse_expression(matches[2], safe: false) else - filter_args << parse_context.parse_expression(a) + filter_args << parse_context.parse_expression(a, safe: safe) end end result = [filter_name, filter_args] --- lib/liquid/condition.rb | 4 ++-- lib/liquid/parse_context.rb | 8 ++++---- lib/liquid/tag.rb | 4 ++-- lib/liquid/tags/for.rb | 2 +- lib/liquid/tags/if.rb | 8 ++++---- lib/liquid/tags/include.rb | 9 ++++----- lib/liquid/tags/render.rb | 9 ++++----- lib/liquid/variable.rb | 12 ++++++------ 8 files changed, 27 insertions(+), 29 deletions(-) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index e5c321dca..9ab350f07 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -48,8 +48,8 @@ def self.operators @@operators end - def self.parse_expression(parse_context, markup) - @@method_literals[markup] || parse_context.parse_expression(markup) + def self.parse_expression(parse_context, markup, safe: false) + @@method_literals[markup] || parse_context.parse_expression(markup, safe: safe) end attr_reader :attachment, :child_condition diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 1c59fe4a6..82cf5768c 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -51,13 +51,13 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) end def safe_parse_expression(parser) - Expression.safe_parse(parser) + Expression.safe_parse(parser, @string_scanner, @expression_cache) end - def parse_expression(markup) + def parse_expression(markup, safe: false) # todo(guilherme): remove this once rigid mode is fully using safe_parse_expression - # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" if @error_mode == :rigid - puts("🚨 parse_expression used in rigid mode") if @error_mode == :rigid + # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" if !safe && @error_mode == :rigid + puts("🚨 parse_expression used in rigid mode") if !safe && @error_mode == :rigid Expression.parse(markup, @string_scanner, @expression_cache) end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 656d2e47c..374ee511e 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -72,8 +72,8 @@ def safe_parse_expression(parser) parse_context.safe_parse_expression(parser) end - def parse_expression(markup) - parse_context.parse_expression(markup) + def parse_expression(markup, safe: false) + parse_context.parse_expression(markup, safe: safe) end end end diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index c2be5db1d..3182983b4 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -93,7 +93,7 @@ def strict_parse(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') collection_name = p.expression - @collection_name = parse_expression(collection_name) + @collection_name = parse_expression(collection_name, safe: true) @name = "#{@variable_name}-#{collection_name}" @reversed = p.id?('reversed') diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 342374f1c..e25d62505 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -81,8 +81,8 @@ def push_block(tag, markup) block.attach(new_body) end - def parse_expression(markup) - Condition.parse_expression(parse_context, markup) + def parse_expression(markup, safe: false) + Condition.parse_expression(parse_context, markup, safe: safe) end def lax_parse(markup) @@ -124,9 +124,9 @@ def parse_binary_comparisons(p) end def parse_comparison(p) - a = parse_expression(p.expression) + a = parse_expression(p.expression, safe: true) if (op = p.consume?(:comparison)) - b = parse_expression(p.expression) + b = parse_expression(p.expression, safe: true) Condition.new(a, op, b) else Condition.new(a) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 6cdbfd6f2..b72a235b3 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -87,10 +87,11 @@ def render_to_output_buffer(context, output) def rigid_parse(markup) p = @parse_context.new_parser(markup) - template_name = p.expression + @template_name_expr = safe_parse_expression(p) with_or_for = p.id?("for") || p.id?("with") || nil + @variable_name_expr = nil if with_or_for - variable_name = p.expression + @variable_name_expr = parse_expression(p.consume(:id), safe: true) end alias_name = nil @@ -98,8 +99,6 @@ def rigid_parse(markup) alias_name = p.consume(:id) end - @template_name_expr = parse_expression(template_name) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil @alias_name = alias_name # optional comma @@ -109,7 +108,7 @@ def rigid_parse(markup) while p.look(:id) key = p.consume p.consume(:colon) - @attributes[key] = parse_expression(p.expression) + @attributes[key] = safe_parse_expression(p) p.consume?(:comma) # optional comma end end diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 89c11063c..4f716b246 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -88,10 +88,11 @@ def render_tag(context, output) def rigid_parse(markup) p = @parse_context.new_parser(markup) - template_name = rigid_template_name(p) + @template_name_expr = parse_expression(rigid_template_name(p), safe: true) + @variable_name_expr = nil with_or_for = p.id?("for") || p.id?("with") || nil if with_or_for - variable_name = p.expression + @variable_name_expr = safe_parse_expression(p) end alias_name = nil @@ -99,8 +100,6 @@ def rigid_parse(markup) alias_name = p.consume(:id) end - @template_name_expr = parse_expression(template_name) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil @alias_name = alias_name @is_for_loop = (with_or_for == FOR) @@ -111,7 +110,7 @@ def rigid_parse(markup) while p.look(:id) key = p.consume p.consume(:colon) - @attributes[key] = parse_expression(p.expression) + @attributes[key] = safe_parse_expression(p) p.consume?(:comma) # optional comma end end diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 209570654..a3623bc53 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -65,11 +65,11 @@ def strict_parse(markup) return if p.look(:end_of_string) - @name = parse_context.parse_expression(p.expression) + @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) + @filters << parse_filter_expressions(filtername, filterargs, safe: true) end p.consume(:end_of_string) end @@ -122,15 +122,15 @@ def disabled_tags private - def parse_filter_expressions(filter_name, unparsed_args) + def parse_filter_expressions(filter_name, unparsed_args, safe: false) filter_args = [] keyword_args = nil unparsed_args.each do |a| - if (matches = a.match(JustTagAttributes)) + if (matches = a.match(JustTagAttributes)) # we'll need to fix this keyword_args ||= {} - keyword_args[matches[1]] = parse_context.parse_expression(matches[2]) + keyword_args[matches[1]] = parse_context.parse_expression(matches[2], safe: false) else - filter_args << parse_context.parse_expression(a) + filter_args << parse_context.parse_expression(a, safe: safe) end end result = [filter_name, filter_args] From 1308b9780cec697c22c9f59f546d41e77182c703 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 8 Oct 2025 15:42:17 -0400 Subject: [PATCH 16/41] 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 799250a06..c6b09bc86 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -5,11 +5,17 @@ module ParserSwitching # 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 a3623bc53..f355d694b 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -54,7 +54,7 @@ def lax_parse(markup) 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 @@ def strict_parse(markup) 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 @@ def disabled_tags 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 @@ def parse_filter_expressions(filter_name, unparsed_args, safe: false) 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 2cc42e7d3..c8e572c97 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -135,14 +135,52 @@ def test_lax_filter_argument_parsing 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 From 5718f8b11426c9b1ad3e917745290f4693399e62 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 8 Oct 2025 15:50:33 -0400 Subject: [PATCH 17/41] rigid set_attribute in for parsing --- lib/liquid/tags/for.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 3182983b4..da06d64ad 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -104,7 +104,7 @@ def strict_parse(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_attribute") end p.consume(:colon) - set_attribute(attribute, p.expression) + set_attribute(attribute, p.expression, safe: true) end p.consume(:end_of_string) end @@ -178,16 +178,16 @@ def render_segment(context, output, segment) output end - def set_attribute(key, expr) + def set_attribute(key, expr, safe: false) case key when 'offset' @from = if expr == 'continue' :continue else - parse_expression(expr) + parse_expression(expr, safe: safe) end when 'limit' - @limit = parse_expression(expr) + @limit = parse_expression(expr, safe: safe) end end From 1ba6fab494694805cc8bf7e81dea4f4b22ee64a5 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 8 Oct 2025 14:27:01 -0400 Subject: [PATCH 18/41] No longer test `ParseContext` directly on `RigidModeUnitTest` as now the `safe: true` calls are considered safe Test the entire template instead --- lib/liquid/parse_context.rb | 6 +- test/unit/rigid_mode_unit_test.rb | 141 +++++++++++++++++------------- 2 files changed, 85 insertions(+), 62 deletions(-) diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 82cf5768c..1b192d8cb 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -56,8 +56,10 @@ def safe_parse_expression(parser) def parse_expression(markup, safe: false) # todo(guilherme): remove this once rigid mode is fully using safe_parse_expression - # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" if !safe && @error_mode == :rigid - puts("🚨 parse_expression used in rigid mode") if !safe && @error_mode == :rigid + if !safe && @error_mode == :rigid + # raise Liquid::InternalError, "parse_expression is not supported in rigid mode" + puts("🚨 parse_expression used in rigid mode") + end Expression.parse(markup, @string_scanner, @expression_cache) end diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb index 1071d483f..0658c719c 100644 --- a/test/unit/rigid_mode_unit_test.rb +++ b/test/unit/rigid_mode_unit_test.rb @@ -6,57 +6,38 @@ class RigidModeUnitTest < Minitest::Test include Liquid def test_direct_parse_expression_comparison - skip("todo(guilherme): parse_context.safe_parse_expression in progress...") - test_cases = [ - 'foo bar', - 'user.name first', - 'items[0] next', - 'products[0].name extra', + '{{ foo bar }}', + '{{ user.name first }}', + '{{ items[0] next }}', + '{{ products[0].name extra }}', ] - test_cases.each do |expr| - ctx_strict = ParseContext.new(environment: strict_env) - result = ctx_strict.parse_expression(expr) - refute_nil(result, "Strict mode should parse '#{expr}'") - - ctx_rigid = ParseContext.new(environment: rigid_env) - error = assert_raises(SyntaxError) do - ctx_rigid.parse_expression(expr) - end - - assert_match(/Expected end_of_string but found id/, error.message) + test_cases.each do |template| + refute_nil(lax_parse(template)) + assert_raises(SyntaxError) { strict_parse(template) } + assert_raises(SyntaxError) { rigid_parse(template) } end end def test_comparison_strict_vs_rigid_with_space_separated_lookups - skip("todo(guilherme): parse_context.safe_parse_expression in progress...") - - expr = 'product title' - - ctx_lax = ParseContext.new(environment: lax_env) - result_lax = ctx_lax.parse_expression(expr) - assert_equal('product', result_lax.name) - assert_equal(['title'], result_lax.lookups) + template = '{{ product title }}' - ctx_strict = ParseContext.new(environment: strict_env) - result_strict = ctx_strict.parse_expression(expr) - assert_equal('product', result_strict.name) - assert_equal(['title'], result_strict.lookups) + output = lax_parse(template).render({ 'product' => { 'title' => 'Snow' } }) + assert_equal('{"title"=>"Snow"}', output) - ctx_rigid = ParseContext.new(environment: rigid_env) - assert_raises(SyntaxError) do - ctx_rigid.parse_expression(expr) - end + assert_raises(SyntaxError) { strict_parse(template) } + assert_raises(SyntaxError) { rigid_parse(template) } end def test_tablerow_limit_with_invalid_expression - skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + skip template = <<~LIQUID {% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %} LIQUID + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -66,12 +47,13 @@ def test_tablerow_limit_with_invalid_expression end def test_tablerow_offset_with_invalid_expression - skip("todo(guilherme): parse_context.safe_parse_expression in progress...") + skip template = <<~LIQUID {% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %} LIQUID + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -87,6 +69,7 @@ def test_cycle_name_with_invalid_expression {% endfor %} LIQUID + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -102,6 +85,7 @@ def test_cycle_variable_with_invalid_expression {% endfor %} LIQUID + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -118,6 +102,7 @@ def test_case_with_invalid_expression {% endcase %} LIQUID + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -129,6 +114,7 @@ def test_case_with_invalid_expression def test_include_template_with_invalid_expression template = "{% include foo=>bar %}" + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -140,6 +126,7 @@ def test_include_template_with_invalid_expression def test_include_with_invalid_expression template = '{% include "snippet" with foo=>bar %}' + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -151,6 +138,7 @@ def test_include_with_invalid_expression def test_include_attribute_with_invalid_expression template = '{% include "snippet", key: foo=>bar %}' + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -162,6 +150,7 @@ def test_include_attribute_with_invalid_expression def test_render_with_invalid_expression template = '{% render "snippet" with foo=>bar %}' + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -173,6 +162,7 @@ def test_render_with_invalid_expression def test_render_attribute_with_invalid_expression template = '{% render "snippet", key: foo=>bar %}' + refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) error = assert_raises(SyntaxError) do @@ -193,9 +183,14 @@ def test_valid_expressions_work_in_rigid_mode } test_cases.each do |template_str, data| - t = rigid_parse(template_str) - result = t.render(data) - assert(result.is_a?(String), "Should render successfully for '#{template_str}'") + lax_result = lax_parse(template_str).render(data) + assert(lax_result.is_a?(String), "Lax mode should render '#{template_str}'") + + strict_result = strict_parse(template_str).render(data) + assert(strict_result.is_a?(String), "Strict mode should render '#{template_str}'") + + rigid_result = rigid_parse(template_str).render(data) + assert(rigid_result.is_a?(String), "Rigid mode should render '#{template_str}'") end end @@ -204,9 +199,14 @@ def test_rigid_mode_with_ranges {% for i in (1..3) %}{{ i }}{% endfor %} LIQUID - t = rigid_parse(template) - result = t.render - assert_equal("123\n", result) + lax_result = lax_parse(template).render + assert_equal("123\n", lax_result) + + strict_result = strict_parse(template).render + assert_equal("123\n", strict_result) + + rigid_result = rigid_parse(template).render + assert_equal("123\n", rigid_result) end def test_rigid_mode_with_variable_ranges @@ -214,9 +214,16 @@ def test_rigid_mode_with_variable_ranges {% for i in (start..end) %}{{ i }}{% endfor %} LIQUID - t = rigid_parse(template) - result = t.render({ 'start' => 1, 'end' => 3 }) - assert_equal("123\n", result) + data = { 'start' => 1, 'end' => 3 } + + lax_result = lax_parse(template).render(data) + assert_equal("123\n", lax_result) + + strict_result = strict_parse(template).render(data) + assert_equal("123\n", strict_result) + + rigid_result = rigid_parse(template).render(data) + assert_equal("123\n", rigid_result) end def test_rigid_mode_valid_filters @@ -224,9 +231,14 @@ def test_rigid_mode_valid_filters {{ "hello" | upcase | prepend: "Say: " }} LIQUID - t = rigid_parse(template) - result = t.render - assert_equal("Say: HELLO\n", result) + lax_result = lax_parse(template).render + assert_equal("Say: HELLO\n", lax_result) + + strict_result = strict_parse(template).render + assert_equal("Say: HELLO\n", strict_result) + + rigid_result = rigid_parse(template).render + assert_equal("Say: HELLO\n", rigid_result) end def test_rigid_mode_valid_filter_with_correct_variable_args @@ -234,39 +246,48 @@ def test_rigid_mode_valid_filter_with_correct_variable_args {{ "hello" | append: world.name }} LIQUID - t = rigid_parse(template) - result = t.render({ 'world' => { 'name' => ' world' } }) - assert_equal("hello world\n", result) + data = { 'world' => { 'name' => ' world' } } + + lax_result = lax_parse(template).render(data) + assert_equal("hello world\n", lax_result) + + strict_result = strict_parse(template).render(data) + assert_equal("hello world\n", strict_result) + + rigid_result = rigid_parse(template).render(data) + assert_equal("hello world\n", rigid_result) end def test_empty_expression_handling - ctx_rigid = ParseContext.new(environment: rigid_env) - result = ctx_rigid.parse_expression('') - assert_nil(result) + ctx_rigid = ParseContext.new(environment: rigid) - result = ctx_rigid.parse_expression(' ') - assert_nil(result) + assert_nil(ctx_rigid.parse_expression('', safe: true)) + assert_nil(ctx_rigid.parse_expression(' ', safe: true)) end private def rigid_parse(source) - Template.parse(source, environment: rigid_env) + Template.parse(source, environment: rigid) end def strict_parse(source) - Template.parse(source, environment: strict_env) + Template.parse(source, environment: strict) + end + + def lax_parse(source) + Template.parse(source, environment: lax) end - def lax_env + def lax Environment.build(error_mode: :lax) end - def rigid_env + def rigid Environment.build(error_mode: :rigid) end - def strict_env + def strict Environment.build(error_mode: :strict) end end From fd81ac1ab6cc9ac202e1f8436248cbc5eb3b3850 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 8 Oct 2025 14:46:01 -0400 Subject: [PATCH 19/41] Remove redundant tests where rigid and strict modes have the same behavior --- test/unit/rigid_mode_unit_test.rb | 152 ++++-------------------------- 1 file changed, 16 insertions(+), 136 deletions(-) diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb index 0658c719c..cb0de7c44 100644 --- a/test/unit/rigid_mode_unit_test.rb +++ b/test/unit/rigid_mode_unit_test.rb @@ -5,31 +5,6 @@ class RigidModeUnitTest < Minitest::Test include Liquid - def test_direct_parse_expression_comparison - test_cases = [ - '{{ foo bar }}', - '{{ user.name first }}', - '{{ items[0] next }}', - '{{ products[0].name extra }}', - ] - - test_cases.each do |template| - refute_nil(lax_parse(template)) - assert_raises(SyntaxError) { strict_parse(template) } - assert_raises(SyntaxError) { rigid_parse(template) } - end - end - - def test_comparison_strict_vs_rigid_with_space_separated_lookups - template = '{{ product title }}' - - output = lax_parse(template).render({ 'product' => { 'title' => 'Snow' } }) - assert_equal('{"title"=>"Snow"}', output) - - assert_raises(SyntaxError) { strict_parse(template) } - assert_raises(SyntaxError) { rigid_parse(template) } - end - def test_tablerow_limit_with_invalid_expression skip @@ -72,9 +47,8 @@ def test_cycle_name_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -88,9 +62,8 @@ def test_cycle_variable_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -105,9 +78,8 @@ def test_case_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -117,9 +89,8 @@ def test_include_template_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -129,9 +100,8 @@ def test_include_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -141,9 +111,8 @@ def test_include_attribute_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -153,9 +122,8 @@ def test_render_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end + error = assert_raises(SyntaxError) { rigid_parse(template) } + assert_match(/Unexpected character =/, error.message) end @@ -165,97 +133,9 @@ def test_render_attribute_with_invalid_expression refute_nil(lax_parse(template)) refute_nil(strict_parse(template)) - error = assert_raises(SyntaxError) do - rigid_parse(template) - end - assert_match(/Unexpected character =/, error.message) - end - - def test_valid_expressions_work_in_rigid_mode - test_cases = { - '{{ foo }}' => { 'foo' => 'bar' }, - '{{ foo.bar }}' => { 'foo' => { 'bar' => 'baz' } }, - '{{ items[0] }}' => { 'items' => ['first', 'second'] }, - '{{ product.variants[0].title }}' => { 'product' => { 'variants' => [{ 'title' => 'Small' }] } }, - '{{ "hello" }}' => {}, - '{{ 42 }}' => {}, - '{{ 3.14 }}' => {}, - } - - test_cases.each do |template_str, data| - lax_result = lax_parse(template_str).render(data) - assert(lax_result.is_a?(String), "Lax mode should render '#{template_str}'") - - strict_result = strict_parse(template_str).render(data) - assert(strict_result.is_a?(String), "Strict mode should render '#{template_str}'") - - rigid_result = rigid_parse(template_str).render(data) - assert(rigid_result.is_a?(String), "Rigid mode should render '#{template_str}'") - end - end - - def test_rigid_mode_with_ranges - template = <<~LIQUID - {% for i in (1..3) %}{{ i }}{% endfor %} - LIQUID - - lax_result = lax_parse(template).render - assert_equal("123\n", lax_result) - - strict_result = strict_parse(template).render - assert_equal("123\n", strict_result) - - rigid_result = rigid_parse(template).render - assert_equal("123\n", rigid_result) - end - - def test_rigid_mode_with_variable_ranges - template = <<~LIQUID - {% for i in (start..end) %}{{ i }}{% endfor %} - LIQUID - - data = { 'start' => 1, 'end' => 3 } + error = assert_raises(SyntaxError) { rigid_parse(template) } - lax_result = lax_parse(template).render(data) - assert_equal("123\n", lax_result) - - strict_result = strict_parse(template).render(data) - assert_equal("123\n", strict_result) - - rigid_result = rigid_parse(template).render(data) - assert_equal("123\n", rigid_result) - end - - def test_rigid_mode_valid_filters - template = <<~LIQUID - {{ "hello" | upcase | prepend: "Say: " }} - LIQUID - - lax_result = lax_parse(template).render - assert_equal("Say: HELLO\n", lax_result) - - strict_result = strict_parse(template).render - assert_equal("Say: HELLO\n", strict_result) - - rigid_result = rigid_parse(template).render - assert_equal("Say: HELLO\n", rigid_result) - end - - def test_rigid_mode_valid_filter_with_correct_variable_args - template = <<~LIQUID - {{ "hello" | append: world.name }} - LIQUID - - data = { 'world' => { 'name' => ' world' } } - - lax_result = lax_parse(template).render(data) - assert_equal("hello world\n", lax_result) - - strict_result = strict_parse(template).render(data) - assert_equal("hello world\n", strict_result) - - rigid_result = rigid_parse(template).render(data) - assert_equal("hello world\n", rigid_result) + assert_match(/Unexpected character =/, error.message) end def test_empty_expression_handling From 376d849873fff454940e4e4ae13ed044c015ab4c Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 8 Oct 2025 16:35:41 -0400 Subject: [PATCH 20/41] Add rigid parser to `tablerow` tag --- lib/liquid/locales/en.yml | 1 + lib/liquid/tags/table_row.rb | 38 ++++ test/integration/tags/table_row_test.rb | 257 ++++++++++++++++++++++++ test/unit/rigid_mode_unit_test.rb | 30 ++- 4 files changed, 320 insertions(+), 6 deletions(-) diff --git a/lib/liquid/locales/en.yml b/lib/liquid/locales/en.yml index cd1607b3c..b99d490c8 100644 --- a/lib/liquid/locales/en.yml +++ b/lib/liquid/locales/en.yml @@ -20,6 +20,7 @@ invalid_template_encoding: "Invalid template encoding" render: "Syntax error in tag 'render' - Template name must be a quoted string" table_row: "Syntax Error in 'table_row loop' - Valid syntax: table_row [item] in [collection] cols=3" + table_row_invalid_attribute: "Invalid attribute '%{attribute}' in tablerow loop. Valid attributes are cols, limit, offset, and range" tag_never_closed: "'%{block_name}' tag was never closed" tag_termination: "Tag '%{token}' was not properly terminated with regexp: %{tag_end}" unexpected_else: "%{block_name} tag does not expect 'else' tag" diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index 6767e4fb5..11aa4ef50 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -25,11 +25,49 @@ module Liquid # @liquid_optional_param range [untyped] A custom numeric range to iterate over. class TableRow < Block Syntax = /(\w+)\s+in\s+(#{QuotedFragment}+)/o + ALLOWED_ATTRIBUTES = ['cols', 'limit', 'offset', 'range'].freeze attr_reader :variable_name, :collection_name, :attributes def initialize(tag_name, markup, options) super + parse_with_selected_parser(markup) + end + + def rigid_parse(markup) + p = @parse_context.new_parser(markup) + + @variable_name = p.consume(:id) + + unless p.id?("in") + raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") + end + + @collection_name = safe_parse_expression(p) + + # optional comma + p.consume?(:comma) + + @attributes = {} + while p.look(:id) + key = p.consume + unless ALLOWED_ATTRIBUTES.include?(key) + raise SyntaxError, options[:locale].t("errors.syntax.table_row_invalid_attribute", attribute: key) + end + + p.consume(:colon) + @attributes[key] = safe_parse_expression(p) + p.consume?(:comma) # optional comma + end + + p.consume(:end_of_string) + end + + def strict_parse(markup) + lax_parse(markup) + end + + def lax_parse(markup) if markup =~ Syntax @variable_name = Regexp.last_match(1) @collection_name = parse_expression(Regexp.last_match(2)) diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index ddc0877df..ad12d76f1 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -255,4 +255,261 @@ def test_table_row_does_not_leak_interrupts template, ) end + + def test_tablerow_with_cols_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..6) cols: 3 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + 456 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_limit_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..10) limit: 3 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_offset_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..5) offset: 2 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 345 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_range_attribute_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..3) range: (1..10) %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_multiple_attributes_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..10) cols: 2, limit: 4, offset: 1 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 23 + 45 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_variable_collection_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow n in numbers cols: 2 %}{{ n }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 12 + 34 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render('numbers' => [1, 2, 3, 4]) + assert_equal(expected, result) + end + + def test_tablerow_with_dotted_access_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow n in obj.numbers cols: 2 %}{{ n }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 12 + 34 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render('obj' => { 'numbers' => [1, 2, 3, 4] }) + assert_equal(expected, result) + end + + def test_tablerow_with_bracketed_access_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow n in obj["numbers"] cols: 2 %}{{ n }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 1020 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render('obj' => { 'numbers' => [10, 20] }) + assert_equal(expected, result) + end + + def test_tablerow_without_attributes_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..3) %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_trailing_comma_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in (1..3) cols: 2, %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 12 + 3 + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render + assert_equal(expected, result) + end + + def test_tablerow_with_invalid_attribute_name_in_rigid_mode + template = '{% tablerow i in (1..10) invalid_attr: 5 %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: Invalid attribute 'invalid_attr' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid_attr: 5\"", error.message) + end + + def test_tablerow_with_invalid_expression_in_limit_in_rigid_mode + template = '{% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) limit: foo=>bar\"", error.message) + end + + def test_tablerow_with_invalid_expression_in_offset_in_rigid_mode + template = '{% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) offset: foo=>bar\"", error.message) + end + + def test_tablerow_with_invalid_expression_in_cols_in_rigid_mode + template = '{% tablerow i in (1..10) cols: foo=>bar %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) cols: foo=>bar\"", error.message) + end + + def test_tablerow_with_invalid_expression_in_range_in_rigid_mode + template = '{% tablerow i in (1..10) range: foo=>bar %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) range: foo=>bar\"", error.message) + end + + def test_tablerow_without_in_keyword_in_rigid_mode + template = '{% tablerow i (1..10) %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) + end + + def test_tablerow_with_multiple_invalid_attributes_reports_first_in_rigid_mode + template = '{% tablerow i in (1..10) invalid1: 5, invalid2: 10 %}{{ i }}{% endtablerow %}' + error = assert_raises(SyntaxError) do + Template.parse(template, environment: rigid_environment) + end + assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) + end + + def test_tablerow_with_empty_collection_in_rigid_mode + template = <<~LIQUID.chomp + {% tablerow i in empty_array cols: 2 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + + OUTPUT + + result = Template.parse(template, environment: rigid_environment).render('empty_array' => []) + assert_equal(expected, result) + end + + def test_tablerow_lax_mode_still_accepts_invalid_attributes + template = <<~LIQUID.chomp + {% tablerow i in (1..3) invalid_attr: 5 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + result = Template.parse(template, environment: lax_environment).render + assert_equal(expected, result) + end + + def test_tablerow_strict_mode_still_accepts_invalid_attributes + template = <<~LIQUID.chomp + {% tablerow i in (1..3) invalid_attr: 5 %}{{ i }}{% endtablerow %} + LIQUID + + expected = <<~OUTPUT + + 123 + OUTPUT + + result = Template.parse(template, environment: strict_environment).render + assert_equal(expected, result) + end + + private + + def rigid_environment + Environment.build(error_mode: :rigid) + end + + def strict_environment + Environment.build(error_mode: :strict) + end + + def lax_environment + Environment.build(error_mode: :lax) + end end diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb index cb0de7c44..60ac0423f 100644 --- a/test/unit/rigid_mode_unit_test.rb +++ b/test/unit/rigid_mode_unit_test.rb @@ -6,10 +6,10 @@ class RigidModeUnitTest < Minitest::Test include Liquid def test_tablerow_limit_with_invalid_expression - skip - template = <<~LIQUID - {% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %} + {% tablerow i in (1..10) limit: foo=>bar %} + {{ i }} + {% endtablerow %} LIQUID refute_nil(lax_parse(template)) @@ -22,10 +22,10 @@ def test_tablerow_limit_with_invalid_expression end def test_tablerow_offset_with_invalid_expression - skip - template = <<~LIQUID - {% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %} + {% tablerow i in (1..10) offset: foo=>bar %} + {{ i }} + {% endtablerow %} LIQUID refute_nil(lax_parse(template)) @@ -37,6 +37,24 @@ def test_tablerow_offset_with_invalid_expression assert_match(/Unexpected character =/, error.message) end + def test_tablerow_with_invalid_attribute + template = <<~LIQUID + {% tablerow i in (1..10) invalid_attr: 5 %} + {{ i }} + {% endtablerow %} + LIQUID + + refute_nil(lax_parse(template)) + refute_nil(strict_parse(template)) + + error = assert_raises(SyntaxError) do + rigid_parse(template) + end + + assert_match(/Invalid attribute 'invalid_attr'/, error.message) + assert_match(/Valid attributes are cols, limit, offset, and range/, error.message) + end + def test_cycle_name_with_invalid_expression template = <<~LIQUID {% for i in (1..3) %} From 1c3c979a12214e8256e451d30d14b980719c72d7 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 14 Oct 2025 14:57:07 -0400 Subject: [PATCH 21/41] render end of string is not optional --- lib/liquid/tags/include.rb | 2 ++ lib/liquid/tags/render.rb | 2 ++ test/integration/tags/include_tag_test.rb | 4 ++++ test/integration/tags/render_tag_test.rb | 4 ++++ 4 files changed, 12 insertions(+) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index b72a235b3..e9049e65f 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -111,6 +111,8 @@ def rigid_parse(markup) @attributes[key] = safe_parse_expression(p) p.consume?(:comma) # optional comma end + + p.consume(:end_of_string) end def strict_parse(markup) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 4f716b246..4df5b954c 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -113,6 +113,8 @@ def rigid_parse(markup) @attributes[key] = safe_parse_expression(p) p.consume?(:comma) # optional comma end + + p.consume(:end_of_string) end def rigid_template_name(p) diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index e8cd68576..9f92f1b2c 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -220,6 +220,10 @@ def test_rigid_parsing_errors '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', error_mode: mode, ) + assert_syntax_error( + '{% include "snippet" | filter %}', + error_mode: mode, + ) end end diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index b6cd5cfb6..15a0adc22 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -121,6 +121,10 @@ def test_rigid_parsing_errors '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', error_mode: mode, ) + assert_syntax_error( + '{% render "snippet" | filter %}', + error_mode: mode, + ) end end From bc3253136f9bc7ba353f0b5a1d6af930fb962b22 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 16 Oct 2025 11:13:20 -0400 Subject: [PATCH 22/41] Fix alias parsing --- lib/liquid/tags/render.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 4df5b954c..efe83b0d3 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -96,7 +96,7 @@ def rigid_parse(markup) end alias_name = nil - if p.consume?(:as) + if p.id?("as") alias_name = p.consume(:id) end From 26f092bf0b9115f5c54f921261cb05a4c5901ca0 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 16 Oct 2025 14:14:53 -0400 Subject: [PATCH 23/41] Fixup include parsing of with expression --- lib/liquid/tags/include.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index e9049e65f..8dc9567a5 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -91,11 +91,11 @@ def rigid_parse(markup) with_or_for = p.id?("for") || p.id?("with") || nil @variable_name_expr = nil if with_or_for - @variable_name_expr = parse_expression(p.consume(:id), safe: true) + @variable_name_expr = safe_parse_expression(p) end alias_name = nil - if p.consume?(:as) + if p.id?("as") alias_name = p.consume(:id) end From fd186dc0ac3fea2ff410fc1be20c238a0c831539 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Mon, 20 Oct 2025 13:54:36 -0400 Subject: [PATCH 24/41] Fixup cycle rigid parsing to be backwards compatible --- lib/liquid/tags/cycle.rb | 42 ++++++++----- test/integration/tags/cycle_tag_test.rb | 78 ++++++++++++++++++------- 2 files changed, 87 insertions(+), 33 deletions(-) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 5a2048fca..60f44d04d 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -58,19 +58,27 @@ def render_to_output_buffer(context, output) def rigid_parse(markup) p = @parse_context.new_parser(markup) - if p.look(:id) && p.look(:colon, 1) - @name = p.consume(:id) - @is_named = true - p.consume(:colon) - end - @variables = [] raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string) - while (var = safe_parse_expression(p)) - @variables << var - break unless p.consume?(:comma) + first_expression = safe_parse_expression(p) + if p.look(:colon) + # cycle name: expr1, expr2, ... + @name = first_expression + @is_named = true + p.consume(:colon) + # After the colon, parse the first variable (required for named cycles) + @variables << maybe_dup_lookup(safe_parse_expression(p)) + else + # cycle expr1, expr2, ... + @variables << maybe_dup_lookup(first_expression) + end + + # Parse remaining comma-separated expressions + while p.consume?(:comma) + break if p.look(:end_of_string) + @variables << maybe_dup_lookup(safe_parse_expression(p)) end p.consume(:end_of_string) @@ -106,14 +114,22 @@ def variables_from_string(markup) var =~ /\s*(#{QuotedFragment})\s*/o next unless Regexp.last_match(1) - # Expression Parser returns cached objects, and we need to dup them to - # start the cycle over for each new cycle call. - # Liquid-C does not have a cache, so we don't need to dup the object. var = parse_expression(Regexp.last_match(1)) - var.is_a?(VariableLookup) ? var.dup : var + maybe_dup_lookup(var) end.compact end + # For backwards compatibility, whenever a lookup is used in an unnamed cycle, + # we make it so that the @variables.to_s produces different strings for cycles + # called with the same arguments (since @variables.to_s is used as the cycle counter key) + # This makes it so {% cycle a, b %} and {% cycle a, b %} have independent counters even if a and b share value. + # This is not true for literal values, {% cycle "a", "b" %} and {% cycle "a", "b" %} share the same counter. + # I was really scratching my head about this one, but migrating away from this would be more headache + # than it's worth. So we're keeping this quirk for now. + def maybe_dup_lookup(var) + var.is_a?(VariableLookup) ? var.dup : var + end + class ParseTreeVisitor < Liquid::ParseTreeVisitor def children Array(@node.variables) diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index dde8a1bc1..b0ee69255 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -3,20 +3,11 @@ require 'test_helper' class CycleTagTest < Minitest::Test - def test_simple_cycle - template = <<~LIQUID - {%- cycle '1', '2', '3' -%} - {%- cycle '1', '2', '3' -%} - {%- cycle '1', '2', '3' -%} - LIQUID - - assert_template_result("123", template) - end def test_simple_cycle_inside_for_loop template = <<~LIQUID {%- for i in (1..3) -%} - {% cycle '1', '2', '3' %} + {%- cycle '1', '2', '3' -%} {%- endfor -%} LIQUID @@ -36,14 +27,61 @@ def test_cycle_with_variables_inside_for_loop assert_template_result("123", template) end - def test_cycle_tag_always_resets_cycle + def test_cycle_named_groups_string + template = <<~LIQUID + {%- for i in (1..3) -%} + {%- cycle 'placeholder1': 1, 2, 3 -%} + {%- cycle 'placeholder2': 1, 2, 3 -%} + {%- endfor -%} + LIQUID + + assert_template_result("112233", template) + end + + def test_cycle_named_groups_vlookup + template = <<~LIQUID + {%- assign placeholder1 = 'placeholder1' -%} + {%- assign placeholder2 = 'placeholder2' -%} + {%- for i in (1..3) -%} + {%- cycle placeholder1: 1, 2, 3 -%} + {%- cycle placeholder2: 1, 2, 3 -%} + {%- endfor -%} + LIQUID + + assert_template_result("112233", template) + end + + def test_unnamed_cycle_have_independent_counters_when_used_with_lookups template = <<~LIQUID {%- assign a = "1" -%} - {%- cycle a, "2" -%} - {%- cycle a, "2" -%} + {%- for i in (1..3) -%} + {%- cycle a, "2" -%} + {%- cycle a, "2" -%} + {%- endfor -%} + LIQUID + + assert_template_result("112211", template) + end + + def test_unnamed_cycle_dependent_counter_when_used_with_literal_values + template = <<~LIQUID + {%- cycle "1", "2" -%} + {%- cycle "1", "2" -%} + {%- cycle "1", "2" -%} LIQUID - assert_template_result("11", template) + assert_template_result("121", template) + end + + def test_optional_trailing_comma + template = <<~LIQUID + {%- cycle "1", "2", -%} + {%- cycle "1", "2", -%} + {%- cycle "1", "2", -%} + {%- cycle "1", -%} + LIQUID + + assert_template_result("1211", template) end def test_cycle_tag_without_arguments @@ -56,19 +94,19 @@ def test_cycle_tag_without_arguments def test_cycle_tag_with_error_mode # QuotedFragment is more permissive than what Parser#expression allows. - temlate1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" - temlate2 = "{% cycle .5: 'a', 'b' %}" + template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" + template2 = "{% cycle .5: 'a', 'b' %}" [:lax, :strict].each do |mode| with_error_mode(mode) do - assert_template_result("b", temlate1) - assert_template_result("a", temlate2) + assert_template_result("b", template1) + assert_template_result("a", template2) end end with_error_mode(:rigid) do - error1 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate1) } - error2 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate2) } + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/ From 937b7338ddde10a8bbd58bd271fcc83de919377e Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Mon, 20 Oct 2025 15:00:11 -0400 Subject: [PATCH 25/41] Fix assert_template_result tests not picking up Liquid::Environment.default.error_mode The `rake test` command gave us the impression that we were running all the tests on all the error modes, that was false. --- lib/liquid/tags/cycle.rb | 1 + test/integration/expression_test.rb | 9 +++++++-- test/integration/tags/cycle_tag_test.rb | 1 - test/integration/tags/table_row_test.rb | 9 ++++++--- test/test_helper.rb | 2 +- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 60f44d04d..51fa9714a 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -78,6 +78,7 @@ def rigid_parse(markup) # Parse remaining comma-separated expressions while p.consume?(:comma) break if p.look(:end_of_string) + @variables << maybe_dup_lookup(safe_parse_expression(p)) end diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 918f87c28..719699001 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -26,8 +26,12 @@ def test_int def test_float assert_template_result("-17.42", "{{ -17.42 }}") assert_template_result("2.5", "{{ 2.5 }}") - assert_expression_result(0.0, "0.....5") - assert_expression_result(0.0, "-0..1") + + with_error_mode(:lax) do + assert_expression_result(0.0, "0.....5") + assert_expression_result(0.0, "-0..1") + end + assert_expression_result(1.5, "1.5") # this is a unfortunate quirky behavior of Liquid @@ -61,6 +65,7 @@ def test_quirky_negative_sign_expression_markup assert_template_result( "", "{{ - 'theme.css' - }}", + error_mode: :lax, ) end diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index b0ee69255..c41fb68e1 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -3,7 +3,6 @@ require 'test_helper' class CycleTagTest < Minitest::Test - def test_simple_cycle_inside_for_loop template = <<~LIQUID {%- for i in (1..3) -%} diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index ad12d76f1..81444945b 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -138,7 +138,7 @@ def test_nil_offset_is_treated_as_zero def test_tablerow_loop_drop_attributes template = <<~LIQUID.chomp - {% tablerow i in (1...2) %} + {% tablerow i in (1..2) %} col: {{ tablerowloop.col }} col0: {{ tablerowloop.col0 }} col_first: {{ tablerowloop.col_first }} @@ -192,12 +192,14 @@ def test_table_row_renders_correct_error_message_for_invalid_parameters assert_template_result( "Liquid error (line 1): invalid integer", '{% tablerow n in (1...10) limit:true %} {{n}} {% endtablerow %}', + error_mode: :warn, render_errors: true, ) assert_template_result( "Liquid error (line 1): invalid integer", '{% tablerow n in (1...10) offset:true %} {{n}} {% endtablerow %}', + error_mode: :warn, render_errors: true, ) @@ -205,18 +207,19 @@ def test_table_row_renders_correct_error_message_for_invalid_parameters "Liquid error (line 1): invalid integer", '{% tablerow n in (1...10) cols:true %} {{n}} {% endtablerow %}', render_errors: true, + error_mode: :warn, ) end def test_table_row_handles_interrupts assert_template_result( "\n 1 \n", - '{% tablerow n in (1...3) cols:2 %} {{n}} {% break %} {{n}} {% endtablerow %}', + '{% tablerow n in (1..3) cols:2 %} {{n}} {% break %} {{n}} {% endtablerow %}', ) assert_template_result( "\n 1 2 \n 3 \n", - '{% tablerow n in (1...3) cols:2 %} {{n}} {% continue %} {{n}} {% endtablerow %}', + '{% tablerow n in (1..3) cols:2 %} {{n}} {% continue %} {{n}} {% endtablerow %}', ) end diff --git a/test/test_helper.rb b/test/test_helper.rb index 4f4447384..69172d470 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -34,7 +34,7 @@ module Assertions def assert_template_result( expected, template, assigns = {}, - message: nil, partials: nil, error_mode: nil, render_errors: false, + message: nil, partials: nil, error_mode: Liquid::Environment.default.error_mode, render_errors: false, template_factory: nil ) file_system = StubFileSystem.new(partials || {}) From 82428defce6eb48f11d011f27720ff5a88ff8292 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 11:45:49 +0200 Subject: [PATCH 26/41] * Update `bin/render` script to present an error when no template is passed * Remove `bin/example.liquid` as it's not executable --- README.md | 11 +++++------ bin/example.liquid | 5 ----- bin/render | 36 ++++++++++++++++++++++++++---------- 3 files changed, 31 insertions(+), 21 deletions(-) delete mode 100644 bin/example.liquid diff --git a/README.md b/README.md index 93a726a91..da24e54fa 100644 --- a/README.md +++ b/README.md @@ -99,15 +99,14 @@ Setting the error mode of Liquid lets you specify how strictly you want your tem Normally the parser is very lax and will accept almost anything without error. Unfortunately this can make it very hard to debug and can lead to unexpected behaviour. -Liquid also comes with a stricter parser that can be used when editing templates to give better error messages +Liquid also comes with a parser that can be used when editing templates to give better error messages when templates are invalid. You can enable this new parser like this: ```ruby -Liquid::Environment.default.error_mode = :strict -Liquid::Environment.default.error_mode = :strict # Raises a SyntaxError when invalid syntax is used -Liquid::Environment.default.error_mode = :warn # Adds strict errors to template.errors but continues as normal -Liquid::Environment.default.error_mode = :lax # The default mode, accepts almost anything. -Liquid::Environment.default.error_mode = :rigid # Uses Parser.new instead of Expression.parse for stricter parsing +Liquid::Environment.default.error_mode = :rigid # Raises a SyntaxError when invalid syntax is used in all tags +Liquid::Environment.default.error_mode = :strict # Raises a SyntaxError when invalid syntax is used in some tags +Liquid::Environment.default.error_mode = :warn # Adds strict errors to template.errors but continues as normal +Liquid::Environment.default.error_mode = :lax # The default mode, accepts almost anything. ``` If you want to set the error mode only on specific templates you can pass `:error_mode` as an option to `parse`: diff --git a/bin/example.liquid b/bin/example.liquid deleted file mode 100644 index c4a93aa3d..000000000 --- a/bin/example.liquid +++ /dev/null @@ -1,5 +0,0 @@ - - {% tablerow i in (1..10) limit: foo=>bar %} - {{ i }} - {% endtablerow %} -
diff --git a/bin/render b/bin/render index 004c103f0..26d9b53d8 100755 --- a/bin/render +++ b/bin/render @@ -6,12 +6,20 @@ require 'liquid' class VirtualFileSystem def initialize - snippet_1 = '

{{ greating | default: "Hello" }}, {{ name | default: "world" }}!

' - snippet_2 = '{% for i in (1..5) %} > {{ i }}{% endfor %}' + snippet_1 = <<~LIQUID +

+ {{- greating | default: 'Hello' }}, {{ name | default: 'world' -}}! +

+ LIQUID + snippet_2 = <<~LIQUID + {%- for i in (1..5) -%} + > {{ i }} + {%- endfor -%} + LIQUID @templates = { - 'snippet_1' => snippet_1, - 'snippet_2' => snippet_2, + 'snippet-1' => snippet_1, + 'snippet-2' => snippet_2, } end @@ -20,11 +28,19 @@ class VirtualFileSystem end end -error_mode = :strict -# error_mode = :rigid -file = File.read(ARGV[0]) -template = Liquid::Template.parse(file, error_mode: error_mode) +def source + File.read(ARGV[0]) +rescue StandardError + 'Usage: bin/render example/server/templates/index.liquid' +end -template.registers[:file_system] = VirtualFileSystem.new +def assigns + { + 'date' => Time.now, + } +end -puts template.render +puts Liquid::Template + .parse(source, error_mode: :rigid) + .tap { |t| t.registers[:file_system] = VirtualFileSystem.new } + .render(assigns) From e4bf43ec8ce89f451c6e19dee84f0ed17d5b67de Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 11:48:12 +0200 Subject: [PATCH 27/41] 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 1b192d8cb..476123393 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -55,10 +55,15 @@ def safe_parse_expression(parser) 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 c6b09bc86..7c662e928 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -6,16 +6,14 @@ module ParserSwitching # # 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 719699001..fe54bd99b 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -71,7 +71,6 @@ def test_quirky_negative_sign_expression_markup 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 @@ def test_expression_cache 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 @@ def test_expression_cache_with_true_boolean 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 @@ def test_expression_cache_with_lru_redux 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 @@ def test_disable_expression_cache 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 69172d470..a09f22771 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -82,10 +82,12 @@ def with_global_filter(*globals, &blk) 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 000000000..b1c8fe470 --- /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 From f0ed8e5ed597500d683b6fa5a3b2116104a49d12 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 12:05:19 +0200 Subject: [PATCH 28/41] Fix variable to keep it backward-compatible in strict mode * lax_parse - no changes * strict_parse - uses the `lax_parse_filter_expressions` (as it was doing before) * rigid_parse - uses the `rigid_parse_filter_expressions` --- lib/liquid/variable.rb | 17 ++++++-- test/integration/variable_test.rb | 65 +++++++++++++++++++++++++++++++ test/unit/variable_unit_test.rb | 50 ++++++++++++++---------- 3 files changed, 109 insertions(+), 23 deletions(-) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index f355d694b..3fd947a7d 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -66,12 +66,23 @@ def strict_parse(markup) return if p.look(:end_of_string) @name = parse_context.safe_parse_expression(p) - @filters << strict_parse_filter_expressions(p) while p.consume?(:pipe) + while p.consume?(:pipe) + filtername = p.consume(:id) + filterargs = p.consume?(:colon) ? parse_filterargs(p) : Const::EMPTY_ARRAY + @filters << lax_parse_filter_expressions(filtername, filterargs) + end p.consume(:end_of_string) end def rigid_parse(markup) - strict_parse(markup) + @filters = [] + p = @parse_context.new_parser(markup) + + return if p.look(:end_of_string) + + @name = parse_context.safe_parse_expression(p) + @filters << rigid_parse_filter_expressions(p) while p.consume?(:pipe) + p.consume(:end_of_string) end def parse_filterargs(p) @@ -145,7 +156,7 @@ def lax_parse_filter_expressions(filter_name, unparsed_args) # argument = (positional_argument | keyword_argument) # positional_argument = expression # keyword_argument = id ":" expression - def strict_parse_filter_expressions(p) + def rigid_parse_filter_expressions(p) filtername = p.consume(:id) filter_args = [] keyword_args = {} diff --git a/test/integration/variable_test.rb b/test/integration/variable_test.rb index 19922c191..740ea9f9a 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -209,4 +209,69 @@ def test_variable_lookup_should_not_hang_with_invalid_syntax end end end + + def test_filter_with_single_trailing_comma + template = '{{ "hello" | append: "world", }}' + + with_error_mode(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_mode(:rigid) do + assert_template_result('helloworld', template) + end + end + + def test_multiple_filters_with_trailing_commas + template = '{{ "hello" | append: "1", | append: "2", }}' + + with_error_mode(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_mode(:rigid) do + assert_template_result('hello12', template) + end + end + + def test_filter_with_colon_but_no_arguments + template = '{{ "test" | upcase: }}' + + with_error_mode(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_mode(:rigid) do + assert_template_result('TEST', template) + end + end + + def test_filter_chain_with_colon_no_args + template = '{{ "test" | append: "x" | upcase: }}' + + with_error_mode(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_mode(:rigid) do + assert_template_result('TESTX', template) + end + end + + def test_combining_trailing_comma_and_empty_args + template = '{{ "test" | append: "x", | upcase: }}' + + with_error_mode(:strict) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/is not a valid expression/, error.message) + end + + with_error_mode(:rigid) do + assert_template_result('TESTX', template) + end + end end diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index c8e572c97..82ac03302 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -154,33 +154,43 @@ def test_lax_filter_argument_parsing end def test_strict_filter_argument_parsing - # optional colon - var = create_variable(%(n | f1 | f2:), error_mode: :strict) - assert_equal([['f1', []], ['f2', []]], var.filters) + with_error_mode(:strict) do + assert_raises(SyntaxError) do + create_variable(%( number_of_comments | pluralize: 'comment': 'comments' )) + end + end + end - # 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) } + def test_rigid_filter_argument_parsing + with_error_mode(:rigid) do + # optional colon + var = create_variable(%(n | f1 | f2:)) + assert_equal([['f1', []], ['f2', []]], var.filters) - # arg requires colon - assert_raises(SyntaxError) { create_variable(%(n | f1 1), error_mode: :strict) } + # missing argument throws error + assert_raises(SyntaxError) { create_variable(%(n | f1: ,)) } + assert_raises(SyntaxError) { create_variable(%(n | f1: ,| f2)) } - # trailing comma doesn't throw - create_variable(%(n | f1: 1, 2, 3, | f2:), error_mode: :strict) + # arg requires colon + assert_raises(SyntaxError) { create_variable(%(n | f1 1)) } - # missing comma throws error - assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3), error_mode: :strict) } + # trailing comma doesn't throw + create_variable(%(n | f1: 1, 2, 3, | f2:)) - # 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) + # missing comma throws error + assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3)) } - # 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) + # positional and kwargs parsing + var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2)) + assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) - # string key throws - assert_raises(SyntaxError) { create_variable(%(n | pluralize: 'comment': 'comments'), error_mode: :strict) } + # positional and kwargs intermixed (pos1, key1: val1, pos2) + var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title")) + 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')) } + end end def test_output_raw_source_of_variable From 0946c4bf703df2511e589a8849f32392337c3c9f Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 12:09:32 +0200 Subject: [PATCH 29/41] Covered changes with more tests, remove redundant cases, and the new `with_error_mode(*modes)` Most of changes update this: ``` [:lax, :strict].each do |mode| with_error_mode(mode) do assert_template_result(... ``` to be this: ``` with_error_mode(:lax, :strict) do assert_template_result(... ``` --- lib/liquid/tags/case.rb | 1 - lib/liquid/tags/cycle.rb | 1 - lib/liquid/template.rb | 4 +- test/integration/tags/cycle_tag_test.rb | 56 +++++-- test/integration/tags/include_tag_test.rb | 57 +++++-- test/integration/tags/render_tag_test.rb | 44 +++-- test/integration/tags/table_row_test.rb | 171 +++++++------------ test/unit/condition_unit_test.rb | 31 ++++ test/unit/rigid_mode_unit_test.rb | 191 ---------------------- test/unit/tags/case_tag_unit_test.rb | 28 ++-- 10 files changed, 228 insertions(+), 356 deletions(-) delete mode 100644 test/unit/rigid_mode_unit_test.rb diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index bdf329a2b..926e4107a 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -123,7 +123,6 @@ def parse_rigid_when(markup, body) block.attach(body) @blocks << block - # Temporarily until support :or lexeme. break unless parser.id?('or') || parser.consume?(:comma) end diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 51fa9714a..639ed9a5b 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -90,7 +90,6 @@ def rigid_parse(markup) end end - # Temporarily until we migrate def strict_parse(markup) lax_parse(markup) end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index acda1e4db..5d349abd6 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -24,8 +24,8 @@ class << self # Sets how strict the parser should be. # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases. # :warn is the default and will give deprecation warnings when invalid syntax is used. - # :strict will enforce correct syntax. - # :rigid is stricter even. + # :strict enforces correct syntax for most tags + # :rigid enforces correct syntax for all tags def error_mode=(mode) Deprecations.warn("Template.error_mode=", "Environment#error_mode=") Environment.default.error_mode = mode diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index c41fb68e1..14d8997b4 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -96,11 +96,9 @@ def test_cycle_tag_with_error_mode template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" template2 = "{% cycle .5: 'a', 'b' %}" - [:lax, :strict].each do |mode| - with_error_mode(mode) do - assert_template_result("b", template1) - assert_template_result("a", template2) - end + with_error_mode(:lax, :strict) do + assert_template_result("b", template1) + assert_template_result("a", template2) end with_error_mode(:rigid) do @@ -123,14 +121,12 @@ def test_cycle_with_trailing_elements template4 = "#{assignments}{% cycle n e: 'a', 'b', 'c' %}" template5 = "#{assignments}{% cycle n e 'a', 'b', 'c' %}" - [:lax, :strict].each do |mode| - with_error_mode(mode) do - assert_template_result("a", template1) - assert_template_result("a", template2) - assert_template_result("a", template3) - assert_template_result("N", template4) - assert_template_result("N", template5) - end + with_error_mode(:lax, :strict) do + assert_template_result("a", template1) + assert_template_result("a", template2) + assert_template_result("a", template3) + assert_template_result("N", template4) + assert_template_result("N", template5) end with_error_mode(:rigid) do @@ -149,4 +145,38 @@ def test_cycle_with_trailing_elements assert_match(expected_error, error5.message) end end + + def test_cycle_name_with_invalid_expression + template = <<~LIQUID + {% for i in (1..3) %} + {% cycle foo=>bar: "a", "b" %} + {% endfor %} + LIQUID + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_cycle_variable_with_invalid_expression + template = <<~LIQUID + {% for i in (1..3) %} + {% cycle foo=>bar, "a", "b" %} + {% endfor %} + LIQUID + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end end diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index 9f92f1b2c..b41a98c6e 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -205,24 +205,20 @@ def test_dynamically_choosen_template end def test_rigid_parsing_errors - [:lax, :strict].each do |mode| - with_error_mode(mode) do - assert_template_result( - 'hello value1 value2', - '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, - ) - end + with_error_mode(:lax, :strict) do + assert_template_result( + 'hello value1 value2', + '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, + ) end - [:rigid].each do |mode| + with_error_mode(:rigid) do assert_syntax_error( '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - error_mode: mode, ) assert_syntax_error( '{% include "snippet" | filter %}', - error_mode: mode, ) end end @@ -404,4 +400,43 @@ def test_render_tag_renders_error_with_template_name_from_template_factory render_errors: true, ) end + + def test_include_template_with_invalid_expression + template = "{% include foo=>bar %}" + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_include_with_invalid_expression + template = '{% include "snippet" with foo=>bar %}' + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_include_attribute_with_invalid_expression + template = '{% include "snippet", key: foo=>bar %}' + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end end # IncludeTagTest diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index 15a0adc22..e2f4530a5 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -106,24 +106,20 @@ def test_dynamically_choosen_templates_are_not_allowed end def test_rigid_parsing_errors - [:lax, :strict].each do |mode| - with_error_mode(mode) do - assert_template_result( - 'hello value1 value2', - '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, - ) - end + with_error_mode(:lax, :strict) do + assert_template_result( + 'hello value1 value2', + '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, + ) end - [:rigid].each do |mode| + with_error_mode(:rigid) do assert_syntax_error( '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - error_mode: mode, ) assert_syntax_error( '{% render "snippet" | filter %}', - error_mode: mode, ) end end @@ -318,4 +314,30 @@ def test_render_tag_renders_error_with_template_name_from_template_factory render_errors: true, ) end + + def test_render_with_invalid_expression + template = '{% render "snippet" with foo=>bar %}' + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end + + def test_render_attribute_with_invalid_expression + template = '{% render "snippet", key: foo=>bar %}' + + with_error_mode(:lax, :strict) do + refute_nil(Template.parse(template)) + end + + with_error_mode(:rigid) do + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end + end end diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index 81444945b..0f9b051fe 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -270,8 +270,9 @@ def test_tablerow_with_cols_attribute_in_rigid_mode 456 OUTPUT - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template) + end end def test_tablerow_with_limit_attribute_in_rigid_mode @@ -284,8 +285,9 @@ def test_tablerow_with_limit_attribute_in_rigid_mode 123 OUTPUT - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template) + end end def test_tablerow_with_offset_attribute_in_rigid_mode @@ -298,8 +300,9 @@ def test_tablerow_with_offset_attribute_in_rigid_mode 345 OUTPUT - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template) + end end def test_tablerow_with_range_attribute_in_rigid_mode @@ -312,8 +315,9 @@ def test_tablerow_with_range_attribute_in_rigid_mode 123 OUTPUT - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template) + end end def test_tablerow_with_multiple_attributes_in_rigid_mode @@ -327,8 +331,9 @@ def test_tablerow_with_multiple_attributes_in_rigid_mode 45 OUTPUT - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template) + end end def test_tablerow_with_variable_collection_in_rigid_mode @@ -342,8 +347,9 @@ def test_tablerow_with_variable_collection_in_rigid_mode 34 OUTPUT - result = Template.parse(template, environment: rigid_environment).render('numbers' => [1, 2, 3, 4]) - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template, { 'numbers' => [1, 2, 3, 4] }) + end end def test_tablerow_with_dotted_access_in_rigid_mode @@ -357,8 +363,9 @@ def test_tablerow_with_dotted_access_in_rigid_mode 34 OUTPUT - result = Template.parse(template, environment: rigid_environment).render('obj' => { 'numbers' => [1, 2, 3, 4] }) - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template, { 'obj' => { 'numbers' => [1, 2, 3, 4] } }) + end end def test_tablerow_with_bracketed_access_in_rigid_mode @@ -371,8 +378,9 @@ def test_tablerow_with_bracketed_access_in_rigid_mode 1020 OUTPUT - result = Template.parse(template, environment: rigid_environment).render('obj' => { 'numbers' => [10, 20] }) - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template, { 'obj' => { 'numbers' => [10, 20] } }) + end end def test_tablerow_without_attributes_in_rigid_mode @@ -385,79 +393,27 @@ def test_tablerow_without_attributes_in_rigid_mode 123 OUTPUT - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) - end - - def test_tablerow_with_trailing_comma_in_rigid_mode - template = <<~LIQUID.chomp - {% tablerow i in (1..3) cols: 2, %}{{ i }}{% endtablerow %} - LIQUID - - expected = <<~OUTPUT - - 12 - 3 - OUTPUT - - result = Template.parse(template, environment: rigid_environment).render - assert_equal(expected, result) - end - - def test_tablerow_with_invalid_attribute_name_in_rigid_mode - template = '{% tablerow i in (1..10) invalid_attr: 5 %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) - end - assert_equal("Liquid syntax error: Invalid attribute 'invalid_attr' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid_attr: 5\"", error.message) - end - - def test_tablerow_with_invalid_expression_in_limit_in_rigid_mode - template = '{% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) - end - assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) limit: foo=>bar\"", error.message) - end - - def test_tablerow_with_invalid_expression_in_offset_in_rigid_mode - template = '{% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) - end - assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) offset: foo=>bar\"", error.message) - end - - def test_tablerow_with_invalid_expression_in_cols_in_rigid_mode - template = '{% tablerow i in (1..10) cols: foo=>bar %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) + with_error_mode(:rigid) do + assert_template_result(expected, template) end - assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) cols: foo=>bar\"", error.message) - end - - def test_tablerow_with_invalid_expression_in_range_in_rigid_mode - template = '{% tablerow i in (1..10) range: foo=>bar %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) - end - assert_equal("Liquid syntax error: Unexpected character = in \"i in (1..10) range: foo=>bar\"", error.message) end def test_tablerow_without_in_keyword_in_rigid_mode template = '{% tablerow i (1..10) %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) + + with_error_mode(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) end - assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) end def test_tablerow_with_multiple_invalid_attributes_reports_first_in_rigid_mode template = '{% tablerow i in (1..10) invalid1: 5, invalid2: 10 %}{{ i }}{% endtablerow %}' - error = assert_raises(SyntaxError) do - Template.parse(template, environment: rigid_environment) + + with_error_mode(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) end - assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) end def test_tablerow_with_empty_collection_in_rigid_mode @@ -470,49 +426,44 @@ def test_tablerow_with_empty_collection_in_rigid_mode OUTPUT - result = Template.parse(template, environment: rigid_environment).render('empty_array' => []) - assert_equal(expected, result) + with_error_mode(:rigid) do + assert_template_result(expected, template, { 'empty_array' => [] }) + end end - def test_tablerow_lax_mode_still_accepts_invalid_attributes - template = <<~LIQUID.chomp - {% tablerow i in (1..3) invalid_attr: 5 %}{{ i }}{% endtablerow %} - LIQUID + def test_tablerow_with_invalid_attribute_strict_vs_rigid + template = '{% tablerow i in (1..5) invalid_attr: 10 %}{{ i }}{% endtablerow %}' expected = <<~OUTPUT - 123 + 12345 OUTPUT - result = Template.parse(template, environment: lax_environment).render - assert_equal(expected, result) - end - - def test_tablerow_strict_mode_still_accepts_invalid_attributes - template = <<~LIQUID.chomp - {% tablerow i in (1..3) invalid_attr: 5 %}{{ i }}{% endtablerow %} - LIQUID - - expected = <<~OUTPUT - - 123 - OUTPUT + with_error_mode(:lax, :strict) do + assert_template_result(expected, template) + end - result = Template.parse(template, environment: strict_environment).render - assert_equal(expected, result) + with_error_mode(:rigid) do + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_match(/Invalid attribute 'invalid_attr'/, error.message) + end end - private - - def rigid_environment - Environment.build(error_mode: :rigid) - end + def test_tablerow_with_invalid_expression_strict_vs_rigid + template = '{% tablerow i in (1..5) limit: foo=>bar %}{{ i }}{% endtablerow %}' - def strict_environment - Environment.build(error_mode: :strict) - end + with_error_mode(:lax, :strict) do + expected = <<~OUTPUT + + + OUTPUT + assert_template_result(expected, template) + end - def lax_environment - Environment.build(error_mode: :lax) + with_error_mode(:rigid) do + # Rigid mode validates expression syntax and rejects invalid expressions + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) + end end end diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index acc71dc4e..f5206ff53 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -166,6 +166,37 @@ def test_default_context_is_deprecated assert_includes(err.lines.map(&:strip), expected) end + def test_parse_expression_in_strict_mode + environment = Environment.build(error_mode: :strict) + parse_context = ParseContext.new(environment: environment) + result = Condition.parse_expression(parse_context, 'product.title') + + assert_instance_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + + def test_parse_expression_in_rigid_mode_raises_internal_error + environment = Environment.build(error_mode: :rigid) + parse_context = ParseContext.new(environment: environment) + + error = assert_raises(Liquid::InternalError) do + Condition.parse_expression(parse_context, 'product.title') + end + + assert_match(/unsafe parse_expression cannot be used in rigid mode/, error.message) + end + + def test_parse_expression_with_safe_true_in_rigid_mode + environment = Environment.build(error_mode: :rigid) + parse_context = ParseContext.new(environment: environment) + result = Condition.parse_expression(parse_context, 'product.title', safe: true) + + assert_instance_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) + end + private def assert_evaluates_true(left, op, right) diff --git a/test/unit/rigid_mode_unit_test.rb b/test/unit/rigid_mode_unit_test.rb deleted file mode 100644 index 60ac0423f..000000000 --- a/test/unit/rigid_mode_unit_test.rb +++ /dev/null @@ -1,191 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -class RigidModeUnitTest < Minitest::Test - include Liquid - - def test_tablerow_limit_with_invalid_expression - template = <<~LIQUID - {% tablerow i in (1..10) limit: foo=>bar %} - {{ i }} - {% endtablerow %} - LIQUID - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) do - rigid_parse(template) - end - assert_match(/Unexpected character =/, error.message) - end - - def test_tablerow_offset_with_invalid_expression - template = <<~LIQUID - {% tablerow i in (1..10) offset: foo=>bar %} - {{ i }} - {% endtablerow %} - LIQUID - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) do - rigid_parse(template) - end - assert_match(/Unexpected character =/, error.message) - end - - def test_tablerow_with_invalid_attribute - template = <<~LIQUID - {% tablerow i in (1..10) invalid_attr: 5 %} - {{ i }} - {% endtablerow %} - LIQUID - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) do - rigid_parse(template) - end - - assert_match(/Invalid attribute 'invalid_attr'/, error.message) - assert_match(/Valid attributes are cols, limit, offset, and range/, error.message) - end - - def test_cycle_name_with_invalid_expression - template = <<~LIQUID - {% for i in (1..3) %} - {% cycle foo=>bar: "a", "b" %} - {% endfor %} - LIQUID - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_cycle_variable_with_invalid_expression - template = <<~LIQUID - {% for i in (1..3) %} - {% cycle foo=>bar, "a", "b" %} - {% endfor %} - LIQUID - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_case_with_invalid_expression - template = <<~LIQUID - {% case foo=>bar %} - {% when 1 %} - one - {% endcase %} - LIQUID - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_include_template_with_invalid_expression - template = "{% include foo=>bar %}" - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_include_with_invalid_expression - template = '{% include "snippet" with foo=>bar %}' - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_include_attribute_with_invalid_expression - template = '{% include "snippet", key: foo=>bar %}' - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_render_with_invalid_expression - template = '{% render "snippet" with foo=>bar %}' - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_render_attribute_with_invalid_expression - template = '{% render "snippet", key: foo=>bar %}' - - refute_nil(lax_parse(template)) - refute_nil(strict_parse(template)) - - error = assert_raises(SyntaxError) { rigid_parse(template) } - - assert_match(/Unexpected character =/, error.message) - end - - def test_empty_expression_handling - ctx_rigid = ParseContext.new(environment: rigid) - - assert_nil(ctx_rigid.parse_expression('', safe: true)) - assert_nil(ctx_rigid.parse_expression(' ', safe: true)) - end - - private - - def rigid_parse(source) - Template.parse(source, environment: rigid) - end - - def strict_parse(source) - Template.parse(source, environment: strict) - end - - def lax_parse(source) - Template.parse(source, environment: lax) - end - - def lax - Environment.build(error_mode: :lax) - end - - def rigid - Environment.build(error_mode: :rigid) - end - - def strict - Environment.build(error_mode: :strict) - end -end diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index 687cf9d91..c0bc2c344 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -20,8 +20,8 @@ def test_case_with_trailing_element {%- endcase -%} LIQUID - [:lax, :strict].each do |mode| - with_error_mode(mode) { assert_template_result("one", template) } + with_error_mode(:lax, :strict) do + assert_template_result("one", template) end with_error_mode(:rigid) do @@ -41,8 +41,8 @@ def test_case_when_trailing_element {%- endcase -%} LIQUID - [:lax, :strict].each do |mode| - with_error_mode(mode) { assert_template_result("one", template) } + with_error_mode(:lax, :strict) do + assert_template_result("one", template) end with_error_mode(:rigid) do @@ -62,8 +62,8 @@ def test_case_when_with_comma {%- endcase -%} LIQUID - [:lax, :strict, :rigid].each do |mode| - with_error_mode(mode) { assert_template_result("one", template) } + with_error_mode(:lax, :strict, :rigid) do + assert_template_result("one", template) end end @@ -77,8 +77,8 @@ def test_case_when_with_or {%- endcase -%} LIQUID - [:lax, :strict, :rigid].each do |mode| - with_error_mode(mode) { assert_template_result("one", template) } + with_error_mode(:lax, :strict, :rigid) do + assert_template_result("one", template) end end @@ -93,10 +93,8 @@ def test_case_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - [:lax, :strict].each do |mode| - with_error_mode(mode) do - assert_template_result("one", template, assigns) - end + with_error_mode(:lax, :strict) do + assert_template_result("one", template, assigns) end with_error_mode(:rigid) do @@ -117,10 +115,8 @@ def test_case_when_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - [:lax, :strict].each do |mode| - with_error_mode(mode) do - assert_template_result("one", template, assigns) - end + with_error_mode(:lax, :strict) do + assert_template_result("one", template, assigns) end with_error_mode(:rigid) do From 44dfa39568420bcb7a6565e5c7903fc6602fc4ae Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 12:32:40 +0200 Subject: [PATCH 30/41] Update `History.md` --- History.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/History.md b/History.md index 3d979dc14..be20e055e 100644 --- a/History.md +++ b/History.md @@ -1,5 +1,8 @@ # Liquid Change Log +## 5.8.8 +* Introduce `:rigid` error mode for stricter, safer parsing of all tags [CP Clermont, Guilherme Carreiro] + ## 5.8.7 * Expose body content in the `Doc` tag [James Meng] From 195fd5a0fc85a1df88340f12d86242c1092cd5e3 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Wed, 22 Oct 2025 16:46:47 +0200 Subject: [PATCH 31/41] Update `History.md` (5.8.8 -> 5.9.0) --- History.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/History.md b/History.md index be20e055e..763b80533 100644 --- a/History.md +++ b/History.md @@ -1,6 +1,6 @@ # Liquid Change Log -## 5.8.8 +## 5.9.0 * Introduce `:rigid` error mode for stricter, safer parsing of all tags [CP Clermont, Guilherme Carreiro] ## 5.8.7 From 54b41dbb9f2bf1e07a0437bc21035777bdba82f6 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 23 Oct 2025 09:52:53 +0200 Subject: [PATCH 32/41] Update Rakefile Co-authored-by: Alok Swamy --- Rakefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rakefile b/Rakefile index ae41567a3..586b61734 100755 --- a/Rakefile +++ b/Rakefile @@ -33,7 +33,7 @@ task :rubocop do end end -desc('runs test suite with all parsers (lax, strict, and rigid)') +desc('runs test suite with lax, strict, and rigid parsers') task :test do ENV['LIQUID_PARSER_MODE'] = 'lax' Rake::Task['base_test'].invoke From 36ec055631991d8f0fc74fda2f97f9b29c799f1e Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 23 Oct 2025 09:57:21 +0200 Subject: [PATCH 33/41] Update test/unit/tags/case_tag_unit_test.rb Co-authored-by: Alok Swamy --- test/unit/tags/case_tag_unit_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index c0bc2c344..e6a5ababf 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -31,7 +31,7 @@ def test_case_with_trailing_element end end - def test_case_when_trailing_element + def test_case_when_with_trailing_element template = <<~LIQUID {%- case 1 -%} {%- when 1 bar -%} From 8c8a843bc81ce7a67f2df03e757de88ddb9724c5 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 23 Oct 2025 09:57:30 +0200 Subject: [PATCH 34/41] Update test/integration/tags/render_tag_test.rb Co-authored-by: Alok Swamy --- test/integration/tags/render_tag_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index e2f4530a5..12759daf5 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -131,7 +131,7 @@ def test_optional_commas assert_template_result('hello value1 value2', '{% render "snippet" arg1: "value1" arg2: "value2" %}', partials: partials) end - def test_include_tag_caches_second_read_of_same_partial + def test_render_tag_caches_second_read_of_same_partial file_system = StubFileSystem.new('snippet' => 'echo') assert_equal( 'echoecho', From ba5fb9934d047f508e2c742c87120f3643dd9431 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Thu, 23 Oct 2025 10:01:01 +0200 Subject: [PATCH 35/41] Rename `with_error_mode(...)` to `with_error_modes(...)` --- test/integration/context_test.rb | 2 +- test/integration/error_handling_test.rb | 2 +- test/integration/expression_test.rb | 2 +- test/integration/parsing_quirks_test.rb | 20 +++++++------- test/integration/tags/cycle_tag_test.rb | 16 ++++++------ test/integration/tags/include_tag_test.rb | 20 +++++++------- test/integration/tags/render_tag_test.rb | 12 ++++----- test/integration/tags/table_row_test.rb | 32 +++++++++++------------ test/integration/variable_test.rb | 20 +++++++------- test/test_helper.rb | 2 +- test/unit/tags/case_tag_unit_test.rb | 20 +++++++------- test/unit/variable_unit_test.rb | 6 ++--- 12 files changed, 77 insertions(+), 77 deletions(-) diff --git a/test/integration/context_test.rb b/test/integration/context_test.rb index f6afaca52..f59147e29 100644 --- a/test/integration/context_test.rb +++ b/test/integration/context_test.rb @@ -632,7 +632,7 @@ def notice(output) end def test_has_key_will_not_add_an_error_for_missing_keys - with_error_mode(:strict) do + with_error_modes(:strict) do context = Context.new context.key?('unknown') assert_empty(context.errors) diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index 26b0e5f95..0fda83ca5 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -67,7 +67,7 @@ def test_missing_endtag_parse_time_error end def test_unrecognized_operator - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ') end diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index fe54bd99b..ae84fa36d 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -27,7 +27,7 @@ def test_float assert_template_result("-17.42", "{{ -17.42 }}") assert_template_result("2.5", "{{ 2.5 }}") - with_error_mode(:lax) do + with_error_modes(:lax) do assert_expression_result(0.0, "0.....5") assert_expression_result(0.0, "-0..1") end diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb index f5b483fcc..b82ce86c5 100644 --- a/test/integration/parsing_quirks_test.rb +++ b/test/integration/parsing_quirks_test.rb @@ -31,18 +31,18 @@ def test_raise_on_label_and_no_close_bracets_percent def test_error_on_empty_filter assert(Template.parse("{{test}}")) - with_error_mode(:lax) do + with_error_modes(:lax) do assert(Template.parse("{{|test}}")) end - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) { Template.parse("{{|test}}") } assert_raises(SyntaxError) { Template.parse("{{test |a|b|}}") } end end def test_meaningless_parens_error - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" Template.parse("{% if #{markup} %} YES {% endif %}") @@ -51,7 +51,7 @@ def test_meaningless_parens_error end def test_unexpected_characters_syntax_error - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do markup = "true && false" Template.parse("{% if #{markup} %} YES {% endif %}") @@ -70,7 +70,7 @@ def test_no_error_on_lax_empty_filter end def test_meaningless_parens_lax - with_error_mode(:lax) do + with_error_modes(:lax) do assigns = { 'b' => 'bar', 'c' => 'baz' } markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" assert_template_result(' YES ', "{% if #{markup} %} YES {% endif %}", assigns) @@ -78,7 +78,7 @@ def test_meaningless_parens_lax end def test_unexpected_characters_silently_eat_logic_lax - with_error_mode(:lax) do + with_error_modes(:lax) do markup = "true && false" assert_template_result(' YES ', "{% if #{markup} %} YES {% endif %}") markup = "false || true" @@ -93,7 +93,7 @@ def test_raise_on_invalid_tag_delimiter end def test_unanchored_filter_arguments - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result('hi', "{{ 'hi there' | split$$$:' ' | first }}") assert_template_result('x', "{{ 'X' | downcase) }}") @@ -106,14 +106,14 @@ def test_unanchored_filter_arguments end def test_invalid_variables_work - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result('bar', "{% assign 123foo = 'bar' %}{{ 123foo }}") assert_template_result('123', "{% assign 123 = 'bar' %}{{ 123 }}") end end def test_extra_dots_in_ranges - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result('12345', "{% for i in (1...5) %}{{ i }}{% endfor %}") end end @@ -133,7 +133,7 @@ def test_contains_in_id end def test_incomplete_expression - with_error_mode(:lax) do + with_error_modes(:lax) do assert_template_result("false", "{{ false - }}") assert_template_result("false", "{{ false > }}") assert_template_result("false", "{{ false < }}") diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index 14d8997b4..f3f865f08 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -96,12 +96,12 @@ def test_cycle_tag_with_error_mode template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" template2 = "{% cycle .5: 'a', 'b' %}" - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result("b", template1) assert_template_result("a", template2) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } @@ -121,7 +121,7 @@ def test_cycle_with_trailing_elements template4 = "#{assignments}{% cycle n e: 'a', 'b', 'c' %}" template5 = "#{assignments}{% cycle n e 'a', 'b', 'c' %}" - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result("a", template1) assert_template_result("a", template2) assert_template_result("a", template3) @@ -129,7 +129,7 @@ def test_cycle_with_trailing_elements assert_template_result("N", template5) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } error3 = assert_raises(Liquid::SyntaxError) { Template.parse(template3) } @@ -153,11 +153,11 @@ def test_cycle_name_with_invalid_expression {% endfor %} LIQUID - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end @@ -170,11 +170,11 @@ def test_cycle_variable_with_invalid_expression {% endfor %} LIQUID - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index b41a98c6e..3ccd17794 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -205,7 +205,7 @@ def test_dynamically_choosen_template end def test_rigid_parsing_errors - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result( 'hello value1 value2', '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', @@ -213,7 +213,7 @@ def test_rigid_parsing_errors ) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_syntax_error( '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', ) @@ -303,13 +303,13 @@ def test_passing_options_to_included_templates assert_raises(Liquid::SyntaxError) do Template.parse("{% include template %}", error_mode: :strict, environment: env).render!("template" => '{{ "X" || downcase }}') end - with_error_mode(:lax) do + with_error_modes(:lax) do assert_equal('x', Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: true, environment: env).render!("template" => '{{ "X" || downcase }}')) end assert_raises(Liquid::SyntaxError) do Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: [:locale], environment: env).render!("template" => '{{ "X" || downcase }}') end - with_error_mode(:lax) do + with_error_modes(:lax) do assert_equal('x', Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: [:error_mode], environment: env).render!("template" => '{{ "X" || downcase }}')) end end @@ -404,11 +404,11 @@ def test_render_tag_renders_error_with_template_name_from_template_factory def test_include_template_with_invalid_expression template = "{% include foo=>bar %}" - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end @@ -417,11 +417,11 @@ def test_include_template_with_invalid_expression def test_include_with_invalid_expression template = '{% include "snippet" with foo=>bar %}' - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end @@ -430,11 +430,11 @@ def test_include_with_invalid_expression def test_include_attribute_with_invalid_expression template = '{% include "snippet", key: foo=>bar %}' - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index 12759daf5..d6453fb51 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -106,7 +106,7 @@ def test_dynamically_choosen_templates_are_not_allowed end def test_rigid_parsing_errors - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result( 'hello value1 value2', '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', @@ -114,7 +114,7 @@ def test_rigid_parsing_errors ) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_syntax_error( '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', ) @@ -318,11 +318,11 @@ def test_render_tag_renders_error_with_template_name_from_template_factory def test_render_with_invalid_expression template = '{% render "snippet" with foo=>bar %}' - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end @@ -331,11 +331,11 @@ def test_render_with_invalid_expression def test_render_attribute_with_invalid_expression template = '{% render "snippet", key: foo=>bar %}' - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do refute_nil(Template.parse(template)) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index 0f9b051fe..828f77380 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -270,7 +270,7 @@ def test_tablerow_with_cols_attribute_in_rigid_mode 456 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template) end end @@ -285,7 +285,7 @@ def test_tablerow_with_limit_attribute_in_rigid_mode 123 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template) end end @@ -300,7 +300,7 @@ def test_tablerow_with_offset_attribute_in_rigid_mode 345 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template) end end @@ -315,7 +315,7 @@ def test_tablerow_with_range_attribute_in_rigid_mode 123 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template) end end @@ -331,7 +331,7 @@ def test_tablerow_with_multiple_attributes_in_rigid_mode 45 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template) end end @@ -347,7 +347,7 @@ def test_tablerow_with_variable_collection_in_rigid_mode 34 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template, { 'numbers' => [1, 2, 3, 4] }) end end @@ -363,7 +363,7 @@ def test_tablerow_with_dotted_access_in_rigid_mode 34 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template, { 'obj' => { 'numbers' => [1, 2, 3, 4] } }) end end @@ -378,7 +378,7 @@ def test_tablerow_with_bracketed_access_in_rigid_mode 1020 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template, { 'obj' => { 'numbers' => [10, 20] } }) end end @@ -393,7 +393,7 @@ def test_tablerow_without_attributes_in_rigid_mode 123 OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template) end end @@ -401,7 +401,7 @@ def test_tablerow_without_attributes_in_rigid_mode def test_tablerow_without_in_keyword_in_rigid_mode template = '{% tablerow i (1..10) %}{{ i }}{% endtablerow %}' - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(SyntaxError) { Template.parse(template) } assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) end @@ -410,7 +410,7 @@ def test_tablerow_without_in_keyword_in_rigid_mode def test_tablerow_with_multiple_invalid_attributes_reports_first_in_rigid_mode template = '{% tablerow i in (1..10) invalid1: 5, invalid2: 10 %}{{ i }}{% endtablerow %}' - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(SyntaxError) { Template.parse(template) } assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) end @@ -426,7 +426,7 @@ def test_tablerow_with_empty_collection_in_rigid_mode OUTPUT - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result(expected, template, { 'empty_array' => [] }) end end @@ -439,11 +439,11 @@ def test_tablerow_with_invalid_attribute_strict_vs_rigid 12345 OUTPUT - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result(expected, template) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(SyntaxError) { Template.parse(template) } assert_match(/Invalid attribute 'invalid_attr'/, error.message) end @@ -452,7 +452,7 @@ def test_tablerow_with_invalid_attribute_strict_vs_rigid def test_tablerow_with_invalid_expression_strict_vs_rigid template = '{% tablerow i in (1..5) limit: foo=>bar %}{{ i }}{% endtablerow %}' - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do expected = <<~OUTPUT @@ -460,7 +460,7 @@ def test_tablerow_with_invalid_expression_strict_vs_rigid assert_template_result(expected, template) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do # Rigid mode validates expression syntax and rejects invalid expressions error = assert_raises(SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) diff --git a/test/integration/variable_test.rb b/test/integration/variable_test.rb index 740ea9f9a..f0f2bc409 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -213,12 +213,12 @@ def test_variable_lookup_should_not_hang_with_invalid_syntax def test_filter_with_single_trailing_comma template = '{{ "hello" | append: "world", }}' - with_error_mode(:strict) do + with_error_modes(:strict) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/is not a valid expression/, error.message) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result('helloworld', template) end end @@ -226,12 +226,12 @@ def test_filter_with_single_trailing_comma def test_multiple_filters_with_trailing_commas template = '{{ "hello" | append: "1", | append: "2", }}' - with_error_mode(:strict) do + with_error_modes(:strict) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/is not a valid expression/, error.message) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result('hello12', template) end end @@ -239,12 +239,12 @@ def test_multiple_filters_with_trailing_commas def test_filter_with_colon_but_no_arguments template = '{{ "test" | upcase: }}' - with_error_mode(:strict) do + with_error_modes(:strict) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/is not a valid expression/, error.message) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result('TEST', template) end end @@ -252,12 +252,12 @@ def test_filter_with_colon_but_no_arguments def test_filter_chain_with_colon_no_args template = '{{ "test" | append: "x" | upcase: }}' - with_error_mode(:strict) do + with_error_modes(:strict) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/is not a valid expression/, error.message) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result('TESTX', template) end end @@ -265,12 +265,12 @@ def test_filter_chain_with_colon_no_args def test_combining_trailing_comma_and_empty_args template = '{{ "test" | append: "x", | upcase: }}' - with_error_mode(:strict) do + with_error_modes(:strict) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/is not a valid expression/, error.message) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do assert_template_result('TESTX', template) end end diff --git a/test/test_helper.rb b/test/test_helper.rb index a09f22771..293ea4766 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -82,7 +82,7 @@ def with_global_filter(*globals, &blk) Environment.dangerously_override(environment, &blk) end - def with_error_mode(*modes) + def with_error_modes(*modes) old_mode = Liquid::Environment.default.error_mode modes.each do |mode| Liquid::Environment.default.error_mode = mode diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index e6a5ababf..5e181f3eb 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -20,11 +20,11 @@ def test_case_with_trailing_element {%- endcase -%} LIQUID - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result("one", template) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Expected end_of_string but found/, error.message) @@ -41,11 +41,11 @@ def test_case_when_with_trailing_element {%- endcase -%} LIQUID - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result("one", template) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Expected end_of_string but found/, error.message) @@ -62,7 +62,7 @@ def test_case_when_with_comma {%- endcase -%} LIQUID - with_error_mode(:lax, :strict, :rigid) do + with_error_modes(:lax, :strict, :rigid) do assert_template_result("one", template) end end @@ -77,7 +77,7 @@ def test_case_when_with_or {%- endcase -%} LIQUID - with_error_mode(:lax, :strict, :rigid) do + with_error_modes(:lax, :strict, :rigid) do assert_template_result("one", template) end end @@ -93,11 +93,11 @@ def test_case_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result("one", template, assigns) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) @@ -115,11 +115,11 @@ def test_case_when_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - with_error_mode(:lax, :strict) do + with_error_modes(:lax, :strict) do assert_template_result("one", template, assigns) end - with_error_mode(:rigid) do + with_error_modes(:rigid) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 82ac03302..b46fc23f4 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -108,7 +108,7 @@ def test_dashes assert_equal(VariableLookup.new('foo-bar'), create_variable('foo-bar').name) assert_equal(VariableLookup.new('foo-bar-2'), create_variable('foo-bar-2').name) - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } assert_raises(Liquid::SyntaxError) { create_variable('-foo') } assert_raises(Liquid::SyntaxError) { create_variable('2foo') } @@ -154,7 +154,7 @@ def test_lax_filter_argument_parsing end def test_strict_filter_argument_parsing - with_error_mode(:strict) do + with_error_modes(:strict) do assert_raises(SyntaxError) do create_variable(%( number_of_comments | pluralize: 'comment': 'comments' )) end @@ -162,7 +162,7 @@ def test_strict_filter_argument_parsing end def test_rigid_filter_argument_parsing - with_error_mode(:rigid) do + with_error_modes(:rigid) do # optional colon var = create_variable(%(n | f1 | f2:)) assert_equal([['f1', []], ['f2', []]], var.filters) From 1e684d4e5eff05e722db2d6981f32b1ce85756f4 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Fri, 24 Oct 2025 09:06:12 +0200 Subject: [PATCH 36/41] Add rigid mode to `rake benchmark` task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The benchmark results show that rigid mode performs a bit better than both strict and lax modes across most metrics, including tokenization, parsing, rendering, and their combined operations. Rigid mode consistently delivers the highest number of iterations per second, with performance differences staying within 1–2% compared to the other modes ``` ================================================================================ /opt/rubies/3.4.1/bin/ruby ./performance/benchmark.rb lax Running benchmark for 20 seconds (with 10 seconds warmup). ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +YJIT +PRISM [arm64-darwin23] Warming up -------------------------------------- tokenize: 332.000 i/100ms parse: 14.000 i/100ms render: 61.000 i/100ms parse & render: 11.000 i/100ms Calculating ------------------------------------- tokenize: 3.325k (± 1.0%) i/s (300.73 μs/i) - 66.732k in 20.070562s parse: 148.166 (± 0.7%) i/s (6.75 ms/i) - 2.968k in 20.032971s render: 654.428 (± 4.0%) i/s (1.53 ms/i) - 13.115k in 20.090452s parse & render: 116.108 (± 1.7%) i/s (8.61 ms/i) - 2.332k in 20.089221s ================================================================================ /opt/rubies/3.4.1/bin/ruby ./performance/benchmark.rb strict Running benchmark for 20 seconds (with 10 seconds warmup). ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +YJIT +PRISM [arm64-darwin23] Warming up -------------------------------------- tokenize: 332.000 i/100ms parse: 14.000 i/100ms render: 61.000 i/100ms parse & render: 11.000 i/100ms Calculating ------------------------------------- tokenize: 3.332k (± 0.2%) i/s (300.14 μs/i) - 66.732k in 20.029095s parse: 145.674 (± 0.0%) i/s (6.86 ms/i) - 2.926k in 20.086104s render: 656.711 (± 4.6%) i/s (1.52 ms/i) - 13.115k in 20.050810s parse & render: 114.705 (± 0.0%) i/s (8.72 ms/i) - 2.299k in 20.043028s ================================================================================ /opt/rubies/3.4.1/bin/ruby ./performance/benchmark.rb rigid Running benchmark for 20 seconds (with 10 seconds warmup). ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +YJIT +PRISM [arm64-darwin23] Warming up -------------------------------------- tokenize: 333.000 i/100ms parse: 14.000 i/100ms render: 62.000 i/100ms parse & render: 11.000 i/100ms Calculating ------------------------------------- tokenize: 3.334k (± 0.3%) i/s (299.93 μs/i) - 66.933k in 20.075484s parse: 148.349 (± 2.0%) i/s (6.74 ms/i) - 2.968k in 20.019775s render: 663.752 (± 2.6%) i/s (1.51 ms/i) - 13.268k in 20.010303s parse & render: 116.464 (± 2.6%) i/s (8.59 ms/i) - 2.332k in 20.037869s liquid$ ``` --- Rakefile | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Rakefile b/Rakefile index 586b61734..878536bf5 100755 --- a/Rakefile +++ b/Rakefile @@ -88,8 +88,13 @@ namespace :benchmark do ruby "./performance/benchmark.rb strict" end - desc "Run the liquid benchmark with both lax and strict parsing" - task run: [:lax, :strict] + desc "Run the liquid benchmark with rigid parsing" + task :rigid do + ruby "./performance/benchmark.rb rigid" + end + + desc "Run the liquid benchmark with lax, strict, and rigid parsing" + task run: [:lax, :strict, :rigid] desc "Run unit benchmarks" namespace :unit do From 0064198d479f032f7298b046d76f18788f2fedda Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Fri, 24 Oct 2025 09:10:00 +0200 Subject: [PATCH 37/41] Simplify render/include tags following PR review feedback --- lib/liquid/tags/include.rb | 18 +++--------------- lib/liquid/tags/render.rb | 20 +++++--------------- lib/liquid/tags/table_row.rb | 3 +-- 3 files changed, 9 insertions(+), 32 deletions(-) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 8dc9567a5..5b2aaa432 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -88,20 +88,9 @@ def rigid_parse(markup) p = @parse_context.new_parser(markup) @template_name_expr = safe_parse_expression(p) - with_or_for = p.id?("for") || p.id?("with") || nil - @variable_name_expr = nil - if with_or_for - @variable_name_expr = safe_parse_expression(p) - end - - alias_name = nil - if p.id?("as") - alias_name = p.consume(:id) - end + @variable_name_expr = safe_parse_expression(p) if p.id?("for") || p.id?("with") + @alias_name = p.consume(:id) if p.id?("as") - @alias_name = alias_name - - # optional comma p.consume?(:comma) @attributes = {} @@ -109,7 +98,7 @@ def rigid_parse(markup) key = p.consume p.consume(:colon) @attributes[key] = safe_parse_expression(p) - p.consume?(:comma) # optional comma + p.consume?(:comma) end p.consume(:end_of_string) @@ -121,7 +110,6 @@ def strict_parse(markup) def lax_parse(markup) if markup =~ SYNTAX - template_name = Regexp.last_match(1) variable_name = Regexp.last_match(3) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index efe83b0d3..4d29e420e 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -89,21 +89,11 @@ def rigid_parse(markup) p = @parse_context.new_parser(markup) @template_name_expr = parse_expression(rigid_template_name(p), safe: true) - @variable_name_expr = nil - with_or_for = p.id?("for") || p.id?("with") || nil - if with_or_for - @variable_name_expr = safe_parse_expression(p) - end - - alias_name = nil - if p.id?("as") - alias_name = p.consume(:id) - end - - @alias_name = alias_name - @is_for_loop = (with_or_for == FOR) + with_or_for = p.id?("for") || p.id?("with") + @variable_name_expr = safe_parse_expression(p) if with_or_for + @alias_name = p.consume(:id) if p.id?("as") + @is_for_loop = (with_or_for == FOR) - # optional comma p.consume?(:comma) @attributes = {} @@ -111,7 +101,7 @@ def rigid_parse(markup) key = p.consume p.consume(:colon) @attributes[key] = safe_parse_expression(p) - p.consume?(:comma) # optional comma + p.consume?(:comma) end p.consume(:end_of_string) diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index 11aa4ef50..7348b5d91 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -45,7 +45,6 @@ def rigid_parse(markup) @collection_name = safe_parse_expression(p) - # optional comma p.consume?(:comma) @attributes = {} @@ -57,7 +56,7 @@ def rigid_parse(markup) p.consume(:colon) @attributes[key] = safe_parse_expression(p) - p.consume?(:comma) # optional comma + p.consume?(:comma) end p.consume(:end_of_string) From d6c8892beb5a1daf4c5eb0c47bfc42d7f5270fd4 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Fri, 24 Oct 2025 09:21:51 +0200 Subject: [PATCH 38/41] Extract `/\w+:0x\h{8}/` regex to `UNNAMED_CYCLE_PATTERN` constant --- lib/liquid/tags/cycle.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 639ed9a5b..7e17a9662 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -17,6 +17,7 @@ module Liquid class Cycle < Tag SimpleSyntax = /\A#{QuotedFragment}+/o NamedSyntax = /\A(#{QuotedFragment})\s*\:\s*(.*)/om + UNNAMED_CYCLE_PATTERN = /\w+:0x\h{8}/ attr_reader :variables @@ -86,7 +87,7 @@ def rigid_parse(markup) unless @is_named @name = @variables.to_s - @is_named = !@name.match?(/\w+:0x\h{8}/) + @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN) end end @@ -103,7 +104,7 @@ def lax_parse(markup) when SimpleSyntax @variables = variables_from_string(markup) @name = @variables.to_s - @is_named = !@name.match?(/\w+:0x\h{8}/) + @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN) else raise SyntaxError, options[:locale].t("errors.syntax.cycle") end From 0916f9a236212ab95abf9947baab72ba27ba7256 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Mon, 27 Oct 2025 09:53:14 +0100 Subject: [PATCH 39/41] Update README.md Co-authored-by: Gray Gilmore --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index da24e54fa..34884464d 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ Setting the error mode of Liquid lets you specify how strictly you want your tem Normally the parser is very lax and will accept almost anything without error. Unfortunately this can make it very hard to debug and can lead to unexpected behaviour. -Liquid also comes with a parser that can be used when editing templates to give better error messages +Liquid also comes with different parsers that can be used when editing templates to give better error messages when templates are invalid. You can enable this new parser like this: ```ruby From 438ac4f8ca3ddadaeaf32697ca6111ddf2aecd70 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Mon, 27 Oct 2025 10:02:16 +0100 Subject: [PATCH 40/41] Update test/integration/tags/table_row_test.rb Co-authored-by: Gray Gilmore --- test/integration/tags/table_row_test.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index 828f77380..44cdcdd99 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -461,7 +461,6 @@ def test_tablerow_with_invalid_expression_strict_vs_rigid end with_error_modes(:rigid) do - # Rigid mode validates expression syntax and rejects invalid expressions error = assert_raises(SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) end From 462a8b28cd02f287141b7904c8edaec2122ee324 Mon Sep 17 00:00:00 2001 From: Guilherme Carreiro Date: Mon, 27 Oct 2025 12:14:09 +0100 Subject: [PATCH 41/41] Add unit test mixing positional and kwargs arguments --- test/unit/variable_unit_test.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index b46fc23f4..cb3a6d8ce 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -184,6 +184,10 @@ def test_rigid_filter_argument_parsing var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2)) assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) + # positional and kwargs mixed + var = create_variable(%(n | filter: 'a', 'b', key1: 1, key2: 2, 'c')) + assert_equal([["filter", ["a", "b", "c"], { "key1" => 1, "key2" => 2 }]], var.filters) + # positional and kwargs intermixed (pos1, key1: val1, pos2) var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title")) assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters)