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: correct url path params processing order #519 #705

Merged
merged 1 commit into from Sep 24, 2023
Merged

Conversation

jeevatkm
Copy link
Member

Closes #519

@jeevatkm jeevatkm added the bug label Sep 24, 2023
@jeevatkm jeevatkm added this to the v2.9.0 milestone Sep 24, 2023
@jeevatkm jeevatkm self-assigned this Sep 24, 2023
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4801bed) 95.65% compared to head (28012cc) 95.65%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #705   +/-   ##
=======================================
  Coverage   95.65%   95.65%           
=======================================
  Files          12       12           
  Lines        1611     1611           
=======================================
  Hits         1541     1541           
  Misses         42       42           
  Partials       28       28           
Files Changed Coverage Δ
middleware.go 92.22% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jeevatkm jeevatkm merged commit 0424ad1 into master Sep 24, 2023
4 checks passed
@jeevatkm jeevatkm deleted the gh-519 branch September 24, 2023 05:12
@SVilgelm
Copy link
Contributor

@jeevatkm, the previous implementation was correct.
Let's say we have url example.com/{foo}/{bar}/
And have the parameters:

Client: {"foo": "123"}
Request: {"foo": "456", "bar": "789"}

r.URL = "example.com/{foo}/{bar}/"

the current code will apply the parameters from Client first:

  • "foo":
r.URL = r.URL = strings.Replace(r.URL, "{foo}", url.PathEscape("123"), -1)`
---
r.URL = "example.com/123/{bar}/"

then the request parameters:

  • "foo":
r.URL = r.URL = strings.Replace(r.URL, "{foo}", url.PathEscape("456"), -1)`
---
r.URL = "example.com/123/{bar}/"

no changes because there is no {foo} placeholder in the r.URL

  • "bar":
r.URL = r.URL = strings.Replace(r.URL, "{bar}", url.PathEscape("789"), -1)`
---
r.URL = "example.com/123/789/"

so the request parameters must be applied first, then the client, or we need to change the logic to use a map and strings.Replacer

@jeevatkm
Copy link
Member Author

@SVilgelm Thanks for reminding me. Damn it, last night, how did I forget this!! 🤦 I will revert the changes.

jeevatkm added a commit that referenced this pull request Sep 24, 2023
@SVilgelm
Copy link
Contributor

We need to add the unit tests to prevent such thing in the future, I can do it later today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

the applying order of PathParams
2 participants