From 1fa0e6d9980d0d5d9b9355d0cb1eee24c60e7a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Mon, 26 Apr 2021 13:33:36 +0200 Subject: [PATCH] Revert "Accept responses with no etag header" This reverts commit 84147066c14d5c1d95fd4f309824bc735506ee77. Since we made that change we added one spec that relied on responses without an Etag being accepted. That spec needs to be updated with a correct checksum since we no longer do that. Also, we add a regression spec for the issue being fixed here. Namely, if the gem server does not provide ETags, its responses don't have incremental content, but it still replies to partial range requests. In this situation, we want to fallback to making a full update so that things work. --- .../bundler/compact_index_client/updater.rb | 6 +-- .../compact_index_client/updater_spec.rb | 13 ++++--- .../spec/install/gems/compact_index_spec.rb | 22 +++++++++++ ..._partial_update_no_etag_not_incremental.rb | 39 +++++++++++++++++++ 4 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 bundler/spec/support/artifice/compact_index_partial_update_no_etag_not_incremental.rb diff --git a/bundler/lib/bundler/compact_index_client/updater.rb b/bundler/lib/bundler/compact_index_client/updater.rb index 1dc70012c288..9348f3f5d082 100644 --- a/bundler/lib/bundler/compact_index_client/updater.rb +++ b/bundler/lib/bundler/compact_index_client/updater.rb @@ -58,8 +58,8 @@ def update(local_path, remote_path, retrying = nil) end end - etag = (response["ETag"] || "").gsub(%r{\AW/}, "") - if etag.length.zero? || etag_for(local_temp_path) == etag + response_etag = (response["ETag"] || "").gsub(%r{\AW/}, "") + if etag_for(local_temp_path) == response_etag SharedHelpers.filesystem_access(local_path) do FileUtils.mv(local_temp_path, local_path) end @@ -67,7 +67,7 @@ def update(local_path, remote_path, retrying = nil) end if retrying - raise MisMatchedChecksumError.new(remote_path, etag, etag_for(local_temp_path)) + raise MisMatchedChecksumError.new(remote_path, response_etag, etag_for(local_temp_path)) end update(local_path, remote_path, :retrying) diff --git a/bundler/spec/bundler/compact_index_client/updater_spec.rb b/bundler/spec/bundler/compact_index_client/updater_spec.rb index 4acd7dbc6341..699ce684d839 100644 --- a/bundler/spec/bundler/compact_index_client/updater_spec.rb +++ b/bundler/spec/bundler/compact_index_client/updater_spec.rb @@ -16,11 +16,14 @@ # Regression test for https://github.com/rubygems/bundler/issues/5463 let(:response) { double(:response, :body => "abc123") } - it "treats the response as an update" do - expect(response).to receive(:[]).with("ETag") { nil } - expect(fetcher).to receive(:call) { response } + it "MisMatchedChecksumError is raised" do + # Twice: #update retries on failure + expect(response).to receive(:[]).with("ETag").twice { nil } + expect(fetcher).to receive(:call).twice { response } - updater.update(local_path, remote_path) + expect do + updater.update(local_path, remote_path) + end.to raise_error(Bundler::CompactIndexClient::Updater::MisMatchedChecksumError) end end @@ -56,7 +59,7 @@ begin $VERBOSE = false Encoding.default_internal = "ASCII" - expect(response).to receive(:[]).with("ETag") { nil } + expect(response).to receive(:[]).with("ETag").and_return('"348dd9e974848a61e8abe2dffcfcce93"') expect(fetcher).to receive(:call) { response } updater.update(local_path, remote_path) diff --git a/bundler/spec/install/gems/compact_index_spec.rb b/bundler/spec/install/gems/compact_index_spec.rb index b1f361ebec94..9454508e41e4 100644 --- a/bundler/spec/install/gems/compact_index_spec.rb +++ b/bundler/spec/install/gems/compact_index_spec.rb @@ -826,6 +826,28 @@ def start expect(the_bundle).to include_gems "rack 1.0.0" end + it "performs full update if server endpoints serve partial content responses but don't have incremental content and provide no Etag" do + build_repo4 do + build_gem "rack", "0.9.1" + end + + install_gemfile <<-G, :artifice => "compact_index", :env => { "BUNDLER_SPEC_GEM_REPO" => gem_repo4.to_s } + source "#{source_uri}" + gem 'rack', '0.9.1' + G + + update_repo4 do + build_gem "rack", "1.0.0" + end + + install_gemfile <<-G, :artifice => "compact_index_partial_update_no_etag_not_incremental", :env => { "BUNDLER_SPEC_GEM_REPO" => gem_repo4.to_s } + source "#{source_uri}" + gem 'rack', '1.0.0' + G + + expect(the_bundle).to include_gems "rack 1.0.0" + end + it "performs full update of compact index info cache if range is not satisfiable" do gemfile <<-G source "#{source_uri}" diff --git a/bundler/spec/support/artifice/compact_index_partial_update_no_etag_not_incremental.rb b/bundler/spec/support/artifice/compact_index_partial_update_no_etag_not_incremental.rb new file mode 100644 index 000000000000..e2fa6b41386d --- /dev/null +++ b/bundler/spec/support/artifice/compact_index_partial_update_no_etag_not_incremental.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require_relative "compact_index" + +Artifice.deactivate + +class CompactIndexPartialUpdateNoEtagNotIncremental < CompactIndexAPI + def partial_update_no_etag + response_body = yield + headers "Surrogate-Control" => "max-age=2592000, stale-while-revalidate=60" + content_type "text/plain" + requested_range_for(response_body) + end + + get "/versions" do + partial_update_no_etag do + file = tmp("versions.list") + FileUtils.rm_f(file) + file = CompactIndex::VersionsFile.new(file.to_s) + file.create(gems) + lines = file.contents.split("\n") + + # shuffle versions so new versions are not appended to the end + [*lines[0..-2], lines.last.split(",").reverse.join(",")].join("\n") + end + end + + get "/info/:name" do + partial_update_no_etag do + gem = gems.find {|g| g.name == params[:name] } + lines = CompactIndex.info(gem ? gem.versions : []).split("\n") + + # shuffle versions so new versions are not appended to the end + [lines.first, lines.last, *lines[1..-2]].join("\n") + end + end +end + +Artifice.activate_with(CompactIndexPartialUpdateNoEtagNotIncremental)