Skip to content

Avoid an expression match on a dynamic variable#61

Merged
dylanahsmith merged 1 commit into
masterfrom
avoid-expression-match-on-dynamic-variable
Sep 28, 2020
Merged

Avoid an expression match on a dynamic variable#61
dylanahsmith merged 1 commit into
masterfrom
avoid-expression-match-on-dynamic-variable

Conversation

@dylanahsmith
Copy link
Copy Markdown
Contributor

Problem

Shopify/liquid#1300 introduced an error in liquid-c's unit tests:

  1) Failure:
VariableTest#test_literals [/Users/dylants/src/liquid-c/test/unit/variable_test.rb:37]:
--- expected
+++ actual
@@ -1 +1 @@
-[#<Liquid::VariableLookup:0xXXXXXX @name="", @lookups=[], @command_flags=0>, []]
+[nil, []]

Which is basically saying that [blank] is being parsed as having a nil expression instead of being the expected variable lookup for a blank string.

The problem is that an Expression::LITERALS lookup was being done twice on a square bracket variable lookup in liquid-c, once on the expression inside the square brackets (blank) and another time on the resulting expression. Only the former is done by the liquid gem (in Expression.parse).

Shopify/liquid#1300 caused this test to fail because there is an Expression::LITERALS mapping "blank" => "" and another mapping "" => nil. Previously, the first lookup would return a MethodLiteral object, which there wasn't a second mapping for in Expression::LITERALS.

Note that the assign tag doesn't even allow a variable to be assigned with an empty string for a name, it requires at least one character for the name.

Solution

Use a boolean flag to make sure we are only doing this Expression::LITERALS lookup when we haven't just parsed square brackets around the variable name.

Note that #60 will remove parse_variable method, which is why I wasn't seeing the failure when testing on that liquid-c branch.

for compatibility with `empty` and `blank` expression literals resolving
to an empty string, which shouldn't then be looked up again as a literl
since there is an empty string literal.
@dylanahsmith dylanahsmith merged commit d4a14c7 into master Sep 28, 2020
@dylanahsmith dylanahsmith deleted the avoid-expression-match-on-dynamic-variable branch September 28, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants