Merge download strategy specs.

Library/Homebrew/download_strategy.rb

@@ -193,13 +193,20 @@ class AbstractFileDownloadStrategy < AbstractDownloadStrategy
   private
 
   def ext
+    uri_path = if URI::DEFAULT_PARSER.make_regexp =~ @url
+      uri = URI(@url)
+      uri.query ? "#{uri.path}?#{uri.query}" : uri.path
+    else
+      @url
+    end
+
     # We need a Pathname because we've monkeypatched extname to support double
     # extensions (e.g. tar.gz).
     # We can't use basename_without_params, because given a URL like
     #   https://example.com/download.php?file=foo-1.0.tar.gz
     # the extension we want is ".tar.gz", not ".php".
-    Pathname.new(@url).ascend do |path|
+    Pathname.new(uri_path).ascend do |path|
-      ext = path.extname[/[^?]+/]
+      ext = path.extname[/[^?&]+/]
       return ext if ext
     end
     nil
@@ -335,14 +342,15 @@ end
 # Query parameters on the URL are converted into POST parameters
 class CurlPostDownloadStrategy < CurlDownloadStrategy
   def _fetch
-    base_url, data = if meta.key?(:data)
+    args = if meta.key?(:data)
       escape_data = ->(d) { ["-d", URI.encode_www_form([d])] }
-      [@url, meta[:data].flat_map(&escape_data)]
+      [@url, *meta[:data].flat_map(&escape_data)]
     else
-      @url.split("?", 2)
+      url, query = @url.split("?", 2)
+      query.nil? ? [url, "-X", "POST"] : [url, "-d", query]
     end
 
-    curl_download base_url, "--data", data, to: temporary_path
+    curl_download(*args, to: temporary_path)
   end
 end
 
@@ -606,9 +614,9 @@ class SubversionDownloadStrategy < VCSDownloadStrategy
     end
 
     if target.directory?
-      system_command("svn", args: ["update", *args], chdir: target.to_s)
+      system_command!("svn", args: ["update", *args], chdir: target.to_s)
     else
-      system_command("svn", args: ["checkout", url, target, *args])
+      system_command!("svn", args: ["checkout", url, target, *args])
     end
   end
 

Library/Homebrew/test/download_strategies_spec.rb

@@ -227,7 +227,7 @@ describe CurlDownloadStrategy do
 
   let(:name) { "foo" }
   let(:url) { "http://example.com/foo.tar.gz" }
-  let(:version) { nil }
+  let(:version) { "1.2.3" }
   let(:specs) { { user: "download:123456" } }
 
   it "parses the opts and sets the corresponding args" do
@@ -238,13 +238,182 @@ describe CurlDownloadStrategy do
     subject { described_class.new(url, name, version, **specs).cached_location }
 
     context "when URL ends with file" do
-      it { is_expected.to eq(HOMEBREW_CACHE/"foo--.tar.gz") }
+      it { is_expected.to eq(HOMEBREW_CACHE/"foo--1.2.3.tar.gz") }
     end
 
     context "when URL file is in middle" do
       let(:url) { "http://example.com/foo.tar.gz/from/this/mirror" }
 
-      it { is_expected.to eq(HOMEBREW_CACHE/"foo--.tar.gz") }
+      it { is_expected.to eq(HOMEBREW_CACHE/"foo--1.2.3.tar.gz") }
+    end
+  end
+
+  describe "#fetch" do
+    before(:each) do
+      FileUtils.touch subject.temporary_path
+    end
+
+    it "calls curl with default arguments" do
+      expect(subject).to receive(:curl).with(
+        "--location",
+        "--remote-time",
+        "--continue-at", "-",
+        "--output", an_instance_of(Pathname),
+        url,
+        an_instance_of(Hash)
+      )
+
+      subject.fetch
+    end
+
+    context "with an explicit user agent" do
+      let(:specs) { { user_agent: "Mozilla/25.0.1" } }
+
+      it "adds the appropriate curl args" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2)).to include(["--user-agent", "Mozilla/25.0.1"])
+        }
+
+        subject.fetch
+      end
+    end
+
+    context "with a generalized fake user agent" do
+      alias_matcher :a_string_matching, :match
+
+      let(:specs) { { user_agent: :fake } }
+
+      it "adds the appropriate curl args" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2).to_a).to include(["--user-agent", a_string_matching(/Mozilla.*Mac OS X 10.*AppleWebKit/)])
+        }
+
+        subject.fetch
+      end
+    end
+
+    context "with cookies set" do
+      let(:specs) {
+        {
+          cookies: {
+            coo: "kie",
+            mon: "ster",
+          },
+        }
+      }
+
+      it "adds the appropriate curl args" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2)).to include(["-b", "coo=kie;mon=ster"])
+        }
+
+        subject.fetch
+      end
+    end
+
+    context "with referer set" do
+      let(:specs) { { referer: "http://somehost/also" } }
+
+      it "adds the appropriate curl args" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2)).to include(["-e", "http://somehost/also"])
+        }
+
+        subject.fetch
+      end
+    end
+  end
+
+  describe "#cached_location" do
+    context "with a file name trailing the URL path" do
+      let(:url) { "http://example.com/cask.dmg" }
+      its("cached_location.extname") { is_expected.to eq(".dmg") }
+    end
+
+    context "with no discernible file name in it" do
+      let(:url) { "http://example.com/download" }
+      its("cached_location.basename.to_path") { is_expected.to eq("foo--1.2.3") }
+    end
+
+    context "with a file name trailing the first query parameter" do
+      let(:url) { "http://example.com/download?file=cask.zip&a=1" }
+      its("cached_location.extname") { is_expected.to eq(".zip") }
+    end
+
+    context "with a file name trailing the second query parameter" do
+      let(:url) { "http://example.com/dl?a=1&file=cask.zip&b=2" }
+      its("cached_location.extname") { is_expected.to eq(".zip") }
+    end
+
+    context "with an unusually long query string" do
+      let(:url) do
+        [
+          "https://node49152.ssl.fancycdn.example.com",
+          "/fancycdn/node/49152/file/upload/download",
+          "?cask_class=zf920df",
+          "&cask_group=2348779087242312",
+          "&cask_archive_file_name=cask.zip",
+          "&signature=CGmDulxL8pmutKTlCleNTUY%2FyO9Xyl5u9yVZUE0",
+          "uWrjadjuz67Jp7zx3H7NEOhSyOhu8nzicEHRBjr3uSoOJzwkLC8L",
+          "BLKnz%2B2X%2Biq5m6IdwSVFcLp2Q1Hr2kR7ETn3rF1DIq5o0lHC",
+          "yzMmyNe5giEKJNW8WF0KXriULhzLTWLSA3ZTLCIofAdRiiGje1kN",
+          "YY3C0SBqymQB8CG3ONn5kj7CIGbxrDOq5xI2ZSJdIyPysSX7SLvE",
+          "DBw2KdR24q9t1wfjS9LUzelf5TWk6ojj8p9%2FHjl%2Fi%2FVCXN",
+          "N4o1mW%2FMayy2tTY1qcC%2FTmqI1ulZS8SNuaSgr9Iys9oDF1%2",
+          "BPK%2B4Sg==",
+        ].join
+      end
+
+      its("cached_location.extname") { is_expected.to eq(".zip") }
+      its("cached_location.to_path.length") { is_expected.to be_between(0, 255) }
+    end
+  end
+end
+
+describe CurlPostDownloadStrategy do
+  subject { described_class.new(url, name, version, **specs) }
+
+  let(:name) { "foo" }
+  let(:url) { "http://example.com/foo.tar.gz" }
+  let(:version) { "1.2.3" }
+  let(:specs) { {} }
+
+  describe "#fetch" do
+    before(:each) do
+      FileUtils.touch subject.temporary_path
+    end
+
+    context "with :using and :data specified" do
+      let(:specs) {
+        {
+          using: :post,
+          data:  {
+            form: "data",
+            is:   "good",
+          },
+        }
+      }
+
+      it "adds the appropriate curl args" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2)).to include(["-d", "form=data"])
+          expect(args.each_cons(2)).to include(["-d", "is=good"])
+        }
+
+        subject.fetch
+      end
+    end
+
+    context "with :using but no :data" do
+      let(:specs) { { using: :post } }
+
+      it "adds the appropriate curl args" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2)).to include(["-X", "POST"])
+        }
+
+        subject.fetch
+      end
     end
   end
 end
@@ -329,6 +498,40 @@ describe ScpDownloadStrategy do
   end
 end
 
+describe SubversionDownloadStrategy do
+  subject { described_class.new(url, name, version, **specs) }
+
+  let(:name) { "foo" }
+  let(:url) { "http://example.com/foo.tar.gz" }
+  let(:version) { "1.2.3" }
+  let(:specs) { {} }
+
+  describe "#fetch" do
+    context "with :trust_cert set" do
+      let(:specs) { { trust_cert: true } }
+
+      it "adds the appropriate svn args" do
+        expect(subject).to receive(:system_command!)
+          .with("svn", args: array_including("--trust-server-cert", "--non-interactive"))
+
+        subject.fetch
+      end
+    end
+
+    context "with :revision set" do
+      let(:specs) { { revision: "10" } }
+
+      it "adds svn arguments for :revision" do
+        expect(subject).to receive(:system_command!) { |*, args:, **|
+          expect(args.each_cons(2)).to include(["-r", "10"])
+        }
+
+        subject.fetch
+      end
+    end
+  end
+end
+
 describe DownloadStrategyDetector do
   describe "::detect" do
     subject { described_class.detect(url, strategy) }