From 086b83641356d0451765a7d1295ebb32aada9425 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 21 May 2020 17:18:25 -0400 Subject: [PATCH 1/5] Better answer the question, field supported by sql --- lib/miq_expression/field.rb | 11 +++++++++-- lib/miq_expression/target.rb | 2 ++ spec/lib/miq_expression/field_spec.rb | 9 +++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/miq_expression/field.rb b/lib/miq_expression/field.rb index c4f8f516485..2e34f316f1e 100644 --- a/lib/miq_expression/field.rb +++ b/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? @@ -64,8 +70,9 @@ 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... + # 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)) diff --git a/lib/miq_expression/target.rb b/lib/miq_expression/target.rb index 23deda5aae2..099663cd1d1 100644 --- a/lib/miq_expression/target.rb +++ b/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 diff --git a/spec/lib/miq_expression/field_spec.rb b/spec/lib/miq_expression/field_spec.rb index 640f6879834..ca812e6f29e 100644 --- a/spec/lib/miq_expression/field_spec.rb +++ b/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 From f418f77e7752d1b2569947677d821dea0842275c Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 21 May 2020 17:38:03 -0400 Subject: [PATCH 2/5] better detect valid contains --- lib/miq_expression.rb | 10 ++------- spec/lib/miq_expression_spec.rb | 36 ++++++++++++++++++--------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index d648ffbb38c..8ed2ee70e7a 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -337,14 +337,8 @@ def sql_supports_atom?(exp) 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"]) + elsif exp[operator].key?("field") + Field.parse(exp[operator]["field"]).attribute_supported_by_sql? else return false end diff --git a/spec/lib/miq_expression_spec.rb b/spec/lib/miq_expression_spec.rb index 8e37ecc0955..acdc6b6ea94 100644 --- a/spec/lib/miq_expression_spec.rb +++ b/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 @@ -2983,12 +2993,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"}} From ff179cf1a76106bb96c4140d1f38daa451d47697 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 21 May 2020 18:44:22 -0400 Subject: [PATCH 3/5] make miq expression tag specs valid --- spec/lib/miq_expression/tag_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/lib/miq_expression/tag_spec.rb b/spec/lib/miq_expression/tag_spec.rb index 1195c55f1c0..db8fc52ee94 100644 --- a/spec/lib/miq_expression/tag_spec.rb +++ b/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 From 27a280b14be77e846e2a8183371ae4b3237d2e47 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 21 May 2020 18:45:03 -0400 Subject: [PATCH 4/5] MiqExpression::Tag now supports arel_attribute --- lib/miq_expression/field.rb | 24 ------------------------ lib/miq_expression/tag.rb | 7 +++++++ lib/miq_expression/target.rb | 24 ++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/lib/miq_expression/field.rb b/lib/miq_expression/field.rb index 2e34f316f1e..c87035c99e5 100644 --- a/lib/miq_expression/field.rb +++ b/lib/miq_expression/field.rb @@ -64,30 +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 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) if target - end - private def custom_attribute_column_name diff --git a/lib/miq_expression/tag.rb b/lib/miq_expression/tag.rb index 8979b22f0af..870106bd6f5 100644 --- a/lib/miq_expression/tag.rb +++ b/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 diff --git a/lib/miq_expression/target.rb b/lib/miq_expression/target.rb index 099663cd1d1..cc51cd782d3 100644 --- a/lib/miq_expression/target.rb +++ b/lib/miq_expression/target.rb @@ -104,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 From f01c37e01a2449ff937e9777eb5d8668694b57c4 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 21 May 2020 18:58:48 -0400 Subject: [PATCH 5/5] MiqExpresion: support tags with relationships --- lib/miq_expression.rb | 8 ++++---- spec/lib/miq_expression_spec.rb | 20 ++++++++++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/miq_expression.rb b/lib/miq_expression.rb index 8ed2ee70e7a..bfb99396468 100644 --- a/lib/miq_expression.rb +++ b/lib/miq_expression.rb @@ -335,8 +335,8 @@ 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 + 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 @@ -1367,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 diff --git a/spec/lib/miq_expression_spec.rb b/spec/lib/miq_expression_spec.rb index acdc6b6ea94..c6312161f67 100644 --- a/spec/lib/miq_expression_spec.rb +++ b/spec/lib/miq_expression_spec.rb @@ -560,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 @@ -2973,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)