Add rails 7.1 and 7.2 support #756

jrafanie · 2024-10-04T22:01:08Z

Depends on:

Use core minimum but allow 7.0/7.1/7.2 since core sets upper limit (breaking change) activerecord-id_regions#40 Merge and release

High level:

YAML serialize is going out of style and we should convert as many to json or other types if possible. The interface for serialize changed so I have a bridge that changes our existing migrations to use the 7.1 interface with a layer to convert it back to 7.0 format if using 7.0. I think this way is easier to read than the prior commit: Prepend serialize in 7.1+ to convert positional arguments to kwargs

jrafanie · 2024-10-05T02:01:53Z

spec/dummy/config/application.rb

+    # 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"


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.

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

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.

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?

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

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.

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.

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.

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:

try moving the module to the plugin instead of the dummy app

treat coder the same way as type, convert all serialize calls to specify a YAML coder and remove the default column serializer setting.

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.

jrafanie · 2024-10-10T17:26:07Z

Ok, ManageIQ/activerecord-id_regions#40 was merged and released as 0.5.0 so this is now ready for review.

jrafanie · 2024-10-10T17:27:57Z

spec/dummy/config/initializers/serialize_positional_to_kwargs_bridge.rb

@@ -0,0 +1,19 @@
+module Dummy
+  module SerializePositionalToKwargsBridge
+    def serialize(*args, **options)


This is the largest change in rails 7.1+ for this repository. Unlike the "coder" change above, there isn't an option to default the type so what I did was accepted the new interface: kwargs for type and built a bridge to convert kwargs back to positional arguments when run with rails 7.0.

Fryguy · 2024-10-10T18:45:00Z

spec/dummy/config/initializers/serialize_positional_to_kwargs_bridge.rb

@@ -0,0 +1,19 @@
+module Dummy


This feels like it should be part of the plugin itself, and not dummy. The dummy app is only for tests here, so I presume we will hit this same issue in application code where the dummy won't be present. Perhaps it could be done similar to how we patch migrate for clearing caches.

Nice, I can look into that. Good idea.

jrafanie · 2024-10-10T19:27:32Z

spec/lib/generators/migration_generator_spec.rb

+  if Rails.version >= "7.1"
+    include Rails::Generators::Testing::Behavior
+  else
+    include Rails::Generators::Testing::Behaviour


Fryguy · 2024-10-10T19:34:41Z

spec/lib/generators/migration_generator_spec.rb

+    include Rails::Generators::Testing::Behavior
+  else
+    include Rails::Generators::Testing::Behaviour
+  end


Really? 😞

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.

Add them to the matrix.

See https://www.github.com/rails/rails/pull/47463

We can intercept serialize for 7.0 and convert kwargs back to the positional class_name_or_coder. Update existing migrations to use rails 7.1 interface.

See https://www.github.com/rails/rails/pull/45180

See: https://www.github.com/rails/rails/pull/51162

jrafanie · 2024-10-23T14:59:43Z

Ok, I'm taking this out of WIP. I suspect we'll do something similar in serialize calls in the models in core.

Type was changed from positional to a keyword argument 'type' in 7.1. The positional argument is removed in rails 7.2. We have a helper bridge module prepending support for either kwargs or positional arguments passed to serialize which converts to positional for rails 7.0 and kwargs for 7.1+. This was added and automatically loaded in the rails app via the engine in schema: ManageIQ/manageiq-schema#756

Type was changed from positional to a keyword argument 'type' in 7.1. The positional argument is removed in rails 7.2. We have a helper bridge module prepending support for either kwargs or positional arguments passed to serialize which converts to positional for rails 7.0 and kwargs for 7.1+. This was added and automatically loaded in the rails app via the engine in schema: ManageIQ/manageiq-schema#756 Similar change as core: ManageIQ/manageiq#23253

Type was changed from positional to a keyword argument 'type' in 7.1. The positional argument is removed in rails 7.2. We have a helper bridge module prepending support for either kwargs or positional arguments passed to serialize which converts to positional for rails 7.0 and kwargs for 7.1+. This was added and automatically loaded in the rails app via the engine in schema: ManageIQ/manageiq-schema#756

jrafanie added enhancement wip rails7.1 rails7.2 labels Oct 4, 2024

jrafanie force-pushed the rails71 branch from 509d1ee to 2a23f67 Compare October 4, 2024 22:04

jrafanie mentioned this pull request Oct 4, 2024

Support rails 7.1 ManageIQ/manageiq#23200

Open

42 tasks

jrafanie commented Oct 5, 2024

View reviewed changes

jrafanie force-pushed the rails71 branch from 2a23f67 to ba2f867 Compare October 10, 2024 17:19

jrafanie changed the title ~~[WIP] Add rails 7.1 and 7.2 support~~ Add rails 7.1 and 7.2 support Oct 10, 2024

jrafanie removed the wip label Oct 10, 2024

jrafanie commented Oct 10, 2024

View reviewed changes

Fryguy reviewed Oct 10, 2024

View reviewed changes

jrafanie commented Oct 10, 2024

View reviewed changes

Fryguy reviewed Oct 10, 2024

View reviewed changes

jrafanie added the wip label Oct 10, 2024

jrafanie changed the title ~~Add rails 7.1 and 7.2 support~~ [WIP]Add rails 7.1 and 7.2 support Oct 10, 2024

jrafanie changed the title ~~[WIP]Add rails 7.1 and 7.2 support~~ [WIP] Add rails 7.1 and 7.2 support Oct 10, 2024

jrafanie force-pushed the rails71 branch from f5f2d41 to 488b6ab Compare October 22, 2024 20:55

jrafanie added 8 commits October 23, 2024 10:47

Support using rails 7.1 and 7.2 versioned gems

07500c8

Add them to the matrix.

Set default_column_serializer to YAML for now for backward compatibility

9ad9001

Prepend serialize in 7.1+ to convert positional arguments to kwargs

3eab6b5

See https://www.github.com/rails/rails/pull/47463

Use the rails 7.1+ serialize interface even for 7.0

320a780

We can intercept serialize for 7.0 and convert kwargs back to the positional class_name_or_coder. Update existing migrations to use rails 7.1 interface.

Behaviour is deprecated/removed in 7.1/7.2, use Behavior

f3d7002

See https://www.github.com/rails/rails/pull/45180

Rails 7.2 refactored schema_migration metadata/context to the pool

16b6d7e

See: https://www.github.com/rails/rails/pull/51162

Bump id_regions to support rails 7.1+

157860a

Move SerializePositionalToKwargsBridge to the plugin itself

a3eb361

jrafanie force-pushed the rails71 branch from 488b6ab to a3eb361 Compare October 23, 2024 14:50

jrafanie changed the title ~~[WIP] Add rails 7.1 and 7.2 support~~ Add rails 7.1 and 7.2 support Oct 23, 2024

jrafanie removed the wip label Oct 23, 2024

kbrock merged commit fe983e4 into ManageIQ:master Oct 25, 2024
11 of 12 checks passed

jrafanie deleted the rails71 branch November 1, 2024 20:28

jrafanie assigned kbrock Nov 1, 2024

jrafanie mentioned this pull request Nov 1, 2024

Use the rails 7.1+ serialize interface ManageIQ/manageiq#23253

Merged

jrafanie mentioned this pull request Nov 1, 2024

Use the rails 7.1+ serialize interface ManageIQ/manageiq-consumption#224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rails 7.1 and 7.2 support #756

Add rails 7.1 and 7.2 support #756

jrafanie commented Oct 4, 2024 •

edited

Loading

jrafanie Oct 5, 2024

jrafanie Oct 7, 2024

jrafanie Oct 7, 2024

Fryguy Oct 10, 2024 •

edited

Loading

kbrock Oct 10, 2024

jrafanie Oct 10, 2024

Fryguy Oct 10, 2024 •

edited

Loading

Fryguy Oct 10, 2024

jrafanie Oct 10, 2024

jrafanie Oct 22, 2024

jrafanie commented Oct 10, 2024

jrafanie Oct 10, 2024 •

edited

Loading

Fryguy Oct 10, 2024

jrafanie Oct 10, 2024

jrafanie Oct 10, 2024

Fryguy Oct 10, 2024

jrafanie Oct 10, 2024

jrafanie commented Oct 23, 2024

Add rails 7.1 and 7.2 support #756

Add rails 7.1 and 7.2 support #756

Conversation

jrafanie commented Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Oct 10, 2024

jrafanie Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Oct 23, 2024

jrafanie commented Oct 4, 2024 •

edited

Loading

Fryguy Oct 10, 2024 •

edited

Loading

Fryguy Oct 10, 2024 •

edited

Loading

jrafanie Oct 10, 2024 •

edited

Loading