URL encoding of square brackets is not consistent

karatakis commented 1 month ago

Prerequisites

[X] I have checked the existing issues to ensure this is not a duplicate.
[X] I am running the latest version of Tailcall.

Describe the bug

When URL encoding the following link

/endpoint?bank_account_ids[]=id1&bank_account_ids[]=id2

we receive

/endpoint?bank_account_ids[]=id1&bank_account_ids%5B%5D=id2

Notice the [] for first id it is [] but for the next it is %5B%5D

Steps to reproduce

You can use code provided from this pull request https://github.com/tailcallhq/tailcall/pull/2495
Make the following change https://github.com/tailcallhq/tailcall/pull/2495#discussion_r1687588501

Expected behavior

Either all [] are encoded to %5B%5D or all %5B%5D to []

Actual behavior

Notice the [] for first id it is [] but for the next it is %5B%5D

Environment information:

Operating System: Linux Mint 21.2
Tailcall Version: tailcall 0.1.0-dev
Rust Version: rustc 1.79.0
GraphQL Client/version tailcall webui

ssddOnTop commented 1 month ago

should should be the consistency? Should it be URL safe all over or should it use []?

karatakis commented 1 month ago

I used https://www.urlencoder.org/ to determine the correct URL, and it returned %2Fendpoint%3Fbank_account_ids%5B%5D%3Did1%26bank_account_ids%5B%5D%3Did2

Maybe it is a different issue (the code editor or terminal decodes the URL safely)

karatakis commented 1 month ago

It is a bug in the depending crate.

Code that replicates the issue.

let mut url = Url::parse("https://example.com/?id[]=1&id[]=2").unwrap();
let pairs = vec![
    ("id[]", "3"),
    ("id[]", "4"),
];
url.query_pairs_mut().extend_pairs(
  pairs.iter().map(|&(k, v)| { (&k[..], &v[..]) })
);|
// Not working as intended
assert_eq!("https://example.com/?id[]=1&id[]=2&id[]=3&id[]=4", url.as_str());

karatakis commented 1 month ago

Apart from the test mocking, there are no other problems. With real servers, we have no issue because they decode the URL. (Tested with ExpressJS and tokio Hyper)

karatakis commented 1 month ago

Does not have any real-world impact apart from appearance. Closing until further notice that this bug impacts someone.

tailcallhq / tailcall