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

Handle unexpected behavior with URI#merge and subpaths missing trailing slashes #3123

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

drcapulet
Copy link
Contributor

@drcapulet drcapulet commented Feb 4, 2020

Due to how URI#merge works (which URI#+ is aliased to), when using a source under a subpath, it can get overwritten:

> URI.parse('https://gems.example.com/private') + './api/v1/dependencies'
=> #<URI::HTTPS https://gems.example.com/api/v1/dependencies>
> URI.parse('https://gems.example.com/private/') + './api/v1/dependencies'
=> #<URI::HTTPS https://gems.example.com/private/api/v1/dependencies>

It's not immediately obvious that errors fetching would be caused by a missing slash, especially if you have a server mounted at both / and /private.

This PR makes sure that calls to URI#+ are more obvious and subpaths aren't overwritten and instead appended.

Fixes #1829.

@welcome
Copy link

welcome bot commented Feb 4, 2020

Thanks for opening a pull request and helping make RubyGems better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Github Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Github Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

@bronzdoc
Copy link
Member

bronzdoc commented Feb 4, 2020

Hey @drcapulet! could you add the test into test/rubygems/test_gem_source.rb? thank you!

@drcapulet
Copy link
Contributor Author

@bronzdoc Was there something you'd want me to cover in test/rubygems/test_gem_source.rb that isn't in test/rubygems/test_gem_source_subpath_problem.rb? Or do you mean try merging them together? If so, I wasn't sure on a good way to achieve what I wanted with #setup.

@bronzdoc
Copy link
Member

bronzdoc commented Feb 5, 2020

@drcapulet sorry for the confusion. I meant "move" not "add"

@hsbt
Copy link
Member

hsbt commented Aug 6, 2020

@drcapulet Hi, Can you move the your test into the test_gem_source.rb?

After that, I will merge this.

@drcapulet
Copy link
Contributor Author

@hsbt As I mentioned earlier, I wasn't sure of a good way to set @gem_repo - do you just want me to copy the setup across the three tests?

@deivid-rodriguez
Copy link
Member

@hsbt Should @drcapulet copy the test setup across each of the tests or do you have another preferred way? In my opinion, a separate test file like it is coded currently is fine.

By the way, does this fix or is related to #1829?

@drcapulet
Copy link
Contributor Author

@deivid-rodriguez Yes, this should fix #1829.

@deivid-rodriguez
Copy link
Member

Great! Could you rebase this PR to make sure everything is still good, and so that the PR is ready to merge in case @hsbt is fine with it now?

@drcapulet drcapulet force-pushed the alexc-source-slash branch 2 times, most recently from 625c8cc to 9ec4728 Compare September 17, 2020 20:07
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased this PR and added my suggestions. I'll merge in a couple of days unless there's further feedback.

@deivid-rodriguez
Copy link
Member

Thanks @drcapulet!

@deivid-rodriguez deivid-rodriguez merged commit 6d7fe84 into rubygems:master Oct 30, 2020
@drcapulet drcapulet deleted the alexc-source-slash branch October 30, 2020 18:24
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Handle unexpected behavior with URI#merge and subpaths missing trailing slashes

(cherry picked from commit 6d7fe84)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Handle unexpected behavior with URI#merge and subpaths missing trailing slashes

(cherry picked from commit 6d7fe84)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sources without trailing slash not resolved correctly
5 participants