-
Notifications
You must be signed in to change notification settings - Fork 129
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: use escaped strings for services' path #755
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a quick reading of EscapedPath()
source, it doesn't feel like the safest approach, as it checks if the raw url is "okish" to use, and if so, returns it directly.
my first approach would be to first distinguish two very different cases:
- "we're having too many issues with "special" characters" -> better to straight up encode.
- "everything has been working well with decoded path, except this very specific thing" -> just do
path.replace(' ', '%20')
Yeah I think we're falling mostly on the second case, which was also the initial way I was working it out. |
Codecov Report
@@ Coverage Diff @@
## main #755 +/- ##
==========================================
+ Coverage 39.98% 40.00% +0.01%
==========================================
Files 87 87
Lines 9715 9717 +2
==========================================
+ Hits 3885 3887 +2
Misses 5459 5459
Partials 371 371
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@javierguerragiraldez would you mind taking another look whenever you have time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. don't forget to update the commit message, since this no longer uses EscapedPath.
7df6eae
to
cfc491e
Compare
Right now, when a `url` containing an encoded whitespace (`%20`) is passed to a service, decK unwraps it and replace it with an actual whitespace. This is because decK uses `url.Parse` under the cover, which is the one making this translation. This commit makes decK URL unwrapping method to encode back decoded whitespaces.
cfc491e
to
7c1ba12
Compare
Right now, when a
url
containing an encoded whitespace (%20
)is passed to a service, decK unwraps it and replace it with an
actual whitespace. This is because decK uses
url.Parse
underthe cover, which is the one making this translation.
This commit makes decK URL unwrapping method to encode back
decoded whitespaces.