improvement: include response body on fetch_http error #7148

Paul-Bob · 2023-11-09T14:49:29Z

What was the end-user or developer problem that led to this PR?

We would like to show custom error messages on our private rubygems server (https://packager.dev) that serves private Avo gems.

Right now the error message looks like (using local-host for testing purposes):

Bundler::HTTPError: Could not download gem from http://localhost:4567/avo-hq/ due to underlying error <bad response Forbidden 403 (http://localhost:4567/avo-hq/gems/avo-menu-0.1.18.gem)>

It would be helpful to us to make this message more customized so we can inform our users about the reason that download failed.

@adrianthedev opened this issue and @duckinator pointed us in the right direction in this comment.

Here is where we can fully customize the message lib/bundler/rubygems_integration.rb L519 but I noticed that the error that is rescued on that line it's already pre-processed here (remote_fetcher.rb L237) and it passes only the response.message and the response.code.

What is your fix for the problem, implemented in this PR?

With this PR the error message also includes the response's body and looks like:

Bundler::HTTPError: Could not download gem from http://localhost:4567/avo-hq/ due to underlying error <bad response Forbidden 403 Invalid License tier for 'avo-menu'. (http://localhost:4567/avo-hq/gems/avo-menu-0.1.18.gem)>

Notice that the error now includes the response's body Invalid License tier for 'avo-menu'..

Not sure if is the best approach, open for any kind of improvement.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

PS:

Alternative approach

What if we add an attribute on FetchError that will store the original response and we would be able to manipulate it on lib/bundler/rubygems_integration.rb L519 ?

This way the error message can be fully customizable.

welcome · 2023-11-09T14:49:32Z

Thanks for opening a pull request and helping make RubyGems and Bundler 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 or #bundler channel on Slack.

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

…_http_error

adrianthedev · 2023-11-13T14:04:12Z

@duckinator does this look good?

duckinator · 2023-11-13T15:19:41Z

At a glance, this looks good. I'll test it locally when I'm at my computer later today (mainly to see what errors from rubygems.org look like with it), but don't expect any problems. 👍

adrianthedev · 2023-11-13T15:20:49Z

Thank you! We appreciate it!
Let us know if we need to update anything else.

duckinator · 2023-11-13T17:48:38Z

The test failures don't seem to be related to this PR; I asked the other maintainers if they know what's up. I'll get back to you when I have more info. 🙂

duckinator · 2023-11-14T00:51:41Z

Looks like that issue is resolved by #7156. Merging master back into this branch to get the latest changes. CI should in theory pass now.

…_http_error

duckinator · 2023-11-14T03:19:26Z

Thanks for the PR, @Paul-Bob!

simi · 2023-11-14T03:26:51Z

lib/rubygems/remote_fetcher.rb

@@ -234,7 +234,7 @@ def fetch_http(uri, last_modified = nil, head = false, depth = 0)

      fetch_http(location, last_modified, head, depth + 1)
    else
-      raise FetchError.new("bad response #{response.message} #{response.code}", uri)
+      raise FetchError.new("bad response #{response.message} #{response.code} #{response.body.strip}", uri)


This can be suuuuuuuuuuuuper messy IMHO when remote provides like 404 HTML page. Would it make sense to limit this to first X characters at least or scan for friendly response and report back only when recognized format is returned? What's returned by RubyGems.org in here?

Referencing a gem that doesn't exist doesn't result in this message being displayed.

I'm not opposed to adding a character limit, though.

puppy@orthrus:~/test$ ls -a ./ ../ Gemfile puppy@orthrus:~/test$ cat Gemfile source 'https://rubygems.org' gem 'asdfasdfsadfihgjklsadkjfhakljfhsaf-this-gem-does-not-exist' puppy@orthrus:~/test$ bundle install Fetching gem metadata from https://rubygems.org/. Could not find gem 'asdfasdfsadfihgjklsadkjfhakljfhsaf-this-gem-does-not-exist' in rubygems repository https://rubygems.org/ or installed locally. puppy@orthrus:~/test$

There are other gem servers which can return super long 500 HTML page for example. I would suggest to revert this and instead build some kind of "protocol" to let the error message propagate to client. IMHO it is not good idea to propagate licensing error back to clients using HTTP message on gem install. There is no guarantee how RubyGems handles HTTP errors and IMHO it should not be mis-used to handing over messages to client.

I would be happy to keep this output, but only in kind of "verbose" mode and that is most likely against the original need of @adrianthedev.

We have also sometimes needed this in Bundler, for example when rate-limiting certain requests. I like the idea of this being officially supported, but I agree that an unlimited message size is messy and unhelpful.

Oh, sorry, I misunderstood! I thought you were saying only responses not 4xx or 5xx. 🙃 Sounds good to me.

@adrianthedev would that work for you? ^ Are you in control of returned HTTP headers for 403 on your gemserver?

Yes @simi, we are in control of the returned HTTP headers. Our gem server is operated by a sinatra app.

Not completely sure how to send the message.
Is it something like this?

halt 403, {'Content-Type' => 'application/vnd.rubygems.text'}, { message: 'some text' }.to_json

Our message is something like this. That's 164 characters. We might play around with it so the number of chars might increase.

It seems that you don't have a valid GEM_SERVER_TOKEN for Avo (https://bundler.dev/). Please read this for more information https://docs.avohq.io/3.0/some-path-here

Did I understand correctly that we'd rather revert this PR and go with the cleaner solution that @simi proposed?

Yes, if possible, don't release this. 🙏

adrianthedev · 2023-11-15T15:12:06Z

Thanks for the help!
This be available in newer versions of bundler, right?

…sponse_body_on_fetch_http_error" This reverts commit e5a4fd9, reversing changes made to c5a5363.

improvement: include response body on fetch_http error

de4189a

Merge branch 'master' into improvement/include_response_body_on_fetch…

75d3d61

…_http_error

Merge branch 'master' into improvement/include_response_body_on_fetch…

0bcb46a

…_http_error

duckinator merged commit e5a4fd9 into rubygems:master Nov 14, 2023
60 checks passed

duckinator mentioned this pull request Nov 14, 2023

Ability to provide custom error message from private gem server #7105

Closed

simi reviewed Nov 14, 2023

View reviewed changes

deivid-rodriguez added a commit that referenced this pull request Dec 7, 2023

Revert "Merge pull request #7148 from Paul-Bob/improvement/include_re…

e21c45c

…sponse_body_on_fetch_http_error" This reverts commit e5a4fd9, reversing changes made to c5a5363.

deivid-rodriguez mentioned this pull request Dec 7, 2023

Revert PR to include response body with fetch errors #7232

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: include response body on fetch_http error #7148

improvement: include response body on fetch_http error #7148

Paul-Bob commented Nov 9, 2023 •

edited

welcome bot commented Nov 9, 2023

adrianthedev commented Nov 13, 2023

duckinator commented Nov 13, 2023

adrianthedev commented Nov 13, 2023

duckinator commented Nov 13, 2023

duckinator commented Nov 14, 2023 •

edited

duckinator commented Nov 14, 2023

simi Nov 14, 2023

duckinator Nov 15, 2023

simi Nov 15, 2023

simi Nov 15, 2023

indirect Nov 15, 2023

indirect Nov 15, 2023

simi Nov 15, 2023

adrianthedev Nov 16, 2023

deivid-rodriguez Dec 7, 2023

simi Dec 7, 2023

adrianthedev commented Nov 15, 2023

improvement: include response body on fetch_http error #7148

improvement: include response body on fetch_http error #7148

Conversation

Paul-Bob commented Nov 9, 2023 • edited

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

PS:

Alternative approach

welcome bot commented Nov 9, 2023

adrianthedev commented Nov 13, 2023

duckinator commented Nov 13, 2023

adrianthedev commented Nov 13, 2023

duckinator commented Nov 13, 2023

duckinator commented Nov 14, 2023 • edited

duckinator commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianthedev commented Nov 15, 2023

Paul-Bob commented Nov 9, 2023 •

edited

duckinator commented Nov 14, 2023 •

edited