Skip to content

ENG-3298: Fix DetachedInstanceError on Comment.delete() replies access #7906

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
JadeCara merged 3 commits into from
Apr 13, 2026
+13 −4
Merged

ENG-3298: Fix DetachedInstanceError on Comment.delete() replies access #7906

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e14c4a
ENG-3298: Fix DetachedInstanceError on Comment.delete() replies access
JadeCara Apr 13, 2026
96a22cb
Address PR review feedback
JadeCara Apr 13, 2026
1e8f4e1
Merge branch 'main' into ENG-3298/fix-comment-replies-detached-instance
galvana Apr 13, 2026
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
17 changes: 13 additions & 4 deletions src/fides/api/models/comment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from enum import Enum as EnumType
from typing import TYPE_CHECKING, Any

from loguru import logger
from sqlalchemy import Column, DateTime, ForeignKey, Index, String, func, orm
from sqlalchemy import Enum as EnumColumn
from sqlalchemy.ext.declarative import declared_attr
Expand Down Expand Up @@ -168,9 +169,17 @@ def delete(self, db: Session) -> None:
# are added, prevent deletion of comments with those types to preserve
# correspondence threads.

# Re-fetch from the DB to guarantee a session-bound instance.
# Callers (e.g. test fixture teardown) may hold a detached reference,
# which would raise DetachedInstanceError on any lazy relationship access.
comment = db.query(Comment).filter(Comment.id == self.id).first()
Comment thread
JadeCara marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 13, 2026

Choose a reason for hiding this comment

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

src/fides/api/models/comment.py:175

The re-fetch unconditionally issues a SELECT even when self is already bound to db (the common production path). SQLAlchemy's identity map means the returned object will be the same Python instance if it's present, but the DB round-trip still happens unless the session has expire_on_commit=False and the object is unexpired.

A lighter alternative that avoids the extra query for the happy path:

from sqlalchemy import inspect as sa_inspect

if sa_inspect(self).detached:
    comment = db.query(Comment).filter(Comment.id == self.id).first()
    if comment is None:
        logger.debug("Comment {} already deleted, skipping", self.id)
        return
else:
    comment = self

This reserves the DB round-trip for the detached case (fixture teardown, etc.) while the normal flow incurs no overhead. Not a blocker if the call volume is low, but worth considering.

if comment is None:
logger.debug("Comment {} already deleted, skipping", self.id)
return
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 13, 2026

Choose a reason for hiding this comment

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

src/fides/api/models/comment.py:178

The None guard here and the comment not in db.deleted guard at line 239 handle two distinct scenarios that are worth documenting together:

  • This guard (line 176–178): protects against a comment that was deleted in a prior committed transaction — the re-fetch returns None because the row is gone.
  • db.deleted guard (line 239): protects against a comment staged for deletion within the current transaction (e.g. a reply that both has its own CommentReference row and is a descendant of another top-level comment in the same delete_comments_for_reference_and_type call).

The two are complementary, not redundant. A brief note linking them would help future readers understand why both checks are needed.


# Collect all descendants iteratively (breadth-first)
to_delete = []
stack = list(self.replies)
stack = list(comment.replies)
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 13, 2026

Choose a reason for hiding this comment

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

src/fides/api/models/comment.py:182

replies uses default lazy="select", so list(comment.replies) here and node.replies on line 185 each issue a separate SELECT. For the current use case (shallow reply trees) this is fine, but it's worth noting in a comment that lazy="selectin" was intentionally avoided here because self-referential selectin triggers recursive loading on every read path — not just the delete path.

while stack:
node = stack.pop()
stack.extend(node.replies)
Expand All @@ -187,11 +196,11 @@ def delete(self, db: Session) -> None:

# Delete self
AttachmentService(db).delete_for_reference(
self.id, AttachmentReferenceType.comment
comment.id, AttachmentReferenceType.comment
)
for reference in self.references:
for reference in comment.references:
reference.delete(db)
db.delete(self)
db.delete(comment)

@staticmethod
def delete_comments_for_reference_and_type(
Expand Down
Loading