Skip to content

feat: [SP-2400] - Create crypto sub command #117

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

matiasdaloia
Copy link
Contributor

@matiasdaloia matiasdaloia commented May 5, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a new top-level "crypto" command with sub-commands: "algorithms", "hints", and "versions-in-range" for cryptographic queries.
    • Enabled querying cryptographic algorithms, encryption hints, and version ranges with optional range filters.
    • Added support for JSON and plain text output formats.
  • Improvements

    • Unified and extended command-line options across all crypto sub-commands.
    • Enhanced input validation and error handling for file inputs and gRPC communication.
    • Refactored cryptography functionality into dedicated classes and streamlined gRPC client methods.
  • Other Changes

    • Added a new default API timeout of 600 seconds.
    • Improved JSON file validation to return detailed validation results.
    • Updated version to 1.24.0 and added corresponding documentation for the new crypto commands.

@matiasdaloia matiasdaloia requested a review from eeisegn May 5, 2025 11:05
@matiasdaloia matiasdaloia self-assigned this May 5, 2025
Copy link

coderabbitai bot commented May 5, 2025

Walkthrough

This update introduces a new top-level crypto command in the CLI, replacing the previous component crypto sub-command. The new crypto command includes nested sub-commands: algorithms, hints, and versions-in-range, each supporting cryptographic-related queries with optional range arguments. These sub-commands use a newly created Cryptography class that interacts with a gRPC backend through refactored and expanded cryptography-specific gRPC client methods. The update adds a new API timeout constant and improves JSON file validation by updating type annotations and documentation for the validation function.

Changes

File(s) Change Summary
src/scanoss/cli.py Removed component crypto sub-command; added new top-level crypto command with nested sub-commands algorithms, hints, and versions-in-range; replaced old handler with three new handlers; integrated new cryptography class and updated argument handling.
src/scanoss/constants.py Added DEFAULT_API_TIMEOUT = 600 constant.
src/scanoss/cryptography.py Added new module defining CryptographyConfig dataclass, Cryptography class for cryptographic data retrieval via gRPC, error class, and CryptographyPresenter for output formatting.
src/scanoss/scanossgrpc.py Removed get_crypto_json method; added five new methods for cryptography-related gRPC calls supporting algorithms, encryption hints, and versions in range with and without range queries; refactored gRPC call handling.
src/scanoss/utils/file.py Updated validate_json_file function’s return type annotation and docstring to return a JsonValidation object instead of a tuple.
CHANGELOG.md Added version 1.24.0 entry describing the new crypto subcommand with its functionalities and a comparison link to previous version.
docs/source/index.rst Added documentation for the new crypto CLI command group and its subcommands with usage examples and argument descriptions.
src/scanoss/__init__.py Updated version string from '1.23.0' to '1.24.0'.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant Cryptography
    participant ScanossGrpc

    User->>CLI: Run "crypto algorithms" command
    CLI->>Cryptography: Create config and instance
    Cryptography->>ScanossGrpc: Request algorithms (with or without range)
    ScanossGrpc-->>Cryptography: Return algorithms data
    Cryptography->>CLI: Present data in requested format
    CLI-->>User: Output results
Loading

Suggested labels

enhancement

Suggested reviewers

  • eeisegn

Poem

In the warren, a new command hops in view,
"Crypto algorithms" now leads the queue.
Out with the old, in with the neat,
A fresh gRPC dance makes results complete.
Constants are set, and types clarified,
With every command, this rabbit’s pride!
🐇✨

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🔭 Outside diff range comments (1)
src/scanoss/constants.py (1)

14-15: 🛠️ Refactor suggestion

Avoid proliferating timeout constants – reuse an existing one instead of DEFAULT_API_TIMEOUT.

DEFAULT_TIMEOUT (180 s) and DEFAULT_SC_TIMEOUT (600 s) already exist. Adding another constant with the same value introduces ambiguity and the risk that different modules silently diverge.
Prefer re-using DEFAULT_SC_TIMEOUT (or renaming it to a more generic name) and delete the new symbol, or alias it explicitly:

-DEFAULT_API_TIMEOUT = 600
+# Alias for backward-compatibility – keeps a single source of truth
+DEFAULT_API_TIMEOUT = DEFAULT_SC_TIMEOUT

This keeps callers consistent and simplifies future changes.

🧹 Nitpick comments (4)
src/scanoss/scanossgrpc.py (1)

579-611: Harmonise response handling and fix misleading docstrings in the new crypto helpers.

  1. _call_rpc already includes status in the returned dict, whereas the older, hand-rolled helpers remove it (del resp_dict['status']).
    Decide on one convention so downstream code doesn’t have to special-case crypto calls.

  2. The docstrings reference protobuf classes (AlgorithmResponse, AlgorithmsInRangeResponse) that are no longer imported and not what the method actually returns (a dict).
    Clarify the return type and remove stale names.

-    Returns:
-        Optional[Dict]: AlgorithmResponse, or None if the request was not successfull
+    Returns:
+        Optional[Dict]: Parsed response dictionary (includes 'status' key) or None on failure
  1. Consider wrapping the call in try/except ScanossGrpcError and logging like other helpers, to keep behaviour consistent.
src/scanoss/cli.py (1)

1419-1451: crypto_algorithms swallows all exceptions – surface actionable errors to users.

The broad except Exception as e prints a generic message and exits.
Consider catching ScanossGrpcError separately to show server details, and log unexpected errors with stack traces when --debug is set:

-    except Exception as e:
-        print_stderr(f'ERROR: {e}')
-        sys.exit(1)
+    except ScanossGrpcError as e:
+        print_stderr(f'API ERROR: {e}')
+        sys.exit(1)
+    except Exception as e:
+        if args.debug:
+            import traceback; traceback.print_exc()
+        print_stderr(f'UNEXPECTED ERROR: {e}')
+        sys.exit(1)

This improves debuggability without overwhelming normal users.

src/scanoss/cryptography.py (2)

27-37: Inconsistent default types returned from argument parsing

getattr(args, 'debug', None) (and similar) returns None when the flag is absent, whereas the dataclass expects booleans.
Down-stream truthiness checks (if self.debug:) work, but mixing None/bool complicates typing and mypy.

-        debug=getattr(args, 'debug', None),
+        debug=bool(getattr(args, 'debug', False)),

Apply the same pattern to trace, quiet, and get_range.


132-135: present() can be invoked before get_algorithms()

If the caller forgets to call get_algorithms() first, the presenter will emit null, which is misleading.
Consider asserting that self.results is populated or calling get_algorithms() implicitly.

if self.results is None:
    raise ScanossCryptographyError('No data to present. Call get_algorithms() first.')
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ad4793 and c69e9dc.

📒 Files selected for processing (5)
  • src/scanoss/cli.py (9 hunks)
  • src/scanoss/constants.py (1 hunks)
  • src/scanoss/cryptography.py (1 hunks)
  • src/scanoss/scanossgrpc.py (1 hunks)
  • src/scanoss/utils/file.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/scanoss/scanossgrpc.py (1)
src/scanoss/api/cryptography/v2/scanoss_cryptography_pb2_grpc.py (4)
  • GetAlgorithms (64-69)
  • GetAlgorithms (162-176)
  • GetAlgorithmsInRange (71-76)
  • GetAlgorithmsInRange (179-193)
src/scanoss/cli.py (3)
src/scanoss/cryptography.py (4)
  • Cryptography (40-134)
  • create_cryptography_config_from_args (27-37)
  • get_algorithms (76-97)
  • present (132-134)
src/scanoss/scanossgrpc.py (2)
  • create_grpc_config_from_args (649-662)
  • ScanossGrpc (94-626)
src/scanoss/scanossbase.py (1)
  • print_stderr (45-49)
src/scanoss/cryptography.py (5)
src/scanoss/scanossbase.py (1)
  • ScanossBase (28-107)
src/scanoss/scanossgrpc.py (2)
  • get_crypto_algorithms_in_range_for_purl (596-611)
  • get_crypto_algorithms_for_purl (579-594)
src/scanoss/utils/abstract_presenter.py (1)
  • AbstractPresenter (6-103)
src/scanoss/utils/file.py (1)
  • validate_json_file (44-84)
src/scanoss/cli.py (1)
  • print_stderr (80-84)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (3)
src/scanoss/utils/file.py (1)

52-53:

✅ Verification successful

Double-check all call sites now that the return type changed to an object.

validate_json_file no longer returns a (bool, str) tuple but a JsonValidation dataclass.
Please verify every invocation (e.g., is_valid, msg = …) and update them accordingly to avoid runtime unpacking errors.

You can quickly locate potential offenders:


🏁 Script executed:

#!/bin/bash
# Find places still assuming a 2-tuple return value.
rg -n "validate_json_file\(.*\)" | rg -v "\.is_valid"

Length of output: 411


Let’s inspect each call site to see how the return value is consumed:


🏁 Script executed:

#!/bin/bash
# Show context around validate_json_file invocations to verify tuple unpacking vs. dataclass usage
rg -n -C2 "validate_json_file" src/scanoss/cryptography.py
rg -n -C2 "validate_json_file" src/scanoss/scanoss_settings.py
rg -n -C2 "validate_json_file" src/scanoss/cli.py

Length of output: 1393


All validate_json_file call sites correctly use JsonValidation fields

I checked the three invocations in:

  • src/scanoss/cryptography.py (uses input_file_validation.is_valid)
  • src/scanoss/scanoss_settings.py (uses result.is_valid and result.error_code)
  • src/scanoss/cli.py (uses result.is_valid and result.error)

None of these sites still unpack a (bool, str) tuple. You can safely ignore the unpacking warning—no further updates are needed.

src/scanoss/cli.py (2)

357-386: Sub-parser adds a --range/-r option that shadows Python’s built-in range.

Shadowing isn’t fatal here (it lives only in argparse.Namespace), but it hampers readability and may confuse linters.
Rename to something more explicit, e.g., --version-range / -vr, and adjust CryptographyConfig.get_range accordingly.

-p_crypto_algorithms.add_argument(
-    '--range',
-    '-r',
+p_crypto_algorithms.add_argument(
+    '--version-range',
+    '-vr',

[ suggest_nitpick ]


398-400: Default timeout uses the new constant – ensure it remains in sync with gRPC client defaults.

ScanossGrpc.__init__ defaults timeout=600. If the earlier comment about consolidating timeout constants is addressed, remember to import the single agreed-upon constant here to avoid silent drift.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🔭 Outside diff range comments (1)
src/scanoss/cli.py (1)

1466-1505: 🛠️ Refactor suggestion

Good implementation of crypto_algorithms command handler.

The function properly validates input arguments, checks for certificate file existence, creates configurations, and handles errors. However, the PAC file functionality is marked as a TODO and is commented out.

Implement the TODO for PAC file support or remove the commented-out code to avoid confusion. This functionality appears to be implemented in other command handlers (like comp_vulns at line 1606).

-        # TODO: Add PAC file support
-        # pac_file = get_pac_file(config.pac)
+        pac_file = get_pac_file(args.pac)
+        if pac_file:
+            # Use PAC file if provided
+            grpc_config.pac = pac_file
♻️ Duplicate comments (1)
src/scanoss/cryptography.py (1)

143-172: ⚠️ Potential issue

The _build_purls_request method has a potential issue with single string handling.

While the method correctly handles both input file and direct purl list cases, there's an issue with how it processes the purl attribute.

If self.config.purl is a single string (rather than a list), iterating over it will treat each character as a separate purl. Fix this by ensuring proper handling of both single string and list cases:

        if self.config.purl:
-            return {'purls': [{'purl': p} for p in self.config.purl]}
+            # Handle both single string and list cases
+            purls = self.config.purl
+            if isinstance(purls, str):
+                purls = [purls]
+            return {'purls': [{'purl': p} for p in purls]}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75bc9bf and 84ea3a4.

📒 Files selected for processing (3)
  • src/scanoss/cli.py (9 hunks)
  • src/scanoss/cryptography.py (1 hunks)
  • src/scanoss/scanossgrpc.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/scanoss/scanossgrpc.py (1)
src/scanoss/api/cryptography/v2/scanoss_cryptography_pb2_grpc.py (10)
  • GetAlgorithms (64-69)
  • GetAlgorithms (162-176)
  • GetAlgorithmsInRange (71-76)
  • GetAlgorithmsInRange (179-193)
  • GetEncryptionHints (92-97)
  • GetEncryptionHints (230-244)
  • GetHintsInRange (85-90)
  • GetHintsInRange (213-227)
  • GetVersionsInRange (78-83)
  • GetVersionsInRange (196-210)
src/scanoss/cli.py (3)
src/scanoss/cryptography.py (6)
  • Cryptography (40-176)
  • create_cryptography_config_from_args (27-37)
  • get_algorithms (76-96)
  • present (174-176)
  • get_encryption_hints (98-120)
  • get_versions_in_range (122-141)
src/scanoss/scanossgrpc.py (3)
  • create_grpc_config_from_args (700-713)
  • ScanossGrpc (94-677)
  • ScanossGrpcError (77-82)
src/scanoss/scanossbase.py (1)
  • print_stderr (45-49)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (25)
src/scanoss/scanossgrpc.py (5)

579-594: Well-structured API method for retrieving cryptographic algorithms.

The implementation follows a consistent pattern with other gRPC methods in the class, leveraging the _call_rpc helper method for standardized request handling and error management. This promotes code reuse and maintainability.


596-611: Excellent implementation of range-based algorithm retrieval.

This method provides the capability to query algorithms within a specific version range, complementing the basic algorithm retrieval functionality. The consistent use of the _call_rpc helper method maintains a uniform approach across all cryptography API methods.


613-628: Clean implementation of encryption hints retrieval.

The method follows the established pattern of using the _call_rpc helper, maintaining consistency with other API methods. The docstring clearly explains the purpose, parameters, and return values.


630-645: Well-designed method for retrieving encryption hints within a range.

The implementation maintains a consistent pattern with other gRPC methods, leveraging the standardized _call_rpc helper. The debug message passed to the helper method is descriptive and helpful for troubleshooting.


647-662: Effective implementation of version range retrieval.

This method completes the set of cryptography-related gRPC methods, providing a way to query component versions that contain or don't contain cryptographic algorithms. The consistent approach across all these methods will make the codebase easier to maintain.

src/scanoss/cli.py (9)

34-34: Good import of new cryptography module.

The import brings in the new Cryptography class and helper function needed for the new CLI commands.


54-54: Appropriate use of the new API timeout constant.

Using a named constant for the default timeout improves maintainability and provides consistency across the application.


357-406: Well-structured new crypto command with appropriate sub-commands.

This new command structure separates cryptography functionality into its own top-level command with clear sub-commands for different operations. The required=True parameter on line 369 ensures that a sub-command must be specified, and the help text for each command is descriptive and helpful.


408-409: Good inclusion of crypto commands in the common purl options group.

This ensures that all commands that process PURLs have consistent options available.


413-422: Appropriate grouping of crypto commands for common component options.

Including the new crypto commands in the existing component options group maintains consistency across the CLI.


427-428: Updated timeout default to use the new constant.

Using the DEFAULT_API_TIMEOUT constant provides consistency and makes it easier to update timeouts across the application.


651-653: Good addition of crypto commands to output options.

This ensures the new crypto commands have consistent output handling with the rest of the application.


776-778: Appropriate inclusion of crypto commands in global GRPC options.

This maintains consistency across all commands that use gRPC functionality.


833-835: Good addition of crypto commands to the help/trace options.

This ensures consistent debug and tracing capabilities across all commands.

src/scanoss/cryptography.py (11)

11-12: Good practice to create a custom exception class.

Creating a dedicated exception class for cryptography-related errors allows for more specific error handling and improves code clarity.


27-37: Clean config creation from CLI arguments.

The function effectively extracts configuration from CLI arguments using getattr with default values to handle missing attributes.


40-74: Well-organized class initialization.

The Cryptography class is properly initialized with dependencies injected, following good software design principles. It creates its own ScanossBase and CryptographyPresenter instances and stores the provided configuration and client.


76-96: Good implementation of get_algorithms method.

The method properly checks for missing purls, logs the operation, calls the appropriate gRPC method based on configuration, and handles the response.


98-120: Good implementation of get_encryption_hints method.

Similar to get_algorithms, this method validates inputs, logs actions, calls the correct gRPC method, and handles the response appropriately.


122-141: Good implementation of get_versions_in_range method.

Following the pattern of the other methods, this implementation properly validates, logs, and handles the gRPC call.


174-176: Clean presentation method.

The present method provides a simple interface to the presenter, delegating all formatting details.


179-188: Good presenter class design.

The CryptographyPresenter class properly inherits from AbstractPresenter and takes the cryptography instance in its constructor, following good OOP practices.


189-196: Simple and effective JSON output formatting.

The method directly uses json.dumps with proper indentation, which is appropriate for JSON formatting.


198-206: Good plain text output formatting with type checking.

The method handles both dictionary and non-dictionary results appropriately.


208-218: Clear indication of unimplemented output formats.

Using NotImplementedError with descriptive messages is a good way to indicate that these output formats are not yet supported.

Comment on lines 15 to 24
@dataclass
class CryptographyConfig:
debug: bool = False
trace: bool = False
quiet: bool = False
with_range: bool = False
purl: List[str] = None
input_file: str = None
output_file: str = None
header: str = None
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Well-structured configuration dataclass.

The CryptographyConfig class effectively encapsulates all configuration options needed for the cryptography functionality. However, there's an issue with the type of the purl field.

The purl field is defined as List[str] but isn't handled correctly in _build_purls_request. This can cause issues if a single string is provided:

-    purl: List[str] = None
+    purl: Optional[List[str]] = None

This should also be complemented with proper type handling in the _build_purls_request method.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 157 to 159
input_file_validation = validate_json_file(self.config.input_file)
if not input_file_validation.is_valid:
raise Exception(f'There was a problem with the purl input file. {input_file_validation.error}')
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve exception handling in file validation.

Using a generic Exception with a hardcoded message obscures the actual error.

Use a more specific exception type and preserve the original exception details:

-                raise Exception(f'There was a problem with the purl input file. {input_file_validation.error}')
+                raise ScanossCryptographyError(f'There was a problem with the purl input file: {input_file_validation.error}')
📝 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.

Suggested change
input_file_validation = validate_json_file(self.config.input_file)
if not input_file_validation.is_valid:
raise Exception(f'There was a problem with the purl input file. {input_file_validation.error}')
input_file_validation = validate_json_file(self.config.input_file)
if not input_file_validation.is_valid:
- raise Exception(f'There was a problem with the purl input file. {input_file_validation.error}')
+ raise ScanossCryptographyError(
+ f'There was a problem with the purl input file: {input_file_validation.error}'
+ )

Comment on lines 167 to 168
):
raise Exception('The supplied input file is not in the correct PurlRequest format.')
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve exception handling for format validation.

Similar to the previous issue, using a generic Exception with a hardcoded message isn't ideal.

Use the custom exception class for better error handling:

-                raise Exception('The supplied input file is not in the correct PurlRequest format.')
+                raise ScanossCryptographyError('The supplied input file is not in the correct PurlRequest format.')
📝 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.

Suggested change
):
raise Exception('The supplied input file is not in the correct PurlRequest format.')
):
raise ScanossCryptographyError('The supplied input file is not in the correct PurlRequest format.')

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
src/scanoss/cli.py (1)

1467-1506: 💡 Verification agent

🧩 Analysis chain

Good implementation of crypto_algorithms command handler.

The function properly validates inputs, checks certificate existence, and creates the necessary configurations to execute the command. The error handling is appropriate, capturing both specific gRPC errors and general exceptions.

However, there's a TODO comment about PAC file support that should be addressed.

Please consider implementing the PAC file support or provide a timeline for when this will be addressed:


🏁 Script executed:

#!/bin/bash
# Check for similar PAC file implementations in other handlers
grep -A 3 "pac_file = get_pac_file" src/scanoss/cli.py

Length of output: 1414


Add PAC file support to crypto_algorithms handler

The crypto_algorithms subcommand currently skips proxy configuration, even though other commands in src/scanoss/cli.py already handle PAC files. Please align this handler with the existing patterns:

  • Retrieve the PAC file at the start of the handler:
    pac_file = get_pac_file(args.pac)
    if pac_file is None:
        print_stderr(f'No proxy configuration for: {args.pac}')
        sys.exit(1)
  • Pass pac_file through your gRPC client or into Cryptography (via its config) so that all outgoing calls respect the proxy.
  • Remove the # TODO: Add PAC file support placeholder once implemented.

Example diff:

--- a/src/scanoss/cli.py
+++ b/src/scanoss/cli.py
@@ def crypto_algorithms(parser, args):
-    # TODO: Add PAC file support
-    # pac_file = get_pac_file(config.pac)
+    # Load PAC file for proxy configuration
+    pac_file = get_pac_file(args.pac)
+    if pac_file is None:
+        print_stderr(f'No proxy configuration for: {args.pac}')
+        sys.exit(1)
+
     cryptography = Cryptography(config=config, client=client)

[src/scanoss/cli.py lines 1467–1506]

♻️ Duplicate comments (2)
src/scanoss/cli.py (2)

1508-1547: Good implementation of crypto_hints command handler.

Similar to the algorithms handler, this function validates inputs, creates configurations, and handles errors appropriately. It also has the same issue with PAC file support being marked as a TODO.


1549-1587: Good implementation of crypto_versions_in_range command handler.

Like the other crypto command handlers, this function properly validates inputs, creates configurations, and handles errors. It also has the same issue with PAC file support.

🧹 Nitpick comments (1)
CHANGELOG.md (1)

12-17: CHANGELOG properly updated with new version and features.

The entry correctly documents the new functionality added in version 1.24.0, with clear descriptions of each new crypto sub-command.

Minor grammatical improvement suggestion: Consider standardizing the wording to avoid repetition of "Add".

## [1.24.0] - 2025-05-06
### Added
- Add `crypto` subcommand to retrieve cryptographic algorithms for the given components
- Add `crypto hints` subcommand to retrieve cryptographic hints for the given components
- Add `crypto versions-in-range` subcommand to retrieve cryptographic versions in range for the given components

Could be improved to:

## [1.24.0] - 2025-05-06
### Added
- New `crypto` subcommand to retrieve cryptographic algorithms for the given components
- New `crypto hints` subcommand to retrieve cryptographic hints for the given components
- New `crypto versions-in-range` subcommand to retrieve cryptographic versions in range for the given components
🧰 Tools
🪛 LanguageTool

[grammar] ~13-~13: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...hanges... ## [1.24.0] - 2025-05-06 ### Added - Add crypto subcommand to retrieve cryptog...

(REPEATED_VERBS)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84ea3a4 and 2133ef8.

📒 Files selected for processing (4)
  • CHANGELOG.md (2 hunks)
  • docs/source/index.rst (1 hunks)
  • src/scanoss/__init__.py (1 hunks)
  • src/scanoss/cli.py (10 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/scanoss/init.py
  • docs/source/index.rst
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md

[grammar] ~13-~13: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...hanges... ## [1.24.0] - 2025-05-06 ### Added - Add crypto subcommand to retrieve cryptog...

(REPEATED_VERBS)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (5)
src/scanoss/cli.py (4)

34-34: Good import of the new cryptography module.

The import statement correctly references the new Cryptography class and its configuration helper function, which will be used by the new command handlers.


357-405: Well-structured implementation of the new crypto command hierarchy.

The command structure follows the established pattern used for other command groups in the codebase:

  • Main 'crypto' command with appropriate aliases
  • Logical sub-commands with clear descriptions
  • The '--with-range' flag is properly implemented where needed
  • Clear help messages that explain the purpose of each command

407-421: Good integration with existing CLI infrastructure.

The new crypto commands are properly integrated into all the necessary command groups to inherit common CLI options, including output options, API options, GRPC options, and help/trace options.

Also applies to: 650-652, 737-739, 775-778, 832-834


848-849: Proper inclusion of the new crypto command in the sub-command verification.

The condition correctly includes 'crypto' and its alias 'cr' when checking if a sub-command is required.

CHANGELOG.md (1)

524-525: Correctly added version comparison link.

The link to compare changes between versions 1.23.0 and 1.24.0 is correctly formatted and included at the end of the file.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/scanoss/cli.py (1)

1467-1590: Consider refactoring common code in crypto handlers.

The three crypto handler functions share very similar structure and logic. Consider refactoring the common parts into a helper function to improve maintainability and reduce code duplication.

+ def _handle_crypto_command(parser, args, command_func, command_name):
+     """
+     Common handler function for crypto commands
+     Parameters
+     ----------
+         parser: ArgumentParser
+             command line parser object
+         args: Namespace
+             Parsed arguments
+         command_func: Callable
+             The Cryptography method to call
+         command_name: str
+             Name of the command for error messages
+     """
+     if (not args.purl and not args.input) or (args.purl and args.input):
+         print_stderr('Please specify an input file or purl to decorate (--purl or --input)')
+         parser.parse_args([args.subparser, args.subparsercmd, '-h'])
+         sys.exit(1)
+     if args.ca_cert and not os.path.exists(args.ca_cert):
+         print_stderr(f'Error: Certificate file does not exist: {args.ca_cert}.')
+         sys.exit(1)
+ 
+     try:
+         config = create_cryptography_config_from_args(args)
+         grpc_config = create_grpc_config_from_args(args)
+         if args.pac:
+             grpc_config.pac = get_pac_file(args.pac)
+         if args.header:
+             grpc_config.req_headers = process_req_headers(args.header)
+         client = ScanossGrpc(**asdict(grpc_config))
+ 
+         cryptography = Cryptography(config=config, client=client)
+         command_func(cryptography)
+         cryptography.present(output_file=args.output)
+     except ScanossGrpcError as e:
+         print_stderr(f'API ERROR: {e}')
+         sys.exit(1)
+     except Exception as e:
+         if args.debug:
+             import traceback
+ 
+             traceback.print_exc()
+         print_stderr(f'ERROR: {e}')
+         sys.exit(1)
+ 
+ def crypto_algorithms(parser, args):
+     """
+     Run the "crypto algorithms" sub-command
+     Parameters
+     ----------
+         parser: ArgumentParser
+             command line parser object
+         args: Namespace
+             Parsed arguments
+     """
+     _handle_crypto_command(parser, args, lambda crypto: crypto.get_algorithms(), "algorithms")
+ 
+ def crypto_hints(parser, args):
+     """
+     Run the "crypto hints" sub-command
+     Parameters
+     ----------
+         parser: ArgumentParser
+             command line parser object
+         args: Namespace
+             Parsed arguments
+     """
+     _handle_crypto_command(parser, args, lambda crypto: crypto.get_encryption_hints(), "hints")
+ 
+ def crypto_versions_in_range(parser, args):
+     """
+     Run the "crypto versions-in-range" sub-command
+     Parameters
+     ----------
+         parser: ArgumentParser
+             command line parser object
+         args: Namespace
+             Parsed arguments
+     """
+     _handle_crypto_command(parser, args, lambda crypto: crypto.get_versions_in_range(), "versions-in-range")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2133ef8 and 3a2dff6.

📒 Files selected for processing (2)
  • src/scanoss/cli.py (10 hunks)
  • src/scanoss/scanossgrpc.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/scanoss/scanossgrpc.py (1)
src/scanoss/api/cryptography/v2/scanoss_cryptography_pb2_grpc.py (10)
  • GetAlgorithms (64-69)
  • GetAlgorithms (162-176)
  • GetAlgorithmsInRange (71-76)
  • GetAlgorithmsInRange (179-193)
  • GetEncryptionHints (92-97)
  • GetEncryptionHints (230-244)
  • GetHintsInRange (85-90)
  • GetHintsInRange (213-227)
  • GetVersionsInRange (78-83)
  • GetVersionsInRange (196-210)
src/scanoss/cli.py (3)
src/scanoss/cryptography.py (6)
  • Cryptography (40-176)
  • create_cryptography_config_from_args (27-37)
  • get_algorithms (76-96)
  • present (174-176)
  • get_encryption_hints (98-120)
  • get_versions_in_range (122-141)
src/scanoss/scanossgrpc.py (3)
  • create_grpc_config_from_args (701-713)
  • ScanossGrpc (94-677)
  • ScanossGrpcError (77-82)
src/scanoss/scanossbase.py (1)
  • print_stderr (45-49)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (10)
src/scanoss/scanossgrpc.py (6)

579-594: Well-implemented crypto algorithm query method.

This method follows the established pattern of using _call_rpc helper for gRPC calls, making the code more maintainable and consistent with other methods in the class.


596-611: Good implementation of algorithms in range query method.

The method properly uses the internal _call_rpc helper with appropriate parameters and error handling. The docstring clearly describes its purpose and return values.


613-628: Encryption hints query method looks good.

This implements the client function for calling the GetEncryptionHints RPC in a consistent manner with other methods, using the shared _call_rpc helper.


630-645: Encryption hints in range query method is well-implemented.

The method uses the same pattern as other crypto methods, ensuring a consistent API for the client code.


647-662: Versions in range query method looks correct.

This method follows the established pattern for gRPC client methods, with appropriate error handling via the _call_rpc helper method.


697-698: Fields reordered in GrpcConfig dataclass.

The pac and req_headers fields have been reordered. This is a minor change that maintains the functionality while aligning with the updated code structure.

src/scanoss/cli.py (4)

357-405: Well-structured crypto command group with appropriate subcommands.

The implementation adds a new top-level crypto command with three nested subcommands: algorithms, hints, and versions-in-range. Each has appropriate aliases and descriptive help text. The implementation follows the same pattern as other command groups in the codebase.


1467-1507: Good implementation of crypto_algorithms command handler.

The handler correctly:

  1. Validates input parameters
  2. Checks certificate files
  3. Creates appropriate configurations
  4. Sets up gRPC client with proxy and header handling
  5. Initializes the Cryptography class
  6. Performs the API call and presents results
  7. Handles both specific ScanossGrpcError and general exceptions

1509-1549: Good implementation of crypto_hints command handler.

The function correctly validates inputs, creates configurations, and handles errors appropriately, following the same pattern as the algorithms handler. The PAC file support is properly implemented.


1551-1590: Good implementation of crypto_versions_in_range command handler.

Like the other crypto command handlers, this function properly validates inputs, creates configurations, and handles errors. The implementation is consistent with the other crypto handlers.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (4)
src/scanoss/cryptography.py (4)

40-42: 🛠️ Refactor suggestion

Use custom exception class instead of generic Exception

The class defines a custom ScanossCryptographyError exception but doesn't use it consistently throughout the code. Using generic exceptions with hardcoded error messages obscures the root cause.

-                raise Exception(
-                    f'There was a problem with the purl input file. {input_file_validation.error}'
-                )
+                raise ScanossCryptographyError(
+                    f'There was a problem with the purl input file: {input_file_validation.error}'
+                )

54-56: 🛠️ Refactor suggestion

Use custom exception class for format validation errors

For consistency, use the custom ScanossCryptographyError exception instead of ValueError.

-                raise ValueError(
-                    'The supplied input file is not in the correct PurlRequest format.'
-                )
+                raise ScanossCryptographyError(
+                    'The supplied input file is not in the correct PurlRequest format.'
+                )

214-216: 🛠️ Refactor suggestion

Use custom exception class and preserve original error details

Use the custom exception class consistently and preserve the original error information.

-                raise Exception(
-                    f'There was a problem with the purl input file. {input_file_validation.error}'
-                )
+                raise ScanossCryptographyError(
+                    f'There was a problem with the purl input file: {input_file_validation.error}'
+                )

219-229: 🛠️ Refactor suggestion

Handle potential None value for self.config.purl

The code currently assumes self.config.purl is never None, but the previous comments suggest it should be made optional. Ensure proper handling of None values.

        if self.config.purl:
            return {
                'purls': [
                    {
                        'purl': p,
                        'requirement': self._extract_version_from_purl(p),
                    }
                    for p in self.config.purl
                ]
            }

This code handles None correctly but would need to be updated if you change the type to Optional[List[str]] to also handle if self.config.purl is a single string rather than a list.

🧹 Nitpick comments (4)
src/scanoss/cryptography.py (4)

32-32: Replace magic value with a named constant

Using a magic value in comparison reduces code readability and maintainability.

+    # Minimum number of parts expected when splitting a PURL by '@'
+    MIN_PURL_PARTS = 2
-                    if not (len(parts) >= 2 and parts[1]):
+                    if not (len(parts) >= MIN_PURL_PARTS and parts[1]):

This change addresses the static analysis warning PLR2004 and improves code readability.

🧰 Tools
🪛 Ruff (0.8.2)

32-32: Magic value used in comparison, consider replacing 2 with a constant variable

(PLR2004)

🪛 GitHub Actions: Lint

[warning] 32-32: PLR2004 Magic value used in comparison, consider replacing 2 with a constant variable


201-210: Update docstring to match method signature

The docstring for _build_purls_request references parameters that don't exist in the method signature.

-        """
-        Load the specified purls from a JSON file or a list of PURLs and return a dictionary
-
-        Args:
-            json_file (Optional[str], optional): The JSON file containing the PURLs. Defaults to None.
-            purls (Optional[List[str]], optional): The list of PURLs. Defaults to None.
-
-        Returns:
-            Optional[dict]: The dictionary containing the PURLs
-        """
+        """
+        Build a purls request dictionary from either the input file or the list of purls
+        in the configuration.
+
+        Returns:
+            Optional[dict]: The dictionary containing the PURLs formatted for the API request, 
+                           or None if no purls are available.
+        """

246-247: Improve error handling for version extraction

The error handling for version extraction from PURL could be improved by using the custom exception class and providing more context.

        try:
            return purl.split('@')[-1]
        except IndexError:
-            raise ValueError(f'Invalid purl format: {purl}')
+            raise ScanossCryptographyError(
+                f'Invalid purl format: {purl}. A valid purl must include a version (e.g., pkg:type/name@version)'
+            )

284-287: Improve plain text output formatting

The plain text output is identical to the JSON output when results are a dictionary. Consider creating a truly plain-text representation that's more human-readable.

-        return (
-            json.dumps(self.cryptography.results, indent=2)
-            if isinstance(self.cryptography.results, dict)
-            else str(self.cryptography.results)
-        )
+        """
+        Format the scan output data into a plain text string
+        """
+        if not self.cryptography.results:
+            return "No results available."
+            
+        if isinstance(self.cryptography.results, dict):
+            # Create a more readable plain text representation
+            output = []
+            
+            # Example formatting for algorithms data
+            if "algorithms" in self.cryptography.results:
+                output.append("CRYPTOGRAPHIC ALGORITHMS:")
+                for algo_data in self.cryptography.results["algorithms"]:
+                    purl = algo_data.get("purl", "Unknown PURL")
+                    output.append(f"\n{purl}")
+                    
+                    if "algorithms" in algo_data:
+                        for algo in algo_data["algorithms"]:
+                            output.append(f"  - {algo.get('name', 'Unknown')} ({algo.get('type', 'Unknown type')})")
+            
+            # Add similar formatting for other result types
+            
+            return "\n".join(output)
+        else:
+            return str(self.cryptography.results)

Note: The exact implementation would depend on the structure of the results for different API calls.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a2dff6 and 0fc961b.

📒 Files selected for processing (1)
  • src/scanoss/cryptography.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/scanoss/cryptography.py (5)
src/scanoss/scanossbase.py (1)
  • ScanossBase (28-107)
src/scanoss/scanossgrpc.py (5)
  • get_crypto_algorithms_in_range_for_purl (596-611)
  • get_crypto_algorithms_for_purl (579-594)
  • get_encryption_hints_in_range_for_purl (630-645)
  • get_encryption_hints_for_purl (613-628)
  • get_versions_in_range_for_purl (647-662)
src/scanoss/utils/abstract_presenter.py (1)
  • AbstractPresenter (6-103)
src/scanoss/utils/file.py (1)
  • validate_json_file (44-84)
src/scanoss/cli.py (1)
  • print_stderr (80-84)
🪛 Ruff (0.8.2)
src/scanoss/cryptography.py

32-32: Magic value used in comparison, consider replacing 2 with a constant variable

(PLR2004)

🪛 GitHub Actions: Lint
src/scanoss/cryptography.py

[warning] 32-32: PLR2004 Magic value used in comparison, consider replacing 2 with a constant variable

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (1)
src/scanoss/cryptography.py (1)

17-17:

❌ Incorrect review comment

Ensure purl field is properly handled as optional

The purl field is declared as a non-optional List[str] without a default value, which differs from other optional parameters that explicitly set defaults to None. This could cause initialization issues if no purls are provided.

-    purl: List[str]
+    purl: Optional[List[str]] = None

This change would make the field's optionality explicit and consistent with how it's handled in the error checks on lines 123-126, 151-154, and 181-184.


🏁 Script executed:

#!/bin/bash
grep -R -n "purl" src/scanoss/cryptography.py

Length of output: 3258


🏁 Script executed:

#!/bin/bash
sed -n '1,200p' src/scanoss/cryptography.py

Length of output: 6737


🏁 Script executed:

#!/bin/bash
sed -n '190,260p' src/scanoss/cryptography.py

Length of output: 2223


Ignore optionality change for purl

The purl field is always supplied by create_cryptography_config_from_args (it defaults to an empty list if no --purl is given), so it can never be uninitialized at runtime. Making it Optional[List[str]] = None isn’t necessary and would complicate the existing default logic. If you did want a default built in at the dataclass level, the more idiomatic approach is:

from dataclasses import field

@dataclass
class CryptographyConfig:
    purl: List[str] = field(default_factory=list)
    # …

The current setup correctly triggers a “No PURLs supplied. Provide --purl or --input.” error when both purl and input_file are empty.

Likely an incorrect or invalid review comment.

Comment on lines +60 to +62
raise ValueError(
'One or more PURLs are missing the "requirement" field.'
)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use custom exception class for requirement field errors

For consistency, use the custom ScanossCryptographyError exception here as well.

-                    raise ValueError(
-                        'One or more PURLs are missing the "requirement" field.'
-                    )
+                    raise ScanossCryptographyError(
+                        'One or more PURLs are missing the "requirement" field.'
+                    )
📝 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.

Suggested change
raise ValueError(
'One or more PURLs are missing the "requirement" field.'
)
raise ScanossCryptographyError(
'One or more PURLs are missing the "requirement" field.'
)

Comment on lines +211 to +218
if self.config.input_file:
input_file_validation = validate_json_file(self.config.input_file)
if not input_file_validation.is_valid:
raise Exception(
f'There was a problem with the purl input file. {input_file_validation.error}'
)

return input_file_validation.data
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Eliminate code duplication in file validation logic

The input file validation logic is duplicated between __post_init__ and _build_purls_request.

Consider extracting the validation logic into a separate helper method:

+    def _validate_input_file(self, input_file: str) -> dict:
+        """
+        Validate the input file and return the parsed data.
+        
+        Args:
+            input_file (str): Path to the input file
+            
+        Returns:
+            dict: The parsed data from the input file
+            
+        Raises:
+            ScanossCryptographyError: If the input file is invalid
+        """
+        input_file_validation = validate_json_file(input_file)
+        if not input_file_validation.is_valid:
+            raise ScanossCryptographyError(
+                f'There was a problem with the purl input file: {input_file_validation.error}'
+            )
+        return input_file_validation.data
+
     def _build_purls_request(
         self,
     ) -> Optional[dict]:
         """
         Build a purls request dictionary from either the input file or the list of purls
         in the configuration.

         Returns:
             Optional[dict]: The dictionary containing the PURLs formatted for the API request,
                           or None if no purls are available.
         """
         if self.config.input_file:
-            input_file_validation = validate_json_file(self.config.input_file)
-            if not input_file_validation.is_valid:
-                raise Exception(
-                    f'There was a problem with the purl input file. {input_file_validation.error}'
-                )
-
-            return input_file_validation.data
+            return self._validate_input_file(self.config.input_file)

Then update the __post_init__ method to use this helper method as well.

📝 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.

Suggested change
if self.config.input_file:
input_file_validation = validate_json_file(self.config.input_file)
if not input_file_validation.is_valid:
raise Exception(
f'There was a problem with the purl input file. {input_file_validation.error}'
)
return input_file_validation.data
def _validate_input_file(self, input_file: str) -> dict:
"""
Validate the input file and return the parsed data.
Args:
input_file (str): Path to the input file
Returns:
dict: The parsed data from the input file
Raises:
ScanossCryptographyError: If the input file is invalid
"""
input_file_validation = validate_json_file(input_file)
if not input_file_validation.is_valid:
raise ScanossCryptographyError(
f'There was a problem with the purl input file: {input_file_validation.error}'
)
return input_file_validation.data
def _build_purls_request(
self,
) -> Optional[dict]:
"""
Build a purls request dictionary from either the input file or the list of purls
in the configuration.
Returns:
Optional[dict]: The dictionary containing the PURLs formatted for the API request,
or None if no purls are available.
"""
if self.config.input_file:
return self._validate_input_file(self.config.input_file)
# ... existing logic for self.config.purl ...

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (4)
src/scanoss/cryptography.py (4)

63-65: Use custom exception class for requirement field errors

For consistency with the module's error handling approach, use the custom ScanossCryptographyError exception instead of a generic ValueError.

-                    raise ValueError(
+                    raise ScanossCryptographyError(
                         'One or more PURLs are missing the "requirement" field.'
                     )

217-219: 🛠️ Refactor suggestion

Use custom exception class for file validation errors

Replace the generic Exception with the custom ScanossCryptographyError for consistency with the rest of the module and better error handling.

-                raise Exception(
+                raise ScanossCryptographyError(
                     f'There was a problem with the purl input file. {input_file_validation.error}'
                 )

42-45: 🛠️ Refactor suggestion

Use custom exception class for input file validation errors

The code is using a generic Exception with a hardcoded message. For consistency with the rest of the module and better error handling, use the custom ScanossCryptographyError class you've defined.

-                raise Exception(
+                raise ScanossCryptographyError(
                     f'There was a problem with the purl input file. {input_file_validation.error}'
                 )

214-221: 🛠️ Refactor suggestion

Eliminate code duplication in file validation logic

The input file validation logic is duplicated between __post_init__ and _build_purls_request. Extract this into a separate helper method to avoid duplication and ensure consistent validation.

+    def _validate_input_file(self, input_file: str) -> dict:
+        """
+        Validate the input file and return the parsed data.
+        
+        Args:
+            input_file (str): Path to the input file
+            
+        Returns:
+            dict: The parsed data from the input file
+            
+        Raises:
+            ScanossCryptographyError: If the input file is invalid
+        """
+        input_file_validation = validate_json_file(input_file)
+        if not input_file_validation.is_valid:
+            raise ScanossCryptographyError(
+                f'There was a problem with the purl input file: {input_file_validation.error}'
+            )
+        return input_file_validation.data
+
     def _build_purls_request(
         self,
     ) -> Optional[dict]:
         """
         Load the specified purls from a JSON file or a list of PURLs and return a dictionary

         Args:
             json_file (Optional[str], optional): The JSON file containing the PURLs. Defaults to None.
             purls (Optional[List[str]], optional): The list of PURLs. Defaults to None.

         Returns:
             Optional[dict]: The dictionary containing the PURLs
         """
         if self.config.input_file:
-            input_file_validation = validate_json_file(self.config.input_file)
-            if not input_file_validation.is_valid:
-                raise Exception(
-                    f'There was a problem with the purl input file. {input_file_validation.error}'
-                )
-
-            return input_file_validation.data
+            return self._validate_input_file(self.config.input_file)

Then update the __post_init__ method to use this helper method as well.

🧹 Nitpick comments (3)
src/scanoss/cryptography.py (3)

223-231: Conditionally include requirement field based on with_range setting

The code always extracts and includes the version requirement from PURLs, even when with_range is False. This might lead to unnecessary processing or confusion. Consider conditionally including the requirement field only when it's needed.

         if self.config.purl:
+            purls_dict = {'purls': []}
+            for p in self.config.purl:
+                purl_entry = {'purl': p}
+                if self.config.with_range:
+                    purl_entry['requirement'] = self._extract_version_from_purl(p)
+                purls_dict['purls'].append(purl_entry)
+            return purls_dict
-            return {
-                'purls': [
-                    {
-                        'purl': p,
-                        'requirement': self._extract_version_from_purl(p),
-                    }
-                    for p in self.config.purl
-                ]
-            }

248-250: Improve error handling in version extraction

The current implementation catches IndexError but it's unclear when this would occur since .split() always returns at least one item. Also, the error message doesn't provide actionable guidance.

     def _extract_version_from_purl(self, purl: str) -> str:
         """
         Extract version from purl

         Args:
             purl (str): The purl string to extract the version from

         Returns:
             str: The extracted version

         Raises:
             ValueError: If the purl is not in the correct format
         """
         try:
-            return purl.split('@')[-1]
+            parts = purl.split('@')
+            if len(parts) < 2 or not parts[1]:
+                raise ValueError(
+                    f'Invalid PURL format: "{purl}". It must include a version (e.g., pkg:type/name@version)'
+                )
+            return parts[1]
         except IndexError:
-            raise ValueError(f'Invalid purl format: {purl}')
+            # This should never happen with split(), but just in case
+            raise ValueError(
+                f'Invalid PURL format: "{purl}". It must include a version (e.g., pkg:type/name@version)'
+            )

282-290: Simplify _format_plain_output with conditional expression

The implementation of _format_plain_output can be simplified using a conditional expression rather than a multi-line return statement.

     def _format_plain_output(self) -> str:
         """
         Format the scan output data into a plain text string
         """
-        return (
-            json.dumps(self.cryptography.results, indent=2)
-            if isinstance(self.cryptography.results, dict)
-            else str(self.cryptography.results)
-        )
+        return json.dumps(self.cryptography.results, indent=2) if isinstance(self.cryptography.results, dict) else str(self.cryptography.results)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fc961b and 8e18dd0.

📒 Files selected for processing (1)
  • src/scanoss/cryptography.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/scanoss/cryptography.py (5)
src/scanoss/scanossbase.py (1)
  • ScanossBase (28-107)
src/scanoss/scanossgrpc.py (5)
  • get_crypto_algorithms_in_range_for_purl (596-611)
  • get_crypto_algorithms_for_purl (579-594)
  • get_encryption_hints_in_range_for_purl (630-645)
  • get_encryption_hints_for_purl (613-628)
  • get_versions_in_range_for_purl (647-662)
src/scanoss/utils/abstract_presenter.py (1)
  • AbstractPresenter (6-103)
src/scanoss/utils/file.py (1)
  • validate_json_file (44-84)
src/scanoss/cli.py (1)
  • print_stderr (80-84)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (4)
src/scanoss/cryptography.py (4)

126-129: Consistent validation approach for missing PURLs

Good implementation of early validation for missing PURLs using a custom exception, which was suggested in a previous review.


292-302: Good use of NotImplementedError for unimplemented format methods

The implementation correctly raises NotImplementedError for output formats that aren't supported yet, following the contract defined in the AbstractPresenter class. This is a good practice to clearly indicate methods that need implementation.


66-66: 🛠️ Refactor suggestion

Remove unnecessary return statement in post_init

The __post_init__ method is called automatically after object initialization and shouldn't return a value as it won't be used anywhere. Remove this return statement.

-            return input_file_validation.data

Likely an incorrect or invalid review comment.


222-232:

❌ Incorrect review comment

Consider type safety when building the request from purls

The purl field in CryptographyConfig is declared as List[str], but there's no typecasting to ensure it's always a list. If a single string was incorrectly passed (e.g., due to a missing comma in CLI), each character might be treated as a separate PURL.

Verify that the purl field is always a list by adding a check and typecasting:


🏁 Script executed:

#!/bin/bash
# Search for usages of the purl field in the codebase
rg "self\.config\.purl" --include "*.py" -A 2 -B 2

Length of output: 494


Let’s rerun the searches without the unsupported --include flag and also locate where CryptographyConfig and the CLI --purl argument are defined:

#!/bin/bash
# 1. Search for usages of self.config.purl
rg "self\\.config\\.purl"

# 2. Locate the CryptographyConfig class
rg "class CryptographyConfig" -n .

# 3. If found, show lines defining the purl field
CONFIG_FILE=$(rg "class CryptographyConfig" -l . | head -n1)
if [[ -n "$CONFIG_FILE" ]]; then
  echo "purl in $CONFIG_FILE:"
  grep -n "purl" "$CONFIG_FILE"
fi

# 4. Find the CLI parser definition for --purl
rg "add_argument.*--purl" -n .

Let’s broaden our search for any purl occurrences, locate the CryptographyConfig definition, and inspect the CLI parser setup:


🏁 Script executed:

#!/bin/bash
# 1. Search globally for any 'purl' references
echo "=== All 'purl' occurrences ==="
rg -n "purl"

# 2. Locate the CryptographyConfig class file
CONFIG_FILE=$(rg -l "class CryptographyConfig" | head -n1)
if [[ -n "$CONFIG_FILE" ]]; then
  echo "=== Inspecting $CONFIG_FILE for purl field ==="
  sed -n '1,200p' "$CONFIG_FILE" | rg -n "purl"
else
  echo "CryptographyConfig class not found."
fi

# 3. Find the CLI parser definition for --purl
echo "=== CLI parser definitions for --purl ==="
rg -n "add_argument.*--purl"

Length of output: 63442


To confirm that self.config.purl is always a list in the Cryptography commands, let’s inspect:

#!/bin/bash
# 1. Show the CLI subparser for cryptography to verify purl’s argparse configuration
echo "=== CLI around cryptography (lines 340–430) ==="
sed -n '340,430p' src/scanoss/cli.py

# 2. Show how CryptographyConfig is defined and how _build_purls_request uses self.config.purl
echo "=== CryptographyConfig & _build_purls_request (lines 1–120 & 180–260) ==="
sed -n '1,120p' src/scanoss/cryptography.py
sed -n '180,260p' src/scanoss/cryptography.py

No type‐safety change required for purl in CryptographyConfig
The --purl option in the Crypto commands is defined with nargs='*', so args.purl will always be a List[str] (or None when not used, which is correctly handled by the input‐vs‐purl logic). There is no scenario where a lone string would be iterated character‐by‐character in _build_purls_request, so no additional typecasting is needed.

Likely an incorrect or invalid review comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant