Enums unusable as path params due to not deriving Display

[zephraph]

In oxidecomputer/omicron#1957 I'm experimenting with a new API approach as laid out in RFD-322. In this approach we have at most one path param that may be a Name or Uuid type.

Example endpoint

#[derive(Serialize, Deserialize, Display)]
#[display("{0}")]
pub enum NameOrId {
    Name(Name),
    Id(Uuid),
}

/// Path parameters for Instance requests
#[derive(Deserialize, JsonSchema)]
struct InstanceLookupPathParam {
    instance: NameOrId,
}

#[endpoint {
    method = GET,
    path = "/v1/instances/{instance}",
    tags = ["instances"],
}]
async fn v1_instance_view(
    rqctx: Arc<RequestContext<Arc<ServerContext>>>,
    path_params: Path<InstanceLookupPathParam>,
) -> Result<HttpResponseOk<Instance>, HttpError> {
  ...
}

When nexus.json is first generated it succeeds with an openapi spec that seems reasonable to me.

Spec example

{
 "/v1/instances/{instance}": {
      "get": {
        "tags": [
          "instances"
        ],
        "operationId": "v1_instance_view",
        "parameters": [
          {
            "in": "path",
            "name": "instance",
            "description": "If Name is used to reference the instance you must also include one of the following qualifiers as query parameters: - `project_id` - `project_name`, `organization_id` - `project_name`, `organization_name`\n\nIf Id is used the above qualifiers are will be ignored",
            "required": true,
            "schema": {
              "$ref": "#/components/schemas/NameOrId"
            },
            "style": "simple"
          }
        ]
  },
  "components": {
    "schemas": {
      "NameOrId": {
          "oneOf": [
            {
              "$ref": "#/components/schemas/Name"
            },
            {
              "type": "string",
              "format": "uuid"
            }
          ]
        }
     }
  }
}

After nexus.json is updated though, the progenitor macro fails with the following error:

error[E0599]: `NameOrId` doesn't implement `std::fmt::Display`
   --> oxide-client/src/lib.rs:7:1
    |
7   | / progenitor::generate_api!(
8   | |     spec = "../openapi/nexus.json",
9   | |     interface = Builder,
10  | |     tags = Separate,
11  | | );
    | | ^
    | | |
    | | method `to_string` not found for this enum
    | | `NameOrId` cannot be formatted with the default formatter
    | |_doesn't satisfy `NameOrId: ToString`
    |   doesn't satisfy `NameOrId: std::fmt::Display`
    |
    = note: the following trait bounds were not satisfied:
            `NameOrId: std::fmt::Display`
            which is required by `NameOrId: ToString`
note: the following trait must be implemented
   --> /Users/just-be/.rustup/toolchains/nightly-2022-09-27-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:755:1
    |
755 | pub trait Display {
    | ^^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `progenitor::generate_api` (in Nightly builds, run with -Z macro-backtrace for more info)

The NameOrId type generated via progenitor doesn't derive Display but when we're building out send endpoints for routes the URL is constructed with generated code like this:

::core::fmt::ArgumentV1::new_display(
    &encode_path(&instance.to_string()),
),

instance in the above case is the NameOrId type (or at least a wrapper around that type).

Deriving display for single member enums in progenitor would make this usecase possible.

[jclulow]

I'm curious about how deserialisation of the enum in the webserver itself works. I would kind of expect that you'd need that enum to be #[serde(untagged)]? And that the Id(Uuid) would have to be somehow marked as the preferred serialisation because Name(String) will basically always work if it's doing the untagged "try each one until it fits" heuristic thing.

[zephraph]

Progenitor actually generates the internal type w/ untagged so yes, I believe you're right there.

    #[serde(untagged)]
    pub enum NameOrId {
        Variant0(Name),
        Variant1(uuid::Uuid),
    }

Some context here is that Uuid isn't a valid Name (we explicitly disallowed that for exactly this reason), but that said I should probably switch them around given that Uuid should probably have priority.

[zephraph]

Ironically the order of the enum isn't preserved on serialization so the point is moot. Regardless, it'll work in either direction so we're covered on that bit.

[ahl]

I think we'll want to insert "label hints" for the two variants as we do for IpRange in Nexus:
https://github.com/oxidecomputer/omicron/blob/ef6a5a5b0bb1c45333d0e38986e2899cf0c58b84/nexus/types/src/external_api/shared.rs#L170

Regardless, progenitor can add yet another special case trait impls / derive macros to handle this situation. The generalized case will need to consider the full type graph with cycles etc.

[ahl]

I'm curious about how deserialisation of the enum in the webserver itself works. I would kind of expect that you'd need that enum to be #[serde(untagged)]? And that the Id(Uuid) would have to be somehow marked as the preferred serialisation because Name(String) will basically always work if it's doing the untagged "try each one until it fits" heuristic thing.

Arguably a bug: we just derive Deserialize for Name with no checks:
https://github.com/oxidecomputer/omicron/blob/56a586d367bc63f72ed7c3006c781952f55c1a85/common/src/api/external/mod.rs#L133

Presumably, the Derserialize impl should invoke the TryFrom<String> or FromStr impls.