search

graphql-rust / juniper

GraphQL server library for Rust
Other
5.72k stars 425 forks source link

Provide support for Jiff date/time types #1270

Closed sgoll closed 2 months ago

sgoll commented 3 months ago

Description

This is a proposal to add first-class support for the data types from Jiff. First-class support is necessary, and an external crate would be much more inconvenient to use, because of Rust's orphan rule as outlined in https://github.com/graphql-rust/juniper/issues/87#issuecomment-325918618.

This should cover the following date/time types:

Timestamps

[^1]: This always includes the time zone specifier and uses RFC ~8536~ 9557. This format seems not widely used yet. It is also not a valid input for GraphQL scalar DateTime, therefore we serialize it as the GraphQL scalar ZonedDateTime which has not been stabilized, or even defined elsewhere, yet.

Wall-clocked

Durations

Time zones

Where types overlap, the resulting serialization would be identical to the one for the chrono and time crates, with the difference of using GraphQL scalar LocalDate instead of Date.[^2]

[^2]: LocalDate seems to be the more appropriate type. I am not sure why chrono and time integration chose to use Date instead.

I am aware that Jiff is a very new library but I think it has great potential in the ecosystem. Having dealt with date/time intricacies in several projects, I regard it as one of the top-contenders for following or eventually even superseding chrono/time.

~I am willing to provide a PR if this proposal is accepted.~ See #1271 for a PR that implements these features.

See #1272 for a PR that adds support for the types Zoned and TimeZone.

BurntSushi commented 3 months ago

Jiff author here. Great to see this! Happy to answer questions. :-)

For Zoned, in order to deserialize into it, a time zone ID like [America/New_York] is required. This is from RFC 9557 (not RFC 8536) and extends RFC 3339. You are absolutely correct that it does not yet enjoy broad support. Temporal will support it (but that isn't in browsers yet) and java.time supports it. I'm not sure what else does, but hopefully more will support it soon.

With that said, if your datetimes do not have a time zone ID in them, and GraphQL cannot otherwise store them, then probably the right thing to do is to not offer Zoned support directly. Either that, or error if the Zoned contains a time zone with a non-None TimeZone::iana_name, since otherwise serializing and deserializing will be lossy.

The idea with a Zoned is that if you have a Zoned in hand, then you have an assertion about what the local time is in relationship to UTC.

Everything else looks correct to me, including the mapping from Jiff's civil date types to GraphQL's local data types. (With the caveat that I don't know too much about GraphQL. But the docs you linked all seem like they support the mapping you chose here.)

sgoll commented 3 months ago

@BurntSushi Wow, didn't expect you to notice this already. Nice to see you here, and thank you so much for making Jiff :slightly_smiling_face:

I can see two potential issues from my putting together a PR for this feature:

  1. GraphQL scalar LocalTime officially only supports millisecond-resolution (i.e. only three digits in the sub-second part .SSS in HH:mm[:ss[.SSS]]). GraphQL scalar LocalDateTime does not mention sub-second resolution at all, but I guess it's the same. I assume there would be some support in GraphQL libraries for higher-resolution times but the current chrono and time integrations in Juniper also limit serialization to three digits, resulting in potential loss of resolution.
  2. GraphQL scalar DateTime supports time zones other than Z as laid out in RFC 3339: explicitly given offsets, but without time zone names, e.g. 2024-06-19T15:22:00-04:00. This would allow us to preserve some information from any Zoned objects, i.e. at least the offset, in contrast to Timestamp which we can only serialize as UTC. Would it be worthwhile to consider serialization of Zoned under these circumstances after all?

Both of these lose data when going through GraphQL scalars. I guess we need to decide whether this can be tolerated.

BurntSushi commented 3 months ago
  1. I think this should be fine. If GraphQL outright rejects datetimes with precision better than milliseconds, then I think you'll just need to strip anything less than milliseconds from the datetime (possibly via rounding). Otherwise, we can teach jiff::fmt::temporal::DateTimePrinter a new knob for controlling output precision.
  2. Yeah that's a tough one, but I think that fits into my suggestion. That is, you return an error when trying to serialize a Zoned with an IANA time zone. But you specifically allow serializing a Zoned when its TimeZone::iana_name is none. This permits users to serialize a fixed offset Zoned. And if they have an IANA time zone name, they'll need to do an explicit conversion, e.g., zdt.with_time_zone(TimeZone::fixed(zdt.offset())). But the point is that it's explicit and they aren't subjecting themselves to a subtle footgun. And this is a very subtle footgun, because a fixed offset will "just work" in a lot of cases.
sgoll commented 3 months ago
  1. I think this should be fine. If GraphQL outright rejects datetimes with precision better than milliseconds, then I think you'll just need to strip anything less than milliseconds from the datetime (possibly via rounding). Otherwise, we can teach jiff::fmt::temporal::DateTimePrinter a new knob for controlling output precision.

In my current draft, I am rounding manually. The need to handle errors is unfortunate, but since these can happen only for configuration errors (which we can rule out), there should be no problems at runtime.

Regarding the principle decision to not allow sub-millisecond resolution, I have opened an issue upstream.

  1. Yeah that's a tough one, but I think that fits into my suggestion. That is, you return an error when trying to serialize a Zoned with an IANA time zone. But you specifically allow serializing a Zoned when its TimeZone::iana_name is none. This permits users to serialize a fixed offset Zoned. And if they have an IANA time zone name, they'll need to do an explicit conversion, e.g., zdt.with_time_zone(TimeZone::fixed(zdt.offset())). But the point is that it's explicit and they aren't subjecting themselves to a subtle footgun. And this is a very subtle footgun, because a fixed offset will "just work" in a lot of cases.

I am wondering how we would try to document this. It seems rather subtle to force users to make sure to use properly fixed Zoned values in their GraphQL schema types. To me, there seems to be a substantial risk involved where developers are not aware of this and forget to put in the appropriate normalization, leading to unexpected errors at runtime since the type gives no indication of whether IANA names were used for the value's TimeZone or not.

Thus, I tend to agree with your initial assessment above and would probably indeed not offer serialization for Zoned—which is unfortunate, but it still seems better than having to pick between two footguns :neutral_face:

BurntSushi commented 3 months ago

In my current draft, I am rounding manually. The need to handle errors is unfortunate, but since these can happen only for configuration errors (which we can rule out), there should be no problems at runtime.

Yeah that works. You could also drop the rounding if you're okay with truncation, which is what the %.3f will do.

Thus, I tend to agree with your initial assessment https://github.com/graphql-rust/juniper/issues/1270#issuecomment-2265865082 and would probably indeed not offer serialization for Zoned—which is unfortunate, but it still seems better than having to pick between two footguns 😐

Holding off on a Zoned impl is fine I think. But to push back a little, presuming serialization returns an error, it doesn't seem like it'd be a huge footgun since the caller will get an error if they try to do something wrong. Then they can fix it. It's at least better than the footgun of silently dropping information.

sgoll commented 3 months ago

Yeah that works. You could also drop the rounding if you're okay with truncation, which is what the %.3f will do.

I was unable to do this. It seems the format specifier sets only the minimum number of digits but does not limit the maximum number of digits. I opened https://github.com/BurntSushi/jiff/issues/73 for tracking this separately (but I don't think supporting this would be absolutely necessary, so feel free to comment there or even close that issue if this would make formatting too complex).

Holding off on a Zoned impl is fine I think. But to push back a little, presuming serialization returns an error, it doesn't seem like it'd be a huge footgun since the caller will get an error if they try to do something wrong. Then they can fix it. It's at least better than the footgun of silently dropping information.

My worries are that the caller has no control over this. Assume they are running a GraphQL query and (at runtime) the Zoned object in question (that is part of the query) has an IANA time zone where it normally has only UTC or fixed offset time zones. Maybe the server database now stores dates differently to track this information.

Then the code would still compile as usual but now querying this attribute in a GraphQL type would suddenly fail.

But, taking a step back, this might all come down to what the transport serialization is able to express anyway:

Since we as developers explicitly decide on what target format to use (e.g. GraphQL scalar DateTime), and we know the limitations of that target format, then it would probably be okay to silently drop the IANA time zone information and just serialize the offset, because RFC 3339 does not support IANA specifiers—and neither do the client-side libraries that consume this data, presumably.

In another environment where the client side supports it, developers could then explicitly settle on using an RFC 9557 serializer type instead for any Zoned objects.[^1]

[^1]: It seems there doesn't exist such a type in https://github.com/urigo/graphql-scalars at the moment but we could suggest adding this.

This is similar to the restrictions of LocalTime where we as developers explicitly decide to serialize into a type that is unable to express sub-millisecond resolution and lose information from the original civil::Time object, but this is fine because it is a conscious decision (and must match the client side that has to consume these values).

Unfortunately, I am not aware of an easy way to tell Juniper to use different serialization formats for the same underlying type, other than using newtypes or explicitly rolling out custom conversions—which kind of counters the first-class support for these types that we are trying to add in this PR.

To enable an easy upgrade path to RFC 9557 serialization when a matching GraphQL scalar is added, I see two possibilities:

  1. Leave out Zoned serialization for now, only serialize Timestamp into GraphQL scalar DateTime. Then, when some new GraphQL scalar ZonedDateTime[^2] has been added, we serialize Zoned into it.
  2. Add Zoned serialization to GraphQL scalar DateTime now, dropping IANA time zone names in the conversion. Then, add some feature flag jiff-rfc9557 or similar later to convert into the RFC 9557 scalar type instead.

[^2]: We would need to come up with a meaningful name that captures the differences between RFC 3339 and RFC 9557.

BurntSushi commented 3 months ago

Gotya. Yeah I'm not especially familiar with GraphQL, but I think what you're saying there makes sense.

I think it would be good to leave out Zoned for now. Not necessarily because we ought to wait for better GraphQL support, but also just to see what practice demands. If the practical advantages of supporting Zoned end up being large enough to outweigh the footgun of dropping the time zone, then it might make sense to add it at that point.

tyranron commented 3 months ago

@sgoll

  1. Leave out Zoned serialization for now, only serialize Timestamp into GraphQL scalar DateTime. Then, when some new GraphQL scalar ZonedDateTime2 has been added, we serialize Zoned into it.

The fact that https://graphql-scalars.dev/docs/scalars/ collection doesn't provide such a scalar is not a blocker as for me. We just can introduce the ZonedDateTime scalar by ourselves and don't bother, as we had done with the LocalDateTime scalar before it appeared there, and as we do with some scalars absent there even now (like BigDecimal).

So, additional PR is welcome!

sgoll commented 3 months ago

@tyranron

The fact that https://graphql-scalars.dev/docs/scalars/ collection doesn't provide such a scalar is not a blocker as for me. We just can introduce the ZonedDateTime scalar by ourselves and don't bother, as we had done with the LocalDateTime scalar before it appeared there, and as we do with some scalars absent there even now (like BigDecimal).

So, additional PR is welcome!

Thank you for giving the go here. I have opened #1278.