Skip to content

Support :message_pack as message serializer - #47964

Merged
jonathanhefner merged 1 commit into
rails:mainfrom
jonathanhefner:message_pack-message-serializer
May 8, 2023
Merged

Support :message_pack as message serializer #47964
jonathanhefner merged 1 commit into
rails:mainfrom
jonathanhefner:message_pack-message-serializer

Conversation

@jonathanhefner

@jonathanhefner jonathanhefner commented Apr 17, 2023

Copy link
Copy Markdown
Member

This commit adds support for :message_pack as a serializer for MessageEncryptor and MessageVerifier, and, consequently, as an option for config.active_support.message_serializer.

The :message_pack serializer is implemented via ActiveSupport::Messages::SerializerWithFallback and can fall back to deserializing with AS::JSON. Additionally, the :marshal, :json, and :json_allow_marshal serializers can now fall back to deserializing with AS::MessagePack.

This commit also adds support for :message_pack_allow_marshal as a serializer, which can fall back to deserializing with Marshal as well as AS::JSON.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Rubocop flags this with Performance/StringInclude: Use String#include? instead of a regex match with literal-only pattern, but format is a symbol, not a string.

I could work around using format.to_s.include?("message_pack"), but I'm more inclined to disable the Performance/StringInclude rule, since this is valid code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

format.start_with?("message_pack") could do the trick.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or just if format == :message_pack || format == :message_pack_allow_marshal. A bit longer but very explicit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My thinking was to require when format included any mention of message_pack because tests may not catch a missing require (due to previous tests performing the require). So if some :message_pack_* or :*_message_pack variant is added in the future, it would be covered. (I admit that may be overkill though.)

@jonathanhefner
jonathanhefner force-pushed the message_pack-message-serializer branch from 8e19f9d to 425d6a5 Compare May 1, 2023 19:49
@jonathanhefner
jonathanhefner force-pushed the message_pack-message-serializer branch 2 times, most recently from ca25e3d to 0c58701 Compare May 8, 2023 19:34
This commit adds support for `:message_pack` as a serializer for
`MessageEncryptor` and `MessageVerifier`, and, consequently, as an
option for `config.active_support.message_serializer`.

The `:message_pack` serializer is implemented via
`ActiveSupport::Messages::SerializerWithFallback` and can fall back to
deserializing with `AS::JSON`.  Additionally, the `:marshal`, `:json`,
and `:json_allow_marshal` serializers can now fall back to deserializing
with `AS::MessagePack`.

This commit also adds support for `:message_pack_allow_marshal` as a
serializer, which can fall back to deserializing with `Marshal` as well
as `AS::JSON`.
@jonathanhefner
jonathanhefner force-pushed the message_pack-message-serializer branch from 0c58701 to af6d835 Compare May 8, 2023 19:39
@jonathanhefner
jonathanhefner marked this pull request as ready for review May 8, 2023 19:59
@jonathanhefner
jonathanhefner merged commit 6b52c1b into rails:main May 8, 2023
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Sep 10, 2023
This covers rails#47770, rails#47964, rails#48103, and rails#48104, plus the follow-up
changes to `ActiveSupport::Cache::Store` options from rails#48449 and rails#48451.
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.

3 participants