`Timestamp::from_rfc4122` panics when using timestamp before unix epoch about uuid HOT 4 CLOSED

Thanks for the report @zheland. I think we've got two options here:

1. Enshrine the panic by making it explicit (not just debug) and add a section to the docs.
2. Truncate the timestamp so those before the epoch will be set to the epoch, along with a section in the docs.

Do either of those options appeal to you more than the other?

from uuid.

@KodrAus,

1. I don't like panic variant. I do not think that generating UUID with a time value smaller than unix epoch is a good reason for a panic. It seems to me that such things should be forgiven quietly in most UUID generation usecases.
2. Truncating seems much better to me, but with an incorrect time value it may result in a very similar generated UUIDs for UUIDv1 and UUIDv6. However, if considering that the use of incorrect time is rather wrong, then this option is basically completely fine.
3. Alternatively, I might suggest using wrapping operations when converting between rfc4122 and unix time. This will make it possible to obtain some unix time value from any rfc4122. And if necessary convert it back to the same rfc4122 timestamp.
   Pros: UUIDv1 will support any valid rfc4122 timestamp and UUIDv7 will support any valid unix timestamp.
   Cons: Possible silent wrapping operations when using invalid unix time to generate UUIDv1 or silent wrapping operations when using invalid rfc4122 to generate UUIDv7. If mentioned in the documentation it looks quite tolerable behavior.

// draft example, I have not checked the implementation in detail

pub const UUID_TICKS_BETWEEN_EPOCHS: u64 = 0x01B2_1DD2_1381_4000;
pub const UUID_SECS_BETWEEN_EPOCHS: u64 = UUID_TICKS_BETWEEN_EPOCHS / 10_000_000;

const fn unix_to_rfc4122_ticks(seconds: u64, nanos: u32) -> u64 {
    seconds
        .wrapping_add(UUID_SECS_BETWEEN_EPOCHS)
        .wrapping_mul(10_000_000)
        .wrapping_add(nanos as u64 / 100)
}

const fn rfc4122_to_unix(ticks: u64) -> (u64, u32) {
    (
        (ticks / 10_000_000).wrapping_sub(UUID_SECS_BETWEEN_EPOCHS),
        (ticks % 10_000_000) as u32 * 100,
    )
}

#[test]
fn any_rfc4122_can_be_mapped_to_the_corresponding_unixtime_and_back() {
    for (rfc4122, unix) in [
        (u64::MAX, (0x0000_01AA_A6D6_0F4A, 955_161_500)),
        (u64::MAX / 2 + 1, (0x0000_00D3_E741_3965, 477_580_800)),
        (UUID_TICKS_BETWEEN_EPOCHS, (0, 0)),
        (UUID_TICKS_BETWEEN_EPOCHS - 1, (u64::MAX, 999_999_900)),
        (10_000_000, (0xFFFF_FFFD_27AC_6381, 0)),
        (1, (0xFFFF_FFFD_27AC_6380, 100)),
        (0, (0xFFFF_FFFD_27AC_6380, 0)),
    ] {
        assert_eq!(rfc4122_to_unix(rfc4122), unix);
        assert_eq!(unix_to_rfc4122_ticks(unix.0, unix.1), rfc4122);
    }
}

#[test]
fn not_any_unixtime_can_be_mapped_to_the_corresponding_rfc4122() {
    assert_eq!(unix_to_rfc4122_ticks(0, 0), UUID_TICKS_BETWEEN_EPOCHS);
    assert_eq!(unix_to_rfc4122_ticks(0, 99), UUID_TICKS_BETWEEN_EPOCHS);
    assert_eq!(
        unix_to_rfc4122_ticks(0xFE00_0000_0000_0000, 0),
        UUID_TICKS_BETWEEN_EPOCHS
    );
}

from uuid.

I’m also more inclined to a non-panicking variant here. For anybody that’s concerned about quietly changing behaviour through wrapping or truncation we can offer a checked_from_rfc4122 that returns an Option.

I do like that the wrapping variant gives you a more uniquely useful timestamp. I’d be happy with an implementation that did that. It might also be worth checking the current draft spec that introduces v6-v8 and see if it recommends any behaviour converting RRC4122 timestamps to Unix time.

from uuid.

Thanks @zheland 👍 I've opened #691 that addresses this by explicitly wrapping, basically as you've suggested here. All we're guaranteeing is that those methods won't panic, not that they'll wrap to any particular value, so I've reflected that in the test by only ensuring overflowing inputs don't panic.

from uuid.