Generated client for ExternalIpCreate feels clunky

[iliana]

type_ is awkward here. What I would expect is to be able to write something like ExternalIpCreate::Ephemeral { pool_name: None }; is the pool name relevant for all future kinds of external IP creation objects?

If we want to leave the shape the same I would recommend renaming type to kind.

cc @bnaecker

[bnaecker]

I think this is due to interaction with the generator, either because we're using an externally-tagged type or how the generator consumes them. The native type definition is indeed what you were hoping to write. The tag-name type is the most common choice in Omicron today (though not the only one), and I think renaming it to kind is perfectly fine.

Also, I recall some discussion a while ago about generating enum representations in a way that feels at least passable in a number of languages. All I can find at the moment is #573, but @ahl might have a better memory for that.

[ahl]

Progenitor usually does a good job of reconstructing enums. This is a case I hadn't considered: an enum with a single variant. Here's what the OpenAPI JSON looks like:

      "ExternalIpCreate": {
        "description": "Parameters for creating an external IP address for instances.",
        "oneOf": [
          {
            "description": "An IP address providing both inbound and outbound access. The address is automatically-assigned from the provided IP Pool, or all available pools if not specified.",
            "type": "object",
            "properties": {
              "pool_name": {
                "nullable": true,
                "allOf": [
                  {
                    "$ref": "#/components/schemas/Name"
                  }
                ]
              },
              "type": {
                "type": "string",
                "enum": [
                  "ephemeral"
                ]
              }
            },
            "required": [
              "type"
            ]
          }
        ]
      },

For quite awhile, typify has made the assumption that a lone subschema can just be treated as a wrapper around the subordinate schema. This is fairly common construction... we even see it above with the $ref for Name!

We can fix this by first seeing if the type fits one of our more attractive enum types (i.e. not untagged) and only failing that, seeing if there's a singleton subschema.

Note however that these two enums emit the same JSON schema:

#[derive(JsonSchema)]
#[serde(tag = "type")] // internal tagging
pub enum ExternalIpCreate {
    Ephemeral { pool_name: Option<Name> },
}

#[derive(JsonSchema)]
#[serde(tag = "type", content = "pool_name")] // adjacent tagging
pub enum ExternalIpCreate {
    Ephemeral(String),
}

Typify needs to prefer one or the other when interpreting the JSON schema, and there's a clear choice because adjacent tagging is a strict subset of internal tagging! Consider this enum:

#[derive(JsonSchema)]
#[serde(tag = "who", content = "what")]
pub enum Friend {
    Bert(String),
    Ernie(String),
}

If we were to prefer to interpret this with internal tagging, we'd reconstruct this:

#[derive(JsonSchema)]
#[serde(tag = "who")]
    Bert { what: String },
    Ernie { what: String },
}

... which would be gross.

This fix for this needs to go into typify and then percolate to progenitor and then omicron.

[ahl]

Here's the original:

/// Parameters for creating an external IP address for instances.
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
#[serde(tag = "type", rename_all = "snake_case")]
pub enum ExternalIpCreate {
    /// An IP address providing both inbound and outbound access. The address is
    /// automatically-assigned from the provided IP Pool, or all available pools
    /// if not specified.
    Ephemeral { pool_name: Option<Name> },
    // TODO: Add floating IPs: https://github.com/oxidecomputer/omicron/issues/1334
}

With the fixes applied, here's how it comes out:

    ///Parameters for creating an external IP address for instances.
    #[derive(Clone, Debug, Deserialize, Serialize)]
    #[serde(tag = "type")]
    pub enum ExternalIpCreate {
        ///An IP address providing both inbound and outbound access. The
        /// address is automatically-assigned from the provided IP Pool, or all
        /// available pools if not specified.
        #[serde(rename = "ephemeral")]
        Ephemeral {
            #[serde(default, skip_serializing_if = "Option::is_none")]
            pool_name: Option<Name>,
        },
    }

Everyone happy?

[bnaecker]

Before expending any effort, I should make clear that this enum will soon have more than one variant. I can add it now, and have the server error out, if it makes the generated client better in the meantime.

…

On 26 Jul 2022, at 20:37, Adam Leventhal ***@***.***> wrote:  Progenitor usually does a good job of reconstructing enums. This is a case I hadn't considered: an enum with a single variant. Here's what the OpenAPI JSON looks like: "ExternalIpCreate": { "description": "Parameters for creating an external IP address for instances.", "oneOf": [ { "description": "An IP address providing both inbound and outbound access. The address is automatically-assigned from the provided IP Pool, or all available pools if not specified.", "type": "object", "properties": { "pool_name": { "nullable": true, "allOf": [ { "$ref": "#/components/schemas/Name" } ] }, "type": { "type": "string", "enum": [ "ephemeral" ] } }, "required": [ "type" ] } ] }, For quite awhile, typify has made the assumption that a lone subschema can just be treated as a wrapper around the subordinate schema. This is fairly common construction... we even see it above with the $ref for Name! We can fix this by first seeing if the type fits one of our more attractive enum types (i.e. not untagged) and only failing that, seeing if there's a singleton subschema. Note however that these two enums emit the same JSON schema: #[derive(JsonSchema)] #[serde(tag = "type")] // internal tagging pub enum ExternalIpCreate { Ephemeral { pool_name: Option<Name> }, } #[derive(JsonSchema)] #[serde(tag = "type", content = "pool_name")] // adjacent tagging pub enum ExternalIpCreate { Ephemeral(String), } Typify needs to prefer one or the other when interpreting the JSON schema, and there's a clear choice because adjacent tagging is a strict subset of internal tagging! Consider this enum: #[derive(JsonSchema)] #[serde(tag = "who", content = "what")] pub enum Friend { Bert(String), Ernie(String), } If we were to prefer to interpret this with internal tagging, we'd reconstruct this: #[derive(JsonSchema)] #[serde(tag = "who")] Bert { what: String }, Ernie { what: String }, } ... which would be gross. This fix for this needs to go into typify and then percolate to progenitor and then omicron. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[ahl]

Meh... turns out Digest had this problem too:

before

    pub struct Digest {
        #[serde(rename = "type")]
        pub type_: DigestType,
        pub value: String,
    }

    #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
    pub enum DigestType {
        #[serde(rename = "sha256")]
        Sha256,
    }

after

    #[serde(tag = "type", content = "value")]
    pub enum Digest {
        #[serde(rename = "sha256")]
        Sha256(String),
    }

[bnaecker]

Fair enough! Glad it was neither a lot of work, nor a one-off for this type. It seems like we'll resolve this with the merger of #1502, correct? Thanks Adam.