Conversion between Timestamp and chrono::DateTime

[daladim]

These changes enable idiomatic conversions between chrono::DateTime and the protobuf Timestamp.

Such conversions were already provided with objects from std::time, but not with ones from the chrono crate.
Since it adds a dependency, this code is conditionally used, only if feature chrono-conversions is set.

[danburkert]

Can you not use the existing SystemTime conversion? I would have thought chrono could interoperate with the std lib time APIs.

[danburkert]

To add a bit more color, in the medium term I'd like to add options to prost-build to emit std::time::SystemTime fields instead of the wkt Timestamp type, and have a corresponding impl Message for std::time::SystemTime. When that's done this conversion won't be very useful, best I can tell. I'd very much prefer not to have a dependency on chrono for a host of reasons.

[sgg]

@danburkert To clarify, are you saying prost would use std::time::SystemTime as the Rust representation of google.protobuf.Timestamp (as shown below)?

syntax = "proto3";
import "google/protobuf/timestamp.proto";

message Record {
  google.protobuf.Timestamp created_at = 1
}

#[derive(::prost::Message)]
pub struct Record {
  pub created_at: std::time::SystemTime
}

---

FWIW I use chrono::DateTime<Utc> all over the place and I don't find the status quo particularly cumbersome. If let ts = prost_types::Timestamp::from(SystemTime::from(date_time)) becomes let ts = date_time.into() then I'd be content, especially if it keeps chrono out of prost's dependencies.

[danburkert]

@danburkert To clarify, are you saying prost would use std::time::SystemTime as the Rust representation of google.protobuf.Timestamp (as shown below)?

Yes. Half of this is already possible today through a prost_build::Config::extern_paths(".google.protobuf.Timestamp", "std::time::SystemTime"). The only piece that's missing is adding an impl Message for std::time::SystemTime.

[danburkert]

( I think, that's not tested)

[sivakov512]

Any news here? I think it would be cool to have ability for converting between protobuf Timestamp and Chrono DateTime

[argv-minus-one]

Can you not use the existing SystemTime conversion? I would have thought chrono could interoperate with the std lib time APIs.

The resolution and representable range of SystemTime varies by platform. A conversion of prost_types::Timestamp→SystemTime→chrono::NaiveDateTime or vice versa is lossy and/or fallible on anything other than a 64-bit Unix-like system (where prost_types::Timestamp and SystemTime use exactly the same representation of time).

[danburkert]

I haven't looked in chrono in more than a year, but my experience is that chrono's supported range is significantly less than SystemTime, at least on macos and linux.

[danburkert]

Yeah it's not even close: https://github.com/chronotope/chrono/blob/b0f32832a73a45bf89eaeae74e471aa5ee10fad1/src/naive/internals.rs#L24-L28

[argv-minus-one]

Right, but like I said, it varies by platform. macOS and 64-bit Linux use the same time representation as prost_types::Timestamp (64-bit signed seconds and 32-bit nanoseconds), so the conversion is lossless and infallible, but some other platforms' SystemTime is more restricted than both prost_types::Timestamp and chrono:

• SystemTime on most (but not all) 32-bit Unix-like systems has nanosecond resolution but a 32-bit seconds counter, leading to the year 2038 problem.
• Windows SystemTime has 100ns resolution and cannot represent time before 1 January 1601.

[LucioFranco]

I am going to close this PR since it seems like we have consensus that we don't want to include chrono as a dep for prost. I believe the systemtime conversion should work.

[Roms1383]

My question might be naive, but why not just gate chrono under a feature flag?

[LucioFranco]

Totally valid question and I am not 100% sure if we don't want to include it.

There are two reasons 1) the fields of timestamp are public so its quite easy to write your own custom conversion, 2) its more timestamp code to maintain. That said, I would be open to accepting this now I had a change of heart :)

[LucioFranco]

I would like to consider a bit what @danburkert said above as well before we introduce chrono. I am not convinced either way and need to spend sometime reading up on timestamps (super fun stuff).

[Roms1383]

Great @LucioFranco, IMHO it's part of the additions that would make prost more convenient to work with.
I believe the question I posted yesterday (#537) is distantly related to this one too :)

[daladim]

(Note that I've fixed the conflicts in case you'd want to merge this MR)

[LucioFranco]

I've thought about this a bit more and now believe this feature should be added to chrono behind a prost feature flag as I think this fits similar to how it works with serde. Going to close this PR thanks everyone for the feedback :)

[djc]

@LucioFranco I respectfully disagree with your judgement on this. I feel like best practice in the ecosystem is that higher-level crates (more complex API/functionality, more semver-breaking releases) should hold the integration with lower-level crates (simpler API/functionality, fewer breaking releases). In this case, it seems clear to me that prost is higher-level than chrono, so if the integration will live in either one of our crates it should be in the prost one IMO.

[parrotmac]

As a user of both chrono and prost I'd like to advocate for getting a conversion between the types added to one of the projects. FWIW I definitely expected this to be a toggle on prost rather than chrono -- perhaps because of the difference in size/ubiquity of the projects (prost! vs chrono). In any case, it doesn't appear to be a huge patch but it would definitely be appreciated.