Merge pull request #20199 from kbrock/miq_expression_expansion

MiqExpression#contains improvement

lib/miq_expression.rb

@@ -335,16 +335,10 @@ def sql_supports_atom?(exp)
     operator = exp.keys.first
     case operator.downcase
     when "contains"
-      if exp[operator].keys.include?("tag") && exp[operator]["tag"].split(".").length == 2 # Only support for tags of the main model
-        return true
-      elsif exp[operator].keys.include?("field") && exp[operator]["field"].split(".").length == 2
-        db, field = exp[operator]["field"].split(".")
-        assoc, _column = field.split("-")
-        ref = db.constantize.reflect_on_association(assoc.to_sym)
-        return false unless ref
-        return false unless ref.macro == :has_many || ref.macro == :has_one
-        return false if ref.options && ref.options.key?(:as)
-        return field_in_sql?(exp[operator]["field"])
+      if exp[operator].key?("tag")
+        Tag.parse(exp[operator]["tag"]).reflection_supported_by_sql?
+      elsif exp[operator].key?("field")
+        Field.parse(exp[operator]["field"]).attribute_supported_by_sql?
       else
         return false
       end
@@ -1373,8 +1367,8 @@ def to_arel(exp, tz)
       # Only support for tags of the main model
       if exp[operator].key?("tag")
         tag = Tag.parse(exp[operator]["tag"])
-        ids = tag.model.find_tagged_with(:any => parsed_value, :ns => tag.namespace).pluck(:id)
-        tag.model.arel_attribute(:id).in(ids)
+        ids = tag.target.find_tagged_with(:any => parsed_value, :ns => tag.namespace).pluck(:id)
+        subquery_for_contains(tag, tag.arel_attribute.in(ids))
       else
         subquery_for_contains(field, arel_attribute.eq(parsed_value))
       end

lib/miq_expression/field.rb

@@ -28,10 +28,16 @@ def to_s
   def valid?
     (target < ApplicationRecord) &&
       (target.column_names.include?(column) || virtual_attribute? || custom_attribute_column?)
+  rescue ArgumentError
+    # the association chain is not legal, so no, it not valid
+    false
   end
 
   def attribute_supported_by_sql?
     !custom_attribute_column? && target.attribute_supported_by_sql?(column) && reflection_supported_by_sql?
+  rescue ArgumentError
+    # the association chain is not legal, so no, it is not supported by sql
+    false
   end
 
   def custom_attribute_column?
@@ -58,29 +64,6 @@ def report_column
     (associations + [column]).join('.')
   end
 
-  # this should only be accessed in MiqExpression
-  # please avoid using it
-  def arel_table
-    if associations.none?
-      model.arel_table
-    else
-      # if we are pointing to a table that already in the query, need to alias it
-      # seems we should be able to ask AR to do this for us...
-      ref = reflections.last
-      if ref.klass.table_name == model.table_name
-        ref.klass.arel_table.alias(ref.alias_candidate(model.table_name))
-      else
-        ref.klass.arel_table
-      end
-    end
-  end
-
-  # this should only be accessed in MiqExpression
-  # please avoid using it
-  def arel_attribute
-    target.arel_attribute(column, arel_table) if target
-  end
-
   private
 
   def custom_attribute_column_name

lib/miq_expression/tag.rb

@@ -48,6 +48,13 @@ def report_column
     "#{@base_namespace}.#{column}"
   end
 
+  # this should only be accessed in MiqExpression
+  # please avoid using it
+  # for tags, the tag tables are joined to the table's id
+  def arel_attribute
+    target&.arel_attribute("id", arel_table)
+  end
+
   private
 
   def tag_path

lib/miq_expression/target.rb

@@ -59,6 +59,8 @@ def plural?
 
   def reflection_supported_by_sql?
     model&.follow_associations(associations).present?
+  rescue ArgumentError
+    false
   end
 
   # AR or virtual reflections
@@ -102,6 +104,30 @@ def exclude_col_by_preprocess_options?(options)
     end
   end
 
+  # this should only be accessed in MiqExpression
+  # please avoid using it
+  def arel_table
+    if associations.none?
+      model.arel_table
+    else
+      # if the target attribute is in the same table as the model (the base table),
+      # alias the table to avoid conflicting table from clauses
+      # seems AR should do this for us...
+      ref = reflections.last
+      if ref.klass.table_name == model.table_name
+        ref.klass.arel_table.alias(ref.alias_candidate(model.table_name))
+      else
+        ref.klass.arel_table
+      end
+    end
+  end
+
+  # this should only be accessed in MiqExpression
+  # please avoid using it
+  def arel_attribute
+    target&.arel_attribute(column, arel_table)
+  end
+
   private
 
   def tag_path

spec/lib/miq_expression/field_spec.rb

@@ -142,6 +142,11 @@
       field = described_class.new(Vm, [], "destroy")
       expect(field).not_to be_valid
     end
+
+    it "returns false for non-valid associations" do
+      field = described_class.new(Vm, %w[bogus association], "foo")
+      expect(field).not_to be_valid
+    end
   end
 
   describe "#reflections" do
@@ -300,6 +305,10 @@
     it "detects if column is supported by sql through virtual association" do
       expect(MiqExpression::Field.parse("Vm.service-name").attribute_supported_by_sql?).to be_falsey
     end
+
+    it "returns false if the associations are bogus" do
+      expect(MiqExpression::Field.parse("Vm.bogus.service-name").attribute_supported_by_sql?).to be_falsey
+    end
   end
 
   describe "#virtual_attribute?" do

spec/lib/miq_expression/tag_spec.rb

@@ -140,25 +140,25 @@
 
   describe "#column_type" do
     it "is always a string" do
-      expect(described_class.new(Vm, [], "/host").column_type).to eq(:string)
+      expect(described_class.new(Vm, [], "host").column_type).to eq(:string)
     end
   end
 
   describe "#numeric?" do
     it "is never numeric" do
-      expect(described_class.new(Vm, [], "/host")).not_to be_numeric
+      expect(described_class.new(Vm, [], "host")).not_to be_numeric
     end
   end
 
   describe "#sub_type" do
     it "is always a string" do
-      expect(described_class.new(Vm, [], "/host").sub_type).to eq(:string)
+      expect(described_class.new(Vm, [], "host").sub_type).to eq(:string)
     end
   end
 
   describe "#attribute_supported_by_sql?" do
     it "is always false" do
-      expect(described_class.new(Vm, [], "/host")).not_to be_attribute_supported_by_sql
+      expect(described_class.new(Vm, [], "host")).not_to be_attribute_supported_by_sql
     end
   end
 end

spec/lib/miq_expression_spec.rb

@@ -495,24 +495,34 @@
       expect(sql).to eq(expected)
     end
 
-    it "can't generate the SQL for a CONTAINS expression with association.association-field" do
+    it "generates the SQL for a CONTAINS expression with association.association-field" do
       sql, * = MiqExpression.new("CONTAINS" => {"field" => "Vm.guest_applications.host-name", "value" => "foo"}).to_sql
-      expect(sql).to be_nil
+      rslt = "\"vms\".\"id\" IN (SELECT \"vms\".\"id\" FROM \"vms\" INNER JOIN \"guest_applications\" ON \"guest_applications\".\"vm_or_template_id\" = \"vms\".\"id\" INNER JOIN \"hosts\" ON \"hosts\".\"id\" = \"guest_applications\".\"host_id\" WHERE \"hosts\".\"name\" = 'foo')"
+      expect(sql).to eq(rslt)
     end
 
-    it "can't generate the SQL for a CONTAINS expression with belongs_to field" do
+    it "generates the SQL for a CONTAINS expression with belongs_to field" do
       sql, * = MiqExpression.new("CONTAINS" => {"field" => "Vm.host-name", "value" => "foo"}).to_sql
-      expect(sql).to be_nil
+      rslt = "\"vms\".\"id\" IN (SELECT \"vms\".\"id\" FROM \"vms\" INNER JOIN \"hosts\" ON \"hosts\".\"id\" = \"vms\".\"host_id\" WHERE \"hosts\".\"name\" = 'foo')"
+      expect(sql).to eq(rslt)
     end
 
-    it "can't generate the SQL for multi level contains with a scope" do
+    it "generates the SQL for multi level contains with a scope" do
       sql, _ = MiqExpression.new("CONTAINS" => {"field" => "ExtManagementSystem.clustered_hosts.operating_system-name", "value" => "RHEL"}).to_sql
-      expect(sql).to be_nil
+      rslt = "\"ext_management_systems\".\"id\" IN (SELECT \"ext_management_systems\".\"id\" FROM \"ext_management_systems\" " \
+             "INNER JOIN \"hosts\" ON \"hosts\".\"ems_id\" = \"ext_management_systems\".\"id\" AND \"hosts\".\"ems_cluster_id\" IS NOT NULL " \
+             "INNER JOIN \"operating_systems\" ON \"operating_systems\".\"host_id\" = \"hosts\".\"id\" " \
+             "WHERE \"operating_systems\".\"name\" = 'RHEL')"
+      expect(sql).to eq(rslt)
     end
 
-    it "can't generate the SQL for field belongs to 'has_and_belongs_to_many' association" do
+    it "generates the SQL for field belongs to 'has_and_belongs_to_many' association" do
       sql, _ = MiqExpression.new("CONTAINS" => {"field" => "ManageIQ::Providers::InfraManager::Vm.storages-name", "value" => "abc"}).to_sql
-      expect(sql).to be_nil
+      rslt = "\"vms\".\"id\" IN (SELECT \"vms\".\"id\" FROM \"vms\" " \
+             "INNER JOIN \"storages_vms_and_templates\" ON \"storages_vms_and_templates\".\"vm_or_template_id\" = \"vms\".\"id\" " \
+             "INNER JOIN \"storages\" ON \"storages\".\"id\" = \"storages_vms_and_templates\".\"storage_id\" " \
+             "WHERE \"storages\".\"name\" = 'abc')"
+      expect(sql).to eq(rslt)
     end
 
     it "can't generate the SQL for a CONTAINS expression virtualassociation" do
@@ -537,9 +547,9 @@
       expect(sql).to eq(expected)
     end
 
-    it "can't generate the SQL for a CONTAINS in the main table" do
+    it "generates the SQL for a CONTAINS in the main table" do
       sql, * = MiqExpression.new("CONTAINS" => {"field" => "Vm-name", "value" => "foo"}).to_sql
-      expect(sql).to be_nil
+      expect(sql).to eq("\"vms\".\"name\" = 'foo'")
     end
 
     it "generates the SQL for a CONTAINS expression with tag" do
@@ -550,6 +560,16 @@
       expect(sql).to eq("\"vms\".\"id\" IN (#{vm.id})")
     end
 
+    it "generates the SQL for a CONTAINS expression with multi tier tag" do
+      tag = FactoryBot.create(:tag, :name => "/managed/operations/analysis_failed")
+      host = FactoryBot.create(:host_vmware, :tags => [tag])
+      exp = {"CONTAINS" => {"tag" => "VmInfra.host.managed-operations", "value" => "analysis_failed"}}
+      rslt = "\"vms\".\"id\" IN (SELECT \"vms\".\"id\" FROM \"vms\" INNER JOIN \"hosts\" ON \"hosts\".\"id\" = \"vms\".\"host_id\" WHERE \"hosts\".\"id\" IN (#{host.id}))"
+
+      sql, * = MiqExpression.new(exp).to_sql
+      expect(sql).to eq(rslt)
+    end
+
     it "returns nil for a Registry expression" do
       exp = {"=" => {"regkey" => "test", "regval" => "value", "value" => "data"}}
       sql, * = MiqExpression.new(exp).to_sql
@@ -2963,15 +2983,21 @@
           expect(described_class.new(nil).sql_supports_atom?(expression)).to eq(true)
         end
 
-        it "returns false for tag of associated model" do
+        it "returns true for tag of associated model" do
           field = "Vm.ext_management_system.managed-openshiftroles"
           expression = {"CONTAINS" => {"tag" => field, "value" => "node"}}
+          expect(described_class.new(nil).sql_supports_atom?(expression)).to eq(true)
+        end
+
+        it "returns false for tag of virtual associated model" do
+          field = "Vm.processes.managed-openshiftroles"
+          expression = {"CONTAINS" => {"tag" => field, "value" => "node"}}
           expect(described_class.new(nil).sql_supports_atom?(expression)).to eq(false)
         end
       end
 
       context "operation with 'field'" do
-        it "returns false if format of field is model.association..association-field" do
+        it "returns false if format of field is model.association.association-field" do
           field = "ManageIQ::Providers::InfraManager::Vm.service.user.vms-active"
           expression = {"CONTAINS" => {"field" => field, "value" => "true"}}
           expect(described_class.new(nil).sql_supports_atom?(expression)).to eq(false)
@@ -2983,12 +3009,6 @@
           expect(described_class.new(nil).sql_supports_atom?(expression)).to eq(false)
         end
 
-        it "returns false if field belongs to 'has_and_belongs_to_many' association" do
-          field = "ManageIQ::Providers::InfraManager::Vm.storages-name"
-          expression = {"CONTAINS" => {"field" => field, "value" => "abc"}}
-          expect(described_class.new(nil).sql_supports_atom?(expression)).to eq(false)
-        end
-
         it "returns false if field belongs to 'has_many' polymorhic/polymorhic association" do
           field = "ManageIQ::Providers::InfraManager::Vm.advanced_settings-region_number"
           expression = {"CONTAINS" => {"field" => field, "value" => "1"}}