From c6303ffc9afd189dc32f57b52fa171e68cf668e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Fri, 13 Nov 2020 11:39:10 +0100 Subject: [PATCH 1/3] Remove unnecessary rescued exception classes `OpenSSL::SSL::SSLError` and `Net::HTTPFatalError` should never be raised here. --- bundler/lib/bundler/fetcher/index.rb | 2 +- bundler/spec/bundler/fetcher/index_spec.rb | 117 +++++++++------------ 2 files changed, 50 insertions(+), 69 deletions(-) diff --git a/bundler/lib/bundler/fetcher/index.rb b/bundler/lib/bundler/fetcher/index.rb index 7e07eaea7b95..363d3c0ac652 100644 --- a/bundler/lib/bundler/fetcher/index.rb +++ b/bundler/lib/bundler/fetcher/index.rb @@ -8,7 +8,7 @@ class Fetcher class Index < Base def specs(_gem_names) Bundler.rubygems.fetch_all_remote_specs(remote) - rescue Gem::RemoteFetcher::FetchError, OpenSSL::SSL::SSLError, Net::HTTPFatalError => e + rescue Gem::RemoteFetcher::FetchError => e case e.message when /certificate verify failed/ raise CertificateFailureError.new(display_uri) diff --git a/bundler/spec/bundler/fetcher/index_spec.rb b/bundler/spec/bundler/fetcher/index_spec.rb index 5ecd7d9e059e..2d8b176c1f75 100644 --- a/bundler/spec/bundler/fetcher/index_spec.rb +++ b/bundler/spec/bundler/fetcher/index_spec.rb @@ -17,100 +17,81 @@ end context "error handling" do - shared_examples_for "the error is properly handled" do - let(:remote_uri) { Bundler::URI("http://remote-uri.org") } - before do - allow(subject).to receive(:remote_uri).and_return(remote_uri) - end + let(:remote_uri) { Bundler::URI("http://remote-uri.org") } + before do + allow(rubygems).to receive(:fetch_all_remote_specs) { raise Gem::RemoteFetcher::FetchError.new(error_message, nil) } + allow(subject).to receive(:remote_uri).and_return(remote_uri) + end - context "when certificate verify failed" do - let(:error_message) { "certificate verify failed" } + context "when certificate verify failed" do + let(:error_message) { "certificate verify failed" } - it "should raise a Bundler::Fetcher::CertificateFailureError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::CertificateFailureError, - %r{Could not verify the SSL certificate for http://sample_uri.com}) - end + it "should raise a Bundler::Fetcher::CertificateFailureError" do + expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::CertificateFailureError, + %r{Could not verify the SSL certificate for http://sample_uri.com}) end + end - context "when a 401 response occurs" do - let(:error_message) { "401" } - - before do - allow(remote_uri).to receive(:userinfo).and_return(userinfo) - end - - context "and there was userinfo" do - let(:userinfo) { double(:userinfo) } + context "when a 401 response occurs" do + let(:error_message) { "401" } - it "should raise a Bundler::Fetcher::BadAuthenticationError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::BadAuthenticationError, - %r{Bad username or password for http://remote-uri.org}) - end - end + before do + allow(remote_uri).to receive(:userinfo).and_return(userinfo) + end - context "and there was no userinfo" do - let(:userinfo) { nil } + context "and there was userinfo" do + let(:userinfo) { double(:userinfo) } - it "should raise a Bundler::Fetcher::AuthenticationRequiredError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::AuthenticationRequiredError, - %r{Authentication is required for http://remote-uri.org}) - end + it "should raise a Bundler::Fetcher::BadAuthenticationError" do + expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::BadAuthenticationError, + %r{Bad username or password for http://remote-uri.org}) end end - context "when a 403 response occurs" do - let(:error_message) { "403" } + context "and there was no userinfo" do + let(:userinfo) { nil } - before do - allow(remote_uri).to receive(:userinfo).and_return(userinfo) + it "should raise a Bundler::Fetcher::AuthenticationRequiredError" do + expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::AuthenticationRequiredError, + %r{Authentication is required for http://remote-uri.org}) end + end + end - context "and there was userinfo" do - let(:userinfo) { double(:userinfo) } + context "when a 403 response occurs" do + let(:error_message) { "403" } - it "should raise a Bundler::Fetcher::BadAuthenticationError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::BadAuthenticationError, - %r{Bad username or password for http://remote-uri.org}) - end - end + before do + allow(remote_uri).to receive(:userinfo).and_return(userinfo) + end - context "and there was no userinfo" do - let(:userinfo) { nil } + context "and there was userinfo" do + let(:userinfo) { double(:userinfo) } - it "should raise a Bundler::Fetcher::AuthenticationRequiredError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::AuthenticationRequiredError, - %r{Authentication is required for http://remote-uri.org}) - end + it "should raise a Bundler::Fetcher::BadAuthenticationError" do + expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::BadAuthenticationError, + %r{Bad username or password for http://remote-uri.org}) end end - context "any other message is returned" do - let(:error_message) { "You get an error, you get an error!" } - - before { allow(Bundler).to receive(:ui).and_return(double(:trace => nil)) } + context "and there was no userinfo" do + let(:userinfo) { nil } - it "should raise a Bundler::HTTPError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::HTTPError, "Could not fetch specs from http://sample_uri.com") + it "should raise a Bundler::Fetcher::AuthenticationRequiredError" do + expect { subject.specs(gem_names) }.to raise_error(Bundler::Fetcher::AuthenticationRequiredError, + %r{Authentication is required for http://remote-uri.org}) end end end - context "when a Gem::RemoteFetcher::FetchError occurs" do - before { allow(rubygems).to receive(:fetch_all_remote_specs) { raise Gem::RemoteFetcher::FetchError.new(error_message, nil) } } + context "any other message is returned" do + let(:error_message) { "You get an error, you get an error!" } - it_behaves_like "the error is properly handled" - end + before { allow(Bundler).to receive(:ui).and_return(double(:trace => nil)) } - context "when a OpenSSL::SSL::SSLError occurs" do - before { allow(rubygems).to receive(:fetch_all_remote_specs) { raise OpenSSL::SSL::SSLError.new(error_message) } } - - it_behaves_like "the error is properly handled" - end - - context "when a Net::HTTPFatalError occurs" do - before { allow(rubygems).to receive(:fetch_all_remote_specs) { raise Net::HTTPFatalError.new(error_message, 404) } } - - it_behaves_like "the error is properly handled" + it "should raise a Bundler::HTTPError" do + expect { subject.specs(gem_names) }.to raise_error(Bundler::HTTPError, "Could not fetch specs from http://sample_uri.com") + end end end end From 22a6cbf65a3e250de78b8d9280d4802383d8b393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Fri, 13 Nov 2020 12:00:26 +0100 Subject: [PATCH 2/3] Gem::Fetcher::FetchError should receive an uri The one that failed to be fetched. --- bundler/spec/bundler/fetcher/index_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundler/spec/bundler/fetcher/index_spec.rb b/bundler/spec/bundler/fetcher/index_spec.rb index 2d8b176c1f75..dd695b53b7fa 100644 --- a/bundler/spec/bundler/fetcher/index_spec.rb +++ b/bundler/spec/bundler/fetcher/index_spec.rb @@ -19,7 +19,7 @@ context "error handling" do let(:remote_uri) { Bundler::URI("http://remote-uri.org") } before do - allow(rubygems).to receive(:fetch_all_remote_specs) { raise Gem::RemoteFetcher::FetchError.new(error_message, nil) } + allow(rubygems).to receive(:fetch_all_remote_specs) { raise Gem::RemoteFetcher::FetchError.new(error_message, display_uri) } allow(subject).to receive(:remote_uri).and_return(remote_uri) end From 954c54365cae670a761e7a5f10e6830d4ae689d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Fri, 13 Nov 2020 12:02:15 +0100 Subject: [PATCH 3/3] Always show underlying error when fetching specs fails --- bundler/lib/bundler/fetcher/index.rb | 3 +-- bundler/spec/bundler/fetcher/index_spec.rb | 2 +- bundler/spec/realworld/mirror_probe_spec.rb | 28 ++++++++++----------- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/bundler/lib/bundler/fetcher/index.rb b/bundler/lib/bundler/fetcher/index.rb index 363d3c0ac652..08b041897eb3 100644 --- a/bundler/lib/bundler/fetcher/index.rb +++ b/bundler/lib/bundler/fetcher/index.rb @@ -19,8 +19,7 @@ def specs(_gem_names) raise BadAuthenticationError, remote_uri if remote_uri.userinfo raise AuthenticationRequiredError, remote_uri else - Bundler.ui.trace e - raise HTTPError, "Could not fetch specs from #{display_uri}" + raise HTTPError, "Could not fetch specs from #{display_uri} due to underlying error <#{e.message}>" end end diff --git a/bundler/spec/bundler/fetcher/index_spec.rb b/bundler/spec/bundler/fetcher/index_spec.rb index dd695b53b7fa..b8ce46321e1b 100644 --- a/bundler/spec/bundler/fetcher/index_spec.rb +++ b/bundler/spec/bundler/fetcher/index_spec.rb @@ -90,7 +90,7 @@ before { allow(Bundler).to receive(:ui).and_return(double(:trace => nil)) } it "should raise a Bundler::HTTPError" do - expect { subject.specs(gem_names) }.to raise_error(Bundler::HTTPError, "Could not fetch specs from http://sample_uri.com") + expect { subject.specs(gem_names) }.to raise_error(Bundler::HTTPError, "Could not fetch specs from http://sample_uri.com due to underlying error ") end end end diff --git a/bundler/spec/realworld/mirror_probe_spec.rb b/bundler/spec/realworld/mirror_probe_spec.rb index 626092c7ebee..a2b5c89150db 100644 --- a/bundler/spec/realworld/mirror_probe_spec.rb +++ b/bundler/spec/realworld/mirror_probe_spec.rb @@ -74,10 +74,10 @@ bundle :install, :artifice => nil, :raise_on_error => false expect(out).to include("Fetching source index from #{mirror}") - expect(err).to include("Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from #{mirror}") - expect(err).to include("Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from #{mirror}") - expect(err).to include("Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from #{mirror}") - expect(err).to include("Could not fetch specs from #{mirror}") + expect(err).to include("Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error ") + expect(err).to include("Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error ") + expect(err).to include("Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error ") + expect(err).to include("Could not fetch specs from #{mirror}/ due to underlying error ") end it "prints each error and warning on a new line" do @@ -90,10 +90,10 @@ expect(out).to include "Fetching source index from #{mirror}/" expect(err).to include <<-EOS.strip -Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ -Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ -Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ -Could not fetch specs from #{mirror}/ +Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error +Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error +Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error +Could not fetch specs from #{mirror}/ due to underlying error EOS end end @@ -112,10 +112,10 @@ bundle :install, :artifice => nil, :raise_on_error => false expect(out).to include("Fetching source index from #{mirror}") - expect(err).to include("Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from #{mirror}") - expect(err).to include("Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from #{mirror}") - expect(err).to include("Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from #{mirror}") - expect(err).to include("Could not fetch specs from #{mirror}") + expect(err).to include("Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error ") + expect(err).to include("Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error ") + expect(err).to include("Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from #{mirror}/ due to underlying error ") + expect(err).to include("Could not fetch specs from #{mirror}/ due to underlying error ") end end @@ -138,7 +138,7 @@ def setup_server end def setup_mirror - mirror_port = find_unused_port - @mirror_uri = "http://#{host}:#{mirror_port}" + @mirror_port = find_unused_port + @mirror_uri = "http://#{host}:#{@mirror_port}" end end