OpenAPI has a pretty rich interface to provide examples of path, query, and body inputs and responses.

It would be cool to allow developers to specify these. It could make sense to do this in the endpoint macro or with the structures for path, query, body, and response. The former might get a bit redundant, but may also allow for more highly tailored examples that pertain not just to a struct (that may be reused with multiple endpoints) but to the specific endpoint.

In either case, we can validate the example against the struct/schema to make sure the example isn't invalid.

If nothing else, it's helpful for this repo's primary branch to be consistent with other repos created with newer defaults.

If you have a local clone, these are the steps GitHub recommends to get past this change:

git branch -m master main
git fetch origin
git branch -u origin/main main

Otherwise, this should have no impact on you.

While bringing PR #48 up to date, I found that I was getting inconsistent results from the expectorate tests in dropshot/tests/test_openapi.rs. Sometimes it would want to add this block:

+    "/datagoeshere": {
+      "put": {
+        "operationId": "handler7",
+        "requestBody": {
+          "content": {
+            "application/octet-stream": {
+              "schema": {
+                "type": "string",
+                "format": "binary"
+              }
+            }
+          },
+          "required": true
+        },
+        "responses": {
+          "200": {
+            "description": "successful operation"
+          }
+        }
+      }
+    },

and sometimes it would want to remove it.

The problem is that test_openapi.rs has two different tests that use expectorate to compare against the same output file: test_openapi() and test_openapi_old(). This will always work correctly on master today because those two functions happen to construct equivalent ApiDescriptions, but there's nothing that enforces that. If they were to diverge (as happened during my merge), then one of these tests will always fail. You might look at the output, think the change looks sensible, and rerun with EXPECTORATE=overwrite to update the expected output. If you do this, then on the next run one of the tests will still fail (since the two tests check the same file and have different actual output), and it could be either of them depending on the order they ran during the "overwrite" run.

I'm fixing this by doing two things:

• moving the ApiDescription setup to a helper function so that the two tests cannot diverge in terms of the ApiDescription that they create
• moving the expected output of test_openapi_old() from test_openapi.json to test_openapi_old.json

This second change has the drawback that previously, we were testing that both the old and new interfaces for generating an OpenAPI spec generate exactly the same spec. Now, we're relying on a person to make that check if/when they choose to update the expected output files. I think this is an okay tradeoff because the behavior with expectorate today is quite confusing and results in non-deterministic test results.

It's possible that expectorate could do something better here -- like maybe allow you to pass a flag that disables "overwrite" for a particular call site. The message it prints out on failure could say "the expected output is wrong and somebody's disabled overwrite -- go see the call site to figure out how to fix this."

When using the Logger provided on the RequestContext, some properties are already included by default; most usefully for correlation of multiple log entries is the request ID. Unfortunately the only place in the logs that the source IP and port of the connection through which the request was sent appears not to contain this request ID:

Aug 15 04:03:11.580 INFO accepted connection, remote_addr: 10.1.1.90:47312
Aug 15 04:03:11.580 WARN lunar waneshaft not ambifacient, uri: /prefabulate/amulite, method: POST, req_id: acd330b9-9e9c-494d-ac7a-610ba0fc8933
Aug 15 04:03:11.581 INFO request completed, error_message_external: invalid, error_message_internal: invalid, response_code: 400, uri: /prefabulate/amulite, method: POST, req_id: acd330b9-9e9c-494d-ac7a-610ba0fc8933

One imagines the request ID is not actually available yet at the time we receive the incoming connection, as it will presumably be taken from a request header (if provided) to allow tracing. It seems like we could, then, do at least one of:

• another ID that tracks the connection; e.g., $PID/$CONNID where CONNID would start at 0 and increment for each connection served by this process. This could also be a UUID separate from the one the client passes in, which may subsequently be useful for OpenTracing-style spans and sub-spans anyway.
• the remote_addr field appear in each log line as well as the request ID
• defer emitting the accepted connection message until we have enough context to know what the request ID is (seems not great)

For an example, see the output from this test:
https://github.com/oxidecomputer/dropshot/blob/96faa6f9dc523541d50dfb1508888bd3619f59df/dropshot/tests/test_openapi.json

The "GET /impairment" operation should have more query parameters (see the test case). Assigning to @ahl because I think you volunteered to take a look.

It would be really helpful if when tests failed as a result of a panic (e.g., assertion failure), we had a core file from the process.

On the Rust side, you didn't use to be able to set panic = "abort" in tests (well, you could, but it wouldn't do anything). But it seems that there's an experimental flag "panic-abort-tests". I have not played with this enough to see how it works, or if it works.

On the GitHub CI side, it's possible to upload artifacts including core files:
https://docs.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts

I have a pretty simple context object:

struct Context {
    db: db::Database,
    sig: WaitSignal,
}

impl Context {
    fn from_private(ctx: Arc<dyn Any + Send + Sync + 'static>)
        -> Arc<Context>
    {
        ctx.downcast::<Context>().expect("context downcast")
    }

    fn from_request(rqctx: &Arc<RequestContext>)
        -> Arc<Context>
    {
        Self::from_private(Arc::clone(&rqctx.server.private))
    }
}

I then grab this in handlers thus:

#[endpoint {
    method = GET,
    path = "/gusty/winds/may/exist",
}]
async fn gusty_winds_check(
    rqctx: Arc<RequestContext>,
) -> std::result::Result<HttpResponseOk<GustyWinds>, HttpError> {
    let c = Context::from_request(&rqctx);
    ...

I think it is fine to require the Context::from_request() call here, but I wonder if we couldn't provide a derive macro to avoid the need to copy paste the impl Context that does the conversion.

One could imagine a From<&Arc<RequestContext> that you'd get thus:

#[derive(DropshotContext)]
struct Context {
    blah: String,
};

fn handler(rqctx: Arc<RequestContext>) -> Result<()> {
    let c: Context = rqctx.into();

If From doesn't work for some reason I am missing, it'd also be fine for DropshotContext to just have a from_request() method that you'd call the same way as in my original example.

@ahl saw a case where test_config_bind_address failed in GitHub CI. (To see it in that link, ignore all the green checkmarks on the left and look in the right-hand pane, where you should see the "Run tests" stage failed on macos-10.15.) The error looks like this:

test test_config_bad_bind_address_port_too_large ... ok
test test_config_bind_address ... FAILED

failures:

---- test_config_bind_address stdout ----
log file: /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/test_config-b443ebedf9b5fd06-config_bind_address.1833.0.log
note: configured to log to "/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/test_config-b443ebedf9b5fd06-config_bind_address.1833.0.log"
config "bind_address": Ok(ConfigDropshot { bind_address: V4(127.0.0.1:12215) })
thread 'test_config_bind_address' panicked at 'assertion failed: error.is_connect()', dropshot/tests/test_config.rs:130:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_config_bind_address

test result: FAILED. 3 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '-p dropshot --test test_config'

Unfortunately, there's very little more information we have about this from the original failure. A core file at the point of the assertion failure would let us look at the exact error (not to mention the client and server states), but I haven't successfully gotten Rust to dump core on a panic during testing yet and we haven't configured it that way in CI. The log file is not available either, but even if it were, it likely wouldn't have information about the client-side error in this case.

To first principles: what the test is trying to do is:

• it has a Dropshot HTTP server set up against which it has just successfully made an HTTP request
• it shuts down the server and waits for the server to cleanly come down
• it makes a request (using the same hyper::Client it was using before) and expects that request to fail

The intent was to make sure that after you close the server down, it's not still somehow listening on its socket. That test seems valid, although as I'll explain below, I think there's a bug in step 3 here.

Trying to reproduce it

Meanwhile, I tried to reproduce this by running this in a tight loop:

$ while cargo +nightly test --test=test_config test_config_bind_address; do :; done

I caught it after 2 minutes, but of course still didn't have a core file. I added some flags to try to get a core file and also added this patch:

$ git diff
diff --git a/Cargo.toml b/Cargo.toml
index 5ebcba0..d954b78 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -6,3 +6,12 @@
 [workspace]
 members = ["dropshot", "dropshot_endpoint" ]
 default-members = ["dropshot", "dropshot_endpoint" ]
+
+[profile.dev]
+panic = "abort"
+
+[profile.release]
+panic = "abort"
+
+[profile.test]
+panic = "abort"
diff --git a/dropshot/tests/test_config.rs b/dropshot/tests/test_config.rs
index b23664e..6a55153 100644
--- a/dropshot/tests/test_config.rs
+++ b/dropshot/tests/test_config.rs
@@ -127,6 +127,7 @@ async fn test_config_bind_address() {
      * the server.
      */
     let error = client.request(cons_request(bind_port)).await.unwrap_err();
+    eprintln!("error: {:?}", error);
     assert!(error.is_connect());
 
     /*

so that we'd at least know more about the error. I never did get the core file working (for other reasons related to my Mac), but I did get the error output:

---- test_config_bind_address stdout ----
---- test_config_bind_address stderr ----
log file: /var/folders/x3/6tbrhnss6zz677z4zjsyvkqc0000gn/T/test_config-8db7435ff1c4e356-config_bind_address.27610.0.log
note: configured to log to "/var/folders/x3/6tbrhnss6zz677z4zjsyvkqc0000gn/T/test_config-8db7435ff1c4e356-config_bind_address.27610.0.log"
config "bind_address": Ok(ConfigDropshot { bind_address: V4(127.0.0.1:12215) })
error: hyper::Error(Io, Os { code: 54, kind: ConnectionReset, message: "Connection reset by peer" })
thread 'main' panicked at 'assertion failed: error.is_connect()', dropshot/tests/test_config.rs:131:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

For testing, @ahl also updated the branch being tested in GitHub CI to run the same loop, and it caught the same issue. This manifestation could be a symptom of TCP local port exhaustion, since we're running this in a tight loop. It seems unlikely, though, that we'd see that if we weren't running it in a tight loop, so I'm not sure this is what the original issue was.

A hypothesis and proposed fix

There's another possible cause here. Under normal conditions, here's what we'd expect to happen in this test:

1. We shut down the server and wait for it to come down cleanly. This includes closing its open socket that's connected to the client. This would normally send a FIN. (This is all over loopback, but I don't think that'll materially change the sequence of events here.)
2. The kernel networking stack processes that on the client side (either receiving the FIN or else a more explicit call, in the case of loopback). Depending on the networking stack implementation, and definitely if it weren't over loopback, this would be asynchronous with respect to shutting down the server's side of the socket.
3. The kernel causes the client, which ought to be sitting in poll(2) (or equivalent), to return from poll(2) with state indicating that the client fd is ready to be read from without blocking (probably POLLIN asserted on the fd).
4. Up in Rust, maybe in the tokio executor, we see that we can read from the fd without blocking, we read from it, we get 0 bytes, and we see that the server has shut down the connection. We clean up the connection.
5. Some time later, we use the same client to make another HTTP request to the same server. We no longer have an established connection to the server, so we try to make a new one. The server socket is closed, though, so this fails with ECONNREFUSED.

That's all expected behavior -- in fact, the test is verifying exactly this by checking for ECONNREFUSED.

However, here's another possible sequence of events:

1. Same as (1) above.
2. We go straight to (5) above: the client attempts to make an HTTP request to the same server to which it previously had a valid connection. Except now, because it hasn't gone through steps (2)-(4), it doesn't know that its connection isn't valid. It calls write(2) (or equivalent) to send packets for the HTTP request to the socket.
3. There are two possibilities here, but they both end the same way: if step (2) from above has already happened, then the client TCP stack knows this socket cannot accept more data and immediately returns ECONNRESET when the client tries to write it. If the client TCP stack hasn't processed the FIN yet, then it may send it to the server. The corresponding socket on the server is likely in FIN_WAIT_1. It can technically accept data in this state, but the server application has closed the socket, so it can't actually accept data. The server TCP stack is likely to send an ECONNRESET in this case.

Either way, the client gets an ECONNRESET.

We can apply this fix:

diff --git a/dropshot/tests/test_config.rs b/dropshot/tests/test_config.rs
index b23664e..0b38095 100644
--- a/dropshot/tests/test_config.rs
+++ b/dropshot/tests/test_config.rs
@@ -124,8 +124,13 @@ async fn test_config_bind_address() {
 
     /*
      * Make another request to make sure it fails now that we've shut down
-     * the server.
+     * the server.  We need a new client to make sure our client-side connection
+     * starts from a clean slate.  (Otherwise, a race during shutdown could
+     * cause us to successfully send a request packet, only to have the TCP
+     * stack return with ECONNRESET, which gets in the way of what we're trying
+     * to test here.)
      */
+    let client = hyper::Client::new();
     let error = client.request(cons_request(bind_port)).await.unwrap_err();
     assert!(error.is_connect());
 

All this does is cause us to use a new client for the new request, which I expect would eliminate any possibility of using the previous file descriptor. (It's conceivable that hyper is using a shared cache of client connections under the hood, in which case this won't actually help.)

I don't think it's possible to prove or disprove this hypothesis from the original instance of the problem. If we can reproduce it, then if we weren't over loopback, a packet capture could confirm this behavior. Over loopback, we'd likely need to trace OS internals. I'm not sure it's worth it here. We can say this much: (1) whether this is what we hit or not, the above sequence seems like a possibility, (2) the fix should eliminate that problem, (3) the fix does not make anything worse. I'd prefer a smoking gun, but I'm just going to apply this.

Tokio 0.3 came out in mid-october (https://tokio.rs/blog/2020-10-tokio-0-3), and they're now on 0.3.3, so it seems like it should be stable enough to upgrade to now. I haven't done an upgrade from 0.2 - 0.3, but just speculating, it looks like the main thing to do is use the new feature flag names, and call the .compat() method from tokio-compat-02 to wrap futures from crates that are still on 0.2.

Add a list of tags to endpoint metadata to let developers specify the OpenAPI tags; should be an array.

HttpResponseOkObject had the Object suffix because there was also HttpResponseOkObjectList, which in principle supported streaming. We changed the latter to take a vector (so it's no longer streaming), and that pattern is largely superseded by ResultsPage for pagination anyway, so we may as well trim HttpResponseOkObject to just HttpResponseOk. You can still give it a Vec if that's what you want.

See #76 for background. I wanted to see if there was a better approach here. I found that rustfmt intends that your code should not be reformatted if you stick to stable options. From my testing, this appears to be true both with stable options and with the unstable options that we're using in Dropshot. (We previously thought that we were seeing a bunch of churn. I now suspect that churn was a result of accidentally switching between nightly and stable versions of otherwise equivalent rustfmt releases. Details below.)

Given that, I see three options here:

1. Keep things the way they are. This means we document in the README that users must use a nightly release for rustfmt and we tell them which specific release to use. Developers are expected to configure this correctly for themselves, whether that's vim, VScode (settings for which are in this repo), or muscle memory (cargo +nightly-2020-07-26 fmt rather than cargo fmt).
2. Keep things mostly the same, but remove require_version from rustfmt.toml. We document that developers must use nightly, and that we expect any nightly is likely to work. (If we run into cases where a specific nightly starts formatting the code differently, we'll see that in the pre-integration check.) In this case, we're using nightly only for its policy that unstable options are supported, not because we need a bleeding-edge version of the software. Like option (1), developers still have to configure things locally. On the plus side, we don't have to keep bumping the specific require_version, CI config, or README instructions. Right now, there's basically no reason to ever bump these unless that build stops becoming available or we adopt a newer rustfmt that actually changes the way code is formatted. But people might be confused about why our instructions reference an ancient 2020-07 nightly build or suggest that we update it. Instead, the CI can use the known-good version until we need to update it, and everybody else can just see and remember "nightly" rather than "nightly-2020-07-26". (This might be a pretty big advantage for people that use "nightly" for other purposes -- like other projects that require "nightly" -- since they can use that toolchain for Dropshot, too.)
3. Drop all the unstable options altogether, including require_version. Keep us on stable rustfmt. This is smoothest for everybody -- you only need one toolchain, you don't need any custom local config, and there's nothing to forget about.

Option 3 sounds great, but some of the defaults really do seem unfortunate. Here's the result of reformatting today's Dropshot with the default options:
7d12647

By my spot check: most of the changes appear due to struct_lit_single_line, and they mostly look a lot less readable to me. Here's a typical example:
7d12647#diff-1feac7e55ea61939c504aaa3cfc07e35ca99d8eb3b38cb154a0866302832a73fR350

Maybe I'm overthinking it, though. That one sucks because Projects are eventually going to have lots of fields, but when they do, they won't fit on one line anyway. @ahl What do you think?

If we don't go with (3), I'm tempted by (2) so that people don't forever need to remember 2020-07-26.

Methodology

Feel free to skip this.

I wanted to answer the question: if we just used stable rustfmt, how often would we wind up reformatting our code?

My plan was to identify the rustfmt versions that shipped with stable Rust releases in 2020, reformat Dropshot with the first one, then create a sequence of commits showing the diffs from each stable version's rustfmt to the next. I created two branches to show this:

• rustfmt-test-stable-only tries to test this for versions of rustfmt that ship with stable Rust. This branch starts from the current tip of "master", removes all unstable options (including require_version), and then does what I said: one commit for each subsequent rustfmt version (that shipped with a stable Rust version). Each commit shows the changes between two rustfmt versions.
• rustfmt-test-unstable-options seeks to see how much we'd be broken if we kept our unstable options but used the latest nightly. This branch starts from the current tip of "master" and removes only the require_version option so that we can test with different versions. Then it proceeds as in the other case. This was trickier than above because in order to support the unstable options, we have to be using a nightly version. I didn't go track down which nightly toolchain corresponded with each stable release's rustfmt. Instead, I grabbed 13 nightly builds, spaced about a month apart, and tested those.

As promised, the code never changed in the stable branch. But to our surprise, the code also didn't change in the unstable branch. This contradicted our previous experience! Looking through logs, I now suspect that when we thought things had changed between versions, they actually changed because rustfmt was run under "stable", not "nightly", and it ignored several key options.

See above for a summary of our choices, given this information.

For posterity, here's the (awful, unpolished) script that I used:

#!/bin/bash

#
# Test various versions of `rustfmt` on a code base.
#

set -o errexit
set -o pipefail
set -o xtrace

# "releases" is the set of toolchains (recognized by rustup) that we will test against.
#releases="1.40.0 1.41.0 1.42.0 1.43.0 1.44.0 1.44.1 1.45.0 1.45.1 1.46 1.47 1.48 1.49"
releases=""
releases="$releases nightly-2020-01-02"
releases="$releases nightly-2020-02-01"
releases="$releases nightly-2020-03-01"
releases="$releases nightly-2020-04-06"
releases="$releases nightly-2020-05-01"
releases="$releases nightly-2020-06-01"
releases="$releases nightly-2020-07-01"
releases="$releases nightly-2020-08-13"
releases="$releases nightly-2020-09-07"
releases="$releases nightly-2020-10-01"
releases="$releases nightly-2020-11-09"
releases="$releases nightly-2020-12-07"
releases="$releases nightly-2021-01-01"

# "dropshot_repos" are the local clones of repositories that we will test.
# This has nothing to do with dropshot, actually.
#dropshot_repos="dropshot-stable-options dropshot-unstable-options"
dropshot_repos="dropshot-unstable-options"
last_version=""
last_release=""
for release_version in $releases; do
	cargo +$release_version version
	cargo +$release_version fmt --version

	rustfmt_version="$(cargo +$release_version fmt --version)"

	message="reformat: Rust $release_version ($rustfmt_version)"
	if [[ -n "$last_version" ]]; then
		message="$message (from Rust $last_release ($last_version))"
	fi
	last_version="$rustfmt_version"
	last_release="$release_version"

	echo "$message"

	for dropshot_repo in $dropshot_repos; do
		(cd $dropshot_repo &&
		    echo release_version=$release_version > rustfmt_output &&
		    cargo +$release_version fmt --version >> rustfmt_output 2>&1 &&
		    cargo +$release_version fmt >> rustfmt_output 2>&1 &&
		    git add rustfmt_output &&
		    git commit -a -m "$message")
	done
done

Hi,

I would like the ability to set specific HTTP response headers on specific endpoints. E.g. it would be nice to have the ability to set a Content-Disposition header at specific endpoints to trigger a file download when accessed via a browser.

I do not see this currently exposed in the API anywhere but maybe I missed something.

Hey Oxiders!

I heard about this project from your post on twitter. Since we're creating a public API at my work, I was interested to know what you thought of the alternative crates in the ecosystem, and why you decided to roll your own. I was excited to see that in the documentation for this crate, you wrote:

For a discussion of alternative crates considered, see Oxide RFD 10.

Unfortunately it looks like that's an internal discussion that I can't see. I was wondering if you could share any of the specifics about what other crates you looked at and what you thought about them?

The problem

In oxidecomputer/omicron#64 @david-crespo reported that when running openapi-generator for TypeScript on the Omicron API schema, he was getting duplicate type definitions like ApiRackView and ApiRackView1. The types were exactly the same. This API has two different endpoints that use this type: one returns an ApiRackView, and the other returns a list of ApiRackView. It appears that the dropshot-generated OpenAPI spec inlines the top-level type in the response, but it generates references for subtypes.

Test case

I was able to replicate this by modifying test_openapi.rs with this change:

diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs
index 97daf80..3af7e5f 100644
--- a/dropshot/tests/test_openapi.rs
+++ b/dropshot/tests/test_openapi.rs
@@ -136,6 +136,36 @@ async fn handler7(
     unimplemented!();
 }
 
+#[derive(JsonSchema, Serialize)]
+struct NeverDuplicatedTopLevel {
+    b: NeverDuplicatedNextLevel,
+}
+
+#[derive(JsonSchema, Serialize)]
+struct NeverDuplicatedNextLevel {
+    v: bool,
+}
+
+#[endpoint {
+    method = PUT,
+    path = "/dup1",
+}]
+async fn handler8(
+    _rqctx: Arc<RequestContext<()>>,
+) -> Result<HttpResponseOk<NeverDuplicatedTopLevel>, HttpError> {
+    unimplemented!();
+}
+
+#[endpoint {
+    method = PUT,
+    path = "/dup2",
+}]
+async fn handler9(
+    _rqctx: Arc<RequestContext<()>>,
+) -> Result<HttpResponseOk<NeverDuplicatedTopLevel>, HttpError> {
+    unimplemented!();
+}
+
 fn make_api() -> Result<ApiDescription<()>, String> {
     let mut api = ApiDescription::new();
     api.register(handler1)?;
@@ -145,6 +175,8 @@ fn make_api() -> Result<ApiDescription<()>, String> {
     api.register(handler5)?;
     api.register(handler6)?;
     api.register(handler7)?;
+    api.register(handler8)?;
+    api.register(handler9)?;
     Ok(api)
 }

Here's how that changes the generated schema for that test:

dap@zathras dropshot $ git diff dropshot/tests/test_openapi.json
diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json
index 1233f79..688000d 100644
--- a/dropshot/tests/test_openapi.json
+++ b/dropshot/tests/test_openapi.json
@@ -26,6 +26,58 @@
         }
       }
     },
+    "/dup1": {
+      "put": {
+        "operationId": "handler8",
+        "responses": {
+          "200": {
+            "description": "successful operation",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "title": "NeverDuplicatedTopLevel",
+                  "type": "object",
+                  "properties": {
+                    "b": {
+                      "$ref": "#/components/schemas/NeverDuplicatedNextLevel"
+                    }
+                  },
+                  "required": [
+                    "b"
+                  ]
+                }
+              }
+            }
+          }
+        }
+      }
+    },
+    "/dup2": {
+      "put": {
+        "operationId": "handler9",
+        "responses": {
+          "200": {
+            "description": "successful operation",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "title": "NeverDuplicatedTopLevel",
+                  "type": "object",
+                  "properties": {
+                    "b": {
+                      "$ref": "#/components/schemas/NeverDuplicatedNextLevel"
+                    }
+                  },
+                  "required": [
+                    "b"
+                  ]
+                }
+              }
+            }
+          }
+        }
+      }
+    },
     "/impairment": {
       "get": {
         "operationId": "handler6",
@@ -288,6 +340,17 @@
   },
   "components": {
     "schemas": {
+      "NeverDuplicatedNextLevel": {
+        "type": "object",
+        "properties": {
+          "v": {
+            "type": "boolean"
+          }
+        },
+        "required": [
+          "v"
+        ]
+      },
       "ResponseItem": {
         "type": "object",
         "properties": {

We see that the top-level type shows up twice, inline in both places, while the next level type gets a reference instead.

Analysis

When you use the endpoint macro, we generate a From<...> for ApiEndpoint for this specific handler

#[allow(non_camel_case_types, missing_docs)]
#[doc = "API Endpoint: handler9"]
struct handler9 {}
#[allow(non_upper_case_globals, missing_docs)]
#[doc = "API Endpoint: handler9"]
const handler9: handler9 = handler9 {};
impl From<handler9>
    for dropshot::ApiEndpoint<
        <Arc<RequestContext<()>> as dropshot::RequestContextArgument>::Context,
    >
{
    fn from(_: handler9) -> Self {
        async fn handler9(
            _rqctx: Arc<RequestContext<()>>,
        ) -> Result<HttpResponseOk<NeverDuplicatedTopLevel>, HttpError> {
            ::core::panicking::panic("not implemented");
        }
        dropshot::ApiEndpoint::new(
            "handler9".to_string(),
            handler9,
            dropshot::Method::PUT,
            "/dup2",
        )
    }
}

(That's from cargo expand -p dropshot --test=test_openapi with my changes.)

When you register the handler function, we use that From impl (well, the corresponding Into) here:

We see above that that invokes ApiEndpoint::new(), which winds up invoking metadata() on the response type:

ResponseType is HttpResponseOk here, and the metadata() impl comes from HttpTypedResponse:

What's important here is that we supply Body::json_schema as the schema function here:

Again, that's all at registration time. Later when we generate the schema, we use that schema function to generate a schema for the response body:

Let's take a closer look at the derived JsonSchema impls. For the top-level type, it looks like this (back to cargo expand):

impl schemars::JsonSchema for NeverDuplicatedTopLevel {
    fn schema_name() -> std::string::String {
        "NeverDuplicatedTopLevel".to_owned()
    }
    fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema {
        {
            let schema = {
                let mut schema_object = schemars::schema::SchemaObject {
                    instance_type: Some(schemars::schema::InstanceType::Object.into()),
                    ..Default::default()
                };
                <NeverDuplicatedNextLevel as schemars::JsonSchema>::add_schema_as_property(
                    gen,
                    &mut schema_object,
                    "b".to_owned(),
                    None,
                    true,
                );
                schemars::schema::Schema::Object(schema_object)
            };
            gen.apply_metadata(schema, None)
        }
    }
}

The schema for NeverDuplicatedNextLevel must be added via add_schema_as_property, which winds up invoking not json_schema() on NeverDuplicatedNextLevel, but gen.subschema_for(). That's documented as:

Generates a JSON Schema for the type T, and returns either the schema itself or a $ref schema referencing T’s schema.

If T is referenceable, this will add T’s schema to this generator’s definitions, and return a $ref schema referencing that schema. Otherwise, this method behaves identically to JsonSchema::json_schema.

This explains why the non-top-level schemas get references: JsonSchema internally uses a function that tries to add references if possible. The top-level ones don't get references because we're using json_schema() directly, which always generates a new, non-reference schema.

Numeric path parameters work, but they require the workaround for serde-rs/serde#1183. We should add a test for this.

If you write an endpoint function and forget to api.register it, the server will 404 on those routes, but it's not easy to figure out the problem. It would be helpful if there was a compiler error or warning for unregistered route handlers.

The "page_token" format that we use for pagination is implicitly part of the consumer's API surface. It doesn't appear in the OpenAPI spec, and clients aren't supposed to know or care about the format, but critically: if the consumer changes the structure of their PageSelector type, that can invalidate URLs generated before the change. This has impact on consumers that might want to do a rolling upgrade of a fleet of servers without breaking clients in confusing ways.

I'm not sure how best to handle this. One way would be to provide a test function for consumers to generate an OpenAPI-like spec for the page selector. Then they could deal with this the same way they might deal with the actual OpenAPI spec, probably by generating it from the source, storing it in version control, and raising a flag whenever it changes.

When I run cargo build, I get some unusual output which perhaps presents some challenges in interpretation:

 $ cargo build
   Compiling rust-notif v0.1.0 (/ws/rust-notif)
next_key_seed core::marker::PhantomData<dropshot_endpoint::_::<impl serde::de::Deserialize for dropshot_endpoint::Metadata>::deserialize::__Field>
next_value_seed core::marker::PhantomData<dropshot_endpoint::MethodType>
next_key_seed core::marker::PhantomData<dropshot_endpoint::_::<impl serde::de::Deserialize for dropshot_endpoint::Metadata>::deserialize::__Field>
next_value_seed core::marker::PhantomData<alloc::string::String>
next_key_seed core::marker::PhantomData<dropshot_endpoint::_::<impl serde::de::Deserialize for dropshot_endpoint::Metadata>::deserialize::__Field>
next_key_seed core::marker::PhantomData<dropshot_endpoint::_::<impl serde::de::Deserialize for dropshot_endpoint::Metadata>::deserialize::__Field>
next_value_seed core::marker::PhantomData<dropshot_endpoint::MethodType>
next_key_seed core::marker::PhantomData<dropshot_endpoint::_::<impl serde::de::Deserialize for dropshot_endpoint::Metadata>::deserialize::__Field>
next_value_seed core::marker::PhantomData<alloc::string::String>
next_key_seed core::marker::PhantomData<dropshot_endpoint::_::<impl serde::de::Deserialize for dropshot_endpoint::Metadata>::deserialize::__Field>

    Finished dev [unoptimized + debuginfo] target(s) in 7.48s

It is not completely clear from this if I need to do something, but the build finishes and the resultant executable appears otherwise functional.

We should have first-class support for paginated endpoints, i.e. endpoints that enumerate lists of items and return a page at a time of content. We want to make it easy for developers to write these paginated endpoints without too much additional boiler plate.

In addition, it would be nice to have a generic manifestation of these paginated endpoints in the openapi doc. While OpenAPI doesn't have first-class support for pagination, we can potentially provide hints in the extensions (e.g. x-dropshot-paginated) that we could use in reference doc generation as well as client library code gen (both would require modifications to existing tools).

As @ahl pointed out, the Json type doesn't have to be tied to JSON. It could look at the content-type and parse other formats as well. Let's call it TypedBody. (I'd just use Body but that will be confusing with hyper::Body.)

I have an API where I am just accepting a freeform string in a POST. I was able to read a capped amount of bytes, thus:

use hyper::body::HttpBody;

async fn read_body(rqctx: &Arc<RequestContext>, max: usize) -> Result<String> {
    let mut req = rqctx.request.lock().await;
    let body = req.body_mut();
    let mut out: Vec<u8> = Vec::new();

    while let Some(x) = body.data().await.transpose()
        .map_err(|e| anyhow!("body read: {}", e))?
    {
        if out.len().saturating_add(x.len()) > max {
            bail!("request size exceeded {} bytes", max);
        }

        out.append(&mut x.to_vec());
    }

    Ok(String::from_utf8(out)?)
}

It'd be neat if there was some way to get either a UTF-8 String or a slice or vector of u8 (I think both will be useful at different times) through an extractor in the way that you can get a specific Deserializable today.

Right now, Dropshot has three hardcoded "tunables":

These should come from the ConfigDropshot struct, probably with default values.

In the case that a server presents both an API and some other artefacts (e.g., a descriptive or interactive set of HTML pages, images, etc) it would be good to be able to omit those handlers from any API specification that is produced.

Good Day,

I'm currently learning dropshot, and am curious on how to configure a handler to run on all requests.

Similar to http://restify.com/docs/server-api/#use (https://github.com/joyent/sdc-cloudapi/blob/master/lib/app.js#L426)

Or perhaps there is an alternative pattern I should use while using DropShot?

Thanks,
Bruce

For paginated resources, there's always a per-page limit that's either the limit requested by the client, the default limit provided by the server, or the maximum limit supported by the server. We provide RequestContext::page_limit() to choose the right value, but of course consumers have to actually use that. It'd be nice if we Dropshot could identify when they've done the wrong thing. That way, consumers wouldn't have to test this behavior themselves for every one of their paginated resources. We could do this if, when calling ResultsPage::new(), the consumer provided the pagination parameters. Then we could call page_limit() directly and compare that to the number of items they provided.

There are a lot of things we could choose to do if the consumer gave us the wrong number of items:

• panic: this is, after all, a programmer error, and a core file this will give people the best chance of debugging it.
• return a 500 response: this presumably will have a smaller blast radius for the user than panicking, but still noticeable and potentially debuggable from the log (although probably less so than a core file would be). It's still not great for clients.
• log a warning and proceed anyway, returning all the items we were given: less noticeable (which is potentially bad), but also doesn't impact the consumer's users
• log a warning and truncate the results to the intended limit: also least noticeable, probably more correct in that it obeys the limit.

On the one hand, it seems silly to panic or fail a request that we can otherwise process correctly. But allowing these requests to complete defeats the whole purpose of pagination: among the reasons we use pagination are to bound the amount of work an operation does and the amount of time it takes, in turn to facilitate scalability and availability and mitigate DoS. If we get here because a consumer forgets to append "LIMIT N" to a SQL query, and Dropshot is basically silent about it, the user may get by for a long time, only to discover after months when they dig into some pathological performance problem that this has been acting like an unpaginated API (and they've got a DoS vector in their application).

We could make this a tunable policy but I think we may as well start by picking a policy we want for ourselves and doing that. I lean towards panicking or returning a 500.

This is related to #20, which also needs the limit available in ResultsPage::new().

Currently path and query parameters are a little hacked up, assume string types, and don't allow for optional parameters (query only). Rather than using this ExtractedParameter trait, we should look at using JsonSchema.

It might be more intuitive to look for the limit from PaginationParams and since the limit field itself is not accessible (or documented publicly) a method there might be more easily found than in RequestContext

In particular, this output from bad_endpoint6.stderr seems to have regressed:

error[E0063]: missing fields `x`, `y` in initializer of `Ret`
  --> $DIR/bad_endpoint6.rs:17:1
   |
17 | / #[endpoint {
18 | |     method = GET,
19 | |     path = "/test",
20 | | }]
   | |__^ missing `x`, `y`

error[E0277]: the trait bound `Ret: serde::ser::Serialize` is not satisfied
  --> $DIR/bad_endpoint6.rs:17:1
   |
17 | / #[endpoint {
18 | |     method = GET,
19 | |     path = "/test",
20 | | }]
   | |__^ the trait `serde::ser::Serialize` is not implemented for `Ret`
   |
   = note: required by `dropshot::HttpResponseOk`

error[E0277]: the trait bound `fn(std::sync::Arc<dropshot::RequestContext>) -> impl std::future::Future {<impl std::convert::From<bad_endpoint> for dropshot::ApiEndpoint>::from::bad_endpoint}: dropshot::handler::    HttpHandlerFunc<_, _>` is not satisfied
  --> $DIR/bad_endpoint6.rs:17:1
   |
17 | / #[endpoint {
18 | |     method = GET,
19 | |     path = "/test",
20 | | }]
   | |__^ the trait `dropshot::handler::HttpHandlerFunc<_, _>` is not implemented for `fn(std::sync::Arc<dropshot::RequestContext>) -> impl std::future::Future {<impl std::convert::From<bad_endpoint> for                dropshot::ApiEndpoint>::from::bad_endpoint}`

That should be referencing the offending code, not the macro.

As it says in error.rs, we envision that errors from an API typically have a string code (separate from the HTTP status code):

I've generally assumed that the namespace of these codes needs to be under the control of the Dropshot consumer, since they will likely be defining most of the codes for their own application-specific conditions. However, Dropshot currently generates a bunch of different errors of its own (e.g., when a request arrives for a path that has no handler associated with it). What code should it use?

I'm thinking out loud through a bunch of options here:

1. Have Dropshot define a set of codes for itself and let consumers use whatever codes they want, too. This kind of sucks. It's hard to expand Dropshot's set later since we might stomp on a code that consumers are already using. We could use a prefix to carve out part of the namespace, but this is part of the consumer's public API -- that's ugly. Plus, it's hard for Dropshot to make judgments about what error cases a consumer's client wants to distinguish.
2. Let the consumer define the entire namespace and allow consumers to define the codes that Dropshot will use for its own errors. Maybe the caller provides a function that maps HTTP status codes to Strings (or something that we can turn into a String). Or maybe Dropshot defines an enum for the conditions it needs to generate codes for and consumers provide a function that maps from that. We could even provide a default implementation that would hopefully be suitable for most consumers. This gives us most of the benefits of (1) but without the expansion problem -- if we expand it, if the consumer uses a match, they'll get a compile error that forces them to decide how to map the new condition.
3. Have Dropshot own the entire namespace: create a set of codes like ObjectNotFound, BadArgs, etc. and require that consumers use these. This might actually be nice because many consumers will wind up using a lot of the same codes, but it seems like a non-starter that consumers can't extend this.

The behavior today is that the code is optional and Dropshot generally provides None. That was basically a hack to be able to make forward progress -- it's pretty crappy for consumers and their users.

See also #39.

It would be great if Dropshot provided an ergonomic way for consumers to put their server behind TLS. What this means is a little up in the air, but I imagine we'd need to do some combination of:

• pick a library for doing TLS and integrate it with HttpServer
• provide configuration for specifying that you want TLS
• provide configuration for specifying a certificate (for custom certs) and any other security-related params
• ideally: provide an easy way to use letsencrypt or something like that, particularly for development

It's worth a bit of research to see what libraries and best practices are out there already for doing this in Rust.

The current print_openapi() is a little unwieldy; it contains a non-optional argument after a sea of optionals, and even if you intend to provide all of the optionals it's a lot of strings that you have to put in the right order.

A simple call, with no optionals, looks like this today:

api.print_openapi(
    &mut f,
    &"Keeper API".to_string(),
    None,
    None,
    None,
    None,
    None,
    None,
    None,
    &"1.0".to_string(),
)?;

I suspect it'd be better as a struct, like:

api.print_openapi(
    &mut f,
    OpenApiDetails {
        name: "Keeper API".to_string(),
        version: "1.0".to_string(),
        ..Default::default()
    },
)?;

It'd be easier and clearer to then incrementally add additional fields; e.g.,

api.print_openapi(
    &mut f,
    OpenApiDetails {
        name: "Keeper API".to_string(),
        version: "1.0".to_string(),
        license_name: Some("CDDL").to_string(),
        ..Default::default()
    },
)?;

At present, the OpenAPI document emitted by print_openapi() uses inline objects to describe the schema for the body of a request, and does not include the title field in those objects. When client code is generated from that document, the code generator has to invent names for those types. The most prominent code generators (the OpenAPI/Swagger tools) do a relatively poor job at this: they use names like InlineObject1, InlineObject2, etc, and those numbers can even move around depending on enumeration order during the generation sequence.

Here is a reduced sample OpenAPI document (as produced by dropshot and then edited):

{
  "openapi": "3.0.3",
  "info": { "title": "The Royal Sampler API", "contact": {}, "version": "1.0" },
  "paths": {
    "/discombobulate": {
      "post": {
        "operationId": "discombobulate",
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "object",
                "properties": {
                  "newtons": { "type": "integer", "format": "int32" }
                },
                "required": [ "newtons" ]
              }
            }
          },
          "required": true
        },
        "responses": {
          "201": { "description": "successful creation" }
        }
      }
    }
  }
}

In the original source, this object looked like:

#[derive(Debug, Serialize, Deserialize, JsonSchema)]
struct DiscombobulateBody {
    newtons: i32,
}

The generated client code (from openapi-generator-cli-5.0.0-beta3.jar using the rust generator) for this object is:

/*
 * The Royal Sampler API
 *
 * No description provided (generated by Openapi Generator https://github.com/openapitools/openapi-generator)
 *
 * The version of the OpenAPI document: 1.0
 *
 * Generated by: https://openapi-generator.tech
 */

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct InlineObject {
    #[serde(rename = "newtons")]
    pub newtons: i32,
}

impl InlineObject {
    pub fn new(newtons: i32) -> InlineObject {
        InlineObject {
            newtons,
        }
    }
}

There appear to be two potential remedies, at least with respect to the contents of the OpenAPI document, that we could try:

• specify the title field within the inline object
• don't use inline objects at all, instead using a reference to the components section as we would for an object property

Using the title field results in the smallest change to the document:

--- sampler_current.json        Tue Dec 22 21:31:03 2020
+++ sampler_inline.json Mon Dec 21 19:37:52 2020
@@ -9,6 +9,7 @@
           "content": {
             "application/json": {
               "schema": {
+                "title": "DiscombobulateBody",
                 "type": "object",
                 "properties": {
                   "newtons": { "type": "integer", "format": "int32" }

Using referenced components is a bigger change to the structure:

--- sampler_current.json        Tue Dec 22 21:31:03 2020
+++ sampler_explicit.json       Mon Dec 21 19:37:04 2020
@@ -9,11 +9,7 @@
           "content": {
             "application/json": {
               "schema": {
-                "type": "object",
-                "properties": {
-                  "newtons": { "type": "integer", "format": "int32" }
-                },
-                "required": [ "newtons" ]
+                "$ref": "#/components/schemas/DiscombobulateBody"
               }
             }
           },
@@ -24,5 +20,16 @@
         }
       }
     }
+  },
+  "components": {
+    "schemas": {
+      "DiscombobulateBody": {
+        "type": "object",
+        "properties": {
+          "newtons": { "type": "integer", "format": "int32" }
+        },
+        "required": [ "newtons" ]
+      }
+    }
   }
 }

Both options result in a correctly named object in the generated client code:

/*
 * The Royal Sampler API
 *
 * No description provided (generated by Openapi Generator https://github.com/openapitools/openapi-generator)
 *
 * The version of the OpenAPI document: 1.0
 *
 * Generated by: https://openapi-generator.tech
 */

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct DiscombobulateBody {
    #[serde(rename = "newtons")]
    pub newtons: i32,
}

impl DiscombobulateBody {
    pub fn new(newtons: i32) -> DiscombobulateBody {
        DiscombobulateBody {
            newtons,
        }
    }
}

I've spent a bit of time digging into api_description.rs and handler.rs and I'm not yet sure which of these options will be less work there.

I just stumbled on dropshot and when I went to crates.io to check it out, it said dropshot v0.5.1 appears to have no README.md file.

I see that docs.rs has the dropshot docs.

Will it be possible to investigate what is wrong with crates.io?

A lot of the building blocks that make up HttpServer are hidden inside the crate.
It's not really possible to customize the specifics of the HTTP server by just using dropshot as a building block of a Hyper server.

I'm looking for one of these:

• Implement Service<hyper::Request<hyper::Body>> on ApiDescription
• Make HttpRouter public and implement Service<hyper::Request<hyper::Body>> on it

This way people could compose their own HTTP server and the surface area of what dropshot needs to care about is reduced.

Might fix #10

I'm seeing this on Helios on commit 5a6c355 (tip of "main" right now), from a clean build:

$ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.08s
     Running target/debug/deps/dropshot-3bb78da38c3e66f9

running 59 tests
test api_description::test::test_empty_struct ... ok
test api_description::test::test_badpath1 ... ok
test api_description::test::test_badpath3 ... ok
test api_description::test::test_badpath2 ... ok
test api_description::test::test_two_bodies ... ok
test from_map::test::test_lone_literal ... ok
test from_map::test::test_deep ... ok
test from_map::test::test_missing_data2 ... ok
test from_map::test::test_missing_data1 ... ok
test from_map::test::test_types ... ok
test handler::test::test_metadata_flattened_enum ... ok
test handler::test::test_metadata_flattened ... ok
test api_description::test::test_garbage_barge_structure_conversion ... FAILED
test handler::test::test_metadata_simple ... ok
test handler::test::test_metadata_pagination ... ok
test logging::test::test_config_bad_file_no_file ... ok
test logging::test::test_config_bad_file_no_level ... ok
test logging::test::test_config_bad_log_mode ... ok
test logging::test::test_config_bad_file_path_exists_fail ... ok
test logging::test::test_config_bad_file_bad_path_type ... ok
test logging::test::test_config_bad_terminal_bad_level ... ok
test logging::test::test_config_bad_terminal_no_level ... ok
test pagination::test::test_pagination_schema ... ok
test pagination::test::test_page_token_serialization ... ok
test pagination::test::test_pagparams_parsing ... ok
test logging::test::test_config_stderr_terminal ... ok
test pagination::test::test_results_page ... ok
test router::test::test_duplicate_route1 ... ok
test router::test::test_duplicate_route2 ... ok
test router::test::test_duplicate_route3 ... ok
test router::test::test_duplicate_varname ... ok
test router::test::test_embedded_non_variable ... ok
test router::test::test_empty_variable ... ok
test router::test::test_inconsistent_varname ... ok
test router::test::test_error_cases ... ok
test router::test::test_iter2 ... ok
test router::test::test_iter ... ok
test router::test::test_iter_null ... ok
test router::test::test_router_basic ... ok
test router::test::test_variable_after_literal ... ok
test logging::test::test_config_file ... ok
test router::test::test_literal_after_variable ... ok
test router::test::test_variable_name_bad_end ... ok
test router::test::test_variable_name_empty ... ok
test router::test::test_variable_name_bad_start ... ok
test router::test::test_variables_basic ... ok
test test_util::test::test_bunyan_bad_hostname ... ok
test router::test::test_variables_multi ... ok
test test_util::test::test_bunyan_bad_name ... ok
test test_util::test::test_bunyan_bad_pid ... ok
test test_util::test::test_bunyan_bad_v ... ok
test test_util::test::test_bunyan_seq_bad_order ... ok
test test_util::test::test_bunyan_easy_cases ... ok
test test_util::test::test_bunyan_seq_bounds_bad ... ok
test test_util::test::test_bunyan_seq_lower_violated ... ok
test test_util::test::test_bunyan_seq_easy_cases ... ok
test test_util::test::test_bunyan_seq_upper_violated ... ok
test server::test::test_drop_server_without_close_panics ... ok
test server::test::test_server_run_then_close ... ok

failures:

---- api_description::test::test_garbage_barge_structure_conversion stdout ----
thread 'api_description::test::test_garbage_barge_structure_conversion' panicked at 'not yet implemented', dropshot/src/api_description.rs:642:46
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    api_description::test::test_garbage_barge_structure_conversion

test result: FAILED. 58 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

error: test failed, to rerun pass '-p dropshot --lib'

I'm also seeing this on MacOS on the same commit. CI is seeing it under #100 (which has other changes in it, but still seems relevant).

In the openapi output from the oxide-api-prototype oxide_controller I see the types ApiIdSortMode, ApiNameSortMode, and ApiNameOrIdSortMode referenced, but not defined.

This seems to be a problem with the way we (I) handle parameters and references; I'm a little surprised to see that we're generating references at all given https://github.com/oxidecomputer/dropshot/blob/master/dropshot/src/handler.rs#L684

OpenAPI gives us several places where we can put relevant information related to a response:

    "/hardware/racks/{rack_id}": {
      "get": {
        "description": "**1** Retrieve information about a rack specified by ID.",
        "operationId": "api_hardware_racks_get_rack",
        "parameters": [
          { "...": "..."}
        ],
        "responses": {
          "200": {
            "description": "**2** Successfully retrieved information about the rack.",
            "content": {
              "application/json": {
                "schema": {
                  "description": "**3** Information that describes a rack",
                  "type": "object",
                  "properties": {
                    "id": {
                      "type": "string",
                      "format": "uuid"
                    }
                  },
                  "required": [
                    "id"
                  ]
                }
              }
            }
          }
        }
      }
    },

**1** is for the description of the OpenAPI "operation" i.e. the endpoint. Dropshot derives this field from the doc comment on the handler function
**2** is for the description of the OpenAPI "response". In this context, we mostly case about successful responses as error codes will be handled more-or-less generically (see #7). Dropshot doesn't have any content in this field today.
**3** is for the description of the OpenAPI "schema" i.e. the datatype that the response produces. Dropshot derives this today from the doc comment on the structure type.

Question 1: Do we need all three? 1 seems useful as it describes the purpose of the interface. 2 and 3 seem a bit redundant, but I'd argue that we may want both: the datatype may be used in several places and we may want to customize the response description. For example, POST, GET, and PUT (create, view, update) may return the same structure but we may want the description for 2 or 3 to differ. Alternatively, we may just want 3, leaving 2 implied.

Question 2.1: if we want to get rid of one of 2 or 3, which should we keep? Should the text continue to come from the doc comment on the type?

Question 2.2: if we want to keep all 3, where should the description for 2 come from? One could imagine asking the developer to specify this in the #[endpoint {...}] attribute, but this seems like it might be kind of a nuisance. Alternatively, we could use the HttpTypedResponse implementations to have an automatically generated, semi-tailored description. For example, HttpResponseCreated would say something like "creation successful"; HttpResponseDeleted, "deletion successful"; HttpResponseOkObjectList, "list of entities".

We should use one of the programs in tests or add some new ones that call print_openapi() and use a json diff crate to validate the output.

Functions that consume arguments and return values of type &String may as well use &str instead.

See https://petstore3.swagger.io/

It doesn't need to be an exact match, but it should be pretty close.

Dropshot expects some of the consumer's types to impl schemars::JsonSchema, serde::Serialize, and serde::Deserialize. Today, I think consumers add schemars and serde to their Cargo.toml with the same versions as Dropshot and this works fine. However, it's not obvious to consumers that you'd need to do this, or what version(s) would be acceptable here.

I'm trying to better understand the best practice in Rust around this. I expect we'll either want to export JsonSchema from Dropshot and have consumers use that (so they're always using a compatible version, and it's clear where it comes from and what version it is) or else document the dependencies. I'm not sure about serde's interfaces. It seems like overkill to export these, but it also seems like the more correct approach here.

This came up because JsonSchema updated to 0.8. While we updated Dropshot in the master branch (see #61), we didn't bother publishing this to crates.io. So if you grab that version and try to use it with the latest JsonSchema, you get an error about your type not implementing JsonSchema.

Not completely clear what to do here, but I wanted to highlight something about the API as it works today that presents at least a slight rough edge.

When failing, it seems a handler is expected to produce a HttpError of some sort (e.g., for a 500-style server failure, or a 400-style client failure). Often, at least while getting started with a new program, it is convenient to use the anyhow::Error object: a basic string-based error with the ability to hold a cause chain and attach context.

Unfortunately, because of the Rust rules around Trait implementation it is not possible to implement From or Into to convert directly from anyhow::Error to an appropriate HttpError for the application; neither type is in my crate, and thus extension is prohibited. This means you cannot trivially use the ? operator. I worked around this using a pattern similar to the anyhow::Context trait context() method, providing my own extension trait:

trait AnyhowHttpError<T> {
    fn or_500(self) -> std::result::Result<T, HttpError>;
    fn or_400(self) -> std::result::Result<T, HttpError>;
}

impl<T> AnyhowHttpError<T> for anyhow::Result<T> {
    fn or_500(self) -> std::result::Result<T, HttpError> {
        self.map_err(|e| {
            let msg = format!("internal error: {}", e);
            HttpError::for_internal_error(msg)
        })
    }

    fn or_400(self) -> std::result::Result<T, HttpError> {
        self.map_err(|e| {
            HttpError::for_client_error(None, hyper::StatusCode::BAD_REQUEST,
                format!("request error: {}", e))
        })
    }
}

#[endpoint {
    method = GET,
    path = "/some/endpoint",
}]
async fn handle_fetch(
    rqctx: Arc<RequestContext>,
) -> std::result::Result<HttpResponseOk<SomeResult>, HttpError> {
    let c = Context::from_request(&rqctx);

    /*
     * generate_response() returns anyhow::Result<SomeResult>:
     */
    Ok(HttpResponseOk(c.generate_response().or_500()?))
}

Hi,

This may not be an issue with dropshot and most likely I am doing something wrong.

I am trying to use dropshot in a server. The api endpoint is supposed to return "audio/mp3" as content type. To do that my function looks like (retained only relevant portions of the code)

#[derive(Debug, Serialize, Deserialize, JsonSchema)]
pub struct Args {
    port: Option<String>,
    text: String,
}

#[endpoint {
    method = GET,
    path = "/api/cool_api",
}]
async fn cool_api(
    rqctx: Arc<RequestContext<ServerContext>>,
    query: Query<Args>,
) -> Result<Response<Body>, HttpError> {
    let x: Response<Body> = Response::builder()
        .header(http::header::CONTENT_TYPE, "audio/mp3")
        .header(http::header::CONTENT_LENGTH, "hello".len())
        .status(StatusCode::OK)
        .body("hello".try_into().unwrap())?;
    Ok(x)
}

When I try to compile that code, I get following error. What do I need to do to get that working?

    |
277 | async fn cool_api(    |          ^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for `fn(Arc<RequestContext<ServerContext>>, dropshot::Query<Args>) -> impl Future {<impl From<cool_api> for ApiEndpoint<<Arc<RequestContext<ServerContext>> as RequestContextArgument>::Context>>::from::cool_api}`
    | 

50  |         FuncParams: Extractor + 'static,
    |                                 ------- required by this bound in `ApiEndpoint::<Context>::new`

Thanks in advance!

As I was giving Dropshot a go, I noticed that the relatively critical endpoint attribute macro is not documented:

I was able to figure out some of the behaviour by looking at the Oxide API prototype, but obviously some documentation would be good -- especially if it covered how to get path parameters working, as that took me a little to figure out.

Things are otherwise shaping up quite well, though!

If consumers try to define a ScanParams type for use with PaginationParams, and their type uses a non-String like a boolean or u64, this will go badly at runtime. In particular, they'll never successfully deserialize that parameter. Take this example in test_pagination.rs, which has a doit: bool querystring parameter. If you remove the workaround described below and run the test, it fails like this:

$ cargo test --test=test_pagination test_paginate_with_required_params -- --nocapture
   Compiling dropshot v0.3.0 (/Users/dap/oxide/dropshot/dropshot)
    Finished test [unoptimized + debuginfo] target(s) in 13.49s
     Running target/debug/deps/test_pagination-ed16123546a18dbb

running 1 test
log file: "/var/folders/x3/6tbrhnss6zz677z4zjsyvkqc0000gn/T/test_pagination-ed16123546a18dbb-required_params.92084.0.log"
note: configured to log to "/var/folders/x3/6tbrhnss6zz677z4zjsyvkqc0000gn/T/test_pagination-ed16123546a18dbb-required_params.92084.0.log"
thread 'test_paginate_with_required_params' panicked at 'assertion failed: `(left == right)`
  left: `"unable to parse query string: data did not match any variant of untagged enum RawWhichPage"`,
 right: `"you did not say to do it"`', dropshot/tests/test_pagination.rs:655:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test test_paginate_with_required_params ... FAILED

failures:

failures:
    test_paginate_with_required_params

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 8 filtered out

error: test failed, to rerun pass '-p dropshot --test test_pagination'

The operative part is the error unable to parse query string: data did not match any variant of untagged enum RawWhichPage. This is wrong -- the querystring did match the variant (and there's no querystring you can use that will work).

The root cause here is serde-rs/serde#1183, via nox/serde_urlencoded#66 (and, under an earlier alternative design, nox/serde_urlencoded#33). We use an untagged enum in RawWhichPage in order to distinguish between the WhichPage::First case and the WhichPage::Next case.

Fixing this

The problem we have is that we want the querystring parameters to match what's idiomatic for REST APIs -- i.e., presence of "page_token" means WhichPage::Next, while anything else means WhichPage::First. An untagged enum implements exactly this behavior but has the above problem. The challenge is that whatever form I can think of that we can parse the querystring to that allows us to see whether "page_token" is present, we can't then try to deserialize the result again as the user's struct.

I looked into using serde_value to do this, because that's an intermediate form that you can serialize to or deserialize from and it has a Map(BTreeMap<Value, Value>) variant. But I believe when coming from serde_urlencoded, the value in the map is always a String and it won't try to convert it to a bool or u64 or whatever when you deserialize it.

@ahl and I talked about implementing Deserializer (not Deserialize) for BTreeMap<String, String> that does try to parse a string value to some other type when that other type is requested.

Workaround

There's a workaround for this from @dtolnay's comment on serde-rs/serde#1183. This works, but you also need to apply a schemars attribute because schemars only supports serde(with) attributes whose values are actual types, not modules. The workaround looks like this:

    /* Work around serde-rs/serde#1183 */
    #[schemars(with = "bool")]
    #[serde(with = "serde_with::rust::display_fromstr")]
    doit: bool,

Hey there! I was playing around with Dropshot, and noticed there isn't anything in the documentation yet about auth/security. Do you have any recommendations on how to do auth with Dropshot, or have any plans to add support for oauth or other auth strategies?

I'm not very familiar with OpenAPI yet, but I noticed in the spec (http://spec.openapis.org/oas/v3.0.3) that each endpoint has an optional security section, and you can define security schemes to use throughout the API Schema. Would it be in scope for Dropshot to generate these sections?

One imagines many dropshot servers will be straight REST APIs, where clients are just libraries or commands and uniform JSON error messages make sense. Some, however, may also serve other artefacts; e.g., a web server that had an API component, but also an interactive HTML component that a browser may access with some pages and images and such.

Presently dropshot will produce a stock uniform JSON error response for, say, endpoints that do not map to a handler. If the client sends an Accept header that suggests they understand or prefer a HTML response, it'd be good to be able to choose somehow to render a reasonable 404 page or event a 302 redirect to the right place for browsers.

We may do this in several PRs, but the end state should be that the various error types are well-captured for each endpoint. This will likely require us to think more carefully about the specific return types we want to expose to developers, the appropriate granularity, and the types of remediation we expect developers to take in the case of failures.

There are types of failures where we have more information:

• we could provide detailed information about input validation failures
• we could provide specific guidance about retries in the case of 503 (service unavailable)
• etc.

Ideally we should make use of the components/responses field of OpenAPI to minimize the number of times these shared error structures are represented in the OpenAPI document.

Currently, the next_page token on ResultsPage is populated whenever the page that we're returning to the client has any items in it. This works, but it means that for any scan through a collection, the client needs to make an extra request at the end to fetch an empty page in order to see that it's the end of the scan. Ideally, the last page with any results on it would indicate that there were no more results and we could skip this extra request. The problem is that right now, Dropshot doesn't know this information. One simple interface would be for the consumer to provide the limit that they used, and Dropshot could say that if they provided fewer items than the limit, then it's the end of the collection. (This would still require a theoretically request that's theoretically unnecessary if the scan ends with a full page of items, but this seems a lot less likely.) We could also ask the consumer to provide an explicit boolean indicating whether there are more items.

Another consideration: I've worked with systems that would fetched N records from the database, fail to process 1-2 of them, and send the results back to the client. If a consumer worked this way, using the interface I described above, we would erroneously conclude that because the page was non-full, we've finished the scan -- having omitted an arbitrarily large number of results after that. Obviously that's just a bug in the consumer, but it'd be nice to make that impossible if we can.