Thoughts on Copy Options API Design

[Wwwsylvia]

We have already supported Copy and ExtendedCopy functions in oras-go V2 (See #52). The current interfaces are:

func Copy(ctx context.Context, src Target, srcRef string, dst Target, dstRef string) (ocispec.Descriptor, error) {
    // ...
}

func ExtendedCopy(ctx context.Context, src GraphTarget, srcRef string, dst Target, dstRef string) (ocispec.Descriptor, error) {
    // ...
}

Next, we would like to extend some options for both the Copy and the ExtendedCopy functions to support advanced scenarios (See #59). Based on the current implementation, there are a few approaches to achieve this:

1. Modify the original methods to accept an options struct parameter
2. Declare new methods which accept an options struct
3. Use the Functional Option Pattern

Let's take a look at these approaches and see which one fits our scenarios better.

1. Modify the original methods to accept an options struct parameter

Suppose we define the options like below:

type copyOptions struct {
	var1 int
}

func NewCopyOptions() *copyOptions {
	return &copyOptions{}
}

func (o *copyOptions) WithVar1(v1 int) *copyOptions {
	o.var1 = v1
	return o
}

type extendedCopyOptions struct {
	copyOptions
	var2 int
}

func (o *extendedCopyOptions) NewExtendedCopyOptions() *extendedCopyOptions {
	return &extendedCopyOptions{}
}

func (o *extendedCopyOptions) WithVar1(v int) *extendedCopyOptions {
	o.var1 = v
	return o
}

func (o *extendedCopyOptions) WithVar2(v int) *extendedCopyOptions {
	o.var2 = v
	return o
}

To take these options, we need to modify the Copy and ExtendedCopy functions to accept an opts parameter.

func Copy(ctx context.Context, src Target, srcRef string, dst Target, dstRef string, opts *copyOptions) (ocispec.Descriptor, error) {
    // ...
}

func ExtendedCopy(ctx context.Context, src GraphTarget, srcRef string, dst Target, dstRef string, opts *extendedCopyOptions) (ocispec.Descriptor, error) {
    // ...
}

From client perspective, calling Copy and ExtendedCopy would be like:

if cmdOpts.recursive {
    opts := oras.NewExtendedCopyOptions().WithVar1(cmdOpts.var1).WithVar2(cmdOpts.var2)
    desc, err := oras.ExtendedCopy(ctx, src, ref, dst, opts)
} else {
    opts := oras.NewCopyOptions().WithVar1(cmdOpts.var1)
    desc, err := oras.Copy(ctx, src, ref, dst, opts)
}

• Pros:
  • Straightforward
  • Easy to use for clients
• Cons:
  • Introduce breaking changes. Need to modify the existing interfaces and unit tests

2. Declare new methods which accept an options struct

For this approach, the options definition remains the same, but the Copy and ExtendedCopy would be something like:

func Copy(ctx context.Context, src Target, srcRef string, dst Target, dstRef string) (ocispec.Descriptor, error) {
    // ...
}

func CopyWithOptions(ctx context.Context, src Target, srcRef string, dst Target, dstRef string, opts *copyOptions) (ocispec.Descriptor, error) {
    // ...
}


func ExtendedCopy(ctx context.Context, src GraphTarget, srcRef string, dst Target, dstRef string) (ocispec.Descriptor, error) {
    // ...
}

func ExtendedCopyWithOptions(ctx context.Context, src GraphTarget, srcRef string, dst Target, dstRef string, opts *extendedCopyOptions) (ocispec.Descriptor, error) {
    // ...
}

The client code would be like:

// basic client
if cmdOpts.recursive {
    desc, err := oras.ExtendedCopy(ctx, src, ref, dst)
} else {
    desc, err := oras.Copy(ctx, src, ref, dst)
}


// advanced client
if cmdOpts.recursive {
    opts := oras.NewExtendedCopyOptions().WithVar1(cmdOpts.var1).WithVar2(cmdOpts.var2)
    desc, err := oras.ExtendedCopy(ctx, src, ref, dst, opts)
} else {
    opts := oras.NewCopyOptions().WithVar1(cmdOpts.var1)
    desc, err := oras.Copy(ctx, src, ref, dst, opts)
}

• Pros:
  • No breaking changes to the current interfaces
• Cons:
  • We will also need "WithOptions" version of CopyGraph and ExtendedCopy
  • Passing options is not flexible for clients

3. Use the Functional Option Pattern

For this approach, the options definition would become:

type copyOptions struct {
	var1 int
}

type CopyOption func(*copyOptions) error

func NewCopyOptions(opts ...CopyOption) *copyOptions {
	co := &copyOptions{}
	for _, opt := range opts {
		opt(co)
	}

	return co
}

func WithCopyOptionsVar1(v int) CopyOption {
	return func(co *copyOptions) error {
		co.var1 = v
		return nil
	}
}

type extendedCopyOptions struct {
	copyOptions
	var2 int
}

type ExtendedCopyOption func(*extendedCopyOptions) error

func NewExtendedCopyOptions(opts ...ExtendedCopyOption) *extendedCopyOptions {
	eco := &extendedCopyOptions{}
	for _, opt := range opts {
		opt(eco)
	}

	return eco
}

func WithExtendedCopyOptionsVar1(v int) ExtendedCopyOption {
	return func(eco *extendedCopyOptions) error {
		eco.var1 = v
		return nil
	}
}

func WithExtendedCopyOptionsVar2(v int) ExtendedCopyOption {
	return func(eco *extendedCopyOptions) error {
		eco.var2 = v
		return nil
	}
}

The Copy and ExtendedCopy functions would take variadic arguments:

func Copy(ctx context.Context, src Target, srcRef string, dst Target, dstRef string, opts ...CopyOption) (ocispec.Descriptor, error) {
    // ...
}

func ExtendedCopy(ctx context.Context, src GraphTarget, srcRef string, dst Target, dstRef string, opts ...ExtendedCopyOption) (ocispec.Descriptor, error) {
    // ...
}

The clients may use them like this:

// basic client
if cmdOpts.recursive {
    desc, err := oras.ExtendedCopy(ctx, src, ref, dst)
} else {
    desc, err := oras.Copy(ctx, src, ref, dst)
}

// advanced client
if cmdOpts.recursive {
   opts := []oras.ExtendedCopyOption{
    oras.WithExtendedCopyOptionsVar1(cmdOpts.var1),
    oras.WithExtendedCopyOptionsVar2(cmdOpts.var2)
   }
   desc, err := oras.ExtendedCopy(ctx, src, ref, dst, opts)
} else {
    opts:= []oras.CopyOption{
        oras.WithCopyOptionsVar1(cmdOpts.var1)
    }
    desc, err := oras.Copy(ctx, src, ref, dst, opts)
}

• Pros:
  • No breaking changes to the current interfaces
• Cons:
  • Since ExtendedCopy calls Copy internally, it would need extra steps to convert ...ExtendedCopyOption to ...CopyOption
  • When there are common fields shared by different option types, the WithXX function names can be long and ugly

Note: We may leverage the Go 1.18's Generics feature to simplify the naming. (Reference: (Generic) Functional Options Pattern)

type copyOptions struct {
	var1 int
}

type extendedCopyOptions struct {
	copyOptions
	var2 int
}

type Option[T copyOptions | extendedCopyOptions] func(*T) error

func WithVar1[T copyOptions | extendedCopyOptions](v int) Option[T] {
	return func(t *T) error {
		switch o := any(t).(type) {
		case *copyOptions:
			o.var1 = v
		case *extendedCopyOptions:
			o.var1 = v
		default:
			return errors.New("unsupported type")
		}
		return nil
	}
}

Conclusion

Comparing the above 3 approaches, I think the first approach fits our scenarios better and it's simple. Although there will be breaking changes, but it's OK as we have not cut an official release yet, just that we need to update the unit tests. The second approach may not add much value. The third approach looks a bit fancier though, it does not quite fit our scenario where ExtendedCopy and Copy may share some common options.

[Wwwsylvia]

Any Comments are welcomed! /cc @shizhMSFT @qweeah

[shizhMSFT]

@deitch @jdolitsky @sajayantony Any inputs are welcome!

[shizhMSFT]

For option 1 and 2, do we consider CopyOptions instead of *CopyOptions?

[shizhMSFT]

I just found that pointers are better than values since we are doing chaining stuffs.

[deitch]

I think option 1 is the simplest. Everything in go is hash-based modules (or semver), so I don't think breaking change is too big a deal.

[shizhMSFT]

Option 1 looks clean. If someone passes nil to options, we can just use the default option.

[qmuntal]

Agree on make options nillable.

[qmuntal]

I like the option 1 intent, but I don't like the fact that copyOptions and extendedCopyOptions have all its properties unexported and also are themself unexported.

It would be easier to use, and more idiomatic, to just define a plain struct with open properties and without setters. It would look like:

type CopyOptions struct {
	Var1 int
}

type ExtendedCopyOptions struct {
	CopyOptions
	Var2 int
}

// advanced client
opts := oras.CopyOptions{Var1: cmdOpts.var1}
if cmdOpts.recursive {
    extopts := oras.ExtendedCopyOptions{CopyOptions: opts, Var2: cmdOpts.var2}
    desc, err := oras.ExtendedCopy(ctx, src, ref, dst, extopts)
} else {
    desc, err := oras.Copy(ctx, src, ref, dst, opts)
}

I would go even further, and pick option 1 avoiding breaking changes. I think clients will value more keeping the major version the same adding CopyWithOptions (and friends) than having to upgrade to a new major major version, which I known to be somehow painful in Go.

Notice that option 3 is also a breaking change, as you are adding a variadic parameter to the function signature. Example of client that would be broken:

// We use fnCopy instead of oras.Copy so it can be replaced and mocked in tests.
var fnCopy = oras.Copy

func Foo() {
   fnCopy(...)
}

func mockCopy(ctx context.Context, src Target, srcRef string, dst Target, dstRef string) (ocispec.Descriptor, error) {
    // ...
}
func TestFoo(t *testing.T) {
  fnCopy = mockCopy // <----- BOOOM
  Foo()
}

[Wwwsylvia]

Thanks @qmuntal for the valuable suggestions! Yeah using the plain struct is more idiomatic, we can go for it if people agree that it's easier to use. Also thanks for bringing up the mock issue for option 3!
Since oras-go V2 is still under development and is not released yet, actually we've been continually making breaking changes these days, we may not mind adding another one😂.

[qmuntal]

If we want to get fancy and implement functional options pattern, like option 3, I suggest an alternative implementation:

type copyOpts struct {
	var1 string
	var2 int
}

type copyWith struct{}

func (copyWith) Var1(var1 string) func(*copyOpts) {
	return func(co *copyOpts) {
		co.var1 = var1
	}
}

func (copyWith) Var2(var2 int) func(*copyOpts) {
	return func(co *copyOpts) {
		co.var2 = var2
	}
}

var CopyOpt copyWith

func CopyWithOpts(ctx context.Context, src Target, srcRef string, dst Target, dstRef string, opts ...func(*copyOpts)) (ocispec.Descriptor, error) {
        // ...
	var co copyOpts
	for _, opts := range opts {
		opts(&co)
	}
        // ...
}

The clients may use them like this:

oras.CopyWithOpts(ctx, src, srcRef, dst, "", oras.CopyOpt.Var1("foo"), oras.CopyOpt.Var2(3))

Pros:

• There is no need to construct an object to hold the properties
• The package namespace is not polluted with functions like WithCopyOptionsVar1. Everything is encapsulated under oras.CopyOpt and oras.ExtCopyOpt.

Cons:

• I like option 1 with exported props more than this.

[Wwwsylvia]

This looks nice!