[Rust][REQ] Escape parameters instead of local variables

[xMAC94x]

@wing328 and I discussed the prefixes in local variables with generated Rust code. The following is a write-up trying to summarize our Ideas and discussion.

Is your feature request related to a problem? Please describe.

Current rust generated code, prepends all local variables with a local_var_ prefix. The intention of this prefix is, to avoid name collisions (See below a whole chapter on how they are possible right now).

However, this has 3 downsides:

1. the mustache code is harder to read.
2. the generated rust code is harder to read.
3. The prefix on local variables does not prevent name collisions, it just makes it harder. E.g.
   • No Prefix -> easy a common parameter like client would cause problems.
   • local_var_ prefix -> probably safe enough, no one will probably name their OpenApi Parameter local_var_client
   • local_var_31rsefw3eddfesrt234efae5zse6ftsera_ as a prefix -> if a user really uses local_var_31rsefw3eddfesrt234efae5zse6ftsera_client in their naming, they are crazy

Describe the solution you'd like

There exists a solution to it that solves 1) and 3) and greatly improves on 2) :
Instead of adding a prefix to the local variable we add a prefix to the parameter.

Why does adding a prefix to a parameter works, but not to a local variable?

When you want to protect against SQL injection you escape the user input instead of choosing weird Table names and hoping the client doesn't use them

OpenAPI parameters are user input, they can be whatever the user wants.
local variables are defined in mustache files, they are not user-input, but are carefully chosen by OpenApiTools and changes need to be reviewed and merged.

If we now escape the local_variables (e.g. by adding a prefix) we should slightly decrease the chance for a name collision).
However, if we properly escape the user-input, e.g. by adding some prefix p_ to the OpenAPI parameters, AND we don't use the same prefix on our local variables, we can AVOID all such collisions. Because it is possible to add more characters for user-input OpenAPI parameters, but they cannot get rid of the prefix p_ (let aside they try to inject rust code in some sneaky way, we limit ourselves to valid names here).

But changing parameter names disrupts the public interface

So before I opted to add a prefix to OpenAPI parameters in the generated code. But OpenAPI parameters are mapped to rust parameters, and we don't want to have an ugly function like

fn ugly_stuff_we_dont_want(configuration: &Config, p_user: String, p_pet_id: i64) { }

because then this stuff is put in rust-generated docs and it is shown in the IDEs of people.

To mitigate this problem I would propose a mapping of parameters to prefixed_parameters at the beginning of the function:

fn pretty_interface(configuration: &Config, user: String, pet_id: i64) { 
  let p_user = user;
  let p_pet_id = pet_id;
  
  // ...
}

Such a mapping is already used when useSingleRequestParameter is set. (see:

openapi-generator/modules/openapi-generator/src/main/resources/rust/reqwest/api.mustache

Lines 86 to 89 in e87c4ea

	// unbox the parameters
	{{#allParams}}
	let {{paramName}} = params.{{paramName}};
	{{/allParams}}

)
Note: reusing an existing variable internally is not a problem, but a rust feature, even mentioned in the official book as "Shadowing"
https://doc.rust-lang.org/book/ch03-01-variables-and-mutability.html#shadowing

Note: there is a single limitation to local mapping, if the user creates an OpenAPI parameter and names it configuration this cannot be mapped away. HOWEVER this is no new limitation, the limitation already exists in all today's variants. The status quo workaround here would be to use NameMapping or set useSingleRequestParameter. The workaround still would work with a parameter-prefix approach.

Code example

You read enough, lets talk code:

// The old code is linked below in the chapter "Name Collisions"
pub fn delete_pet(configuration: &configuration::Configuration, pet_id: i64, api_key: Option<&str>) -> Result<(), Error<DeletePetError>> {
    // map all parameters to something with a prefix here.
    let p_pet_id = pet_id;
    let p_api_key = api_key;
    
    // now any business logic below here, 100% avoids any naming confiics by using the prefixed name
    let uri_str = format!("{}/pet/{petId}", configuration.base_path, petId=p_pet_id);
    let mut req_builder = configuration.client.request(reqwest::Method::DELETE, uri_str.as_str());

    if let Some(ref user_agent) = configuration.user_agent {
        req_builder = req_builder.header(reqwest::header::USER_AGENT, user_agent.clone());
    }
    if let Some(value) = p_api_key {
        req_builder = req_builder.header("api_key", value.to_string());
    }
    if let Some(ref token) = configuration.oauth_access_token {
        req_builder = req_builder.bearer_auth(token.to_owned());
    };

    let req = req_builder.build()?;
    let resp = configuration.client.execute(req)?;

    let status = resp.status();

    if !status.is_client_error() && !status.is_server_error() {
        Ok(())
    } else {
        let content = resp.text()?;
        let entity: Option<DeletePetError> = serde_json::from_str(&content).ok();
        Err(Error::ResponseError(ResponseContent { status, content, entity }))
    }
}

We see: that the resulting rust code is easier to read. The mustache code too, because all those p_ names are injected via variables anyway. The parameters stay unaffected and the resulting code is backward compatible on an interface level with the old code.
There exists a mapping at the beginning of that function to put all parameters in a safe "namespace". Thus there can be no name conflicts afterwards. When using useSingleRequestParameter we can get rid of the mapping completely, as we could just use the value from its Grouped struct (e.g. See PoC: https://github.com/OpenAPITools/openapi-generator/pull/20203/files#diff-85ae3f26660bc69ea8d32481b66d705799993c1429e096f4be81c948bb1ab19dR59 )

Describe alternatives you've considered

@wing328 and I discussed some alternatives on Slack, e.g. configurable prefixes for local variables ( #20219 ) or no prefixes for local variables and no parameter unboxing in useSingleRequestParameter ( #20203 ). But they only shift the problem a bit rather than having a solid solution

Additional context

Name Collisions

A name collision happens when a parameter is inserted in the function where a local variable exists.
E.g. look at

openapi-generator/samples/client/petstore/rust/reqwest/petstore/src/apis/pet_api.rs

Lines 119 to 124 in e87c4ea

	pub fn delete_pet(configuration: &configuration::Configuration, pet_id: i64, api_key: Option<&str>) -> Result<(), Error<DeletePetError>> {
	let local_var_configuration = configuration;
	let local_var_client = &local_var_configuration.client;
	let local_var_uri_str = format!("{}/pet/{petId}", local_var_configuration.base_path, petId=pet_id);

Imagine the user creates an OpenApi parameter called configuration. In that case, the generated rust could would NOT COMPILE because of a duplicate parameter error.
Imagine the user creates an OpenApi parameter called local_var_client. As the Parameter is prob from a different type than reqwest::Client (it is probably a String/int/float/bool), this will NOT COMPILE because of a type error.
Imagine the user creates an OpenApi parameter called local_var_uri_str and makes it a String, the could would probably COMPILE, but we would have a logic error hard to detect.

So we can conclude that we want to avoid Name collisions.

To avoid name-collisions in the past there were 3 options in openapi-generator:

1. The local_var_ prefix already used right now, greatly reduces chance but is not perfect.
2. Name Mapping: https://openapi-generator.tech/docs/customization/#name-mapping The user can carefully map parameters on its own, which requires them to notice the error (which is not trivial) and manual work.
3. Grouping parameter names using the useSingleRequestParameter config (already works), and don't unpack them (works in theory, PoC [Rust] fix #20198, cleanup generated code #20203 )

[wing328]

thanks for the suggestion 👍

let's see if the community has feedback/question on this.

cc @frol (2017/07) @farcaller (2017/08) @richardwhiuk (2019/07) @paladinzh (2020/05) @jacob-pro (2022/10)

[farcaller]

One option would be to take in openapi params as a struct. I've seen this pattern used around and it also solves the problem of naming the keys on the user side of using the generated code. Rust is smart enough to optimize those structs away, too, so it's not a performance hit (if you would ever care about that performance in face of the price of a networking call anyway).

Do we expect users to look into generated code at all? because I could see some extreme corner case where having myparam and p_myparam would still break things, and that's were we would want to just generate a random 6 chars of a suffix that doesn't collide with any args and slap it onto all the variables. Then remapping would be bullet-proof.

[xMAC94x]

One option would be to take in openapi params as a struct. I've seen this pattern used around and it also solves the problem of naming the keys on the user side of using the generated code. Rust is smart enough to optimize those structs away, too, so it's not a performance hit (if you would ever care about that performance in face of the price of a networking call anyway).

That is enabled by the useSingleRequestParameter config, see:

openapi-generator/samples/client/petstore/rust/reqwest/petstore-async/src/apis/user_api.rs

Lines 198 to 202 in e87c4ea

	pub async fn create_user(configuration: &configuration::Configuration, params: CreateUserParams) -> Result<ResponseContent<CreateUserSuccess>, Error<CreateUserError>> {
	let local_var_configuration = configuration;
	// unbox the parameters
	let user = params.user;

Agree to the performance impact, but its sometimes more convenient to just pass a single parameter in, rather than create a dummy struct, so I see why the config exists, to give users a choice.

Do we expect users to look into generated code at all? because I could see some extreme corner case where having myparam and p_myparam would still break things, and that's were we would want to just generate a random 6 chars of a suffix that doesn't collide with any args and slap it onto all the variables. Then remapping would be bullet-proof.

Already thought about a random string, however the same input should always generate the same output, as people probably want to add generated code to git and don't want to have thousand line diffs each run.
I definitively want to have a look in the generated source code to investigate what is done which way.
Even if no one besides me wants to look in the generated code, it also cleans up the mustache files, and thus makes it easier for developers of openapi-generator to make changes there.
Do you have an example of such a extreme corner case?