Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AuthenticationMixin] authentication_status to virtual_delegate #18196

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions app/models/authentication.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow - turns out arel_table[:status] does not access the database.

who knew?

end
end.else(-1)

RETRYABLE_STATUS = %w(error unreachable).freeze

CREDENTIAL_TYPES = {
Expand Down
16 changes: 10 additions & 6 deletions app/models/mixins/authentication_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

-> { 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
Expand Down Expand Up @@ -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
Expand Down
79 changes: 79 additions & 0 deletions spec/models/mixins/authentication_mixin_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Copy link
Member Author

@NickLaMuro NickLaMuro Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting: This spec and the one above have a important distinction to make here since it is slightly confusing when looking at the result that is provided by virtual_column_sql_value. The default value of "None" is desired here as we want to maintain the previous functionality when calling authentication_status.

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