Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 48 additions & 38 deletions common/lib/dependabot/git_commit_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,48 +100,36 @@ def head_commit_for_local_branch(name)
local_repo_git_metadata_fetcher.head_commit_for_ref(name)
end

def local_tags_for_latest_version_commit_sha
tags = allowed_version_tags
max_tag = max_version_tag(tags)
def local_tag_for_latest_version_matching_existing_precision
max_tag = max_version_tag_for_current_precision(allowed_version_tags)

return [] unless max_tag
return unless max_tag

tags.
select { |t| t.commit_sha == max_tag.commit_sha }.
map do |t|
version = t.name.match(VERSION_REGEX).named_captures.fetch("version")
{
tag: t.name,
version: version_class.new(version),
commit_sha: t.commit_sha,
tag_sha: t.tag_sha
}
end
to_local_tag(max_tag)
end

def local_tag_for_latest_version
tag = max_version_tag(allowed_version_tags)
max_tag = max_version_tag(allowed_version_tags)

return unless tag
return unless max_tag

version = tag.name.match(VERSION_REGEX).named_captures.fetch("version")
{
tag: tag.name,
version: version_class.new(version),
commit_sha: tag.commit_sha,
tag_sha: tag.tag_sha
}
to_local_tag(max_tag)
end

def max_version_tag(tags)
tags.
max_by do |t|
version = t.name.match(VERSION_REGEX).named_captures.
fetch("version")
version_class.new(version)
version_from_tag(t)
end
end

def max_version_tag_for_current_precision(tags)
current_precision = precision(dependency.version)

# Find the latest version with the same precision as the pinned version.
max_version_tag(tags.select { |tag| precision(scan_version(tag.name)) == current_precision })
end

def allowed_version_tags
tags =
local_tags.
Expand All @@ -159,16 +147,14 @@ def allowed_version_tags
def current_version
return unless dependency.version && version_tag?(dependency.version)

version = dependency.version.match(VERSION_REGEX).named_captures.fetch("version")
version_class.new(version)
version_from_ref(dependency.version)
end

def filter_lower_versions(tags)
return tags unless current_version

versions = tags.map do |t|
version = t.name.match(VERSION_REGEX).named_captures.fetch("version")
version_class.new(version)
version_from_tag(t)
end

versions.select do |version|
Expand Down Expand Up @@ -198,6 +184,10 @@ def git_repo_reachable?

attr_reader :dependency, :credentials, :ignored_versions

def precision(version)
version.split(".").length
end

def pinned_ref_in_release?(version)
raise "Not a git dependency!" unless git_dependency?

Expand Down Expand Up @@ -341,6 +331,16 @@ def matches_existing_prefix?(tag)
tag.gsub(VERSION_REGEX, "").gsub(/v$/i, "")
end

def to_local_tag(tag)
version = version_from_tag(tag)
{
tag: tag.name,
version: version,
commit_sha: tag.commit_sha,
tag_sha: tag.tag_sha
}
end

def listing_source_url
@listing_source_url ||=
begin
Expand Down Expand Up @@ -404,19 +404,29 @@ def wants_prerelease?
return false unless dependency_source_details&.fetch(:ref, nil)
return false unless pinned_ref_looks_like_version?

version = dependency_source_details.fetch(:ref).match(VERSION_REGEX).
named_captures.fetch("version")
version_class.new(version).prerelease?
version = version_from_ref(dependency_source_details.fetch(:ref))
version.prerelease?
end

def tag_included_in_ignore_requirements?(tag)
version = tag.name.match(VERSION_REGEX).named_captures.fetch("version")
ignore_requirements.any? { |r| r.satisfied_by?(version_class.new(version)) }
version = version_from_tag(tag)
ignore_requirements.any? { |r| r.satisfied_by?(version) }
end

def tag_is_prerelease?(tag)
version = tag.name.match(VERSION_REGEX).named_captures.fetch("version")
version_class.new(version).prerelease?
version_from_tag(tag).prerelease?
end

def version_from_tag(tag)
version_from_ref(tag.name)
end

def version_from_ref(name)
version_class.new(scan_version(name))
end

def scan_version(name)
name.match(VERSION_REGEX).named_captures.fetch("version")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can raise an exception if the name doesn't match VERSION_REGEX. Would it make sense to use safe navigation here to return nil if the name doesn't match?

Suggested change
name.match(VERSION_REGEX).named_captures.fetch("version")
name.match(VERSION_REGEX)&.named_captures&.fetch("version")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the call sites, you may need to chain additional calls to compact to handle any returned nil values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an extraction of currently existing duplicated logic. I'm not sure if this can be actually fed names that don't match or if they are validated further up the stack.

My empirical approach would be that the check is not needed, since I don't think I have seen that error in the wild or reports pointing to it. But in any case seems unrelated to this PR, since the aggressive code is already there, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further thinking about this, while I extracted existing instances of this logic to a method, I am indeed adding a new call site, so worth checking wether the case you mentioned can happen and add defensive code if yes. I will check this in more detail later 👍.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, all these helpers deal with "version tags" (the allowed_version_tags method specifically), which are essentially git tags that look like versions, i.e., have been filtered through this method:

https://github.com/dependabot/dependabot-core/blob/fbfca13459c89f0664807452ab207bfc2114f287/common/lib/dependabot/git_commit_checker.rb#L323-L325

On further iterations I will try to make this more clear, but I think this code is safe as is 👍.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattt I'll merge this now based on my above comments, but if you still think there's something to address I'll open a followup PR!

end

def version_class
Expand Down
52 changes: 33 additions & 19 deletions common/spec/dependabot/git_commit_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1066,8 +1066,8 @@
end
end

describe "#local_tags_for_latest_version_commit_sha" do
subject { checker.local_tags_for_latest_version_commit_sha }
describe "#local_tag_for_latest_version_matching_existing_precision" do
subject { checker.local_tag_for_latest_version_matching_existing_precision }
let(:repo_url) { "https://github.com/gocardless/business.git" }
let(:service_pack_url) { repo_url + "/info/refs?service=git-upload-pack" }
before do
Expand All @@ -1083,7 +1083,7 @@
let(:upload_pack_fixture) { "no_tags" }

context "with no tags on GitHub" do
it { is_expected.to eq([]) }
it { is_expected.to be_nil }
end

context "but GitHub returns a 404" do
Expand All @@ -1098,35 +1098,49 @@
end

it "raises a helpful error" do
expect { checker.local_tags_for_latest_version_commit_sha }.
expect { checker.local_tag_for_latest_version_matching_existing_precision }.
to raise_error(Dependabot::GitDependenciesNotReachable)
end
end

context "with tags on GitHub" do
context "but no version tags" do
let(:upload_pack_fixture) { "no_versions" }
it { is_expected.to eq([]) }
it { is_expected.to be_nil }
end

context "with version tags" do
let(:upload_pack_fixture) { "actions-checkout" }
let(:tags) do
[{
commit_sha: "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f",
tag: "v2",
tag_sha: anything,
version: anything
},
{
commit_sha: "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f",
tag: "v2.3.4",
tag_sha: anything,
version: anything
}]

context "and current precision of major" do
let(:version) { "1" }

let(:latest_major) do
{
commit_sha: "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f",
tag: "v2",
tag_sha: anything,
version: anything
}
end

it { is_expected.to match(latest_major) }
end

it { is_expected.to match_array(tags) }
context "current precision of patch" do
let(:version) { "2.1.1" }

let(:latest_patch) do
{
commit_sha: "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f",
tag: "v2.3.4",
tag_sha: anything,
version: anything
}
end

it { is_expected.to match(latest_patch) }
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,10 @@ def latest_version_tag
@latest_version_tag ||= begin
return git_commit_checker.local_tag_for_latest_version if dependency.version.nil?

latest_tags = git_commit_checker.local_tags_for_latest_version_commit_sha

# Find the latest version with the same precision as the pinned version.
current_precision = precision(dependency.version)
latest_tags.select { |tag| precision(tag[:version].to_s) == current_precision }.max_by { |tag| tag[:version] }
git_commit_checker.local_tag_for_latest_version_matching_existing_precision
end
end

def precision(version)
version.split(".").length
end

def updated_source
# TODO: Support Docker sources
return dependency_source_details unless git_dependency?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,54 +284,22 @@
end
end

context "given a dependency with a tag reference when an update with the same precision is not available" do
let(:latest_versions) { [] }
context "given a repo when the latest major does not point to the latest patch" do
let(:upload_pack_fixture) { "cache" }

before do
version_tags = latest_versions.map do |v|
{
tag: "v#{v}",
version: Dependabot::GithubActions::Version.new(v)
}
end

checker.instance_variable_set(:@git_commit_checker, git_commit_checker)
allow(git_commit_checker).to receive(:local_tags_for_latest_version_commit_sha).and_return(version_tags)
end

context "using the major version" do
let(:reference) { "v1" }
let(:latest_versions) { ["2.1", "2.1.0"] }
context "and pinned to patch" do
let(:reference) { "v2.1.3" }

it "does not choose a version with different precision" do
expect(subject).to be_nil
it "updates to the latest patch" do
expect(subject).to eq(Dependabot::GithubActions::Version.new("3.0.11"))
end
end

context "using the major minor version" do
let(:reference) { "v1.0" }
let(:latest_versions) { ["2", "2.1.0"] }

it "does not choose a version with different precision" do
expect(subject).to be_nil
end
end

context "using the full version" do
let(:reference) { "v1.0.0" }
let(:latest_versions) { ["2", "2.1"] }

it "does not choose a version with different precision" do
expect(subject).to be_nil
end
end

context "using the full version" do
let(:reference) { "v1.0.0" }
let(:latest_versions) { ["1.0.5", "2", "2.1"] }
context "and pinned to major" do
let(:reference) { "v2" }

it "chooses a higher version with the same precision" do
expect(subject).to eq(Dependabot::GithubActions::Version.new("1.0.5"))
it "updates to the latest major" do
expect(subject).to eq(Dependabot::GithubActions::Version.new("3"))
end
end
end
Expand Down
Binary file not shown.