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.

Sign up for GitHub

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
liuruibin merged 1 commit into from
Aug 5, 2025
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/knowledge/serializers/document.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Aug 5, 2025

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.

Expand Down
Loading