fix: replace bare raise with descriptive ValueError in get_uploader() (#6568)#6588
fix: replace bare raise with descriptive ValueError in get_uploader() (#6568)#6588fazalpsinfo-cmyk wants to merge 1 commit into
Conversation
Two bare `raise` statements were outside any exception handler, causing RuntimeError instead of a meaningful error. Replaced both with ValueError. Closes crewAIInc#6568
📝 WalkthroughWalkthroughUpdated ChangesUploader error handling
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py`:
- Around line 199-202: The Bedrock S3 uploader factory validates only whether
bucket_name exists in kwargs, allowing None or empty values through. Update the
bucket validation guard in the Bedrock uploader selection logic to reject any
falsy bucket_name value, while preserving acceptance of a valid
environment-provided bucket or non-empty bucket_name kwarg.
- Line 219: Update cleanup_provider_files and FileResolver._get_uploader callers
of get_uploader to match its ValueError contract: replace None-based
unsupported-provider fallback checks with explicit ValueError handling or
intentional propagation, preserving each caller’s expected cleanup/resolution
behavior. Revise the get_uploader docstring to state that unsupported providers
raise ValueError rather than return None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b5d5f44-8373-45b0-83dc-96e06d2d73e9
📒 Files selected for processing (1)
lib/crewai-files/src/crewai_files/uploaders/factory.py
| raise ValueError( | ||
| "Bedrock S3 uploader requires CREWAI_BEDROCK_S3_BUCKET " | ||
| "environment variable or bucket_name kwarg" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the bucket value, not only the kwarg’s presence.
When bucket_name=None or bucket_name="" is passed, "bucket_name" in kwargs bypasses this guard, so the factory returns an uploader without a usable bucket and the error is deferred until later use. Check the value instead, e.g. not kwargs.get("bucket_name").
Based on the supplied Bedrock uploader contract, missing bucket values should fail during uploader selection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py` around lines 199 -
202, The Bedrock S3 uploader factory validates only whether bucket_name exists
in kwargs, allowing None or empty values through. Update the bucket validation
guard in the Bedrock uploader selection logic to reject any falsy bucket_name
value, while preserving acceptance of a valid environment-provided bucket or
non-empty bucket_name kwarg.
|
|
||
| logger.debug(f"No file uploader available for provider: {provider}") | ||
| raise | ||
| raise ValueError(f"Unsupported file uploader provider: {provider}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Update callers for the new exception contract.
cleanup_provider_files and FileResolver._get_uploader currently call get_uploader() and handle an unsupported provider by checking for None. This new exception bypasses those fallback paths and can make cleanup or resolution fail unexpectedly. Update those callers to handle or intentionally propagate ValueError, and revise the docstring that still promises None for unsupported providers.
Based on the supplied downstream snippets, this changes an existing cross-file API contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai-files/src/crewai_files/uploaders/factory.py` at line 219, Update
cleanup_provider_files and FileResolver._get_uploader callers of get_uploader to
match its ValueError contract: replace None-based unsupported-provider fallback
checks with explicit ValueError handling or intentional propagation, preserving
each caller’s expected cleanup/resolution behavior. Revise the get_uploader
docstring to state that unsupported providers raise ValueError rather than
return None.
1 participant
Description
Two bare
raisestatements inget_uploader()were outside any exception handler:This caused
RuntimeError: No active exception to re-raiseinstead of a meaningful error.Fix
Replaced both bare
raisestatements with descriptiveValueErrormessages that clearly indicate the configuration problem.Closes #6568
Support my work: https://buymeacoffee.com/muhamedfazalps