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

Add rails 7.1 and 7.2 support #756

Merged
merged 8 commits into from
Oct 25, 2024
Merged
5 changes: 5 additions & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ jobs:
- '3.3'
rails-version:
- '7.0'
- '7.1'
- '7.2'
exclude:
- ruby-version: '3.0'
rails-version: '7.2'
services:
postgres:
image: manageiq/postgresql:13
Expand Down
8 changes: 6 additions & 2 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@ require File.join(Bundler::Plugin.index.load_paths("bundler-inject")[0], "bundle
# your gem to rubygems.org.

minimum_version =
case ENV.fetch('TEST_RAILS_VERSION', nil)
when "7.0"
case ENV['TEST_RAILS_VERSION']
when "7.2"
"~>7.2.1"
when "7.1"
"~>7.1.4"
else
# Default local bundling to use this version for generating migrations
"~>7.0.8"
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class SettingsChange < ActiveRecord::Base
serialize :value
end
class Zone < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class ConvertQuadiconSettingsKeys < ActiveRecord::Migration[5.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class MiqRequestTask < ActiveRecord::Base
self.inheritance_column = :_type_disabled
include ActiveRecord::IdRegions

serialize :options, Hash
serialize :options, :type => Hash
belongs_to :conversion_host, :class_name => "AddConversionHostIdToMiqRequestTasks::ConversionHost"
end

Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20191002103406_remove_quadicon_settings.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class RemoveQuadiconSettings < ActiveRecord::Migration[5.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20200331150436_rename_foreman_features.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class RenameForemanFeatures < ActiveRecord::Migration[5.1]
class MiqProductFeature < ActiveRecord::Base; end
class MiqRolesFeature < ActiveRecord::Base; end
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

FEATURE_MAPPING = {
Expand Down
2 changes: 1 addition & 1 deletion db/migrate/20200512201614_update_chargeback_startpage.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateChargebackStartpage < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateChargebackAssignmentsStartpage < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
include ActiveRecord::IdRegions
end

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateChargebackReportsStartpage < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
include ActiveRecord::IdRegions
end

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class AdjustControlExplorerStartpageEntries < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateStartupShortcutAfterPolicyProfileDeexplorization < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateStartpageShortcutAfterActionsDeExplorization < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdatePolicyStartpageUrlAfterDeExplorization < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdatePolicyRsopStartpageUrl < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdatePolicyExportStartpageUrl < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdatePolicyLogStartpageUrl < ActiveRecord::Migration[5.2]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateAlertsStartpageUrlAfterDeExplorization < ActiveRecord::Migration[6.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateAlertProfilesStartpageUrlAfterDeExplorization < ActiveRecord::Migration[6.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateEventsStartpageUrlAfterDeExplorization < ActiveRecord::Migration[6.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateConditionsStartpageUrlAfterDeExplorization < ActiveRecord::Migration[6.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateAutomationProviderStartpage < ActiveRecord::Migration[6.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class UpdateStartpageShortcutAfterServicesDeExplorization < ActiveRecord::Migration[6.0]
class User < ActiveRecord::Base
serialize :settings, Hash
serialize :settings, :type => Hash
end

def up
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class MiqRequest < ActiveRecord::Base

self.inheritance_column = :_type_disabled

serialize :options, Hash
serialize :options, :type => Hash
end

def up
Expand Down
3 changes: 2 additions & 1 deletion lib/manageiq/schema/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ class Engine < ::Rails::Engine

ActiveSupport.on_load(:active_record) do
require_relative 'migrate_with_cleared_schema_cache'
require_relative 'serialize_positional_to_kwargs_bridge'
require_relative 'schema_statements'
require_relative 'command_recorder'
require_relative 'schema_dumper'

ActiveRecord::Migration.prepend(MigrateWithClearedSchemaCache)

ActiveRecord::AttributeMethods::Serialization::ClassMethods.send(:prepend, ManageIQ::Schema::SerializePositionalToKwargsBridge)
ActiveRecord::ConnectionAdapters::AbstractAdapter.include(SchemaStatements)
ActiveRecord::Migration::CommandRecorder.include(CommandRecorder)
ActiveRecord::ConnectionAdapters::SchemaDumper.prepend(SchemaDumper)
Expand Down
19 changes: 19 additions & 0 deletions lib/manageiq/schema/serialize_positional_to_kwargs_bridge.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module ManageIQ
module Schema
module SerializePositionalToKwargsBridge
def serialize(*args, **options)
return super if Rails.version >= "7.1"

# For rails 7.0.x, convert 7.1+ kwargs for coder/type into the positional argument
# class_name_or_coder
if options[:coder]
args << options.delete(:coder)
elsif options[:type]
args << options.delete(:type)
end

super(*args, **options)
end
end
end
end
2 changes: 1 addition & 1 deletion manageiq-schema.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Gem::Specification.new do |spec|
spec.require_paths = ["lib"]

spec.add_dependency "ancestry"
spec.add_dependency "activerecord-id_regions", "~> 0.4.0"
spec.add_dependency "activerecord-id_regions", "~> 0.5.0"
spec.add_dependency "manageiq-password", ">= 1.2.0", "< 2"
spec.add_dependency "more_core_extensions", ">= 3.5", "< 5"
spec.add_dependency "pg"
Expand Down
5 changes: 5 additions & 0 deletions spec/dummy/config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ class Application < Rails::Application
# migration specs to honor it.
config.active_record.belongs_to_required_by_default = false

# Note, you can't pass kwargs :coder => YAML to serialize until rails 7.1 as it was a positional
# argument previously. To avoid a case statement in all usages of serialize, we're defaulting
# all serialized columns to YAML for rails 7.1+ here. Ideally, we would use JSON if we find we can
# use a simpler datatype. See: https://github.com/rails/rails/pull/47463
config.active_record.default_column_serializer = YAML if Rails.version >= "7.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Do we want to inject this for each of the migrations and use the "bridge" in a similar way I'm handling :type above? We'd need to specify :coder => YAML for nearly all the migrations that do serialize. This approach could help guide us on what to do in core in the models.

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words, we can specify the column serializer/coder in each migration or here only once. The negative of doing it here is it's less explicit and we're less likely to actually change to json or another / more supported coder.

cc @Fryguy @kbrock

Copy link
Member Author

Choose a reason for hiding this comment

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

On the flip side, I assume you can override the default in each migration and specify an override coder one by one so any that are serializing basic objects can likely specify json while the default for others will be to use yaml.

Copy link
Member

@Fryguy Fryguy Oct 10, 2024

Choose a reason for hiding this comment

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

I'm trying to understand the implications of this change.

Ideally, ActiveRecord::Migration would know to use coder yaml for serialize on older migration versions that are indicated with the [7.0] thing in the constant name. To me, it's a bug that they didn't do this, so maybe we should open a bug with them.

That aside, I'm concerned if we set a global default, then it will break future migrations. So, considering that, I think it's better, though more verbose, to set the coder => yaml on every migration. We're already changing it for the :type change, so it's not like it's more lines of code. This way we can do it the "right way" moving forward.


Related question, what is the coder on the model side? Do we need a similar change in core changing every serialized column to say what coder it is?

Copy link
Member

Choose a reason for hiding this comment

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

What is in our way from converting all our serialized fields from yaml => json?
What do we see as the biggest barier

  • Time to write migration code
  • Time to run migrations on customer code (upgrades)
  • Possible datatype incompatibility (symbols, dates, active record)
  • Test that app is still working

Copy link
Member Author

Choose a reason for hiding this comment

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

Related question, what is the coder on the model side? Do we need a similar change in core changing every serialized column to say what coder it is?

You can see some of the very wet stuff on the core PR... so far, type is where I did changes since I could specify a default coder using config.active_record.default_column_serializer = YAML. I would prefer a common approach for both and I like the idea of writing serialize in the rails 7.1+ kwargs way and the bridge module would convert it to positional arguments for rails 7.0.

Copy link
Member

@Fryguy Fryguy Oct 10, 2024

Choose a reason for hiding this comment

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

What is in our way from converting all our serialized fields from yaml => json?

non-json types, notable Ruby objects, DateTime, Symbol. If we know for a fact a column doesn't have those it's an easy conversion.

However, that's moving forward - we still need to deal with older migrations regardless.

Copy link
Member

Choose a reason for hiding this comment

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

Technically, this is a gray area because we're defining stub models in migrations. I don't think they can guard for that type of thing.

'nuff said. My brain didn't click that this was a model problem, not really a migration problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, I kept the first attempt commit. I had the bridge module implementing "7.0 format" we don't need to change model calls to serialize but it's ugly:
737278c

Compare that to the "7.1 format" where we convert 7.0 format to 7.1 we have now 410fbfc

I kept it around to help decide which way forward would be best.

I'll mark this was WIP again and apply the changes suggested here:

  1. try moving the module to the plugin instead of the dummy app
  2. treat coder the same way as type, convert all serialize calls to specify a YAML coder and remove the default column serializer setting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I tried removing config.active_record.default_column_serializer = YAML if Rails.version >= "7.1" and defaulted to setting :coder => YAML throughout but that didn't work well with 7.0. I removed that change. We'll carry this for now and revisit it once we're off of 7.0.

config.active_record.use_yaml_unsafe_load = true
end
end
7 changes: 6 additions & 1 deletion spec/lib/generators/migration_generator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
require 'generators/migration/migration_generator'

describe ManageIQ::Schema::MigrationGenerator do
include Rails::Generators::Testing::Behaviour
if Rails.version >= "7.1"
include Rails::Generators::Testing::Behavior
else
include Rails::Generators::Testing::Behaviour
Copy link
Member Author

Choose a reason for hiding this comment

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

🇬🇧

end
Copy link
Member

Choose a reason for hiding this comment

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

Really? 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

It was more a "consistency in naming" vs. 🇬🇧 / 🇺🇸 spelling fight. https://www.github.com/rails/rails/pull/45180

I do prefer the fight though. It's more fun.


include Rails::Generators::Testing::SetupAndTeardown
include Rails::Generators::Testing::Assertions
include FileUtils
Expand Down
19 changes: 13 additions & 6 deletions spec/support/migration_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ def suppress_migration_messages
def migrate_to(version)
suppress_migration_messages do
migration_dir = Rails.application.config.paths["db/migrate"]
migration_conn = ::ActiveRecord::Base.connection.schema_migration
ActiveRecord::MigrationContext.new(migration_dir, migration_conn).migrate(version)
ActiveRecord::MigrationContext.new(migration_dir, schema_migration).migrate(version)
end
end

Expand All @@ -126,16 +125,24 @@ def previous_migration_version

def run_migrate
migration_dir = Rails.application.config.paths["db/migrate"]
migration_conn = ::ActiveRecord::Base.connection.schema_migration
context = ActiveRecord::MigrationContext.new(migration_dir, migration_conn)
context = ActiveRecord::MigrationContext.new(migration_dir, schema_migration)

context.run(migration_direction, this_migration_version)
end

def schema_migration
# Rails 7.2 refactored the schema_migration metadata and context to the pool
# https://www.github.com/rails/rails/pull/51162
if Rails.version >= "7.2"
::ActiveRecord::Base.connection.pool.schema_migration
else
::ActiveRecord::Base.connection.schema_migration
end
end

def schema_migrations
migration_dir = Rails.application.config.paths["db/migrate"]
migration_conn = ::ActiveRecord::Base.connection.schema_migration
ActiveRecord::MigrationContext.new(migration_dir, migration_conn).migrations
ActiveRecord::MigrationContext.new(migration_dir, schema_migration).migrations
end

def migrations_and_index
Expand Down