Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix custom value with / #1229

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Conversation

labuladong
Copy link
Contributor

@labuladong labuladong commented Jun 26, 2024

Even #1228 fix the double escape error, there still some issue about custom dynamic config.

When the custom config value has /, the golang net/http package will unescape it by default. see golang/go#7356

For example, if the config value is a json like {"key": "https://example.com/"}, the expected request should be:

/admin/v2/brokers/configuration/someConfig/%7B%22key%22%3A%20%22https%3A%2F%2Fexample.com%2F%22%7D

But the actual request will be this:

/admin/v2/brokers/configuration/someConfig/{"key": "https://example.com/"}

And it will causes 404 not found because / in the URL.

If you escape the value twice, it will be:

/admin/v2/brokers/configuration/someConfig/%257B%2522key%2522%253A%2520%2522https%253A%252F%252Fexample.com%252F%2522%257D

The solution is url.RawPath field in this golang/go@874a605 , we need to modify this part to add the RawPath field:

func (c *Client) newRequest(method, path string) (*request, error) {
base, err := url.Parse(c.ServiceURL)
if err != nil {
return nil, err
}
u, err := url.Parse(path)
if err != nil {
return nil, err
}
req := &request{
method: method,
url: &url.URL{
Scheme: base.Scheme,
User: base.User,
Host: base.Host,
Path: endpoint(base.Path, u.Path),
},
params: make(url.Values),
}
return req, nil
}

But the newRequest is used by many other methods, I don't want to touch it. So an easy solution is to provide a MakeRequestWithURL method, users can use it to customize.

@labuladong labuladong requested a review from tuteng June 26, 2024 06:23
@tuteng tuteng merged commit 552b541 into apache:master Jun 26, 2024
8 checks passed
@tuteng
Copy link
Member

tuteng commented Jun 26, 2024

Sorry, I thought it was approved after the approve run test

image

@tuteng
Copy link
Member

tuteng commented Jun 26, 2024

This pr LGTM

@RobertIndie RobertIndie added this to the v0.13.0 milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants