Is there a reason so many interfaces accept `String` instead of `&str`? about oauth2-rs

[ramosbugs]

Very reasonable question!

Most of the methods in this crate that accept String need to store the string (in self) beyond the lifetime of the method call. That requires storing either &'static str (#171) or String to avoid introducing lifetimes to each struct's generic types. Adding lifetimes would significantly complicate the interface, and I don't think it's worth it since the cost of a string clone is many orders of magnitude smaller than the overhead of the HTTP requests involved in OAuth2. This is also why I haven't merged #171. This would be over-optimization that isn't worth the complexity for code that isn't in a tight loop (in which case, the overhead can add up) or another situation in which nanoseconds matter. The HTTP requests are going to take tens of milliseconds in the best case, and any performance improvement from removing a few string clones will be dwarfed by typical variations in network latency.

That answers why we're storing Strings. The reason we're also passing Strings is that if we were to pass &str instead, each method would need to call .to_owned() or .to_string() internally before storing it. While I don't think the overhead of string clones is significant here, it's still nice to avoid them when it's easy to do so. In cases where the caller already has an owned String and doesn't need its value anymore, they can simply pass it into the method and give up ownership. If we accepted &str, we'd have to make a whole other copy even though the caller doesn't need theirs anymore.

What many Rust APIs do to improve the ergonomics in both cases, and which this library could also do (with a breaking change), is to accept generic arguments that implement Into<String>. This would allow callers to pass String, &str, or any other type that implements Into<String>. If they pass a String, the .into() call is a no-op. Otherwise, it'll clone the string. Rewriting this library was my first major Rust project, and I wasn't aware of that idiom at the time. I would probably be open to that change, though it would also require updating the openidconnect crate's APIs to match. I'm not sure it's worth the effort and breaking API changes.

[rdbo]

I'm finding this a bit odd as well. I'm in a situation where it seems that most calls to this library require a .clone():
Examples (NOTE: The 'settings' variable is static):

BasicClient::new(
    settings.google.client_id.clone(),
    Some(settings.google.client_secret.clone()),
    settings.google.auth_url.clone(),
    Some(settings.google.token_url.clone()),
)
.set_redirect_uri(settings.google.redirect_url.clone())

let (authorize_url, _) = client
    .authorize_url(CsrfToken::new_random)
    .add_scopes(settings.google.scopes.clone())
    .url();

I haven't looked at the codebase so I take the following take with a grain of salt.
I don't think it is a bad idea to introduce lifetimes to the structs. Most of the times, the compiler can figure out the lifetimes without you having to explicitly type it, so the end users of this library would probably not even notice (except for the fact that now they are passing references instead of owned strings now, which is good). Even though I agree that the string cloning will be a minor overhead compared to how long the HTTP requests take, it just doesn't feel right doing things this way in my opinion.

Hoping to see updates on this topic. Great work on this library 💯

[ramosbugs]

Looks like the current API is in line with the official Rust API Guidelines:

If a function requires ownership of an argument, it should take ownership of the argument rather than borrowing and cloning the argument.

Accepting &str and then cloning it before storing in self is explicitly against these guidelines.

[rdbo]

@ramosbugs I'm just wondering if ownership is really necessary here. For some things I can understand, like consuming the authentication code to exchange for a token - the code is no longer valid after the exchange, might as well consume it. As for the rest, like auth URL, token URL, etc, I'm not so sure.
If ownership is really necessary, I agree, don't take a reference and clone, that's just not ideal.
I might take some time to look into this further and see if I can propose a change or something.

[ramosbugs]

I think this is over-optimization and not worth the time and effort, especially with the breaking changes that would be involved. I'm unlikely to merge these sorts of changes, just as a heads-up.

[ikehz]

Looks like the current API is in line with the official Rust API Guidelines:

If a function requires ownership of an argument, it should take ownership of the argument rather than borrowing and cloning the argument.

Thank you for your thorough responses, as usual, @ramosbugs! This answers my question. I was considering the general robustness principle, but I understand why Rust's ownership model complicates that.