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

initial implementation of UUIDv6 and UUIDv7 based on RFC Draft Rev 2 #93

Merged
merged 1 commit into from
Oct 16, 2021

Conversation

theckman
Copy link
Member

@theckman theckman commented Aug 28, 2021

There is currently an RFC draft in progress[1] to add three new UUID formats,
versions 6, 7, and 8, that change the internal data format so that the generated
UUIDs are k-sortable. Version 8 is a special UUID format designed for future
implementations or extensions, and as a result isn't implemented here.

Because this impelmentation is based on a draft RFC, they may be subject to
change in later revisions. As such, these new functions/methods include a
comment indicating they do not yet fall under the API compatibility guarantee of
the project, and changes to meet new revisions to the spec will be done in minor
releases.

[1] https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-02

@theckman theckman force-pushed the draft-peabody-dispatch-new-uuid-format-01 branch 2 times, most recently from b2f60a0 to 2b49ae5 Compare September 8, 2021 18:14
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2021

Codecov Report

Merging #93 (03e219b) into master (8e62f78) will increase coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   98.87%   99.27%   +0.39%     
==========================================
  Files           4        4              
  Lines         356      549     +193     
==========================================
+ Hits          352      545     +193     
  Misses          3        3              
  Partials        1        1              
Impacted Files Coverage Δ
generator.go 98.66% <100.00%> (+2.08%) ⬆️
uuid.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e62f78...03e219b. Read the comment docs.

@theckman theckman force-pushed the draft-peabody-dispatch-new-uuid-format-01 branch from 2b49ae5 to 72e1726 Compare September 8, 2021 19:27
@theckman
Copy link
Member Author

theckman commented Sep 8, 2021

So I updated my commit but codecov did not update the comment to indicate I'd fixed the coverage decrease...

@theckman theckman closed this Sep 8, 2021
@theckman theckman reopened this Sep 8, 2021
@theckman
Copy link
Member Author

theckman commented Sep 8, 2021

There we go...

@cameracker cameracker self-assigned this Sep 10, 2021
binary.BigEndian.PutUint16(u[4:], uint16(timeNow>>12)) // set time_mid
binary.BigEndian.PutUint16(u[6:], uint16(timeNow&0xfff)) // set time_low (minus four version bits)
binary.BigEndian.PutUint16(u[8:], clockSeq&0x3fff) // set clk_seq_hi_res (minus two variant bits)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set u[9:] to clk_sec_low_res

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does that implicitly. uint16 is represented as two bytes, so we use the BigEndian binary encoder to put the uint16 starting at u[8]. So it writes both bytes out, covering the hi and lo bits.

Copy link
Collaborator

@cameracker cameracker Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've approved the PR, but maybe a comment to that effect would be nice!

binary.BigEndian.PutUint16(u[8:], clockSeq&0x3fff) // set clk_seq_hi_res (minus two variant bits)

u.SetVersion(V6)
u.SetVariant(VariantRFC4122)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this variant correct? It's looking for the value, but can't remember of go has iotas 0 indexed (which would make this 1) or if this is indeed the right value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cameracker SetVariant does the translation.

d |= (msec << 16) // set msec field
d |= (uint64(seq) & 0xfff) // set seq field

binary.BigEndian.PutUint64(u[:], d)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is grouping it this way going to result in the msec and seq bits being stored BigEndian? That may be a deviation from the spec, because only the nano bits are stored as big endian (at least according to my reading).

Note, this concern applies to all the v7 generators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original spec (RFC 4122) specifies that values should be in network byte order (Big Endian):

The fields are encoded as 16 octets, with the sizes and order of the fields defined above, and with each field encoded with the Most Significant Byte first (known as network byte order). Note that the field names, particularly for multiplexed fields, follow historical practice.

This draft doesn't make any changes to that.

Copy link
Collaborator

@cameracker cameracker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few questions, but this seems mostly correct!

@cameracker
Copy link
Collaborator

I guess one more question, do you think we'd benefit from a test that verifies these are sortable as we expect?

@theckman
Copy link
Member Author

@cameracker I could probably do that in one of the tests I currently have where I generate a bunch of UUIDs to actually exercise the clock sequence generator. I could fail the test if the previous UUID isn't < the current one.

I'll add that in tonight.

@theckman theckman force-pushed the draft-peabody-dispatch-new-uuid-format-01 branch 2 times, most recently from 10d97a6 to 9f8ac2f Compare October 15, 2021 23:54
@theckman theckman changed the title initial implementation of UUIDv6 and UUIDv7 based on RFC Draft Rev 1 initial implementation of UUIDv6 and UUIDv7 based on RFC Draft Rev 2 Oct 15, 2021
There is currently an RFC draft in progress[1] to add three new UUID formats,
versions 6, 7, and 8, that change the internal data format so that the generated
UUIDs are k-sortable. Version 8 is a special UUID format designed for future
implementations or extensions, and as a result isn't implemented here.

Because this implementation is based on a draft RFC, they may be subject to
change in later revisions. As such, these new functions/methods include a
comment indicating they do not yet fall under the API compatibility guarantee of
the project, and changes to meet new revisions to the spec will be done in minor
releases.

[1] https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-02
@theckman theckman force-pushed the draft-peabody-dispatch-new-uuid-format-01 branch from 9f8ac2f to 03e219b Compare October 15, 2021 23:55
@theckman
Copy link
Member Author

theckman commented Oct 15, 2021

Confirmed we are compatible with Revision 2 of the RFC Draft. Updated the PR to reflect that, and added the tests. Ready for review+merge.

@theckman theckman merged commit 6fe74ff into master Oct 16, 2021
@theckman theckman deleted the draft-peabody-dispatch-new-uuid-format-01 branch October 16, 2021 02:03
@johanbrandhorst
Copy link

Do we need to update the README for this or are we waiting for stabilization?

@cameracker
Copy link
Collaborator

cameracker commented Nov 3, 2021

Thanks @johanbrandhorst , I took a swing at that here: #94

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 this pull request may close these issues.

4 participants