fix(prefect-gcp): prevent double-nesting in GcsBucket._resolve_path#5
Conversation
When storage_block_id is null (e.g., context serialized to Ray workers), create_result_record() adds bucket_folder to storage_key via _resolve_path. Then write_path() calls _resolve_path again, causing double-nested paths like "results/results/abc123" instead of "results/abc123". Add duplicate-prefix check to _resolve_path, matching the existing check in _join_bucket_folder. Fixes PrefectHQ#20174 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- fix .prefectignore content in test fixture (remove leading whitespace) - add filterwarnings to pyproject.toml to suppress upstream deprecation warnings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
remove extra disable_logging fixture and temporary_settings wrapper to match pattern used by prefect-aws and other integrations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fixes ephemeral server startup timeouts on Python 3.11+ with xdist workers see PrefectHQ#16397 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change fixes a bucket path resolution bug where Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/integrations/prefect-gcp/prefect_gcp/cloud_storage.py`:
- Around line 734-735: The code uses "if self.bucket_folder and
self.bucket_folder in path" which can incorrectly match substrings; change this
to a precise prefix check in the function/method that builds bucket paths
(referencing self.bucket_folder and the code branch that returns path) by
normalizing both values (strip leading/trailing slashes) and using equality or
startswith with a separator (e.g., path == bucket_folder or
path.startswith(bucket_folder + "/")) so only true prefixed paths are treated as
already prefixed; ensure you handle the empty or root cases and preserve
original slashes when returning.
In `@src/integrations/prefect-gcp/tests/test_cloud_storage.py`:
- Around line 143-165: The test test_resolve_path_no_double_nesting constructs
already_prefixed_path incorrectly because gcs_bucket.bucket_folder already ends
with a '/', producing a double-slash; update the construction of
already_prefixed_path in that test to avoid the extra slash (e.g., use
bucket_folder.rstrip('/') + '/abc123' or concatenate without adding another '/')
so the path mirrors real usage and then call gcs_bucket._resolve_path to assert
no double-nesting; references: test_resolve_path_no_double_nesting,
already_prefixed_path, gcs_bucket.bucket_folder, and _resolve_path.
🧹 Nitpick comments (1)
src/integrations/prefect-gcp/prefect_gcp/cloud_storage.py (1)
700-711: Use a model-level validator for the cross-field check.
Cross-field validation via a field validator relies on field ordering and leaves a confusing “incorrectly uses” comment in production code. Prefer a model-level validator and keep the field validator focused on suffixing.Suggested refactor
`@field_validator`("bucket_folder") `@classmethod` -def _bucket_folder_suffix(cls, value, info): +def _bucket_folder_suffix(cls, value): """ 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") - return value + +@model_validator(mode="after") +def _bucket_folder_not_bucket_name(self): + if self.bucket_folder.strip("/") == self.bucket: + raise ValueError("bucket_folder cannot be the same as bucket name") + return self
| if self.bucket_folder and self.bucket_folder in path: | ||
| return path |
There was a problem hiding this comment.
Avoid substring matches when detecting pre-prefixed paths.
Using self.bucket_folder in path can skip prefixing for unrelated paths that merely contain the folder name (e.g., my-results/file). This can write to the wrong location.
Proposed fix
- if self.bucket_folder and self.bucket_folder in path:
- return path
+ if self.bucket_folder:
+ bucket_prefix = self.bucket_folder.rstrip("/")
+ if path == bucket_prefix or path.startswith(f"{bucket_prefix}/"):
+ return path🤖 Prompt for AI Agents
In `@src/integrations/prefect-gcp/prefect_gcp/cloud_storage.py` around lines 734 -
735, The code uses "if self.bucket_folder and self.bucket_folder in path" which
can incorrectly match substrings; change this to a precise prefix check in the
function/method that builds bucket paths (referencing self.bucket_folder and the
code branch that returns path) by normalizing both values (strip
leading/trailing slashes) and using equality or startswith with a separator
(e.g., path == bucket_folder or path.startswith(bucket_folder + "/")) so only
true prefixed paths are treated as already prefixed; ensure you handle the empty
or root cases and preserve original slashes when returning.
| def test_resolve_path_no_double_nesting(self, gcs_bucket): | ||
| """ | ||
| Regression test for https://github.com/PrefectHQ/prefect/issues/20174 | ||
|
|
||
| When storage_block_id is null (e.g., context serialized to Ray workers), | ||
| create_result_record() adds bucket_folder to storage_key via _resolve_path. | ||
| Then write_path() calls _resolve_path again. Without the duplicate check, | ||
| this causes double-nested paths like "results/results/abc123". | ||
| """ | ||
| bucket_folder = gcs_bucket.bucket_folder | ||
| if not bucket_folder: | ||
| pytest.skip("Test only applies when bucket_folder is set") | ||
|
|
||
| # Simulate path that already has bucket_folder prefix | ||
| # (as would happen when create_result_record calls _resolve_path) | ||
| already_prefixed_path = f"{bucket_folder}/abc123" | ||
|
|
||
| # When write_path calls _resolve_path again, it should NOT double-nest | ||
| result = gcs_bucket._resolve_path(already_prefixed_path) | ||
|
|
||
| # Should return the same path, not bucket_folder/bucket_folder/abc123 | ||
| assert result == already_prefixed_path | ||
| assert not result.startswith(f"{bucket_folder}{bucket_folder}") |
There was a problem hiding this comment.
Avoid double slash in the already-prefixed test path.
bucket_folder is already suffixed with /, so the current construction yields base_folder//abc123. Use direct concatenation to match real usage.
Proposed fix
- already_prefixed_path = f"{bucket_folder}/abc123"
+ already_prefixed_path = f"{bucket_folder}abc123"🤖 Prompt for AI Agents
In `@src/integrations/prefect-gcp/tests/test_cloud_storage.py` around lines 143 -
165, The test test_resolve_path_no_double_nesting constructs
already_prefixed_path incorrectly because gcs_bucket.bucket_folder already ends
with a '/', producing a double-slash; update the construction of
already_prefixed_path in that test to avoid the extra slash (e.g., use
bucket_folder.rstrip('/') + '/abc123' or concatenate without adding another '/')
so the path mirrors real usage and then call gcs_bucket._resolve_path to assert
no double-nesting; references: test_resolve_path_no_double_nesting,
already_prefixed_path, gcs_bucket.bucket_folder, and _resolve_path.
4 participants
Benchmark PR from agentic-review-benchmarks#5
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests