From 7bf5f780281edc2fca83a0657e3a8d256e6e7065 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Mon, 21 Aug 2023 14:05:57 -0700 Subject: [PATCH] [rubygems/rubygems] Refactor Fetcher#api_fetcher? and fetcher loading logic https://github.com/rubygems/rubygems/commit/f664d60114 --- lib/bundler/fetcher.rb | 61 ++++++++++++------------- lib/bundler/fetcher/compact_index.rb | 4 -- lib/bundler/resolver.rb | 13 ++++-- lib/bundler/source/rubygems.rb | 2 +- spec/bundler/bundler/fetcher_spec.rb | 66 ++++++++++++++++++++++++++++ 5 files changed, 108 insertions(+), 38 deletions(-) diff --git a/lib/bundler/fetcher.rb b/lib/bundler/fetcher.rb index 2119799f68..9b64a3c771 100644 --- a/lib/bundler/fetcher.rb +++ b/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 - @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| + fetch_specs(gem_names).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 = { diff --git a/lib/bundler/fetcher/compact_index.rb b/lib/bundler/fetcher/compact_index.rb index d3160251cb..6786a841f5 100644 --- a/lib/bundler/fetcher/compact_index.rb +++ b/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 diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index 2ad35bc931..fa95ff879b 100644 --- a/lib/bundler/resolver.rb +++ b/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| - s.dependencies.any? {|d| d.name == name && !d.requirement.satisfied_by?(s.version) } # ignore versions that depend on themselves incorrectly - end.sort_by {|s| [s.version, s.platform.to_s] } + matches = source_for(name).specs.search(name) + matches = filter_invalid_self_dependencies(matches, name) + 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 diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb index 9790753204..15bd8dacbb 100644 --- a/lib/bundler/source/rubygems.rb +++ b/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 diff --git a/spec/bundler/bundler/fetcher_spec.rb b/spec/bundler/bundler/fetcher_spec.rb index 27a63c476d..92790dc7d6 100644 --- a/spec/bundler/bundler/fetcher_spec.rb +++ b/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