Allow datetime() constructor to represent all valid datetimes
#110
Conversation
Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>
Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>
patjakdev
force-pushed
the
datetime-year-range-restriction
branch
from
a9e22ea to
3e79634
Compare
…tetime() constructor. Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>
patjakdev
force-pushed
the
datetime-year-range-restriction
branch
from
0a5b0cc to
7218df2
Compare
patjakdev
changed the title
datetime year range to [0001, 9999]datetime() constructor to represent all valid datetimes
cdisselkoen
left a comment
cdisselkoen
left a comment
There was a problem hiding this comment.
Looks good to me, and I appreciate requiring a leading + or - for the non-RFC-3339 compliant dates, that seems prudent for the future. I agree that the main proposal is well-motivated and is better than all the alternatives.
Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>
There was a problem hiding this comment.
Approving this RFC because I think it's the best resolution for the datetime issue, but I want to highlight that accepting this effectively accepts a breaking change to the entity data format as implemented by the Rust library.
It's a minor break, and we can put off making that change, but I think it's important to weigh that in the decisions to accept.
|
|
||
| The maximum representable Cedar datetime, corresponding to 2^63−1 milliseconds since the Unix epoch, is `+292278994-08-17T07:12:55.807Z`. The minimum representable Cedar datetime, corresponding to 2^63 milliseconds before the UNIX epoch, is `-292275055-05-17T16:47:04.192Z` (using the [proleptic Gregorian calendar](https://en.wikipedia.org/wiki/Proleptic_Gregorian_calendar)). Therefore, in order to cover the full range of the 64-bit `datetime` type, the string argument to the `datetime()` constructor must support a nine-digit year. | ||
|
|
||
| In order to preserve backwards compatibility and readability for the common case of a four-digit year, this RFC proposes that the existing grammar for the string argument to the `datetime()` constructor be amended to allow both the four-digit year RFC 3339 format and nine-digit expanded year profile of ISO 8601, as distinguished by a leading `+` or `-` token. |
There was a problem hiding this comment.
Are there any differences between these standards? From the examples it looks like we're purely extending the year, but I'm wondering if we might end up with something like a slightly different time format being accepted in the 9 digit year version.
There was a problem hiding this comment.
From my limited understanding , RFC 3339 is a profile of ISO 8601 - meaning it's a selection of various options permitted by the ISO 8601 spec. It's difficult to get the full picture because ISO 8601 is behind a paywall.
The ISO 8601 profile we're allowing in addition to RFC 3339 in this RFC is otherwise exactly the same as RFC 3339 except that requires a leading +/- and exactly nine digits in the year rather than four.
| 2. A similar construction would have to be devised for the [proposed entity literal syntax](https://github.com/cedar-policy/rfcs/pull/104). | ||
| 3. This construction seems to be a tacit approval of allowing extension method invocations in serialized entities generally. In the author's opinion, Cedar entities ought to be considered [plain old data](https://en.wikipedia.org/wiki/Passive_data_structure) and such constructions as the one proposed in this alternative are contrary to that idea. | ||
|
|
||
| The upside of this approach is that it's not a breaking change and preserves the ability to perform authorizations involving exotic dates. |
There was a problem hiding this comment.
Some variant of this sentence needs to be moved to the drawback section.
If we accept this RFC we will need to:
- Make a breaking change to entity parsing as implemented by the Rust library OR
- Have the Rust library keep it's current behavior as an "extension" to the Cedar language spec
Neither is great. Realistically I don't think there's appetite for any sort of breaking change right now, so we'd leave the current behavior in the Rust library for a while at least, though I hope we'd remove it eventually.
There was a problem hiding this comment.
Why will a breaking change need to be made to the Rust entity parsing? I think this RFC allows the Cedar Rust team to remove the undocumented parsing of method invocations on entities but doesn't require it.
There was a problem hiding this comment.
I guess it's a matter of perspective, but IMO the Cedar specification should be precise, and a compliant implementation should match it exactly for the parts of the specification it implements. So, the Rust implementation should parse entities exactly as described in this and other RFCs.
As a practical matter, I agree the Rust library developers do not need to make any changes immediately, or even soon, but needing to make an eventual change should factor into our decision making.
3 participants
Rendered