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 YAML serialization #508

Merged
merged 1 commit into from May 19, 2023
Merged

Conversation

casperisfine
Copy link
Contributor

Ref: #486

Following the introduction of NONE flags, Addressable::URI instance can no longer be correctly serialized with YAML.

It appear to work, but NONE is serialized as !ruby/object {}, so when the YAML is parsed back, the attributes are assigned a distinct anonymous object.

FYI: @tenderlove

@tenderlove
Copy link
Contributor

I'd need to examine the values, but maybe we could use a symbol or number instead of a singleton object instance? Then we don't need to implement the yaml coder stuff

@casperisfine
Copy link
Contributor Author

Then we don't need to implement the yaml coder stuff

Yes and no. It would work for newly serialized objects, however if you have some serialized Addressable::URI stored somewhere (e.g. in cache or database) you still need this to be able to properly deserialize these old ones.

Ref: sporkmonger#486

Following the introduction of `NONE` flags, `Addressable::URI`
instance can no longer be correctly serialized with YAML.

It appear to work, but `NONE` is serialized as `!ruby/object {}`,
so when the YAML is parsed back, the attributes are assigned a
distinct anonymous object.
@dentarg
Copy link
Collaborator

dentarg commented May 9, 2023

Is #509 needed if this is merged?

@casperisfine
Copy link
Contributor Author

Is #509 needed if this is merged?

Not strictly, but I think it would be desireable. #509 add backward compatibility, allowing processes running older versions of assressable to deserialize YAML generated with newer version, which is important to allow a rollout without downtime.

@dentarg
Copy link
Collaborator

dentarg commented May 10, 2023

@tenderlove did you have any other idea or are you okay with this?

@tenderlove
Copy link
Contributor

@dentarg I don't have any better ideas, I think we should merge this 😅

@dentarg dentarg merged commit 736c42a into sporkmonger:main May 19, 2023
33 checks passed
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jul 6, 2023
This help treating caches entries as expandable.

Because Marshal will hapily serialize almost everything, It's not uncommon to
inadvertently cache a class that's not particularly stable, and cause deserialization
errors on rollout when the implementation changes.

E.g. sporkmonger/addressable#508

With this change, in case of such event, the hit rate will suffer for a
bit, but the application won't return 500s responses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants