Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Implement auto-documenting routes #1624

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThouCheese
Copy link
Contributor

Very large WIP, does nothing for now.

This PR aims at eventually closing #297, or at least providing some of the requested functionality. For now I have created the traits Documented and Undocumented, and I have updated the derive macro to pull in the doc comments of the route handler function. I am opening an early PR to get some feedback.

Unresolved questions (non-exhaustive :)):

  1. We will probably want users to be able to use their own documentation format: be it OpenAPI, or RAML, or something custom. How do we allow users to provide their own format? Which formats do we support out of the box?
  2. Rocket should probably come with one or a couple of documentation frontends built in: Redoc, Swagger, Rapi-Doc, ..... Again the question is which ones do we support out of the box? Do we provide a mechanism where users can hook in and set up their own front end or is it sufficient to just make them serve the HTML for their front end with a Route.
  3. What info do we need to collect for each route to be able to adequately document each route? How will we handle global settings (think authentication, maybe a server-spanning description, ...)

Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super excited for this. Some simplifications and forward-looking statements.

Thanks for submitting this early!

core/codegen/src/attribute/route/parse.rs Outdated Show resolved Hide resolved
core/codegen/src/attribute/route/parse.rs Outdated Show resolved Hide resolved
core/codegen/src/attribute/route/parse.rs Outdated Show resolved Hide resolved
core/codegen/src/attribute/route/mod.rs Outdated Show resolved Hide resolved
core/codegen/src/attribute/route/mod.rs Outdated Show resolved Hide resolved
core/codegen/src/attribute/route/parse.rs Outdated Show resolved Hide resolved
core/lib/src/route/route.rs Outdated Show resolved Hide resolved
core/lib/src/route/route.rs Outdated Show resolved Hide resolved
core/lib/src/route/route.rs Outdated Show resolved Hide resolved
@SergioBenitez
Copy link
Member

SergioBenitez commented Apr 30, 2021

  • We will probably want users to be able to use their own documentation format: be it OpenAPI, or RAML, or something custom. How do we allow users to provide their own format? Which formats do we support out of the box?

I think we have Rocket act as a data provider wherein Rocket allows any consumer to access all of the available documentation information. The information should be rich enough (richer, ideally!) to allow shipping standard doc consumers in Rocket directly, such as those for OpenAPI and RAML, while also allowing external consumers for custom formats.

Effectively, Rocket's core should be unaware of any standard like OpenAPI or RAML but provide an interface rich enough to allow extracting into such standards.

  • Rocket should probably come with one or a couple of documentation frontends built in: Redoc, Swagger, Rapi-Doc, ..... Again the question is which ones do we support out of the box? Do we provide a mechanism where users can hook in and set up their own front end or is it sufficient to just make them serve the HTML for their front end with a Route.

As above, Rocket itself should be agnostic to doc frontends as it should be for doc standards. We might consider shipping a fairing or custom handler or something of that sort of a few front-ends, whoever.

In general, I envision the following pipeline:

-------------------    -------------------------------    ------------
| Rocket Doc Data | -> | Standardized Representation | -> | Frontend |
-------------------    -------------------------------    ------------

Where Rocket's core participates only in the first stage and everything else can be implemented externally. Standards like OpenAPI and RAML live in the second box while frontends like Swagger and Redoc live in the third. You might even have just one stage after the first, or more than two; totally up to the consumer.

  • What info do we need to collect for each route to be able to adequately document each route? How will we handle global settings (think authentication, maybe a server-spanning description, ...)

If global, auth will be a fairing, which implies fairings should be able to add docs. If local, auth will be a guard, which we already know should be able to add docs. It would be nice to be able to read the crate docstring, but that's not accessible easily. I wonder if an attribute on a docstring works:

#[rocket(doc)]
/// Some docs to make global, or something.

This doesn't work because one binary might have more than one Rocket server, and the docs need to be per-server. Oh! We could take the doc-string from the launch or main method:

/// This is my application. Here are some nice docs.
///
/// These will show up somewhere.
#[launch]
fn rocket() -> _ { ... }

@ThouCheese
Copy link
Contributor Author

I have created some types that can do some of the lifting when documenting a type (I'm 100% sure that they are not sufficient even for just OpenAPI, but lets start small). I think the next steps should be to modify Rocket to collect and aggregate the documentation that we have now gathered for each route and to create a macro that can derive the Schema trait for responders and "requesters".

Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The different files for the different impls are unnecessary; they can and should all be in one file. Eventually, we want to dedup these by using macros, too.

In terms of Schema, we should start with structures that are able to describe the entirety of the serde data model, though several of these can be coalesced when reified. I wouldn't be surprised if structures that do so already exist somewhere. There's Content, which is a serialized version of this, and Figment's Value and this Value, which is similar but excludes Option, which we shouldn't. So, effectively something like these but without the actual values.

I don't think Schema here makes too much sense. min/max don't make sense for many types, for example. I think any sort of value spec should be per-value.

In any case, the next step is to actually grab the schema for the route types much like is done now for sentinels minus the part about embeddings.

core/codegen/src/attribute/route/mod.rs Outdated Show resolved Hide resolved
@@ -209,9 +211,11 @@ impl Route {
})
.collect();

let docstring = String::from_attrs("doc", &handler.attrs)?.join("\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you determine if we need the \n?

Copy link
Contributor Author

@ThouCheese ThouCheese May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need it, because Rust comments are markdown, which is also supported for OpenApi and RAML. Markdown is a newline sensitive format: # title \n remainder\n is not the same as # title remainder, so removing the newline would alter the structure of the docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is really whether rustdoc already inserts a new line or whether it infers one based on having multiple attributes. You should be able to easily check. We only need the new line here if it is inferred.

@ThouCheese
Copy link
Contributor Author

The different files for the different impls are unnecessary

Gone

we should start with structures that are able to describe the entirety of the serde data model

I have implemented HasSchema for every type in the serde data model, except for unit_struct, unit_variant, newtype_struct, newtype_variant, tuple_struct, tuple_variant, struct and struct, because it is not possible to do a generic implementation for those.

I think any sort of value spec should be per-value.

I agree that some of the spec must come from the value, but some of it can also come from the type. Many auto documenters provide defaults like true and "string" if no default is specified by the user. I think we should strive to do something similar.

In any case, the next step is to actually grab the schema for the route types much like is done now for sentinels minus the part about embeddings.

So would this mean adding a field to StaticInfo with the ingoing types and the outgoing types?

@SergioBenitez
Copy link
Member

The different files for the different impls are unnecessary

Gone

we should start with structures that are able to describe the entirety of the serde data model

I have implemented HasSchema for every type in the serde data model, except for unit_struct, unit_variant, newtype_struct, newtype_variant, tuple_struct, tuple_variant, struct and struct, because it is not possible to do a generic implementation for those.

What I'm suggesting is that we have a struct/enum that we can use to dynamically determine the serde data type of a given Rust type. I don't see where we would need generics for this.

I think any sort of value spec should be per-value.

I agree that some of the spec must come from the value, but some of it can also come from the type. Many auto documenters provide defaults like true and "string" if no default is specified by the user. I think we should strive to do something similar.

What I mean is that different data types have different "specs", so we shouldn't have one "spec" kind that applies to every data type.

In any case, the next step is to actually grab the schema for the route types much like is done now for sentinels minus the part about embeddings.

So would this mean adding a field to StaticInfo with the ingoing types and the outgoing types?

Not quite. See how this is done with Sentinels and the internal Sentry type. We should only add one field to Route.

@SergioBenitez SergioBenitez marked this pull request as draft May 23, 2021 03:09
Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, progress here seems a bit without aim. Instead of working forward in an ambiguous direction, I'd like for us to set an ideal target. What would be the gold standard here? The answer should include:

  • What the user has to do to render docs.
  • What the API will look like to add a doc renderer.
  • How the user can progressively document their app.

It would be ideal to work with a non-trivial example. Perhaps choose a real API, like the Bitwarden API or perhaps the Stripe API, imagine how endpoints might be written in Rocket, and then design a doc system, without concern for feasibility, that would allow rendering those kinds of docs as streamlined as possible.

core/codegen/src/attribute/route/mod.rs Show resolved Hide resolved
core/codegen/src/attribute/route/mod.rs Outdated Show resolved Hide resolved
@@ -319,6 +352,9 @@ fn codegen_route(route: Route) -> Result<TokenStream> {
let rank = Optional(route.attr.rank);
let format = Optional(route.attr.format.as_ref());

// Get the doc comment
let docstring = &route.docstring;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to separate this. Group it with the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean add it to the StaticInfo::schemas field?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just need style wise. Basically to just remove the whitespace and comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right okay

@@ -209,9 +211,11 @@ impl Route {
})
.collect();

let docstring = String::from_attrs("doc", &handler.attrs)?.join("\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is really whether rustdoc already inserts a new line or whether it infers one based on having multiple attributes. You should be able to easily check. We only need the new line here if it is inferred.

Set,
}

pub struct TypeSchema<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make much sense to me. name seems to be a string serialization of the kind enum, so why have it at all? Or alternatively, why not have the trait simply have a const string?

I think we need to start thinking more seriously about how schema is going to be represented. Otherwise, we will be bouncing back-and-forth between ideas without justification. A good approach here is the take a reasonably sized application and imagine the exact documentation we would like to render and work backwards from there. We also have the benefit of existing schema like JSON schema and so on. 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants