Validate package names in poetry update command#10721
Validate package names in poetry update command#10721veeceey wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's GuideAdds upfront validation for package names passed to Sequence diagram for updated poetry update validation flowsequenceDiagram
actor Developer
participant CLI as PoetryCLI
participant UpdateCommand
participant PoetryApp as Poetry
participant Installer
Developer->>CLI: poetry update pkg==1.2.0
CLI->>UpdateCommand: handle(packages=["pkg==1.2.0"])
UpdateCommand->>PoetryApp: poetry.package.all_requires
PoetryApp-->>UpdateCommand: dependencies
UpdateCommand->>UpdateCommand: build all_dependencies set
UpdateCommand->>UpdateCommand: extract package_name from pkg==1.2.0
UpdateCommand->>UpdateCommand: canonicalize_name(package_name)
UpdateCommand->>UpdateCommand: check membership in all_dependencies
alt package not a declared dependency
UpdateCommand->>Developer: line_error(The following packages are not dependencies...)
UpdateCommand-->>CLI: return 1
CLI-->>Developer: non_zero_exit_code
else package is a declared dependency
UpdateCommand->>Installer: whitelist({"pkg==1.2.0": "*"})
UpdateCommand->>Installer: only_groups(activated_groups)
Installer-->>CLI: perform update
CLI-->>Developer: success_exit_code
end
Updated class diagram for UpdateCommand and LockerclassDiagram
class Command
class InstallerCommand
class Installer
class Poetry
class Package
class Locker
Command <|-- InstallerCommand
InstallerCommand <|-- UpdateCommand
class UpdateCommand {
+handle() int
-installer Installer
-poetry Poetry
}
class Poetry {
+package Package
}
class Package {
+all_requires list
}
UpdateCommand --> Installer : uses
UpdateCommand --> Poetry : uses
Poetry --> Package : has
class DependencyConstraint
class Locker {
+_dump_package(package Package, target str, dependency_category str, optional bool, with_extras bool) dict
}
Locker --> Package : serializes
Locker --> DependencyConstraint : sorts constraints
class SortedConstraintsBehavior {
+markers str
+optional bool
+version str
}
DependencyConstraint --> SortedConstraintsBehavior : compared_by
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The package name extraction in
UpdateCommand.handle(chainedspliton>,<,=,!) is quite brittle and will likely misbehave on more complex specs (extras, spaces,~,^, URLs, environment markers, etc.); consider delegating to Poetry’s existing requirement/dependency parsing utilities so the validation logic stays aligned with how the installer interprets the arguments. - Since you are canonicalizing dependency names from
all_requires, it might be safer to also normalize the user-supplied package arguments in a single helper (shared with other commands if possible) so that validation behavior is consistent acrossupdate,add,remove, etc., and you don’t duplicate name parsing rules in multiple places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The package name extraction in `UpdateCommand.handle` (chained `split` on `>`, `<`, `=`, `!`) is quite brittle and will likely misbehave on more complex specs (extras, spaces, `~`, `^`, URLs, environment markers, etc.); consider delegating to Poetry’s existing requirement/dependency parsing utilities so the validation logic stays aligned with how the installer interprets the arguments.
- Since you are canonicalizing dependency names from `all_requires`, it might be safer to also normalize the user-supplied package arguments in a single helper (shared with other commands if possible) so that validation behavior is consistent across `update`, `add`, `remove`, etc., and you don’t duplicate name parsing rules in multiple places.
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/update.py:58` </location>
<code_context>
+ for package in packages:
+ # Check if package name contains version constraint
+ # (e.g., "package==1.0.0" or "package>=1.0")
+ package_name = package.split(">")[0].split("<")[0].split("=")[0].split("!")[0].strip()
+ canonical_name = canonicalize_name(package_name)
+
</code_context>
<issue_to_address>
**issue:** Package name parsing is brittle and may break for extras or more complex specifiers.
Chaining `split` calls to drop version constraints is fragile and will mis-handle valid specs, e.g. `foo[extra]==1.0` (becoming `foo[extra`) or `foo>=1,<2`, and will ignore environment markers. Instead of manual splitting on operators, use `packaging.requirements.Requirement` (or an existing helper in this codebase) to parse the requirement and extract the project name safely.
</issue_to_address>
### Comment 2
<location> `tests/console/commands/test_update.py:105-114` </location>
<code_context>
assert tester.command.installer._requires_synchronization
+
+
+def test_update_with_invalid_package_name_shows_error(
+ poetry_with_outdated_lockfile: Poetry,
+ command_tester_factory: CommandTesterFactory,
+) -> None:
+ """
+ Providing non-existent package names should raise an error.
+ """
+ tester = command_tester_factory("update", poetry=poetry_with_outdated_lockfile)
+
+ status = tester.execute("nonexistent-package")
+
+ assert status == 1
+ assert (
+ "The following packages are not dependencies of this project: nonexistent-package"
+ in tester.io.fetch_error()
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for a package with a version constraint (e.g. `pkg==1.0.0`) to ensure name extraction is covered
Since `UpdateCommand` now strips version constraints (`==`, `>=`, `<`, `!=`, etc.) before validating names, please add at least one test using a constrained specifier (e.g. `tester.execute("nonexistent==1.2.3")`, and optionally another with `>=` or a compound specifier) to verify that parsing works correctly and that the invalid package still appears in the error message.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
you have made a mess of your branches, this pull request is mixed up with changes from another |
ba4acd4 to
8321399
Compare
|
Fixed - rebased the branch so it only contains the changes for this PR. Sorry about the branch mix-up. |
|
Tested with various package specs (extras, version constraints, URLs, environment markers). Parser correctly extracts package names in all cases. All 8 tests in test_update.py pass. |
|
|
||
| invalid_packages = [] | ||
| for package in packages: | ||
| package_name = _extract_package_name(package) |
There was a problem hiding this comment.
I do not think extracting the package name and ignoring the rest makes sense. The interface of update is to pass package names. When passing a requirement string instead, the command should probably fail because not respecting the version constraint is unexpected.
There was a problem hiding this comment.
You're right, that makes more sense. I've removed the name extraction logic entirely - the command now just validates the argument as-is via canonicalize_name. So passing something like poetry update "pkg==1.0.0" will correctly fail with an error instead of silently stripping the version specifier. Pushed the simplified version.
|
That's a fair point - silently ignoring the version constraint would definitely be confusing for users. Would it make more sense to detect when a requirement string (with version specifiers) is passed and raise a clear error message like 'poetry update expects package names, not requirement strings. Use poetry add to specify version constraints'? Happy to rework the PR in that direction if you think that's the right approach, or close this if you'd rather handle it differently. |
Yes, it makes sense to me. |
|
Implemented the rework — now detects version specifiers in package arguments and raises a clear error pointing users to |
Detect version specifiers in package arguments and raise a clear error pointing users to `poetry add` instead. Also validate that all specified packages are declared dependencies of the project. Fixes python-poetry#10422
6716d4f to
623170a
Compare
|
Hey @radoering, I've pushed the reworked implementation as discussed — it now detects version specifiers and raises a clear error pointing users to poetry add, rather than silently stripping constraints. Would love your review on the updated approach when you have a chance! |
Summary
Fixes #10422
This PR adds validation to the
poetry updatecommand to ensure that all specified package names are actually declared dependencies of the project.Problem
Previously,
poetry updatewould silently ignore any package names that didn't exist in the project dependencies, making it impossible to detect typos or mistakes. This was particularly problematic when trying to update to a specific version (e.g.,poetry update "package==0.2.0") as the command would succeed without doing anything.Changes
Testing
Summary by Sourcery
Validate packages passed to the
poetry updatecommand and make lockfile dependency output deterministic.Bug Fixes:
poetry updatewith a clear error when given package names that are not declared dependencies of the project, including names with version constraints.Enhancements:
Tests:
poetry updateexits with status 1 and shows an appropriate error message when given single or multiple invalid package names.