Model single-discriminator, single-value enums as interfaces.

[jmcarp]

This patch updates code generation for oneOf types, specifically for the case when we have exactly one discriminator field and exactly one type-varying value field. In this case, we define an interface type for the oneOf, and a struct wrapper for each of its variant types. Variant structs implement an empty marker method to implement the interface. We also implement custom UnmarshalJSON and MarshalJSON methods for the parent type.

Note: We intentionally don't handle other oneOf types here, but may change them in the future.

[jmcarp]

How to review this PR:

• Read DESIGN.md. I found that it was hard for me (or claude) to think clearly about the enum problem in sdk without breaking it down into separate pieces, so I wrote up a design doc that walks through the different kinds of enums in rust, the openapi schemas they generate, and the go code that I think we should generate. Note that I'm only proposing changes to the specific case where we have a single discriminator field ("type", "kind") and a single, type-varying value field. Other enum types, like untagged enums, aren't changed under this proposal.
• Once we're happy with DESIGN.md, look at the generated code in oxide/types.go. That contains the generated changes under this proposal.
• Once we're happy with the generated code, look at the updated codegen code in internal/. This was fully written by claude, and I'm not going to clean it up until we agree on how the generated code should look.

[lgfa29]

The design and output code LGTM. Just left some minor comments.

[lgfa29]

Just to make sure I understand this part, would PrivateIpStack be an example of this? Where the same field (value) can be one of many types?

If so, would we be able to provide at least some helper methods for now so users don't have to marshal and unmarshal bytes themselves?

[lgfa29]

I think I would lean more towards this approach. Strong typing can be annoying at times, but I think in general it's a net positive.

[sudomateo]

I left some comments and questions for the design. I'm onboard with these changes overall. I wish Go had better support for these types of types.

[sudomateo]

Not storing the discriminator field in the struct makes sense but the natural next question is how do we know what type the raw bytes are when unmarshalling. You answer that here but I think updating the wording to note that we explicitly read the discriminator from the JSON first before then deciding to read the rest of the JSON would be helpful. Perhaps this information is elsewhere already but I'm reading this linearly and making comments as I go.

[sudomateo]

Is this our desired end goal for such types or just a stopgap to maintain the current behavior today before we have time to revisit this?

[jmcarp]

This is a stopgap. I think it would be reasonable to change this to an interface type as well, but I didn't want to overcomplicate this initial PR. I'm down to explore that possibility here if you want, or maybe start a second PR branching off this one so we don't make this too hard to review.

[jmcarp]

Updates:

• I mentioned this on the diff, but I added more discussion about wrapper structs to DESIGN.md: https://github.com/oxidecomputer/oxide.go/pull/359/changes#diff-3dc5dd454e080eb849ee5efaf79df2585fbe0a06804d69249b4c81da05a63875R157. We can almost get rid of them, but there are enough edge cases that I think it's overall clearer to leave them in.
• I added some non-code-generated helpers to implement String() on a few oneOf types whose variants are all effectively strings. Honestly, I'm not sure how much use these will get outside the terraform provider, but it's easy enough to add these methods, so the cost is low.
• I updated our draft pr for the terraform provider to use this branch of the sdk, and the diff is smaller and simpler than before.

From here, I think we have two options:

• Merge this PR into main. The changes only affect a few types, several of which were just added to nexus, and the diff against the terraform provider is small. Arguably the blast radius is small enough that we don't need another long-lived branch.
• Or merge this branch into a long-lived branch like dev or v1.x. This has the advantage of keeping our changes isolated from main, and from external users, but also means we'd want to apply incoming changes to both main and dev.

Honestly, I could go either way on this. The blast radius of this change is actually fairly small, so I'm not sure the extra overhead of another long-lived branch is worth it. But it's also not much work to maintain, so I'm not going to argue against it either.

[sudomateo]

I spent some time working through how to best approach this and ultimately came back to wrapper structs plus these marker interfaces. I wrote up an implementation on the plane, with the notable differences from the approach thus far being the following.

1. Wrapper struct stores the enum value in unexported field, forcing callers to go through a constructor function (not shown in the example code below).
2. Helper methods are exposed on the wrapper struct to retrieve the concrete struct for the enum variant using the comma, ok idiom.

Overall I think this is a clean approach that applies to all enums types, whether each variant is the same type or different types.

Click to expand

package main

import "fmt"

// Exported wrapper struct with unexported interface field. The unexported
// interface field forces external clients to go through a constructor to
// construct this type.
type IPStack struct {
        value IPStackInterface
}

// UnmarshalJSON remains as discussed. Peek at the discriminator and unmarshal
// accordingly.
func (i *IPStack) UnmarshalJSON(b []byte) error {
        return nil
}

// MarshalJSON remains as discussed.
func (i *IPStack) MarshalJSON() ([]byte, error) {
        return nil, nil
}

// This helper method prevents clients from needing to do excessing type
// assertions. It's not any better or worse than type assertions really but it's
// another option and we can generate this.
func (i *IPStack) AsV4() (*IPStackV4, bool) {
        if i.value == nil {
                return nil, false
        }

        v4, ok := i.value.(*IPStackV4)
        return v4, ok
}

// This helper method prevents clients from needing to do excessing type
// assertions. It's not any better or worse than type assertions really but it's
// another option and we can generate this.
func (i *IPStack) AsV6() (*IPStackV6, bool) {
        if i.value == nil {
                return nil, false
        }

        v6, ok := i.value.(*IPStackV6)
        return v6, ok
}

// Marker interface so each variant can be linked to the [IPStack] "enum".
type IPStackInterface interface {
        isIPStack()
}

// The V4 variant.
type IPStackV4 struct {
        IP  string
        Foo string
}

func (i *IPStackV4) isIPStack() {}

// The V6 variant.
type IPStackV6 struct {
        IP  string
        Bar string
}

func (i *IPStackV6) isIPStack() {}

func main() {
        stacks := make([]IPStack, 0)

        v4stack := IPStack{
                value: &IPStackV4{
                        IP:  "1.1.1.1",
                        Foo: "V4",
                },
        }

        v6stack := IPStack{
                value: &IPStackV6{
                        IP:  "::1",
                        Bar: "V6",
                },
        }

        // Just showing that both variants can be treated the same inside some
        // collection. This is obvious since the wrapper struct is, well, a struct but I
        // wanted to showcase it.
        stacks = append(stacks, v4stack, v6stack)

        for _, stack := range stacks {
                printIPStack(stack)
        }
}

// This showcases how a client would access the variants should they choose to
// do so. We could provide helper accessor methods to access popular fields from
// the variants but it's probably just better to have these `As*` helpers to get
// the entire concrete struct for the variant that way the caller has access to
// all the fields.
func printIPStack(i IPStack) {
        if stack, ok := i.AsV4(); ok {
                fmt.Printf("V4 - %v, %v\n", stack.IP, stack.Foo)
                return
        }

        if stack, ok := i.AsV6(); ok {
                fmt.Printf("V6 - %v, %v\n", stack.IP, stack.Bar)
                return
        }
}

[sudomateo]

• I added some non-code-generated helpers to implement String() on a few oneOf types whose variants are all effectively strings. Honestly, I'm not sure how much use these will get outside the terraform provider, but it's easy enough to add these methods, so the cost is low.

In #359 (comment) I showed how we could implement As* methods to fetch the concrete type for an enum variant rather than one-off accessor methods.

Merge this PR into main. The changes only affect a few types, several of which were just added to nexus, and the diff against the terraform provider is small. Arguably the blast radius is small enough that we don't need another long-lived branch.

I think we can merge this into main given that we have a clear pattern and need for this today.

[lgfa29]

(Sorry for the late reply. I just realized that I got distracted and forgot to actually push the button 🤦)

Changes LGTM, just left some minor questions/thoughts/commentrs that I think are safe to ignore. And I also agree that it seems OK to merge this to main.

[lgfa29]

Probably outside of the scope for this PR, but another challenge I faced while using the SDK is some confusion in the generated type names.

For example, PrivateIpStackValue is missing from this code block, but I'm assuming it looks something like this:

type PrivateIpStackValue struct {
  V4 PrivateIpv4Stack
  V6 PrivateIpv6Stack
}

But in isolation, PrivateIpStackValue is a very generic name that doesn't communicate a whole lot. Something like PrivateIpDualStack would probably be better, and match its sibling variants name pattern (PrivateIpv4Stack and PrivateIpv6Stack).

The other variants also have a naming issue, with PrivateIpStackV4 being way too close to PrivateIpv4Stack. Maybe in this case we could add a *Variant suffix to make the variant struct more distinct and add an additional signal that this is a variant out of many options?

Putting it all together, these changes would make the generated code look something like this:

// Variant wrapper types (one per discriminator value)
type PrivateIpStackV4Variant struct {
    Value PrivateIpv4Stack `json:"value"`
}
func (PrivateIpStackV4Variant) isPrivateIpStackVariant() {}

type PrivateIpStackV6Variant struct {
    Value PrivateIpv6Stack `json:"value"`
}
func (PrivateIpStackV6Variant) isPrivateIpStackVariant() {}

type PrivateIpStackDualStackVariant struct {
    Value PrivateIpDualStack `json:"value"`
}
func (PrivateIpStackDualStackVariant) isPrivateIpStackVariant() {}

type PrivateIpDualStack struct {
  V4 PrivateIpv4Stack
  V6 PrivateIpv6Stack
}

But I'm not sure how much of this would be possible to auto-generate based on the spec alone.

[lgfa29]

We don't technically need it for our APIs, but would we need the YAML equivalent for feature completeness?

[jmcarp]

Ok, I'm calling this good enough for the initial implementation! There are other cases I want to handle separately, as noted in DESIGN.md, but I'm going to merge this and start updating our client applications.