fix(api): surface subscription deletion errors to users#24
Conversation
Previously, when rebuilding a trigger subscription, errors from the unsubscribe operation were silently caught and logged without propagating to the user. This left users unaware of failures during subscription management. Changes: - Check UnsubscribeResult.success and raise ValueError with the error message when unsubscribe fails - Simplify the rebuild logic by removing unnecessary try/except wrapper - Refactor update API to use cleaner conditional logic - Remove redundant test cases that tested silent error handling
… TriggerSubscriptionUpdateRequest - Introduced a model validator in TriggerSubscriptionUpdateRequest to enforce that at least one of the fields (name, credentials, parameters, properties) must be provided. - Refactored the TriggerSubscriptionUpdateApi to use the validated request object and simplified the logic for updating subscriptions based on the credential type. - Updated the credential type check in TriggerProviderService to use a set for better performance and clarity.
…i and TriggerProviderService
…ptionUpdateApi and TriggerProviderService
…oviderService - Deleted the test for rebuilding trigger subscriptions with unsupported credential types, as it was deemed unnecessary. - This change helps streamline the test suite by focusing on relevant scenarios.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| subscription = TriggerProviderService.get_subscription_by_id( | ||
| tenant_id=tenant_id, | ||
| subscription_id=subscription_id, | ||
| ) | ||
| if not subscription: | ||
| raise ValueError(f"Subscription {subscription_id} not found") | ||
|
|
||
| # Commit the transaction | ||
| session.commit() | ||
| credential_type = CredentialType.of(subscription.credential_type) |
There was a problem hiding this comment.
Bug: The rebuild_trigger_subscription method incorrectly passes the literal HIDDEN_VALUE string to external APIs during partial credential updates, instead of merging with the existing stored credential.
Severity: CRITICAL
Suggested Fix
Reinstate the logic to merge credentials in rebuild_trigger_subscription. Before calling TriggerManager.subscribe_trigger, iterate through the incoming credentials dictionary. If a value is HIDDEN_VALUE, replace it with the corresponding value from the original, decrypted subscription.credentials.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: api/services/trigger/trigger_provider_service.py#L871-L878
Potential issue: The `rebuild_trigger_subscription` service method no longer merges
existing credentials when a request contains `HIDDEN_VALUE` placeholders for fields that
are not being changed. Instead, it passes the credentials object, including the literal
`HIDDEN_VALUE` string, directly to the `TriggerManager`. This will cause subscription
rebuilds to fail when a user performs a partial credential update, as the external
provider will receive an invalid credential value.
Did we get this right? 👍 / 👎 to inform future reviews.
| user_id=user_id, | ||
| provider_id=provider_id, | ||
| subscription=subscription.to_entity(), | ||
| credentials=subscription.credentials, |
There was a problem hiding this comment.
Bug: The rebuild_trigger_subscription method is missing error handling for the unsubscribe_trigger call. A failure in this external API call will now crash the entire operation.
Severity: HIGH
Suggested Fix
Wrap the TriggerManager.unsubscribe_trigger call within a try...except Exception block, similar to the implementation in delete_trigger_provider. Log the exception for debugging purposes but do not re-raise it, allowing the rebuild process to continue to the subscription step.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: api/services/trigger/trigger_provider_service.py#L889
Potential issue: The `rebuild_trigger_subscription` method no longer gracefully handles
exceptions from the `TriggerManager.unsubscribe_trigger` call. Previously, this
operation was wrapped in a `try...except` block to allow the process to continue even if
the unsubscribe call failed. Now, any failure in the external API call will raise an
unhandled exception, halting the entire rebuild process and leaving the subscription in
an inconsistent state. This behavior is inconsistent with other methods like
`delete_trigger_provider` which still handle this error gracefully.
Did we get this right? 👍 / 👎 to inform future reviews.
| # For the rest cases(API_KEY, OAUTH2) | ||
| # we need to call third party provider(e.g. GitHub) to rebuild the subscription | ||
| TriggerProviderService.rebuild_trigger_subscription( | ||
| tenant_id=user.current_tenant_id, | ||
| name=request.name, | ||
| provider_id=provider_id, | ||
| subscription_id=subscription_id, | ||
| credentials=request.credentials or subscription.credentials, | ||
| parameters=request.parameters or subscription.parameters, | ||
| ) | ||
| return 200 |
There was a problem hiding this comment.
Bug: Updating properties on a manually created (UNAUTHORIZED) subscription without also changing its name incorrectly calls rebuild_trigger_subscription, causing a ValueError due to an unsupported credential type.
Severity: HIGH
Suggested Fix
Modify the conditional logic in the controller. Before falling through to rebuild_trigger_subscription, add a check for CredentialType.UNAUTHORIZED. If the subscription is of this type, route the request to update_trigger_subscription to handle name and property updates, preventing the call to the incompatible rebuild service.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: api/controllers/console/workspace/trigger_providers.py#L368-L378
Potential issue: When updating a manually created subscription (type `UNAUTHORIZED`), if
the update request modifies properties but does not change the name, the request is
incorrectly routed to the `rebuild_trigger_subscription` service. This service contains
a validation check that explicitly rejects `UNAUTHORIZED` credential types, causing a
`ValueError`. The correct behavior would be to route such updates to
`update_trigger_subscription`, which can handle property-only changes for these
subscription types.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
Benchmark PR from qodo-benchmark#181