-
Notifications
You must be signed in to change notification settings - Fork 125
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
Changes from all commits
07500c8
9ad9001
3eab6b5
320a780
f3d7002
16b6d7e
157860a
a3eb361
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🇬🇧 |
||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really? 😞 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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 doserialize
. This approach could help guide us on what to do in core in the models.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 therails 7.1+
kwargs way and the bridge module would convert it to positional arguments for rails 7.0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'nuff said. My brain didn't click that this was a model problem, not really a migration problem.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.