fix: error handling for db based integrity errors #6632
Conversation
WalkthroughThe changes enhance error handling and validation across multiple modules. Try-except blocks have been added to catch database integrity errors in bulk creation and update operations for issue assignees, labels, comment reactions, and favorites. Additionally, a new validation ensures label names remain unique within a project. Background tasks now include error handling for database operations to prevent task failures. These modifications improve the overall robustness of API endpoints and scheduled tasks. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant IssueSerializer
participant Database
Client->>IssueSerializer: Create Issue with assignees/labels
IssueSerializer->>Database: Bulk create IssueAssignee/IssueLabel
alt Successful Creation
Database-->>IssueSerializer: Return success
else IntegrityError Occurs
Database-->>IssueSerializer: Throw IntegrityError
Note right of IssueSerializer: Exception caught and handled
end
IssueSerializer-->>Client: Return creation response
sequenceDiagram
actor Client
participant LabelViewSet
participant Database
Client->>LabelViewSet: Request label update (new name)
LabelViewSet->>Database: Query for duplicate labels in project
alt Duplicate Found
LabelViewSet-->>Client: Return 400 error "Label exists"
else No Duplicate
LabelViewSet->>Database: Proceed with label update
Database-->>LabelViewSet: Return updated label
LabelViewSet-->>Client: Return successful response
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
apiserver/plane/bgtasks/recent_visited_task.py (1)
27-31: Consider logging database errors for monitoring.While silently handling database errors is acceptable for background tasks, it would be beneficial to log these errors for monitoring and debugging purposes. You already have the
log_exceptionutility imported.try: recent_visited.visited_at = timezone.now() recent_visited.save(update_fields=["visited_at"]) except DatabaseError: - pass + log_exception("Database error while updating recent visit")apiserver/plane/app/views/workspace/favorite.py (1)
46-49: Enhance error message with specific details.The error message could be more informative by including details about which favorite already exists, helping users understand why their request failed.
return Response( - {"error": "Favorite already exists"}, status=status.HTTP_400_BAD_REQUEST + { + "error": f"Favorite already exists for entity '{request.data.get('entity_identifier')}' of type '{request.data.get('entity_type')}'" + }, + status=status.HTTP_400_BAD_REQUEST )apiserver/plane/app/views/issue/label.py (1)
91-91: Consider using a predefined color palette.Instead of generating completely random colors, consider using a predefined color palette to ensure good contrast and readability.
- color=f"#{random.randint(0, 0xFFFFFF + 1):06X}", + color=random.choice([ + "#FF5630", "#FFC400", "#36B37E", "#00B8D9", "#6554C0", + "#FFAB00", "#FF7452", "#00875A", "#0052CC", "#5243AA" + ]),apiserver/plane/app/views/issue/comment.py (1)
168-193: Consider restructuring the reaction creation flow.
- The issue activity task is triggered before checking for duplicates, which could lead to unnecessary task creation.
- The error message could be more specific about which reaction already exists.
try: serializer = CommentReactionSerializer(data=request.data) if serializer.is_valid(): serializer.save( project_id=project_id, actor_id=request.user.id, comment_id=comment_id, ) + # Move issue activity task here after successful save issue_activity.delay( type="comment_reaction.activity.created", requested_data=json.dumps(request.data, cls=DjangoJSONEncoder), actor_id=str(request.user.id), issue_id=None, project_id=str(project_id), current_instance=None, epoch=int(timezone.now().timestamp()), notification=True, origin=request.META.get("HTTP_ORIGIN"), ) return Response(serializer.data, status=status.HTTP_201_CREATED) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) except IntegrityError: return Response( - {"error": "Reaction already exists for the user"}, + { + "error": f"Reaction '{request.data.get('reaction')}' already exists for this comment" + }, status=status.HTTP_400_BAD_REQUEST )apiserver/plane/api/serializers/issue.py (3)
141-158: Consider logging the integrity error for debugging.While catching the IntegrityError prevents the application from crashing, silently ignoring it without logging could make debugging difficult.
Apply this diff to add error logging:
try: IssueAssignee.objects.bulk_create( [ IssueAssignee( assignee_id=assignee_id, issue=issue, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for assignee_id in assignees ], batch_size=10, ) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while bulk creating issue assignees for issue {issue.id}")
174-191: Consider logging the integrity error for debugging.Similar to the IssueAssignee case, silently ignoring IntegrityError without logging could make debugging difficult.
Apply this diff to add error logging:
try: IssueLabel.objects.bulk_create( [ IssueLabel( label_id=label_id, issue=issue, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for label_id in labels ], batch_size=10, ) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while bulk creating issue labels for issue {issue.id}")
207-224: Remove redundant error handling in update method.The update method uses both
ignore_conflicts=Trueand try-except block for IntegrityError, which is redundant. Theignore_conflicts=Trueparameter is sufficient.Apply this diff to remove the redundant error handling:
-try: IssueAssignee.objects.bulk_create( [ IssueAssignee( assignee_id=assignee_id, issue=instance, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for assignee_id in assignees ], batch_size=10, ignore_conflicts=True, ) -except IntegrityError: - pass if labels is not None: IssueLabel.objects.filter(issue=instance).delete() -try: IssueLabel.objects.bulk_create( [ IssueLabel( label_id=label_id, issue=instance, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for label_id in labels ], batch_size=10, ignore_conflicts=True, ) -except IntegrityError: - passAlso applies to: 228-245
apiserver/plane/app/serializers/issue.py (2)
138-154: Consider logging integrity errors in create method.While catching IntegrityError prevents the application from crashing, silently ignoring it without logging could make debugging difficult.
Apply this diff to add error logging:
try: IssueAssignee.objects.bulk_create( [ IssueAssignee( assignee=user, issue=issue, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for user in assignees ], batch_size=10, ) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while bulk creating issue assignees for issue {issue.id}") if labels is not None and len(labels): try: IssueLabel.objects.bulk_create( [ IssueLabel( label=label, issue=issue, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for label in labels ], batch_size=10, ) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while bulk creating issue labels for issue {issue.id}")Also applies to: 171-187
203-220: Remove redundant error handling in update method.The update method uses both
ignore_conflicts=Trueand try-except block for IntegrityError, which is redundant. Theignore_conflicts=Trueparameter is sufficient.Apply this diff to remove the redundant error handling:
-try: IssueAssignee.objects.bulk_create( [ IssueAssignee( assignee=user, issue=instance, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for user in assignees ], batch_size=10, ignore_conflicts=True, ) -except IntegrityError: - pass if labels is not None: IssueLabel.objects.filter(issue=instance).delete() -try: IssueLabel.objects.bulk_create( [ IssueLabel( label=label, issue=instance, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) for label in labels ], batch_size=10, ignore_conflicts=True, ) -except IntegrityError: - passAlso applies to: 224-241
apiserver/plane/app/views/asset/v2.py (1)
683-688: Consider logging integrity errors in bulk asset updates.While catching IntegrityError prevents the application from crashing when the associated entity has been deleted, silently ignoring it without logging could make debugging difficult.
Apply this diff to add error logging:
# For some cases, the bulk api is called after the issue is deleted creating # an integrity error try: assets.update(issue_id=entity_id) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while updating assets for issue {entity_id}") if asset.entity_type == FileAsset.EntityTypeContext.COMMENT_DESCRIPTION: # For some cases, the bulk api is called after the comment is deleted # creating an integrity error try: assets.update(comment_id=entity_id) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while updating assets for comment {entity_id}") if asset.entity_type == FileAsset.EntityTypeContext.DRAFT_ISSUE_DESCRIPTION: # For some cases, the bulk api is called after the draft issue is deleted # creating an integrity error try: assets.update(draft_issue_id=entity_id) except IntegrityError: - pass + import logging + logger = logging.getLogger(__name__) + logger.warning(f"Integrity error while updating assets for draft issue {entity_id}")Also applies to: 691-696, 702-707
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apiserver/plane/api/serializers/issue.py(3 hunks)apiserver/plane/app/serializers/issue.py(3 hunks)apiserver/plane/app/views/asset/v2.py(2 hunks)apiserver/plane/app/views/issue/comment.py(2 hunks)apiserver/plane/app/views/issue/label.py(2 hunks)apiserver/plane/app/views/workspace/favorite.py(2 hunks)apiserver/plane/bgtasks/issue_activities_task.py(2 hunks)apiserver/plane/bgtasks/recent_visited_task.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
apiserver/plane/app/views/issue/label.py (1)
58-70: LGTM! Robust validation for label name uniqueness.The validation logic correctly checks for duplicate label names within the project scope, excluding the current label being updated.
apiserver/plane/api/serializers/issue.py (1)
4-4: LGTM!Added import for handling database integrity errors.
apiserver/plane/app/serializers/issue.py (1)
5-5: LGTM!Added import for handling database integrity errors.
apiserver/plane/app/views/asset/v2.py (1)
8-8: LGTM!Added import for handling database integrity errors.
apiserver/plane/bgtasks/issue_activities_task.py (2)
793-802: LGTM!Improved handling of None values in cycle issue activity string formatting.
1417-1417: LGTM!Consistent string formatting in issue relation activity.
* fix: error handling for db based integrity errors * fix: meta endpoint to return correct error message * fix: module activity
Description
This PR handles all the integrity errors and issue activity errors
Type of Change
Test Scenarios
Summary by CodeRabbit
Bug Fixes
Enhancements