Azure / Azure/azure-sdk-tools

[APIView] Comments are orphaned when API revisions are deleted or replaced

Open
#14,187 1 comment 0 reactions 1 assignee Assigned to @tjprescott View on GitHub
APIView bug
Dominant language
C#
Stars
135
Forks
260
Avg merge
3d 2h
Merged PRs (30d)
130

Description

## Problem

When an API revision is deleted (soft or hard), its associated comments remain in the database with their original `APIRevisionId` unchanged. This creates orphaned comments that reference revisions that no longer exist. The frontend works around this by falling back orphaned comments to the active revision for display, but the underlying data relationship is broken.

There are two distinct scenarios where this matters:

## Scenario 1: Revision Replacement (Automatic Revisions)

When `AutoReviewService.CreateAutomaticRevisionAsync` creates a new automatic revision, it soft-deletes superseded revisions that are not approved, not released, and not identical to the incoming revision. The check in `AutoReviewService.cs` protects revisions that have comments:

```csharp
comments.Any(c => latestAutomaticAPIRevision.Id == c.APIRevisionId)
```

However, `CarryForwardRevisionDataAsync` only carries forward approval status and the `HasAutoGeneratedComments` flag — it does **not** re-associate comment records to the new revision. This means:

- If a revision has no comments, it gets deleted and replaced — no issue yet.
- If a revision *does* have comments, it is kept around solely to preserve the comment linkage, even though it has been logically superseded. Over time this leads to stale revisions that exist only as comment anchors.
- The comments' `APIRevisionId` is never updated to point to the successor revision.

## Scenario 2: Direct Deletion (Archive, PR Cleanup, Manual Delete, Purge)

When revisions are deleted via:

- **Auto-archive** — inactive manual revisions older than N months
- **PR cleanup** — closed PR revisions with no approvers
- **Manual user deletion**
- **Auto-purge** — hard delete of long-soft-deleted revisions (Cosmos DB entry + blob storage removed)

...no cleanup or migration of associated comments occurs. The comments remain in the `Comments` Cosmos container referencing a revision ID that may no longer exist in the `Revisions` container.

## Expected Behavior

1. **Replacement operations** (automatic revision superseding): All non-diagnostic user comments from the old revision should be re-associated to the replacing revision (update `comment.APIRevisionId` to the new revision ID).

2. **Direct deletion** (no replacement): Comments associated with the deleted revision should be deleted (or soft-deleted) along with the revision.

3. **Diagnostic comments** should always be deleted with their revision, since they are revision-specific by nature.

## Current Workarounds in the Frontend

The Conversations panel in `conversations.component.ts` has a fallback that maps orphaned comments to the active revision for display:

```typescript
// If the thread's apiRevisionId doesn't match any loaded revision,
// fall back to the active revision so it still appears in the panel.
if (apiRevisionPostion === Number.MAX_SAFE_INTEGER && this.activeApiRevisionId) {
const activePosition = apiRevisionPositionMap.get(this.activeApiRevisionId);
if (activePosition !== undefined) {
apiRevisionPostion = activePosition;
}
}
```

The `comment-visibility.helper.ts` also shows all non-diagnostic comments regardless of revision, which further hides orphaned comments from users. These workarounds mask the issue at the UI level but do not fix the data integrity problem.

## Affected Code Paths

| Path | File | Action Needed |
|------|------|---------------|
| Automatic revision creation | `AutoReviewService.cs` | Re-associate comments to new revision before soft-deleting old one |
| `CarryForwardRevisionDataAsync` | `APIRevisionsManager.cs` | Extend to migrate comment `APIRevisionId` references |
| `SoftDeleteAPIRevisionAsync` | `APIRevisionsManager.cs` | Soft-delete associated comments (or migrate them if a replacement exists) |
| `AutoPurgeAPIRevisions` | `APIRevisionsManager.cs` | Delete orphaned comments when hard-purging revisions |
| `ClosePullRequestAPIRevision` | `PullRequestManager.cs` | Handle associated comments on PR revision cleanup |
| `AutoArchiveAPIRevisions` | `APIRevisionsManager.cs` | Handle associated comments on archive |

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.