Athens: Accept interfaces and return struct

[devdinu]

Storage implementations could return actual struct instead of storage.Backend, our tests would help us ensure the implementations match the interface.

• This would help us write better unit tests against the struct.
• This would make the implementation consistent.

We already have mongo, olympus, s3 implementation properly, mem, fs, minio have to be changed. wdyt?

If it sounds good i could pick this up.

[michalpristas]

Go ahead @devdinu

[marwan-at-work]

I initially disagree here (but not strongly)

There are standard library packages that return interfaces instead of structs:
The context package returns interfaces such as context.Background, or context.WithTimeout.

The http.NewFileTransport returns a RoundTripper interface.

Returning an interface sends a clear message to the user about what functionality they have access to from the Storage. For example, users outside of the storage package implementation itself, shouldn't have access to change a struct field of the storage implementation.

Furthermore, if we have to return structs from storage implementations, this means we have to make all the struct-implementations public which increases the API surface area.

Lastly, for testing purposes you can do type-assertion without caring about panics, because they're tests.

[arschles]

I agree with @marwan-at-work here but I'm gonna add a little more from personal opinion.

Generally I think and have seen returning structs can help the caller if the struct implements some functionality beyond the interface that it implements. Otherwise, if you return a struct that implements the interface and nothing more, you have to document that the struct implements the interface and you don't gain much. Regarding tests also, the compiler checks that a struct implements the interface, so you don't have to write a test for it (and as a sidenote, you don't have to do some things like var _ MyInterface = &myStruct{} - some codebases do this). To get access to the underlying storage implementation in the test, you do a type assert (i.e. theStruct, ok := theInterface.(*myStruct)). This first gives you the ability to inspect the implementation struct, but also adds a test to make sure the constructor function returns the right underlying type, which can be important too.

[michalpristas]

Just to add a few cents here, probably 1.
I've seen patterns where interfaces are defined on the client side. The client defines what is the main point of interest and build on top of that. In such a case, returning structs might come in handy as well

[devdinu]

@marwan-at-work Agreed i don't prefer exposing the struct either as it would bring direct dependencies and hard to change. Then looking at the code, instead of proxy/storage.go GetStorage() storage.Backend we could move that behaviour to pkg/storage, as it's that package responsibility.

New(storageType string) storage.Backend, we could have non exported package specific storage struct in implementations, also tests doesn't have to cast to verify, and the behaviour of New storage could be tested.

This looks like a middle ground, and If its valuable to do we can do it if not could park for now, since everyone have different opinions.

[marwan-at-work]

@devdinu if you move GetStorage() storage.Backend to pkg/storage, then it won't compile because you'd create import cycles. GetStorage() imports various storage implementations, and all storages import pkg/storage.

As for testing, I don't believe it's necessary to test an initializer unless it had some runtime business logic (as opposed to boot-time which our initializers are just boot-time AFAIK).

Lastly, our storage implementations should all share the same exact tests because we're testing interface-compliance and not each storage logic separately. Right now, that is storage_tests but there's a lot of room for improvement as we discussed in our last Dev Meeting (https://github.com/gomods/athens/issues/488)

If we want to test an implementation-specific feature, we can write those tests inside the package implementation and then we can use the private struct directly (or cast to it without safety-checks because we're 100% sure of the type)

I'm going to close this for now just so we don't leave it hanging, but feel free to open again or keep discussing.