search

time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.06k stars 261 forks source link

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

Open jeff-hiner opened 3 months ago

jeff-hiner commented 3 months ago

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 https://github.com/rust-lang/rust-clippy/issues/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 commented 3 months ago

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.