feat: External entity dedupe with union-find and FK violation recovery - #2138
Merged
Conversation
BenchstatBase: 📊 7 minor regression(s) (all within 5% threshold)
Full benchstat output |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThe PR adds warning deduplication and aggregation to the scrape pipeline, introduces resolved change tracking in the post-transformation pipeline, enhances external entity caching and ID resolution, implements FK-violation recovery with diagnostics for permission and access-log upserts, and refactors the extraction pipeline to track orphaned changes and FK errors in ScrapeSummary. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
adityathebe
force-pushed
the
feat/external-entity-dedupe
branch
from
April 20, 2026 15:02
551b661 to
e4b2228
Compare
Deduplication:
- Replace naive ID-match dedup with union-find over alias overlap so
entries from different scrapers (e.g. Azure AD with real UUIDs and
Azure DevOps with descriptor-only aliases) collapse into one survivor.
Prefer non-nil ID survivors when merging with nil-ID entries.
- Remove logic that added entity ID to aliases (no longer needed with
union-find grouping).
FK violation recovery (config_access + config_access_logs):
- Wrap bulk inserts in savepoints; on FK violation, roll back the
savepoint (preserving the temp table) and retry row-by-row with
per-row exception handling so valid rows still persist.
- Create stub external_users/roles/groups for missing references before
the bulk insert to minimize FK fallback hits.
- Add logFKDiagnostics and logAccessLogFKDiagnostics to classify and
summarize unresolvable rows (user_missing, role_deleted, etc.).
- Rewrite SaveConfigAccessLogs to use the same temp-table + savepoint +
stub-user + row-by-row fallback pattern as upsertConfigAccess.
Entity resolution:
- Add findExternalEntityByID: resolves by canonical ID, falls back to
alias overlap for merged/loser IDs.
- Add ID-keyed caches for all three entity types (previously only users
had one) and populate them in WarmExternalEntityCaches.
Merge function resilience:
- Wrap merge_and_upsert_external_* calls in savepoints via
runMergeFunctionWithDump; on failure, dump temp + overlapping live
rows to traces/ as JSON for post-mortem diagnosis.
- Isolate user_groups upsert in its own savepoint so a failure there
does not roll back already-successful user/group/role upserts.
Config access validation:
- Require both a principal (user or group) AND a role; skip rows
missing either with a warning instead of silently passing through.
Scrape summary improvements:
- Track orphaned changes and FK error changes on ScrapeSummary.
- Add warning deduplication (AddScrapeWarning with warningIndex).
- Track access log last_created_at per config type; retain from
previous scrape when no new logs are seen.
- Add resolveChange to populate ChangeResult.Resolved with the final
ConfigChange state after the change mapping pipeline.
Co-authored-by: Aditya Thebe <contact@adityathebe.com>
adityathebe
force-pushed
the
feat/external-entity-dedupe
branch
from
April 20, 2026 15:16
e4b2228 to
91bd2b7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
db/permission_changes.go (1)
162-222:⚠️ Potential issue | 🟠 MajorFK-recovery fallback silently drops
PermissionAddedchange events.The
result.addedloop below iteratesnewRows, which is populated only by the original bulkINSERT ... RETURNING. When the bulk path hits a foreign-key error we roll back tobulk_insertand run the row-by-rowDO $$ ... $$fallback — but that fallback has noRETURNINGequivalent and doesn't populatenewRows. As a result, every access row that's successfully persisted in the recovery path emits zeroPermissionAddedchange records, andextractResult.newChangeswill under-report.Concretely: if the first scrape touches a brand-new user that happens to be resolvable and the bulk insert succeeds for most rows but hits one FK-violating row, the fallback will insert the good rows silently — the user gets no audit trail for those new permissions.
Options:
- Have the
DOblockRAISE NOTICEor write into a temp collector table the ids it successfully inserted, then scan those back intonewRowsbefore continuing.- Or, after the fallback, re-query
config_accessfor ids that are in the originalitemslist but not in the temp table (i.e., the complement of the "FK error" set) and buildresult.addedfrom that.Additionally on this block:
- Line 163: the
SAVEPOINT bulk_insertexec's error is ignored. If savepoint creation fails, the laterROLLBACK TO SAVEPOINTwill also fail and the outer tx is left aborted. Worth at least logging.- Line 203:
fkErrorCountscan error is also ignored — if that scan fails,result.foreignKeyErrorsstays 0 andresult.savedis unadjusted, silently overstating saved rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/permission_changes.go` around lines 162 - 222, The fallback DO block leaves newRows empty so PermissionAdded events are dropped; fix in the function handling the bulk upsert by after the fallback path re-populating newRows (or another collector) from the actual persisted rows — e.g., query config_access for rows whose ids were in the original items set but no longer exist in tempTable, and assign those results to newRows before the loop that builds result.added (references: tempTable, newRows, result.added, buildPermissionSummary). Also handle errors for tx.Exec("SAVEPOINT bulk_insert") and the fkErrorCount scan (check and return/log errors rather than ignoring) so savepoint creation and count failures don’t silently corrupt transaction state or metrics (references: the SAVEPOINT exec and the fkErrorCount tx.Raw(...).Scan call); keep existing FK diagnostics/logFKDiagnostics behavior.api/v1/interface.go (2)
612-626:⚠️ Potential issue | 🟠 MajorMerge the new top-level scrape diagnostics.
Mergecurrently dropsOrphanedChanges,FKErrorChanges,Warnings, andStatefromother, so combined summaries lose the diagnostics this PR adds.Proposed fix
func (s *ScrapeSummary) Merge(other ScrapeSummary) { s.initConfigTypes() for k, v := range other.ConfigTypes { if existing, ok := s.ConfigTypes[k]; ok { s.ConfigTypes[k] = existing.Merge(v) } else { s.ConfigTypes[k] = v } } s.ExternalUsers = s.ExternalUsers.Merge(other.ExternalUsers) s.ExternalGroups = s.ExternalGroups.Merge(other.ExternalGroups) s.ExternalRoles = s.ExternalRoles.Merge(other.ExternalRoles) s.ConfigAccess = s.ConfigAccess.Merge(other.ConfigAccess) s.AccessLogs = s.AccessLogs.Merge(other.AccessLogs) + s.OrphanedChanges = append(s.OrphanedChanges, other.OrphanedChanges...) + s.FKErrorChanges = append(s.FKErrorChanges, other.FKErrorChanges...) + for _, w := range other.Warnings { + s.AddScrapeWarning(w) + } + if len(other.State) > 0 { + if s.State == nil { + s.State = make(map[string]any, len(other.State)) + } + for k, v := range other.State { + s.State[k] = v + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/interface.go` around lines 612 - 626, The Merge method on ScrapeSummary currently only merges some fields and omits the new diagnostic fields; update ScrapeSummary.Merge (the method named Merge on type ScrapeSummary) to also merge OrphanedChanges, FKErrorChanges, Warnings, and State from the other summary the same way other fields are merged (i.e., initialize if needed and call the appropriate Merge method or combine values) so that diagnostics are preserved when combining summaries.
430-431:⚠️ Potential issue | 🟠 MajorInclude the new summary keys in new-format JSON detection.
A payload like
{"warnings":[...]}or{"orphaned_changes":[...]}can omit all existingnewFormatKeys, then this falls through to the legacymap[string]ConfigTypeScrapeSummarypath and fails or misclassifies the summary.Proposed fix
- newFormatKeys := []string{"config_types", "external_users", "external_groups", "external_roles", "config_access", "access_logs"} + newFormatKeys := []string{ + "config_types", + "external_users", + "external_groups", + "external_roles", + "config_access", + "access_logs", + "orphaned_changes", + "fk_error_changes", + "warnings", + "state", + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/interface.go` around lines 430 - 431, The new-format detection currently checks newFormatKeys in the slice defined in api/v1/interface.go but misses summary-only top-level keys like "warnings" and "orphaned_changes", causing those payloads to be misclassified as legacy; update the newFormatKeys slice (the variable named newFormatKeys) to include "warnings" and "orphaned_changes" so these payloads are detected as new-format JSON during the payload detection logic.db/update.go (2)
1734-1747:⚠️ Potential issue | 🟠 MajorDon’t drop summaries that only contain ignored-by-action counts.
extractChangesrecords explicit ignores viaAddIgnoredByAction, but this gate callsChangeSummary.IsEmpty(), which currently ignoresIgnoredByActionandForeignKeyErrors. A result with onlyAction: Ignorechanges is skipped here and never reaches the summary/counters.Proposed fix
func (t ChangeSummary) IsEmpty() bool { - return len(t.Orphaned) == 0 && len(t.Ignored) == 0 + return len(t.Orphaned) == 0 && + len(t.Ignored) == 0 && + len(t.IgnoredByAction) == 0 && + t.ForeignKeyErrors == 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/update.go` around lines 1734 - 1747, The code in extractChanges is skipping chResult.changeSummary when ChangeSummary.IsEmpty() returns true, but IsEmpty() currently ignores IgnoredByAction and ForeignKeyErrors so summaries that only contain ignored-by-action counts get dropped; update the logic so these ignored-only summaries are preserved—either by changing the predicate here to consider chResult.changeSummary.HasIgnoredByAction() || chResult.changeSummary.HasForeignKeyErrors() in addition to !IsEmpty(), or by updating ChangeSummary.IsEmpty() to treat IgnoredByAction/ForeignKeyErrors as non-empty; then ensure extractResult.changeSummary.Merge(configType, chResult.changeSummary) still runs for those cases (symbols: extractChanges, chResult.changeSummary, ChangeSummary.IsEmpty, AddIgnoredByAction, ForeignKeyErrors, extractResult.changeSummary.Merge).
783-786:⚠️ Potential issue | 🟡 MinorCount access logs skipped for missing users.
This branch emits a warning and continues, but does not increment
summary.AccessLogs.Skipped, so skipped access-log totals are underreported.Proposed fix
if accessLog.ExternalUserID == uuid.Nil { summary.AddWarning("AccessLog", fmt.Sprintf("access log has no user_id aliases=%v %s", accessLog.ExternalUserAliases, accessLog)) + summary.AccessLogs.Skipped++ continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/update.go` around lines 783 - 786, The branch that checks accessLog.ExternalUserID == uuid.Nil emits a warning and continues but fails to increment the skipped counter, causing underreported totals; update the block that calls summary.AddWarning(...) to also increment summary.AccessLogs.Skipped (or the appropriate skipped field on the summary struct) before continuing so every skipped access log is counted, ensuring you reference accessLog.ExternalUserID and summary.AccessLogs.Skipped in the same branch.
🧹 Nitpick comments (5)
db/change_traversal.go (1)
17-33: LGTM — clean extraction, but consider surfacingResolvedfor the Ignore/Delete/skip paths too.The helper centralizes construction of
ChangeResult.Resolvedand is cleanly applied to MoveUp/CopyUp/Move/Copy. A couple of small observations:
- Fields copied are static; if
v1.ChangeResultever gains additional persisted fields (e.g.,Fingerprint,Patchesvariants, labels), this helper will silently drop them. A short// keep in sync with dutyModels.ConfigChangecomment above the struct literal would help future readers.- The AI summary mentions
db/update.goalso callingresolveChangefor Ignore/Delete/exclusion/orphan paths — worth confirming those branches set a sensibleActionstring (e.g.,""or a well-known marker) since the helper requires the caller to provide it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/change_traversal.go` around lines 17 - 33, The resolveChange helper constructs ChangeResult.Resolved but can silently omit future fields and relies on callers to supply a meaningful Action; add a short comment above resolveChange and the struct literal saying "// keep in sync with dutyModels.ConfigChange" and/or note which fields must be preserved, and audit all callers (e.g., usages in MoveUp/CopyUp/Move/Copy and db/update.go branches for Ignore/Delete/exclusion/orphan paths) to ensure they pass an explicit, well-known Action string (or a documented empty marker) so ignored/deleted/skipped changes get a sensible Action value.db/permission_changes.go (1)
405-412: Top-10 FK reason logging has nondeterministic ordering.
countsis a Go map, sofor key, count := range countsyields a different ordering on each run. With a hard cap at 10, two scraper runs against identical data can report different subsets of reasons, making diagnosis harder.Consider sorting by
countdesc (most-frequent first) before truncating:♻️ Proposed fix
- logged := 0 - for key, count := range counts { - if logged >= 10 { - break - } - ctx.Logger.Warnf(" reason=%s id=%s count=%d", key.Reason, key.FKID, count) - logged++ - } + type entry struct { + key groupKey + count int + } + entries := make([]entry, 0, len(counts)) + for k, c := range counts { + entries = append(entries, entry{k, c}) + } + sort.Slice(entries, func(i, j int) bool { return entries[i].count > entries[j].count }) + for i, e := range entries { + if i >= 10 { + break + } + ctx.Logger.Warnf(" reason=%s id=%s count=%d", e.key.Reason, e.key.FKID, e.count) + }Requires adding
"sort"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/permission_changes.go` around lines 405 - 412, The current loop over counts (map variable counts) is nondeterministic because Go map iteration order is random, so the top-10 output (loop using logged, key.Reason, key.FKID, count) can vary between runs; fix by creating a slice of entries from counts (e.g., struct with key and count), sort that slice by count descending using sort.Slice, then iterate the first min(10, len(slice)) entries and call ctx.Logger.Warnf for each; remember to add "sort" to the imports.db/external_entities_test.go (1)
16-94: Coverage gap: two distinct non-nil IDs sharing an alias.The
dedupeByIDWithIndexspecs cover (a) all-nil-ID alias-overlap, (b) same-non-nil-ID dedup, and (c) one-nil-one-non-nil alias-overlap. They do not cover the adversarial case where two items carry different non-nil IDs but share an alias. Given the alias-union step atexternal_entities.goL787-793 unions everyone regardless of ID, that case will silently drop one of the non-nil IDs and the dropped ID never enters the merge function's loser→winner map — which is where downstream ExternalUserID/ExternalGroupID rewrites forconfig_accesscome from.Adding a test that pins down the intended behavior (either "survivor is deterministic and loser ID is preserved via the caller's remap flow" or "this shouldn't happen, we panic/warn") would keep future refactors safe.
It("surfaces a dropped ID when two distinct non-nil IDs share an alias", func() { idA := "00000000-0000-0000-0000-0000000000a1" idB := "00000000-0000-0000-0000-0000000000a2" items := []models.ExternalGroup{ mk(idA, "shared-alias", "a-only"), mk(idB, "shared-alias", "b-only"), } out, idx := dedupeByIDWithIndex(items, getID, getAliases, setAliases) Expect(out).To(HaveLen(1)) Expect(idx).To(Equal([]int{0, 0})) // Pin down which ID wins and assert something about the loser. })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/external_entities_test.go` around lines 16 - 94, Add a spec that covers two distinct non-nil IDs that share an alias using the existing helpers (getID, getAliases, setAliases, mk) and call dedupeByIDWithIndex; assert it collapses to a single survivor (Expect(out).To(HaveLen(1))), assert the survivor ID is deterministic (for now, expect the first item's ID to win, e.g. idA via Expect(out[0].ID.String()).To(Equal(idA))) and assert the index map points both inputs to that survivor (Expect(idx).To(Equal([]int{0,0}))). This pins the intended deterministic behavior of dedupeByIDWithIndex when two different non-nil IDs share an alias.db/external_entities.go (1)
365-429: Merge-failure dumps in./traces/will accumulate indefinitely and depend on a writable cwd.Two operational concerns with the dump mechanism:
writeMergeFailureDumpalways writes tofilepath.Join("traces", ...)relative to the process working directory. In containerized deployments the cwd may be/(read-only) or a runtime-provided ephemeral dir; failures will then silently fail to persist (writeErris logged but the dump is lost) and you're back to the tracef fallback. Consider making the trace dir configurable via a property (similar toCACHE_TIMEOUT) with a sane default like/tmp/config-db-traces, or falling back toos.TempDir()whenMkdirAllfails.- There's no retention/rotation. A scraper that hits this failure mode on every run will write a new JSON file each time — over weeks this can fill the volume. At minimum document the expected retention story, or add a
filepath.Glob+ mtime-based trim to a last-N files.Minor: the envelope at L447-454 is string-formatted JSON; if
mergeErr.Error()ever contains a raw tab/newline/quote that%qdoesn't handle (edge case with non-UTF8 bytes) the file becomes malformed. Building the envelope withencoding/jsonand marshalingjson.RawMessage(tempDump)/json.RawMessage(liveDump)would be more robust and no slower.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/external_entities.go` around lines 365 - 429, The dump routine in runMergeFunctionWithDump relies on writeMergeFailureDump writing into a hard-coded "traces" directory in the current working directory and never rotates files; make writeMergeFailureDump configurable and add retention: add a new configuration variable (e.g. MergeDumpDir or MERGE_DUMP_DIR) that defaults to a sane temp location (os.TempDir() or "/tmp/config-db-traces") and use that path instead of relying on cwd; update writeMergeFailureDump to attempt MkdirAll and, on failure, fall back to os.TempDir(); additionally implement simple rotation/retention inside writeMergeFailureDump (or a helper it calls) that enumerates existing dump files (filepath.Glob) and removes oldest by mtime until only N most recent remain (configurable or a reasonable constant like 10); finally, change how the JSON envelope is formed in writeMergeFailureDump to build a struct and marshal it with encoding/json using json.RawMessage for tempDump/liveDump to avoid malformed output from raw string formatting.db/external_cache.go (1)
148-161: Simplify error handling—Plucknever returnsErrRecordNotFound.The check
err != gorm.ErrRecordNotFoundis dead code: GORM v2'sPluck(a multi-record method) returns a nil error and an empty slice when no rows match;ErrRecordNotFoundis only returned by single-record methods likeFirst,Last, andTake. The subsequentif len(foundIDs) > 0correctly handles the not-found case.Additionally, the direct equality comparison is non-idiomatic; use
errors.Is()for wrapped-error safety.Simplify to:
♻️ Proposed simplification
var zero T var foundIDs []uuid.UUID err := ctx.DB().Table(zero.TableName()). Select("id"). Where("id = ? AND deleted_at IS NULL", id). Limit(1). Pluck("id", &foundIDs).Error - if err != nil && err != gorm.ErrRecordNotFound { + if err != nil { return nil, fmt.Errorf("failed to query %s by id: %w", zero.TableName(), err) } var found uuid.UUID if len(foundIDs) > 0 { found = foundIDs[0] }This removes the dead branch and eliminates the unnecessary
gormimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/external_cache.go` around lines 148 - 161, The Pluck call can never return gorm.ErrRecordNotFound (it returns nil + empty slice on no rows), so in db/external_cache.go simplify the error handling in the block that queries zero.TableName(): remove the dead check "err != gorm.ErrRecordNotFound" and just treat any non-nil err as fatal (return the wrapped error from fmt.Errorf), keep the existing len(foundIDs) > 0 handling for the not-found case, and after this change remove the now-unused gorm import; if you ever compare errors elsewhere prefer errors.Is(...) for wrapped-error checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/external_entities.go`:
- Around line 747-838: Update the comment above dedupeByIDWithIndex to clarify
that although the union-find logic (using idToIdx/aliasToIdx and union/find) can
in theory merge two non-nil IDs when they share an alias, this only happens if a
single scraper emits multiple distinct non-nil IDs; in practice that doesn't
occur because scrapers produce keyed maps (e.g.,
map[uuid.UUID]models.ExternalUser) so distinct non-nil IDs are not emitted by
the same scraper, and cross-scraper merges are handled later by the SQL
merge_and_upsert_external_* logic and remapExternalUserGroups.
In `@db/update.go`:
- Around line 791-813: The code advances access-log cursors using
extractResult.configAccessLogs (which may include rows that failed validation)
so last_created_at can jump past rows that weren't persisted; change the loop to
compute max times from the validated rows actually appended (e.g., use
extractResult.resolvedAccessLogs or the slice returned/recorded by
SaveConfigAccessLogs) instead of extractResult.configAccessLogs, updating
summary.ConfigTypes[configType].AccessLogs.LastCreatedAt and
summary.AccessLogs.LastCreatedAt from those validated timestamps; alternatively
modify SaveConfigAccessLogs to return per-configType max persisted CreatedAt
values and use that returned data to set the summary timestamps.
- Around line 660-664: The code silently increments summary.ConfigAccess.Skipped
when both configAccess.ExternalUserID and configAccess.ExternalGroupID are nil;
update that branch to emit a warning log before continuing so skipped rows are
discoverable. Modify the block that checks "if configAccess.ExternalUserID ==
nil && configAccess.ExternalGroupID == nil" to call the existing logger (or
context logger) to warn and include identifying fields from configAccess (e.g.,
ExternalUserID, ExternalGroupID, any config or row identifier present on the
struct) and then increment summary.ConfigAccess.Skipped and continue as before.
Ensure the log message mirrors the style used by the missing-role paths below
for consistency.
---
Outside diff comments:
In `@api/v1/interface.go`:
- Around line 612-626: The Merge method on ScrapeSummary currently only merges
some fields and omits the new diagnostic fields; update ScrapeSummary.Merge (the
method named Merge on type ScrapeSummary) to also merge OrphanedChanges,
FKErrorChanges, Warnings, and State from the other summary the same way other
fields are merged (i.e., initialize if needed and call the appropriate Merge
method or combine values) so that diagnostics are preserved when combining
summaries.
- Around line 430-431: The new-format detection currently checks newFormatKeys
in the slice defined in api/v1/interface.go but misses summary-only top-level
keys like "warnings" and "orphaned_changes", causing those payloads to be
misclassified as legacy; update the newFormatKeys slice (the variable named
newFormatKeys) to include "warnings" and "orphaned_changes" so these payloads
are detected as new-format JSON during the payload detection logic.
In `@db/permission_changes.go`:
- Around line 162-222: The fallback DO block leaves newRows empty so
PermissionAdded events are dropped; fix in the function handling the bulk upsert
by after the fallback path re-populating newRows (or another collector) from the
actual persisted rows — e.g., query config_access for rows whose ids were in the
original items set but no longer exist in tempTable, and assign those results to
newRows before the loop that builds result.added (references: tempTable,
newRows, result.added, buildPermissionSummary). Also handle errors for
tx.Exec("SAVEPOINT bulk_insert") and the fkErrorCount scan (check and return/log
errors rather than ignoring) so savepoint creation and count failures don’t
silently corrupt transaction state or metrics (references: the SAVEPOINT exec
and the fkErrorCount tx.Raw(...).Scan call); keep existing FK
diagnostics/logFKDiagnostics behavior.
In `@db/update.go`:
- Around line 1734-1747: The code in extractChanges is skipping
chResult.changeSummary when ChangeSummary.IsEmpty() returns true, but IsEmpty()
currently ignores IgnoredByAction and ForeignKeyErrors so summaries that only
contain ignored-by-action counts get dropped; update the logic so these
ignored-only summaries are preserved—either by changing the predicate here to
consider chResult.changeSummary.HasIgnoredByAction() ||
chResult.changeSummary.HasForeignKeyErrors() in addition to !IsEmpty(), or by
updating ChangeSummary.IsEmpty() to treat IgnoredByAction/ForeignKeyErrors as
non-empty; then ensure extractResult.changeSummary.Merge(configType,
chResult.changeSummary) still runs for those cases (symbols: extractChanges,
chResult.changeSummary, ChangeSummary.IsEmpty, AddIgnoredByAction,
ForeignKeyErrors, extractResult.changeSummary.Merge).
- Around line 783-786: The branch that checks accessLog.ExternalUserID ==
uuid.Nil emits a warning and continues but fails to increment the skipped
counter, causing underreported totals; update the block that calls
summary.AddWarning(...) to also increment summary.AccessLogs.Skipped (or the
appropriate skipped field on the summary struct) before continuing so every
skipped access log is counted, ensuring you reference accessLog.ExternalUserID
and summary.AccessLogs.Skipped in the same branch.
---
Nitpick comments:
In `@db/change_traversal.go`:
- Around line 17-33: The resolveChange helper constructs ChangeResult.Resolved
but can silently omit future fields and relies on callers to supply a meaningful
Action; add a short comment above resolveChange and the struct literal saying
"// keep in sync with dutyModels.ConfigChange" and/or note which fields must be
preserved, and audit all callers (e.g., usages in MoveUp/CopyUp/Move/Copy and
db/update.go branches for Ignore/Delete/exclusion/orphan paths) to ensure they
pass an explicit, well-known Action string (or a documented empty marker) so
ignored/deleted/skipped changes get a sensible Action value.
In `@db/external_cache.go`:
- Around line 148-161: The Pluck call can never return gorm.ErrRecordNotFound
(it returns nil + empty slice on no rows), so in db/external_cache.go simplify
the error handling in the block that queries zero.TableName(): remove the dead
check "err != gorm.ErrRecordNotFound" and just treat any non-nil err as fatal
(return the wrapped error from fmt.Errorf), keep the existing len(foundIDs) > 0
handling for the not-found case, and after this change remove the now-unused
gorm import; if you ever compare errors elsewhere prefer errors.Is(...) for
wrapped-error checks.
In `@db/external_entities_test.go`:
- Around line 16-94: Add a spec that covers two distinct non-nil IDs that share
an alias using the existing helpers (getID, getAliases, setAliases, mk) and call
dedupeByIDWithIndex; assert it collapses to a single survivor
(Expect(out).To(HaveLen(1))), assert the survivor ID is deterministic (for now,
expect the first item's ID to win, e.g. idA via
Expect(out[0].ID.String()).To(Equal(idA))) and assert the index map points both
inputs to that survivor (Expect(idx).To(Equal([]int{0,0}))). This pins the
intended deterministic behavior of dedupeByIDWithIndex when two different
non-nil IDs share an alias.
In `@db/external_entities.go`:
- Around line 365-429: The dump routine in runMergeFunctionWithDump relies on
writeMergeFailureDump writing into a hard-coded "traces" directory in the
current working directory and never rotates files; make writeMergeFailureDump
configurable and add retention: add a new configuration variable (e.g.
MergeDumpDir or MERGE_DUMP_DIR) that defaults to a sane temp location
(os.TempDir() or "/tmp/config-db-traces") and use that path instead of relying
on cwd; update writeMergeFailureDump to attempt MkdirAll and, on failure, fall
back to os.TempDir(); additionally implement simple rotation/retention inside
writeMergeFailureDump (or a helper it calls) that enumerates existing dump files
(filepath.Glob) and removes oldest by mtime until only N most recent remain
(configurable or a reasonable constant like 10); finally, change how the JSON
envelope is formed in writeMergeFailureDump to build a struct and marshal it
with encoding/json using json.RawMessage for tempDump/liveDump to avoid
malformed output from raw string formatting.
In `@db/permission_changes.go`:
- Around line 405-412: The current loop over counts (map variable counts) is
nondeterministic because Go map iteration order is random, so the top-10 output
(loop using logged, key.Reason, key.FKID, count) can vary between runs; fix by
creating a slice of entries from counts (e.g., struct with key and count), sort
that slice by count descending using sort.Slice, then iterate the first min(10,
len(slice)) entries and call ctx.Logger.Warnf for each; remember to add "sort"
to the imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a405ee65-2e8c-4071-830f-12c7a45cdd34
📒 Files selected for processing (10)
api/v1/interface.goapi/v1/interface_test.godb/change_traversal.godb/external_cache.godb/external_entities.godb/external_entities_test.godb/external_loser_alias_test.godb/permission_changes.godb/update.goscrapers/config_access_test.go
…external_role_aliases in their config_access entries. The validation at db/update.go:666 requires every config_access to have a role — without one, the entries are skipped with the "missing role" warning, resulting in 0 config access rows instead of 2. Changes made: 1. fixtures/data/incremental_rbac_full_test.json — added external_roles section with a "Viewer" role, and added external_role_aliases to both config_access entries 2. fixtures/data/incremental_rbac_partial_test.json — same: added role and role alias reference 3. scrapers/incremental_rbac_test.go — added ExternalRole cleanup in AfterAll to avoid the same FK violation we just fixed
adityathebe
force-pushed
the
feat/external-entity-dedupe
branch
from
April 20, 2026 16:10
1aa1f8c to
ad3a167
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deduplication:
FK violation recovery (config_access + config_access_logs):
Entity resolution:
Merge function resilience:
Config access validation:
Scrape summary improvements:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes