Dropshot does not allow you to rename path variables across versions

[jgallagher]

I bumped into this in omicron, and I'm not sure if I'm holding it wrong or if this is a dropshot bug. If I modify the versioning example to take a path argument, but attempt to change the name of the field inside that path between the two versions, like this:

diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs
index fca2bdd..034ea84 100644
--- a/dropshot/examples/versioning.rs
+++ b/dropshot/examples/versioning.rs
@@ -103,11 +103,13 @@ use dropshot::ConfigLogging;
 use dropshot::ConfigLoggingLevel;
 use dropshot::HttpError;
 use dropshot::HttpResponseOk;
+use dropshot::Path;
 use dropshot::RequestContext;
 use dropshot::ServerBuilder;
 use dropshot::VersionPolicy;
 use http::HeaderName;
 use schemars::JsonSchema;
+use serde::Deserialize;
 use serde::Serialize;

 #[tokio::main]
@@ -207,14 +209,20 @@ mod v1 {
         thing1_early: &'static str,
     }

+    #[derive(Deserialize, JsonSchema)]
+    struct Thing1Arg {
+        arg: String,
+    }
+
     /// Fetch `thing1`
     #[endpoint {
         method = GET,
-        path = "/thing1",
+        path = "/thing1/{arg}",
         versions = "1.0.0"..TWO_DOT_OH
     }]
     pub async fn get_thing1(
         _rqctx: RequestContext<()>,
+        _path: Path<Thing1Arg>,
     ) -> Result<HttpResponseOk<Thing1>, HttpError> {
         Ok(HttpResponseOk(Thing1 { thing1_early: "hello from an early v1" }))
     }
@@ -231,14 +239,20 @@ mod v2 {
         thing1_late: &'static str,
     }

+    #[derive(Deserialize, JsonSchema)]
+    struct Thing1Arg {
+        myarg: String,
+    }
+
     /// Fetch `thing1`
     #[endpoint {
         method = GET,
-        path = "/thing1",
+        path = "/thing1/{myarg}",
         versions = "2.0.0"..
     }]
     pub async fn get_thing1(
         _rqctx: RequestContext<()>,
+        _path: Path<Thing1Arg>,
     ) -> Result<HttpResponseOk<Thing1>, HttpError> {
         Ok(HttpResponseOk(Thing1 { thing1_late: "hello from a LATE v1" }))
     }

Attempting to run the example (via either run or openapi) induces this panic:

thread 'main' (762450) panicked at /home/john/github/dropshot/dropshot/src/router.rs:314:33:
URI path "/thing1/{myarg}": attempted to use variable name "myarg", but a different name ("arg") has already been used for this
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/01f6ddf7588f42ae2d7eb0a2f21d44e8e96674cf/library/std/src/panicking.rs:689:5
   1: core::panicking::panic_fmt
             at /rustc/01f6ddf7588f42ae2d7eb0a2f21d44e8e96674cf/library/core/src/panicking.rs:80:14
   2: dropshot::router::HttpRouter<Context>::insert
             at ./dropshot/src/router.rs:314:33
   3: dropshot::api_description::ApiDescription<Context>::register::_register
             at ./dropshot/src/api_description.rs:463:22
   4: dropshot::api_description::ApiDescription<Context>::register
             at ./dropshot/src/api_description.rs:468:9
   5: versioning::main::{{closure}}
             at ./dropshot/examples/versioning.rs:130:9
   6: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/park.rs:284:71
   7: tokio::task::coop::with_budget
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/task/coop/mod.rs:167:5
   8: tokio::task::coop::budget
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/task/coop/mod.rs:133:5
   9: tokio::runtime::park::CachedParkThread::block_on
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/park.rs:284:31
  10: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/context/blocking.rs:66:14
  11: tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/scheduler/multi_thread/mod.rs:89:22
  12: tokio::runtime::context::runtime::enter_runtime
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/context/runtime.rs:65:16
  13: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/scheduler/multi_thread/mod.rs:88:9
  14: tokio::runtime::runtime::Runtime::block_on_inner
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/runtime.rs:370:50
  15: tokio::runtime::runtime::Runtime::block_on
             at /home/john/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.49.0/src/runtime/runtime.rs:342:18
  16: versioning::main
             at ./dropshot/examples/versioning.rs:183:12
  17: core::ops::function::FnOnce::call_once
             at /rustc/01f6ddf7588f42ae2d7eb0a2f21d44e8e96674cf/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[jgallagher]

The analogous change but through Query instead of Path works fine.

[jgallagher]

EDIT: The "workaround" below blows up at runtime:

% curl -H 'dropshot-demo-version: 1.0.0' http://127.0.0.1:12345/thing1/foo
{"thing1_early":"hello from an early v1"}
% curl -H 'dropshot-demo-version: 2.0.0' http://127.0.0.1:12345/thing1/foo
{
  "request_id": "9976af22-6e18-4a22-9dd9-4a7e4cbfa0d3",
  "message": "bad parameter in URL path: missing field `myarg`"
}

---

Found a workaround way to get it to compile that blows up at runtime, don't do this:

1. Change the old version's endpoint path to match the new version's field name
2. Annotate the old verision's field with #[schemars(rename = "new_field_name)]

That means in the diff, we'd keep v2 as-is and change v1 to:

    #[derive(Deserialize, JsonSchema)]
    struct Thing1Arg {
        #[schemars(rename = "myarg")]
        arg: String,
    }

    /// Fetch `thing1`
    #[endpoint {
        method = GET,
        path = "/thing1/{myarg}",
        versions = "1.0.0"..TWO_DOT_OH
    }]
    pub async fn get_thing1(
        _rqctx: RequestContext<()>,
        _path: Path<Thing1Arg>,
    ) -> Result<HttpResponseOk<Thing1>, HttpError> {
        Ok(HttpResponseOk(Thing1 { thing1_early: "hello from an early v1" }))
    }

This doesn't produce any errors and gives the 2.0.0 OpenAPI spec we want. It does change the 1.0.0 OpenAPI spec, but I believe since this is a path arg this is a wire-compatible / drift-compatible change. We get the new field name (myarg) here when we would have previously gotten just arg:

  "paths": {
    "/thing1/{myarg}": {
      "get": {
        "summary": "Fetch `thing1`",
        "operationId": "get_thing1",
        "parameters": [
          {
            "in": "path",
            "name": "myarg",
            "required": true,
            "schema": {
              "type": "string"
            }
          }
        ]

[davepacheco]

Here's the comment:

dropshot/dropshot/src/router.rs

Lines 310 to 313 in d707f54

	// Don't allow people to use different names for
	// the same part of the path. Again, this could
	// be supported, but it seems likely to be
	// confusing and probably a mistake.

I believe this code predates versioning and I think I stand by it within an API version. I don't think we want to be defining GET /projects/{project_id} and GET /projects/{id}/vpcs or something like that -- I think it's helpful to enforce that those path parameter names be the same.

What the comment says (unwittingly) applies to versioning, too: we could support this as long as the name used within a version is consistent.

[davepacheco]

I believe doing this correctly requires that Dropshot keep track inside the Router which variable name is used for which version, then make sure it uses the right variable names as it traverses the router to find the right handler. This probably introduces a case where it would bail out much earlier than currently does if given an API version that doesn't exist or where that route doesn't exist. Today, it wouldn't find out until the end of the traversal. With this change, it would find out when it got to any "variable" path component that doesn't have a variable name for the requested version.

[jgallagher]

A functional workaround courtesy of @davepacheco: rename the old type's field and old endpoint's Path argument to match the desired, new version:

diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs
index fca2bdd..c1f47b3 100644
--- a/dropshot/examples/versioning.rs
+++ b/dropshot/examples/versioning.rs
@@ -103,11 +103,13 @@ use dropshot::ConfigLogging;
 use dropshot::ConfigLoggingLevel;
 use dropshot::HttpError;
 use dropshot::HttpResponseOk;
+use dropshot::Path;
 use dropshot::RequestContext;
 use dropshot::ServerBuilder;
 use dropshot::VersionPolicy;
 use http::HeaderName;
 use schemars::JsonSchema;
+use serde::Deserialize;
 use serde::Serialize;

 #[tokio::main]
@@ -207,14 +209,20 @@ mod v1 {
         thing1_early: &'static str,
     }

+    #[derive(Deserialize, JsonSchema)]
+    struct Thing1Arg {
+        myarg: String,
+    }
+
     /// Fetch `thing1`
     #[endpoint {
         method = GET,
-        path = "/thing1",
+        path = "/thing1/{myarg}",
         versions = "1.0.0"..TWO_DOT_OH
     }]
     pub async fn get_thing1(
         _rqctx: RequestContext<()>,
+        _path: Path<Thing1Arg>,
     ) -> Result<HttpResponseOk<Thing1>, HttpError> {
         Ok(HttpResponseOk(Thing1 { thing1_early: "hello from an early v1" }))
     }
@@ -231,14 +239,20 @@ mod v2 {
         thing1_late: &'static str,
     }

+    #[derive(Deserialize, JsonSchema)]
+    struct Thing1Arg {
+        myarg: String,
+    }
+
     /// Fetch `thing1`
     #[endpoint {
         method = GET,
-        path = "/thing1",
+        path = "/thing1/{myarg}",
         versions = "2.0.0"..
     }]
     pub async fn get_thing1(
         _rqctx: RequestContext<()>,
+        _path: Path<Thing1Arg>,
     ) -> Result<HttpResponseOk<Thing1>, HttpError> {
         Ok(HttpResponseOk(Thing1 { thing1_late: "hello from a LATE v1" }))
     }

It works at runtime:

% curl -H 'dropshot-demo-version: 1.0.0' http://127.0.0.1:12345/thing1/foo
{"thing1_early":"hello from an early v1"}
% curl -H 'dropshot-demo-version: 2.0.0' http://127.0.0.1:12345/thing1/foo
{"thing1_late":"hello from a LATE v1"}

It does mean the generated 1.0.0 spec changes, in that we see the new field name:

  "paths": {
    "/thing1/{myarg}": {
      "get": {
        "summary": "Fetch `thing1`",
        "operationId": "get_thing1",
        "parameters": [
          {
            "in": "path",
            "name": "myarg",
            "required": true,
            "schema": {
              "type": "string"
            }
          }
        ],

However, this is a wire-compatible change, and if using drift to confirm no wire-incompatible changes (as omicron does), it already throws away Path variable names and only keeps indices (which makes sense, since these names don't show up on the wire at all): https://github.com/oxidecomputer/drift/blob/13294362c5863e85b4bac48b0c4791e69dfa0042/src/operations.rs#L136-L150