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.

Sign up for GitHub

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

Jump to bottom

Generator options #98

Closed
wants to merge 2 commits into from
Closed

Generator options #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fee135
Added generator options so that default values can be changed
mlesar Feb 25, 2022
b037f1b
Removed go.mod
mlesar Feb 25, 2022
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
48 changes: 37 additions & 11 deletions generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import (
// UUID epoch (October 15, 1582) and Unix epoch (January 1, 1970).
const epochStart = 122192928000000000

type epochFunc func() time.Time
type EpochFunc func() time.Time
Copy link
Member

Choose a reason for hiding this comment

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

Now that this is being exported, could you please add a GoDoc-compatible comment that explains what this is:

// EpochFunc is ...


// HWAddrFunc is the function type used to provide hardware (MAC) addresses.
type HWAddrFunc func() (net.HardwareAddr, error)
Expand Down Expand Up @@ -126,7 +126,7 @@ type Gen struct {

rand io.Reader

epochFunc epochFunc
epochFunc EpochFunc
hwAddrFunc HWAddrFunc
lastTime uint64
clockSequence uint16
Expand All @@ -137,13 +137,43 @@ type Gen struct {
v7ClockSequence uint16
}

type GenOption func(*Gen)

// interface check -- build will fail if *Gen doesn't satisfy Generator
var _ Generator = (*Gen)(nil)

// NewGen returns a new instance of Gen with some default values set. Most
// people should use this.
func NewGen() *Gen {
return NewGenWithHWAF(defaultHWAddrFunc)
// NewGen returns a new instance of Gen with default values if no options are
// passed.
func NewGen(opts ...GenOption) *Gen {
Copy link
Member

Choose a reason for hiding this comment

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

As written this would be a breaking change for the module, and so I don't think we can accept this if it changes NewGen(). We should probably rename this to NewGenWithConfig(), and keep the NewGen() signature as-is.

Secondarily, I really dislike functional options and would prefer we do not adopt them for this use case. They are really hard to discover via documentation, where-as a configuration struct could be a lot more approachable / discoverable and still also support adding new options without it being a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

As written this would be a breaking change for the module,

Why it would be a breaking change if you can call it as before uuid.NewGen()?

Secondarily, I really dislike functional options and would prefer we do not adopt them for this use case. They are really hard to discover via documentation, where-as a configuration struct could be a lot more approachable / discoverable and still also support adding new options without it being a breaking change.

Just to make sure I understand this, you would prefer to have it like this:

type GenConfig struct {
	EpochFunc  EpochFunc
	HWAddrFunc HWAddrFunc
	RandReader io.Reader
}

func NewGen() *Gen {
	return NewGenWithConfig(GenConfig{})
}

func NewGenWithConfig(config GenConfig) *Gen {
	if config.EpochFunc == nil {
		config.EpochFunc = time.Now
	}
	if config.HWAddrFunc == nil {
		config.HWAddrFunc = defaultHWAddrFunc
	}
	if config.RandReader == nil {
		config.RandReader = rand.Reader
	}

	return &Gen{
		epochFunc:  config.EpochFunc,
		hwAddrFunc: config.HWAddrFunc,
		rand:       config.RandReader,
	}
}

?

I find it more specific when using options, as you just use the one that you want to adjust, with config it seems as if someone has to set all of the properties.

gen := &Gen{
epochFunc: time.Now,
hwAddrFunc: defaultHWAddrFunc,
rand: rand.Reader,
}

for _, opt := range opts {
opt(gen)
}

return gen
}

func WithHWAddrFunc(hwaf HWAddrFunc) GenOption {
return func(gen *Gen) {
gen.hwAddrFunc = hwaf
}
}

func WithEpochFunc(epochf EpochFunc) GenOption {
return func(gen *Gen) {
gen.epochFunc = epochf
}
}

func WithRandomReader(reader io.Reader) GenOption {
return func(gen *Gen) {
gen.rand = reader
}
}

// NewGenWithHWAF builds a new UUID generator with the HWAddrFunc provided. Most
Expand All @@ -158,11 +188,7 @@ func NewGen() *Gen {
// MAC address being used, you'll need to create a new generator using this
// function.
func NewGenWithHWAF(hwaf HWAddrFunc) *Gen {
return &Gen{
epochFunc: time.Now,
hwAddrFunc: hwaf,
rand: rand.Reader,
}
return NewGen(WithHWAddrFunc(hwaf))
}

// NewV1 returns a UUID based on the current timestamp and MAC address.
Expand Down
128 changes: 55 additions & 73 deletions generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,11 @@ func testNewV1DifferentAcrossCalls(t *testing.T) {
}

func testNewV1StaleEpoch(t *testing.T) {
g := &Gen{
epochFunc: func() time.Time {
g := NewGen(
WithEpochFunc(func() time.Time {
return time.Unix(0, 0)
},
hwAddrFunc: defaultHWAddrFunc,
rand: rand.Reader,
}
}),
)
u1, err := g.NewV1()
if err != nil {
t.Fatal(err)
Expand All @@ -128,13 +126,11 @@ func testNewV1StaleEpoch(t *testing.T) {
}

func testNewV1FaultyRand(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: defaultHWAddrFunc,
rand: &faultyReader{
g := NewGen(
WithRandomReader(&faultyReader{
readToFail: 0, // fail immediately
},
}
}),
)
u, err := g.NewV1()
if err == nil {
t.Fatalf("got %v, want error", u)
Expand All @@ -145,29 +141,26 @@ func testNewV1FaultyRand(t *testing.T) {
}

func testNewV1MissingNetwork(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: func() (net.HardwareAddr, error) {
g := NewGen(
WithHWAddrFunc(func() (net.HardwareAddr, error) {
return []byte{}, fmt.Errorf("uuid: no hw address found")
},
rand: rand.Reader,
}
}),
)
_, err := g.NewV1()
if err != nil {
t.Errorf("did not handle missing network interfaces: %v", err)
}
}

func testNewV1MissingNetworkFaultyRand(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: func() (net.HardwareAddr, error) {
g := NewGen(
WithHWAddrFunc(func() (net.HardwareAddr, error) {
return []byte{}, fmt.Errorf("uuid: no hw address found")
},
rand: &faultyReader{
}),
WithRandomReader(&faultyReader{
readToFail: 1,
},
}
}),
)
u, err := g.NewV1()
if err == nil {
t.Errorf("did not error on faulty reader and missing network, got %v", u)
Expand Down Expand Up @@ -252,27 +245,26 @@ func testNewV4DifferentAcrossCalls(t *testing.T) {
}

func testNewV4FaultyRand(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: defaultHWAddrFunc,
rand: &faultyReader{
g := NewGen(
WithRandomReader(&faultyReader{
readToFail: 0, // fail immediately
},
}
}),
)
u, err := g.NewV4()
if err == nil {
t.Errorf("got %v, nil error", u)
}
}

func testNewV4ShortRandomRead(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: func() (net.HardwareAddr, error) {
g := NewGen(
WithHWAddrFunc(func() (net.HardwareAddr, error) {
return []byte{}, fmt.Errorf("uuid: no hw address found")
},
rand: bytes.NewReader([]byte{42}),
}
}),
WithRandomReader(&faultyReader{
readToFail: 0, // fail immediately
}),
)
u, err := g.NewV4()
if err == nil {
t.Errorf("got %v, nil error", u)
Expand Down Expand Up @@ -359,13 +351,11 @@ func testNewV6DifferentAcrossCalls(t *testing.T) {
}

func testNewV6StaleEpoch(t *testing.T) {
g := &Gen{
epochFunc: func() time.Time {
g := NewGen(
WithEpochFunc(func() time.Time {
return time.Unix(0, 0)
},
hwAddrFunc: defaultHWAddrFunc,
rand: rand.Reader,
}
}),
)
u1, err := g.NewV6()
if err != nil {
t.Fatal(err)
Expand All @@ -381,13 +371,11 @@ func testNewV6StaleEpoch(t *testing.T) {

func testNewV6FaultyRand(t *testing.T) {
t.Run("randomData", func(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: defaultHWAddrFunc,
rand: &faultyReader{
g := NewGen(
WithRandomReader(&faultyReader{
readToFail: 0, // fail immediately
},
}
}),
)
u, err := g.NewV6()
if err == nil {
t.Fatalf("got %v, want error", u)
Expand All @@ -398,13 +386,11 @@ func testNewV6FaultyRand(t *testing.T) {
})

t.Run("clockSequence", func(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
hwAddrFunc: defaultHWAddrFunc,
rand: &faultyReader{
g := NewGen(
WithRandomReader(&faultyReader{
readToFail: 1, // fail immediately
},
}
}),
)
u, err := g.NewV6()
if err == nil {
t.Fatalf("got %v, want error", u)
Expand All @@ -416,10 +402,9 @@ func testNewV6FaultyRand(t *testing.T) {
}

func testNewV6ShortRandomRead(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
rand: bytes.NewReader([]byte{42}),
}
g := NewGen(
WithRandomReader(bytes.NewReader([]byte{42})),
)
u, err := g.NewV6()
if err == nil {
t.Errorf("got %v, nil error", u)
Expand Down Expand Up @@ -554,12 +539,11 @@ func makeTestNewV7DifferentAcrossCalls(p Precision) func(t *testing.T) {

func makeTestNewV7StaleEpoch(p Precision) func(t *testing.T) {
return func(t *testing.T) {
g := &Gen{
epochFunc: func() time.Time {
g := NewGen(
WithEpochFunc(func() time.Time {
return time.Unix(0, 0)
},
rand: rand.Reader,
}
}),
)
u1, err := g.NewV7(p)
if err != nil {
t.Fatal(err)
Expand All @@ -576,12 +560,11 @@ func makeTestNewV7StaleEpoch(p Precision) func(t *testing.T) {

func makeTestNewV7FaultyRand(p Precision) func(t *testing.T) {
return func(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
rand: &faultyReader{
g := NewGen(
WithRandomReader(&faultyReader{
readToFail: 0, // fail immediately
},
}
}),
)
u, err := g.NewV7(p)
if err == nil {
t.Errorf("got %v, nil error", u)
Expand All @@ -591,10 +574,9 @@ func makeTestNewV7FaultyRand(p Precision) func(t *testing.T) {

func makeTestNewV7ShortRandomRead(p Precision) func(t *testing.T) {
return func(t *testing.T) {
g := &Gen{
epochFunc: time.Now,
rand: bytes.NewReader([]byte{42}),
}
g := NewGen(
WithRandomReader(bytes.NewReader([]byte{42})),
)
u, err := g.NewV7(p)
if err == nil {
t.Errorf("got %v, nil error", u)
Expand Down