Bad handling of malformed dates coming from BSky

[merickson]

On occasion, sometimes dates are incorrectly formatted coming from the BlueSky API.

For example, take this BlueSky post (https://bsky.app/profile/did:plc:xplfowvb5fkrduzpw4s6fsrv/post/3k4jng4uxmv2d):

Object {
        "$type": String("app.bsky.feed.post"),
        "createdAt": String("2023-08-09T09:44:46.189371"),
        "embed": Object {
            "$type": String("app.bsky.embed.images"),
            "images": Array [
                Object {
                    "alt": String(""),
                    "image": Object {
                        "cid": String("bafkreiea22et5bwq4ulvc5r6etbqstqlx5onccnfeblve4zwdz62grbdx4"),
                        "mimeType": String("image/jpeg"),
                    },
                },
            ],
        },
        "text": String("🇧🇷Choró-boi\n🌎Taraba major\n\nO choró-boi é uma ave  passeriforme da família Thamnophilidae"),
    }

If you'll notice, the createdAt timestamp does not match the regular expression ensuring conformance to the union of RFC3339 and ISO8601 formatting in string.rs:

atrium/atrium-api/src/types/string.rs

Line 236 in b48810f

.get_or_init(|| Regex::new(r"^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?(Z|(\+[0-9]{2}|\-[0-9][1-9]):[0-9]{2})$").unwrap())

Furthermore, because of the use of unwrap() when using RecordData::try_from_unknown() the error isn't bubbled up:

atrium/atrium-api/src/types.rs

Line 279 in b48810f

Ok(serde_json::from_slice(&json).unwrap())

I'm not the best Rusteacean at present, but this is what I've come up with for a workaround so far. Assuming r is an individual post coming out of BskyAgent.api.app.bsky.feed.get_posts():

let inter_json = serde_json::to_vec(&r.record).unwrap();
let record: Object<RecordData> = match serde_json::from_slice(inter_json.as_slice()) {
    Ok(o) => o,
    Err(e) => {
        warn!("Got error {}", e);
        warn!("Attempting to fix date...");               
        
        let mut inter_obj: serde_json::value::Value = serde_json::from_slice(inter_json.as_slice()).unwrap();
        let date_str = inter_obj["createdAt"].as_str().unwrap();
        let new_date = chrono::NaiveDateTime::parse_from_str(date_str, "%Y-%m-%dT%H:%M:%S%.f").unwrap();
        debug!("new_date: {}", new_date);
        inter_obj["createdAt"] = serde_json::to_value(new_date.format("%Y-%m-%dT%H:%M:%S%.3fZ").to_string()).unwrap();
        debug!("{:#?}", inter_obj);
        serde_json::from_slice(serde_json::to_vec(&inter_obj).unwrap().as_slice()).unwrap()
    }
};

When I have a little more time I'm happy to contribute to make the fix in the library where appropriate but I wanted to make sure this was logged for future reference.

[sugyan]

Thank you for the detailed bug report! I wasn't aware of this issue with malformed dates coming from the BlueSky API, so I really appreciate you taking the time to document it with examples.

I've created PR #320 which addresses the error handling part of this issue by replacing unwrap() with proper error propagation in try_from_unknown. This should make it easier to catch and debug these kinds of errors, but as you correctly noted, it doesn't solve the underlying problem - try_from_unknown will still fail when encountering malformed dates.

Your workaround is quite clever! I tried implementing something similar and ended up with code like this:

let post =
    match atrium_api::app::bsky::feed::post::Record::try_from_unknown(record.value.clone()) {
        Ok(post) => post,
        Err(atrium_api::error::Error::SerdeJson(_)) => {
            println!("Converting to post failed, trying to fix createdAt field");
            let mut value = record.data.value;
            if let Unknown::Object(map) = &mut value {
                for (key, value) in map.iter_mut() {
                    if key == "createdAt" {
                        if let Ipld::String(s) = value.deref_mut() {
                            println!("Fixing createdAt from {s} to {s}Z");
                            *value = DataModel::try_from(Ipld::String(format!("{s}Z")))?;
                        }
                    }
                }
            }
            atrium_api::app::bsky::feed::post::Record::try_from_unknown(value)?
        }
        Err(e) => {
            panic!("unexpected error: {e}");
        }
    };

While part of me would like to handle timezone-less dates more gracefully, I believe that since this library aims to conform to the ATProto specification, having conversion errors for dates without proper timezone information is actually the correct and strict behavior.

[merickson]

Thank you on better error propogation in try_from_unknown!

Respectfully, I disagree on sticking only to the "strict" behavior of the defined protocol, seeing that the de-facto correct implementation (BlueSky) doesn't adhere to it completely. Based on a quick reading of the first-party Go implementation, I think doing something similar as in their implementation of datetime.go would be useful: https://github.com/bluesky-social/indigo/blob/0f3058806983bc7de7bb3ab92294feff29a829bc/atproto/syntax/datetime.go#L64

Also referencing the Go implementation, their lexicon validation has a switch to allow use of the lenient datetime parser: https://github.com/bluesky-social/indigo/blob/0f3058806983bc7de7bb3ab92294feff29a829bc/atproto/lexicon/language.go#L608

In looking at this, I propose:

1. Implementation of a similarly lenient parser
2. Implementing a similar flag in try_from_unknown() to use the lenient datetime parser.
3. Ensuring bad regex validation raises a specific error (that'll pop out due to fix: Improve error handling in types conversion #320 , thank you!)

That would let applications be strict-by-default, while letting applications catch the error and try again with leniency.

I also recognize that this approach would require some foolery in the implementation of Datetime to get that flag from try_from_unknown() into from_str() and don't have any better answers at this moment on a lazy Sunday morning 😄

[sugyan]

Thank you for the thoughtful proposal and insights! I really appreciate you taking the time to research the official implementations and provide concrete suggestions.

You're absolutely right - I wasn't aware that the official Go implementation and other first-party implementations already have lenient datetime parsers. That's a compelling argument for providing similar functionality in the Rust implementation.

I think Rust's feature flag system could be a great solution for this. We could add a feature flag (something like lenient-datetime) that, when enabled, switches the DateTime's FromStr implementation to a more lenient version similar to what you've shown in the Go code. This would:

• Maintain backward compatibility (strict parsing by default)
• Allow users to opt into lenient parsing when needed
• Provide zero runtime overhead since it's a compile-time decision

What do you think about this approach? It would let applications choose the behavior that works best for their use case while keeping the library's default behavior compliant with the spec.

[merickson]

If you notice, the default behavior of the Go library is strict- and as a consumer of atrium I'd prefer a runtime option instead of a feature (allowing me to handle cases where I absolutely don't want malformed dates- like SENDING data, alongside cases where I do, like receiving it)

If I'm the only one hitting this for now I'm down to take a crack at solving it properly soon and submitting a PR.

[sugyan]

I see what you mean - a runtime option would be much more flexible than a feature flag. I hadn't considered that the same application might need different behaviors for sending vs receiving data, and feature flags would indeed lock that behavior at compile time.

A runtime option would be the better solution. If you're willing to work on a PR for this, I'd be happy to review it properly. Thank you for taking the initiative on this!