fix(admin): include context in bucket error messages#4777
fix(admin): include context in bucket error messages#4777
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add bucket name and SNS ARN to error messages across add/update/remove so callers can identify which resource caused the failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ## unreleased | ||
|
|
||
| ### Python API |
There was a problem hiding this comment.
The CHANGELOG entry links to #4776, but this change is being introduced in PR #4777. This will result in a broken or misleading reference in the project history.
| ### Python API | |
| * [Fixed] `quilt3.admin.buckets.add/update/remove` now include the bucket name (and SNS ARN where applicable) in error messages, making failures easier to debug ([#4777](https://github.com/quiltdata/quilt/pull/4777)) |
api/python/quilt3/admin/buckets.py
Outdated
| raise exceptions.Quilt3AdminError(result.message) | ||
| if isinstance(result, _graphql_client.BucketAddBucketAddSnsInvalid): | ||
| raise exceptions.Quilt3AdminError("Invalid SNS notification ARN") | ||
| raise exceptions.Quilt3AdminError(f"Invalid SNS notification ARN: {sns_notification_arn!r}") |
There was a problem hiding this comment.
None when no ARN was provided
When sns_notification_arn is None (not passed by the caller), the error messages become "Invalid SNS notification ARN: None" and "Notification topic not found: None". The string None suggests the user explicitly passed None as the ARN value, which is misleading. This is the same pattern that BucketNotFoundError avoids by using a conditional.
Consider following the same pattern for consistency:
if isinstance(result, _graphql_client.BucketAddBucketAddSnsInvalid):
arn_info = f": {sns_notification_arn!r}" if sns_notification_arn else ""
raise exceptions.Quilt3AdminError(f"Invalid SNS notification ARN{arn_info}")This also applies to the NotificationTopicNotFound case on line 104, and to the equivalent branches in _handle_bucket_update_result (lines 182 and 186).
There was a problem hiding this comment.
Fixed — SNS/ARN suffix now omitted when ARN is None, using the same conditional pattern as BucketNotFoundError.
| class BucketNotFoundError(Quilt3AdminError): | ||
| def __init__(self): | ||
| super().__init__(None) | ||
| def __init__(self, name: str = None): |
There was a problem hiding this comment.
name parameter
name: str = None is technically incorrect — a parameter with a default of None should be annotated as Optional[str] (or str | None in Python 3.10+) to accurately reflect that None is a valid value. Mypy and similar type checkers will flag this as an error. Since the file doesn't currently import typing, the simplest fix is to use the union syntax:
| def __init__(self, name: str = None): | |
| def __init__(self, name: str | None = None): |
There was a problem hiding this comment.
Fixed — updated to str | None = None.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #4777 +/- ##
===========================================
- Coverage 44.33% 28.55% -15.78%
===========================================
Files 813 724 -89
Lines 32734 24677 -8057
Branches 5723 5723
===========================================
- Hits 14511 7046 -7465
+ Misses 16220 15628 -592
Partials 2003 2003
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
quilt3.admin.buckets.add/update/removenow include the bucket name in all error messages (e.g.BucketAlreadyAdded,BucketNotFound,IndexingInProgress)SnsInvalidandNotificationTopicNotFounderrorsBucketNotFoundError(previously raised withNonemessage) now surfaces the bucket nametabulator.list_tablespasses the bucket name toBucketNotFoundErrorTest plan
test_admin_api.pytests updated and passing (76/77 pass; 1 pre-existing unrelated pydantic failure intest_api_keys_list)quilt3.admin.buckets.addwith a nonexistent bucket🤖 Generated with Claude Code
Greptile Summary
This PR improves error message quality across
quilt3.admin.bucketsby threading contextual information (bucket name and SNS ARN) through the internal result-handler helpers so failures are easier to diagnose. It also fixes a pre-existing bug intabulator.list_tableswhereBucketNotFoundErrorwas raised as a bare class reference rather than an instantiated exception.Key changes:
_handle_bucket_add/update/remove_resulthelpers now acceptnameandsns_notification_arnand embed them in relevant error messagesBucketNotFoundErrorgains an optionalnameparameter and surfaces it in the message (gracefully omitting it whenNone)tabulator.list_tablesnow correctly instantiatesBucketNotFoundError(bucket_name)instead of raising the class itself#4776instead of#4777)Confidence Score: 4/5
str = Nonevsstr | None = None), and a minor UX inconsistency where SNS error messages display the literal stringNonewhen no ARN was supplied — the same pattern thatBucketNotFoundErroralready handles conditionally. None of these affect runtime correctness.Important Files Changed
nameandsns_notification_arncontext and include them in error messages. Logic is correct; minor UX issue when SNS ARN isNone.BucketNotFoundErrornow accepts an optional bucket name and conditionally includes it in the message. Type annotationname: str = Noneshould be `strBucketNotFoundErrorwas raised as a class reference instead of an instantiated exception; now correctly passesbucket_name.Nonewhen no ARN is passed in the test setup.unreleasedsection added with accurate description; links to PR#4776instead of the correct#4777.Sequence Diagram
sequenceDiagram participant Caller participant buckets.py participant GraphQL Client participant _handle_result Caller->>buckets.py: add(name, title, sns_notification_arn=...) buckets.py->>GraphQL Client: bucket_add(input) GraphQL Client-->>buckets.py: result (union type) buckets.py->>_handle_result: _handle_bucket_add_result(result, name, sns_notification_arn) alt BucketAddSuccess _handle_result-->>Caller: Bucket else BucketAlreadyAdded _handle_result-->>Caller: Quilt3AdminError("Bucket already added: {name!r}") else BucketDoesNotExist _handle_result-->>Caller: Quilt3AdminError("Bucket does not exist in S3: {name!r}") else SnsInvalid / NotificationTopicNotFound _handle_result-->>Caller: Quilt3AdminError("...SNS...: {sns_notification_arn!r}") end Caller->>buckets.py: update(name, title, ...) buckets.py->>GraphQL Client: bucket_update(name, input) GraphQL Client-->>buckets.py: result buckets.py->>_handle_result: _handle_bucket_update_result(result, name, sns_notification_arn) alt BucketNotFound _handle_result-->>Caller: BucketNotFoundError(name) end Caller->>buckets.py: remove(name) buckets.py->>GraphQL Client: bucket_remove(name) GraphQL Client-->>buckets.py: result buckets.py->>_handle_result: _handle_bucket_remove_result(result, name) alt BucketNotFound _handle_result-->>Caller: BucketNotFoundError(name) else IndexingInProgress _handle_result-->>Caller: Quilt3AdminError("...indexing...: {name!r}") endLast reviewed commit: "docs(changelog): not..."