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.

Sign up for GitHub

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

Jump to bottom

Rails 7 #22873

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Rails 7 #22873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b74f5a
Update to latest rails 7.0 gems
jrafanie Dec 8, 2023
9b6ebb1
Drop no longer needed setting in rails 7
jrafanie Jan 23, 2024
3263fdd
Use Rails 7 interface for the Preloader
jrafanie Feb 1, 2024
21bc1a9
Fix rails 7 deprecation warnings
jrafanie Feb 8, 2024
b85a0c7
Ignore calls to subclasses and descendants from rails 7 callers
jrafanie May 29, 2024
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
12 changes: 6 additions & 6 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ manageiq_plugin "manageiq-schema"

# Unmodified gems
gem "activerecord-session_store", "~>2.0"
gem "activerecord-virtual_attributes", "~>6.1.2"
gem "activerecord-virtual_attributes", "~>7.0.0"
gem "acts_as_tree", "~>2.7" # acts_as_tree needs to be required so that it loads before ancestry
gem "ancestry", "~>4.1.0", :require => false
gem "awesome_spawn", "~>1.6", :require => false
Expand All @@ -48,11 +48,11 @@ gem "inventory_refresh", "~>2.1", :require => false
gem "kubeclient", "~>4.0", :require => false # For scaling pods at runtime
gem "linux_admin", "~>3.0", :require => false
gem "listen", "~>3.2", :require => false
gem "manageiq-api-client", "~>0.3.6", :require => false
gem "manageiq-api-client", "~>0.5.0", :require => false
gem "manageiq-loggers", "~>1.0", ">=1.1.1", :require => false
gem "manageiq-messaging", "~>1.0", ">=1.4.3", :require => false
gem "manageiq-password", "~>1.0", :require => false
gem "manageiq-postgres_ha_admin", "~>3.2", :require => false
gem "manageiq-postgres_ha_admin", "~>3.3", :require => false
gem "manageiq-ssh-util", "~>0.2.0", :require => false
gem "memoist", "~>0.16.0", :require => false
gem "money", "~>6.13.5", :require => false
Expand All @@ -69,8 +69,8 @@ gem "psych", ">=3.1", :require => false #
gem "query_relation", "~>0.1.0", :require => false
gem "rack", ">=2.2.6.4", :require => false
gem "rack-attack", "~>6.5.0", :require => false
gem "rails", "~>6.1.7", ">=6.1.7.8"
gem "rails-i18n", "~>6.x"
gem "rails", "~>7.0.8", ">=7.0.8.4"
Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed this change to set minimum to ensure latest CVE coverage

gem "rails-i18n", "~>7.x"
gem "rake", ">=12.3.3", :require => false
gem "rest-client", "~>2.1.0", :require => false
gem "ruby_parser", :require => false # Required for i18n string extraction, and DescentdantLoader (via prism)
Expand Down Expand Up @@ -306,7 +306,7 @@ group :test do
gem "brakeman", "~>5.4", :require => false
gem "bundler-audit", :require => false
gem "capybara", "~>2.5.0", :require => false
gem "db-query-matchers", "~>0.10.0"
gem "db-query-matchers", "~>0.11.0"
gem "factory_bot", "~>5.1", :require => false
gem "simplecov", ">=0.21.2", :require => false
gem "timecop", "~>0.9", "!= 0.9.7", :require => false
Expand Down
2 changes: 1 addition & 1 deletion app/models/vm_reconfigure_task.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def self.build_message(options, key, message, modifier = nil)

def self.build_disk_message(options)
if options[:disk_add].present?
disk_sizes = options[:disk_add].collect { |d| d["disk_size_in_mb"].to_i.megabytes.to_s(:human_size) + ", Type: " + d["type"].to_s }
disk_sizes = options[:disk_add].collect { |d| d["disk_size_in_mb"].to_i.megabytes.to_fs(:human_size) + ", Type: " + d["type"].to_s }
"Add Disks: #{options[:disk_add].length} : #{disk_sizes.join(", ")} "
end
end
Expand Down
2 changes: 0 additions & 2 deletions config/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ class Application < Rails::Application

config.autoload_paths += config.eager_load_paths

config.autoloader = :zeitwerk

# config.load_defaults 6.1
# Disable defaults as ActiveRecord::Base.belongs_to_required_by_default = true causes MiqRegion.seed to fail validation on belongs_to maintenance zone

Expand Down
2 changes: 1 addition & 1 deletion lib/acts_as_ar_model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ def attribute_for_inspect(attr_name)
if value.kind_of?(String) && value.length > 50
"#{value[0..50]}...".inspect
elsif value.kind_of?(Date) || value.kind_of?(Time)
%("#{value.to_s(:db)}")
%("#{value.to_fs(:db)}")
else
value.inspect
end
Expand Down
63 changes: 58 additions & 5 deletions lib/extensions/descendant_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,20 +261,73 @@ def scoped_name(name, scopes)
end

module ArDescendantsWithLoader
# Use caution if modifying the list of excluded caller_locations below.
# These methods are called very early from rails during code load time,
# before the models are even loaded, as documented below.
# Ideally, a more resilient conditional can be used in the future.
#
# Called from __update_callbacks during model load time:
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/callbacks.rb:706:in `__update_callbacks'",
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/callbacks.rb:764:in `set_callback'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations.rb:172:in `validate'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/with.rb:96:in `block in validates_with'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/with.rb:85:in `each'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/with.rb:85:in `validates_with'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/format.rb:109:in `validates_format_of'",
# "xxx/gems/ancestry-4.1.0/lib/ancestry/has_ancestry.rb:30:in `has_ancestry'",
# "xxx/manageiq/app/models/vm_or_template.rb:15:in `<class:VmOrTemplate>'",
# "xxx/manageiq/app/models/vm_or_template.rb:6:in `<main>'",
def descendants
if Vmdb::Application.instance.initialized? && !defined? @loaded_descendants
if Vmdb::Application.instance.initialized? && !defined?(@loaded_descendants) && %w[__update_callbacks].exclude?(caller_locations(1..1).first.base_label)
Copy link
Member Author

Choose a reason for hiding this comment

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

the bot/cop complained about using caller_locations to build the full array so I accepted that suggestion even though I think it makes it a bit more convoluted. This method can be run many times, especially when models aren't yet loaded, so it should be as performant as possible.

@loaded_descendants = true
DescendantLoader.instance.load_subclasses(self)
end

super
end

# Rails 6.1 added an alias for subclasss to call direct_descendants in:
# https://github.com/rails/rails/commit/8f8aa857e084b76b1120edaa9bb9ce03ba1e6a19
# We need to get in front of it, like we do for descendants.
# Use caution if modifying the list of excluded caller_locations below.
# These methods are called very early from rails during code load time,
# before the models are even loaded, as documented below.
# Ideally, a more resilient conditional can be used in the future.
#
# Called from reload_schema_from_cache from virtual column definitions from ar_region from many/all models:
# "xxx/gems/activerecord-7.0.8.4/lib/active_record/model_schema.rb:609:in `reload_schema_from_cache'",
# "xxx/gems/activerecord-7.0.8.4/lib/active_record/timestamp.rb:94:in `reload_schema_from_cache'",
# "xxx/bundler/gems/activerecord-virtual_attributes-2c077434608f/lib/active_record/virtual_attributes.rb:60:in `virtual_attribute'",
# "xxx/bundler/gems/activerecord-virtual_attributes-2c077434608f/lib/active_record/virtual_attributes.rb:55:in `virtual_column'",
# "xxx/manageiq/lib/extensions/ar_region.rb:14:in `block in inherited'",
# "xxx/manageiq/lib/extensions/ar_region.rb:13:in `class_eval'",
# "xxx/manageiq/lib/extensions/ar_region.rb:13:in `inherited'",
# "xxx/manageiq/app/models/vm_or_template.rb:6:in `<main>'",
#
# Called from subclasses call in descendant_loader after the above callstack:
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/descendants_tracker.rb:83:in `subclasses'",
# "xxx/manageiq/lib/extensions/descendant_loader.rb:313:in `subclasses'",
# "xxx/gems/activerecord-7.0.8.4/lib/active_record/model_schema.rb:609:in `reload_schema_from_cache'",
# ...
#
# Called from descendants from descendant_loader via __update_callbacks:
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/descendants_tracker.rb:89:in `descendants'",
# "xxx/manageiq/lib/extensions/descendant_loader.rb:296:in `descendants'",
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/callbacks.rb:706:in `__update_callbacks'",
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/callbacks.rb:764:in `set_callback'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations.rb:172:in `validate'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/with.rb:96:in `block in validates_with'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/with.rb:85:in `each'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/with.rb:85:in `validates_with'",
# "xxx/gems/activemodel-7.0.8.4/lib/active_model/validations/format.rb:109:in `validates_format_of'",
# "xxx/gems/ancestry-4.1.0/lib/ancestry/has_ancestry.rb:30:in `has_ancestry'",
# "xxx/manageiq/app/models/vm_or_template.rb:15:in `<class:VmOrTemplate>'",
# "xxx/manageiq/app/models/vm_or_template.rb:6:in `<main>'",
#
# Called from subclasses from above callstack:
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/descendants_tracker.rb:83:in `subclasses'",
# "xxx/manageiq/lib/extensions/descendant_loader.rb:313:in `subclasses'",
# "xxx/gems/activesupport-7.0.8.4/lib/active_support/descendants_tracker.rb:89:in `descendants'",
# ...
def subclasses
if Vmdb::Application.instance.initialized? && !defined? @loaded_descendants
if Vmdb::Application.instance.initialized? && !defined?(@loaded_descendants) && %w[descendants reload_schema_from_cache subclasses].exclude?(caller_locations(1..1).first.base_label)
@loaded_descendants = true
DescendantLoader.instance.load_subclasses(self)
end
Expand Down
5 changes: 3 additions & 2 deletions lib/miq_preloader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ module MiqPreloader
# Currently an array does not work
# @return [Array<ActiveRecord::Base>] records
def self.preload(records, associations, preload_scope = nil)
preloader = ActiveRecord::Associations::Preloader.new
preloader.preload(records, associations, preload_scope)
# Rails 7 changed the interface. See rails commit: e3b9779cb701c63012bc1af007c71dc5a888d35a
# Note, added Array(records) as it could be a single element
ActiveRecord::Associations::Preloader.new(:records => Array(records), :associations => associations, :available_records => Array(preload_scope)).call
end

# for a record, cache results. Also cache the children's links back
Expand Down
8 changes: 4 additions & 4 deletions spec/lib/extensions/ar_to_model_hash_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@

# we're testing the preload of associations, skip the recursive .to_model_hash
allow_any_instance_of(test_vm_class).to receive(:to_model_hash_recursive)
allow(ActiveRecord::Associations::Preloader).to receive(:new).and_return(mocked_preloader)
end

def assert_preloaded(associations)
expect(mocked_preloader).to receive(:preload) do |_recs, assocs|
expect(assocs).to match_array(associations)
end
allow(ActiveRecord::Associations::Preloader).to receive(:new)
.with(:records => kind_of(Array), :associations => array_including(associations), :available_records => kind_of(Array))
.and_return(mocked_preloader)
expect(mocked_preloader).to receive(:call)

test_vm_class.new.to_model_hash(fixed_options)
end
Expand Down
57 changes: 44 additions & 13 deletions spec/lib/miq_preloader_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@
FactoryBot.create_list(:vm, 2, :ext_management_system => ems)
emses = ExtManagementSystem.all.load
vms = Vm.where(:ems_id => emses.select(:id))
expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 1)

# TODO: With rails 7, the query count below increased by 1.
# This PR says only the singular associations are preloaded in rails 7:
# https://github.com/rails/rails/pull/42654
expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 2)
expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries
Comment on lines +52 to 53
Copy link
Member

Choose a reason for hiding this comment

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

I'm content with this answer.

The most important part of this test is the second line: not_to make_database_queries

Copy link
Member Author

Choose a reason for hiding this comment

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

So, as I was looking at these tests, I was thinking some were not really the test and it's a test smell that we have several failures with rails 7 we needed to fix across the tests. Perhaps, we should have 1 failing test that we need to either mark pending, commented out with a TODO, etc. and remove the other assertions with comments that fail if they're testing the same things.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, I just fixed the comments and made them todos

end

Expand All @@ -55,45 +58,73 @@
vms = Vm.where(:ems_id => ems.id)
vms2 = vms.order(:id).to_a # preloaded vms to be used in tests

# there is a query for every ems
expect { preload(ems, :vms, vms) }.to make_database_queries(:count => 1)
# TODO: With rails 7, the query count below increased by 1.
# This PR says only the singular associations are preloaded in rails 7:
# https://github.com/rails/rails/pull/42654
expect { preload(ems, :vms, vms) }.to make_database_queries(:count => 2)

expect { preload(ems, :vms, vms) }.not_to make_database_queries
expect { expect(ems.vms).to match_array(vms2) }.not_to make_database_queries
# vms does not get loaded, so it is not preloaded
expect { expect(vms.first.ext_management_system).to eq(ems) }.to make_database_queries(:count => 2)

# TODO: With rails 7, the query count below decreased by 1.
expect { expect(vms.first.ext_management_system).to eq(ems) }.to make_database_queries(:count => 1)
end

it "preloads with a loaded relation (records is a relation)" do
ems
emses = ExtManagementSystem.all.load
vms = Vm.where(:ems_id => emses.select(:id)).load
expect { preload(emses, :vms, vms) }.not_to make_database_queries

# TODO: With rails 7, the query count below increased by 1.
# This PR says only the singular associations are preloaded in rails 7:
# https://github.com/rails/rails/pull/42654
expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 1)

expect { preload(emses, :vms, vms) }.not_to make_database_queries
expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries
expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries

# TODO: With rails 7, the query count below increased by 1.
expect { expect(vms.first.ext_management_system).to eq(ems) }.to make_database_queries(:count => 1)
end

it "preloads singular association with a loaded relation (records is a relation)" do
emses = ExtManagementSystem.select(:id).where(:id => ems.id).load
vms = Vm.where(:ems_id => emses.select(:id)).load
preload(vms, :ext_management_system, emses)

expect { preload(vms, :ext_management_system, emses) }.not_to make_database_queries
expect { expect(vms.size).to eq(2) }.not_to make_database_queries
expect { expect(vms.first.ext_management_system).to eq(emses.first) }.not_to make_database_queries
end

it "preloads with an array (records is a relation)" do
ems
emses = ExtManagementSystem.all.load
vms = Vm.where(:ems_id => emses.select(:id)).to_a
expect { preload(emses, :vms, vms) }.not_to make_database_queries
expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 1)

expect { preload(emses, :vms, vms) }.not_to make_database_queries
expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries
expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries

# TODO: With rails 7, the query count below increased by 1.
# This PR says only the singular associations are preloaded in rails 7:
# https://github.com/rails/rails/pull/42654
expect { expect(vms.first.ext_management_system).to eq(ems) }.to make_database_queries(:count => 1)
end

it "preloads with an array (records is an array)" do
ems
emses = ExtManagementSystem.all.load.to_a
vms = Vm.where(:ems_id => emses.map(&:id)).to_a
expect { preload(emses, :vms, vms) }.not_to make_database_queries
expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 1)

expect { preload(emses, :vms, vms) }.not_to make_database_queries
expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries
expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries

# TODO: With rails 7, the query count below increased by 1.
# This PR says only the singular associations are preloaded in rails 7:
# https://github.com/rails/rails/pull/42654
expect { expect(vms.first.ext_management_system).to eq(ems) }.to make_database_queries(:count => 1)
end

it "preloads a through with a loaded scope" do
Expand All @@ -109,8 +140,8 @@
expect { expect(nodes.first.container_images).to eq([image]) }.to make_database_queries(:count => 1)
end

def preload(*args)
MiqPreloader.preload(*args)
def preload(*args, **kwargs)
MiqPreloader.preload(*args, **kwargs)
end
end

Expand Down
6 changes: 3 additions & 3 deletions spec/models/vm_reconfigure_task_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
context "Single Disk add " do
let(:request_options) { {:disk_add => [{"disk_size_in_mb" => "33", "persistent" => "true", "type" => "thin"}.with_indifferent_access]} }
let(:description_partial) do
"Add Disks: 1 : #{request.options[:disk_add][0]["disk_size_in_mb"].to_i.megabytes.to_s(:human_size)}, Type: " \
"Add Disks: 1 : #{request.options[:disk_add][0]["disk_size_in_mb"].to_i.megabytes.to_fs(:human_size)}, Type: " \
"#{request.options[:disk_add][0]["type"]} "
end

Expand All @@ -55,8 +55,8 @@
{"disk_size_in_mb" => "44", "persistent" => "true", "type" => "thick"}.with_indifferent_access]}
end
let(:description_partial) do
"Add Disks: 2 : #{request.options[:disk_add][0]["disk_size_in_mb"].to_i.megabytes.to_s(:human_size)}, Type: " \
"#{request.options[:disk_add][0]["type"]}, #{request.options[:disk_add][1]["disk_size_in_mb"].to_i.megabytes.to_s(:human_size)}, Type: " \
"Add Disks: 2 : #{request.options[:disk_add][0]["disk_size_in_mb"].to_i.megabytes.to_fs(:human_size)}, Type: " \
"#{request.options[:disk_add][0]["type"]}, #{request.options[:disk_add][1]["disk_size_in_mb"].to_i.megabytes.to_fs(:human_size)}, Type: " \
"#{request.options[:disk_add][1]["type"]} "
end

Expand Down
2 changes: 1 addition & 1 deletion spec/support/chargeback_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def add_metric_rollups_for(resources, range, step, metric_rollup_params, trait =
base_values
end

record_values << "(#{values}, '#{time.to_s(:db)}')"
record_values << "(#{values}, '#{time.to_fs(:db)}')"
end
end

Expand Down