Refactor and test enum type creation

[jmcarp]

In thinking through #342, I noticed that the enum type creation logic in createOneOf is a bit hard to follow, and not tested thoroughly. In advance of changing that behavior, I wanted to clean up the existing logic and add some tests. This should make the code more readable and easier to change in the future.

Relates to #350.
Relates to SSE-123.

[jmcarp]

Ready for review when someone needs a break! Assigning @lgfa29 since you've been reviewing the other refactors, but happy for any feedback.

[lgfa29]

What a tricky piece of code to write 😅

I think I get the gist of it, but the OpenAPI spec seems so broad in this area that there may be weird edge cases laying around that I didn't think about. None of them impact our current code generation for Nexus though, so we're good here.

[lgfa29]

I squinted at this for a while and I think there's a potential bug if a property has enum with more than one value, but I'm not sure if a spec like this would even make sense. I think, in general, you would create more variants?

The bug I saw by playing around with the tests is that convertToValidGoType uses propName for the struct name when enumField is empty, and so we end up with two structs with the same name (ImageSource in this case):

@@ -331,8 +332,9 @@ func Test_createOneOf(t *testing.T) {
                                                Value: &openapi3.Schema{
                                                        Type: &openapi3.Types{"object"},
                                                        Properties: map[string]*openapi3.SchemaRef{
-                                                               "type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url"}}},
+                                                               "type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url", "abc"}}},
                                                                "url":  {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
+                                                               "abc":  {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
                                                        },
                                                        Required: []string{"type", "url"},

// ImageSourceType is the type definition for a ImageSourceType.
type ImageSourceType string
// ImageSource is the type definition for a ImageSource.
//
// Required fields:
// - Type
// - Url
type ImageSource struct {
        Abc string `json:"abc,omitempty" yaml:"abc,omitempty"`
        Type ImageSourceType `json:"type" yaml:"type"`
        Url string `json:"url" yaml:"url"`
}

// ImageSourceSnapshot is the type definition for a ImageSourceSnapshot.
//
// Required fields:
// - Id
// - Type
type ImageSourceSnapshot struct {
        Id string `json:"id" yaml:"id"`
        Type ImageSourceType `json:"type" yaml:"type"`
}

// ImageSource is the source of the underlying image.
type ImageSource struct {
        // Abc is the type definition for a Abc.
        Abc string `json:"abc,omitempty" yaml:"abc,omitempty"`
        // Type is the type definition for a Type.
        Type ImageSourceType `json:"type,omitempty" yaml:"type,omitempty"`
        // Url is the type definition for a Url.
        Url string `json:"url,omitempty" yaml:"url,omitempty"`
        // Id is the type definition for a Id.
        Id string `json:"id,omitempty" yaml:"id,omitempty"`
}

// ImageSourceTypeUrl represents the ImageSourceType `"url"`.
const ImageSourceTypeUrl ImageSourceType = "url"

// ImageSourceTypeAbc represents the ImageSourceType `"abc"`.
const ImageSourceTypeAbc ImageSourceType = "abc"

// ImageSourceTypeSnapshot represents the ImageSourceType `"snapshot"`.
const ImageSourceTypeSnapshot ImageSourceType = "snapshot"

But again, I think this is an unrealistic scenario, so probably safe to ignore.

The other thing I noticed is that this path is currently triggered once:

[WARN] TODO: oneOf for "DiskSource" -> "block_size" enum []interface {}{512, 2048, 4096}

But that sounds like a false positive? The generated code looks correct. This is just a bit of noise, so I think safe to ignore if it's hard to avoid it.

[sudomateo]

I was also looking at duplicate struct generation when a pathological OpenAPI specification is created with oneOf variants. I updated the AddressLotKind scheme like so.

{
  "description": "The kind associated with an address lot.",
  "oneOf": [
    {
      "description": "Infrastructure address lots are used for network infrastructure like addresses assigned to rack switches.",
      "type": "string",
      "enum": [
        "infra"
      ]
    },
    {
      "description": "Pool address lots are used by IP pools.",
      "type": "string",
      "enum": [
        "pool"
      ]
    },
    {
      "description": "Testing things.",
      "type": "object",
      "properties": {
        "type": {
          "type": "string",
          "enum": [
            "foo"
          ]
        },
        "foo": {
          "type": "string"
        }
      },
      "required": [
        "allow",
        "type"
      ]
    }
  ]
}

Both the changes on this pull request and the current main changes generated the following duplicate types.

type AddressLotKind string
type AddressLotKindFoo struct {
    // ...
}

[sudomateo]

The other thing I noticed is that this path is currently triggered once:

[WARN] TODO: oneOf for "DiskSource" -> "block_size" enum []interface {}{512, 2048, 4096}

But that sounds like a false positive? The generated code looks correct. This is just a bit of noise, so I think safe to ignore if it's hard to avoid it.

This was an interesting one since it only occurs for this variant of DiskSource even though another variant uses block_size as well.

          {
            "description": "Create a blank disk that will accept bulk writes or pull blocks from an external source.",
            "type": "object",
            "properties": {
              "block_size": {
                "$ref": "#/components/schemas/BlockSize"
              },
              "type": {
                "type": "string",
                "enum": [
                  "importing_blocks"
                ]
              }
            },
            "required": [
              "block_size",
              "type"
            ]
          }

It's because the block_size here directly references the BlockSize schema whereas the other variant uses block_size behind an allOf, like so.

              "block_size": {
                "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096",
                "allOf": [
                  {
                    "$ref": "#/components/schemas/BlockSize"
                  }
                ]
              },

[jmcarp]

I'm going to merge this because I believe the edge cases discussed above (1) don't happen in nexus today and (2) are already present in main. But as we discussed earlier today, there's a lot we can improve here in future patches.