From 49d854d1f81033d0e861dc04ef48730d2a0e092e Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 13 Nov 2018 23:26:14 -0600 Subject: [PATCH] [AuthenticationMixin] authentication_status to virtual_delegate Creates a virtual_delegate for AuthenticationMixin#authentication_status which allows it to be included in reports and put in as part of the main query. The biggest thing of note in this change is how the status_severity_arel is being used to handle the sorting properly in the SQL query. It makes use of a SQL's case statement to handle what would normally be a `sort_by` on the hash lookup of Authentication::STATUS_SEVERITY. Instead, we match each key to the status column and use that as the value for the `ORDER BY exp DESC` in the `has_one`, which allows for the same affect. This also means that it can inject itself in the `SELECT` of a `MiqReport`, and avoid N+1's on things like quadicons and such for pages that use `MiqReport#paged_view_search`. --- app/models/authentication.rb | 16 ++++ app/models/mixins/authentication_mixin.rb | 16 ++-- .../mixins/authentication_mixin_spec.rb | 79 +++++++++++++++++++ 3 files changed, 105 insertions(+), 6 deletions(-) diff --git a/app/models/authentication.rb b/app/models/authentication.rb index 52dd992eed0..6497b5d21b6 100644 --- a/app/models/authentication.rb +++ b/app/models/authentication.rb @@ -55,6 +55,22 @@ def self.new(*args, &block) "invalid" => 3, ).freeze + # Builds a case statement that case be used in a sql ORDER BY. + # + # Generated SQL looks like: + # + # CASE + # WHEN LOWER(status) = '' THEN -1 + # WHEN LOWER(status) = 'valid' THEN 0 + # ... + # ELSE -1 + # + STATUS_SEVERITY_AREL = Arel::Nodes::Case.new.tap do |arel_case| + STATUS_SEVERITY.each do |value, sort_weight| + arel_case.when(arel_table[:status].lower.eq(value)).then(sort_weight) + end + end.else(-1) + RETRYABLE_STATUS = %w(error unreachable).freeze CREDENTIAL_TYPES = { diff --git a/app/models/mixins/authentication_mixin.rb b/app/models/mixins/authentication_mixin.rb index a92b849211c..6bfd97a785e 100644 --- a/app/models/mixins/authentication_mixin.rb +++ b/app/models/mixins/authentication_mixin.rb @@ -4,7 +4,16 @@ module AuthenticationMixin included do has_many :authentications, :as => :resource, :dependent => :destroy, :autosave => true - virtual_column :authentication_status, :type => :string + has_one :authentication_status_severity_level, + -> { order(Authentication::STATUS_SEVERITY_AREL.desc) }, + :as => :resource, + :inverse_of => :resource, + :class_name => "Authentication" + + virtual_delegate :authentication_status, + :to => "authentication_status_severity_level.status", + :default => "None", + :allow_nil => true def self.authentication_check_schedule zone = MiqServer.my_server.zone @@ -117,11 +126,6 @@ def missing_credentials?(type = nil) !has_credentials?(type) end - def authentication_status - ordered_auths = authentication_for_providers.sort_by(&:status_severity) - ordered_auths.last.try(:status) || "None" - end - def authentication_status_ok?(type = nil) authentication_best_fit(type).try(:status) == "Valid" end diff --git a/spec/models/mixins/authentication_mixin_spec.rb b/spec/models/mixins/authentication_mixin_spec.rb index d750cb93489..3d8cd3e5244 100644 --- a/spec/models/mixins/authentication_mixin_spec.rb +++ b/spec/models/mixins/authentication_mixin_spec.rb @@ -1,4 +1,11 @@ describe AuthenticationMixin do + include Spec::Support::ArelHelper + + let(:host) { FactoryGirl.create(:host) } + let(:invalid_auth) { FactoryGirl.create(:authentication, :resource => host, :status => "Invalid") } + let(:valid_auth) { FactoryGirl.create(:authentication, :resource => host, :status => "Valid") } + let(:authentications) { [invalid_auth, valid_auth] } + let(:test_class_instance) do Class.new(ActiveRecord::Base) do def self.name; "TestClass"; end @@ -721,4 +728,76 @@ def self.name; "TestClass"; end end end end + + describe "#authentication_status_severity_level" do + it "uses the least valid status" do + EvmSpecHelper.local_miq_server + authentications + expect(host.authentication_status_severity_level.status).to eq("Invalid") + end + end + + describe "#authentication_status" do + before do + EvmSpecHelper.local_miq_server + end + + context "with no authentications" do + before { host } + + it "is nil with sql" do + expect(virtual_column_sql_value(Host, "authentication_status")).to be_nil + end + + it "is 'None' with pure ruby (via relations)" do + expect(host.authentication_status).to eq("None") + end + + it "is 'None' when accessed via ruby, but fetched via sql" do + fetched_host = Host.select(:id, :authentication_status).first + expect(fetched_host.attributes[:authentication_status]).to be_nil + expect(fetched_host.authentication_status).to eq("None") + end + end + + context "with a valid authentication" do + before { valid_auth } + + it "is 'Valid' with sql" do + h = Host.select(:id, :authentication_status).first + + expect do + expect(h.authentication_status).to eq("Valid") + end.to match_query_limit_of(0) + expect(h.association(:authentication_status_severity_level)).not_to be_loaded + end + + it "is 'Valid' with ruby" do + h = Host.first # clean host record + + expect(h.authentication_status).to eq("Valid") + expect(h.association(:authentication_status_severity_level)).to be_loaded + end + end + + context "with a valid and invalid authentication" do + before { authentications } + + it "is 'Invalid' with sql" do + h = Host.select(:id, :authentication_status).first + + expect do + expect(h.authentication_status).to eq("Invalid") + end.to match_query_limit_of(0) + expect(h.association(:authentication_status_severity_level)).not_to be_loaded + end + + it "is 'Valid' with ruby" do + h = Host.first # clean host record + + expect(h.authentication_status).to eq("Invalid") + expect(h.association(:authentication_status_severity_level)).to be_loaded + end + end + end end