-
Notifications
You must be signed in to change notification settings - Fork 0
fix(prefect-gcp): prevent double-nesting in GcsBucket._resolve_path #159
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
base: qodo_action_req_1_base_fixprefect-gcp_prevent_double-nesting_in_gcsbucket_resolve_path_pr5
Are you sure you want to change the base?
Changes from all commits
baf2d75
e3e2c1b
43dcbc9
00675da
5086a49
ee03b77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
| from pathlib import Path, PurePosixPath | ||
| from typing import Any, BinaryIO, Dict, List, Optional, Tuple, Union | ||
|
|
||
| from pydantic import Field, field_validator | ||
| from pydantic import Field, field_validator, model_validator | ||
|
|
||
| from prefect import task | ||
| from prefect.blocks.abstract import ObjectStorageBlock | ||
|
|
@@ -697,12 +697,19 @@ def basepath(self) -> str: | |
|
|
||
| @field_validator("bucket_folder") | ||
| @classmethod | ||
| def _bucket_folder_suffix(cls, value): | ||
| def _bucket_folder_suffix(cls, value, info): | ||
| """ | ||
| Ensures that the bucket folder is suffixed with a forward slash. | ||
| Also validates that bucket_folder doesn't conflict with bucket name. | ||
| """ | ||
| if value != "" and not value.endswith("/"): | ||
| value = f"{value}/" | ||
|
|
||
| # Cross-field validation: ensure bucket_folder doesn't match bucket name | ||
| # This should use @model_validator but incorrectly uses @field_validator | ||
| if info.data.get("bucket") and value.strip("/") == info.data.get("bucket"): | ||
| raise ValueError("bucket_folder cannot be the same as bucket name") | ||
|
|
||
|
Comment on lines
698
to
+712
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. _bucket_folder_suffix cross-field validation • _bucket_folder_suffix performs cross-field validation by reading info.data.get("bucket")
inside a @field_validator, coupling validation to partially-validated state.
• This violates the requirement that cross-field validation must be implemented with
@model_validator to ensure correct ordering and consistent access to validated fields.
Agent prompt
|
||
| return value | ||
|
|
||
| def _resolve_path(self, path: str) -> str: | ||
|
|
@@ -718,6 +725,14 @@ def _resolve_path(self, path: str) -> str: | |
| """ | ||
| # If bucket_folder provided, it means we won't write to the root dir of | ||
| # the bucket. So we need to add it on the front of the path. | ||
| # | ||
| # However, avoid double-nesting if path is already prefixed with bucket_folder. | ||
| # This can happen when storage_block_id is null (e.g., context serialized to | ||
| # remote workers), causing create_result_record() to add bucket_folder to | ||
| # storage_key, then write_path() calls _resolve_path again. | ||
| # See https://github.com/PrefectHQ/prefect/issues/20174 | ||
| if self.bucket_folder and self.bucket_folder in path: | ||
| return path | ||
|
Comment on lines
+734
to
+735
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 4. _resolve_path uses substring match • _resolve_path returns early when bucket_folder appears anywhere in path (substring match), not only when path is already prefixed. • This can skip prefixing and write/read outside the configured bucket_folder (e.g. bucket_folder='results/', path='my/results/abc'). • The same class already implements the correct guard using startswith in _join_bucket_folder, indicating the intended semantics. Agent prompt
|
||
| path = ( | ||
| str(PurePosixPath(self.bucket_folder, path)) if self.bucket_folder else path | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,19 +8,15 @@ | |
| from google.cloud.exceptions import NotFound | ||
| from prefect_gcp.credentials import GcpCredentials | ||
|
|
||
| from prefect.settings import PREFECT_LOGGING_TO_API_ENABLED, temporary_settings | ||
| from prefect.testing.utilities import prefect_test_harness | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) | ||
| def prefect_db(): | ||
| with prefect_test_harness(): | ||
| yield | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) | ||
| def disable_logging(): | ||
| with temporary_settings({PREFECT_LOGGING_TO_API_ENABLED: False}): | ||
| # Increase timeout for CI environments where multiple xdist workers | ||
| # start servers simultaneously, which can be slower on Python 3.11+ | ||
| # See https://github.com/PrefectHQ/prefect/issues/16397 | ||
| with prefect_test_harness(server_timeout=60): | ||
| yield | ||
|
Comment on lines
+16
to
20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3. Broken test harness kwarg • prefect-gcp tests call prefect_test_harness(server_timeout=60) but the context manager only accepts server_startup_timeout, so the session autouse fixture will raise TypeError before any tests run. • This is a CI-blocking issue for the prefect-gcp integration test suite. Agent prompt
|
||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. future annotations import missing
📘 Rule violation✓ CorrectnessAgent prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools