Handle local paths with trailing slashes #5402
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bk2204
changed the title
chrisd8088
approved these changes
Jun 21, 2023
Member
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
Looks good! I had a suggestion about the tests only. Thanks!
We'd like to use some of this test code in another test, so let's move it out into a function we can call. Right now, this results in no changes to the code, but we'll make some in a future commit.
When we want to handle a local path, we must rewrite it internally into a `file:///` URL, because we dispatch our standalone transfer agent based on the URL scheme. However, once we've looked up an appropriate URL and formatted it as the scheme prefers, if it contains a trailing slash, we ignore the entire lookup and replace the URL with the raw one given, but without the trailing slash. This behaviour seems a bit bizarre, and it causes us to take a local path with a trailing slash and treat it not as the `file:///` URL it needs to be, but as a raw local path, which gets refused. Let's avoid this problem by looking at the rewritten URL, and modifying that instead if we find its trailing slash to be offensive.
bk2204
force-pushed
the
local-path-trailing-slash
branch
from
d89fa0f to
600132c
Compare
ebardsley
reviewed
Jun 21, 2023
|
|
||
| if strings.HasSuffix(rawurl, "/") { | ||
| ep.Url = rawurl[0 : len(rawurl)-1] | ||
| if strings.HasSuffix(ep.Url, "/") { |
There was a problem hiding this comment.
Thanks for the quick fix! strings.TrimSuffix would be appropriate here too.
Member
Author
There was a problem hiding this comment.
That is a good idea, but I'm going to leave this as it stands for now in the interests of making the minimum possible fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we want to handle a local path, we must rewrite it internally into a
file:///URL, because we dispatch our standalone transfer agent based on the URL scheme. However, once we've looked up an appropriate URL and formatted it as the scheme prefers, if it contains a trailing slash, we ignore the entire lookup and replace the URL with the raw one given, but without the trailing slash.This behaviour seems a bit bizarre, and it causes us to take a local path with a trailing slash and treat it not as the
file:///URL it needs to be, but as a raw local path, which gets refused. Let's avoid this problem by looking at the rewritten URL, and modifying that instead if we find its trailing slash to be offensive.Fixes #5400
/cc @ebardsley as reporter