Use the newer serialize signature of Rails 7.1 if available
#776
Conversation
casperisfine
requested review from
adrianna-chang-shopify,
etiennebarrie and
gmcgibbon
app/models/maintenance_tasks/run.rb
Outdated
| @@ -46,8 +46,13 @@ class Run < ApplicationRecord | |||
|
|
|||
| attr_readonly :task_name | |||
|
|
|||
| serialize :backtrace | |||
| serialize :arguments, JSON | |||
| if respond_to?(:default_column_serializer) # Rails 7.1+ | |||
There was a problem hiding this comment.
I'm surprised this is defined on ActiveRecord::Base, I thought configurations were being moved to ActiveRecord. rails/rails#42445
It's not a mattr_accessor, but still confusing and if it's kept at the ActiveRecord::Base level, we could also just set the value, as it wouldn't be global anyway.
self.default_column_serializer = YAML if respond_to?(:default_column_serializer=)Also the accessor on ActiveRecord::Base is not documented, only as a configuration on config.active_record.default_column_serializer, but these never explicitly say where they're being copied.
| serialize :arguments, JSON | ||
| if respond_to?(:default_column_serializer) # Rails 7.1+ | ||
| serialize :backtrace, coder: YAML | ||
| serialize :arguments, coder: JSON |
There was a problem hiding this comment.
Maybe if we're calling the method differently based on its parameters, we might as well use that as to make the distinction:
if method(:serialize).parameters.include?([:key, :coder])or even just call it and rescue ArgumentError.
Or maybe just a Active Record version check?
There was a problem hiding this comment.
I'm fine with either. Let me know which you prefer.
There was a problem hiding this comment.
Version check is the safest and the easiest to grep for down the line when we want to remove that?
There was a problem hiding this comment.
Version check sounds good to me as well 👍
| serialize :arguments, JSON | ||
| if respond_to?(:default_column_serializer) # Rails 7.1+ | ||
| serialize :backtrace, coder: YAML | ||
| serialize :arguments, coder: JSON |
There was a problem hiding this comment.
Version check sounds good to me as well 👍
casperisfine
force-pushed
the
default-serializers
branch
from
3892d7e
to
27a94d9
Compare
app/models/maintenance_tasks/run.rb
Outdated
| @@ -46,8 +46,13 @@ class Run < ApplicationRecord | |||
|
|
|||
| attr_readonly :task_name | |||
|
|
|||
| serialize :backtrace | |||
| serialize :arguments, JSON | |||
| if Rails.gem_version >= Gem::Version.new("7.1") | |||
There was a problem hiding this comment.
We need to check against alpha to also include the current main.
| if Rails.gem_version >= Gem::Version.new("7.1") | |
| if Rails.gem_version >= Gem::Version.new("7.1.alpha") |
There was a problem hiding this comment.
Duh, I always forget that.
On 7.1 serialize properly split `coder` and `type` arguments instead of taking a single positional argument and try to figure out which is which. While we're at it, we can explictly define we're using YAML here instead of relying on the default that is now configurable.
casperisfine
force-pushed
the
default-serializers
branch
from
27a94d9
to
e2fd841
Compare
etiennebarrie
changed the title
serialize signature if availableserialize signature of Rails 7.1 if available
Ref: rails/rails#47463
On 7.1 serialize properly split
coderandtypearguments instead of taking a single positional argument and try to figure out which is which.While we're at it, we can explictly define we're using YAML here instead of relying on the default that is now configurable.