encapsulate use of #[derive(JsonSchema)]

[ahl]

Fixes #67

This effectively aliases the JsonSchema trait and exports the JsonSchema derive macro under the name ExtractedParameter this both removes the requirement that consumers depend on schemars and would allow us to swap out something else in the future (as seems likely).

I'd suggest starting with dropshot/src/handler.rs and dropshot/src/lib.rs -- the rest is basically search and replace.

[jclulow]

What if we just called it dropshot::Extract, like serde::Deserialize? It's shorter, and it isn't exactly clear that a request or response body object is, in its entirety, an extracted parameter per se.

[ahl]

ExtractedParameter was what @davepacheco and I had landed on originally. We replaced that with JsonSchema so I figured it made sense to revive that original name. I'll defer to Dave if he prefers Extract.

[davepacheco]

Thanks for doing this!

In terms of the name: when we were talking about this before, I'm not sure whether ExtractedParameter also referred to output types. (It's possible that it did and I didn't realize this at the time!) In this version, it definitely does: if you have a type that's returned by an API endpoint, it has to impl ExtractedParameter, which now feels like the wrong name. What about calling it Schema, since if I understand correctly, the defining property is that this thing have a way to generate a schema for itself, which might be used for input or output. dropshot::JsonSchema would probably be okay too since the schema it generates is probably specific to JSON Schema anyway.

Unrelatedly: I'm not sure about you but I'm not positive this will just work for all consumers. I don't have a specific fear in mind but I'd have more confidence if we updated oxide-api-prototype too. I'm happy to take a swing at that when we're ready here.

Lastly: we have a bunch of consumers now -- @jclulow, @jessfraz, and a few others that have filed issues. We probably want to start a changelog that documents breaking changes and how to update past them. I can take a swing at this when we're ready to publish the next version.

[davepacheco]

I'm not sure I grok this. Are these two lines saying: anything that implements ExtractedParameter also needs to impl JsonSchema, and further anything that implements JsonSchema also implements ExtractedParameter? It might be useful to add a (non-doc) comment explaining why we're doing this.

Does this get used anywhere? Is it just in trait bounds? I gather dropshot::ExtractedParameter refers to the JsonSchema derive macro, not this.

[ahl]

This says:

1. ExtractedParameter is a subtrait of JsonSchema that adds nothing new
2. Anything that impls JsonSchema also impls ExtractedParameter

So then the derive macro that we export called ExtractedParamter today just is an alias for the JsonSchema derive macro, but we could change it in the future to, say, the openapiv3 definition of schemas or something compatible.

I'm note sure I completely answered your question so definitely let me know if I didn't.

[davepacheco]

Here's the example where the thing deriving ExtractedParameter is not a parameter at all. It's an output type.

[ahl]

Totally.

[davepacheco]

Can we add a doc comment for this that explains to users what this is and when and how to use it? It doesn't need to be super detailed -- we could refer to the module level example if that makes sense. Maybe:

Derives [ExtractedParameter] for a type, enabling Dropshot to generate a schema for the type. This must be applied to types that are used as input parameters, outputs, and anything else that might have a schema that appears in the generated OpenAPI specification. See the crate-level documentation for examples.

Similarly, I think it would help a lot to document (in a non-doc comment) what's going on here. Maybe something like:

We expose the schemars::JsonSchema derive macro as dropshot::ExtractedParameter so that we can leverage the schemars implementation without consumers having to know about that crate. This will allow us to change implementations in the future. Note that technically, this macro derives the JsonSchema trait, not ExtractedParameter, but ExtractedParameter provides a blanket impl for all types that impl JsonSchema. The net result is that by using the dropshot::ExtractedParameter macro, consumers get an impl of the ExtractedParameter trait.

That might be too wordy -- I don't know. I think what you've done here is idiomatic (where there's a trait and a derive macro (that impls the trait) with the same name), but I find it awfully confusing when debugging compile errors. Even now, I'm not sure what I wrote is correct -- does schemars::JsonSchema refer to the macro or the trait?

[ahl]

Absolutely -- my bad. Thanks for the suggested text.

[ahl]

In terms of the name: when we were talking about this before, I'm not sure whether ExtractedParameter also referred to output types. (It's possible that it did and I didn't realize this at the time!) In this version, it definitely does: if you have a type that's returned by an API endpoint, it has to impl ExtractedParameter, which now feels like the wrong name. What about calling it Schema, since if I understand correctly, the defining property is that this thing have a way to generate a schema for itself, which might be used for input or output. dropshot::JsonSchema would probably be okay too since the schema it generates is probably specific to JSON Schema anyway.

Yeah, that works for me. I'll make that change.

Unrelatedly: I'm not sure about you but I'm not positive this will just work for all consumers. I don't have a specific fear in mind but I'd have more confidence if we updated oxide-api-prototype too. I'm happy to take a swing at that when we're ready here.

Lastly: we have a bunch of consumers now -- @jclulow, @jessfraz, and a few others that have filed issues. We probably want to start a changelog that documents breaking changes and how to update past them. I can take a swing at this when we're ready to publish the next version.

This should be backward compatible i.e. not a breaking change. However if we ever replace schemars::JsonSchema with something else then that would be a breaking change.

That said, I'll do two experiments: oxide-api-prototype as is against my branch and then update it. I'll run cargo test and figure that's good enough. Cool?

[davepacheco]

That said, I'll do two experiments: oxide-api-prototype as is against my branch and then update it. I'll run cargo test and figure that's good enough. Cool?

That'd be great. I'm also happy to help.

[ahl]

@davepacheco I forgot about this: https://github.com/oxidecomputer/oxide-api-prototype/blob/master/src/api_model.rs#L195

In the API prototype we have a custom JsonSchema for the ApiName type.

I now thing the right thing is just to re-export JsonSchema and not bother with this new Schema trait/derive at all. What do you think?

[ahl]

@davepacheco this was a great suggestion: I'm pretty convinced now that my approach is invalid. The schemars::JsonSchema derive macro emits code that explicitly references the schemars crate. I don't think it's possible to encapsulate its work and therefore I don't think it's possible to avoid requiring consumers of dropshot to depend on 1. schemars and 2. the specific version that we're using.

[ahl]

After doing some more digging, I don't think it's going to be possible to avoid having clients 1. enumerate a dependency on schemars and 2. keep that dependency version lined up with ours. The short of it is that schemars_derive generates code that directly references schemars::<item> e.g.

https://github.com/GREsau/schemars/blob/b8c548136a20669d3f4b2a7eed3ad199bb2adb2b/schemars_derive/src/lib.rs#L42

schemars must be in the namespace where those derive macros are used. Even if we alias the macro--and we can--the generated code will still make reference to schemars.

This seems to be endemic with several relevant issues open on the topic

• Proc macros: ability to refer to a specific crate/symbol (something similar to $crate) rust-lang/rust#54363
• Add a #[serde(crate_name = "…")] attribute serde-rs/serde#953
• how to reexport Serialize, Deserialize? serde-rs/serde#1465

The extern crate self as <name of crate> syntax seems related, but not quite what would be needed to solve this:

• Tracking issue for feature extern_crate_self rust-lang/rust#56409