disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
351 stars 72 forks source link

Decrease timestamp resolution #1598

Closed ghostbuster91 closed 1 month ago

ghostbuster91 commented 2 months ago

Long story short the change in https://github.com/disneystreaming/smithy4s/pull/1576 prompted me to review in details timestamp datatype and timestampFormat trait.

According to smithy documentation:

2.12. timestamp A timestamp represents an instant in time in the proleptic Gregorian calendar, independent of local times or timezones. Timestamps support an allowable date range between midnight January 1, 0001 CE to 23:59:59.999 on December 31, 9999 CE, with a temporal resolution of 1 millisecond. This resolution and range ensures broad support across programming languages and guarantees compatibility with RFC 3339.

2.12.1. Timestamp serialization and deserialization The serialization format of a timestamp is an implementation detail that is determined by a protocol and or timestampFormat trait. The format of a timestamp MUST NOT have any effect on the types exposed by tooling to represent a timestamp value. Protocols and timestampFormat traits MAY support temporal resolutions other than 1 millisecond. For example, the http-date timestamp format supports only seconds and forbids fractional precision. Modelers need to be aware of these limitations when defining timestamps to avoid an unintended loss of precision. The use of timestamps outside the allowable range risk not interoperating correctly across Smithy implementations; deserializers that encounter timestamps outside the allowable range SHOULD fail to deserialize the value.

and timestamp format docs

For date-time and epoch-seconds:

Values that are more granular than millisecond precision SHOULD be truncated to fit millisecond precision.

For http-date:

A deserializer that encounters an http-date timestamp with fractional precision SHOULD fail to deserialize the value (for example, an HTTP server SHOULD return a 400 status code).

Base on the above I propose following changes:

  1. for timestampFormat=http-date the value should be truncated to second precision during the serialization
  2. for timestampFormat=date-time | epoch-seconds the value should be truncated to millisecond precision during the serialization
Baccata commented 2 months ago

I don't want to change the default behaviour of serialising as much information as possible, which prevents loss of information. Generally speaking, I think tools ought to abide by some level of lenience when it comes to de-serialisation, but provide as much info as available when it comes to serialisation. I'm happy to accept changes of behaviour that go in that direction, but not willing to make changes that go in the other direction.

That being said :

  1. We could make it customisable.
  2. We could also expose a variant to Timestamp.nowUTC to only capture millisecond/seconds precision. Or alternatively add truncateMillis and truncateSeconds, to make it easier to mitigate problems, if they ever arise.
kubukoz commented 2 months ago

I like the ability to customize and a truncateMillis + truncateSeconds addition. Timestamps often come from something like CE's Clock so nowUTC alone won't do the trick.

kubukoz commented 2 months ago

Actually, truncateMillis and truncateSeconds - these would be alloy traits, or methods on Timestamp? (@ghostbuster91 enlightened me and now it seems like you meant traits)

Baccata commented 2 months ago

I meant methods on Timestamp for starters, but we could later on create corresponding traits. I wouldn't want to pull that trigger unless there's a non-hypothetical scenario that'd warrant it.