Skip to content

Commit

Permalink
Fix undefined method reject! in allowed_storages
Browse files Browse the repository at this point in the history
In the case where a StorageProfile is selected we were failing to filter
out the allowed storages due to the `storages` variable now being an
ActiveRecord Association where `#reject!` isn't defined.

This also resolves an N+1 issue where each storage queries for all
storage_profiles.
  • Loading branch information
agrare committed Jul 25, 2023
1 parent 02ac620 commit c26ae68
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 4 deletions.
6 changes: 2 additions & 4 deletions app/models/miq_request_workflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1076,11 +1076,9 @@ def allowed_storages(_options = {})
rails_logger('allowed_storages', 0)
st = Time.now

storages = Storage.where(:id => HostStorage.writable_accessible.where(:host => hosts).select(:storage_id))
selected_storage_profile_id = get_value(@values[:placement_storage_profile])
if selected_storage_profile_id
storages.reject! { |s| !s.storage_profiles.pluck(:id).include?(selected_storage_profile_id) }
end
storages = selected_storage_profile_id ? StorageProfile.find(selected_storage_profile_id).storages : Storage.all
storages = storages.where(:id => HostStorage.writable_accessible.where(:host => hosts).select(:storage_id))
allowed_storages_cache = process_filter(:ds_filter, Storage, storages).collect do |s|
ci_to_hash_struct(s)
end
Expand Down
50 changes: 50 additions & 0 deletions spec/models/miq_request_workflow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@
let(:ems) { FactoryBot.create(:ext_management_system) }
let(:resource_pool) { FactoryBot.create(:resource_pool, :ems_id => ems.id) }
let(:host) { FactoryBot.create(:host, :ems_id => ems.id) }
let(:storage) { FactoryBot.create(:storage, :ems_id => ems.id) }

before { allow_any_instance_of(User).to receive(:get_timezone).and_return("UTC") }

Expand Down Expand Up @@ -313,6 +314,55 @@
workflow.allowed_clusters
end
end

context "#allowed_storages" do
it "filters out storages not mounted to allowed hosts" do
allow(workflow).to receive(:get_source_and_targets).and_return(:ems => workflow.ci_to_hash_struct(ems))
allow(workflow).to receive(:allowed_hosts_obj).and_return([host])

expect(workflow.allowed_storages).to be_empty
end

context "with writable storages" do
let!(:host_storage) { FactoryBot.create(:host_storage, :host => host, :storage => storage) }

it "returns all storages" do
allow(workflow).to receive(:get_source_and_targets).and_return(:ems => workflow.ci_to_hash_struct(ems))
allow(workflow).to receive(:allowed_hosts_obj).and_return([host])

storages = workflow.allowed_storages
expect(storages.count).to eq(1)
expect(storages.first).to have_attributes(:id => storage.id)
end
end

context "with read-only storages" do
let!(:host_storage) { FactoryBot.create(:host_storage, :host => host, :storage => storage, :read_only => true) }

it "filters out read-only storages" do
allow(workflow).to receive(:get_source_and_targets).and_return(:ems => workflow.ci_to_hash_struct(ems))
allow(workflow).to receive(:allowed_hosts_obj).and_return([host])

expect(workflow.allowed_storages).to be_empty
end
end

context "with a selected storage profile" do
let!(:host_storage) { FactoryBot.create(:host_storage, :host => host, :storage => storage) }
let!(:storage_profile) { FactoryBot.create(:storage_profile, :ems_id => ems.id) }

before do
workflow.values[:placement_storage_profile] = [storage_profile.id, storage_profile.name]
end

it "filters out storages not in the storage profile" do
allow(workflow).to receive(:get_source_and_targets).and_return(:ems => workflow.ci_to_hash_struct(ems))
allow(workflow).to receive(:allowed_hosts_obj).and_return([host])

expect(workflow.allowed_storages).to be_empty
end
end
end
end

context "#ci_to_hash_struct" do
Expand Down

0 comments on commit c26ae68

Please sign in to comment.