fix: add support for pyproject.toml and env.yml/env.yaml files #550
Conversation
|
Please note that this PR in the safety_schemas repo is needed for this to work: https://github.com/pyupio/safety_schemas/pull/1 |
WalkthroughThe changes introduce support for handling Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileFinder
participant PyProjectTomlHandler
participant DependencyParser
User->>FileFinder: Scan directory
FileFinder->>PyProjectTomlHandler: Process pyproject.toml
PyProjectTomlHandler->>DependencyParser: Extract dependencies
DependencyParser-->>PyProjectTomlHandler: Return dependencies
PyProjectTomlHandler-->>FileFinder: Provide dependency info
FileFinder-->>User: Display scan results
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- pyproject.toml (1 hunks)
- safety/scan/command.py (2 hunks)
- safety/scan/ecosystems/python/dependencies.py (3 hunks)
- safety/scan/finder/file_finder.py (3 hunks)
- safety/scan/finder/handlers.py (3 hunks)
Additional context used
Ruff
safety/scan/finder/handlers.py
5-5:
typing.Tupleimported but unusedRemove unused import:
typing.Tuple(F401)
safety/scan/finder/file_finder.py
11-11:
.handlers.PyProjectTomlHandlerimported but unusedRemove unused import:
.handlers.PyProjectTomlHandler(F401)
161-161: Use
enumerate()for index variablelevelinforloop(SIM113)
Additional comments not posted (9)
safety/scan/finder/handlers.py (4)
112-113: LGTM!The empty implementation of the
download_required_assetsmethod is appropriate since there are no required assets to download for Safety project files, as indicated by the docstring.
116-162: LGTM!The implementation of the
PyProjectTomlHandlerclass looks good:
- It correctly extends the
FileHandlerclass and sets the ecosystem toEcosystem.PYPROJECT_TOML.- The
download_required_assetsmethod fetches both the full and partial Safety databases, consistent with thePythonFileHandlerclass.- The
can_handlemethod correctly checks for thepyproject.tomlfile name and returns the correspondingFileType.- The
handlemethod reads thepyproject.tomlfile using thetomllibrary and extracts dependencies from the relevant sections.- The extracted dependencies are returned as a set, which is an appropriate data structure for unique dependencies.
Great job!
177-177: LGTM!Adding the
PyProjectTomlHandlerto theECOSYSTEM_HANDLER_MAPPINGdictionary is necessary and correctly implemented. This associates the handler with the correspondingEcosystem.PYPROJECT_TOMLecosystem.
Line range hint
1-179: Overall, the changes in this file look great!The introduction of the
PyProjectTomlHandlerclass is a valuable addition to support handlingpyproject.tomlfiles. The implementation is consistent with the existingFileHandlerclasses and follows the necessary conventions. The class correctly extends theFileHandlerclass and implements the required methods such asdownload_required_assets,can_handle, andhandleappropriately.The integration of the
PyProjectTomlHandlerwith the existing codebase is smooth, and the class is correctly added to theECOSYSTEM_HANDLER_MAPPINGdictionary to associate it with the corresponding ecosystem.Apart from the minor issues mentioned in the previous comments, the overall changes in this file are well-structured and contribute positively to the project's functionality.
Great work!
Tools
Ruff
167-167: Redefinition of unused
download_required_assetsfrom line 121(F811)
safety/scan/finder/file_finder.py (1)
153-160: LGTM!The new code block correctly handles the special files
pyproject.toml,env.yml, andenv.yamlby adding them to thefilesdictionary under their respective file types. This enhances the functionality of the file finder to support these important configuration files.safety/scan/ecosystems/python/dependencies.py (2)
273-308: LGTM!The
read_pyproject_toml_dependenciesfunction correctly reads dependencies from apyproject.tomlfile and yieldsPythonDependencyobjects. It handles both versioned and unversioned dependencies in the relevant sections of the TOML file.The implementation integrates well with the existing
PythonDependencymodel and thePythonSpecificationclass, enabling support forpyproject.tomlfiles in the project.
330-331: LGTM!The changes to the
get_dependenciesfunction correctly integrate the handling ofFileType.PYPROJECT_TOMLfiles by calling the newly introducedread_pyproject_toml_dependenciesfunction.This modification ensures that dependencies from
pyproject.tomlfiles are properly processed and returned asPythonDependencyobjects, seamlessly integrating with the existing dependency resolution logic.safety/scan/command.py (2)
48-48: LGTM!The new member
PYPROJECT_TOMLis added correctly to theScannableEcosystemsenumeration to support scanning projects defined by apyproject.tomlfile.
Line range hint
1-1: Skipped reviewingsystem_scanfunction.The
system_scanfunction is not changed in the provided code, so there is nothing to review.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
safety/scan/finder/handlers.py (3)
112-113: Consider providing a meaningful implementation or documentation.In the
SafetyProjectFileHandler, thedownload_required_assetsmethod currently contains only apassstatement. If no assets are required, it's a good practice to document this explicitly.Consider adding a docstring:
def download_required_assets(self, session): + """ + No required assets to download for Safety project files. + """ pass
138-139: Remove debug print statement incan_handlemethod.The
print("recognized")statement is likely used for debugging purposes. Consider removing it or replacing it with appropriate logging.Apply this diff to remove the print statement:
def can_handle(self, root: str, file_name: str, include_files: Dict[FileType, List[Path]]) -> Optional[FileType]: if file_name == 'pyproject.toml': - print("recognized") return FileType.PYPROJECT_TOML return None
146-146: Remove debug print statement inhandlemethod.The
print("printing data", data)statement may not be suitable for production code. Consider removing it or using a logging framework.Apply this diff to remove the print statement:
with open(file_path, 'r') as file: data = toml.load(file) - print("printing data", data) dependencies = set()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- safety/scan/finder/handlers.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
safety/scan/finder/handlers.py (2)
55-55:⚠️ Potential issueCorrect the use of
NotImplementedErrorin abstract method.In the
FileHandlerclass, thedownload_required_assetsmethod should raiseNotImplementedErrorinstead of returning it. This ensures that subclasses are properly notified to implement this method.Apply this diff to fix the issue:
def download_required_assets(self, session): """ Abstract method to download required assets for handling files. Should be implemented by subclasses. Args: session: The session object for making network requests. - Returns: - Dict[str, str]: A dictionary of downloaded assets. """ - return NotImplementedError(NOT_IMPLEMENTED) + raise NotImplementedError(NOT_IMPLEMENTED)Likely invalid or redundant comment.
5-5: Verify necessity of all imported types fromtypingmodule.Ensure that all imported types (
Dict,List,Optional,Set) are used in the code. Unused imports can be removed to keep the code clean.Run the following script to check for unused imports:
| # Mapping of ecosystems to their corresponding file handlers | ||
| ECOSYSTEM_HANDLER_MAPPING = MappingProxyType({ | ||
| Ecosystem.PYTHON: PythonFileHandler, | ||
| Ecosystem.SAFETY_PROJECT: SafetyProjectFileHandler, | ||
| # Ecosystem.PYPROJECT_TOML: PyProjectTomlHandler, |
There was a problem hiding this comment.
Uncomment PyProjectTomlHandler in the ecosystem handler mapping.
The PyProjectTomlHandler is commented out in ECOSYSTEM_HANDLER_MAPPING, which means it won't be used. To enable support for pyproject.toml files, uncomment the handler.
Apply this diff:
ECOSYSTEM_HANDLER_MAPPING = MappingProxyType({
Ecosystem.PYTHON: PythonFileHandler,
Ecosystem.SAFETY_PROJECT: SafetyProjectFileHandler,
- # Ecosystem.PYPROJECT_TOML: PyProjectTomlHandler,
+ Ecosystem.PYPROJECT_TOML: PyProjectTomlHandler,
})Committable suggestion was skipped due to low confidence.
| class PyProjectTomlHandler(FileHandler): | ||
| def __init__(self) -> None: | ||
| super().__init__() | ||
| self.ecosystem = Ecosystem.PYTHON | ||
|
|
||
| def download_required_assets(self, session): | ||
| from safety.safety import fetch_database | ||
|
|
||
| SAFETY_DB_DIR = os.getenv("SAFETY_DB_DIR") | ||
|
|
||
| db = False if SAFETY_DB_DIR is None else SAFETY_DB_DIR | ||
|
|
||
|
|
||
| fetch_database(session=session, full=False, db=db, cached=True, | ||
| telemetry=True, ecosystem=Ecosystem.PYTHON, | ||
| from_cache=False) | ||
|
|
||
| fetch_database(session=session, full=True, db=db, cached=True, | ||
| telemetry=True, ecosystem=Ecosystem.PYTHON, | ||
| from_cache=False) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid code duplication in download_required_assets method.
The download_required_assets method in PyProjectTomlHandler duplicates the implementation from PythonFileHandler. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring to reuse the common functionality.
One approach is to have PyProjectTomlHandler inherit from PythonFileHandler:
-class PyProjectTomlHandler(FileHandler):
+class PyProjectTomlHandler(PythonFileHandler):
def __init__(self) -> None:
super().__init__()
self.ecosystem = Ecosystem.PYTHON
- def download_required_assets(self, session):
- from safety.safety import fetch_database
-
- SAFETY_DB_DIR = os.getenv("SAFETY_DB_DIR")
-
- db = False if SAFETY_DB_DIR is None else SAFETY_DB_DIR
-
- fetch_database(session=session, full=False, db=db, cached=True,
- telemetry=True, ecosystem=Ecosystem.PYTHON,
- from_cache=False)
-
- fetch_database(session=session, full=True, db=db, cached=True,
- telemetry=True, ecosystem=Ecosystem.PYTHON,
- from_cache=False)This way, PyProjectTomlHandler will inherit the download_required_assets method from PythonFileHandler.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class PyProjectTomlHandler(FileHandler): | |
| def __init__(self) -> None: | |
| super().__init__() | |
| self.ecosystem = Ecosystem.PYTHON | |
| def download_required_assets(self, session): | |
| from safety.safety import fetch_database | |
| SAFETY_DB_DIR = os.getenv("SAFETY_DB_DIR") | |
| db = False if SAFETY_DB_DIR is None else SAFETY_DB_DIR | |
| fetch_database(session=session, full=False, db=db, cached=True, | |
| telemetry=True, ecosystem=Ecosystem.PYTHON, | |
| from_cache=False) | |
| fetch_database(session=session, full=True, db=db, cached=True, | |
| telemetry=True, ecosystem=Ecosystem.PYTHON, | |
| from_cache=False) | |
| class PyProjectTomlHandler(PythonFileHandler): | |
| def __init__(self) -> None: | |
| super().__init__() | |
| self.ecosystem = Ecosystem.PYTHON |
| if 'tool' in data and 'poetry' in data['tool'] and 'dependencies' in data['tool']['poetry']: | ||
| for dep, version in data['tool']['poetry']['dependencies'].items(): | ||
| dependencies.add(f"{dep}=={version}" if isinstance(version, str) else dep) | ||
|
|
There was a problem hiding this comment.
Improve handling of complex version specifications in Poetry dependencies.
When processing tool.poetry.dependencies, the current implementation might not handle complex version specifications or cases where the version is a table or None. This could lead to incorrect dependency parsing.
Consider enhancing the version handling logic:
for dep, version in data['tool']['poetry']['dependencies'].items():
- dependencies.add(f"{dep}=={version}" if isinstance(version, str) else dep)
+ if isinstance(version, str):
+ dependencies.add(f"{dep}{version}")
+ elif version is None:
+ dependencies.add(dep)
+ elif isinstance(version, dict):
+ # Handle complex version specifications
+ # Example: {'version': '^1.2.3'}
+ if 'version' in version:
+ dependencies.add(f"{dep}{version['version']}")
+ else:
+ dependencies.add(dep)This will ensure that all forms of version specifications are correctly parsed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if 'tool' in data and 'poetry' in data['tool'] and 'dependencies' in data['tool']['poetry']: | |
| for dep, version in data['tool']['poetry']['dependencies'].items(): | |
| dependencies.add(f"{dep}=={version}" if isinstance(version, str) else dep) | |
| if 'tool' in data and 'poetry' in data['tool'] and 'dependencies' in data['tool']['poetry']: | |
| for dep, version in data['tool']['poetry']['dependencies'].items(): | |
| if isinstance(version, str): | |
| dependencies.add(f"{dep}{version}") | |
| elif version is None: | |
| dependencies.add(dep) | |
| elif isinstance(version, dict): | |
| # Handle complex version specifications | |
| # Example: {'version': '^1.2.3'} | |
| if 'version' in version: | |
| dependencies.add(f"{dep}{version['version']}") | |
| else: | |
| dependencies.add(dep) |
Tested this manually by adding these files to the repo and conifrming they are picked up. This supports both versions: env.yml and env.yaml
Original issue: #505
Summary by CodeRabbit
New Features
pyproject.tomlfiles.pyproject.toml.pyproject.tomlfiles.Improvements
pyproject.tomlfor improved dependency management.Bug Fixes