Adding support for Guid.CreateVersion7 #104124
Conversation
|
Note regarding the |
1 similar comment
|
Note regarding the |
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
acbce2a to
7191ba9
Compare
| public static Guid CreateVersion7(DateTimeOffset timestamp) | ||
| { | ||
| // This isn't the most optimal way, but we don't have an easy way to get | ||
| // secure random bytes in corelib without doing this, and its not terrible |
There was a problem hiding this comment.
Fwiw, I don't think this comment is necessary.
But we could also do what the NewGuid impl itself does.
There was a problem hiding this comment.
NewGuid just defers down to the COM API on Windows so there's no benefit in duplicating things.
Ideally we'd be able to use System.Security.Cryptography.RandomNumberGenerator but that's at the wrong layer of the stack.
|
|
||
| // 2^48 is roughly 8925.5 years, which from the Unix Epoch means we won't | ||
| // overflow until around July of 10,895. So there isn't much need to handle it | ||
| // particularly given that DateTimeOffset.MaxValue is December 31, 9999 |
There was a problem hiding this comment.
What would need to be handied then if a larger value can't be passed in? Trying to understand the comment.
There was a problem hiding this comment.
It's a comment that's explicitly covering why only taking 48-bits out of a 64-bit integer is okay, that we won't end up in a scenario where we're silently dropping information.
| // particularly given that DateTimeOffset.MaxValue is December 31, 9999 | ||
| long unix_ts_ms = timestamp.ToUnixTimeMilliseconds(); | ||
|
|
||
| Unsafe.AsRef(in result._a) = (int)(unix_ts_ms >> 16); |
There was a problem hiding this comment.
We don't need to worry about sign extension here because unix_ts_ms will never be negative?
There was a problem hiding this comment.
Hmmm, I think it actually can be negative and we may need to detect this case. In particular, DateTimeOffset supports timestamps prior to the Unix Epoch and so can represent Jan 1st, 0001, meaning ToUnixTimeMilliseconds would return 0xFFFFC77CEDD32800 (-62135596800000).
The RFC specifies
UUIDv7 features a time-ordered value field derived from the widely implemented and well-known Unix Epoch timestamp source, the number of milliseconds since midnight 1 Jan 1970 UTC, leap seconds excluded.
and
48-bit big-endian unsigned number of the Unix Epoch timestamp in milliseconds
So negative timestamps would be an error case.
This resolves #103658