Go: Get rid of *Commands interfaces

[marcind]

I would like to propose getting rid of (or at least de-emphasizing) all the *Commands interfaces in the GO api, such as GlideClientCommands, BaseClient, or pretty much any other interface they embed.

Having the client constructors return interfaces is not idiomatic Go; the preference is to "accept interfaces but return structs". See https://go.dev/wiki/CodeReviewComments#interfaces

This is problematic for the following reasons

• Adding a new method to an interface (e.g. to handle a new command that is introduced in a new server release) is a breaking change for any external code that implements the interface.
• The Go ecosystem is not optimized for large interfaces. While you can link to the documentation of a concrete method, you cannot link to one defined on the interface.
• Only the concrete methods are currently documented in the source code, but an editor such as VSCode will not show that documentation in Intellisense because the symbol type obtained from the constructor is an interface, which currently lacks this documentation. You could duplicate the documentation (either by hand or with tooling) but that introduces unnecessary complexity where none is needed.
• "Go to Definition" editor functionality navigates to the interface declaration; navigating to the actual implementation is difficult. Perhaps it is telling that the project's examples vastly prefer working with the concrete type and almost always cast to it as the first thing they do.
• It requires virtual dispatch, which inhibits compiler optimizations like method inlining, heap escape analysis, etc, leading to slower code.

As a piece of prior art experience, the aws-sdk-go client defined interfaces for all operations, but the redesigned aws-sdk-go-v2 implementation removed them.

[jbrinkman]

Thanks for the feeedback. We originally had all the documentation on the interfaces, but moved it for the reasons you mentioned. I'll look at making this change as part of the current refactor work.

[jbrinkman]

In looking at this, command interfaces breaks down into 2 categories:

1. There are command interfaces for different groupings of commands. These interfaces are only used to categorize different commands and simplify the Client interfaces. Rather than have a single interface that includes 300+ functions, we have a dozen interfaces that inlcude just a handful of functions in each one. These interfaces have no real programatic function. While they could potentially be removed, their value in categorizing and simplifying our main interfaces warrants keeping them.
2. The main interfaces are the BaseClientCommands, GlideClientCommands and GlideClusterClientCommands. These interfaces are needed for implementing polymorphism for GlideClient and GlideClusterClient. While they are not likely to be used in user applications, they are valuable when writing tests so that we can easily test both the GlideClient and GlideClusterClient with a minimum of code duplication. We are able to reuse many tests to handle both the standalone and cluster scenarios.

While we weren't able to find a reasonable alternative to using the command interfaces, we were able to ensure that we only use those interfaces in cases where polymorphism is needed. We have minimized using them as function return types, except where truly warranted and have preferred concrete return types wherever possible.

I think this approach is still in keeping with idiomatic Go, while also providing the flexibility that we need in our testing logic to keep the test framework as simple as possible.

[marcind]

While we weren't able to find a reasonable alternative to using the command interfaces, we were able to ensure that we only use those interfaces in cases where polymorphism is needed. We have minimized using them as function return types, except where truly warranted and have preferred concrete return types wherever possible.

@jbrinkman it looks like NewGlideClient and NewGlideClusterClient are still returning the interface. Would you consider having those return the struct pointer instead? That's my biggest issue with the current usage of interfaces (because of how it affects documentation/intellisense)

I think this approach is still in keeping with idiomatic Go, while also providing the flexibility that we need in our testing logic to keep the test framework as simple as possible.

If the interfaces are only used for in-module tests (they appear to be, based on some spot-checking I just did) then would it be acceptable to only include them in the test packages/files? If the command interfaces remain publicly exported, what will happen when the server adds a new command to a given family (e.g. like Redis did with HEXPIRE and friends for HashCommands)? Adding anything to BaseCommands (directly or via an embed) would technically be a breaking change for anyone who implements the interface.

[jbrinkman]

All of the interface changes are contained in #3856. Interfaces were moved to internal folders since they are really not intended for public use. Where an interface could not be moved to internal due to cyclic dependency issues, it was moved to the Glide test suite where it is used, which is inline with the Interface documentation you linked to above. We are removing the existing JSON module code from the 2.0 release since it is targeted for delivery in 2.1. This is the only place where interfaces were used in Public APIs. When we reintroduce the JSON module, we'll address the interface usage as I think it's not needed to implement the JSON module.