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.

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

Support Poetry 1.5.1 poetry.lock with absence of category key #7418

Closed
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -156,17 +156,38 @@ def lock_declaration_to_new_version!(poetry_object, dep)
end

def create_declaration_at_new_version!(poetry_object, dep)
poetry_object[subdep_type] ||= {}
poetry_object[subdep_type][dependency.name] = dep.version
type = subdep_type(poetry_object, dep)
poetry_object[type] ||= {}
poetry_object[type][dependency.name] = dep.version
end

def subdep_type
category =
TomlRB.parse(lockfile.content).fetch("package", []).
find { |dets| normalise(dets.fetch("name")) == dependency.name }.
fetch("category")
def subdep_type(poetry_object, dep)
locked_dep = TomlRB.parse(lockfile.content).fetch("package", []).
find { |dets| normalise(dets.fetch("name")) == dependency.name }

category == "dev" ? "dev-dependencies" : "dependencies"
category = locked_dep.fetch("category", nil)
# Simple case < Poetry 1.5
return category == "dev" ? "dev-dependencies" : "dependencies" if category

# No category so Poetry > 1.5, find the declaration from pyproject.toml
in_main = poetry_object.fetch("dependencies", []).find(dep.name)
return "dependencies" if in_main

in_legacy_dev = poetry_object.fetch("dev-dependencies", []).find(dep.name)
return "dev-dependencies" if in_legacy_dev

groups = poetry_object.fetch("group", {})
groups.each do |_group_name, group_spec|
in_group = group_spec["dependencies"].find(dep.name)
# All group dependencies get treated the same, as "dev" dependencies
return "dev-dependencies" if in_group
end

# Did not return before this, must not have found it in pyproject.toml. This means it must be a transitive
# dependency. Default all transitive dependencies to main dependencies for now.
# Future TODO: go in reverse order looking at all the packages that declare this dependency as a dependency,
# checking each time for existence in pyproject.toml to determine the category
"dependencies"
end

def sanitize(pyproject_content)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,12 +295,32 @@ def update_dependency_requirement(toml_node, requirement)
end

def subdep_type
Copy link
Contributor Author

@phillipuniverse phillipuniverse Aug 9, 2023

Choose a reason for hiding this comment

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

I just happened to find this function unexpectedly searching around for subdep_type. Looks like this logic was always shared with the file updater, but it was small enough such that it made more sense to copy it vs extract into a utility function.

Since the logic for determining a "subdep_type" is getting larger, maybe it makes sense to refactor to a separate function? Another set of options:

  1. Refactor into a utility function and call from both places. Opinion on where this should go?
  2. Remove this modification since it's not specifically tackling the core issue and we are trying to be pretty targeted per other conversations
  3. Keep as-is

Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to pass poetry_object around, and a real update seems to be still failing because of that. So this needs to be fixed before merging, and also signals some lack of coverage here.

I'll look into a good place for factoring out this method.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez Aug 17, 2023

Choose a reason for hiding this comment

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

I proposed #7826 to refactor this. We can rebase and adapt this PR to it, and should be all good.

category =
TomlRB.parse(lockfile.content).fetch("package", []).
find { |dets| normalise(dets.fetch("name")) == dependency.name }.
fetch("category")
locked_dep = TomlRB.parse(lockfile.content).fetch("package", []).
find { |dets| normalise(dets.fetch("name")) == dependency.name }

category == "dev" ? "dev-dependencies" : "dependencies"
category = locked_dep.fetch("category", nil)
# Simple case < Poetry 1.5
return category == "dev" ? "dev-dependencies" : "dependencies" if category

# No category so Poetry > 1.5, find the declaration from pyproject.toml
in_main = poetry_object.fetch("dependencies", []).find(dep.name)
return "dependencies" if in_main

in_legacy_dev = poetry_object.fetch("dev-dependencies", []).find(dep.name)
return "dev-dependencies" if in_legacy_dev

groups = poetry_object.fetch("group", {})
groups.each do |_group_name, group_spec|
in_group = group_spec["dependencies"].find(dep.name)
# All group dependencies get treated the same, as "dev" dependencies
return "dev-dependencies" if in_group
end

# Did not return before this, must not have found it in pyproject.toml. This means it must be a transitive
# dependency. Default all transitive dependencies to main dependencies for now.
# Future TODO: go in reverse order looking at all the packages that declare this dependency as a dependency,
# checking each time for existence in pyproject.toml to determine the category
"dependencies"
end

def python_requirement_parser
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@

its(:length) { is_expected.to eq(36) }

describe "a development sub-dependency" do
describe "a pre Poetry 1.5 development sub-dependency" do
subject(:dep) { dependencies.find { |d| d.name == "atomicwrites" } }

its(:subdependency_metadata) do
Expand Down Expand Up @@ -227,6 +227,34 @@
expect(dependency.requirements).to eq([])
end
end

context "with Poetry 1.5 locked group dependencies" do
# This is how Poetry treats transitive dev dependencies, see discussion at https://github.com/python-poetry/poetry/pull/7637#issuecomment-1494272266
# and https://github.com/dependabot/dependabot-core/pull/7418#issuecomment-1644012926
let(:pyproject_fixture_name) { "poetry_group_dependencies.toml" }
let(:pyproject_lock_fixture_name) { "poetry_group_dependencies.lock" }

describe "a legacy dev-dependencies sub-dependency" do
subject(:dep) { dependencies.find { |d| d.name == "click" } }
its(:subdependency_metadata) do
is_expected.to eq([{ production: true }])
end
end

describe "a dev group sub-dependency" do
subject(:dep) { dependencies.find { |d| d.name == "pluggy" } }
its(:subdependency_metadata) do
is_expected.to eq([{ production: true }])
end
end

describe "a non-dev group sub-dependency" do
subject(:dep) { dependencies.find { |d| d.name == "sphinxcontrib-applehelp" } }
its(:subdependency_metadata) do
is_expected.to eq([{ production: true }])
end
end
end
end

context "with group dependencies" do
Expand Down
Loading