Accept interfaces, return structs

Our code seems to suffer from what is also referred to as the Preemptive Interface Anti-Pattern in Go , also see the Go style guide on interfaces. We often define an interface without ever expecting to implement a second version of the interface. The main reason for this is to enable mock generation, which is fine, but we then also return the interface from the constructor for this object. This is another go another go anti-pattern. Functions (especially constructors) should accept interfaces, return structs.

Some of the symptoms of this anti-patter existing in our code:

• type casting when the object also implements other interfaces that are not part of the interface returned by the constructor. Like a Close() method. (This happens extensively close to the root of our system).
• interface bloat by adding Close() or io.Closer to the interface to prevent the previous point (violation of Go's minimal viable interfaces)
• type casting in tests when access to struct fields is needed
• Extra mock calls: having to add many irrelevant calls like mockInt.EXPECT.Close() to tests
• Half implemented mock interfaces because we only need a subset of the functions

Constructors should always return a pointer to the concrete type so all interfaces implemented on the object are available locally and upstream (towards root of the system) and from there the scope can be narrowed down by accepting interface in other modules. This prevents many if not all of the symptoms above.

This is also how Go uses interfaces. For example, bytes.NewBuffer() returns a *Buffer that implements 10+ different interfaces of the Go std lib but can still be passed to a function that only needs an io.Reader.

Individual fields on a struct that should not be accessed can be non-exported, but everything can always be reached by reflection, so returning interface provides no extra safety.

The only exception to this would be: (source)

If a type exists only to implement an interface and will never have exported methods beyond that interface, there is no need to export the type itself. Exporting just the interface makes it clear the value has no interesting behavior beyond what is described in the interface. It also avoids the need to repeat the documentation on every instance of a common method.

An example of this is the vcr. The concrete type is vcr and is unexported so the constructor returns the interface:

// VCR is the interface that covers all functionality of the vcr store.
type VCR interface {  
    Issuer() issuer.Issuer  
    Wallet() holder.Wallet  
    Verifier() verifier.Verifier  
    GetOpenIDIssuer(ctx context.Context, id did.DID) (issuer.OpenIDHandler, error)  
    GetOpenIDHolder(ctx context.Context, id did.DID) (holder.OpenIDHandler, error)  
    OpenID4VCIEnabled() bool  
  
    Finder  
    Resolver
    TrustManager
    types.Writer  
}

Of the 12 interfaces implemented on concrete type vcr, the VCR interface wraps 4. The remaining 7 (Engine/System stuff) can only be accessed by type conversion. None of the fields on vcr are exported meaning that the VCR interface can be deleted and vcr can be exported to provide the same level of access to the VCR. Wrapping of the 4 interfaces results in interface bloat that, for example, also allows all modules that take in the VCR interface to manage Trust. If NewVCR returned a concrete type, all conversions and interface bloat could have been avoided.

Access should be restricted by methods only asking for the minimal interface they need, not by trying to limit what is returned from a constructor.

The main questions I think we should consider
a) Is there a reason to not apply this for new constructors?
b) Do we want to spend time applying this to existing code?
c) How do we apply this to engines?

Some thoughts on question c. Some engines need access to multiple interfaces of other engines. I see two main approaches to deal with this.

1. Engines should request each interface they need access to in their constructor.
2. Engines can take in other engines.

Option one could result in many input parameters for a constructor, but probably makes testing easier since it allows passing mocks, only provides access to what is actually needed, actually follows accept interfaces, return structs. The second option allows the scope to be narrowed down in the engine which probably results in less changes to the Engine constructor signature, but IMO not enough of a reason to prefer option 2 over option 1.

We can still define a collection interface for an Engine that lists all interfaces it implements to simplify mocking. So still keep the VCR interface above, but rename it VCRMocker or something and include all interfaces on VCR.

No files or tests are named. Start with the vcr example and the NewVCR constructor, then inspect engine constructors and the interfaces they accept or return; done requires an agreed scope and a consistent approach for new and existing constructors.