Skip to content

Commit

Permalink
Revert "Accept responses with no etag header"
Browse files Browse the repository at this point in the history
This reverts commit 8414706.

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.
  • Loading branch information
deivid-rodriguez committed Apr 30, 2021
1 parent 4c4aabf commit 1fa0e6d
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 8 deletions.
6 changes: 3 additions & 3 deletions bundler/lib/bundler/compact_index_client/updater.rb
Expand Up @@ -58,16 +58,16 @@ 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
return 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)
Expand Down
13 changes: 8 additions & 5 deletions bundler/spec/bundler/compact_index_client/updater_spec.rb
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down
22 changes: 22 additions & 0 deletions bundler/spec/install/gems/compact_index_spec.rb
Expand Up @@ -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}"
Expand Down
@@ -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)

0 comments on commit 1fa0e6d

Please sign in to comment.