Discussion: foo.NewFoo(foo.WithBar()...) design for v4 release

[fornellas-udemy]

• sparked from this thread
• this is an open ended conversation about this design, and how it impacts v4 rollouts

On v3, I've been building tests with VRC using a pattern like so:

• There's a project wide vcr.GetRecorder() function that sets up common behavior for the Recorder.
  • Cassette path, whether to overwrite them, matcher, cleanup etc.
• A per-API being tested SetupFooTest(), which decorates the Recorder behavior with specifics:
  • Deciding which token to use (real or mocked), setup token masking hooks, blocking non-safe requests, rate limit etc.
  • Sometimes there are multiple APIs, and various of these are called.

On v4, the change to foo.NewFoo(foo.WithBar()...) requires refactoring all this logic:

• We can't build the Recorder object and decorate it anymore.
• We need now to, accumulate all recorder.Option functions from the "baseline", then for each used API, and then finally creating the object and setting up the cleanup.
• GetRecorder(t *testing.T) *recorder.Recorder would need changing to something like GetRecorderOptions(t *testing.T) []recorder.Option, so it'd return a list of function pointers.
• SetupFooTest(t *testing.T, vcrRecorder *recorder.Recorder) (authToken string) would need to be changed to something like SetupFooTest(t *testing.T, mode recorder.Mode) (authToken string, []recorder.Option), which creates sort of a chicken and egg problem.
  • GetRecorderOptions() returns function pointers, and we can't know what's the recording mode there.
  • It'd need to be changed to return the recording mode as well as the function pointer.
• Rinse, repeat, until this works...

Considering v3 + #99 + #100 as a baseline, if we called that v4, upgrading would be trivial (only the new stricter default matcher could break some pre-existing brittle / dubious code). With this v4 interface change, upgrades become a big endeavour.

To be clear, I'm 100% on breaking APIs and improving things, assuming the end result brings us somewhere that's so much better, that's worth the migration headache. In this case though:

• Objectively speaking, having "a lot of" WithSomeOption() methods requires (a lot) more code to write and maintain than a simple type Options struct, or even simpler, a plain type Recorder struct, a la http.Server from the standard library (no need for WithOptionFoo() or foo.SetOptionBar() code).
• Migrating from v3 to v4 can be... quite painful.
• IMHO the complexities of WithOption() puts more cognitive load on users than other solutions.
• And mainly... not clear to me what problems does changing to WithOption() addresses, and why it is worth the migration trouble.

@dnaeon, WDYT?

// foo/foo.go

func TestFoo(t *testing.T) {
	vcrRecorder := vcr.GetRecorder(t)
	fooToken := SetupFooTest(t, vcrRecorder)
	runTestWithTokenAndClient(fooToken, vcrRecorder.GetDefaultClient())
}

func SetupFooTest(t *testing.T, vcrRecorder *recorder.Recorder) (authToken string) {
	// based on vcrRecorder.Mode():
	// - If ModeReplayOnly, use a fixed mocked token value
	// - If ModeRecordOnce, try fetching a valid token from env vars
	authToken, replacement := getTestAuthToken(t, vcrRecorder.Mode())

	vcrRecorder.AddHook(
		vcr.GetMaskHeadersHookFn([]vcr.HeaderMaskRule{
			{
				Header:      "Authorization",
				Secret:      authToken,
				Replacement: replacement,
			},
		}),
		recorder.AfterCaptureHook,
	)

	vcrRecorder.SetRealTransport(vcr.NewReadOnlyRoundTripper(t, http.DefaultTransport))

	// if ModeReplayOnly, then remove rate limit
	setTestRateLimit(t, vcrRecorder.Mode())

	client = NewClient(authToken, vcrRecorder.GetDefaultClient())
	require.NotNil(t, client)

	return authToken
}

// vcr/helpers.go

// Gives a baseline common Recorder that behaves coherently across all tests.
func GetRecorder(t *testing.T) *recorder.Recorder {
	cassetteName := fmt.Sprintf("fixtures/%s", t.Name())
	cassettePath := fmt.Sprintf("%s.yaml", cassetteName)

	mode := recorder.ModeReplayOnly
	if _, err := os.Stat(cassettePath); err != nil {
		if errors.Is(err, os.ErrNotExist) {
			mode = recorder.ModeRecordOnce
		} else {
			require.NoError(t, err)
		}
	} else {
		for _, env := range os.Environ() {
			if env == "VCR_UPDATE_FIXTURES=true" {
				if err := os.Remove(cassettePath); err != nil {
					require.NoError(t, err)
				}
				mode = recorder.ModeRecordOnce
				break
			}
		}
	}

	recorder, err := recorder.NewWithOptions(&recorder.Options{
		CassetteName:       cassetteName,
		Mode:               mode,
		SkipRequestLatency: true,
	})
	require.NoError(t, err)

	matcherFn := cassette.NewMatcherFunc(&cassette.MatcherFuncOpts{
		IgnoreUserAgent: true,
	})
	recorder.SetMatcher(matcherFn)

	t.Cleanup(func() {
		if !t.Failed() {
			require.NoError(t, recorder.Stop())
		}
	})

	return recorder
}

[dnaeon]

Hey @fornellas-udemy ,

Thanks for the detailed issue!

On v4, the change to foo.NewFoo(foo.WithBar()...) requires refactoring all this logic:

• We can't build the Recorder object and decorate it anymore.
• We need now to, accumulate all recorder.Option functions from the "baseline", then for each used API, and then finally creating the object and setting up the cleanup.
• GetRecorder(t *testing.T) *recorder.Recorder would need changing to something like GetRecorderOptions(t *testing.T) []recorder.Option, so it'd return a list of function pointers.
• SetupFooTest(t *testing.T, vcrRecorder *recorder.Recorder) (authToken string) would need to be changed to something like SetupFooTest(t *testing.T, mode recorder.Mode) (authToken string, []recorder.Option), which creates sort of a chicken and egg problem.

Not necessarily. Once a recorder has been created, it can be decorated/customized further, if needed. It's just a matter of calling out those options again, but with different values.

I won't be able to go over each point now, but will try to get back to them soon.

For now, I'll leave this code here, which is a rough translation of the provided code to v4.

// foo/foo.go

func TestFoo(t *testing.T) {
	vcrRecorder := vcr.GetRecorder(t)
	fooToken := SetupFooTest(t, vcrRecorder)
	runTestWithTokenAndClient(fooToken, vcrRecorder.GetDefaultClient())
}

func SetupFooTest(t *testing.T, vcrRecorder *recorder.Recorder) (authToken string) {
	// based on vcrRecorder.Mode():
	// - If ModeReplayOnly, use a fixed mocked token value
	// - If ModeRecordOnce, try fetching a valid token from env vars
	authToken, replacement := getTestAuthToken(t, vcrRecorder.Mode())

	recorder.WithHook(
		vcr.GetMaskHeadersHookFn([]vcr.HeaderMaskRule{
			{
				Header:      "Authorization",
				Secret:      authToken,
				Replacement: replacement,
			},
		}),
		recorder.AfterCaptureHook,
	)(vcrRecorder)

	recorder.WithRealTransport(vcr.NewReadOnlyRoundTripper(t, http.DefaultTransport))(vcrRecorder)

	// if ModeReplayOnly, then remove rate limit
	setTestRateLimit(t, vcrRecorder.Mode())

	client = NewClient(authToken, vcrRecorder.GetDefaultClient())
	require.NotNil(t, client)

	return authToken
}

// vcr/helpers.go

// Gives a baseline common Recorder that behaves coherently across all tests.
func GetRecorder(t *testing.T) *recorder.Recorder {
	cassetteName := fmt.Sprintf("fixtures/%s", t.Name())
	cassettePath := fmt.Sprintf("%s.yaml", cassetteName)

	mode := recorder.ModeReplayOnly
	_, err := os.Stat(cassettePath)
	switch {
	case errors.Is(err, os.ErrNotExist):
		// Cassette is missing, recording
		mode = recorder.ModeRecordOnce
	case errors.Is(err, nil):
		// Cassette is present
		if os.Getenv("VCR_UPDATE_FIXTURES=true") == "true" {
			if err := os.Remove(cassettePath); err != nil {
				require.NoError(t, err)
			}
			mode = recorder.ModeRecordOnce
		}
	default:
		// Some other error occurred
		require.NoError(t, err)
	}

	r, err := recorder.New(
		recorder.WithCassette(cassetteName),
		recorder.WithMode(mode),
		recorder.WithSkipRequestLatency(true),
		recorder.WithMatcher(cassette.NewDefaultMatcher(cassette.WithIgnoreUserAgent(true))),
	)
	require.NoError(t, err)

	t.Cleanup(func() {
		if !t.Failed() {
			require.NoError(t, r.Stop())
		}
	})

	return r
}

I think the functional opts pattern offers a cleaner API overall, and also doesn't expose too much of the inner workings of the recorder itself. In v3 we have multiple constructors - New, NewWithOptions, etc. which currently in v4 is accomplished via a single one.

Again, thanks for the detailed issue, and I'll try to get back to the rest of the points soon!

[fornellas-udemy]

recorder.WithRealTransport(vcr.NewReadOnlyRoundTripper(t, http.DefaultTransport))(vcrRecorder)

OK, so because of type Option func(r *Recorder), we can use these "with" methods directly on pre-created objects. This should enable to mimic v3 behaviour indeed.

I think the functional opts pattern offers a cleaner API overall, and also doesn't expose too much of the inner workings of the recorder itself.

I feel like we're disagreeing on this point. The fact the previous suggestion wasn't obvious to me (and maybe others as well), is evidence of that this is not black and white.

The argument about "cleaner API" is subjective, and often a function of our frame of reference, so... neither is view necessarily better / worse than the other.

However, I think we can more objectively look at two different designs, and gauge complexity, easiness to understand, alignment with common Go idioms, volume of code required etc etc. In this light, I'm considering comparing this:

type Recorder struct {
	BlockUnsafeMethods bool
	Cassette  string
	AfterCaptureHook []HookFunc
	BeforeSaveHook []HookFunc
	BeforeResponseReplayHook []HookFunc
	OnRecorderStopHook []HookFunc
	Matcher MatcherFunc
	Mode Mode
	PassthroughFunc PassthroughFunc
	RealTransport http.RoundTripper
	ReplayableInteractions bool
	SkipRequestLatency bool
}

to what we have now on v4:

• This is "as simple as it gets".
• It uses straightforward patterns used by standard Go libraries (eg: http.Server.
• This enables us to not need New* methods, Option and all With* methods.
• Adding any extra options is one-liner, as opposed to adding the private field to the struct, and an options function.
• Creating new objects does not require jumping around Options and various With* function to find "what are all option": it is all is the same short struct.

Personally, I always gravitate towards the simplest solution: the best code is the one I don't have to write or maintain :-P

---

@dnaeon thanks for taking the time to engage on the conversation. I suppose your suggestion above "unblocks" me from having to fight to refactor a lot of stuff, and just do some ad-hoc substitutions, so thanks for the suggestion.

If you feel like keeping the discussion about the v4 design here, I'm happy to put time to help craft something concrete out of this. If not, it is totally fine, as there's some subjective element to this conversation, and no point in being opinionated.

[dnaeon]

If you feel like keeping the discussion about the v4 design here, I'm happy to put time to help craft something concrete out of this. If not, it is totally fine, as there's some subjective element to this conversation, and no point in being opinionated.

Nothing is set in stone, and I'm open to any suggestions and improvements for v4 :)

[fornellas-udemy]

Nothing is set in stone, and I'm open to any suggestions and improvements for v4 :)

So... how about using the plain struct, WDYT?

type Recorder struct {
	BlockUnsafeMethods bool
	Cassette  string
	AfterCaptureHook []HookFunc
	BeforeSaveHook []HookFunc
	BeforeResponseReplayHook []HookFunc
	OnRecorderStopHook []HookFunc
	Matcher MatcherFunc
	Mode Mode
	PassthroughFunc PassthroughFunc
	RealTransport http.RoundTripper
	ReplayableInteractions bool
	SkipRequestLatency bool
}

[dnaeon]

cc: @jsoriano, @calvinmclean

[calvinmclean]

I don't have a huge stake in the implementation of options/config. Either way will be fine for me, but here's a few of my opinions:

• When switching to v4, I appreciated how I can easily add an option to my existing recorder, or place it in an argument to New instead of having to change my code to use NewWithOptions(&recorder.Options{...}) whenever I want to temporarily override the default mode
• It's nice to be able to modify an existing recorder instead of having to setup all preferences in a struct before creating
• Users can create custom Option functions that combine a few of the built-in ones for easy reuse
  • It's also possible to define a custom config struct outside the library that just applies the relevant options
• Possibly easier to maintain, but that's pretty subjective and I don't have any concrete evidence
• One issue I have with v4 is that the cassette name is an option instead of a required argument
  • A benefit of the functional options pattern is that there's one constructor that can be used cleanly with no options, but this benefit is lost since recorder.WithCassette is always required (as far as I can tell)
  • I would prefer a constructor that requires the cassette name: New(cassetteName string, opts ...Option)
• I do agree that functional options can be less intuitive initially because it's not all defined in one place like a config struct
  • It could be more intuitive and easy to find if the options are also defined as methods on the recorder.Recorder so you can use r.WithHook() instead of recorder.WithHook()(r). This would also allow IDE code completion to make it easy to discover options after creating a base recorder
  • Of course this is more code to maintain, but could be done by code generation (I'd be happy to implement this if you like). It would work by identifying functions that return Option and then generating a method with the same name and arguments that just calls the Option function
  • Also if this is added, it creates a lot of ways of doing the same thing which can be annoying. There's not much reason to have options as arguments anymore since you could do recorder.New().WithHooks(...).WithMode(...)

Overall, I have a preference for the new options pattern but don't have any compelling concrete reasons for it since it's mostly my personal preference.
I don't totally object to using a struct, except that it might require a v5 unless it's added alongside the functional options.

[dnaeon]

A benefit of the functional options pattern is that there's one constructor that can be used cleanly with no options, but this benefit is lost since recorder.WithCassette is always required (as far as I can tell)
I would prefer a constructor that requires the cassette name: New(cassetteName string, opts ...Option)

That's a good point. The cassette name is required, which I will take into account and fix that in v4.

Users can create custom Option functions that combine a few of the built-in ones for easy reuse
It's also possible to define a custom config struct outside the library that just applies the relevant options

Exactly. And that is one of the nice things about this pattern.

Here's an example in one of my other projects, where I think this plays out nicely, when configuring options externally.

• cmd/kustomize-dot/generate.go
• cmd/kustomize-dot/plugin.go

Another benefit to me about this pattern is that it helps with encapsulation and not exposing too much of the inner details of the recorder.

It also helps with separation of concerns, because it allows each recorder.Option to validate it's input, without having to have too much domain knowledge about the recorder itself, or what other options might be doing.

Without this, when using a single Config struct, this kind of validation needs to happen centrally (usually in the New constructor), but with options pattern we can move this logic to the option func itself.

Another good read about functional options may be found in Uber Go Style Guide.

It could be more intuitive and easy to find if the options are also defined as methods on the recorder.Recorder so you can use r.WithHook() instead of recorder.WithHook()(r). This would also allow IDE code completion to make it easy to discover options after creating a base recorder

Of course this is more code to maintain, but could be done by code generation (I'd be happy to implement this if you like). It would work by identifying functions that return Option and then generating a method with the same name and arguments that just calls the Option function

Also if this is added, it creates a lot of ways of doing the same thing which can be annoying. There's not much reason to have options as arguments anymore since you could do recorder.New().WithHooks(...).WithMode(...)

That's how I think about it as well -- having too many ways to create a recorder is not a good thing, in my opinion.

Thanks for the feedback, @calvinmclean !

[dnaeon]

A benefit of the functional options pattern is that there's one constructor that can be used cleanly with no options, but this benefit is lost since recorder.WithCassette is always required (as far as I can tell)
I would prefer a constructor that requires the cassette name: New(cassetteName string, opts ...Option)

That's a good point. The cassette name is required, which I will take into account and fix that in v4.

Related change:

• 1ae1a38

[fornellas-udemy]

Thanks for sharing these points. I reckon there's a preference for this pattern here, so arguing about preference either way, when both would be functional, may not be fruitful at this point (especially because the effort to write the code already happened).

How v4 is, is functional for me, regardless on what's my preference. I'm OK closing this issue if you are @dnaeon .