Support usecase where a nil variable value is used in a logical expression

Author: albchu

lib/liquid/expression.rb

@@ -28,7 +28,7 @@ class Expression
     FLOAT_REGEX = /\A(-?\d+)\.\d+\z/
 
     class << self
-      def parse(markup, ss = StringScanner.new(""), cache = nil)
+      def parse(markup, ss = StringScanner.new(""), cache = nil, logical_expression = false)
         return unless markup
 
         markup = markup.strip # markup can be a frozen string
@@ -44,13 +44,13 @@ def parse(markup, ss = StringScanner.new(""), cache = nil)
         if cache
           return cache[markup] if cache.key?(markup)
 
-          cache[markup] = inner_parse(markup, ss, cache).freeze
+          cache[markup] = inner_parse(markup, ss, cache, logical_expression).freeze
         else
-          inner_parse(markup, ss, nil).freeze
+          inner_parse(markup, ss, nil, logical_expression).freeze
         end
       end
 
-      def inner_parse(markup, ss, cache)
+      def inner_parse(markup, ss, cache, logical_expression = false)
         return LogicalExpression.parse(markup, ss, cache)    if LogicalExpression.logical?(markup)
         return ComparisonExpression.parse(markup, ss, cache) if ComparisonExpression.comparison?(markup)
 
@@ -66,7 +66,7 @@ def inner_parse(markup, ss, cache)
         if (num = parse_number(markup, ss))
           num
         else
-          VariableLookup.parse(markup, ss, cache)
+          VariableLookup.parse(markup, ss, cache, logical_expression)
         end
       end
 

lib/liquid/expression/logical_expression.rb

@@ -26,7 +26,7 @@ def parse(markup, ss, cache)
           elsif logical?(last_expr)
             LogicalExpression.parse(last_expr, ss, cache)
           else
-            Condition.new(Expression.parse(last_expr, ss, cache), nil, nil)
+            Condition.new(Expression.parse(last_expr, ss, cache, true), nil, nil)
           end
 
           until expressions.empty?
@@ -40,7 +40,7 @@ def parse(markup, ss, cache)
             elsif logical?(expr)
               LogicalExpression.parse(expr, ss, cache)
             else
-              Condition.new(Expression.parse(expr, ss, cache), nil, nil)
+              Condition.new(Expression.parse(expr, ss, cache, true), nil, nil)
             end
 
             if operator == 'and'

lib/liquid/variable_lookup.rb

@@ -5,12 +5,14 @@ class VariableLookup
     COMMAND_METHODS = ['size', 'first', 'last'].freeze
 
     attr_reader :name, :lookups
+    attr_accessor :logical_expression
 
-    def self.parse(markup, string_scanner = StringScanner.new(""), cache = nil)
-      new(markup, string_scanner, cache)
+    def self.parse(markup, string_scanner = StringScanner.new(""), cache = nil, logical_expression = false)
+      new(markup, string_scanner, cache, logical_expression)
     end
 
-    def initialize(markup, string_scanner = StringScanner.new(""), cache = nil)
+    def initialize(markup, string_scanner = StringScanner.new(""), cache = nil, logical_expression = false)
+      @logical_expression = logical_expression
       lookups = markup.scan(VariableParser)
 
       name = lookups.shift
@@ -45,9 +47,17 @@ def lookup_command?(lookup_index)
     end
 
     def evaluate(context)
+      puts "variable_lookup #evaluate #{@name} #{logical_expression?}"
       name   = context.evaluate(@name)
       object = context.find_variable(name)
 
+      # When evaluating a logical expression, this variable lookup is part of a chain of conditions
+      # If the variable lookup returns nil, we must use the falsey value of the variable lookup
+      # rather than nil which is reserved for the usecase of rendering nothing.
+      if logical_expression? && object.nil?
+        return false
+      end
+
       @lookups.each_index do |i|
         key = context.evaluate(@lookups[i])
 
@@ -89,6 +99,10 @@ def ==(other)
       self.class == other.class && state == other.state
     end
 
+    def logical_expression?
+      @logical_expression
+    end
+
     protected
 
     def state

test/unit/boolean_unit_test.rb

@@ -106,10 +106,10 @@ def test_nil_renders_as_empty_string
   end
 
   def test_nil_comparison_with_blank
-    assert_parity_todo!("nil_value == blank", "false")
-    assert_parity_todo!("nil_value != blank", "true")
-    assert_parity_todo!("undefined != blank", "true")
-    assert_parity_todo!("undefined == blank", "false")
+    assert_parity("nil_value == blank", "false")
+    assert_parity("nil_value != blank", "true")
+    assert_parity("undefined != blank", "true")
+    assert_parity("undefined == blank", "false")
   end
 
   def test_if_with_variables
@@ -121,7 +121,13 @@ def test_if_with_variables
   end
 
   def test_nil_variable_in_and_expression
-    assert_parity_todo!("x and true", "false", { "x" => nil })
+    assert_parity("x and true", "false", { "x" => nil })
+    assert_parity("true and x", "false", { "x" => nil })
+  end
+
+  def test_boolean_variable_in_and_expression
+    assert_parity("true and x", "false", { "x" => false })
+    assert_parity("x and true", "false", { "x" => false })
   end
 
   private
@@ -133,10 +139,18 @@ def assert_parity_todo!(liquid_expression, expected_result, args = {})
   end
 
   def assert_parity(liquid_expression, expected_result, args = {})
-    assert_parity_scenario(:condition, "{% if #{liquid_expression} %}true{% else %}false{% endif %}", expected_result, args)
+    assert_condition(liquid_expression, expected_result, args)
+    assert_expression(liquid_expression, expected_result, args)
+  end
+
+  def assert_expression(liquid_expression, expected_result, args = {})
     assert_parity_scenario(:expression, "{{ #{liquid_expression} }}", expected_result, args)
   end
 
+  def assert_condition(liquid_condition, expected_result, args = {})
+    assert_parity_scenario(:condition, "{% if #{liquid_condition} %}true{% else %}false{% endif %}", expected_result, args)
+  end
+
   def assert_parity_scenario(kind, template, exp_output, args = {})
     act_output = Liquid::Template.parse(template).render(args)