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

Default impl Deserialize/impl Serialize for time::OffsetDateTime is a footgun #672

Open
jeff-hiner opened this issue Mar 28, 2024 · 1 comment
Labels
A-third-party Area: implementations of traits from other crates C-enhancement Category: an enhancement with existing code

Comments

@jeff-hiner
Copy link

jeff-hiner commented Mar 28, 2024

A majority of folks using derives for Serialize/Deserialize are likely trying to write REST APIs for json output. There exists a wonderful set of deserializers for RFC3339, ISO 8601, and RFC2822. They work great! Unfortunately if the serde feature is enabled, time will happily provide a default Serialize/Deserialize pair that doesn't follow any of these standards. I can tweak these formats with serde-human-readable, but not specify one of the standards as a default. There doesn't appear to be a way to turn the default impls off.

To me this is a footgun, because it means that if I forget to annotate an instance with #[serde(with = "time::serde::rfc3339")] the dependent code will compile without errors or warnings, and serialize/deserialize into formats I don't expect or intend. The generated code will choke at runtime trying to parse RFC3339 (what most folks use for interop in json these days).

I've tried adding time::OffsetDateTime::deserialize to disallowed-methods in .clippy.toml but Clippy isn't picking it up. <time::OffsetDateTime as serde::de::Deserialize>::deserialize also does not work, likely because of rust-lang/rust-clippy#8581

It feels like we should feature-flag the "default" serde implementation (and potentially add feature flags to default RFC3339 or each of the others), but that's obviously a breaking change. Thoughts?

@jhpratt
Copy link
Member

jhpratt commented Mar 28, 2024

As you noted, changing the default would be a breaking change. However, it would be permissible to add support for deserializing additional formats, as that's backward compatible. Ultimately, I think what would best serve your purpose would be ensuring the linked clippy issue is resolved.

@jhpratt jhpratt added C-enhancement Category: an enhancement with existing code A-third-party Area: implementations of traits from other crates labels Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-third-party Area: implementations of traits from other crates C-enhancement Category: an enhancement with existing code
Projects
None yet
Development

No branches or pull requests

2 participants