search

gofrs / uuid

A UUID package for Go
MIT License
1.57k stars 110 forks source link

fixed version 2 domain bits to be spec compliant #80

Closed bionoren closed 5 years ago

bionoren commented 5 years ago

I noticed some instability with testNewV2DifferentAcrossCalls. Wikipedia says "Version-2 UUIDs are similar to version 1, except that the least significant 8 bits of the clock sequence are replaced by a "local domain" number", which you're trying to do on line 181, except that you've generated the v1 UUID using binary.BigEndian. Big endian says that the "The most significant byte (MSB) value ... is at the lowest address. The other bytes follow in decreasing order of significance" (wikipedia again).

Since the clockSeq is inserted at [8:], assigning the domain to [9] was replacing the most significant bits (the ones most likely to change), not the least significant as specified. This patch fixes that. Not that anyone actually uses v2 in practice... but it's nice to have passing tests :)

cameracker commented 5 years ago

Hi @bionoren , thanks very much for the contribution.

We also noticed the bugs with UUID V2 and after a long, careful analysis, we concluded that UUID V2 is not specified enough for us to correctly support it in this library, and we're planning on deprecating it in the next major version of the library. https://github.com/gofrs/uuid/pull/71. As a result, despite my appreciation for your work, I will recommend that we reject this PR.

What are your thoughts?

bionoren commented 5 years ago

Oh, no, I have no interest in it. It was just causing flaky tests and I didn't want that to cause problems with our automated builds. If it's going away soon, great!

theckman commented 5 years ago

Not that anyone actually uses v2 in practice... but it's nice to have passing tests :)

I think this comment summarizes the state of things. Since this is now on others' radar, it's likely a good idea to go forth and merge the removal of V2 support completely and cut the next major release of the library to avoid people wasting their time on a contribution we don't plan to merge-in.

For context, I was hesitating on the merge because it's yet another major version bump and I was hopeful Modules would have their stuff sorted out before we had to do that. I also aborted the plans to fix the way it was generated, because I was worried that (depending how people use the library) it may be a breaking change too.

Thank you for this contribution, and I'm sorry to not have removed it sooner so you didn't waste your time on this.