GREsau / okapi

OpenAPI (AKA Swagger) document generation for Rust projects
MIT License
631 stars 112 forks source link

`#[openapi]` should handle `#[deprecated]` #106

Closed matt-phylum closed 1 year ago

matt-phylum commented 2 years ago

Currently, there does not appear to be any way to mark an operation as deprecated without fixing up the OpenApi document afterwards.

When using the #[openapi] attribute to generate documentation for a #[deprecated] handler,

  1. A warning about using a deprecated function should not be emitted at the handler function declaration. I'm pretty sure this is coming from #[openapi] and not Rocket's endpoint attribute (#[put] in my case), but I didn't remove it to verify. In any case, we're giving meaning to the #[deprecated] attribute where it didn't really have one before, so if Rocket is generating this warning, okapi should probably suppress it if possible. Most likely, the warning should still be generated when mounting the route and that can be left up to the user to suppress.
  2. In the generated API spec, the corresponding operation should be marked as deprecated.

This behavior is consistent with schemars.

ralpha commented 2 years ago

Just to make sure I understand it correctly:

And regarding the respecting of the #[deprecated] and reflecting it in the spec. I agree.

matt-phylum commented 2 years ago

I just checked and #[deprecated] is generating a warning on declaration, with or without #[openapi].

use rocket::{get, routes, Build, Rocket};
use rocket_okapi::openapi;

#[get("/")]
#[deprecated]
fn handler_without_docs() -> &'static str {
    "ok"
}

#[openapi]
#[get("/")]
#[deprecated]
fn handler_with_docs() -> &'static str {
    "ok"
}

#[rocket::launch]
#[allow(deprecated)]
fn launch() -> Rocket<Build> {
    rocket::build()
        .mount("/without_docs", routes![handler_without_docs])
        .mount("/with_docs", routes![handler_with_docs])
}
   Compiling okapi_test v0.1.0 (/Users/matt.donoughe/Documents/okapi_test)
warning: use of deprecated function `handler_without_docs`
 --> src/main.rs:8:4
  |
8 | fn handler_without_docs() -> &'static str {
  |    ^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

warning: use of deprecated function `handler_with_docs`
  --> src/main.rs:17:4
   |
17 | fn handler_with_docs() -> &'static str {
   |    ^^^^^^^^^^^^^^^^^

warning: `okapi_test` (bin "okapi_test") generated 2 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 2.08s

IMO it would be great if rocket_okapi suppressed that warning. Adding #[allow(deprecated)] to the function has no effect. If that's not possible, maybe #[openapi] takes key value pairs for the different OpenAPI Operator properties so you can write #[openapi(deprecated = true)], among other things. Maybe that's good for other cases anyway, but it doesn't seem as natural as #[deprecated] for the deprecation case. Maybe later Rocket can suppress that warning so it works nicely with #[openapi].

ralpha commented 2 years ago

This warning has nothing to do with Rocket or Okapi. So there is noting we can do about it.

You can try:

#[rocket::launch]
fn launch() -> Rocket<Build> {
    #[allow(deprecated)]
    rocket::build()
        .mount("/without_docs", routes![handler_without_docs])
        .mount("/with_docs", routes![handler_with_docs])
}

But I'm not sure this will work. Otherwise you can try adding #![allow(deprecated)] tot the top of the file. But this is not really recommended.

There is a difference between depreciating a Rust function and deprecating an API endpoint.

At work I deprecate functions in the following way: I add a note to the top of the endpoint description.

/// **NOTE: This endpoint is deprecated (since 2022/xx/xx) and will be removed at some point.**

And I log the use of function to see where it is still used:

log::error!("Calling of deprecated endpoint: `/endpoint/path`");

I know this is not ideal. But it is good enough as a temporary solution. But using #[openapi(deprecated = true)] might indeed be a better solution. The spec also supports this: https://spec.openapis.org/oas/v3.0.0#fixed-fields-7

matt-phylum commented 2 years ago

The warning is emitted even if you don't mount the endpoint or otherwise use it in any way.

#![allow(unused)]

use rocket::{get, Build, Rocket};
use rocket_okapi::openapi;

#[get("/")]
#[deprecated]
fn handler_without_docs() -> &'static str {
    "ok"
}

#[openapi]
#[get("/")]
#[deprecated]
fn handler_with_docs() -> &'static str {
    "ok"
}

#[rocket::launch]
#[allow(deprecated)]
fn launch() -> Rocket<Build> {
    rocket::build()
}

Actually using the handler does not produce any warnings:

use rocket::{get, Build, Rocket, routes};
use rocket_okapi::openapi;

#[allow(deprecated)]
mod handlers {
    use super::*;

    #[get("/")]
    #[deprecated]
    pub fn handler_without_docs() -> &'static str {
        "ok"
    }

    #[openapi]
    #[get("/")]
    #[deprecated]
    pub fn handler_with_docs() -> &'static str {
        "ok"
    }
}

#[rocket::launch]
#[allow(deprecated)]
fn launch() -> Rocket<Build> {
    rocket::build()
        .mount("/without_docs", routes![handlers::handler_without_docs])
        .mount("/with_docs", routes![handlers::handler_with_docs])
}

I'm pretty sure #[get] is defining a type which implements something like Into<Route> and the warning is generated by that impl.

ralpha commented 2 years ago

I'm pretty sure #[get] is defining a type which implements something like Into and the warning is generated by that impl. Yeah the macro is using it internally somewhere. Strange that the handler thing changes that.

But other then #[openapi(deprecated = true)] I don't see and immediate solution.

matt-phylum commented 2 years ago

It might be possible for the #[openapi] attribute to remove the #[deprecated] attribute so Rocket doesn't see it, or wrap the Rocket-generated code in a module, but neither of those sound good, if they're even possible. I haven't tried to do anything like that in an attribute macro before.

Maybe Rocket can be convinced to change this as the current behavior doesn't seem useful at all.

I like both options. #[openapi(deprecated = true)] is good to have even if later #[deprecated] becomes an option, I think.

matt-phylum commented 2 years ago

I created SergioBenitez/Rocket#2262 for #[deprecated].

ralpha commented 1 year ago

Thanks for following up on this issue and resolving it! Very nicely done! :star_struck: