Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oneOf doesn't really make sense in other languages #573

Closed
jessfraz opened this issue Jan 3, 2022 · 49 comments
Closed

oneOf doesn't really make sense in other languages #573

jessfraz opened this issue Jan 3, 2022 · 49 comments

Comments

@jessfraz
Copy link
Contributor

jessfraz commented Jan 3, 2022

we had discussed this on: oxidecomputer/dropshot#126

but DiskState, the way it is being parsed creates a one of, so when trying to make a nice idiomatic Go client this super sucks. can we try not do things that are very rust-ism but don't apply nicely in other languages

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

Just to be verbose this is on more than just DiskState it is the following:

RouteDestination
RouteTarget
SagaState
VPCFirewallRuleTarget
VPCFirewallRuleHostFilter

@smklein
Copy link
Collaborator

smklein commented Jan 3, 2022

I'm trying to parse the result of oxidecomputer/dropshot#126 - I was under the impression that we concluded:

  • Having data associated with a single enum variant is handy
  • There are many ways the variants can be serialized to JSON (tagged vs untagged)
  • oneOf seems like the right way to represent these enums, and attach variant-specific data

I'm kinda having a gut reaction of "if oneOf doesn't make sense in any languages (other than Rust), why is it a part of OpenAPI?!?", but digging around, it's unclear to me what portion of oneOf / anyOf / allOf we consider fair game to use: (See OpenAPITools/openapi-generator#10514 , in particular the associated bugs under "Additional context").

I'm not sure what the right next step is - it kinda seems to me like we're generating a "technically correct" spec, and it should be up to the language-specific generators to do the right thing.

  • What's wrong with the generated Go client today? Is this in the realm of "fixable" or "so broken we gotta completely restructure the API"?
  • Should we bother trying to improve the language-specific generators, or is this off the table?
  • If we do determine a portion of OpenAPI is so-broken-as-to-be-unusable, we should make it an error to generate (or perhaps at least a warning? @ahl I'm curious for your input here).

We can always change the API exposed by Omicron, but I guess I'm just trying to get a solid confirmation of "what is okay" vs "what is not okay" to create in an OpenAPI spec, so we make this change as few times as possible.

@smklein
Copy link
Collaborator

smklein commented Jan 3, 2022

And, just to be explicit: I'm aware that at the end of the day, generated client bindings are our API, so they should be prioritized over the Rust-specific fit. That being said, these enums do have data that only makes sense with a single variant, so even from an OpenAPI point-of-view, OneOf makes the most sense. If we disallow OneOf - which may be reasonable if the generation is not plausibly fixable - I think we'd benefit from a guideline of "when you want to use OneOf, use ______ instead".

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

its more like oneOf is a huge pain in the ass IMO for generated client and users of said clients, like why make our users suffer when we could just not do weird shit like a rust-ism like ENUM type

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

its not like its impossible but its gross and we should aim for non-gross clients, clients that are idiomatic in their respective languages...

@david-crespo
Copy link
Contributor

david-crespo commented Jan 3, 2022

I think the anyOf issue in oxidecomputer/dropshot#126 is pretty different from this, because that was about a weird/wrong use of the OpenAPI primitive anyOf to represent something that should really be a oneOf. In this case, we're using the OpenAPI primitive idiomatically and the problem is on the client generation side. I don't think it's right to call OpenAPI oneOf a rust-ism.

The TS generator handles these nicely because it has sum types. I see that Go doesn't have sum types, and there are various workarounds people use to simulate them. Is none of the workarounds good enough?

export type DiskState =
  | { state: 'creating' }
  | { state: 'detached' }
  | { state: 'attaching'; instance: string }
  | { state: 'attached'; instance: string }
  | { state: 'detaching'; instance: string }
  | { state: 'destroyed' }
  | { state: 'faulted' }

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

The way we are writing DiskState is a rust-ism. literally that type of enum is a rust ism! The goal is for our customers to not suffer. I do not want to debate this dumb shit, we want idiomatic clients. period. I don't give a shit about rust-ism or not rust-ism but I want to be able to generate idiomatic clients, end of story.

@david-crespo
Copy link
Contributor

Is this not idiomatic OpenAPI independent of the Rust that generated it? What should it look like instead?

omicron/openapi/nexus.json

Lines 3148 to 3272 in f3e7eb1

"DiskState": {
"description": "State of a Disk (primarily: attached or not)",
"oneOf": [
{
"description": "Disk is being initialized",
"type": "object",
"properties": {
"state": {
"type": "string",
"enum": [
"creating"
]
}
},
"required": [
"state"
]
},
{
"description": "Disk is ready but detached from any Instance",
"type": "object",
"properties": {
"state": {
"type": "string",
"enum": [
"detached"
]
}
},
"required": [
"state"
]
},
{
"description": "Disk is being attached to the given Instance",
"type": "object",
"properties": {
"instance": {
"type": "string",
"format": "uuid"
},
"state": {
"type": "string",
"enum": [
"attaching"
]
}
},
"required": [
"instance",
"state"
]
},
{
"description": "Disk is attached to the given Instance",
"type": "object",
"properties": {
"instance": {
"type": "string",
"format": "uuid"
},
"state": {
"type": "string",
"enum": [
"attached"
]
}
},
"required": [
"instance",
"state"
]
},
{
"description": "Disk is being detached from the given Instance",
"type": "object",
"properties": {
"instance": {
"type": "string",
"format": "uuid"
},
"state": {
"type": "string",
"enum": [
"detaching"
]
}
},
"required": [
"instance",
"state"
]
},
{
"description": "Disk has been destroyed",
"type": "object",
"properties": {
"state": {
"type": "string",
"enum": [
"destroyed"
]
}
},
"required": [
"state"
]
},
{
"description": "Disk is unavailable",
"type": "object",
"properties": {
"state": {
"type": "string",
"enum": [
"faulted"
]
}
},
"required": [
"state"
]
}
]
},

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

I'm saying instead of doing

pub enum DiskState {
(that) make it an object/struct not an enum, that enum does not translate to other languages well, its a rust-ism

@david-crespo
Copy link
Contributor

david-crespo commented Jan 3, 2022

If we were to eliminate the oneOf by doing something like this:

{
  state: 'creating' | 'detached' | 'detaching' | // etc
  instance: Option<Instance>
}

That makes it a much worse representation in the many languages that do have sum types because you don't know at the type level which states have instances and which don't. So it's not a simple question of what's better for clients: if we make Go much better we make Rust and TS much worse.

@jclulow
Copy link
Collaborator

jclulow commented Jan 3, 2022

Would it not be possible to represent the client in such a way that you could use the "type switch" construct? I tried to knock together an example of what I mean with RouteDestination:

package main

import "fmt"

/*
 * RouteDestination
 */
type RouteDestination interface {
	Type() string
}

/*
 * RouteDestination variant "ip":
 */
type RouteDestinationIP struct {
	Ip string
}

func (rds RouteDestinationIP) String() string {
	return fmt.Sprintf("%s:%s", rds.Type(), rds.Ip)
}

func (rds RouteDestinationIP) Type() string {
	return "ip"
}

/*
 * RouteDestination variant "vpc":
 */
type RouteDestinationVPC struct {
	Name string
}

func (rds RouteDestinationVPC) String() string {
	return fmt.Sprintf("%s:%s", rds.Type(), rds.Name)
}

func (rds RouteDestinationVPC) Type() string {
	return "vpc"
}

/*
 * RouteDestination variant "subnet":
 */
type RouteDestinationSubnet struct {
	Name string
}

func (rds RouteDestinationSubnet) String() string {
	return fmt.Sprintf("%s:%s", rds.Type(), rds.Name)
}

func (rds RouteDestinationSubnet) Type() string {
	return "subnet"
}

/*
 * Our consumer:
 */
func main() {
	destinations := []RouteDestination{
		RouteDestinationVPC{Name: "blah"},
		RouteDestinationIP{Ip: "127.0.0.1"},
		RouteDestinationSubnet{Name: "farend"},
	}

	for i, rd := range destinations {
		fmt.Println("[", i, "]", rd, "which is variant type", rd.Type())

		switch rd := rd.(type) {
		case RouteDestinationSubnet:
			fmt.Printf("\ta subnet, \"%s\", my favourite!\n", rd.Name)
		case RouteDestinationIP:
			fmt.Printf("\ta direct IP address, %s!\n", rd.Ip)
		case RouteDestinationVPC:
			fmt.Printf("\ton second thought let's not go to %s, it is a silly place\n", rd.Name)
		}
	}
}

When run, this outputs:

[ 0 ] vpc:blah which is variant type vpc
	on second thought let's not go to blah, it is a silly place
[ 1 ] ip:127.0.0.1 which is variant type ip
	a direct IP address, 127.0.0.1!
[ 2 ] subnet:farend which is variant type subnet
	a subnet, "farend", my favourite!

I put this up on the Go playground as well: https://go.dev/play/p/zKoNbVnoGcR

This would be laborious to type out by hand, of course, but it seems like the code generator could make short work of it. Interfaces and the type switch pattern are mentioned in the Go tutorial, so I imagine they are at least somewhat acceptable?

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

I mean its not that hard either make it an enum of all the same type (strings) or make it a struct where there's a nested enum and the attaching/detaching shit there. The way it is today just creates weird shit for all other languages

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

@jclulow why do that when we could just have an object/struct instead

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

honestly the type checking thing is fucking gross imo

@smklein
Copy link
Collaborator

smklein commented Jan 3, 2022

honestly the type checking thing is fucking gross imo

I'm genuinely curious - why? Even within just the context of Go, there seems to be a tradeoff between:

  1. Use type switch constructs - like Josh showed - and have variant-specific data.
  • Pros: Variant-specific data means we're enforcing type safety across more clients - our API is harder to misuse.
  • Cons: We don't like type switches? Looking around, this seemed like a recommended implementation of sum types. Josh mentioned that it's in the Go book, and also recommended more specifically for sum types: https://eli.thegreenplace.net/2018/go-and-algebraic-data-types/
  1. Using a policy that "states can only be represented by the same type (strings)".
  • Pros: Avoids type switching, if that's not favored?
  • Cons: All variant-specific fields become "optional", and are always a part of the API. Which subset of options are applicable to which variant must be interpreted out-of-band from the type system. Notably, this con impacts all generated bindings, including Go.

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022

heres the deal, if one of you volunteers to do the go client (from what I've started https://github.com/oxidecomputer/oxide.go) and the k8s integration, and the hashicorp packer/terraform integration (what consumes the go client) so you can feel the shitty-ness you can have it your way, otherwise I will do those things but I require a nice client before doing it ;)

@davepacheco
Copy link
Collaborator

If I'm understanding right, @jessfraz you're saying that the sum types that we're using in the API spec are Rust-isms that are awkward in all other languages and so we should avoid using them in the API? For what it's worth, sum types aren't Rust-specific -- they've been around since at least the 60s, there are established patterns for using them in languages like C, and they appear to have good support in TypeScript, Swift, Scala, and recent Python. That oneOf is part of OpenAPI suggests there's some use for them in this space.

@jessfraz wrote:

why make our users suffer when we could just not do weird shit like a rust-ism like ENUM type
I mean its not that hard either make it an enum of all the same type (strings) or make it a struct where there's a nested enum and the attaching/detaching shit there. The way it is today just creates weird shit for all other languages
why do that when we could just have an object/struct instead

In case it's not clear why folks prefer to use sum types in the API: the reason to use sum types is that they more precisely specify constraints on a value, which facilitates better type checking and so (we hope) a much better user experience. Without sum types, a client needs to do extra validation on top of what the API spec says. In many languages, it's easy to forget to do that and then introduce bugs (e.g., you forget to check if some null-able field is null and then the program crashes at runtime). The extra validation rules also aren't written down anywhere -- there's no way to really know what you need to check. By foregoing this more precise representation in the API spec, we'd be making it easier to introduce bugs not just in languages that don't support these types well, but also the languages that do, including two that we use heavily within the product (Rust and TypeScript).

I like the way @david-crespo said it above:

That makes it a much worse representation in the many languages that do have sum types because you don't know at the type level which states have instances and which don't. So it's not a simple question of what's better for clients: if we make Go much better we make Rust and TS much worse.

and to @smklein's point above, there are good reasons even in Go that someone might prefer the stronger types.

@jessfraz wrote:

The goal is for our customers to not suffer. I do not want to debate this dumb shit, we want idiomatic clients. period. I don't give a shit about rust-ism or not rust-ism but I want to be able to generate idiomatic clients, end of story.

Is it clear what's idiomatic in Go for this situation? From the links in this thread it seems like at least some folks in Go do use sum type patterns -- enough of them that it's even in the Tour of Go.

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 3, 2022 via email

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022 via email

@jclulow
Copy link
Collaborator

jclulow commented Jan 4, 2022

If it's not acceptable to use the types, then I suspect the client generator could take the union of the properties present in all oneOf variants and present a degenerate struct with effectively optional values; e.g., modifying the RouteDestination example I wrote earlier you could instead end up with:

package main

import "fmt"

/*
 * RouteDestination
 */
type RouteDestination struct {
	Type string /* variant discriminator */
	Ip   string /* used by: "ip" */
	Name string /* used by: "vpc", "subnet" */
}

/*
 * Our consumer:
 */
func main() {
	destinations := []RouteDestination{
		RouteDestination{Type: "vpc", Name: "blah"},
		RouteDestination{Type: "ip", Ip: "127.0.0.1"},
		RouteDestination{Type: "subnet", Name: "farend"},
	}

	for i, rd := range destinations {
		fmt.Printf("[ %d ] %+v which is variant type %s\n", i, rd, rd.Type)

		switch rd.Type {
		case "subnet":
			fmt.Printf("\ta subnet, \"%s\", my favourite!\n", rd.Name)
		case "ip":
			fmt.Printf("\ta direct IP address, %s!\n", rd.Ip)
		case "vpc":
			fmt.Printf("\ton second thought let's not go to %s, it is a silly place\n", rd.Name)
		}
	}
}

To come up with this, the client generator would look at each variant of the oneOf and note its name (subnet, ip, vpc, in this case) which would become the string value of the Type member. Both vpc and subnet appear to contain a Name, so we'd include that in the final struct. The ip one is different, including an ip property instead of a name; we'll include that as well in the final union.

It would be a programming error to produce two variants that include overlapping names with conflicting types, but we could lint for that.

@jessfraz, is that more idiomatic?

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

I could even make the case for rust that doing a match over all the types is quite annoying:

match state {
        DiskState:: Creating => {
            println!("Creating!");
        }
        DiskState:: Detached => (),
        DiskState:: Attaching(uuid) => {
          println!("my uuid is {}", uuid);
        },
       ...
    }

that's a lot of lines of code when I could get the status back as an enum encapsulated in an object with an optional instance, check if the instance is none

@jclulow
Copy link
Collaborator

jclulow commented Jan 4, 2022

Sorry, I left out the output:

[ 0 ] {Type:vpc Ip: Name:blah} which is variant type vpc
	on second thought let's not go to blah, it is a silly place
[ 1 ] {Type:ip Ip:127.0.0.1 Name:} which is variant type ip
	a direct IP address, 127.0.0.1!
[ 2 ] {Type:subnet Ip: Name:farend} which is variant type subnet
	a subnet, "farend", my favourite!

https://go.dev/play/p/itw06wYAo-a

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

@jclulow thats still a crazy amount of code for something that could be much more simple.

@david-crespo
Copy link
Contributor

Can’t you take the flattened approach in the Go client generator based on the oneOf? That would allow other languages to handle it their way.

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

No because the end user is the one that suffers and that is not fair

@jclulow
Copy link
Collaborator

jclulow commented Jan 4, 2022

To be clear, it's just a harness to demonstrate each of the variants. The actual type the generator would produce is:

/*
 * RouteDestination
 */
type RouteDestination struct {
	Type string /* variant discriminator */
	Ip   string /* used by: "ip" */
	Name string /* used by: "vpc", "subnet" */
}

You can do whatever you want with it in your consuming code, obviously; e.g., if you only care about subnets:

if rd.Type == "subnet" {
    /* Do something with rd.Name */
}

@david-crespo
Copy link
Contributor

I don’t understand that. I’m saying can’t you generate the client code you want from the oneOf in the spec? i.e., go through the enum variants and build up a single struct with optional fields

@david-crespo
Copy link
Contributor

david-crespo commented Jan 4, 2022

Oh that looks like what I’m talking about @jclulow

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

@david-crespo you could if all follow the same pattern, but what if in the future we add a type and it doesn't work in that same way, then we created something gross for end-users, like DiskState is kinda special since it can be expressed as an object with an enum of strings, but how do I know that other than I know that it can be expressed that way whereas it may not be true for all OneOfs

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

also @jclulow's example assumes all client code writers know the types off the top of their head, that super sucks, where as just checking if instance is null

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

its also non-idomatic as a go dev I'd be like wtf are these guys doing

@jclulow
Copy link
Collaborator

jclulow commented Jan 4, 2022

its also non-idomatic as a go dev I'd be like wtf are these guys doing

I am the first to admit that my Go is not going to win any awards. If you'll forgive my ignorance, can you show me an example of your ideal representation of RouteDestination in some Go code?

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

I'd do something like this, which to be honest is different than what I'd do for DiskState, making generating oneOfs very hard, I will show both:

(ignore the shitty tabbing I am lazy)

Also I'm doing this based on our rust types, which semi sucks because if I wrote the control plane in Go, I'd write the API differently and we'd probably then be in situations (maybe) where the API is too much go-like for generating other languages (I dunno just a hypothesis). Because RouteDesination doesn't really translate the way its written very nicely to go, so like to me this is kinda sucky go.

type RouteDestinationType string

const (
  RouteDestinationIP RouteDestinationType = "ip"
  RouteDestinationVPC RouteDestinationType = "vpc"
  RouteDestinationSubnet RouteDestinationType = "subnet"
}

type RouteDestination struct {
  Type RouteDestinationType `json:"type"`
  IP string `json:"ip,omitempty"`
  Name string `json:"name,omitempty"`
}
type DiskState string

const (
  {define all the string states)
)

// Naming this sucks since honestly the state is technically just the string representation.
type DiskStateData struct {
   State DiskState `json:"state"`
   Instance *Instance `json:"instance"`
}

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022

I also cant seem to find anything that shows me that python will not hit this problem as well

@david-crespo
Copy link
Contributor

Typed Python has union types: https://docs.python.org/3/library/typing.html#typing.Union

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022 via email

@david-crespo
Copy link
Contributor

An example of the kind of developer experience we lose in languages with sum types:

DiskState

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022 via email

@david-crespo
Copy link
Contributor

Right but to convert it out and get data you need numerous lines of code in like a switch like thing

You do, but IMO if you're using Python with types, that would be exactly what you want. If you're using Python without types you can just check if fields are present with hasattr.

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022 via email

@david-crespo
Copy link
Contributor

That wasn't just IDE hints, that was the typechecker. As a TS API consumer, if we got rid of the discriminated union, I would personally have to write runtime boilerplate that would at best only partly make up for the loss of type safety.

@davepacheco
Copy link
Collaborator

Maybe relevant: oneOf is supported in protobufs as well, with some thought given to the generated Go code.

@jclulow
Copy link
Collaborator

jclulow commented Jan 4, 2022

@jessfraz I think I understand what you're after. Keeping in mind that the demo_*() functions are just to demonstrate usage, not part of the client, does this look better?

package main

import (
	"encoding/json"
	"fmt"
)

type DiskStateState string

const (
	DiskStateCreating  DiskStateState = "creating"
	DiskStateDetached  DiskStateState = "detached"
	DiskStateAttaching DiskStateState = "attaching"
	DiskStateAttached  DiskStateState = "attached"
	DiskStateDetaching DiskStateState = "detaching"
	DiskStateDestroyed DiskStateState = "destroyed"
	DiskStateFaulted   DiskStateState = "faulted"
)

type Instance string

type DiskState struct {
	State    DiskStateState `json:"state"`
	Instance *Instance      `json:"instance"`
}

func demo_disk_state() {
	jsons := []string{
		`{"state":"creating"}`,
		`{"state":"attached","instance":"0000001"}`,
		`{"state":"detaching","instance":"0000002"}`,
		`{"state":"faulted","instance":"0000003"}`,
		`{"state":"detached"}`,
	}

	for i, j := range jsons {
		var ds DiskState

		json.Unmarshal([]byte(j), &ds)

		fmt.Printf("[%02d] %+v\n", i, ds)
		if ds.Instance != nil {
			fmt.Printf("\tinstance = %v\n", *ds.Instance)
		}
		if ds.State == DiskStateFaulted {
			fmt.Printf("\toh noes!\n")
		}
		fmt.Printf("\n")
	}
}

type RouteDestinationType string

const (
	RouteDestinationIP     RouteDestinationType = "ip"
	RouteDestinationVPC    RouteDestinationType = "vpc"
	RouteDestinationSubnet RouteDestinationType = "subnet"
)

type RouteDestination struct {
	Type RouteDestinationType `json:"type"`
	IP   string               `json:"ip,omitempty"`
	Name string               `json:"name,omitempty"`
}

func demo_route_destination() {
	jsons := []string{
		`{"type":"ip","ip":"127.0.0.1"}`,
		`{"type":"vpc","name":"camelot"}`,
		`{"type":"subnet","name":"omega"}`,
	}

	for i, j := range jsons {
		var rd RouteDestination

		json.Unmarshal([]byte(j), &rd)

		fmt.Printf("[%02d] %+v\n", i, rd)
		fmt.Printf("\n")
	}
}

func main() {
	fmt.Printf("----------------- Disk State ---------------\n")
	demo_disk_state()
	fmt.Printf("----------------- Route Destination ---------------\n")
	demo_route_destination()
}

Outputs:

----------------- Disk State ---------------
[00] {State:creating Instance:<nil>}

[01] {State:attached Instance:0xc000112170}
        instance = 0000001

[02] {State:detaching Instance:0xc0001121a0}
        instance = 0000002

[03] {State:faulted Instance:0xc0001121d0}
        instance = 0000003
        oh noes!

[04] {State:detached Instance:<nil>}

----------------- Route Destination ---------------
[00] {Type:ip IP:127.0.0.1 Name:}

[01] {Type:vpc IP: Name:camelot}

[02] {Type:subnet IP: Name:omega}

Demo @ https://go.dev/play/p/g6ZUg8RO4YY

If this is acceptable, I think the same basic algorithm could be used to mechanically generate both degenerate structs from the schema that exists today.

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022 via email

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 4, 2022 via email

@smklein
Copy link
Collaborator

smklein commented Jan 4, 2022

Maybe relevant: oneOf is supported in protobufs as well, with some thought given to the generated Go code.

This approach is really interesting - the TL;DR of Go Oneof in Proto is:

On the one hand, they allow Go clients to follow the "strongly type-safe approach":

  • The "top-level" message is a struct, like Message.
  • Each variant is a new struct, named like Message_Variant.
  • They use type switching on values to access specific variant structs.

But they also add a bunch of stuff that lets you avoid the type cast if you wanna ignore it:

  • A bunch of isMyMessage_OneParticularType methods are generated, for both the top-level message + each variant.
  • Getters are generated for all the variant-specific fields - and they can return the "value for that field or the zero value if not set". (IMO this is kiiiiiinda sketchy - it's hard to distinguish the "zero value means zero" or "a zero value means unset". But @jclulow 's pointer-based approach could totally work as an alternative way to implement "optional").

@david-crespo
Copy link
Contributor

Crespo I’m curious what the boiler plate would look like above and beyond a nil check for instance?

In these cases where there's a single nullable field it's not that different, though you might want to do something differently with the instance depending on the state, in which case you're looking at the state anyway.

For cases where there are more fields you actually save code at runtime by using the discriminant property because you only have to check one thing instead of each property separately. That's on top of typechecker benefits.

type SumType =
  | {
      type: "A";
      age: number;
      height: number;
    }
  | {
      type: "B";
      name: string;
      attrs: Record<string, string>;
    };

function doSomething(x: SumType) {
  if (x.type == "A") {
    x.age; // number
    x.height; // number
    // x.name and x.attrs are type errors
  } else {
    x.name; // string
    x.attrs; // record
    // x.age and x.height are type errors
  }
}

TS playground

@david-crespo
Copy link
Contributor

I appreciate the summary @smklein — I was not understanding that page very easily. I think that's a cool approach. It reminds me of what you get by default in dynamic languages with optional types, like what I described happening in Python.

@ahl
Copy link
Contributor

ahl commented Jan 6, 2022

It sounds like our plan here is to retain the precision expressed in the API using oneOf and to then translate that into the appropriate structures for the target language. This might be done by a pre-processing pass through the OpenAPI document (e.g. to translate a oneOf of structs into a single struct) or just inline in the generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants