Skip to content
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

[WEB-2680] chore: updated queryset for soft delete #5844

Merged
merged 1 commit into from
Oct 17, 2024
Merged

[WEB-2680] chore: updated queryset for soft delete #5844

Show file tree
Hide file tree
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
4 changes: 2 additions & 2 deletions apiserver/plane/api/serializers/issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def update(self, instance, validated_data):
updated_by_id = instance.updated_by_id

if assignees is not None:
IssueAssignee.objects.filter(issue=instance).delete()
IssueAssignee.objects.filter(issue=instance).delete(soft=False)
IssueAssignee.objects.bulk_create(
[
IssueAssignee(
Expand All @@ -229,7 +229,7 @@ def update(self, instance, validated_data):
)

if labels is not None:
IssueLabel.objects.filter(issue=instance).delete()
IssueLabel.objects.filter(issue=instance).delete(soft=False)
IssueLabel.objects.bulk_create(
[
IssueLabel(
Expand Down
8 changes: 6 additions & 2 deletions apiserver/plane/app/serializers/draft.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ def update(self, instance, validated_data):
updated_by_id = instance.updated_by_id

if assignees is not None:
DraftIssueAssignee.objects.filter(draft_issue=instance).delete()
DraftIssueAssignee.objects.filter(draft_issue=instance).delete(
soft=False
)
DraftIssueAssignee.objects.bulk_create(
[
DraftIssueAssignee(
Expand All @@ -186,7 +188,9 @@ def update(self, instance, validated_data):
)

if labels is not None:
DraftIssueLabel.objects.filter(draft_issue=instance).delete()
DraftIssueLabel.objects.filter(draft_issue=instance).delete(
soft=False
)
DraftIssueLabel.objects.bulk_create(
[
DraftIssueLabel(
Expand Down
4 changes: 2 additions & 2 deletions apiserver/plane/app/serializers/issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def update(self, instance, validated_data):
updated_by_id = instance.updated_by_id

if assignees is not None:
IssueAssignee.objects.filter(issue=instance).delete()
IssueAssignee.objects.filter(issue=instance).delete(soft=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider the implications of switching to hard delete

The changes from delete() to delete(soft=False) for IssueAssignee and IssueLabel represent a shift from soft deletion to hard deletion. This modification has several important implications:

  1. Data Permanence: Hard deletion will permanently remove the data, which may not be recoverable.
  2. Audit Trail: It might affect the ability to maintain a complete audit trail of issue changes.
  3. Performance: Hard deletes might be faster and reduce database bloat, but at the cost of data retention.

Consider the following:

  1. Ensure this change aligns with the overall data management strategy of the application.
  2. Update any dependent code that might rely on soft-deleted records still being present.
  3. Verify that this change doesn't conflict with any data retention policies or regulatory requirements.
  4. Consider adding a confirmation step or logging mechanism for these hard deletes to maintain traceability.

Would you like assistance in implementing any of these suggestions?

Also applies to: 221-221

IssueAssignee.objects.bulk_create(
[
IssueAssignee(
Expand All @@ -218,7 +218,7 @@ def update(self, instance, validated_data):
)

if labels is not None:
IssueLabel.objects.filter(issue=instance).delete()
IssueLabel.objects.filter(issue=instance).delete(soft=False)
IssueLabel.objects.bulk_create(
[
IssueLabel(
Expand Down
12 changes: 11 additions & 1 deletion apiserver/plane/db/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,19 @@ class Meta:
abstract = True


class SoftDeletionQuerySet(models.QuerySet):
def delete(self, soft=True):
if soft:
return self.update(deleted_at=timezone.now())
else:
return super().delete()
Comment on lines +46 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Potential Breaking Change Confirmed: Multiple delete() Calls Default to Soft Deletion

The analysis identified numerous instances where delete() is called without specifying soft=False. This means these deletions will now perform soft deletions by default, potentially leading to unintended behavior and accumulation of soft-deleted records.

Affected Files:

  • apiserver/plane/utils/cache.py
  • apiserver/plane/tests/api/test_authentication.py
  • apiserver/plane/space/views/inbox.py
  • apiserver/plane/space/views/issue.py
  • apiserver/plane/db/mixins.py
  • apiserver/plane/license/api/views/admin.py
  • apiserver/plane/db/migrations/0077_draftissue_cycle_user_timezone_project_user_timezone_and_more.py
  • apiserver/plane/bgtasks/
  • apiserver/plane/api/views/
  • apiserver/plane/app/views/
  • apiserver/plane/authentication/utils/workspace_project_join.py
  • apiserver/plane/authentication/provider/credentials/magic_code.py

Please review these deletions to ensure they align with the intended behavior.

🔗 Analysis chain

Potential Breaking Change: QuerySet.delete() Now Defaults to Soft Deletion

The SoftDeletionQuerySet class overrides the delete() method with soft=True by default. This changes the behavior of QuerySet.delete(), so it performs a soft delete unless soft=False is explicitly provided. Existing code that relies on hard deletion through delete() may now perform soft deletions unintentionally, potentially leading to unexpected results or accumulation of soft-deleted records.

To verify where delete() is called without specifying soft=False, consider running the following script:

This script searches for all occurrences of delete() calls on QuerySets, helping to identify places where the default behavior change may impact the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets that may be affected.

# Search for `.delete(` calls in Python files.
rg --type=python '\.delete\('

Length of output: 143

Script:

#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets in Python files.

# Search for `.delete(` calls in all `.py` files.
rg '\.delete\(' --type py

Length of output: 10502

Script:

#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets in Python files that do NOT specify `soft=False`.

# Search for `.delete(` calls not followed by `soft=False`.
rg '\.delete\((?!.*soft\s*=\s*False)' --type py

Length of output: 295

Script:

#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets in Python files that do NOT specify `soft=False`.

# Search for `.delete(` calls in all `.py` files and exclude lines with `soft=False`.
rg '\.delete\(' --type py | grep -v 'soft\s*=\s*False'

Length of output: 9254



class SoftDeletionManager(models.Manager):
def get_queryset(self):
return super().get_queryset().filter(deleted_at__isnull=True)
return SoftDeletionQuerySet(self.model, using=self._db).filter(
deleted_at__isnull=True
)


class SoftDeleteModel(models.Model):
Expand Down
5 changes: 2 additions & 3 deletions apiserver/plane/db/models/issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

# Module imports
from plane.utils.html_processor import strip_tags
from plane.db.mixins import SoftDeletionManager

from .project import ProjectBaseModel

Expand Down Expand Up @@ -79,7 +80,7 @@ def get_default_display_properties():


# TODO: Handle identifiers for Bulk Inserts - nk
class IssueManager(models.Manager):
class IssueManager(SoftDeletionManager):
def get_queryset(self):
return (
super()
Expand All @@ -90,7 +91,6 @@ def get_queryset(self):
| models.Q(issue_inbox__status=2)
| models.Q(issue_inbox__isnull=True)
)
.filter(deleted_at__isnull=True)
.filter(state__is_triage=False)
.exclude(archived_at__isnull=False)
.exclude(project__archived_at__isnull=False)
Expand Down Expand Up @@ -172,7 +172,6 @@ class Issue(ProjectBaseModel):
blank=True,
)

objects = models.Manager()
issue_objects = IssueManager()

class Meta:
Expand Down
4 changes: 2 additions & 2 deletions apiserver/plane/space/serializer/issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ def update(self, instance, validated_data):
updated_by_id = instance.updated_by_id

if assignees is not None:
IssueAssignee.objects.filter(issue=instance).delete()
IssueAssignee.objects.filter(issue=instance).delete(soft=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistency with PR objective: Hard delete implemented instead of soft delete

The changes in lines 424 and 441 are implementing a hard delete (soft=False) for IssueAssignee and IssueLabel instances. This contradicts the PR objective of implementing a soft delete mechanism.

Consider the following points:

  1. Using soft=False will permanently remove the data, which may not be the intended behavior for a soft delete implementation.
  2. This change could lead to unintended data loss, making it difficult to recover or audit historical assignments and labels.

To align with the PR objective, consider changing soft=False to soft=True or removing the soft parameter altogether if the default behavior is soft delete. For example:

IssueAssignee.objects.filter(issue=instance).delete()

and

IssueLabel.objects.filter(issue=instance).delete()

Please verify if this aligns with the intended soft delete implementation across the project.

Also applies to: 441-441

Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Soft delete implementation is inconsistent or missing in IssueAssignee and IssueLabel models

Our analysis reveals that the IssueAssignee and IssueLabel models lack fields typically associated with soft deletes (e.g., is_deleted, deleted_at). Despite using delete(soft=False) in serializers and views, the absence of these fields means that soft delete functionality may not be effectively implemented, leading to potential data loss.

Consider the following points:

  1. Missing Soft Delete Fields: Without fields like is_deleted or deleted_at, the models cannot properly support soft deletion.
  2. Hard Delete Usage: The use of delete(soft=False) across various parts of the codebase bypasses any soft delete logic, resulting in permanent deletions.
  3. Inconsistent Deletion Behavior: This approach contradicts the PR's objective of implementing soft deletes and can lead to data integrity issues.
🔗 Analysis chain

Reconsider the use of hard delete for related entities

The changes to use delete(soft=False) for IssueAssignee and IssueLabel instances contradict the PR objective of implementing soft delete. This approach may lead to unexpected data loss and inconsistency in deletion behavior across the application.

Consider the following points:

  1. Data Integrity: Permanently deleting related records during updates can lead to loss of historical data and potential integrity issues.
  2. Consistency: The use of hard delete here may be inconsistent with soft delete implementations elsewhere in the application.
  3. Recoverability: Soft deletes allow for potential data recovery, which is lost with hard deletes.

To align with the PR objectives and maintain data integrity, consider the following changes:

- IssueAssignee.objects.filter(issue=instance).delete(soft=False)
+ IssueAssignee.objects.filter(issue=instance).delete()  # This will use the default soft delete

- IssueLabel.objects.filter(issue=instance).delete(soft=False)
+ IssueLabel.objects.filter(issue=instance).delete()  # This will use the default soft delete

Additionally, ensure that the queryset used to fetch these related entities is updated to exclude soft-deleted records, if not already implemented.

To confirm the current soft delete implementation, please run the following script:

This script will help us understand the current soft delete implementation and ensure that our changes are consistent with the existing codebase.

Also applies to: 441-441

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the soft delete implementation for IssueAssignee and IssueLabel models

# Check for a soft delete field (e.g., is_deleted, deleted_at) in the models
echo "Checking for soft delete fields in IssueAssignee and IssueLabel models:"
rg -A 5 "class (IssueAssignee|IssueLabel)\(" apiserver/plane/db/models/

# Check for custom manager implementation that might handle soft deletes
echo "\nChecking for custom manager implementations:"
rg -A 10 "class (IssueAssignee|IssueLabel)Manager\(" apiserver/plane/db/models/

# Check for any existing soft delete logic in views or serializers
echo "\nChecking for existing soft delete logic:"
rg "delete\(.*soft" apiserver/plane/

Length of output: 2863

IssueAssignee.objects.bulk_create(
[
IssueAssignee(
Expand All @@ -438,7 +438,7 @@ def update(self, instance, validated_data):
)

if labels is not None:
IssueLabel.objects.filter(issue=instance).delete()
IssueLabel.objects.filter(issue=instance).delete(soft=False)
IssueLabel.objects.bulk_create(
[
IssueLabel(
Expand Down
Loading