Add "AtTime" generators for V1, V6, and V7 #142

kohenkatz · 2024-05-13T01:55:21Z

This PR implements the request in #84 to be able to create UUIDs at specific known timestamps, in addition to the current time.

Although that request only mentions UUIDv1, this PR also includes the other two time-based UUID versions, namely v6 and v7.

To prevent code duplication, this PR reimplements NewV1, NewV6, and NewV7 to each call their respective NewV<X>AtTime with the provided EpochFunc, which defaults to providing the current time.

This PR will remain a draft until I have time to write some tests...

codecov · 2024-05-13T01:57:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (190948b) to head (484207c).

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #142   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          447       459   +12     
=========================================
+ Hits           447       459   +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cameracker · 2024-05-13T02:26:03Z

Nice. Looks good so far. I think it was the right call to add some additional functions. As a mega nit, what do you think of the convention NewVNFromTime()?

kohenkatz · 2024-05-13T02:33:32Z

... what do you think of the convention NewVNFromTime()?

Personally, I like At over From for three reasons:

To me, "From" implies some kind of derivation/modification, while "At" implies that the time is used exactly as-is.
I am used to other frameworks that use At as an indicator that something is a timestamp. For example, Laravel's created_at, updated_at, and deleted_at database fields.
It's slightly shorter to type.

All that said, I really don't care if you want to change it.

generator.go

cameracker · 2024-05-13T14:54:46Z

@kohenkatz Dependabot updated the go-setup github action, and I noted we were testing against older versions of go so I took the opportunity to kick them up to latest and latest -1. Might be worth rebasing against master :)

cameracker · 2024-07-15T18:32:52Z

@kohenkatz Hi, heads up, I fat fingered the update branch button. You'll want to do a pull when you pick this back up again. Apologies.

kohenkatz · 2024-08-09T03:59:10Z

After what seems like forever, this PR now has tests!

Ready for review once again.

dylan-bourque · 2024-08-09T14:20:53Z

generator.go

 // Generator provides an interface for generating UUIDs.
 type Generator interface {
 	NewV1() (UUID, error)
+	NewV1AtTime(time.Time) (UUID, error)


I'd hate to create a new major version for this, but this is pedantically a breaking change if any consumer has their own implementation of the Generator interface.

At a minimum, we should call out that there are 3 new methods on this interface so that consumers will have a pointer to the source of their broken builds.

I think it is fine to remove from the interface, but that will mean we cannot use the package-level convenience methods, because those operate using DefaultGenerator which uses this interface.

This just means that to use the new methods users will need to create a custom generator.

I'm not against adding the methods to the interface, we just need to decide whether we bump to v6.x or we accept what is technically a breaking change in a v5.x minor release and "fix with words" for any consumers that have their own implementations.

I think I'd go with jumping to 6.x. It's not such a big deal to make the change.

Easy enough for this code, but It's a much bigger deal for every consumer to have to update every import to s|v5|v6 😬

Unfortunately, there's no way for us to know how many consumers are implementing the interface separately. Numbers like that would be very useful to this decision.

per pkg.go.dev, 749 public things import /v5. I know we have a bunch of internal modules at work that also import it.

I can confirm that we don't have any custom Generator implementations and someone could theoretically check all of those public importers, but it's impossible to know if any non-public consumers will break.

Personally, I feel like the odds are very low that this will actually be a breaking change and we should do a v5.x minor release with a note.

Sounds good to me, but we probably should wait for at least one additional voice.

dylan-bourque

The code changes LGTM with the added tests, but we need to make a call on the breaking change to the Generator interface.

Super nit: I think I would prefer to only include the full details for a variant on just one of the New*() functions rather than have it repeated 4X across the 2 New*() functions and the 2 Gen methods.

cameracker · 2024-08-11T17:54:29Z

Thank you very much @kohenkatz . Regarding the discussion around releasing it as a minor or major, I think the likelihood of a consumer implementing a custom generator is unlikely due to the extremely niche functionality it offers, and I suspect we won't be impacting much by releasing it as a minor.

This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [github.com/gofrs/uuid/v5](https://redirect.github.com/gofrs/uuid) | `v5.2.0` -> `v5.3.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgofrs%2fuuid%2fv5/v5.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fgofrs%2fuuid%2fv5/v5.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fgofrs%2fuuid%2fv5/v5.2.0/v5.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgofrs%2fuuid%2fv5/v5.2.0/v5.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>gofrs/uuid (github.com/gofrs/uuid/v5)</summary> ### [`v5.3.0`](https://redirect.github.com/gofrs/uuid/releases/tag/v5.3.0) [Compare Source](https://redirect.github.com/gofrs/uuid/compare/v5.2.0...v5.3.0) #### Summary In this release, we updated the package to participate in OpenSSF Scorecard and tuned several development workflows and added some fuzz tests. Additionally, We added `AtTime` generators for V1, V6, and V7 so that users may generate UUIDs from time stamps. **NOTE** Technically, the additional of the `AtTime` generators is a breaking change to the `Generator` interface. We decided to go with a `minor` update because of the unlikelihood of this interface being implemented by a consumer, and to reduce the impact of releasing a major version for this feature. #### What's Changed - Add "AtTime" generators for V1, V6, and V7 by [@kohenkatz](https://redirect.github.com/kohenkatz) in [https://github.com/gofrs/uuid/pull/142](https://redirect.github.com/gofrs/uuid/pull/142) - Fix typo in URL in README by [@kohenkatz](https://redirect.github.com/kohenkatz) in [https://github.com/gofrs/uuid/pull/141](https://redirect.github.com/gofrs/uuid/pull/141) - Add OpenSSF Best Practices Badge to README by [@cameracker](https://redirect.github.com/cameracker) in [https://github.com/gofrs/uuid/pull/144](https://redirect.github.com/gofrs/uuid/pull/144) - Create SECURITY.md by [@cameracker](https://redirect.github.com/cameracker) in [https://github.com/gofrs/uuid/pull/143](https://redirect.github.com/gofrs/uuid/pull/143) - Add OpenSSF Scorecard badge to readme by [@cameracker](https://redirect.github.com/cameracker) in [https://github.com/gofrs/uuid/pull/149](https://redirect.github.com/gofrs/uuid/pull/149) - Update fuzz tests to use go fuzz features by [@cameracker](https://redirect.github.com/cameracker) in [https://github.com/gofrs/uuid/pull/148](https://redirect.github.com/gofrs/uuid/pull/148) #### New Contributors - [@ldez](https://redirect.github.com/ldez) made their first contribution in [https://github.com/gofrs/uuid/pull/168](https://redirect.github.com/gofrs/uuid/pull/168) **Full Changelog**: gofrs/uuid@v5.2.0...v5.3.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone Europe/Paris, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/cozy/cozy-stack).

add "AtTime" generators for V1, V6, and V7

6ca3d15

cameracker self-requested a review May 13, 2024 02:26

dylan-bourque reviewed May 13, 2024

View reviewed changes

generator.go Show resolved Hide resolved

generator.go Show resolved Hide resolved

cameracker and others added 2 commits August 8, 2024 23:18

Merge branch 'master' into at_specific_time

be813fc

doc: update doc strings

ad2043b

kohenkatz force-pushed the at_specific_time branch from ee708e7 to ad2043b Compare August 9, 2024 03:24

kohenkatz added 2 commits August 8, 2024 23:57

fix: convenience methods

ecc494d

test: add tests for AtTime methods

484207c

kohenkatz marked this pull request as ready for review August 9, 2024 03:58

dylan-bourque reviewed Aug 9, 2024

View reviewed changes

cameracker approved these changes Aug 11, 2024

View reviewed changes

cameracker merged commit e826b84 into gofrs:master Aug 11, 2024
7 checks passed

kohenkatz deleted the at_specific_time branch August 11, 2024 17:55

cameracker mentioned this pull request Aug 11, 2024

Ability to encode specific timestamp into a V1 UUID #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "AtTime" generators for V1, V6, and V7 #142

Add "AtTime" generators for V1, V6, and V7 #142

kohenkatz commented May 13, 2024

codecov bot commented May 13, 2024 •

edited

Loading

cameracker commented May 13, 2024 •

edited

Loading

kohenkatz commented May 13, 2024

cameracker commented May 13, 2024

cameracker commented Jul 15, 2024

kohenkatz commented Aug 9, 2024

dylan-bourque Aug 9, 2024

kohenkatz Aug 9, 2024

dylan-bourque Aug 9, 2024

kohenkatz Aug 9, 2024

dylan-bourque Aug 9, 2024

kohenkatz Aug 9, 2024 •

edited

Loading

dylan-bourque Aug 9, 2024

kohenkatz Aug 9, 2024

dylan-bourque left a comment

cameracker commented Aug 11, 2024

Add "AtTime" generators for V1, V6, and V7 #142

Add "AtTime" generators for V1, V6, and V7 #142

Conversation

kohenkatz commented May 13, 2024

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

cameracker commented May 13, 2024 • edited Loading

kohenkatz commented May 13, 2024

cameracker commented May 13, 2024

cameracker commented Jul 15, 2024

kohenkatz commented Aug 9, 2024

dylan-bourque Aug 9, 2024

Choose a reason for hiding this comment

kohenkatz Aug 9, 2024

Choose a reason for hiding this comment

dylan-bourque Aug 9, 2024

Choose a reason for hiding this comment

kohenkatz Aug 9, 2024

Choose a reason for hiding this comment

dylan-bourque Aug 9, 2024

Choose a reason for hiding this comment

kohenkatz Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

dylan-bourque Aug 9, 2024

Choose a reason for hiding this comment

kohenkatz Aug 9, 2024

Choose a reason for hiding this comment

dylan-bourque left a comment

Choose a reason for hiding this comment

cameracker commented Aug 11, 2024

codecov bot commented May 13, 2024 •

edited

Loading

cameracker commented May 13, 2024 •

edited

Loading

kohenkatz Aug 9, 2024 •

edited

Loading