From b4cb47815f10d5914df2c33ccd4ac7ac569743bc Mon Sep 17 00:00:00 2001
From: Sam Ford <1584702+samford@users.noreply.github.com>
Date: Tue, 31 May 2022 13:18:40 -0400
Subject: [PATCH] Sparkle: Pass all items into strategy block
It's sometimes necessary to work with all the items in a Sparkle feed
to be able to correctly identify the newest version but livecheck's
`Sparkle` strategy only passes the `item` it views as newest into a
`strategy` block.
This updates the `Sparkle` strategy to optionally pass all items into
a `strategy` block, so we can manipulate them (e.g., filtering,
sorting). This is enabled by naming the first argument of the
strategy block `items` instead of `item`. `Sparkle` `strategy` blocks
where the first argument is `item` will continue to work as expected.
This necessarily updates `#item_from_content` (now
`items_from_content`) to return all items. I've decided to move the
sorting out of `#items_from_content`, so it simply returns the items
in the order they appear. If there is ever an exceptional situation
where we need the original order, this will technically allow for it.
The sorting has instead been moved into the `#versions_from_content`
method, to maintain the existing behavior. I thought about passing
the items into the `strategy` block in their original order but it
feels like sorting by default is the better approach for now (partly
from the perspective of maintaining existing behavior) and we can
always revisit this in the future if a cask ever requires the
original order.
Lastly, this expands the `Sparkle` tests to increase coverage. The
only untested parts are `#find_versions` (which currently
requires a network request) and a couple safeguard `raise` calls
when there's a `REXML::UndefinedNamespaceException` (which shouldn't
be encountered unless something is broken).
---
.../Homebrew/livecheck/strategy/sparkle.rb | 40 +++-
.../test/livecheck/strategy/sparkle_spec.rb | 224 ++++++++++++++----
2 files changed, 200 insertions(+), 64 deletions(-)
diff --git a/Library/Homebrew/livecheck/strategy/sparkle.rb b/Library/Homebrew/livecheck/strategy/sparkle.rb
index b5cc80aea0..ac17dd344a 100644
--- a/Library/Homebrew/livecheck/strategy/sparkle.rb
+++ b/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)) }
- def self.item_from_content(content)
+ sig { params(content: String).returns(T::Array[Item]) }
+ 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
- # content to be able to successfully parse it using REXML
+ # namespace, it's necessary to remove the prefix from the content
+ # to be able to successfully parse it using REXML
content = content.gsub(%r{(
+ #{item_hash[0][:title]}
+ 10.10
+ https://www.example.com/example/#{item_hash[0][:short_version]}.html
+ #{item_hash[0][:pub_date]}
+
+
+ EOS
+
+ second_item = <<~EOS
+
+ #{item_hash[1][:title]}
+ 10.10
+ https://www.example.com/example/#{item_hash[1][:short_version]}.html
+ #{item_hash[1][:pub_date]}
+ #{item_hash[1][:version]}
+ #{item_hash[1][:short_version]}
+ #{item_hash[1][:url]}
+
+ EOS
+
+ appcast_xml = <<~EOS
@@ -29,30 +60,71 @@ describe Homebrew::Livecheck::Strategy::Sparkle do
#{appcast_url}
Most recent changes with links to updates.en
-
- #{appcast_data[:title]}
- 10.10
- https://www.example.com/example/1.2.3.html
- #{appcast_data[:pub_date]}
-
-
+ #{first_item}
+ #{second_item}
EOS
+
+ extra_items = <<~EOS
+ #{first_item.sub(%r{<(enclosure[^>]+?)\s*?/>}, '<\1 os="not-osx" />')}
+ #{first_item.sub(/()[^<]+?)[^<]+?
+
+ EOS
+
+ appcast_with_omitted_items = appcast_xml.sub("", "\n#{extra_items}")
+ no_versions_item =
+ appcast_xml
+ .sub(second_item, "")
+ .gsub(/sparkle:(shortVersionString|version)="[^"]+?"\s*/, "")
+ .sub(
+ "#{item_hash[0][:title]}",
+ "Version",
+ )
+ no_items = appcast_xml.sub(%r{.+}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) {
- Homebrew::Livecheck::Strategy::Sparkle::Item.new(
- title: appcast_data[:title],
- pub_date: Time.parse(appcast_data[:pub_date]),
- url: appcast_data[:url],
- bundle_version: Homebrew::BundleVersion.new(appcast_data[:short_version], appcast_data[:version]),
- )
+ let(:items) {
+ {
+ appcast: [
+ Homebrew::Livecheck::Strategy::Sparkle::Item.new(
+ title: item_hash[0][:title],
+ 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
- let(:item_from_appcast_xml) { sparkle.item_from_content(appcast_xml) }
+ describe "::items_from_content" do
+ 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
- expect(item_from_appcast_xml).to be_a(Homebrew::Livecheck::Strategy::Sparkle::Item)
- expect(item_from_appcast_xml).to eq(item)
- expect(item_from_appcast_xml.title).to eq(appcast_data[:title])
- expect(item_from_appcast_xml.pub_date).to eq(Time.parse(appcast_data[:pub_date]))
- expect(item_from_appcast_xml.url).to eq(appcast_data[:url])
- expect(item_from_appcast_xml.short_version).to eq(appcast_data[:short_version])
- expect(item_from_appcast_xml.version).to eq(appcast_data[:version])
+ it "returns an array of Items when given XML data" do
+ expect(items_from_appcast_xml).to eq(items[:appcast])
+ expect(first_item.title).to eq(item_hash[0][:title])
+ expect(first_item.pub_date).to eq(Time.parse(item_hash[0][:pub_date]))
+ expect(first_item.url).to eq(item_hash[0][:url])
+ expect(first_item.short_version).to eq(item_hash[0][:short_version])
+ expect(first_item.version).to eq(item_hash[0][: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|
- item.bundle_version&.nice_version&.sub("3", "4")
+ sparkle.versions_from_content(xml[:appcast]) do |item|
+ 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)
- expect(sparkle.versions_from_content(appcast_xml) { versions }).to eq(versions)
+ # Returning an array of strings from block
+ 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 } }
- .to raise_error(TypeError, Homebrew::Livecheck::Strategy::INVALID_BLOCK_RETURN_VALUE_MSG)
+ expect {
+ 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