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

Fix AS::Cache 7.1 format to properly compress bare strings #48660

Merged
merged 1 commit into from Jul 5, 2023

Conversation

casperisfine
Copy link
Contributor

The bare string and compression features weren't working properly together.

I discovered this while trying to fix deprecation warnings.

FYI: @jonathanhefner

The bare string and compression features weren't working properly
together.

I discovered this while trying to fix deprecation warnings.
@byroot byroot merged commit 495cbe9 into rails:main Jul 5, 2023
9 checks passed
@jonathanhefner
Copy link
Member

Thank you for fixing this! ❤️

Something to note: this will prefix bare strings with MARK_UNCOMPRESSED or MARK_COMPRESSED when using the 7.1 coder. My original intent was to dump bare strings independently of the coder, using their own dedicated prefixes. Obviously, I messed that up with Marshal71WithFallback#dump_compressed, but that is how it works with the MessagePackWithFallback coder.

This is also fixed in #48451 since bare strings are actually dumped independently of the coder (aka serializer). #48451 offloads this logic to SerializerAdapter so that the serializer is only concerned with serialization and versioning, and the compressor is only concerned with compression and versioning. For example, see Marshal71WithFallback in #48451

@byroot
Copy link
Member

byroot commented Jul 5, 2023

Something to note: this will prefix bare strings with MARK_UNCOMPRESSED or MARK_COMPRESSED when using the 7.1 coder.

Yes, it's not ideal. As mentioned in #48451 the prefix, expires_at and even version shouldn't be compressed, only the payload should.

I really need to wrap my head around #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.

None yet

3 participants