chore: add external user alias resolver - #2340
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds synchronized external-entity caches, canonical external-user alias and historical-ID resolution, readiness-aware table notifications, periodic cache refreshes, startup error handling, and validation coverage. ChangesExternal user alias lifecycle
Sequence Diagram(s)sequenceDiagram
participant Serve
participant startTableUpdatesHandler
participant PostgreSQL
participant tableUpdatesHandler
participant RefreshExternalUserCaches
Serve->>startTableUpdatesHandler: start listener
startTableUpdatesHandler->>tableUpdatesHandler: launch handler
startTableUpdatesHandler->>PostgreSQL: probe readiness
PostgreSQL-->>tableUpdatesHandler: readiness notification
tableUpdatesHandler-->>startTableUpdatesHandler: readiness confirmation
Serve->>RefreshExternalUserCaches: warm caches
PostgreSQL-->>tableUpdatesHandler: external-user update
tableUpdatesHandler->>RefreshExternalUserCaches: refresh caches
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
c655f7b to
202a6c8
Compare
BenchstatBase: ✅ 1 improvement(s)
Full benchstat output |
Gavel summary
Totals: 1085 passed · 0 failed · 5 skipped · 4m0s |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
db/external_user_alias_mapping_test.go (1)
140-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis spec does not exercise the path it names.
Line 141 clears only
ExternalUserIDCache. The preceding spec at Lines 125-138 calledapplyExternalUserAliasMapping, andfindExternalUserIDsInAliasMappingrepopulatedExternalUserCachewithhistoricalID.String() -> canonicalID.
findExternalEntityByIDthen misses the ID cache, queriesexternal_usersforid = historicalIDand finds nothing, and falls through tofindExternalEntityIDByAliases. That call hits the still-populatedExternalUserCacheand returnscanonicalID. The spec passes through the alias cache, so it would still pass if the database alias-mapping query were broken.Clear both caches to force the database path:
💚 Proposed fix
It("resolves direct references to a historical user ID", func() { - ExternalUserIDCache.Delete(historicalID.String()) + ExternalUserIDCache.Flush() + ExternalUserCache.Flush() resolved, err := findExternalEntityByID[dutymodels.ExternalUser](ctx, historicalID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/external_user_alias_mapping_test.go` around lines 140 - 146, Update the “resolves direct references to a historical user ID” spec to clear both ExternalUserIDCache and ExternalUserCache before calling findExternalEntityByID, ensuring the assertion exercises the database lookup path rather than stale alias-cache data.db/external_entities.go (1)
326-328: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAn ambiguous alias fails the entire external-entity sync.
This error propagates through
resolveExternalUsersat Line 366 intosyncExternalEntities, which returns immediately. One upstream identity whose keys resolve to two canonical users therefore blocks the sync of all users, all groups, all roles, and all user-group linkages for that scrape.The surrounding code handles bad identity data differently.
resolveExternalUsersskips a user with no ID and no aliases and incrementsskippedat Lines 368-372.resolveExternalUserGroupsdrops unresolvable rows and reports av1.Warning. Ambiguity is upstream data, not a programming fault, so it fits the same posture.Skip the ambiguous user, count it, and surface it as a warning instead of aborting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/external_entities.go` around lines 326 - 328, Update the external-user resolution flow around the mappedIDs ambiguity check and resolveExternalUsers so users resolving to multiple canonical IDs are skipped rather than returned as errors. Increment the existing skipped count and emit a v1.Warning for the ambiguous identity, allowing syncExternalEntities to continue processing all other users, groups, roles, and linkages.db/external_cache.go (2)
306-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated alias-validation join.
This join block is identical to the one in
warmExternalUserAliasMappingsat Lines 168-173. The two copies must stay in sync, because both enforce the same "index row is valid only while the alias is still inexternal_users.aliases" rule. Extract one helper that returns the scoped*gorm.DB, then add the differingWhereclause at each call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/external_cache.go` around lines 306 - 318, Extract the shared external-user alias validation JOIN from warmExternalUserAliasMappings and the current mapping query into one helper returning the scoped *gorm.DB. Replace both duplicated join blocks with that helper, while keeping each call site’s distinct Where clause applied afterward.
120-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the
external_user_aliasesexistence check.
externalUserAliasTableExistsruns a catalog query on every call.findExternalUserIDsInAliasMappingcalls it once per unresolved external user during a scrape, so a large scrape adds one extra round trip per user. Memoize the result with a short TTL or refresh it inRefreshExternalUserCaches. Do not memoize afalseresult forever, because a later Duty migration can add the table at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/external_cache.go` around lines 120 - 129, Cache the result of externalUserAliasTableExists with a short TTL, or refresh it from RefreshExternalUserCaches, so repeated calls avoid querying the catalog for every unresolved user. Ensure cached true and false results expire or are refreshed, allowing a later Duty migration to detect the newly created table; preserve the existing nil-database and query-error behavior.db/external_loser_alias_test.go (1)
97-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not swallow the existence-check error, and clean up the index rows.
Two points on this block:
Line 97 treats an error from
externalUserAliasTableExiststhe same as an absent table. A database error therefore skips every assertion and the spec still passes. Assert the error separately so a real failure is visible.The deferred cleanup at Lines 47-49 deletes only
external_usersrows. Nothing deletes theexternal_user_aliasesrows that the merge created. If the Duty schema has no cascade fromexternal_usersto that index, each run leaves orphan index rows whoseexternal_user_idpoints to a deleted user.💚 Proposed fix
- if hasAliasIndex, err := externalUserAliasTableExists(DefaultContext.DB()); err == nil && hasAliasIndex { + hasAliasIndex, err := externalUserAliasTableExists(DefaultContext.DB()) + Expect(err).NotTo(HaveOccurred()) + if hasAliasIndex { migratedAlias := sharedAliasExtend the existing deferred cleanup at Lines 47-49:
defer func() { DefaultContext.DB().Exec( "DELETE FROM external_user_aliases WHERE external_user_id IN ?", []uuid.UUID{winnerID, loserID, bridgeID}, ) DefaultContext.DB().Unscoped().Delete(&dutymodels.ExternalUser{}, "id IN ?", []uuid.UUID{winnerID, loserID, bridgeID}) }()Guard the added delete so it does not fail when the table is absent, or run it inside the same existence check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/external_loser_alias_test.go` around lines 97 - 112, Update the external alias test cleanup defer to delete rows from external_user_aliases for winnerID, loserID, and bridgeID before removing the external users, while safely handling an absent table. In the externalUserAliasTableExists check, assert the returned error separately so database failures fail the test instead of skipping assertions; only run alias assertions when the table exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/external_cache.go`:
- Around line 376-384: Update the alias filtering in
findAllExternalEntityIDsByAliases to distinguish aliases resolved by this call
from entries added concurrently to aliasCache. When dropping an alias because
its cache entry exists, add that cached ID to seen; only retain aliases for
fallback when they remain unresolved by both the call’s mappedIDs and the cache.
- Around line 166-178: Update the cache-warming query in the
external_user_aliases lookup to normalize eua.alias with trimming and
lowercasing before comparing it to the normalized source alias. Preserve the
existing deleted-record filters and cache error handling while ensuring both
sides of the EXISTS comparison use the same normalized representation.
In `@db/external_entities.go`:
- Around line 332-344: Reapply the existing ambiguity validation after the email
fallback updates mappedIDs in the external-user resolution flow. Ensure multiple
IDs from findCachedExternalUserIDs or findExternalUserIDsInAliasMapping return
the same ambiguity error as the earlier stronger-key path before the subsequent
len(mappedIDs) == 1 handling.
---
Nitpick comments:
In `@db/external_cache.go`:
- Around line 306-318: Extract the shared external-user alias validation JOIN
from warmExternalUserAliasMappings and the current mapping query into one helper
returning the scoped *gorm.DB. Replace both duplicated join blocks with that
helper, while keeping each call site’s distinct Where clause applied afterward.
- Around line 120-129: Cache the result of externalUserAliasTableExists with a
short TTL, or refresh it from RefreshExternalUserCaches, so repeated calls avoid
querying the catalog for every unresolved user. Ensure cached true and false
results expire or are refreshed, allowing a later Duty migration to detect the
newly created table; preserve the existing nil-database and query-error
behavior.
In `@db/external_entities.go`:
- Around line 326-328: Update the external-user resolution flow around the
mappedIDs ambiguity check and resolveExternalUsers so users resolving to
multiple canonical IDs are skipped rather than returned as errors. Increment the
existing skipped count and emit a v1.Warning for the ambiguous identity,
allowing syncExternalEntities to continue processing all other users, groups,
roles, and linkages.
In `@db/external_loser_alias_test.go`:
- Around line 97-112: Update the external alias test cleanup defer to delete
rows from external_user_aliases for winnerID, loserID, and bridgeID before
removing the external users, while safely handling an absent table. In the
externalUserAliasTableExists check, assert the returned error separately so
database failures fail the test instead of skipping assertions; only run alias
assertions when the table exists.
In `@db/external_user_alias_mapping_test.go`:
- Around line 140-146: Update the “resolves direct references to a historical
user ID” spec to clear both ExternalUserIDCache and ExternalUserCache before
calling findExternalEntityByID, ensuring the assertion exercises the database
lookup path rather than stale alias-cache data.
🪄 Autofix
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 Plus
Run ID: 2d400787-4652-4291-9c44-1c1e66ebb0a3
📒 Files selected for processing (5)
cmd/server.godb/external_cache.godb/external_entities.godb/external_loser_alias_test.godb/external_user_alias_mapping_test.go
0c5de9e to
2c7a99e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/server.go (1)
84-98: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueProbe loop is correct, but the first probe can race the listener registration.
The loop sends the readiness probe before the router has registered the
LISTEN. That first probe is lost, and the 100 ms ticker retries until readiness or timeout, so the handshake still converges. ThelastErrcapture keeps the last probe failure in the timeout message.One detail: if
ctx.DB().Execfails on every iteration, the function still waits the full 10 s before reporting. Consider returning early after a fixed number of consecutive probe failures so a broken connection surfaces faster.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/server.go` around lines 84 - 98, Update the readiness probe loop around ctx.DB().Exec to track consecutive probe failures and return an error after a fixed threshold, rather than waiting for the full timeout when every probe fails. Preserve the existing retry behavior for transient failures, readiness-channel success, context cancellation, and timeout reporting with the last probe error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/external_cache.go`:
- Around line 310-312: Restore deterministic database recovery for external-user
cache misses: at db/external_cache.go lines 310-312, replace the unconditional
ExternalUser early return with a database lookup of the persisted external user,
while retaining cache fallback behavior for stale misses. Apply the same
recovery to the user alias-lookup path. cmd/run.go lines 98-100 requires no
direct change; its startup cache warming is evidence that later scrapers need
the database fallback.
In `@db/external_user_alias_mapping_test.go`:
- Around line 63-68: Update the AfterAll cleanup to delete external_user_aliases
by external_user_id using canonicalID rather than the incomplete mappingIDs
list, ensuring all trigger-created rows are removed. Since mappingIDs is then
only needed for BeforeAll assertions, remove the staleMappingID append in the
setup.
- Around line 80-95: Replace the inline defer in the spec around
RefreshExternalUserCaches with Ginkgo’s DeferCleanup, registering the rename
from external_user_aliases_unavailable back to external_user_aliases immediately
after the initial rename. Preserve the existing cleanup assertion and ensure it
runs even when later expectations in the test fail.
In `@go.mod`:
- Around line 8-10: Update the github.com/flanksource/commons dependency in
go.mod from the unavailable v1.55.0 to a published version such as v1.54.1,
while leaving the github.com/flanksource/deps and github.com/flanksource/duty
requirements unchanged.
---
Nitpick comments:
In `@cmd/server.go`:
- Around line 84-98: Update the readiness probe loop around ctx.DB().Exec to
track consecutive probe failures and return an error after a fixed threshold,
rather than waiting for the full timeout when every probe fails. Preserve the
existing retry behavior for transient failures, readiness-channel success,
context cancellation, and timeout reporting with the last probe error.
🪄 Autofix
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 Plus
Run ID: 4b6bae37-86b0-458b-8b9e-3b0b4c775484
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/operator.gocmd/run.gocmd/server.godb/external_cache.godb/external_entities.godb/external_entities_test.godb/external_loser_alias_test.godb/external_user_alias_mapping_test.gogo.modscrapers/cron.goscrapers/external_entities_test.go
💤 Files with no reviewable changes (1)
- scrapers/cron.go
🚧 Files skipped from review as they are similar to previous changes (2)
- db/external_loser_alias_test.go
- db/external_entities.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scrapers/extract_e2e_test.go`:
- Around line 362-368: Register the fixture cleanup immediately after creating
scraperModel, before the conditional RefreshExternalUserCaches call or any other
setup that can fail. Ensure the cleanup removes the database rows and scraper
records even when cache warming fails, while preserving the existing refresh
behavior.
- Around line 365-367: Update the fixture setup condition around
ExternalUserGroups to refresh the global external-user cache whenever external
users are pre-populated, including fixtures with users but no groups. In the
cleanup path for those pre-populated external users, invoke
RefreshExternalUserCaches again after deletion so later fixtures cannot observe
stale aliases.
🪄 Autofix
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 Plus
Run ID: 8be9766f-ded6-4605-a4ac-177707bdc796
📒 Files selected for processing (2)
db/config.goscrapers/extract_e2e_test.go
2 participants
Summary by CodeRabbit
Improvements
Reliability