OpenAPI attribute appears to break named lifetimes in the route handler.

[twitchax]

Basically what the title says. 😄

If I change to #[openapi(skip)], it works just fine, so I think there is some problem occurring with how the attribute generates code. The error message is

use of undeclared lifetime name `'r`

So, it seems like the attribute may be stripping the named lifetime argument from the method definition.

[ralpha]

Hmm, this does not look like an easy thing to fix. Will have to take a deeper look if we want to fix this.

But why are you using a &str instead of a String in the function parameters? I don't know if there are any upside to that as Rocket can just give the String, and using &str will not change anything. The worst might be that it will make a copy. Although I don't think that even happens. Because FromForm and FromFormField does not require Copy or Clone. (and even if it did, the performance different is non existing ether way).

If someone can show me a use-case where lifetimes are always needed for the function parameters, please share me and I'll take a look at this. But other then that I don't think this is really an issue (that needs fixing). If you think I'm wrong in this assumption, please let me know.

[twitchax]

Hi @ralpha,

All good questions. :)

A String, by definition, is owned, and I do not need an owned String: I just need the slice. I assume that, if Rocket gives you an owned String it absolutely has to clone (see FromParam for String [1]) that &str data from the form, otherwise, there is nothing to give you to "own". Why waste a clone if I don't need it?

However, and more importantly, most of the return type GetOembedResponse<'r> is able to just return some of those parameter values inside of itself as string slices (&'r str). All in all, I save about 8 String clones per call, which is trivial for each call, but not trivial for thousands or millions of calls.

Really, though, it's not the &'r str inputs that causes this. If you just use &str inputs, openapi is fine, but if you have an output with a lifetime (as I do here), then you have to use a named lifetime. It seems that openapi drops this named lifetime when doing its code generation.

Even if my use case is not valid because it seems my performance needs are not valid, having a named lifetime in a Rocket handler is definitely a valid use case, and, it is currently broken. For me, personally, I didn't need this API to use openapi, so I just skip this endpoint, and I filed the bug because named lifetimes are a valid Rocket scenario, and someone else may run into this.

[1] https://api.rocket.rs/master/src/rocket/request/from_param.rs.html#195-203

[twitchax]

Actually, lol, it literally has a TODO, which is awesome.

// TODO: Tell the user they're being inefficient?

[ralpha]

Thanks for the replay. You made your case, I'll take a look into allowing lifetimes. (It might not be this release, because this requires changing the derive macro and is sometimes not so strait forward)

I want to note though that you are using FromFormField not FromParam because you are using query parameters, not path parameters. For more info see this table: https://rocket.rs/v0.5-rc/guide/requests/#query-strings (still does the .to_string() though)

Not cloning values might be good for performance. But might make code more difficult to maintain (sometimes). So always keep that in mind too. But use-cases also differ, so deepens on your situation. Also please measure performance, most often cloning values is not even close to making an impact compared to I/O, DB queries, or other code parts.

But enough about performance, lets not get to carried away. :laughing:

I'll let you know once I have take a look at how this can be fixed.

[twitchax]

I want to note though that you are using FromFormField not FromParam because you are using query parameters, not path parameters. For more info see this table: https://rocket.rs/v0.5-rc/guide/requests/#query-strings (still does the .to_string() though)

Ha, nice, I did not know that: thanks for the tip!

Yeah, I agree about the performance. In this case, since it's an oembed response, I don't make any I/O, or DB calls. Again, you're definitely right that I likely don't need this performance win of saving those clones. I just got in the habit of using &str everywhere in my path / query params since I rarely need owned Strings.

Thanks for taking a look, but no need to rush on my account. As I said, this is sort of a "hidden" API for me, anyway, so I don't need OpenAPI generation. I figured someone else might run into this at some point, so I'd let you know.

Awesome work, and I considered taking a stab at it, but my proc macro knowledge is on the order of 1e-9, so I would likely cause more harm than good. 🤣

[clotodex]

@twitchax I am now running in the same issue, however I need to lifetime the State in rocket, which I cannot own but only borrow - so I need the lifetimes.

Example:

#[openapi]
#[post("/chat/<chat_id>", format = "json", data = "<msg>")]
async fn chat<'x>(
    chat_id: String,
    msg: Json<Message>,
    db: &'x State<Database>,
    backend: &'x State<Backend>,
) -> EventStream![Event + 'x] {
    EventStream! {
        let app = App { db: db.inner() , backend: backend.inner() };
        let stream = app
            .chat(chat_id, msg.into_inner())
            .await
            .unwrap();
        for await msg in stream {
            yield Event::json(&msg);
        }
    }
}

And I cannot set lifetime 'x

[SohumB]

I have a similar situation where I want to return a rocket::stream::TextStream deriving from an input parameter with a request lifetime, but the route also takes an additional &State<...> argument. The returned TextStream needs a lifetime parameter that derives from both of those lifetimes, which is a case where rust's lifetime inference fails explicit — it asks you to annotate that explicitly.

[gahag-cw]

+1, also facing this issue.

[PixelboysTM]

same issue here