fix(api): surface subscription deletion errors to users#8
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.
WalkthroughThis PR simplifies trigger subscription update flows by removing transactional locking and complex credential merging logic, replacing them with a streamlined unsubscribe-subscribe-update sequence. Runtime validation is added to ensure update requests modify at least one field. Changes
Sequence Diagram(s)sequenceDiagram participant Client participant Controller participant TriggerManager participant Database participant External Client->>Controller: Update subscription request Controller->>Controller: Validate at least one field Controller->>Database: Retrieve existing subscription Database-->>Controller: subscription data Controller->>TriggerManager: Unsubscribe old subscription TriggerManager->>External: Unsubscribe call External-->>TriggerManager: Success Controller->>TriggerManager: Subscribe with new subscription_id TriggerManager->>External: Subscribe call External-->>TriggerManager: Success Controller->>Database: Update subscription (name, parameters, credentials) Database-->>Controller: Updated Controller-->>Client: Response Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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
🤖 Fix all issues with AI agents
In `@api/controllers/console/workspace/trigger_providers.py`: - Around line 353-378: The variable manually_created (subscription.credential_type == CredentialType.UNAUTHORIZED) is unused and causes rebuild_trigger_subscription to be called for UNAUTHORIZED subscriptions (which fail); update the control flow so that when manually_created is true you call TriggerProviderService.update_trigger_subscription (passing tenant_id=user.current_tenant_id, subscription_id=subscription_id, name=request.name, properties=request.properties) and return 200 instead of calling TriggerProviderService.rebuild_trigger_subscription, keeping the existing rename branch (rename) intact so only non-manual, non-rename cases call TriggerProviderService.rebuild_trigger_subscription with credentials=request.credentials or subscription.credentials and parameters=request.parameters or subscription.parameters. In `@api/services/trigger/trigger_provider_service.py`: - Around line 882-911: The unsubscribe→subscribe→update sequence (calls to TriggerManager.unsubscribe_trigger, TriggerManager.subscribe_trigger and TriggerProviderService.update_trigger_subscription) is non-atomic and can leave the remote webhook deleted while the local DB still has stale data; wrap these three operations in a single transactional boundary (begin DB transaction before unsubscribe, perform unsubscribe, subscribe (using generate_plugin_trigger_endpoint_url and subscribing into TriggerSubscriptionEntity), then update_trigger_subscription) and commit only if all succeed, otherwise roll back the DB changes and, if unsubscribe already removed the remote webhook, attempt to re-subscribe in the error path to restore remote state or persist a compensating record; ensure errors during subscribe/update are caught, the transaction is rolled back, and any compensating re-subscribe attempts are idempotent and logged. 🧹 Nitpick comments (1)
api/controllers/console/workspace/trigger_providers.py (1)
341-341: Variablerequestshadows Flask'srequestimport.The local variable
requestshadows the Flaskrequestobject imported on line 5, which could cause confusion or bugs if the Flask request is needed later in this method.♻️ Suggested rename
- request = TriggerSubscriptionUpdateRequest.model_validate(console_ns.payload) + update_request = TriggerSubscriptionUpdateRequest.model_validate(console_ns.payload)Then update references on lines 354, 363, 364, 372, 375, 376 to use
update_request.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/controllers/console/workspace/trigger_providers.pyapi/services/trigger/trigger_provider_service.pyapi/tests/test_containers_integration_tests/services/test_trigger_provider_service.py
💤 Files with no reviewable changes (1)
- api/tests/test_containers_integration_tests/services/test_trigger_provider_service.py
🧰 Additional context used
🧬 Code graph analysis (2)
api/controllers/console/workspace/trigger_providers.py (1)
api/services/trigger/trigger_provider_service.py (3)
TriggerProviderService(45-911)update_trigger_subscription(220-312)rebuild_trigger_subscription(844-911)
api/services/trigger/trigger_provider_service.py (1)
api/models/trigger.py (1)
to_entity(86-92)
🔇 Additional comments (3)
api/services/trigger/trigger_provider_service.py (2)
871-876: LGTM - Clean subscription retrieval.Using
get_subscription_by_idcentralizes the lookup logic and returns decrypted credentials, which is appropriate for this use case.
878-880: Good use of set for membership check.Using a set literal
{CredentialType.OAUTH2, CredentialType.API_KEY}for theincheck is idiomatic and slightly more performant than a list.api/controllers/console/workspace/trigger_providers.py (1)
46-51: LGTM - Good runtime validation.The
model_validatorensures at least one field is provided, preventing no-op update requests at the API boundary.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines +353 to +378
| # For rename only, just update the name | ||
| rename = request.name is not None and not any((request.credentials, request.parameters, request.properties)) | ||
| # When credential type is UNAUTHORIZED, it indicates the subscription was manually created | ||
| # For Manually created subscription, they dont have credentials, parameters | ||
| # They only have name and properties(which is input by user) | ||
| manually_created = subscription.credential_type == CredentialType.UNAUTHORIZED | ||
| if rename: | ||
| TriggerProviderService.update_trigger_subscription( | ||
| tenant_id=user.current_tenant_id, | ||
| subscription_id=subscription_id, | ||
| name=args.name, | ||
| name=request.name, | ||
| properties=request.properties, | ||
| ) | ||
| return 200 | ||
| | ||
| # rebuild for create automatically by the provider | ||
| match subscription.credential_type: | ||
| case CredentialType.UNAUTHORIZED: | ||
| TriggerProviderService.update_trigger_subscription( | ||
| tenant_id=user.current_tenant_id, | ||
| subscription_id=subscription_id, | ||
| name=args.name, | ||
| properties=args.properties, | ||
| ) | ||
| return 200 | ||
| case CredentialType.API_KEY | CredentialType.OAUTH2: | ||
| if args.credentials: | ||
| new_credentials: dict[str, Any] = { | ||
| key: value if value != HIDDEN_VALUE else subscription.credentials.get(key, UNKNOWN_VALUE) | ||
| for key, value in args.credentials.items() | ||
| } | ||
| else: | ||
| new_credentials = subscription.credentials | ||
| | ||
| TriggerProviderService.rebuild_trigger_subscription( | ||
| tenant_id=user.current_tenant_id, | ||
| name=args.name, | ||
| provider_id=provider_id, | ||
| subscription_id=subscription_id, | ||
| credentials=new_credentials, | ||
| parameters=args.parameters or subscription.parameters, | ||
| ) | ||
| return 200 | ||
| case _: | ||
| raise BadRequest("Invalid credential type") | ||
| # 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.
Unused manually_created variable and potential bug for UNAUTHORIZED subscriptions.
The manually_created variable (line 358) is defined but never used. The comments indicate special handling for manually-created (UNAUTHORIZED) subscriptions, but the code doesn't implement it.
If a user updates properties on a manually-created subscription, rename will be False (since request.properties is not None), causing rebuild_trigger_subscription to be called. This will fail with "Credential type UNAUTHORIZED not supported for auto creation" from line 879 in trigger_provider_service.py.
🐛 Suggested fix
rename = request.name is not None and not any((request.credentials, request.parameters, request.properties)) # When credential type is UNAUTHORIZED, it indicates the subscription was manually created # For Manually created subscription, they dont have credentials, parameters # They only have name and properties(which is input by user) manually_created = subscription.credential_type == CredentialType.UNAUTHORIZED - if rename: + if rename or manually_created: TriggerProviderService.update_trigger_subscription( tenant_id=user.current_tenant_id, subscription_id=subscription_id, name=request.name, properties=request.properties, ) return 200📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # For rename only, just update the name | |
| rename = request.name is not None and not any((request.credentials, request.parameters, request.properties)) | |
| # When credential type is UNAUTHORIZED, it indicates the subscription was manually created | |
| # For Manually created subscription, they dont have credentials, parameters | |
| # They only have name and properties(which is input by user) | |
| manually_created = subscription.credential_type == CredentialType.UNAUTHORIZED | |
| if rename: | |
| TriggerProviderService.update_trigger_subscription( | |
| tenant_id=user.current_tenant_id, | |
| subscription_id=subscription_id, | |
| name=args.name, | |
| name=request.name, | |
| properties=request.properties, | |
| ) | |
| return 200 | |
| # rebuild for create automatically by the provider | |
| match subscription.credential_type: | |
| case CredentialType.UNAUTHORIZED: | |
| TriggerProviderService.update_trigger_subscription( | |
| tenant_id=user.current_tenant_id, | |
| subscription_id=subscription_id, | |
| name=args.name, | |
| properties=args.properties, | |
| ) | |
| return 200 | |
| case CredentialType.API_KEY | CredentialType.OAUTH2: | |
| if args.credentials: | |
| new_credentials: dict[str, Any] = { | |
| key: value if value != HIDDEN_VALUE else subscription.credentials.get(key, UNKNOWN_VALUE) | |
| for key, value in args.credentials.items() | |
| } | |
| else: | |
| new_credentials = subscription.credentials | |
| TriggerProviderService.rebuild_trigger_subscription( | |
| tenant_id=user.current_tenant_id, | |
| name=args.name, | |
| provider_id=provider_id, | |
| subscription_id=subscription_id, | |
| credentials=new_credentials, | |
| parameters=args.parameters or subscription.parameters, | |
| ) | |
| return 200 | |
| case _: | |
| raise BadRequest("Invalid credential type") | |
| # 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 | |
| # For rename only, just update the name | |
| rename = request.name is not None and not any((request.credentials, request.parameters, request.properties)) | |
| # When credential type is UNAUTHORIZED, it indicates the subscription was manually created | |
| # For Manually created subscription, they dont have credentials, parameters | |
| # They only have name and properties(which is input by user) | |
| manually_created = subscription.credential_type == CredentialType.UNAUTHORIZED | |
| if rename or manually_created: | |
| TriggerProviderService.update_trigger_subscription( | |
| tenant_id=user.current_tenant_id, | |
| subscription_id=subscription_id, | |
| name=request.name, | |
| properties=request.properties, | |
| ) | |
| return 200 | |
| # 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 |
🤖 Prompt for AI Agents
In `@api/controllers/console/workspace/trigger_providers.py` around lines 353 - 378, The variable manually_created (subscription.credential_type == CredentialType.UNAUTHORIZED) is unused and causes rebuild_trigger_subscription to be called for UNAUTHORIZED subscriptions (which fail); update the control flow so that when manually_created is true you call TriggerProviderService.update_trigger_subscription (passing tenant_id=user.current_tenant_id, subscription_id=subscription_id, name=request.name, properties=request.properties) and return 200 instead of calling TriggerProviderService.rebuild_trigger_subscription, keeping the existing rename branch (rename) intact so only non-manual, non-rename cases call TriggerProviderService.rebuild_trigger_subscription with credentials=request.credentials or subscription.credentials and parameters=request.parameters or subscription.parameters. Comment on lines +882 to +911
| # Delete the previous subscription | ||
| user_id = subscription.user_id | ||
| TriggerManager.unsubscribe_trigger( | ||
| tenant_id=tenant_id, | ||
| user_id=user_id, | ||
| provider_id=provider_id, | ||
| subscription=subscription.to_entity(), | ||
| credentials=subscription.credentials, | ||
| credential_type=credential_type, | ||
| ) | ||
| | ||
| except Exception as e: | ||
| # Rollback on any error | ||
| session.rollback() | ||
| logger.exception("Failed to rebuild trigger subscription", exc_info=e) | ||
| raise | ||
| # Create a new subscription with the same subscription_id and endpoint_id | ||
| new_subscription: TriggerSubscriptionEntity = TriggerManager.subscribe_trigger( | ||
| tenant_id=tenant_id, | ||
| user_id=user_id, | ||
| provider_id=provider_id, | ||
| endpoint=generate_plugin_trigger_endpoint_url(subscription.endpoint_id), | ||
| parameters=parameters, | ||
| credentials=credentials, | ||
| credential_type=credential_type, | ||
| ) | ||
| TriggerProviderService.update_trigger_subscription( | ||
| tenant_id=tenant_id, | ||
| subscription_id=subscription.id, | ||
| name=name, | ||
| parameters=parameters, | ||
| credentials=credentials, | ||
| properties=new_subscription.properties, | ||
| expires_at=new_subscription.expires_at, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file find . -name "trigger_provider_service.py" -type fRepository: qodo-benchmark/dify-combined-coderabbit
Length of output: 133
🏁 Script executed:
# Read the file around lines 882-911 to see the code in context cat -n ./api/services/trigger/trigger_provider_service.py | sed -n '870,920p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 2106
🏁 Script executed:
# Get broader context to see the function name and any error handling cat -n ./api/services/trigger/trigger_provider_service.py | sed -n '850,925p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 3186
🏁 Script executed:
# Search for the method definition and see if there's any error handling cat -n ./api/services/trigger/trigger_provider_service.py | sed -n '830,870p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 2082
🏁 Script executed:
# Search for where rebuild_trigger_subscription is called to see if error handling exists at caller level rg -n "rebuild_trigger_subscription" --type=py -B3 -A3Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 14296
🏁 Script executed:
# Examine the rollback test in detail to understand the error handling cat -n ./api/tests/test_containers_integration_tests/services/test_trigger_provider_service.py | sed -n '424,476p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 2501
🏁 Script executed:
# Look for transaction handling or try/catch in the rebuild_trigger_subscription method cat -n ./api/services/trigger/trigger_provider_service.py | sed -n '843,920p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 3437
🏁 Script executed:
# Look at the controller that calls rebuild_trigger_subscription to see if there's transaction handling cat -n ./api/controllers/console/workspace/trigger_providers.py | sed -n '350,400p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 2634
🏁 Script executed:
# Search for `@transactional` or db.session/transaction decorators in the file rg -n "@db.commit|@transactional|db.session|commit_or_rollback" ./api/services/trigger/trigger_provider_service.py | head -20Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 65
🏁 Script executed:
# Check if there's a transaction context manager used in the method rg -n "with.*session|with.*transaction|with db" ./api/services/trigger/trigger_provider_service.py | head -20Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 980
🏁 Script executed:
# Look at the complete rebuild_trigger_subscription method including any wrapping transaction context cat -n ./api/services/trigger/trigger_provider_service.py | sed -n '843,930p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 3437
🏁 Script executed:
# Check if there's error handling or re-subscription logic anywhere rg -n "except.*subscribe|try:.*unsubscribe" ./api/services/trigger/trigger_provider_service.py -A5Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 65
🏁 Script executed:
# Check the update_trigger_subscription method implementation rg -n "def update_trigger_subscription" ./api/services/trigger/trigger_provider_service.py -A30Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 1694
🏁 Script executed:
# Get the full update_trigger_subscription implementation rg -n "def update_trigger_subscription" ./api/services/trigger/trigger_provider_service.py -A60 | head -80Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 3470
🏁 Script executed:
# Check if rebuild_trigger_subscription is wrapped in a transaction or if there's error handling in the caller cat -n ./api/controllers/console/workspace/trigger_providers.py | sed -n '345,385p'Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 2235
🏁 Script executed:
# Check if rebuild_trigger_subscription has any wrapping context manager or if get_subscription_by_id creates session context rg -n "def get_subscription_by_id" ./api/services/trigger/trigger_provider_service.py -A20Repository: qodo-benchmark/dify-combined-coderabbit
Length of output: 1241
Data inconsistency risk from non-atomic unsubscribe→subscribe→update sequence.
If subscribe_trigger or update_trigger_subscription fails after unsubscribe_trigger succeeds, the third-party webhook is deleted while the local database retains stale data. This breaks consistency between the remote provider and the local subscription record.
Wrap the three operations in a single database transaction with error handling to either complete all steps or roll back all changes, or implement re-subscription logic if the sequence partially fails.
🤖 Prompt for AI Agents
In `@api/services/trigger/trigger_provider_service.py` around lines 882 - 911, The unsubscribe→subscribe→update sequence (calls to TriggerManager.unsubscribe_trigger, TriggerManager.subscribe_trigger and TriggerProviderService.update_trigger_subscription) is non-atomic and can leave the remote webhook deleted while the local DB still has stale data; wrap these three operations in a single transactional boundary (begin DB transaction before unsubscribe, perform unsubscribe, subscribe (using generate_plugin_trigger_endpoint_url and subscribing into TriggerSubscriptionEntity), then update_trigger_subscription) and commit only if all succeed, otherwise roll back the DB changes and, if unsubscribe already removed the remote webhook, attempt to re-subscribe in the error path to restore remote state or persist a compensating record; ensure errors during subscribe/update are caught, the transaction is rolled back, and any compensating re-subscribe attempts are idempotent and logged. This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from qodo-benchmark#181
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.