Skip to content

fix: refactor delete method in DocumentSerializers for improved clarity #3813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 5, 2025

Conversation

shaohuzhang1
Copy link
Contributor

fix: refactor delete method in DocumentSerializers for improved clarity --bug=1060005 --user=刘瑞斌 【资源管理】知识库-删除文档报错 https://www.tapd.cn/62980211/s/1749123

--bug=1060005 --user=刘瑞斌 【资源管理】知识库-删除文档报错 https://www.tapd.cn/62980211/s/1749123
Copy link

f2c-ci-robot bot commented Aug 5, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

f2c-ci-robot bot commented Aug 5, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -662,6 +662,7 @@ def cancel(self, instance, with_valid=True):

@transaction.atomic
def delete(self):
self.is_valid(raise_exception=True)
document_id = self.data.get("document_id")
source_file_ids = [
doc['meta'].get(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code checks if self.is_valid raises an exception during the deletion process. However, this might not be ideal because raising exceptions could be cumbersome and potentially interrupt operations elsewhere.

Potential Issues:

  1. Exception Handling: Raising exceptions in deletion can lead to unpredictable behavior if it disrupts other business logic.
  2. Resource Usage: If you need validation without interruptions, consider using a separate method or returning status codes instead of throwing exceptions.
  3. Logging: Ensure that any errors are logged clearly, helping with debugging and system monitoring.

Optimization Suggestions:

  1. Simplified Validation: You could simplify the call to self.is_valid(raise_exception=True) by directly checking the result. This is less error-prone but requires careful handling of valid states.

    from django.core.exceptions import ValidationError
    
    def cancel(self, instance, with_valid=True):
    
        @transaction.atomic
        def delete(self):
            try:
                # Validate without interrupting normal flow
                self.is_valid()
            except ValidationError as e:
                raise Exception("Deletion failed due to: {}".format(e)) from None
       
            document_id = self.data.get("document_id")
            source_file_ids = [
                    doc['meta'].get(
                        'source_file_id',
                        default=[],
                    )
                    for doc in (json.loads(document_content) or [])
                ]
            ...  # Rest of the function implementation
  2. Separate Method: Consider creating a dedicated method that handles validation and returns an appropriate response. While this won't prevent exceptions being raised, it can improve readability and separation of concerns.

    def cancel(self, instance, with_valid=True):
        try:
            if with_valid:
                self.validate()
            ...
        except Exception as e:
            return {"error": str(e)}
        
        @transaction.atomic
        def delete():
            # Directly handle the data retrieval and processing here
            ...
    
    def validate(self):
        self.is_valid()
    
    def process_data(self):
        document_content = request.POST.get('content')
        ...  # Process the JSON content

Overall, while your current approach works, integrating more robust validation strategies would make your code cleaner, more maintainable, and less prone to runtime issues.

@liuruibin liuruibin merged commit f668daa into v2 Aug 5, 2025
3 of 5 checks passed
@liuruibin liuruibin deleted the pr@v2@fix_delete_doc branch August 5, 2025 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants