[PROPOSAL] Specialize Type used for Query String

[samuelorji]

What/Why

What are you proposing?

Specializing the type of query "string" used in http queries by the client to make raw json requests

What users have asked for this feature?

None that I know of

What problems are you trying to solve?

Currently, the type of the query string is set to just basically any type that implements Serializable . While this doesn't look like a problem at initial glance, looking into the reqwest library that handles http calls, we can see that even if the type is Serializable it uses the serde url-encoded crate that will only serialize types that can be serialized into query strings. The request will fail with this error if the Serialize type cannot be serialized into a query string.

What sucks about this is that this problem cannot be caught at compile time, but at runtime. As an example, this is a valid struct that can be passed as a query "String" since it implements Serialize

 #[derive(Serialize)]
    struct QueryString {
        id : String,
        memebers: HashMap<String, u8>,
        some_other_field : Vec<u8>
    }

    let query = QueryString {
        id : "id".to_string(),
        memebers: HashMap::from([("hi".to_string(), 1)]),
        some_other_field: vec![1,2,3,4,5]
    };

    let info: Value = client
        .send::<(), QueryString>(
            Method::Get,
            "/",
            HeaderMap::new(),
            Some(&query),
            None,
            None,
        )
        .await?
        .json()
        .await?;

This will result in this error:

Error: Error { kind: Http(reqwest::Error { kind: Builder, source: Custom("unsupported value") }) }

I'm proposing a solution where we specialize this query type to be a sequence of simple str key-value pairs for the client facing http method. I know it boxes users, but its safer to catch these errors at Compile time rather than at runtime

pub async fn send<B>(
        &self,
        method: Method,
        path: &str,
        headers: HeaderMap,
        query_string: Option<&[(&str ,&str)]>,
        body: Option<B>,
        timeout: Option<Duration>,
    ) -> Result<Response, Error>
    where
        B: Body
    {
        self.transport
            .send(method, path, headers, query_string, body, timeout)
            .await
    }

What is the developer experience going to be?

Better Compile time error checking

Are there any security considerations?

N/A

Are there any breaking changes to the API

Maybe some ?

What is the user experience going to be?

Users just have to supply query string as a sequence of str key-value pairs

Are there breaking changes to the User Experience?

Maybe ?

Why should it be built? Any reason not to?

To prevent clients from possible runtime errors

What will it take to execute?

A quite simple code change and some changes to the documentation

Any remaining open questions?

N/A

[samuelorji]

@Xtansia WDYT ?

[Xtansia]

@samuelorji This is something that we actually use in the client, for all of the API operations, for example: https://github.com/opensearch-project/opensearch-rs/blob/main/opensearch/src/cat.rs#L284-L320

So this isn't really something that's feasible to restrict the type of, nor is it desirable to restrict, as doing so forces a lot of overhead onto the caller to pre-convert everything to strings (you might have an array of ints, now you have to pre-emptively format them into a comma separated string), it also means you can't pass in a pre-formatted query string.

[samuelorji]

query string

Makes sense, I was thinking of only constraining it at the HTTP api layer here, while leaving the client / transport layer intact as they already handle query strings for the client. The restriction was for the raw and publicj http api that pretty much allowed any Serializable to be passed in.

I see how it can be a little too restrictive, but yeah. really wish we could constrain the type in a non restrictive manner.

Thanks, will close 👍