add Version4 factory to configure UUID generation

[rittneje]

Adds a Version4 factory struct, which can accumulate options for generating version 4 UUIDs. For now the only option is the io.Reader used for randomness.

Also reverts #80, as that implementation cannot be used safely in general. Instead, clients should create a factory with a buffered io.Reader implementation.

Fixes #86.

cc @pborman

[joewreschnig]

I have similar concerns about this that I have about the pool it's replacing - it's a big new API surface that doesn't really relate to "generating and inspecting" UUIDs. It's nothing that couldn't be done in a wrapper library or application, and doesn't abstract anything about UUIDs themselves.

This package should focus on efficient and correct UUID handling and making sure the UUID follows go best practices; it doesn't need to provide every sugared ctor.

[joewreschnig]

Also, maybe more objectively than some point about API taste, it doesn't solve half of the performance issue the pool did, that of requiring an allocation per call to New:

func BenchmarkUUID_NewVersion4(b *testing.B) {
	b.RunParallel(func(pb *testing.PB) {
		v4 := Version4{bufio.NewReaderSize(rander, 16*16)}
		for pb.Next() {
			_, err := v4.New()
			if err != nil {
				b.Fatal(err)
			}
		}
	})
}

BenchmarkUUID_NewVersion4-4   	 6922863	       172.8 ns/op	      16 B/op	       1 allocs/op

[joewreschnig]

I especially object to this and the deprecation of NewRandom etc. - if the library is going to provide ctor sugar, it should at least provide the most common one as the shortest!

[rittneje]

I mean, "deprecation" ultimately means little in the face of semantic versioning, unless and until v2 of this library rolls around. I can alter the comment to instead suggest considering using *Version4.NewUUID() if that would make you feel better.

[pborman]

This is not deprecated. New and NewString are the two most common ways to create a UUID. Version 4 is by far the most common type of UUID due to the issues associated with Version 1 (which are documented in the RFC).

[rittneje]

@joewreschnig The reason this approach is preferable to yours in #89 is because this will not lead to a combinatorial explosion in the number of constructors. It would be trivial to add some ReadInto(*UUID) error method that directly addresses your desire, without having to have two separate versions of it (one that takes the io.Reader and one that defaults to crypto/rand.Rand). And if more options pertaining to the UUID generation arise, they can easily be added to the Version4 struct, again without any combinatorial explosion.

The only reason I did not add said constructor was to not bloat this PR, since there is no existing functionality to have parity with.

[joewreschnig]

If the project had a long history of adding options I would agree, but the ctor interfaces have been stable for years. The best way to ensure a complex set of options is to make an object to stash them in, instead of figuring out the fewer relevant primitives. (That was also the mistake in the pool API.)

no existing functionality to have parity with.

This doesn't seem fair; the reason there's no zero-allocation ctor to claim parity with is because you took it out.

[rittneje]

This doesn't seem fair; the reason there's no zero-allocation ctor to claim parity with is because you took it out.

I don't understand what you are referring to. There is no constructor to generate into a pre-allocated UUID, hence your PR.

[pborman]

I don't like the multiple changes. I would be more in favor of rolling back the random pool version even though that is a non-backwards compatible change. I would probably, for now, leave the functions in and put in a warning written to standard error.

If you want to add a new factory type for V4 it really does not need to be in this package as the user has no way to get to it without calling it explicitly.

A solution to the issue of not knowing what libraries are doing, the easy solution is to simply disallow the calling of EnableRandPool and DisableRandPool outside of package main. I believe the intent was to call it from main (which is also how I viewed it).

This would also be non-backwards compatible, but I am not too worried at this point as this is a very new function that probably (hopefully) few are using.

[pborman]

This is not deprecated. New and NewString are the two most common ways to create a UUID. Version 4 is by far the most common type of UUID due to the issues associated with Version 1 (which are documented in the RFC).

[pborman]

This is not deprecated. New and NewString are the two most common ways to create a UUID. Version 4 is by far the most common type of UUID due to the issues associated with Version 1 (which are documented in the RFC).

[rittneje]

If you want to add a new factory type for V4 it really does not need to be in this package as the user has no way to get to it without calling it explicitly.

I'm not sure what you mean by this. The user has no way to get to what?

A solution to the issue of not knowing what libraries are doing, the easy solution is to simply disallow the calling of EnableRandPool and DisableRandPool outside of package main.

How do you intend to enforce that?

[pborman]

If you want to add a new factory type for V4 it really does not need to be in this package as the user has no way to get to it without calling it explicitly.

I'm not sure what you mean by this. The user has no way to get to what?

The only way to use the new factory is to write new code. They have to have a new factor (e.g., uuid.Version4) and then call NewUUID on it. No existing code would ever use it. Existing libraries will not use it even if you want to use it. Because of this you can easily create a package that does exactly this and does not require changes to the UUID package.

A solution to the issue of not knowing what libraries are doing, the easy solution is to simply disallow the calling of EnableRandPool and DisableRandPool outside of package main.

How do you intend to enforce that?

Using the reflect and runtime packages it is easy to determine what package your caller is in. The functions would make the check and if they are not called from main then they panic.

[rittneje]

After discussing with my team, we have decided to use our own version of this library instead, as it seems you have a different vision for it than we do.