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

support OpenApi #89

Open
xoac opened this issue Sep 4, 2018 · 27 comments
Open

support OpenApi #89

xoac opened this issue Sep 4, 2018 · 27 comments
Labels
rfc Extra attention is needed

Comments

@xoac
Copy link

xoac commented Sep 4, 2018

I didn't find any rust server that support OpenApi. I think it could be supported as future and should be easy to done in warp.

I mean I write a server code and I get sth like this http://petstore.swagger.io/.

What do you think?

@seanmonstar
Copy link
Owner

I think supporting something like this would be very useful! It'd probably require adding a method to Filter that allows it to describe itself, and combinator filters can call inner filters to build up their "description".

@algermissen
Copy link
Contributor

In my opinion, the question of whether it is a good idea for documentation of RESTful (if that is ones goal when using HTTP) services to be auto generated from source code instead of being stable across source code changes (having a decoupled lifecycle) is very debatable.

My suggestion would be to steer clear of any such opinionation in a core library such as warp and have others build sth on top. This on-top library can can serve the same purpose without baking a current fashion into warp.

@xoac
Copy link
Author

xoac commented Sep 4, 2018

I would suggest to leave filter as there are. And create a Description that takes warp::filter.

You can see this https://github.com/oktal/pistache/blob/master/examples/rest_description.cc#L59

@dmexe
Copy link
Contributor

dmexe commented Sep 4, 2018

Also related feature: labels for endpoints

let filter = warp::get().and(warp::path("book")).and(warp::param("id"));
assert_eq!("/book/{id}", filter.label());

@xoac
Copy link
Author

xoac commented Sep 5, 2018

@dmexe It could be in test module

@seanmonstar seanmonstar added the rfc Extra attention is needed label Oct 5, 2018
@remexre
Copy link
Contributor

remexre commented Dec 29, 2018

Has anyone started on this? If not, I might take a swing at it in 2019.

@remexre
Copy link
Contributor

remexre commented Jan 21, 2019

I've been thinking more about the design of this feature; the general approach I'm thinking of would be to have a Filter::describe() method that returns a fairly generic Description struct, and a separate OpenAPIFormatter or the like.

@algermissen

In my opinion, the question of whether it is a good idea for documentation of RESTful (if that is ones goal when using HTTP) services to be auto generated from source code instead of being stable across source code changes (having a decoupled lifecycle) is very debatable.

Presumably, this would become part of testing infrastructure; with a test like

#[test]
fn api_matches_blessed() {
    let expected = parse_json(include_str!("blessed_api.json")).unwrap();
    assert_eq!(routes().describe().to_json_value(), expected);
}

(This is the main thing I want it for, at least.)

My suggestion would be to steer clear of any such opinionation in a core library such as warp and have others build sth on top. This on-top library can can serve the same purpose without baking a current fashion into warp.

Is there a way to create such an addon library without exposing all of warp's implementation? I think it'd make sense to feature-gate this if the performance cost is non-negligible; I'd suspect it would be though, since the .describe() method should be dead code. (Actually, on second thought, I'm not sure if LLVM can dead-code-eliminate trait methods that are used in trait objects.)

@xoac

I would suggest to leave filter as there are. And create a Description that takes warp::filter.

You can see this https://github.com/oktal/pistache/blob/master/examples/rest_description.cc#L59

Wouldn't this require duplicating the code for each handler? i.e., documentation would live separately from code, so it'd be possible for an endpoint to start accepting XML in addition to JSON, but the programmer forgets to change .consumes(MIME(Application, Json)) to .consumes(MIME(Application, Json), MIME(Application, Xml)))?

@hgzimmerman
Copy link
Contributor

I'm attempting to prototype something that can output JSON that can be used by OpenApi, but I'm running into problems with the pervasive use of Copy throughout the codebase.

My approach would involve Filter getting a describe() function, which would output a tree comprised of Description enums which could then be walked to produce an OpenApi specification. Something like:

impl<T, U> FilterBase for And<T, U>
...
    fn describe(&self) -> Description {
        Description::And(Box::new(self.first.describe()), Box::new(self.second.describe()))
    }
}

But in order for things like path() to be describable, FilterFn (or something like it) would need to gain a description field. Unfortunately, because Description couldn't implement Copy because of non-copy datastructures contained within, the resulting FilterFn also couldn't be Copy, making it out of line with the trait constraints specified in many of the return statements within the project. The constraints can be relaxed to use Clone instead, but that is a breaking change with a significant negative effect on the ergonomics of warp.

I'm inclined to think that this approach is a dead-end.

@xoac
Copy link
Author

xoac commented Oct 31, 2019

I see somebody is working on similar solution for rocket https://github.com/GREsau/okapi

@kdy1
Copy link

kdy1 commented Jan 24, 2020

@xoac Please check https://github.com/kdy1/rweb

@fhsgoncalves
Copy link

Is there a way to label the routes, like @dmexe mentioned here: #89 (comment) ?

My idea is to discover which was the route that was requested and collect metrics about it. The point is that I can not use the request path because it contains ids that vary between the requests, so I can not use them as key to the aggregation. The route labels could be really useful for me, if I could get it from the response, for example.

Is there any way to do that, currently?

@HiruNya
Copy link

HiruNya commented Feb 1, 2020

I tried doing this aswell and got a somewhat working fork.

Since the petstore specification was mentioned in the first comment, I made one based on my fork.
This should hopefully be a good enough example of the API.

You can see the documentation created by the openapi file.

And this is the openapi.json file created.

I have not touched any existing function signatures but this required me to give up on things like

  1. Documenting the type returned by path::param (I resorted to matching the TypeId to check whether it was a String or an Integer (or it defaults to Object))

  2. Documenting the type returned by query and json (Deserialize does not give me enough type information to generate documentation on).
    This means that every field will need to be explicitly documented and so I would recommend using a derive macro in the future. (As you can see from the petstore example, it can get very verbose)

It relies on a Filter::describe(&self, route: RouteDocumentation) -> Vec<RouteDocumentation> trait method, but this has a few problems:

  1. Only filters can add to the description, therefore the return/response cannot be described because a response is only created when the server is running and so it can't be described on demand.
    I initially had a DocumentedReply trait which had a describe(route: RouteDcoumentation) -> Vec<RouteDocumentation> method (that didn't take self) that sort of worked for cases where the type was guaranteed (e.g. the map function would always return a JSON, etc.) but I wasn't able to merge it into the general Reply trait due to Reply needing to be boxable which would erase the type-specific information.
  2. Allocating a Vec for every filter is a bit expensive so a description shouldn't be created often. I initially had started of with a DocumentedFilter trait which let you define what output struct to use in the hopes that I could eventually switch to smallvec or some other stack based implementation but when I merged the trait into FilterBase, I was not able to define a default associated type for the trait.

Regarding the API, it generally looks something like this:

use warp::{get, document::{describe, document, description, response, RouteDocumentation}, filters::path::path, document};

let routes = get()
    .and(path("cat"))
    .and(document(description("This gets the name of a cat")))
    .and(document(response(200, None)))
    .map(|| "Muffin");

let docs: Vec<RouteDocumentation> = describe(&routes);

let openapi = warp::document::to_openapi(docs);

Note: the OpenAPI part is currently feature-gated and can definitely be put into an external crate.

A problem with this is warp::document::document always returns a Filter that implements Clone (not Copy). warp::document::explicit on the other hand returns a Filter that implements Copy if the original filter and function both implement Copy.

Regarding the question on labels, this

let filter = warp::get().and(path("book")).and(param());
assert_eq!("/book/{0}", describe(filter).pop().unwrap().path);

should work (I haven't tried it) but unfortunately naming the parameter would require a change in the param function so the only option would be to define your own param filter method like what I did in petstore.

Macros like in rweb would make a lot of this much simpler.

@xla
Copy link
Contributor

xla commented Apr 11, 2020

@HiruNya Thanks for sharing all that context to your awesome work. Is something in the way of opening a PR with your changeset against upstream? I'm very interested in this functionality to aid our API documentation needs and eager to contribute to the feature in general.

@HiruNya
Copy link

HiruNya commented Apr 12, 2020

Thanks @xla, university started so I put the fork on hold for a while but I do have time to contribute in the next few weeks.

The only thing stopping me from making a PR is:

  1. My code is a mess because it was mainly a proof of concept that I used for a few small projects.
  2. I feel like adding more features to it will require the API to also change which is problematic as I believe Warp should probably have a stable API.

One method to address this might be by only including the bare minimum needed in warp itself and hiding them with #[doc(hidden)]. Then an external crate like warp-docs could provide a proper API for it.

I'm keen to hear your opinion about this.

@xla
Copy link
Contributor

xla commented Apr 14, 2020

@HiruNya Thanks for your follow-up. See some ramblings inline.

My code is a mess because it was mainly a proof of concept that I used for a few small projects.

I'm happy to take your initial draft and refactor it along the needs we discover from integration into one of our projects: radicle-dev/radicle-upstream

I feel like adding more features to it will require the API to also change which is problematic as I believe Warp should probably have a stable API.

Started to build on top of your fork, getting some additions I needed immediately. Check the commits and let me know if any of this is off base.

One method to address this might be by only including the bare minimum needed in warp itself and hiding them with #[doc(hidden)]. Then an external crate like warp-docs could provide a proper API for it.

Ha! After integrating against your prior work I also come to believe that the lions share of the work should happen in an external crate. If I understand correctly, you had to make some invasive changes to some of the core implementations to introspect properly. Is this avoidable, if not what is a minimum changeset going to look like to enable an external crate like warp-docs? Curious to hear maintainers opinion on this as well. /cc @seanmonstar @jxs

A point I'm particularly keen on is to move a lot of the boilerplate into macros, they are an ideal candidate as you outlined in the example repo. Also think a tighter integration with already existing serde annotations makes a lot of sense.

My proposed strategy going forward would be to start a fresh crate which provides the functionality of the document module, while maintaining a warp fork with the minimal changes to core implementations necessary to enable the document features. This will make it easy to keep up with upstream, avoid merge dread and have a foundation for a very scoped PR in case these changes will be proposed to make it into warp proper. Curious to hear your thoughts on that approach!

@HiruNya
Copy link

HiruNya commented Apr 14, 2020

Started to build on top of your fork, getting some additions I needed immediately. Check the commits and let me know if any of this is off base.

Those look great! I've been wanting to add support for oneOf but never got around to doing it.

If I understand correctly, you had to make some invasive changes to some of the core implementations to introspect properly. Is this avoidable, if not what is a minimum changeset going to look like to enable an external crate like warp-docs?

Yeah unfortunately since all the structs are sealed and FilterFn only contains a callback, I needed to add implementations for them in the warp crate.

Currently, I can think of three ways of doing this (in order from most changes to warp to least changes):

  1. Keep the FilterBase::describe(&self, route: RouteDocumentation) -> Vec<RouteDocumentation> method and thus also keep the RouteDocumentation struct and all the structs it depends on and move all the functions that remove boilerplate to warp-docs. The problem is that if we wanted to add more features, we would have to change the structs and fields in warp.
  2. Change the trait method to FilterBase::describe<R: RouteDocumentation>(&self, route: R) -> Vec<R> where RouteDocumentation would be a trait that is in warp; and warp-docs would provide the struct that implements that trait (like how serde only acts on a Serializer trait). That way you can change how the trait is internally represented but if you wanted to add support for something new (like how you added 'Tags') then we would need to add more trait methods in warp.
  3. Have our own trait FilterDoc in warp-docs that has a describe method and is implemented on all Filters. Users of the crate would then have to use our implementation of the filters that have been wrapped with an implementation of FilterDoc to get documentation, and filters that don't use our implementation would not be documented at all. This method would mean that it won't work with any function that returns Filter - it would have to use FilterDoc. But at this point we might be better off maintaining a fork which we can use to patch warp.

I'm interested in if you have any other ways this can be done.

A point I'm particularly keen on is to move a lot of the boilerplate into macros, they are an ideal candidate as you outlined in the example repo. Also think a tighter integration with already existing serde annotations makes a lot of sense.

I've been keeping an eye on #395 which was suggesting an external warp-macros crate and I believe the author of that issue also had an implementation of OpenAPI himself so if that goes through they might be able to include support for that in warp-macros.

My proposed strategy going forward would be to start a fresh crate which provides the functionality of the document module, while maintaining a warp fork with the minimal changes to core implementations necessary to enable the document features.

I completely agree with this especially as it would give the fork some time to mature and hopefully stabilise - would you like to create this crate or would you rather I do it?
(If I do it, I would give you write permissions to it as you seem to have a proper use case for it)

Tomorrow I'm going to merge my fork with your fork's branch and I'll try out different ways of minimising the code in the main crate.

@mezeipetister
Copy link

Hi Guys! Do you have any updates on it? It would be an awesome feature!

@dancespiele
Copy link
Contributor

This will be a really great feature. Also waiting for news and I can help with it

@Type1J
Copy link

Type1J commented Oct 11, 2020

Warp with OpenAPI would be great! Please finish this work.

@xla
Copy link
Contributor

xla commented Oct 15, 2020

We dropped OpenAPI in our server a while back it still required too much manual annotation which easily went out-of-date. There is a lot more work to be done before it an ergonomic solution is possible. Namely easier derived information from actual filter definitions and macros.

@Type1J
Copy link

Type1J commented Oct 16, 2020

Yeah, now that grpc-web has streaming support (with the Improbable proxy at least), we may just be using Tonic instead.

@ibraheemdev
Copy link

paperclip is a wip OpenAPI tool for Rust. They currently have an actix-web plugin for generating swagger specs. I'm not sure how difficult it would be to create a warp plugin...

@simonsan
Copy link

Yep, that would be awesome!

@freiguy1
Copy link

freiguy1 commented Jul 1, 2021

I'll just leave this here as maybe some motivation - my company is fairly open about using different languages for web API microservices. One requirement however is that all microservices must support OpenAPI so a familiar interface always exists no mater the backend technology. It makes it much easier for hooking up a client. That requirement restricts me from using something like warp which I would like to use some day!

@kamlesh-nb
Copy link

Hi Guys, Did you tried this crate, it works fine with AXUM, should work with Warp too.

https://docs.rs/utopia/latest/utopia/

@GiKyouGetsu
Copy link

The site is empty, there is no any docs.....

@GiKyouGetsu
Copy link

Hi Guys, Did you tried this crate, it works fine with AXUM, should work with Warp too.

https://docs.rs/utopia/latest/utopia/

Wrong link , patse the correct link to here: https://docs.rs/utoipa/latest/utoipa/

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

No branches or pull requests