[rubygems/rubygems] Refactor Fetcher#api_fetcher? and fetcher loading logic

https://github.com/rubygems/rubygems/commit/f664d60114

lib/bundler/fetcher.rb

@@ -9,6 +9,7 @@ require "rubygems/request"
 module Bundler
   # Handles all the fetching with the rubygems server
   class Fetcher
+    autoload :Base, File.expand_path("fetcher/base", __dir__)
     autoload :CompactIndex, File.expand_path("fetcher/compact_index", __dir__)
     autoload :Downloader, File.expand_path("fetcher/downloader", __dir__)
     autoload :Dependency, File.expand_path("fetcher/dependency", __dir__)
@@ -134,18 +135,7 @@ module Bundler
     def specs(gem_names, source)
       index = Bundler::Index.new
 
-      if Bundler::Fetcher.disable_endpoint
+      fetch_specs(gem_names).each do |name, version, platform, dependencies, metadata|
-        @use_api = false
-        specs = fetchers.last.specs(gem_names)
-      else
-        specs = []
-        @fetchers = fetchers.drop_while do |f|
-          !f.available? || (f.api_fetcher? && !gem_names) || !specs = f.specs(gem_names)
-        end
-        @use_api = false if fetchers.none?(&:api_fetcher?)
-      end
-
-      specs.each do |name, version, platform, dependencies, metadata|
         spec = if dependencies
           EndpointSpecification.new(name, version, platform, self, dependencies, metadata)
         else
@@ -158,22 +148,10 @@ module Bundler
 
       index
     rescue CertificateFailureError
-      Bundler.ui.info "" if gem_names && use_api # newline after dots
+      Bundler.ui.info "" if gem_names && api_fetcher? # newline after dots
       raise
     end
 
-    def use_api
-      return @use_api if defined?(@use_api)
-
-      fetchers.shift until fetchers.first.available?
-
-      @use_api = if remote_uri.scheme == "file" || Bundler::Fetcher.disable_endpoint
-        false
-      else
-        fetchers.first.api_fetcher?
-      end
-    end
-
     def user_agent
       @user_agent ||= begin
         ruby = Bundler::RubyVersion.system
@@ -209,10 +187,6 @@ module Bundler
       end
     end
 
-    def fetchers
-      @fetchers ||= FETCHERS.map {|f| f.new(downloader, @remote, uri) }
-    end
-
     def http_proxy
       return unless uri = connection.proxy_uri
       uri.to_s
@@ -222,9 +196,36 @@ module Bundler
       "#<#{self.class}:0x#{object_id} uri=#{uri}>"
     end
 
+    def api_fetcher?
+      fetchers.first.api_fetcher?
+    end
+
     private
 
-    FETCHERS = [CompactIndex, Dependency, Index].freeze
+    def available_fetchers
+      if Bundler::Fetcher.disable_endpoint
+        [Index]
+      elsif remote_uri.scheme == "file"
+        Bundler.ui.debug("Using a local server, bundler won't use the CompactIndex API")
+        [Index]
+      else
+        [CompactIndex, Dependency, Index]
+      end
+    end
+
+    def fetchers
+      @fetchers ||= available_fetchers.map {|f| f.new(downloader, @remote, uri) }.drop_while {|f| !f.available? }
+    end
+
+    def fetch_specs(gem_names)
+      fetchers.reject!(&:api_fetcher?) unless gem_names
+      fetchers.reject! do |f|
+        specs = f.specs(gem_names)
+        return specs if specs
+        true
+      end
+      []
+    end
 
     def cis
       env_cis = {

lib/bundler/fetcher/compact_index.rb

@@ -60,10 +60,6 @@ module Bundler
           Bundler.ui.debug("FIPS mode is enabled, bundler can't use the CompactIndex API")
           return nil
         end
-        if fetch_uri.scheme == "file"
-          Bundler.ui.debug("Using a local server, bundler won't use the CompactIndex API")
-          return false
-        end
         # Read info file checksums out of /versions, so we can know if gems are up to date
         compact_index_client.update_and_parse_checksums!
       rescue CompactIndexClient::Updater::MisMatchedChecksumError => e

lib/bundler/resolver.rb

@@ -37,9 +37,9 @@ module Bundler
       root_version = Resolver::Candidate.new(0)
 
       @all_specs = Hash.new do |specs, name|
-        specs[name] = source_for(name).specs.search(name).reject do |s|
+        matches = source_for(name).specs.search(name)
-          s.dependencies.any? {|d| d.name == name && !d.requirement.satisfied_by?(s.version) } # ignore versions that depend on themselves incorrectly
+        matches = filter_invalid_self_dependencies(matches, name)
-        end.sort_by {|s| [s.version, s.platform.to_s] }
+        specs[name] = matches.sort_by {|s| [s.version, s.platform.to_s] }
       end
 
       @sorted_versions = Hash.new do |candidates, package|
@@ -318,6 +318,13 @@ module Bundler
       specs.reject {|s| s.version.prerelease? }
     end
 
+    # Ignore versions that depend on themselves incorrectly
+    def filter_invalid_self_dependencies(specs, name)
+      specs.reject do |s|
+        s.dependencies.any? {|d| d.name == name && !d.requirement.satisfied_by?(s.version) }
+      end
+    end
+
     def requirement_satisfied_by?(requirement, spec)
       requirement.satisfied_by?(spec.version) || spec.source.is_a?(Source::Gemspec)
     end

lib/bundler/source/rubygems.rb

@@ -392,7 +392,7 @@ module Bundler
       end
 
       def api_fetchers
-        fetchers.select {|f| f.use_api && f.fetchers.first.api_fetcher? }
+        fetchers.select(&:api_fetcher?)
       end
 
       def remote_specs

spec/bundler/bundler/fetcher_spec.rb

@@ -189,4 +189,70 @@ RSpec.describe Bundler::Fetcher do
       end
     end
   end
+
+  describe "#specs_with_retry" do
+    let(:downloader)  { double(:downloader) }
+    let(:remote)      { double(:remote, :cache_slug => "slug", :uri => uri, :original_uri => nil, :anonymized_uri => uri) }
+    let(:compact_index) { double(Bundler::Fetcher::CompactIndex, :available? => true, :api_fetcher? => true) }
+    let(:dependency)    { double(Bundler::Fetcher::Dependency, :available? => true, :api_fetcher? => true) }
+    let(:index)         { double(Bundler::Fetcher::Index, :available? => true, :api_fetcher? => false) }
+
+    before do
+      allow(Bundler::Fetcher::CompactIndex).to receive(:new).and_return(compact_index)
+      allow(Bundler::Fetcher::Dependency).to receive(:new).and_return(dependency)
+      allow(Bundler::Fetcher::Index).to receive(:new).and_return(index)
+    end
+
+    it "picks the first fetcher that works" do
+      expect(compact_index).to receive(:specs).with("name").and_return([["name", "1.2.3", "ruby"]])
+      expect(dependency).not_to receive(:specs)
+      expect(index).not_to receive(:specs)
+      fetcher.specs_with_retry("name", double(Bundler::Source::Rubygems))
+    end
+
+    context "when APIs are not available" do
+      before do
+        allow(compact_index).to receive(:available?).and_return(false)
+        allow(dependency).to receive(:available?).and_return(false)
+      end
+
+      it "uses the index" do
+        expect(compact_index).not_to receive(:specs)
+        expect(dependency).not_to receive(:specs)
+        expect(index).to receive(:specs).with("name").and_return([["name", "1.2.3", "ruby"]])
+
+        fetcher.specs_with_retry("name", double(Bundler::Source::Rubygems))
+      end
+    end
+  end
+
+  describe "#api_fetcher?" do
+    let(:downloader)  { double(:downloader) }
+    let(:remote)      { double(:remote, :cache_slug => "slug", :uri => uri, :original_uri => nil, :anonymized_uri => uri) }
+    let(:compact_index) { double(Bundler::Fetcher::CompactIndex, :available? => false, :api_fetcher? => true) }
+    let(:dependency)    { double(Bundler::Fetcher::Dependency, :available? => false, :api_fetcher? => true) }
+    let(:index)         { double(Bundler::Fetcher::Index, :available? => true, :api_fetcher? => false) }
+
+    before do
+      allow(Bundler::Fetcher::CompactIndex).to receive(:new).and_return(compact_index)
+      allow(Bundler::Fetcher::Dependency).to receive(:new).and_return(dependency)
+      allow(Bundler::Fetcher::Index).to receive(:new).and_return(index)
+    end
+
+    context "when an api fetcher is available" do
+      before do
+        allow(compact_index).to receive(:available?).and_return(true)
+      end
+
+      it "is truthy" do
+        expect(fetcher).to be_api_fetcher
+      end
+    end
+
+    context "when only the index fetcher is available" do
+      it "is falsey" do
+        expect(fetcher).not_to be_api_fetcher
+      end
+    end
+  end
 end