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

add uuid v6, v7 #138

Closed
wants to merge 4 commits into from
Closed

add uuid v6, v7 #138

wants to merge 4 commits into from

Conversation

it512
Copy link
Contributor

@it512 it512 commented Nov 14, 2023

@it512 it512 requested a review from a team as a code owner November 14, 2023 11:51
Copy link
Collaborator

@bormanp bormanp left a comment

Choose a reason for hiding this comment

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

There needs to be references to the standards being implemented. Are these official UUID versions that have been through any standards committee.

I don't see any unit tests for the generation of version 6 or version 7 uuids.

The test for version 7 really should verify that the right node and time is placed into the UUID. You can change timeNow to return a known time value so you can verify that the right bits are in the right places (version1 should really be tested this way as well, but that is outside your changes).

There is no reason for any changes in version4.go or util.go.

version4.go Outdated
@@ -9,15 +9,15 @@ import "io"
// New creates a new random UUID or panics. New is equivalent to
// the expression
//
// uuid.Must(uuid.NewRandom())
// uuid.Must(uuid.NewRandom())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change impacts godoc, please do not change the formatting.

version4.go Outdated
func New() UUID {
return Must(NewRandom())
}

// NewString creates a new random UUID and returns it as a string or panics.
// NewString is equivalent to the expression
//
// uuid.New().String()
// uuid.New().String()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change impacts godoc, please do not change the formatting.

version4.go Outdated
// hit by a meteorite is estimated to be one chance in 17 billion, that
// means the probability is about 0.00000000006 (6 × 10−11),
// equivalent to the odds of creating a few tens of trillions of UUIDs in a
// year and having one duplicate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change impacts godoc, please do not change the formatting.

util.go Outdated
@@ -41,3 +41,21 @@ func xtob(x1, x2 byte) (byte, bool) {
b2 := xvalues[x2]
return (b1 << 4) | b2, b1 != 255 && b2 != 255
}

// fill uuid (16byte) from poll
func fill16BytesFromPool(b []byte) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comments from version4.go

version6.go Outdated
@@ -0,0 +1,36 @@
// Copyright 2018 Google Inc. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/2018/2023

version6.go Outdated
// or SetNodeInterface then it will be set automatically. If the NodeID cannot
// be set NewV6 returns nil. If clock sequence has not been set by
// SetClockSequence then it will be set automatically. If GetTime fails to
// return the current NewV6 returns nil and an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a reference to the standard that defines Version 6 UUIDs.

version7.go Outdated
@@ -0,0 +1,70 @@
// Copyright 2018 Google Inc. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/2018/2023

return uint64(sec*1000 + nsec/1000000), nil
}

func NewV7FromReader(r io.Reader) (UUID, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This must have a godoc comment and also needs to reference the standard that defines version 7 UUIDs.

version7.go Outdated
}

// NewV7 returns a Version 7 UUID based on the current time.
// If GetTime fails to return the current NewV7 returns nil and an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It returns Nil, not nil, but only when it is unable to generate random data (see above while there will be no error in getting the current time).

"encoding/binary"
"io"
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the following functions can replace these functions:

func NewV7() (UUID, error) {
        uuid, err := NewRandom()
        if err != nil {
                return uuid, err
        }
        makeV7(uuid[:])
        return uuid, nil
}

func NewV7FromReader(r *io.Reader) (UUID, error) {
        uuid, err := NewRandomFromReader(r)
        if err != nil {
                return uuid, err
        }
        makeV7(uuid[:])
        return uuid, nil
}

func makeV7(uuid []byte) {
        t := timeNow().UnixMilli()
        uuid[0] = (t >> 40)
        uuid[1] = (t >> 32)  
        uuid[2] = (t >> 24)
        uuid[3] = (t >> 16) 
        uuid[4] = (t >> 8) 
        uuid[5] = t 
        uuid[6] = 0x70 | (uuid[6] & 0x0F)
}

They need comments, of course. Putting in the bytes directly rather than using binary.BigEndian.PutUint64 is quite a bit more efficient as you only put in 6 bytes, rather than 8, and PutUint64 fills in the bytes in the same way this code does.

Note that uuid[8] already has the right version number in it.

@it512 it512 closed this by deleting the head repository Nov 15, 2023
@it512 it512 mentioned this pull request Nov 15, 2023
@it512
Copy link
Contributor Author

it512 commented Nov 15, 2023

delete and recommit , see #139

@bensie bensie mentioned this pull request Jan 2, 2024
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.

None yet

2 participants