From d7b515bf7340ff6df3dc725f1847cbc9c96c6916 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Thu, 24 Oct 2024 09:00:18 -0400 Subject: [PATCH 1/2] livecheck: error on invalid url symbol Up to now, we haven't been accounting for `#url` symbol arguments in `livecheck` blocks that don't reference a checkable URL. This can either be an invalid symbol (e.g., using the `:stable` formula symbol in a cask) or a valid symbol where the referenced URL doesn't exist (e.g., using `:head` when there's no `head` URL). [Almost all of the valid symbols are required URLs but `head` is optional.] Over the years, we've had a handful of slips where we've used `:url` in formulae (when it's only valid in casks) and `:stable` in casks (when it's only valid in formulae). In this scenario, `livecheck_url_string` is `nil`, so livecheck falls back to `#checkable_urls`. In this scenario, `stable` and `url` are the first checkable URLs for formulae and casks (respectively), so the checks ended up working as expected merely by chance. This isn't obvious in the output and even the debug output looks normal. It only becomes apparent that livecheck isn't working as expected if it iterates through more than one checkable URL before reaching one that works (not the case in those instances). With that in mind, this adds an error when a `#url` symbol is used but it doesn't correspond to a checkable URL. This will account for both of the mentioned scenarios (invalid symbols and valid ones referencing a non-existent URL) and prevent livecheck from quietly proceeding in an unexpected manner. --- Library/Homebrew/livecheck/livecheck.rb | 12 +++-- .../Homebrew/test/livecheck/livecheck_spec.rb | 52 +++++++++++++++++-- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/livecheck/livecheck.rb b/Library/Homebrew/livecheck/livecheck.rb index dc89b367e57f2..ac20b4c74cfcc 100644 --- a/Library/Homebrew/livecheck/livecheck.rb +++ b/Library/Homebrew/livecheck/livecheck.rb @@ -508,10 +508,10 @@ def self.status_hash(package_or_resource, status_str, messages = nil, full_name: params( livecheck_url: T.any(String, Symbol), package_or_resource: T.any(Formula, Cask::Cask, Resource), - ).returns(T.nilable(String)) + ).returns(String) } def self.livecheck_url_to_string(livecheck_url, package_or_resource) - case livecheck_url + livecheck_url_string = case livecheck_url when String livecheck_url when :url @@ -521,6 +521,12 @@ def self.livecheck_url_to_string(livecheck_url, package_or_resource) when :homepage package_or_resource.homepage unless package_or_resource.is_a?(Resource) end + + if livecheck_url.is_a?(Symbol) && !livecheck_url_string + raise ArgumentError, "`url #{livecheck_url.inspect}` does not reference a checkable URL" + end + + livecheck_url_string end # Returns an Array containing the formula/cask/resource URLs that can be used by livecheck. @@ -846,7 +852,7 @@ def self.resource_version( livecheck_strategy = livecheck.strategy livecheck_strategy_block = livecheck.strategy_block - livecheck_url_string = livecheck_url_to_string(livecheck_url, resource) + livecheck_url_string = livecheck_url_to_string(livecheck_url, resource) if livecheck_url urls = [livecheck_url_string] if livecheck_url_string urls ||= checkable_urls(resource) diff --git a/Library/Homebrew/test/livecheck/livecheck_spec.rb b/Library/Homebrew/test/livecheck/livecheck_spec.rb index aa489b711b3b6..775a3f467f1a7 100644 --- a/Library/Homebrew/test/livecheck/livecheck_spec.rb +++ b/Library/Homebrew/test/livecheck/livecheck_spec.rb @@ -134,6 +134,15 @@ end end + let(:f_stable_url_only) do + stable_url_s = stable_url + + formula("test_stable_url_only") do + desc "Test formula with only a stable URL" + url stable_url_s + end + end + let(:r_livecheck_url) { f_livecheck_url.resources.first } let(:c_livecheck_url) do @@ -149,6 +158,17 @@ RUBY end + let(:c_no_checkable_urls) do + Cask::CaskLoader.load(+<<-RUBY) + cask "test_no_checkable_urls" do + version "1.2.3" + + name "Test" + desc "Test cask with no checkable URLs" + end + RUBY + end + it "returns a URL string when given a livecheck_url string for a formula" do expect(livecheck.livecheck_url_to_string(livecheck_url, f_livecheck_url)).to eq(livecheck_url) end @@ -167,9 +187,35 @@ end it "returns nil when not given a string or valid symbol" do - expect(livecheck.livecheck_url_to_string(:invalid_symbol, f_livecheck_url)).to be_nil - expect(livecheck.livecheck_url_to_string(:invalid_symbol, c_livecheck_url)).to be_nil - expect(livecheck.livecheck_url_to_string(:invalid_symbol, r_livecheck_url)).to be_nil + error_text = "`url :%s` does not reference a checkable URL" + + # Invalid symbol in any context + expect { livecheck.livecheck_url_to_string(:invalid_symbol, f_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :invalid_symbol)) + expect { livecheck.livecheck_url_to_string(:invalid_symbol, c_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :invalid_symbol)) + expect { livecheck.livecheck_url_to_string(:invalid_symbol, r_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :invalid_symbol)) + + # Valid symbol in provided context but referenced URL is not present + expect { livecheck.livecheck_url_to_string(:head, f_stable_url_only) } + .to raise_error(ArgumentError, format(error_text, symbol: :head)) + expect { livecheck.livecheck_url_to_string(:homepage, f_stable_url_only) } + .to raise_error(ArgumentError, format(error_text, symbol: :homepage)) + expect { livecheck.livecheck_url_to_string(:homepage, c_no_checkable_urls) } + .to raise_error(ArgumentError, format(error_text, symbol: :homepage)) + expect { livecheck.livecheck_url_to_string(:url, c_no_checkable_urls) } + .to raise_error(ArgumentError, format(error_text, symbol: :url)) + + # Valid symbol but not in the provided context + expect { livecheck.livecheck_url_to_string(:head, c_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :head)) + expect { livecheck.livecheck_url_to_string(:homepage, r_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :homepage)) + expect { livecheck.livecheck_url_to_string(:stable, c_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :stable)) + expect { livecheck.livecheck_url_to_string(:url, f_livecheck_url) } + .to raise_error(ArgumentError, format(error_text, symbol: :url)) end end From bfdb84f67629902d41ef6354e91b368f6cfb4292 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Thu, 24 Oct 2024 22:19:18 -0400 Subject: [PATCH 2/2] Expand #checkable_urls test coverage This expands tests for `#checkable_urls` to cover everything except branches that shouldn't ever be reached. --- .../Homebrew/test/livecheck/livecheck_spec.rb | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/Library/Homebrew/test/livecheck/livecheck_spec.rb b/Library/Homebrew/test/livecheck/livecheck_spec.rb index 775a3f467f1a7..ecd5a0902d6bd 100644 --- a/Library/Homebrew/test/livecheck/livecheck_spec.rb +++ b/Library/Homebrew/test/livecheck/livecheck_spec.rb @@ -36,6 +36,15 @@ end end + let(:f_stable_url_only) do + stable_url_s = stable_url + + formula("test_stable_url_only") do + desc "Test formula with only a stable URL" + url stable_url_s + end + end + let(:r) { f.resources.first } let(:c) do @@ -56,6 +65,17 @@ RUBY end + let(:c_no_checkable_urls) do + Cask::CaskLoader.load(+<<-RUBY) + cask "test_no_checkable_urls" do + version "1.2.3" + + name "Test" + desc "Test cask with no checkable URLs" + end + RUBY + end + describe "::resolve_livecheck_reference" do context "when a formula/cask has a livecheck block without formula/cask methods" do it "returns [nil, []]" do @@ -134,15 +154,6 @@ end end - let(:f_stable_url_only) do - stable_url_s = stable_url - - formula("test_stable_url_only") do - desc "Test formula with only a stable URL" - url stable_url_s - end - end - let(:r_livecheck_url) { f_livecheck_url.resources.first } let(:c_livecheck_url) do @@ -158,17 +169,6 @@ RUBY end - let(:c_no_checkable_urls) do - Cask::CaskLoader.load(+<<-RUBY) - cask "test_no_checkable_urls" do - version "1.2.3" - - name "Test" - desc "Test cask with no checkable URLs" - end - RUBY - end - it "returns a URL string when given a livecheck_url string for a formula" do expect(livecheck.livecheck_url_to_string(livecheck_url, f_livecheck_url)).to eq(livecheck_url) end @@ -235,6 +235,8 @@ expect(livecheck.checkable_urls(c)).to eq([cask_url, homepage_url]) expect(livecheck.checkable_urls(r)).to eq([resource_url]) expect(livecheck.checkable_urls(f_duplicate_urls)).to eq([stable_url, head_url]) + expect(livecheck.checkable_urls(f_stable_url_only)).to eq([stable_url]) + expect(livecheck.checkable_urls(c_no_checkable_urls)).to eq([]) end end