Deserializing using serde deserialize does not work for String timestamp

[Gricha]

Hello!

I was curious whether it's a bug, a design choice, or my incorrect usage of the framework and would definitely appreciate any help and guidance on this one!

I'm using chrono to support deserialization of time formats in my parsing layer. One of the APIs I am currently using sends me timestamps as string. Example

{
  "timestamp": "1512954499"
}

I'd like to represent said JSON in my struct as:

#[derive(Debug, Deserialize)]
pub struct Data {
    timestamp: DateTime<Utc>,
}

I noticed that there is an attempt to support such situation by using the following function:
https://github.com/chronotope/chrono/blob/master/src/datetime.rs#L750

Unfortunately I was not able to get that working with strings. The following implementation of struct does not end up parsing the JSON properly:

#[derive(Debug, Deserialize)]
pub struct Data {
    #[serde(deserialize_with = "from_ts")]
    timestamp: DateTime<Utc>,
}

I was curious whether it's a bug or maybe my setup is plain wrong?

The one thing that I've noticed is that this Deserialize function directly uses SecondsTimestampVisitor to parse out the timestamps. That Visitor though, if I'm reading the code correctly, does not implement either visit_str or visit_string. I'm not sure if that's intended.

Sorry if it turns out to be a silly question, I'm still quite new to Rust! :)

[quodlibetor]

The good news: you are correct about the ts_seconds module only working with integers, and you're also correct about all the reasons!

The tl;dr: I recommend that you copy/paste the serde::ts_seconds module out of chrono and into your own code, with your required modifications, but if you have evidence of this being a common pattern I'm happy to reconsider.

I think that string timestamps are far enough away from a common occurrence that I'm hesitant to add code to support them to chrono.

If you copy/paste the ts_seconds module (from what you've said you really only need the deserialize portions of it, I think) into your own code and add a visit_str method that looks sort of like:

// untested!
fn visit_str<E>(self, value: &str) -> Result<DateTime<FixedOffset>, E>
    where E: de::Error
{
    let ts: i64 = value.parse()
        .map_err(|e| E::custom(format!("Unable to parse timestamp as int: {}", e)))?;
    self.visit_i64(ts)
}

And then call that from your copied in deserialize function, you should be golden. Or at least a lighter shade than "burnt".

[Gricha]

Thanks for quick response @quodlibetor !

Awesome, that sounds great. Yeah I have no strong sense that sending timestamps as strings is something common, at very least I've stumbled upon them for the first time. But I imagine you can best decide whether it's useful to support them long term based on whether there are explicit asks for it.

Copying important bits and adding visit_str sounds good to me! :) I'll test it right away, thank you!

[quodlibetor]

Let me know how it goes, and if anything obvious comes up that it seems like chrono should make easier for this use case.

[Gricha]

Sounds great, thank you!

Just tested it, works like a charm.

For future reference and if anyone needs this, the gist is very simple:
https://gist.github.com/Gricha/21d58b59fb65fb4eeb5f0af8640ca4a2

It just copies ts_seconds excluding some serialization bits and adds visit_str to Visitor.

[quodlibetor]

Thanks!

[heretic13]

Hello.
This is not such a rare case.
Some online trading exchanges use strings to transmit timestamps.

Only in my case there were not seconds but milliseconds ("ts_milliseconds"). I don't know Rust that well, but maybe there is a way to cascade serde handlers?

Something like String -> @hanlder-> Integer -> @ts_milliseconds-> DateTime.

Something like
#[serde (with = "string_to_i64, ts_seconds")] time: DateTime <Utc>

???

[sinking-point]

Yeah. String timestamps aren't that uncommon. Even Google uses them, e.g. in RTDN.

[djc]

I'm open to supporting this, please submit a PR.

[sinking-point]

On it.

[sinking-point]

Another solution we could consider is scrapping the serde timestamp code in this crate and instead telling people to use serde_with. They have an elegant solution using their serde_as macro:

#[serde_as]
#[derive(Deserialize, Serialize)]
struct Timestamps {
    #[serde_as(as = "TimestampSeconds<i64>")]
    dt_i64: DateTime<Utc>,
    #[serde_as(as = "TimestampSeconds<f64>")]
    dt_f64: DateTime<Local>,
    #[serde_as(as = "TimestampSeconds<String>")]
    dt_string: DateTime<Utc>,
};

Would that crate be suitable for all current applications of timestampt serialisation? In terms of:

• Environments it can target
• Binary size
• Performance

[sinking-point]

I propose we close my current PR and instead:

1. Deprecate existing timestamp ser/de modules
2. Update documentation to suggest people use the serde_with crate instead

Any objections?

CC @pitdicker @djc

[jonasbb]

I don't think there is a need to deprecate the existing serde modules. I see serde_with more as an addition than a replacement for other crates. Code can go there if the original crates would rather not carry it or to integrate with the serde_as macro. It is easier and more discoverable if string-based timestamps are directly supported here. Some people also just don't like adding extra dependencies.

There are some advantages of the Timestamp* types from serde_with though. So a link to them in the documentation might not be bad. They already have support for i64, f64, and String, optional fractions, and support deserializing only from one type or adaptive to the data (the strictness parameter). Nesting support, like Option<DateTime<Utc>> or Vec<NaiveDateTime>, which chrono only supports partially with the ts_*_option modules.

[sinking-point]

Hi @jonasbb , thanks for joining the discussion and for your work on the serde_with crate.

The issue is that the way timestamp ser/de is done is very repetitive and verbose, and neither modular nor extensible. Each of the ts_* modules is implemented separately, with its own (nearly identical) documentation. Thus, my PR adding support for string timestamps needed to add 16 new modules: (Timestamp, NaiveTimestamp) * (normal, optional) * (nano-, micro-, milli-, seconds).

In my view, one of the following must be true:

1. We are fine with the repetitive nature of the current timestamp ser/de implementation, in which case my PR should be merged.
2. The repetitive implementation is unacceptable, but we need timestamp ser/de support to be in this crate. We should reimplement timestamp ser/de in a more modular way, which may end up looking very much like the serde_with solution.
3. The repetitive implementation is unacceptable, and we can rely on serde_with for equivalent functionality. We should deprecate and later remove the timestamp ser/de modules in this crate, updating the documentation to point people to the relevant sede_with docs.

If I detach myself from the pain of my work going to waste, I prefer option 3. The problem is already solved very satisfactorily in serde_with and it seems wasteful to reimplement it in chrono. Option 1 is also acceptable to me, though more work to maintain in the long run.

[djc]

None of these must be true because they're extreme positions to take. I find the repetitive implementations unattractive but not unacceptable -- clearly having less repetition is better but some repetition (in code that doesn't need many changes over time) isn't a big problem in my mind. On the need for supporting different kinds of timestamp, I feel that providing these in this crate would make life easier for downstream users, but it's clearly not a must-have since serde has enough affordances that custom implementations and implementations from third-party crates are an option.

As such, if you're satisfied with using serde-with, just do that. If someone wants to come along and spend the time to get it right within this crate, that would be nice.