Validate user input from init command #10457
Conversation
Reviewer's GuideA new validation step for user inputs in the Sequence diagram for user input validation in poetry init commandsequenceDiagram
actor User
participant InitCommand as poetry init
participant Factory
User->>InitCommand: Run poetry init and provide project info
InitCommand->>Factory: validate(pyproject_data)
Factory-->>InitCommand: Validation results
alt Validation errors
InitCommand-->>User: Show validation error and abort
else Validation passes
InitCommand->>InitCommand: Save pyproject.toml
InitCommand-->>User: Project initialized
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rbogart1990 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/init.py:548` </location>
<code_context>
return self._pool
+
+ @staticmethod
+ def _validate(pyproject_data: dict[str, Any]) -> dict[str, Any]:
+ """
+ Validates the given pyproject data and returns the validation results.
</code_context>
<issue_to_address>
Type annotation for dict[str, Any] may not be compatible with Python <3.9.
Use `Dict[str, Any]` from `typing` for compatibility with Python 3.8 and earlier.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
@staticmethod
def _validate(pyproject_data: dict[str, Any]) -> dict[str, Any]:
=======
from typing import Dict
@staticmethod
def _validate(pyproject_data: Dict[str, Any]) -> Dict[str, Any]:
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/poetry/console/commands/init.py
Outdated
| pyproject_dict = parse(pyproject.data.as_string()) | ||
| validation_results = self._validate(pyproject_dict) |
There was a problem hiding this comment.
Factory.validate(pyproject.data) should suffice.
There was a problem hiding this comment.
@radoering , thanks for the suggestion. I updated the code to call Factory.validate(pyproject.data) directly. All tests are passing!
I kept it inside of InitCommand()._validate() so that I can unit-test it.
There was a problem hiding this comment.
Hmm, I see. So we are actually just unit testing Factory.validate(), which does not give much value because it is (or at least should be) already tested in test_factory.py.
Further, my comment suggests that converting pyproject.data to string and parsing it is unnecessary. You probably also do this just for unit testing to have a clear interface? I do not like that the production code becomes more complicated just for unit testing.
I think you should check the other (non-interactive) unit tests in test_init.py and use one of these as base for your unit test so that you do not need a separate method to test. Further, this is probably the wrong place to test the regex in detail because as mentioned the validation is part of Factory. Here, we should just check that validation is called and gives a nicely formatted output.
src/poetry/console/commands/init.py
Outdated
| if validation_results.get("errors"): | ||
| self.line_error(f"<error>Validation failed: {validation_results}</error>") | ||
| return 1 |
There was a problem hiding this comment.
The output is not so nicely formatted. It contains a raw dict, e.g.:
Validation failed: {'errors': ['project.name must match pattern ^([a-zA-Z\\d]|[a-zA-Z\\d][\\w.-]*[a-zA-Z\\d])$'], 'warnings': []}
You may take a look at https://github.com/python-poetry/poetry-core/blob/002aa3e16f98d21645bb9a45f698b55adc40f317/src/poetry/core/factory.py#L53-L60 for better formatting.
There was a problem hiding this comment.
@radoering , thanks for the feedback about this.
Would this format be acceptable?
Validation failed:
- project.name must match pattern ^([a-zA-Z\d]|[a-zA-Z\d][\w.-]*[a-zA-Z\d])$
There was a problem hiding this comment.
@radoering , change made.
There was a problem hiding this comment.
You must be kidding. Error messages should be readable and clear for users. Imagine some beginner developer getting hit by that error message. Hell, 9/10 seasoned developers would have a hard time understanding that error message without some sort of regex explorer. This should be a clear message, that a 5 year old is able to understand. I would split the checks and make a separate clear message for each condition, gather the errors and list them clearly. Poetry's motto is "Python packaging and dependency management made easy". Getting hit with a regex is not "easy" by any means.
There was a problem hiding this comment.
My comment was just about formatting the output. At this point, we get a dict with lists of messages and the output should just be nicely formatted. The content of the single messages has been created before and is fixed at this point.
In my opinion, it is too much effort to improve each possible message that comes from schema validation - or at least this is clearly out of scope of this PR. The message is the same message if you run poetry check on such an invalid pyproject.toml.
This PR introduces validation for user input values during the
poetry initcommand. It uses the rules defined in the project-schema.json schema. These rules are already enforced when running thepoetry addcommand, so this change extends the same validation rules to thepoetry initcommand.Previously, user inputs were not validated against these schema rules, allowing the creation of Poetry projects with invalid or disallowed names (e.g. "new+project").
This change ensures that inputs conform to the schema before proceeding, preventing invalid project configurations.
Pull Request Check List
Resolves: #10170
Summary by Sourcery
Validate user inputs against the project schema in the 'poetry init' command and abort on validation errors.
New Features:
Enhancements:
Tests: