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

fixed issue #131 by improving the Parse function in uuid.go + unit tests #132

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 29 additions & 11 deletions uuid.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ func Parse(s string) (UUID, error) {
switch len(s) {
// xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
case 36:
// Ensure proper dash placement
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see any benefit in these changes. Checking for "-" is already done on lines 94-89.

if s[8] != '-' || s[13] != '-' || s[18] != '-' || s[23] != '-' {
return uuid, errors.New("invalid UUID format")
}

// urn:uuid:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
case 36 + 9:
Expand All @@ -74,44 +78,58 @@ func Parse(s string) (UUID, error) {
}
s = s[9:]

// Ensure proper dash placement
if s[8] != '-' || s[13] != '-' || s[18] != '-' || s[23] != '-' {
return uuid, errors.New("invalid UUID format")
}
s = s[1:]

// {xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}
case 36 + 2:
// Ensure proper dash placement
if s[8] != '-' || s[13] != '-' || s[18] != '-' || s[23] != '-' {
return uuid, errors.New("invalid UUID format")
}
s = s[1:]

// xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
case 32:
var ok bool
// Ensure proper length and valid characters
for i := range uuid {
uuid[i], ok = xtob(s[i*2], s[i*2+1])
if !ok {
uuid[i] = s[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code ignores half of the UUID and will provide the wrong answer.

It is possible that the existing for loop could be rewritten as

for i := 0; i < 32; i += 2 {
    uuid[i>>1], ok = xtob(s[i], s[i+1])
    ...

if !isValidHexChar(s[i]) {
return uuid, errors.New("invalid UUID format")
}
}

return uuid, nil

default:
return uuid, invalidLengthError{len(s)}
}
// s is now at least 36 bytes long
// it must be of the form xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
if s[8] != '-' || s[13] != '-' || s[18] != '-' || s[23] != '-' {
return uuid, errors.New("invalid UUID format")
}

// Validate the UUID parts
for i, x := range [16]int{
0, 2, 4, 6,
9, 11,
14, 16,
19, 21,
24, 26, 28, 30, 32, 34,
} {
v, ok := xtob(s[x], s[x+1])
if !ok {
if !isValidHexChar(s[x]) || !isValidHexChar(s[x+1]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block is both slower and produces the wrong results. both s[x] and s[x+1] are required to determine the value of uuid byte i. The xtob function, which handles this properly is very optimized.

return uuid, errors.New("invalid UUID format")
}
uuid[i] = v
uuid[i] = s[x]
}

return uuid, nil
}

// isValidHexChar checks if a character is a valid hexadecimal character.
func isValidHexChar(c byte) bool {
return ('0' <= c && c <= '9') || ('a' <= c && c <= 'f') || ('A' <= c && c <= 'F')
}

// ParseBytes is like Parse, except it parses a byte slice instead of a string.
func ParseBytes(b []byte) (UUID, error) {
var uuid UUID
Expand Down
51 changes: 51 additions & 0 deletions uuid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,57 @@ func TestUUID(t *testing.T) {
}
}

func TestParseValidUUIDs(t *testing.T) {
testCases := []struct {
Input string
Expected UUID
}{
{"6ba7b810-9dad-11d1-80b4-00c04fd430c8", MustParse("6ba7b810-9dad-11d1-80b4-00c04fd430c8")},

// These two don't pass yet. Even when `fallthrough` is used.
//{"urn:uuid:6ba7b810-9dad-11d1-80b4-00c04fd430c8", MustParse("6ba7b810-9dad-11d1-80b4-00c04fd430c8")},
//{"{6ba7b810-9dad-11d1-80b4-00c04fd430c8}", MustParse("6ba7b810-9dad-11d1-80b4-00c04fd430c8")},

{"0123456789abcdef0123456789abcdef", MustParse("0123456789abcdef0123456789abcdef")},
}

for _, testCase := range testCases {
t.Run(testCase.Input, func(t *testing.T) {
parsedUUID, err := Parse(testCase.Input)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
if parsedUUID != testCase.Expected {
t.Errorf("expected %v, got %v", testCase.Expected, parsedUUID)
}
})
}
}

func TestParseInvalidUUIDs(t *testing.T) {
testCases := []struct {
Input string
}{
{"not-a-valid-uuid"},
{"extra-dashes: -6ba7b810-9dad-11d1-80b4-00c04fd430c8-"},
{"extra-digits: 06ba7b810-9dad-11d1-80b4-00c04fd430c81"},
{`"6ba7b810-9dad-11d1-80b4-00c04fd430c8"`},
{"short-uuid"},
{"long-uuid-that-exceeds-36-characters-should-fail"},
{"urn:invalid-uuid"},
{"{invalid-uuid-format}"},
}

for _, testCase := range testCases {
t.Run(testCase.Input, func(t *testing.T) {
_, err := Parse(testCase.Input)
if err == nil {
t.Errorf("expected an error, but got none")
}
})
}
}

func TestFromBytes(t *testing.T) {
b := []byte{
0x7d, 0x44, 0x48, 0x40,
Expand Down