From 3b74f5a605011381624d17ea2f8579880280fdaf Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Fri, 8 Dec 2023 14:25:14 -0500 Subject: [PATCH 1/5] Update to latest rails 7.0 gems Require minimum for the CVE fixes: https://rubyonrails.org/2024/6/4/Rails-Versions-6-1-7-8-7-0-8-4-and-7-1-3-4-have-been-released Use virtual attributes 7.0 and other gems that support rails 7. --- Gemfile | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index 32441d56e89..b41ed6d903a 100644 --- a/Gemfile +++ b/Gemfile @@ -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 @@ -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 @@ -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" +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) @@ -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 From 9b6ebb1801622cdf904b141d3e996a2b21d25739 Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Tue, 23 Jan 2024 14:12:21 -0500 Subject: [PATCH 2/5] Drop no longer needed setting in rails 7 --- config/application.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/application.rb b/config/application.rb index 8102a7ccd85..7d217bab6b9 100644 --- a/config/application.rb +++ b/config/application.rb @@ -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 From 3263fddef5e5c036c2e3aec5d8fefe929fbd5356 Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Thu, 1 Feb 2024 16:15:10 -0500 Subject: [PATCH 3/5] Use Rails 7 interface for the Preloader See rails commit: e3b9779cb701c63012bc1af007c71dc5a888d35a Very similar to the change found in: https://github.com/ManageIQ/activerecord-virtual_attributes/pull/133/files Stub/mock the preloader in a rails 7 compatible way Rails 7 preloader requires a scope relation, not an array Note, it looks like only singular associations for already available_records are preloaded with an optimization in rails 7. See: https://www.github.com/rails/rails/pull/42654 --- lib/miq_preloader.rb | 5 +- spec/lib/extensions/ar_to_model_hash_spec.rb | 8 +-- spec/lib/miq_preloader_spec.rb | 57 +++++++++++++++----- 3 files changed, 51 insertions(+), 19 deletions(-) diff --git a/lib/miq_preloader.rb b/lib/miq_preloader.rb index 71b260d426c..7c976cadc71 100644 --- a/lib/miq_preloader.rb +++ b/lib/miq_preloader.rb @@ -22,8 +22,9 @@ module MiqPreloader # Currently an array does not work # @return [Array] 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 diff --git a/spec/lib/extensions/ar_to_model_hash_spec.rb b/spec/lib/extensions/ar_to_model_hash_spec.rb index dc0711b7124..8f3725eb429 100644 --- a/spec/lib/extensions/ar_to_model_hash_spec.rb +++ b/spec/lib/extensions/ar_to_model_hash_spec.rb @@ -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 diff --git a/spec/lib/miq_preloader_spec.rb b/spec/lib/miq_preloader_spec.rb index 9d22a8d9a1a..f1342b13cc0 100644 --- a/spec/lib/miq_preloader_spec.rb +++ b/spec/lib/miq_preloader_spec.rb @@ -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 end @@ -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 @@ -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 From 21bc1a9d373926282a50bd67028a0b98cb38409d Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Thu, 8 Feb 2024 11:39:00 -0500 Subject: [PATCH 4/5] Fix rails 7 deprecation warnings Fixes the following deprecation warnings: DEPRECATION WARNING: Time#to_s(:db) is deprecated. Please use Time#to_fs(:db) instead. (called from block (2 levels) in add_metric_rollups_for at /Users/joerafaniello/Code/manageiq/spec/support/chargeback_helper.rb:43) DEPRECATION WARNING: Integer#to_s(:human_size) is deprecated. Please use Integer#to_fs(:human_size) instead. (called from block in build_disk_message at /Users/joerafaniello/Code/manageiq/app/models/vm_reconfigure_task.rb:43) DEPRECATION WARNING: Integer#to_s(:human_size) is deprecated. Please use Integer#to_fs(:human_size) instead. (called from block (3 levels) in
at /Users/joerafaniello/Code/manageiq/spec/models/vm_reconfigure_task_spec.rb:45) DEPRECATION WARNING: Integer#to_s(:human_size) is deprecated. Please use Integer#to_fs(:human_size) instead. (called from block (3 levels) in
at /Users/joerafaniello/Code/manageiq/spec/models/vm_reconfigure_task_spec.rb:58) DEPRECATION WARNING: Integer#to_s(:human_size) is deprecated. Please use Integer#to_fs(:human_size) instead. (called from block (3 levels) in
at /Users/joerafaniello/Code/manageiq/spec/models/vm_reconfigure_task_spec.rb:59) --- app/models/vm_reconfigure_task.rb | 2 +- lib/acts_as_ar_model.rb | 2 +- spec/models/vm_reconfigure_task_spec.rb | 6 +++--- spec/support/chargeback_helper.rb | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/vm_reconfigure_task.rb b/app/models/vm_reconfigure_task.rb index de22865c6ef..8c4b07f6401 100644 --- a/app/models/vm_reconfigure_task.rb +++ b/app/models/vm_reconfigure_task.rb @@ -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 diff --git a/lib/acts_as_ar_model.rb b/lib/acts_as_ar_model.rb index 63eab08d8e7..bee378212ad 100644 --- a/lib/acts_as_ar_model.rb +++ b/lib/acts_as_ar_model.rb @@ -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 diff --git a/spec/models/vm_reconfigure_task_spec.rb b/spec/models/vm_reconfigure_task_spec.rb index e80c639534c..5d0a81b1f07 100644 --- a/spec/models/vm_reconfigure_task_spec.rb +++ b/spec/models/vm_reconfigure_task_spec.rb @@ -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 @@ -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 diff --git a/spec/support/chargeback_helper.rb b/spec/support/chargeback_helper.rb index fa07d76ca3f..94beca462a0 100644 --- a/spec/support/chargeback_helper.rb +++ b/spec/support/chargeback_helper.rb @@ -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 From b85a0c702249066af59af1561b3176cc9404eed2 Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Wed, 29 May 2024 16:31:39 -0400 Subject: [PATCH 5/5] Ignore calls to subclasses and descendants from rails 7 callers Rails changed in 7.0 to call subclasses from reload_schema_from_cache here: rails/rails@6f30cc0 It also changed to call descendants on the callback class (self) insead of the ActiveSupport::DescendantsTracker here: rails/rails@ffae3bd We're now getting called in descendants and subclasses from rails very early, at code load time, before models or as models are in between loading. We cannot trigger loads of subclasses while we're loading the existing class so we must wait and avoid it at this point. --- lib/extensions/descendant_loader.rb | 63 ++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/lib/extensions/descendant_loader.rb b/lib/extensions/descendant_loader.rb index 2f96473251d..f5ee438ba90 100644 --- a/lib/extensions/descendant_loader.rb +++ b/lib/extensions/descendant_loader.rb @@ -261,8 +261,24 @@ 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 `'", + # "xxx/manageiq/app/models/vm_or_template.rb:6:in `
'", 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) @loaded_descendants = true DescendantLoader.instance.load_subclasses(self) end @@ -270,11 +286,48 @@ def descendants 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 `
'", + # + # 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 `'", + # "xxx/manageiq/app/models/vm_or_template.rb:6:in `
'", + # + # 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