[ruby/rubygems] Write gem files atomically

This change updates `write_binary` to use a new class,
`AtomicFileWriter.open` to write the gem's files. This implementation
is borrowed from Active Support's [`atomic_write`](https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/file/atomic.rb).

Atomic write will write the files to a temporary file and then once
created, sets permissions and renames the file. If the file is corrupted
- ie on failed download, an error occurs, or for some other reason, the
real file will not be created. The changes made here make `verify_gz`
obsolete, we don't need to verify it if we have successfully created the
file atomically. If it exists, it is not corrupt. If it is corrupt, the
file won't exist on disk.

While writing tests for this functionality I replaced the
`RemoteFetcher` stub with `FakeFetcher` except for where we really do
need to overwrite the `RemoteFetcher`. The new test implementation is much
clearer on what it's trying to accomplish versus the prior test
implementation.

https://github.com/ruby/rubygems/commit/0cd4b54291

2 files changed, 84 insertions, 84 deletions

diff --git a/test/rubygems/test_gem_installer.rb b/test/rubygems/test_gem_installer.rb
index 293fe1e823..0220a41f88 100644
--- a/test/rubygems/test_gem_installer.rb
+++ b/test/rubygems/test_gem_installer.rb
@@ -2385,25 +2385,31 @@ class TestGemInstaller < Gem::InstallerTestCase
     installer = Gem::Installer.for_spec @spec
     installer.gem_home = @gemhome
 
-    File.singleton_class.class_eval do
-      alias_method :original_binwrite, :binwrite
-
-      def binwrite(path, data)
+    assert_raise(Errno::ENOSPC) do
+      Gem::AtomicFileWriter.open(@spec.spec_file) do
         raise Errno::ENOSPC
       end
     end
 
-    assert_raise Errno::ENOSPC do
-      installer.write_spec
-    end
-
     assert_path_not_exist @spec.spec_file
-  ensure
-    File.singleton_class.class_eval do
-      remove_method :binwrite
-      alias_method :binwrite, :original_binwrite
-      remove_method :original_binwrite
-    end
+  end
+
+  def test_write_default_spec
+    @spec = setup_base_spec
+    @spec.files = %w[a.rb b.rb c.rb]
+
+    installer = Gem::Installer.for_spec @spec
+    installer.gem_home = @gemhome
+
+    installer.write_default_spec
+
+    assert_path_exist installer.default_spec_file
+
+    loaded = Gem::Specification.load installer.default_spec_file
+
+    assert_equal @spec.files, loaded.files
+    assert_equal @spec.name, loaded.name
+    assert_equal @spec.version, loaded.version
   end
 
   def test_dir
diff --git a/test/rubygems/test_gem_remote_fetcher.rb b/test/rubygems/test_gem_remote_fetcher.rb
index 5c1d89fad6..9badd75b42 100644
--- a/test/rubygems/test_gem_remote_fetcher.rb
+++ b/test/rubygems/test_gem_remote_fetcher.rb
@@ -60,7 +60,7 @@ class TestGemRemoteFetcher < Gem::TestCase
     uri = Gem::URI "http://example/file"
     path = File.join @tempdir, "file"
 
-    fetcher = util_fuck_with_fetcher "hello"
+    fetcher = fake_fetcher(uri.to_s, "hello")
 
     data = fetcher.cache_update_path uri, path
 
@@ -75,7 +75,7 @@ class TestGemRemoteFetcher < Gem::TestCase
       path = File.join @tempdir, "file"
       data = String.new("\xC8").force_encoding(Encoding::BINARY)
 
-      fetcher = util_fuck_with_fetcher data
+      fetcher = fake_fetcher(uri.to_s, data)
 
       written_data = fetcher.cache_update_path uri, path
 
@@ -88,7 +88,7 @@ class TestGemRemoteFetcher < Gem::TestCase
     uri = Gem::URI "http://example/file"
     path = File.join @tempdir, "file"
 
-    fetcher = util_fuck_with_fetcher "hello"
+    fetcher = fake_fetcher(uri.to_s, "hello")
 
     data = fetcher.cache_update_path uri, path, false
 
@@ -97,103 +97,79 @@ class TestGemRemoteFetcher < Gem::TestCase
     assert_path_not_exist path
   end
 
-  def util_fuck_with_fetcher(data, blow = false)
-    fetcher = Gem::RemoteFetcher.fetcher
-    fetcher.instance_variable_set :@test_data, data
-
-    if blow
-      def fetcher.fetch_path(arg, *rest)
-        # OMG I'm such an ass
-        class << self; remove_method :fetch_path; end
-        def self.fetch_path(arg, *rest)
-          @test_arg = arg
-          @test_data
-        end
+  def test_cache_update_path_overwrites_existing_file
+    uri = Gem::URI "http://example/file"
+    path = File.join @tempdir, "file"
 
-        raise Gem::RemoteFetcher::FetchError.new("haha!", "")
-      end
-    else
-      def fetcher.fetch_path(arg, *rest)
-        @test_arg = arg
-        @test_data
-      end
-    end
+    # Create existing file with old content
+    File.write(path, "old content")
+    assert_equal "old content", File.read(path)
+
+    fetcher = fake_fetcher(uri.to_s, "new content")
 
-    fetcher
+    data = fetcher.cache_update_path uri, path
+
+    assert_equal "new content", data
+    assert_equal "new content", File.read(path)
   end
 
   def test_download
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://gems.example.com")
-    assert_equal("http://gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_auth
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://user:password@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://user:password@gems.example.com")
-    assert_equal("http://user:password@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_token
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://token@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://token@gems.example.com")
-    assert_equal("http://token@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_x_oauth_basic
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://token:x-oauth-basic@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://token:x-oauth-basic@gems.example.com")
-    assert_equal("http://token:x-oauth-basic@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_encoded_auth
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://user:%25pas%25sword@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://user:%25pas%25sword@gems.example.com")
-    assert_equal("http://user:%25pas%25sword@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
@@ -235,8 +211,9 @@ class TestGemRemoteFetcher < Gem::TestCase
 
   def test_download_install_dir
     a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     install_dir = File.join @tempdir, "more_gems"
 
@@ -245,8 +222,7 @@ class TestGemRemoteFetcher < Gem::TestCase
     actual = fetcher.download(@a1, "http://gems.example.com", install_dir)
 
     assert_equal a1_cache_gem, actual
-    assert_equal("http://gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
 
     assert File.exist?(a1_cache_gem)
   end
@@ -282,7 +258,12 @@ class TestGemRemoteFetcher < Gem::TestCase
       FileUtils.chmod 0o555, @a1.cache_dir
       FileUtils.chmod 0o555, @gemhome
 
-      fetcher = util_fuck_with_fetcher File.read(@a1_gem)
+      fetcher = Gem::RemoteFetcher.fetcher
+      def fetcher.fetch_path(uri, *rest)
+        File.read File.join(@test_gem_dir, "a-1.gem")
+      end
+      fetcher.instance_variable_set(:@test_gem_dir, File.dirname(@a1_gem))
+
       fetcher.download(@a1, "http://gems.example.com")
       a1_cache_gem = File.join Gem.user_dir, "cache", @a1.file_name
       assert File.exist? a1_cache_gem
@@ -301,19 +282,21 @@ class TestGemRemoteFetcher < Gem::TestCase
     end
     e1.loaded_from = File.join(@gemhome, "specifications", e1.full_name)
 
-    e1_data = nil
-    File.open e1_gem, "rb" do |fp|
-      e1_data = fp.read
-    end
+    e1_data = File.open e1_gem, "rb", &:read
 
-    fetcher = util_fuck_with_fetcher e1_data, :blow_chunks
+    fetcher = Gem::RemoteFetcher.fetcher
+    def fetcher.fetch_path(uri, *rest)
+      @call_count ||= 0
+      @call_count += 1
+      raise Gem::RemoteFetcher::FetchError.new("error", uri) if @call_count == 1
+      @test_data
+    end
+    fetcher.instance_variable_set(:@test_data, e1_data)
 
     e1_cache_gem = e1.cache_file
 
     assert_equal e1_cache_gem, fetcher.download(e1, "http://gems.example.com")
 
-    assert_equal("http://gems.example.com/gems/#{e1.original_name}.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
     assert File.exist?(e1_cache_gem)
   end
 
@@ -592,6 +575,8 @@ class TestGemRemoteFetcher < Gem::TestCase
     end
   end
 
+  private
+
   def assert_error(exception_class = Exception)
     got_exception = false
 
@@ -603,4 +588,13 @@ class TestGemRemoteFetcher < Gem::TestCase
 
     assert got_exception, "Expected exception conforming to #{exception_class}"
   end
+
+  def fake_fetcher(url, data)
+    original_fetcher = Gem::RemoteFetcher.fetcher
+    fetcher = Gem::FakeFetcher.new
+    fetcher.data[url] = data
+    Gem::RemoteFetcher.fetcher = fetcher
+  ensure
+    Gem::RemoteFetcher.fetcher = original_fetcher
+  end
 end