[PM-28361] feat: flexible timestamp serde#112
[PM-28361] feat: flexible timestamp serde#112Autoparallel wants to merge 3 commits intobitwarden:mainfrom
Conversation
|
|
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
|
Hey @Autoparallel, Could you provide some details about how you are using the library with the Apple APIs? I don't believe I've seen On our side we're using it in our iOS application with the following relevant code snippets:
We do have to explicitly tell the json encoder to use |
Yeah, this was my issue too. You can definitely specify the JSON encoder, but by default iOS is providing ISO8601. It seemed to me like having flexible deserialization in this library would make developer experience a bit smoother while any time it's serialized we adhere to the spec precisely. Similarly, using Thanks for the quick reply by the way, I appreciate it! :) |
|
Still a bit confused about the ISO8601 strings. While it's possible for them to return ISO dates the default is Is it possible you might be re-using an existing Encoder/Decoder that has this manually configured to ISO? FYI, I submitted an Apple Feedback (FB21076950) to add a note to the apple documentation about I do agree that graceful parsing is possible. My concerns is that the import would now be inconsistent with the export flows, and you would still have to configure the JSONDecoder to handle the dates in the export flow. Is it actually worth gracefully handling this for importers if it means it blows up on exports instead? |
That's a good point. Is your preference to stick solely with a |
From a rust consumer perspective I think moving over to @Progdrasil do you have any opinion on moving timestamps over to |
|
I added a section about compatibility with apple to the credential-exchnge-format readme, #114. |
I suspect as time goes on that we'll need to adapt the deserialization as much as possible to be flexible for spec non-compliant implementations. So long as the output of the library is spec compliant when serializing I think this is fine. It's also the same route we take for 1password/passkey-rs. As for changing the type itself vs using a u64 repr, I think there's value in using a smarter type rather than a primitive here. So I'm okay with this breaking change. |
|
This is getting stale -- not sure where the decision lies but I just wanted to ping here to see if there's still interest! |
|
Hey @Autoparallel, Apologies for not getting back earlier. I believe we're interested in this, I just need to find some time to review it. We have some broader issues with unexpected data (documented in #122 and #123). But I don't think those larger efforts should block this. |
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
I think this looks good overall. There are a few things we should resolve.
Would you mind signing the CLA and also add a note to CHANGELOG.md, thanks!
| /// the provider encounters this [Item]. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub modified_at: Option<u64>, | ||
| pub modified_at: Option<DateTime<Utc>>, |
There was a problem hiding this comment.
question: Should this also use with = "timestamp::option"?
| use chrono::{DateTime, TimeZone, Utc}; | ||
| use serde::{Deserialize, Deserializer, Serializer}; | ||
|
|
||
| /// Serializes a [`DateTime<Utc>`] as a UNIX timestamp (i64). |
There was a problem hiding this comment.
issue: I think we need to use u64 here. The CX specification specifies machine readable datetimes as uint .size 8 and I don't believe it's valid to serialize as signed integers.
There was a problem hiding this comment.
Yes, that's definitely right. i64 would cause errors. Good catch!
| pub mod option { | ||
| //! Serialization and deserialization functions for `Option<DateTime<Utc>>`. | ||
| //! | ||
| //! This module provides the same flexible deserialization as the parent module, | ||
| //! but for optional timestamp fields. | ||
|
|
||
| use super::{DateTime, Deserialize, Deserializer, Serializer, TimeZone, Utc}; | ||
|
|
||
| /// Serializes an `Option<DateTime<Utc>>` as either a UNIX timestamp (i64) or null. | ||
| /// | ||
| /// This function is intended to be used with serde's `#[serde(with = "...")]` attribute. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if the serializer fails to serialize the timestamp value. | ||
| #[allow(clippy::ref_option)] | ||
| pub fn serialize<S>(date: &Option<DateTime<Utc>>, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| match date { | ||
| Some(dt) => serializer.serialize_some(&dt.timestamp()), | ||
| None => serializer.serialize_none(), | ||
| } | ||
| } | ||
|
|
||
| /// Deserializes an `Option<DateTime<Utc>>` from either a UNIX timestamp, an ISO8601 string, | ||
| /// or null. | ||
| /// | ||
| /// This function is intended to be used with serde's `#[serde(with = "...")]` attribute. | ||
| /// | ||
| /// # Accepted Formats | ||
| /// | ||
| /// - UNIX timestamp as i64 or u64 (seconds since Unix epoch) | ||
| /// - ISO8601/RFC3339 string (e.g., `"2023-11-18T10:30:00Z"`) | ||
| /// - null | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if: | ||
| /// - The timestamp value is invalid or out of range | ||
| /// - The ISO8601 string cannot be parsed | ||
| /// - The input is neither a number, string, nor null | ||
| pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<DateTime<Utc>>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| Option::<serde_json::Value>::deserialize(deserializer)? | ||
| .map(|v| { | ||
| v.as_i64().map_or_else( | ||
| || { | ||
| v.as_str().map_or_else( | ||
| || Err(serde::de::Error::custom("expected number or string")), | ||
| |s| { | ||
| s.parse::<DateTime<Utc>>().map_err(|e| { | ||
| serde::de::Error::custom(format!("invalid ISO8601: {e}")) | ||
| }) | ||
| }, | ||
| ) | ||
| }, | ||
| |num| { | ||
| Utc.timestamp_opt(num, 0) | ||
| .single() | ||
| .ok_or_else(|| serde::de::Error::custom("invalid timestamp")) | ||
| }, | ||
| ) | ||
| }) | ||
| .transpose() | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: I believe we can use a helper struct here which would allow re-use of the previous implementation which ensures consistency.
#[allow(clippy::ref_option)]
pub fn serialize<S>(date: &Option<DateTime<Utc>>, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
#[derive(Serialize)]
struct Helper(#[serde(serialize_with = "super::serialize")] DateTime<Utc>);
date.as_ref().map(|dt| Helper(*dt)).serialize(serializer)
}
pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<DateTime<Utc>>, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
struct Helper(#[serde(deserialize_with = "super::deserialize")] DateTime<Utc>);
Option::<Helper>::deserialize(deserializer).map(|opt| opt.map(|h| h.0))
}
Yes, I can do that. It may take me a few days as I have some other priorities right now. |
5 participants
🎟️ Tracking
Adding flexible timestamp deserialization to support both UNIX timestamps and ISO8601 formats in the Credential Exchange Format, addressing compatibility with iOS native credential exchange protocol which outputs ISO8601 timestamps.
📔 Objective
This PR adds flexible timestamp deserialization support to the
credential-exchange-formatcrate. The implementation allows the format to accept timestamps in two formats during deserialization:"2023-11-18T10:30:00Z"All timestamps are consistently serialized as UNIX timestamps (i64) to maintain compatibility with the CXF standard.
Motivation: iOS's native credential exchange protocol (new in iOS 26) outputs timestamps in ISO8601 format. This change provides seamless interoperability without requiring format conversion at the application layer, while maintaining backward compatibility with existing UNIX timestamp-based implementations.
Key changes:
timestampmodule with custom serde serialization/deserialization functionsHeader::timestamp,Collection::creation_at,Collection::modified_at,Item::creation_at, andItem::modified_atfieldschronowithserdefeature)⏰ Reminders before review
🦮 Reviewer guidelines
Areas to focus on:
timestampmodule0.4.0via Rust semverTesting notes: