[Brainstorm] Representing implementation-specific data structures / return values in our Go projects

[thozza]

Background: osbuild/images#1213

In Golang projects, we tend to run into situations where we use a common interface with a set of methods, which is then implemented by multiple implementations. A good example is the new Uploader interface, which needs to be implemented differently for different clouds. However, in some cases, we need the ability to get implementation-specific data after calling a specific method. For example, to get data to identify the image in the target cloud after it has been uploaded, which will look different for each cloud.

So far, we tend to return interface type in such cases, which contains just the IsXXX() function, which is, in turn, implemented by every data structure returned by a specific implementation. Sometimes, we do not necessarily return these structures implementing the interface as a function's return value, but we use it to represent diverse data structures. In principle, it is, in my opinion, the same core idea.

A few examples:
https://github.com/osbuild/images/blob/ce45a6fb33f6f686e15df5a4f42c78a07058c31d/pkg/osbuild/stage.go#L16-L19
https://github.com/osbuild/images/blob/ce45a6fb33f6f686e15df5a4f42c78a07058c31d/pkg/osbuild/input.go#L4-L6
https://github.com/osbuild/images/blob/ce45a6fb33f6f686e15df5a4f42c78a07058c31d/pkg/osbuild/mount.go#L12-L14
https://github.com/osbuild/osbuild-composer/blob/74638a97338f7681db426eccdc35a9d1be921e19/internal/target/target.go#L47-L49
https://github.com/osbuild/osbuild-composer/blob/74638a97338f7681db426eccdc35a9d1be921e19/internal/target/targetresult.go#L26-L28
https://github.com/osbuild/osbuild-composer/blob/74638a97338f7681db426eccdc35a9d1be921e19/internal/weldr/upload.go#L31-L33
https://github.com/osbuild/osbuild-composer/blob/74638a97338f7681db426eccdc35a9d1be921e19/internal/upload/koji/koji.go#L115-L117

@mvo5 does not like it, and I can't blame him. Although I'm not a proponent of this approach, personally I find it acceptable in warranted cases.

Let's have a short brainstorming discussion to hopefully find a nicer way to approach this problem consistently in all of our projects. Or if not a nicer way, let's at least have a common understanding of which approach we want to use in which case.

[mvo5]

Fwiw, I'm not sure we can solve this universally and I am not sure we can/should change the existing code. Some of the examples are architected with the pattern in mind and changing that would be hard (and requires some serious digging).

But for new greenfield code I would like to avoid it, let me explain what I do not like about the approach. I have a (relatively) strong opinion here and I want to apologize in advance if I step on any toes.

Lets take the Uploader interface:

type Uploader interface{
    func UploadAndRegister() error
}

for this we want to also support returning structured data. For an aws cloud it would be AMI name for GCP a URL.
Fwiw, I think we could architect the Uploader differently and just put the result in a different step (not too unusal,
a bufio.Scanner also has a "scanner.Scan()" and when that is done a "scanner.Err()" to check for results, we could
do something similar here outside the Uploader interface [0], but I digress).

Now with the IsFoo() pattern we would do something like:

type UploaderResult inteface{
   func IsUploadResult()
}

type Uploader interface{
    func UploadAndRegister() (UploadResult, error)
}

type awsUploadResult struct {
  ami string
}

func (*awsUploadResult) IsUploadResult() {}

type googleCloudUploadResult struct {
  url string
}

func (*googleCloudUploadResult) IsUploadResult() {}

my concern is that we actually gain very little type safety with this approach and the cost is that we have these empty methods that add (IMHO) very little value and look very out of place in go projects. Why little value? Because

func (* awsUploader) UploaderAndRegister() (UploadResult, error) {
   ...  
   return &googleCloudResult{URL: "no-compilter-will-complain"}
}

is still perfectly valid. We protect against returning the wrong class of datatypes but not the wrong data. And the caller needs to cast to the right type anyway and check that it is correct (just like with an interface{}).

And the call side still needs to do:

   res, err := awsUploader.UploadAndRegister()
   ...
   awsRes, ok := res.(*awsResult)
   if !ok {
     return fmt.Errorf("internal error: ....")
  }

I think if we go down this path, we should simply return an interface{} and require unit tests that check that each return path in the thing that returns an interface{} is of the right type (just like one has to do with python where there is no type safety by default). This is annoying but we have to do the same with the above approach because we just don't know if the right type is returned (it is less likely).

I guess at the end of the day it comes down to a choice like: is the value of (IMHO little) type safety from the IsFoo() pattern bigger than:

1. aesthetic unpleasantness (a big one for me)
2. (un)familiarity to (most?) go developers
3. extra boiler plate
4. returning something that is an interface{} and needs casting but "dressing it" differently

To me the answer is an unequivocally "no - its not worth it" but I see of course that there is a lot of subjectivity in opinions like this.

[0] osbuild/images#1213

[achilleas-k]

I'm not a fan of the isInterfaceType() pattern either, but I do think it's solvable. Not with a single universal rule, though. We'll probably need to consider each case separately.

I am not sure we can/should change the existing code.

I think we should, or at least try. Osbuild modules and their options, where this is most prominent, probably have a common usage pattern we can build around. I could be very wrong, but in my mind the reason we stuck with the isInterfaceType() pattern there was because we never considered the alternative seriously enough to design it. Though, those aren't directly relevant to this discussion because dealing with an interface return type makes things a bit more complicated.

For the uploader, I'm going to link back to my own comment from a couple of weeks ago (quoting relevant part):

I'd like to see this common interface happen, but I think it makes more sense to look at the other cloud providers and consider what it might look like when we add those to the mix. UploadAndRegister() sounds generic enough but I don't know if it makes sense for Azure, or GCP, or Oracle (it probably does).

I think the same goes for the return type. I have to wonder, what would the structured result return types look like for each uploader and how would we use them? What do we do with them? Are there no common patterns at all? If there aren't, then maybe a common interface doesn't make sense at all.

The TL;DR here is: I don't think it's too big of a task to look at each instance of this problem separately.

[lzap]

I will comment on the Uploader use case because I know nothing about stages and what those IsSomething interfaces do, looks fishy but I won't comment on that :-)

I woud start with a idiomatic way of building Go interfaces and split the UploadAndRegister into two separate interfaces: Uploader and Registrator. But this is a minor thing. Here is the big deal.

I do not understand what an inteface Result() brings to the table - this is a generalization of what exactly? Every single implemetnation will likely have a different input and output, what should be generalized is the process not data. The Go idiomatic way of doing this is:

u := AWSUploader{
  InputData1: "input data",
}

u.Upload(imageReader, progressBar)

println(u.OutputData1, u.OuputData2)

Note I intentionally left out a "getter" here, there is no point for that.

Now, I understand that some generalization can be useful for example to be able to print the results in a common way, let me assume that this is needed for the CLI binary, then I am assuming those values will be somewhat printed to the console. In that case, I would think about perhaps key-value pairs that can be nicely formatted and shown to the user. In this scenario, creating a getter does make sense because a result can be created either via upload, or via registration or perhaps both. Tho I slightly lean towards returning slice of key-value pairs from both operations and combining them together. Something in the ballpark of:

type Result struct {
 Name string
 Value string
}

type Uploader struct {
 Upload(...) ([]Result, error)
}

type Registrator struct {
 Register(...) ([]Result, error)
}

The caller can now call both methods and collect the results as needed. Since these values end up in a terminal, strings are probably okay but if needed, values can be generalized. A nice example is the built-in log/slog interface from the stdlib and its slog.Value generalization tho this is perhaps stretched way to far.

The result type can be more sophisticated and details hidden, what matters is a printing interface that can be different for various environments - key/values as strings perhaps suits CLI, there might be a different "renderer" returning data in other forms. But I would rather keep it simple.

I can imagine CLI output like:

Build finished, registered the following:

AMI-ID: ami83427934827423
AMI-Name: a-test-image
AMI-Region: eu-north-7

[thozza]

FWIW, I would leave any user presentation out of designing any internal API (I mean internal to the application). How will the results be presented to users, and if at all, should be IMHO left to the caller. IOW image-builder-cli, osbuild-composer, etc...

[lzap]

Generalized data via interfaces must have a sensible operation (e.g. render them down to something). I do not believe a generalization to some very common format (e.g. any with type casting or JSON string) makes sense, it makes things worse. In that case, I lean towards not trying to generalize at all and just pick up the results from the struct value directly while keeping the types intact.

In the end, you will have to do switch/case it is either before Upload operation is called (which is I think much better and safer) than after getting an any value and type checking it. That is my point. Best of all, one can combine both approaches - results can be easily picked up from the struct while rendered for users.

[mvo5]

Fwiw, it is currently "UploadAndRegister" (instead of two calls) so that there is a common code path in the library to do the cloud specific removal of the temporary image if anything fails along the way. This could be done in different ways but then the burden would be on the caller to ensure that no data is left if an import fails. But its a tradeoff again, it might be good give this control, for ibcli and bib it seems unnecessary as it only wants to upload/register, nothing else.

[ondrejbudai]

Maybe we shouldn't call it UploadAndRegister, just Upload could be enough. From the point of the caller, it should basically be an atomic operation: either it fails, or suceeds. In both cases all intermediate resources are removed. The fact that AWS uploader goes via S3 is rather an implementation detail to the caller I think.

[croissanne]

I think I agree that there's no universal better alternative to this pattern. And I also agree that
it's a weird pattern.

One area where we use this pattern is when serialisation is involved, and there I don't know if
there's a better pattern.

type Target struct {
    …
	Options TargetOptions `json:"options"`
	…
}
…
type TargetOptions interface {
	isTargetOptions()
}

The unmarshal function for Target is annoying:
https://github.com/osbuild/osbuild-composer/blob/5cd2d8a2a9d7773892b9ac13b8282c0514c7375d/internal/target/target.go#L62-L308

It's easy to forget to add you target to this switch statement. Say the TargetOptions interface is
changed so the UnmarshalJSON function on Target doesn't need to care about the specific options:

type TargetOptions interface {
    UnmarshalJSON(data []byte) error
}

And then you'd end up with something like:
https://gist.github.com/croissanne/37d92cf94bfb533cad8380cc0d35e37e

This just moved the problem however, because this would put the onus on the caller to pass the
correct options struct for the correct target. So there the caller needs to have the awkward switch
statement. When the unmarshal of the top level struct determines the implementation of the
isFoobar() interface, this pattern can make sense at least.

[croissanne]

I think with serialisation it's just hard to get rid of because of the fact that type information and interfaces and all that obviously aren't represented in json. But maybe when it stays contained within the go code there would be better ways. Again just showing it's not a universal decision / project wide.