Merge pull request #13357 from samford/livecheck/sparkle-pass-all-items-into-strategy-block

Sparkle: Pass all items into strategy block

Library/Homebrew/livecheck/strategy/sparkle.rb

@@ -53,14 +53,17 @@ module Homebrew
 
           # @api public
           delegate short_version: :bundle_version
+
+          # @api public
+          delegate nice_version: :bundle_version
         end
 
         # Identify version information from a Sparkle appcast.
         #
         # @param content [String] the text of the Sparkle appcast
         # @return [Item, nil]
-        sig { params(content: String).returns(T.nilable(Item)) }
+        sig { params(content: String).returns(T::Array[Item]) }
-        def self.item_from_content(content)
+        def self.items_from_content(content)
           require "rexml/document"
 
           parsing_tries = 0
@@ -75,8 +78,8 @@ module Homebrew
             raise if parsing_tries > 1
 
             # When an XML document contains a prefix without a corresponding
-            # namespace, it's necessary to remove the the prefix from the
+            # namespace, it's necessary to remove the prefix from the content
-            # content to be able to successfully parse it using REXML
+            # to be able to successfully parse it using REXML
             content = content.gsub(%r{(</?| )#{Regexp.escape(undefined_prefix)}:}, '\1')
             retry
           end
@@ -89,7 +92,7 @@ module Homebrew
             end
           end
 
-          items = xml.get_elements("//rss//channel//item").map do |item|
+          xml.get_elements("//rss//channel//item").map do |item|
             enclosure = item.elements["enclosure"]
 
             if enclosure
@@ -132,15 +135,17 @@ module Homebrew
 
             data = {
               title:          title,
-              pub_date:       pub_date || Time.new(0),
+              pub_date:       pub_date,
               url:            url,
               bundle_version: bundle_version,
             }.compact
+            next if data.empty?
 
-            Item.new(**data) unless data.empty?
+            # Set a default `pub_date` (for sorting) if one isn't provided
+            data[:pub_date] ||= Time.new(0)
+
+            Item.new(**data)
           end.compact
-
-          items.max_by { |item| [item.pub_date, item.bundle_version] }
         end
 
         # Identify versions from content
@@ -155,15 +160,24 @@ module Homebrew
           ).returns(T::Array[String])
         }
         def self.versions_from_content(content, regex = nil, &block)
-          item = item_from_content(content)
+          items = items_from_content(content).sort_by { |item| [item.pub_date, item.bundle_version] }.reverse
-          return [] if item.blank?
+          return [] if items.blank?
+
+          item = items.first
 
           if block
-            block_return_value = regex.present? ? yield(item, regex) : yield(item)
+            block_return_value = case block.parameters[0]
+            when [:opt, :item], [:rest]
+              regex.present? ? yield(item, regex) : yield(item)
+            when [:opt, :items]
+              regex.present? ? yield(items, regex) : yield(items)
+            else
+              raise "First argument of Sparkle `strategy` block must be `item` or `items`"
+            end
             return Strategy.handle_block_return(block_return_value)
           end
 
-          version = item.bundle_version&.nice_version
+          version = T.must(item).bundle_version&.nice_version
           version.present? ? [version] : []
         end
 

Library/Homebrew/test/livecheck/strategy/sparkle_spec.rb

@@ -10,18 +10,49 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
   let(:appcast_url) { "https://www.example.com/example/appcast.xml" }
   let(:non_http_url) { "ftp://brew.sh/" }
 
-  let(:appcast_data) {
+  let(:item_hash) {
-    {
+    [
-      title:         "Version 1.2.3",
+      {
-      pub_date:      "Fri, 01 Jan 2021 01:23:45 +0000",
+        title:         "Version 1.2.3",
-      url:           "https://www.example.com/example/example.tar.gz",
+        pub_date:      "Fri, 01 Jan 2021 01:23:45 +0000",
-      short_version: "1.2.3",
+        url:           "https://www.example.com/example/example-1.2.3.tar.gz",
-      version:       "1234",
+        short_version: "1.2.3",
-    }
+        version:       "123",
+      },
+      {
+        title:         "Version 1.2.2",
+        pub_date:      "Not a parseable date string",
+        url:           "https://www.example.com/example/example-1.2.2.tar.gz",
+        short_version: "1.2.2",
+        version:       "122",
+      },
+    ]
   }
 
-  let(:appcast_xml) {
+  let(:xml) {
-    <<~EOS
+    first_item = <<~EOS
+      <item>
+        <title>#{item_hash[0][:title]}</title>
+        <sparkle:minimumSystemVersion>10.10</sparkle:minimumSystemVersion>
+        <sparkle:releaseNotesLink>https://www.example.com/example/#{item_hash[0][:short_version]}.html</sparkle:releaseNotesLink>
+        <pubDate>#{item_hash[0][:pub_date]}</pubDate>
+        <enclosure url="#{item_hash[0][:url]}" sparkle:shortVersionString="#{item_hash[0][:short_version]}" sparkle:version="#{item_hash[0][:version]}" length="12345678" type="application/octet-stream" sparkle:dsaSignature="ABCDEF+GHIJKLMNOPQRSTUVWXYZab/cdefghijklmnopqrst/uvwxyz1234567==" />
+      </item>
+    EOS
+
+    second_item = <<~EOS
+      <item>
+        <title>#{item_hash[1][:title]}</title>
+        <sparkle:minimumSystemVersion>10.10</sparkle:minimumSystemVersion>
+        <sparkle:releaseNotesLink>https://www.example.com/example/#{item_hash[1][:short_version]}.html</sparkle:releaseNotesLink>
+        <pubDate>#{item_hash[1][:pub_date]}</pubDate>
+        <sparkle:version>#{item_hash[1][:version]}</sparkle:version>
+        <sparkle:shortVersionString>#{item_hash[1][:short_version]}</sparkle:shortVersionString>
+        <link>#{item_hash[1][:url]}</link>
+      </item>
+    EOS
+
+    appcast_xml = <<~EOS
       <?xml version="1.0" encoding="utf-8"?>
       <rss version="2.0" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:sparkle="http://www.andymatuschak.org/xml-namespaces/sparkle">
         <channel>
@@ -29,30 +60,71 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
           <link>#{appcast_url}</link>
           <description>Most recent changes with links to updates.</description>
           <language>en</language>
-          <item>
+          #{first_item}
-            <title>#{appcast_data[:title]}</title>
+          #{second_item}
-            <sparkle:minimumSystemVersion>10.10</sparkle:minimumSystemVersion>
-            <sparkle:releaseNotesLink>https://www.example.com/example/1.2.3.html</sparkle:releaseNotesLink>
-            <pubDate>#{appcast_data[:pub_date]}</pubDate>
-            <enclosure url="#{appcast_data[:url]}" sparkle:shortVersionString="#{appcast_data[:short_version]}" sparkle:version="#{appcast_data[:version]}" length="12345678" type="application/octet-stream" sparkle:dsaSignature="ABCDEF+GHIJKLMNOPQRSTUVWXYZab/cdefghijklmnopqrst/uvwxyz1234567==" />
-          </item>
         </channel>
       </rss>
     EOS
+
+    extra_items = <<~EOS
+      #{first_item.sub(%r{<(enclosure[^>]+?)\s*?/>}, '<\1 os="not-osx" />')}
+      #{first_item.sub(/(<sparkle:minimumSystemVersion>)[^<]+?</m, '\1100<')}
+      #{first_item.sub(/(<sparkle:minimumSystemVersion>)[^<]+?</m, '\19000<')}
+      <item>
+      </item>
+    EOS
+
+    appcast_with_omitted_items = appcast_xml.sub("</item>", "</item>\n#{extra_items}")
+    no_versions_item =
+      appcast_xml
+        .sub(second_item, "")
+        .gsub(/sparkle:(shortVersionString|version)="[^"]+?"\s*/, "")
+        .sub(
+          "<title>#{item_hash[0][:title]}</title>",
+          "<title>Version</title>",
+        )
+    no_items = appcast_xml.sub(%r{<item>.+</item>}m, "")
+    undefined_namespace_xml = appcast_xml.sub(/\s*xmlns:sparkle="[^"]+?"/, "")
+
+    {
+      appcast:                    appcast_xml,
+      appcast_with_omitted_items: appcast_with_omitted_items,
+      no_versions_item:           no_versions_item,
+      no_items:                   no_items,
+      undefined_namespace:        undefined_namespace_xml,
+    }
   }
 
   let(:title_regex) { /Version\s+v?(\d+(?:\.\d+)+)\s*$/i }
 
-  let(:item) {
+  let(:items) {
-    Homebrew::Livecheck::Strategy::Sparkle::Item.new(
+    {
-      title:          appcast_data[:title],
+      appcast:          [
-      pub_date:       Time.parse(appcast_data[:pub_date]),
+        Homebrew::Livecheck::Strategy::Sparkle::Item.new(
-      url:            appcast_data[:url],
+          title:          item_hash[0][:title],
-      bundle_version: Homebrew::BundleVersion.new(appcast_data[:short_version], appcast_data[:version]),
+          pub_date:       Time.parse(item_hash[0][:pub_date]),
-    )
+          url:            item_hash[0][:url],
+          bundle_version: Homebrew::BundleVersion.new(item_hash[0][:short_version], item_hash[0][:version]),
+        ),
+        Homebrew::Livecheck::Strategy::Sparkle::Item.new(
+          title:          item_hash[1][:title],
+          pub_date:       Time.new(0),
+          url:            item_hash[1][:url],
+          bundle_version: Homebrew::BundleVersion.new(item_hash[1][:short_version], item_hash[1][:version]),
+        ),
+      ],
+      no_versions_item: [
+        Homebrew::Livecheck::Strategy::Sparkle::Item.new(
+          title:          "Version",
+          pub_date:       Time.parse(item_hash[0][:pub_date]),
+          url:            item_hash[0][:url],
+          bundle_version: nil,
+        ),
+      ],
+    }
   }
 
-  let(:versions) { [item.bundle_version.nice_version] }
+  let(:versions) { [items[:appcast][0].nice_version] }
 
   describe "::match?" do
     it "returns true for an HTTP URL" do
@@ -64,64 +136,114 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
     end
   end
 
-  describe "::item_from_content" do
+  describe "::items_from_content" do
-    let(:item_from_appcast_xml) { sparkle.item_from_content(appcast_xml) }
+    let(:items_from_appcast_xml) { sparkle.items_from_content(xml[:appcast]) }
+    let(:first_item) { items_from_appcast_xml[0] }
 
     it "returns nil if content is blank" do
-      expect(sparkle.item_from_content("")).to be_nil
+      expect(sparkle.items_from_content("")).to eq([])
     end
 
-    it "returns an Item when given XML data" do
+    it "returns an array of Items when given XML data" do
-      expect(item_from_appcast_xml).to be_a(Homebrew::Livecheck::Strategy::Sparkle::Item)
+      expect(items_from_appcast_xml).to eq(items[:appcast])
-      expect(item_from_appcast_xml).to eq(item)
+      expect(first_item.title).to eq(item_hash[0][:title])
-      expect(item_from_appcast_xml.title).to eq(appcast_data[:title])
+      expect(first_item.pub_date).to eq(Time.parse(item_hash[0][:pub_date]))
-      expect(item_from_appcast_xml.pub_date).to eq(Time.parse(appcast_data[:pub_date]))
+      expect(first_item.url).to eq(item_hash[0][:url])
-      expect(item_from_appcast_xml.url).to eq(appcast_data[:url])
+      expect(first_item.short_version).to eq(item_hash[0][:short_version])
-      expect(item_from_appcast_xml.short_version).to eq(appcast_data[:short_version])
+      expect(first_item.version).to eq(item_hash[0][:version])
-      expect(item_from_appcast_xml.version).to eq(appcast_data[:version])
+
+      expect(sparkle.items_from_content(xml[:no_versions_item])).to eq(items[:no_versions_item])
     end
   end
 
   describe "::versions_from_content" do
+    let(:subbed_items) { items[:appcast].map { |item| item.nice_version.sub("1", "0") } }
+
     it "returns an array of version strings when given content" do
-      expect(sparkle.versions_from_content(appcast_xml)).to eq(versions)
+      expect(sparkle.versions_from_content(xml[:appcast])).to eq(versions)
+      expect(sparkle.versions_from_content(xml[:appcast_with_omitted_items])).to eq(versions)
+      expect(sparkle.versions_from_content(xml[:no_versions_item])).to eq([])
+      expect(sparkle.versions_from_content(xml[:undefined_namespace])).to eq(versions)
+    end
+
+    it "returns an empty array if no items are found" do
+      expect(sparkle.versions_from_content(xml[:no_items])).to eq([])
     end
 
     it "returns an array of version strings when given content and a block" do
       # Returning a string from block
       expect(
-        sparkle.versions_from_content(appcast_xml) do |item|
+        sparkle.versions_from_content(xml[:appcast]) do |item|
-          item.bundle_version&.nice_version&.sub("3", "4")
+          item.nice_version&.sub("1", "0")
         end,
-      ).to eq([item.bundle_version.nice_version.sub("3", "4")])
+      ).to eq([subbed_items[0]])
 
-      # Returning an array of strings from block (unlikely to be used)
+      # Returning an array of strings from block
-      expect(sparkle.versions_from_content(appcast_xml) { versions }).to eq(versions)
+      expect(
+        sparkle.versions_from_content(xml[:appcast]) do |items|
+          items.map { |item| item.nice_version&.sub("1", "0") }
+        end,
+      ).to eq(subbed_items)
     end
 
     it "returns an array of version strings when given content, a regex, and a block" do
-      # Returning a string from block
+      # Returning a string from the block
       expect(
-        sparkle.versions_from_content(appcast_xml, title_regex) do |item, regex|
+        sparkle.versions_from_content(xml[:appcast], title_regex) do |item, regex|
           item.title[regex, 1]
         end,
-      ).to eq([item.bundle_version.short_version])
+      ).to eq([items[:appcast][0].short_version])
 
-      # Returning an array of strings from block (unlikely to be used)
       expect(
-        sparkle.versions_from_content(appcast_xml, title_regex) do |item, regex|
+        sparkle.versions_from_content(xml[:appcast], title_regex) do |items, regex|
+          next if (item = items[0]).blank?
+
+          match = item&.title&.match(regex)
+          next if match.blank?
+
+          "#{match[1]},#{item.version}"
+        end,
+      ).to eq(["#{items[:appcast][0].short_version},#{items[:appcast][0].version}"])
+
+      # Returning an array of strings from the block
+      expect(
+        sparkle.versions_from_content(xml[:appcast], title_regex) do |item, regex|
           [item.title[regex, 1]]
         end,
-      ).to eq([item.bundle_version.short_version])
+      ).to eq([items[:appcast][0].short_version])
+
+      expect(
+        sparkle.versions_from_content(xml[:appcast], &:short_version),
+      ).to eq([items[:appcast][0].short_version])
+
+      expect(
+        sparkle.versions_from_content(xml[:appcast], title_regex) do |items, regex|
+          items.map { |item| item.title[regex, 1] }
+        end,
+      ).to eq(items[:appcast].map(&:short_version))
     end
 
     it "allows a nil return from a block" do
-      expect(sparkle.versions_from_content(appcast_xml) { next }).to eq([])
+      expect(
+        sparkle.versions_from_content(xml[:appcast]) do |item|
+          _ = item # To appease `brew style` without modifying arg name
+          next
+        end,
+      ).to eq([])
     end
 
     it "errors on an invalid return type from a block" do
-      expect { sparkle.versions_from_content(appcast_xml) { 123 } }
+      expect {
-        .to raise_error(TypeError, Homebrew::Livecheck::Strategy::INVALID_BLOCK_RETURN_VALUE_MSG)
+        sparkle.versions_from_content(xml[:appcast]) do |item|
+          _ = item # To appease `brew style` without modifying arg name
+          123
+        end
+      }.to raise_error(TypeError, Homebrew::Livecheck::Strategy::INVALID_BLOCK_RETURN_VALUE_MSG)
+    end
+
+    it "errors if the first block argument uses an unhandled name" do
+      expect { sparkle.versions_from_content(xml[:appcast]) { |something| something } }
+        .to raise_error("First argument of Sparkle `strategy` block must be `item` or `items`")
     end
   end
 end