Merge pull request #18709 from d-m-u/fixing_arel_node_grouping_for_and

Group AND expressions properly to account for nesting (cherry picked from commit 5c506fd) https://bugzilla.redhat.com/show_bug.cgi?id=1720753
ManageIQ · Jun 14, 2019 · 581d68f · 581d68f
1 parent e6fc485
commit 581d68f
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 9 deletions.
diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb
@@ -1400,7 +1400,7 @@ def to_arel(exp, tz)
         next if arel.blank?
         result << arel
       end
-      Arel::Nodes::And.new(operands)
+      Arel::Nodes::Grouping.new(Arel::Nodes::And.new(operands))
     when "or"
       operands = exp[operator].each_with_object([]) do |operand, result|
         next if operand.blank?

diff --git a/spec/lib/miq_expression_spec.rb b/spec/lib/miq_expression_spec.rb
@@ -379,14 +379,14 @@
       exp1 = {"STARTS WITH" => {"field" => "Vm-name", "value" => "foo"}}
       exp2 = {"ENDS WITH" => {"field" => "Vm-name", "value" => "bar"}}
       sql, * = MiqExpression.new("AND" => [exp1, exp2]).to_sql
-      expect(sql).to eq("\"vms\".\"name\" LIKE 'foo%' AND \"vms\".\"name\" LIKE '%bar'")
+      expect(sql).to eq("(\"vms\".\"name\" LIKE 'foo%' AND \"vms\".\"name\" LIKE '%bar')")
     end
 
     it "generates the SQL for an AND expression where only one is supported by SQL" do
       exp1 = {"STARTS WITH" => {"field" => "Vm-name", "value" => "foo"}}
       exp2 = {"ENDS WITH" => {"field" => "Vm-platform", "value" => "bar"}}
       sql, * = MiqExpression.new("AND" => [exp1, exp2]).to_sql
-      expect(sql).to eq("\"vms\".\"name\" LIKE 'foo%'")
+      expect(sql).to eq("(\"vms\".\"name\" LIKE 'foo%')")
     end
 
     it "returns nil for an AND expression where none is supported by SQL" do
@@ -417,12 +417,30 @@
       expect(sql).to be_nil
     end
 
-    it "properly groups the items in an AND/OR expression" do
-      exp = {"AND" => [{"EQUAL" => {"field" => "Vm-power_state", "value" => "on"}},
-                       {"OR" => [{"EQUAL" => {"field" => "Vm-name", "value" => "foo"}},
-                                 {"EQUAL" => {"field" => "Vm-name", "value" => "bar"}}]}]}
-      sql, * = described_class.new(exp).to_sql
-      expect(sql).to eq(%q("vms"."power_state" = 'on' AND ("vms"."name" = 'foo' OR "vms"."name" = 'bar')))
+    context "nested expressions" do
+      it "properly groups the items in an AND/OR expression" do
+        exp = {"AND" => [{"EQUAL" => {"field" => "Vm-power_state", "value" => "on"}},
+                         {"OR" => [{"EQUAL" => {"field" => "Vm-name", "value" => "foo"}},
+                                   {"EQUAL" => {"field" => "Vm-name", "value" => "bar"}}]}]}
+        sql, * = described_class.new(exp).to_sql
+        expect(sql).to eq(%q(("vms"."power_state" = 'on' AND ("vms"."name" = 'foo' OR "vms"."name" = 'bar'))))
+      end
+
+      it "properly groups the items in an OR/AND expression" do
+        exp = {"OR" => [{"EQUAL" => {"field" => "Vm-power_state", "value" => "on"}},
+                        {"AND" => [{"EQUAL" => {"field" => "Vm-name", "value" => "foo"}},
+                                   {"EQUAL" => {"field" => "Vm-name", "value" => "bar"}}]}]}
+        sql, * = described_class.new(exp).to_sql
+        expect(sql).to eq(%q(("vms"."power_state" = 'on' OR ("vms"."name" = 'foo' AND "vms"."name" = 'bar'))))
+      end
+
+      it "properly groups the items in an OR/OR expression" do
+        exp = {"OR" => [{"EQUAL" => {"field" => "Vm-power_state", "value" => "on"}},
+                        {"OR" => [{"EQUAL" => {"field" => "Vm-name", "value" => "foo"}},
+                                  {"EQUAL" => {"field" => "Vm-name", "value" => "bar"}}]}]}
+        sql, * = described_class.new(exp).to_sql
+        expect(sql).to eq(%q(("vms"."power_state" = 'on' OR ("vms"."name" = 'foo' OR "vms"."name" = 'bar'))))
+      end
     end
 
     it "generates the SQL for a NOT expression" do