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 ulid.Nil and .IsZero() method #112

Merged
merged 6 commits into from
Apr 13, 2024
Merged

Conversation

tonyhb
Copy link
Contributor

@tonyhb tonyhb commented Feb 10, 2024

This simplifies comparisons with empty ULIDs. Right now, most people are doing something like the following to check if a ULID is non-zero:

id.Compare(ulid.ULID{}) == 0

This requires allocating a new empty ULID each time. Some packages avoid this by creating their own global (or private) nil ULIDs on initialization.

This should be built in and simple for better perf & a cleaner API.

@tonyhb tonyhb changed the title Add uuid.Nil and .IsZero() method Add ulid.Nil and .IsZero() method Feb 10, 2024
@peterbourgon
Copy link
Member

peterbourgon commented Feb 10, 2024

What suggests that a ULID of all-zeros is categorically different than any other ULID?

Is it valid for code to treat an all-zeroes ULID differently than other ULIDs?

related: #109

@tonyhb
Copy link
Contributor Author

tonyhb commented Feb 11, 2024

The following reasons suggest that zero-value ULIDs are categorically different:

  • The Go spec suggests that a ULID of all Zeros is categorically different to any other ULID. The spec says that any data type initialized "without an explicit initial value are given their zero value", with the spec here: https://go.dev/ref/spec#The_zero_value.
  • A zero-value ULID corresponds to 1970-01-01 00:00:00 +0000 UTC with all random bits set to Zero. Given the uniqueness, rarity, and extreme commonplace occurrence of zero values in Go due to the spec, it's common for people to check if a ULID is non-zero when initializing variables, validating data, and so on.
  • Further, ULIDs encode time (as explained above). Go itself treats a zero-value time as unique, and also includes an IsZero package for time in the stdlib: https://pkg.go.dev/time#Time.IsZero.
  • Finally, the library itself returns a zero-value ULID with an error when calling ulid.New(). Given this function failed, the zero-value ULID itself indicates some level of uniqueness and differentiation to valid ULIDs with non-zero bytes.

Given these, in what way are zero value ULIDs not unique? I'm struggling to see any points suggesting that they should be treated the same.

To answer the edited question on whether it's valid to check:

  • Of course. Given the API above, ulid.New() returns a zero-value ULID on error. You may want to check for this alongside errors.
  • As the Go spec initializes variables with their zero value, it's common to check for non-zero ULIDs during validation
  • Most ULIDs represent identifiers, hence the time and randomness built in. I doubt any system using ULIDs wants to accept zero-value ULIDs, and a check is common.

Finally, given Go itself treats zero-values as unique, in what way is including a method wrong? It's optional; conforms to the stdlib; conforms to go norms; and, most importantly, allows for zero checks without extra allocations.

This simplifies comparisons with empty ULIDs.  Right now, most people
are doing something like the following to check if a ULID is non-zero:

```go
id.Compare(ulid.ULID{}) == 0
```

This requires allocating a new empty ULID each time.  Some packages
avoid this by creating their own global (or private) nil ULIDs on
initialization.

This should be built in and simple for better perf & a cleaner API.
@peterbourgon
Copy link
Member

peterbourgon commented Feb 11, 2024

I don't really buy any of those points, except for this one:

Go itself treats a zero-value time as unique, and also includes an IsZero package for time in the stdlib: https://pkg.go.dev/time#Time.IsZero.

This is a reasonably compelling argument. A zero-value time is a valid time, and the existence of time.IsZero would support the addition of ulid.IsZero. Thinking...

edit:

Go spec suggests that a ULID of all Zeros is categorically different to any other ULID. The spec says that any data type initialized "without an explicit initial value are given their zero value", with the spec here: https://go.dev/ref/spec#The_zero_value.

That says that the zero value of types exists and is a valid value for the type, but it doesn't suggest that it is "categorically different" than any other value of the type, in general. Sometimes it can be, e.g. *int — you can make the case that nil is categorically different than a non-nil pointer. But generally it isn't, e.g. int0 isn't categorically different than 42.

@tonyhb
Copy link
Contributor Author

tonyhb commented Feb 11, 2024

Yep, I agree that ints are different. It's why people shouldn't use 0 in an iota as a value or enum, and most people use this as "EnumInvalid".

But when a ULID encodes ms-level time and randomness, an all-zero byte sequence is definitely suspect.

@peterbourgon
Copy link
Member

Let me be clear that I totally understand what you're saying from a pragmatic perspective. I'm trying to weigh that pragmatism against the, I dunno, "correctness" of the package. Correctness only cares about objective stuff like "is this ULID valid per the spec" and can't make any assertions based on subjective stuff like "is this ULID suspect". Hopefully you understand. But I think I'm getting convinced for func (id ULID) IsZero() bool with var Zero ULID (i.e. not Nil ULID). If you wanted to update the PR with those definitions, that would probably be cool 😎

ulid.go Outdated Show resolved Hide resolved
ulid.go Outdated Show resolved Hide resolved
ulid.go Outdated Show resolved Hide resolved
ulid_test.go Outdated Show resolved Hide resolved
ulid_test.go Outdated Show resolved Hide resolved
@peterbourgon peterbourgon merged commit 96c4edf into oklog:main Apr 13, 2024
4 checks passed
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.

2 participants