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

Restore ActiveJob serialization for String subclasses without custom serializers #50122

Merged

Conversation

octokatherine
Copy link
Contributor

Motivation / Background

This Pull Request has been created because #50090 broke serialization of String subclasses that don't have serializers, like ActiveSupport::SafeBuffer.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the activejob label Nov 20, 2023
rails#50090 broke serialization of String
subclasses that don't have serializers, like ActiveSupport::SafeBuffer.

Co-authored-by: John Hawthorn <john@hawthorn.email>
@octokatherine octokatherine force-pushed the job-arg-string-subclass-serialize branch from 4c00051 to e57e462 Compare November 20, 2023 23:43
@jhawthorn
Copy link
Member

cc @fatkodima @byroot

This fixes serialization for String subclasses which don't have serializers. I think this will be pretty common for SafeBuffer, but likely also other strings we don't know about.

Possibly we could deprecate this and remove it in a future version, but we would have to do so very carefully. If we want to make a serializer for SafeBuffer, it needs to be enable-able by a flag just as we do for BigDecimal so that it can be deployed safely.

I'm also not fully convinced we should deprecate serializing string subclasses, we currently allow serializing Hash and Array subclasses, and it would probably be best to be consistent.

@jhawthorn jhawthorn merged commit 3d2e960 into rails:main Nov 20, 2023
4 checks passed
@byroot
Copy link
Member

byroot commented Nov 21, 2023

Works for me. As long as they are serialized as bare strings, I have no concerns.

@jarenas9539
Copy link

jarenas9539 commented Nov 21, 2023

Couldn't previous if statement argument.class == String be removed as it's already scoped on the rescue clause? 🤔

jarenas9539 added a commit to jarenas9539/rails that referenced this pull request Nov 21, 2023
… of String

After changes from rails#50090 and rails#50122, a redundant if statement remains on Arguments#serialize on String related classes serialization. When adding the begin/rescue statement on the else clause the if clause began to be scoped on the rescue clause as StringSerializer is not defined and will throw SerializationError. As if clause and rescue clause share same result, the if clause started to be redundant.
@northeastprince
Copy link
Contributor

@jhawthorn could this be backported?

@byroot
Copy link
Member

byroot commented Nov 30, 2023

The bug it is fixing wasn't present on 7.1 in the first place. Unless I'm missing something?

So no reason to backport it.

@northeastprince
Copy link
Contributor

@byroot
Copy link
Member

byroot commented Dec 1, 2023

So it's #50090 you want backported?

@northeastprince
Copy link
Contributor

Yeah

@byroot
Copy link
Member

byroot commented Dec 2, 2023

Backported to 7-1-stable as 49ac7e5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants