feat: Add option to modify how a query is printed by MattMonk · Pull Request #54 · snakemake/snakemake-interface-storage-plugins

MattMonk · 2024-09-18T08:53:29Z

This adds an attribute to storage_object called print_query which allows a storage provider to change how a query is printed (e.g. removing senstitive information) through a new method safe_print() which by default just returns the query.

Related to issue snakemake/snakemake#3087 and required for PR snakemake/snakemake#3089.

Summary by CodeRabbit

New Features
- Added a method to safely print query strings, designed to prevent accidental exposure of sensitive information.
Bug Fixes
- Improved error reporting by enhancing the clarity of exception messages related to storage operations.

coderabbitai · 2024-09-18T08:53:39Z

📝 Walkthrough

Walkthrough

The changes introduced in this pull request involve modifications to error handling in several asynchronous methods of the StorageObjectRead class in snakemake_interface_storage_plugins/storage_object.py. The error messages raised by WorkflowError have been updated to enhance traceback clarity by using the from e syntax. Additionally, a new method safe_print has been added to both the StorageProviderBase class in snakemake_interface_storage_plugins/storage_provider.py and the StorageProvider class in README.md, aimed at processing queries to remove potentially sensitive information before printing.

Changes

File Path	Change Summary
`snakemake_interface_storage_plugins/storage_object.py`	Updated error handling in `managed_size`, `managed_mtime`, `managed_exists`, `managed_retrieve`, and `managed_touch` methods of `StorageObjectRead` class. Removed original exception from `WorkflowError` and used `from e` syntax.
`snakemake_interface_storage_plugins/storage_provider.py`, `README.md`	Added `safe_print(self, query: str) -> str` method to `StorageProviderBase` and `StorageProvider` classes for processing queries to remove sensitive information.

Possibly related PRs

feat: add postprocess_query method for optionally modifying the query before a storage object is created #51: The addition of the postprocess_query method in snakemake_interface_storage_plugins/storage_provider.py relates to the main PR as both involve handling query strings, enhancing the overall query management in the storage operations.

📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8c31d33 and 5cd2520.

📒 Files selected for processing (4)

snakemake_interface_storage_plugins/storage_object.py (5 hunks)
snakemake_interface_storage_plugins/storage_provider.py (1 hunks)
snakemake_interface_storage_plugins/storage_object.py (3 hunks)
README.md (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

snakemake_interface_storage_plugins/storage_object.py

🧰 Additional context used

📓 Path-based instructions (1)

`**/*.py`: Do not try to improve formatting. Do not suggest ...

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

snakemake_interface_storage_plugins/storage_provider.py
snakemake_interface_storage_plugins/storage_object.py

🪛 Ruff (0.8.2)

snakemake_interface_storage_plugins/storage_object.py

195-197: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🔇 Additional comments (3)

snakemake_interface_storage_plugins/storage_provider.py (1)

144-150: Implementation looks good.

The safe_print method provides a good default implementation that simply returns the query, while allowing storage providers to override it with their own implementation to handle sensitive information.

README.md (1)

119-124: README documentation is clear and consistent.

The documentation explains the purpose of the safe_print method clearly, and is consistent with the implementation in the storage_provider.py file.

snakemake_interface_storage_plugins/storage_object.py (1)

71-71: Implementation of print_query looks good.

Adding self.print_query as an attribute initialized with self.provider.safe_print(self.query) allows for consistent use of the sanitized query throughout the class.

✨ Finishing Touches

📝 Generate Docstrings

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

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

coderabbitai

Actionable comments posted: 7

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 19a5f80 and ed2e41e.

Files selected for processing (2)

snakemake_interface_storage_plugins/storage_object.py (5 hunks)
snakemake_interface_storage_plugins/storage_provider.py (1 hunks)

Additional context used

Path-based instructions (2)

snakemake_interface_storage_plugins/storage_provider.py (1)

Pattern **/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

snakemake_interface_storage_plugins/storage_object.py (1)

Pattern **/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Ruff

snakemake_interface_storage_plugins/storage_object.py

147-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

154-154: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

161-161: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

193-193: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

200-200: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

223-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Additional comments not posted (2)

snakemake_interface_storage_plugins/storage_provider.py (1)

144-150: Excellent addition for enhancing security!

The new safe_print method provides a valuable mechanism for storage providers to remove potentially sensitive information from queries before printing them. This is particularly useful for URL-like queries that may contain authentication tokens, usernames, or passwords.

By default, the method simply returns the original query, allowing storage providers to opt-in to this feature and implement their own custom logic for removing sensitive data. This approach maintains backward compatibility while encouraging better security practices.

This addition aligns well with the PR objective of enhancing the handling of query outputs to improve security and privacy.

snakemake_interface_storage_plugins/storage_object.py (1)

71-71: LGTM!

The addition of the print_query attribute aligns with the PR objective of allowing storage providers to customize how a query is printed. It is assigned the value returned by self.provider.safe_print(self.query), which is expected to return a sanitized version of the query, potentially removing sensitive information.

snakemake_interface_storage_plugins/storage_object.py

snakemake_interface_storage_plugins/storage_provider.py

coderabbitai

Actionable comments posted: 1

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 026cb90 and 1e47e58.

📒 Files selected for processing (1)

snakemake_interface_storage_plugins/storage_object.py (5 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

`**/*.py`: Do not try to improve formatting. Do not suggest ...

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

snakemake_interface_storage_plugins/storage_object.py

🪛 GitHub Actions: CI

snakemake_interface_storage_plugins/storage_object.py

[error] 178-178: E999 SyntaxError: unmatched ')'

🔇 Additional comments (7)

snakemake_interface_storage_plugins/storage_object.py (7)

71-71: Proper implementation of safe_print feature.

This new line adds the print_query attribute that utilizes the provider's safe_print method as described in the PR objectives. This will allow storage providers to customize how the query is printed, which is particularly useful for hiding sensitive information.

147-147: Good error handling implementation.

The change from self.query to self.print_query in the error message aligns with the PR objective to use the safe representation of the query in exception handling. The from e syntax for chaining exceptions is correctly used.

154-154: Good error handling implementation.

The change from self.query to self.print_query in the error message aligns with the PR objective to use the safe representation of the query in exception handling. The from e syntax for chaining exceptions is correctly used.

161-161: Good error handling implementation.

The change from self.query to self.print_query in the error message aligns with the PR objective to use the safe representation of the query in exception handling. The from e syntax for chaining exceptions is correctly used.

193-195: Use raise ... from e to distinguish the exception from errors in exception handling.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

To improve the clarity of the exception, use raise ... from e:
            raise WorkflowError(
-                f"Failed to remove storage object {self.print_query}", e
+                f"Failed to remove storage object {self.print_query}") from e
            )
202-204: Use raise ... from e to distinguish the exception from errors in exception handling.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

To improve the clarity of the exception, use raise ... from e:
            raise WorkflowError(
-                f"Failed to store output in storage {self.print_query}", e
+                f"Failed to store output in storage {self.print_query}") from e
            )
227-227: Good error handling implementation.

The change from self.query to self.print_query in the error message aligns with the PR objective to use the safe representation of the query in exception handling. The from e syntax for chaining exceptions is correctly used.

snakemake_interface_storage_plugins/storage_object.py

johanneskoester

Very good catch! We also have to use print_query in the main snakemake whenever storage_object.query is accessed for logging or exception handling there. Can you create a follow-up PR once this is merged and released?

Further, can you update the example skeleton in the readme in this PR and after the release the poetry-plugin-snakemake repo to contain the method in the skeleton?

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

MattMonk · 2025-03-11T09:22:22Z

Thanks @johanneskoester! I've updated the README and would be happy to open a PR for the poetry plugin. This PR snakemake/snakemake#3089 starts to replace things with print_query but I now see that there are a lot of other places that storage_object.query pop-up so I will start going through the rest!

johanneskoester · 2025-03-12T11:02:13Z

Thanks a lot!

Adds the methods `postprocess_query` and `safe_print` to the storage plugin scaffolding. Depends on snakemake/snakemake-interface-storage-plugins#54.  ## Summary by CodeRabbit - **New Features** - Introduced customizable query processing to support query normalization. - Added secure logging for handling queries safely without exposing sensitive details.

Adds the new safe_print method (snakemake/snakemake-interface-storage-plugins#54) to censor information in the XRootD URL.  ## Summary by CodeRabbit - **New Features** - Introduced secure URL printing functionality that ensures sensitive information is protected when URLs are displayed.

MattMonk mentioned this pull request Sep 18, 2024

feat: Add option to print redacted file names snakemake/snakemake#3089

Merged

2 tasks

coderabbitai bot reviewed Sep 18, 2024

View reviewed changes

MattMonk force-pushed the main branch 2 times, most recently from 7afeb7a to 026cb90 Compare September 21, 2024 09:28

fgvieira reviewed Sep 21, 2024

View reviewed changes

snakemake_interface_storage_plugins/storage_provider.py Show resolved Hide resolved

chrisburr added this to Snakemake Hackathon March 2025 Mar 10, 2025

chrisburr assigned chrisburr and MattMonk Mar 10, 2025

chrisburr moved this to In progress in Snakemake Hackathon March 2025 Mar 10, 2025

coderabbitai bot reviewed Mar 10, 2025

View reviewed changes

snakemake_interface_storage_plugins/storage_object.py Outdated Show resolved Hide resolved

chrisburr force-pushed the main branch from 1e47e58 to 8c31d33 Compare March 10, 2025 12:42

chrisburr moved this from In progress to In review in Snakemake Hackathon March 2025 Mar 10, 2025

chrisburr approved these changes Mar 10, 2025

View reviewed changes

johanneskoester requested changes Mar 10, 2025

View reviewed changes

MattMonk and others added 3 commits March 11, 2025 10:09

Add option to modify how a query is printed

7b19dcf

Apply suggestions from code review

7361b53

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Update the README to include the new safe_print method

5cd2520

MattMonk force-pushed the main branch from 8c31d33 to 5cd2520 Compare March 11, 2025 09:16

MattMonk mentioned this pull request Mar 11, 2025

feat: Add postprocess_query and safe_print to the storage scaffold snakemake/poetry-snakemake-plugin#35

Merged

chrisburr requested a review from johanneskoester March 12, 2025 09:06

johanneskoester approved these changes Mar 12, 2025

View reviewed changes

johanneskoester merged commit 0634a9e into snakemake:main Mar 12, 2025
6 checks passed

github-project-automation bot moved this from In review to Done in Snakemake Hackathon March 2025 Mar 12, 2025

github-actions bot mentioned this pull request Mar 12, 2025

chore(main): release 3.4.0 #53

Merged

coderabbitai bot mentioned this pull request Mar 13, 2025

feat: Allow default configuration of retrieve behaviour #60

Merged

MattMonk mentioned this pull request Mar 14, 2025

feat: Implement new safe_print method snakemake/snakemake-storage-plugin-xrootd#32

Merged

Conversation

MattMonk commented Sep 18, 2024 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related PRs

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johanneskoester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattMonk commented Mar 11, 2025

Uh oh!

Uh oh!

johanneskoester commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MattMonk commented Sep 18, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 18, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

johanneskoester left a comment •

edited

Loading