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

[JadeCara]

Summary

• Fixes DetachedInstanceError caused by the new replies relationship on Comment (added in ENG-3298: Add parent_id column to Comment model for threading #7864) using default lazy loading
• Comment.delete() now re-fetches from the DB to guarantee a session-bound instance before accessing relationships
• replies relationship now uses lazy="selectin" (matching user) to prevent lazy load on detached instances

Root cause

The replies relationship used default lazy="select". When Comment.delete() was called on a detached instance (e.g. during fidesplus test fixture teardown), accessing self.replies, self.references, and db.delete(self) all failed with DetachedInstanceError.

Test plan

• All 19 tests/ctl/models/test_comment.py tests pass (including test_delete_parent_comment_deletes_replies, test_delete_parent_deletes_grandchild_replies)
• Fidesplus comment tests pass with this fix installed

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ready	Preview, Comment	Apr 13, 2026 8:17pm

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
fides-privacy-center	Ignored		Apr 13, 2026 8:17pm

[claude]

Code Review — PR #7906: Fix DetachedInstanceError on Comment.delete() replies access

Scope: src/fides/api/models/comment.py — two changes: (1) lazy="selectin" added to the replies relationship, (2) delete() re-fetches the comment from DB before relationship access.

---

Critical

lazy="selectin" on replies is a read-path regression (see inline comment on line 124).

The self-referential selectin fires an additional SELECT for every Comment loaded anywhere in the app, and recursively for all their replies — an unbounded N+1 per nesting level on every list-comments request. The delete() re-fetch added below is already sufficient to fix the DetachedInstanceError without this change. The selectin is both redundant for the fix and harmful to general read performance.

Recommendation: revert lazy="selectin" from the replies relationship. Keep only the re-fetch change in delete().

---

Suggestions

• Unconditional re-fetch tax (line 175): The re-fetch fires on every delete() call even when self is already session-bound. Consider detecting detachment with sa_inspect(self).detached and only re-fetching when needed (see inline). Minor but relevant if deletions run in bulk.
• Silent swallow on double-delete (line 177): The early return when comment is None is correct, but adding a logger.warning would help surface stale-reference bugs rather than silently no-oping.
• Unchecked test plan item: The PR description has - [ ] Fidesplus comment tests pass with this fix installed unchecked. Given that Comment.delete has consumers in the enterprise extension, those tests should pass before merging.

---

What's good

• The re-fetch pattern is the right approach to solving DetachedInstanceError at the call site rather than requiring callers to manage session state.
• The comment is None guard handles the already-deleted edge case cleanly.
• Switching all self.* accesses to comment.* after the re-fetch is consistent and correct.

---

🔬 Codegraph: connected (46363 nodes)

---

💡 Write /code-review in a comment to re-run this review.

[codecov]

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.06%. Comparing base (2782ce0) to head (1e8f4e1).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/fides/api/models/comment.py	62.50%	2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (62.50%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7906      +/-   ##
==========================================
- Coverage   85.07%   85.06%   -0.01%     
==========================================
  Files         629      629              
  Lines       40850    40855       +5     
  Branches     4747     4748       +1     
==========================================
+ Hits        34752    34754       +2     
- Misses       5027     5029       +2     
- Partials     1071     1072       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JadeCara]

/code-review

[claude]

Code Review: ENG-3298 — Fix DetachedInstanceError on Comment.delete()

The fix is sound and addresses the root cause cleanly. The re-fetch pattern is a well-understood workaround for detached-instance errors, and the early-return guard correctly handles the already-deleted case. A few observations:

Suggestion

Unnecessary DB round-trip on attached instances (line 175)
The unconditional re-fetch hits the DB on every Comment.delete() call, including the common path where self is already bound to the session. Using sa_inspect(self).detached to gate the re-fetch would avoid the overhead for non-detached callers. See inline comment for a code sketch. Not a blocker, but worth considering if this method is called at volume.

Clarity

Two complementary guards serve different scenarios (lines 176–178 and 239)
The None check after re-fetch guards against already-committed deletions; comment not in db.deleted guards against same-transaction in-flight deletions (e.g., a reply that also has its own CommentReference row and appears in both the BFS and the top-level query). These are not redundant — a note linking the two would help future readers.

Lazy-load strategy on replies is undocumented (line 182)
The default lazy="select" on the replies relationship is a deliberate choice (avoiding the recursive-loading hazard of selectin on self-referential relationships), but it isn't noted in the code. The PR description's bullet "replies relationship now uses lazy=\"selectin\"" does not match the actual code — replies retains the default. A short comment in the relationship definition explaining why selectin was not used would prevent future confusion.

Minor

No test covers the comment is None early-return path added in this PR (a comment deleted before delete() is called). A single two-call test would verify the guard and that the debug log fires.

---

Overall the change is focused and correct. The inline comments are suggestions/clarifications rather than blockers.

🔬 Codegraph: unavailable

💡 Write /code-review in a comment to re-run this review.

[claude]

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.

[claude]

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.

[claude]

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.