Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Parse multiple requirements from a poetry dependency #4179

Merged
merged 8 commits into from
Sep 7, 2021
Merged

fix: Parse multiple requirements from a poetry dependency #4179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c4f64a0
test: add test to reproduce multiple version defect
xlgmokha Aug 30, 2021
ea1c9de
test: ensure parser does not raise an error
xlgmokha Aug 30, 2021
0ab9fd4
fix: loop through each poetry requirement
xlgmokha Aug 30, 2021
0043c93
test: assert the expected packages are parsed
xlgmokha Aug 30, 2021
5dd5c83
refactor: extract method to parse requirement
xlgmokha Aug 30, 2021
3e38e1a
fix: use array literal to prevent converting a Hash to an array of tu…
xlgmokha Aug 30, 2021
d6e740e
style: fix linter errors
xlgmokha Aug 30, 2021
69e9e86
fix: exclude dependencies that do not have at least one supported req…
xlgmokha Sep 1, 2021
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
42 changes: 26 additions & 16 deletions python/lib/dependabot/python/file_parser/poetry_files_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,28 +43,38 @@ def pyproject_dependencies

deps_hash.each do |name, req|
next if normalise(name) == "python"
next if req.is_a?(Hash) && UNSUPPORTED_DEPENDENCY_TYPES.any? { |t| req.key?(t) }

check_requirements(req)

dependencies <<
Dependency.new(
name: normalise(name),
version: version_from_lockfile(name),
requirements: [{
requirement: req.is_a?(String) ? req : req["version"],
file: pyproject.name,
source: nil,
groups: [type]
}],
package_manager: "pip"
)

requirements = parse_requirements_from(req, type)
next if requirements.empty?

dependencies << Dependency.new(
name: normalise(name),
version: version_from_lockfile(name),
requirements: requirements,
package_manager: "pip"
)
end
end

dependencies
end

# @param req can be an Array, Hash or String that represents the constraints for a dependency
def parse_requirements_from(req, type)
[req].flatten.compact.map do |requirement|
next if requirement.is_a?(Hash) && (UNSUPPORTED_DEPENDENCY_TYPES & requirement.keys).any?
Copy link
Member

Choose a reason for hiding this comment

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

Just double-checking, this is functionally the same as next if req.is_a?(Hash) && UNSUPPORTED_DEPENDENCY_TYPES.any? { |t| req.key?(t) } right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. It's a set intersect operation.

irb(main):001:0> [1, 2, 3] & [2, 3]
=> [2, 3]
irb(main):002:0> [1, 3, 4] & [5, 6]
=> []


check_requirements(requirement)

{
requirement: requirement.is_a?(String) ? requirement : requirement["version"],
Copy link
Member

@jurre jurre Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Slightly paranoid that we might get something that's not a String but also does not respond to #[] in a way that we'd expect but since no tests break that might not be a concern. Should we handle the type of req more explicitly?

Totally fine to keep as is, just thinking out loud here, but I found the [req].flatten.compact.map do |requirement| a little hard to follow 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly paranoid that we might get something that's not a String but also does not respond to #[] in a way that we'd expect but since no tests break that might not be a concern.

I think this is a valid concern. Matching on type signature feels icky to me. I feel like there's a missing abstraction/interface here but I'm not sure what that is.

Should we handle the type of req more explicitly?

I'm not against this idea. I worked on this issue during my on-call shift last week so I've lost some of the context due to mo's limited memory but I can circle back to this. I think I would prefer to ship this small change as is because there were some jobs failing with this issue so this would at least unblock those. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

WDYT?

👍

file: pyproject.name,
source: nil,
groups: [type]
}
end.compact
end

# Create a DependencySet where each element has no requirement. Any
# requirements will be added when combining the DependencySet with
# other DependencySets.
Expand Down
9 changes: 9 additions & 0 deletions python/spec/dependabot/python/file_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,5 +1315,14 @@
expect { dependencies }.to raise_error(Dependabot::UnexpectedExternalCode)
end
end

context "with multiple requirements" do
let(:files) { project_dependency_files("poetry/multiple_requirements") }

it "returns the dependencies with multiple requirements" do
expect { dependencies }.not_to raise_error
expect(dependencies.map(&:name)).to contain_exactly("numpy", "scipy")
end
end
end
end
154 changes: 154 additions & 0 deletions python/spec/fixtures/projects/poetry/multiple_requirements/poetry.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions python/spec/fixtures/projects/poetry/multiple_requirements/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[tool.poetry]
name = "example"
version = "0.1.0"
description = ""
authors = []

[tool.poetry.dependencies]
numpy = [
{ version = "^1.19", python = "^3.6,<3.7" },
{ version = "^1.20", python = "^3.7" }]
scipy = [
{ version = "^1.5", python = "^3.6,<3.7" },
{ version = "^1.6.0", python = "^3.7" }
]