[docs] update extractor docs: fix a typo, increase verbosity

[joelsa]

This fixes a copy-paste typo in the docs for https://docs.rs/dropshot/latest/dropshot/struct.Header.html and adds some information about how the types for extractors should look like.

Like the user in #1244, I've run into the same issue that enums need to be wrapped in an external struct and also need to be flattened with the serde_flatten attribute. I believe this PR makes it more clear and thus fixes #1244, as the actual bug (#1247) was fixed in #1330 and only a documentation issue remained.

[davepacheco]

Thanks for the improvement! There seems to remain some confusion here.

The text you added was:

[...Type may be any struct of yours that implements serde::Deserialize and schemars::JsonSchema] where primitives and enums have to be wrapped in an outer struct and enums need to be flattened using the #[serde(flatten)] attribute

It's definitely not true that primitives have to be wrapped for all these extractors. I just tested this change to dropshot/examples/basic.rs:

dap@ivanova dropshot $ git diff
diff --git a/dropshot/examples/basic.rs b/dropshot/examples/basic.rs
index 6dc4a81..e508503 100644
--- a/dropshot/examples/basic.rs
+++ b/dropshot/examples/basic.rs
@@ -96,18 +96,18 @@ async fn example_api_get_counter(
 }]
 async fn example_api_put_counter(
     rqctx: RequestContext<ExampleContext>,
-    update: TypedBody<CounterValue>,
+    update: TypedBody<u64>,
 ) -> Result<HttpResponseUpdatedNoContent, HttpError> {
     let api_context = rqctx.context();
     let updated_value = update.into_inner();
 
-    if updated_value.counter == 10 {
+    if updated_value == 10 {
         Err(HttpError::for_bad_request(
             Some(String::from("BadInput")),
-            format!("do not like the number {}", updated_value.counter),
+            format!("do not like the number {}", updated_value),
         ))
     } else {
-        api_context.counter.store(updated_value.counter, Ordering::SeqCst);
+        api_context.counter.store(updated_value, Ordering::SeqCst);
         Ok(HttpResponseUpdatedNoContent())
     }
 }

and it works as expected. I expect enums would work as types for TypedBody as well.

Enums even work as query parameter values, provided they have no data. This one also worked as expected:

diff --git a/dropshot/examples/basic.rs b/dropshot/examples/basic.rs
index 6dc4a81..71ab92f 100644
--- a/dropshot/examples/basic.rs
+++ b/dropshot/examples/basic.rs
@@ -88,6 +88,17 @@ async fn example_api_get_counter(
     }))
 }
 
+#[derive(Deserialize, JsonSchema)]
+struct MyCounter {
+    value: MyCounterValue,
+}
+
+#[derive(Deserialize, JsonSchema)]
+enum MyCounterValue {
+    Big,
+    Small,
+}
+
 /// Update the current value of the counter.  Note that the special value of 10
 /// is not allowed (just to demonstrate how to generate an error).
 #[endpoint {
@@ -96,18 +107,21 @@ async fn example_api_get_counter(
 }]
 async fn example_api_put_counter(
     rqctx: RequestContext<ExampleContext>,
-    update: TypedBody<CounterValue>,
+    update: dropshot::Query<MyCounter>,
 ) -> Result<HttpResponseUpdatedNoContent, HttpError> {
     let api_context = rqctx.context();
-    let updated_value = update.into_inner();
+    let updated_value = match update.into_inner().value {
+        MyCounterValue::Big => 100,
+        MyCounterValue::Small => 10,
+    };
 
-    if updated_value.counter == 10 {
+    if updated_value == 10 {
         Err(HttpError::for_bad_request(
             Some(String::from("BadInput")),
-            format!("do not like the number {}", updated_value.counter),
+            format!("do not like the number {}", updated_value),
         ))
     } else {
-        api_context.counter.store(updated_value.counter, Ordering::SeqCst);
+        api_context.counter.store(updated_value, Ordering::SeqCst);
         Ok(HttpResponseUpdatedNoContent())
     }
 }

What I think is true is that for the Query, Header, and Path extractors, the type parameter on the extractor must describe keys and values, since query parameters, headers, and path parameters are by definition key-value pairs. In practice, we basically expect people to use a struct here. (Primitives are definitely out because what would the key be? I bet you could use an enum if you made sure that its JsonSchema at the top level was an object, rather than a oneOf, but I think you'd have to impl JsonSchema yourself because even an adjacently-tagged enum has a oneOf at the top level.)

I believe it's also true that the values must be flat, not nested. This comes from the fact that the underlying thing (query parameter value, header value, component in a URL path) is a string and there is no obvious way to parse that as a collection. (You could relate this to the fact that FromStr isn't impl'd for Vec<T>. There are many ways to parse a Vec<T> from a string, but none so obvious and universal that Rust would do it for you. Neither does Dropshot.)

Tangentially, long ago we discussed having these type parameters impl some new trait that's more specific than Deserialize. That could make these restrictions evident at compile-time. This would be nice!

---

What does any of this mean for this PR? For TypedBody, I would just make the small change to mention JsonSchema. The other restrictions don't apply to it.

For the other three, I would probably clarify this in the top-level crate docs instead of copying it into each one. There are already bullets like:

Query<Q> extracts parameters from a query string, deserializing them into an instance of type Q. Q must implement serde::Deserialize and schemars::JsonSchema.

I'd add to these bullets: "See below for additional restrictions."

Then below that, add a new paragraph saying something like:

Generally, the type parameter for the Query, Header, and Path extractors should be Rust structs. The struct's field names correspond to the keys in the thing being extracted (i.e., they correspond to query parameter names for Query, header names for Header, and path component names for Path). The struct's field values should generally be Rust primitives, strings, or enums containing no data. There is no facility for automatically parsing the values again (e.g., as JSON), which means nested values or enums with data cannot be supported.

Then we could emphasize the sentence in each extractor's docs saying to look at the top-level crate docs.

[davepacheco]

Suggested change

	/// documentation formore information.
	/// documentation for more information.

[davepacheco]

Suggested change

	/// `#[serde(flatten)]` attribute. While headers are accessible through
	/// [RequestInfo::headers], using this extractor in an entrypoint causes header
	/// inputs to be documented in OpenAPI output.
	/// `#[serde(flatten)]` attribute. While headers are always accessible through
	/// [RequestInfo::headers] even without this extractor, using this extractor causes header
	/// inputs to be documented in OpenAPI output.

[davepacheco]

Suggested change

	/// attribute. See this module's documentation formore information.
	/// attribute. See this module's documentation for more information.

[joelsa]

Thanks for this write-up @davepacheco. This really clarifies what should work and what shouldn't and it's already good documentation in itself. I'll update the PR and try to reflect that in the doc comments.