Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decrease timestamp resolution #1598

Closed
ghostbuster91 opened this issue Sep 27, 2024 · 4 comments · Fixed by #1607
Closed

Decrease timestamp resolution #1598

ghostbuster91 opened this issue Sep 27, 2024 · 4 comments · Fixed by #1607
Assignees

Comments

@ghostbuster91
Copy link
Contributor

ghostbuster91 commented Sep 27, 2024

Long story short the change in #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
Copy link
Contributor

Baccata commented Oct 1, 2024

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
Copy link
Member

kubukoz commented Oct 1, 2024

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
Copy link
Member

kubukoz commented Oct 1, 2024

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
Copy link
Contributor

Baccata commented Oct 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants