[management] migrate auto groups to separate table #5075
Conversation
📝 WalkthroughWalkthroughAdds a relational GroupUser model and persistence, store APIs to add/remove user↔group links, migrations to convert JSON auto_groups to relational records, type changes to User/Group/Account, centralized transactional group-update logic with peer propagation, and SyncUserJWTGroups no longer computes add/remove diffs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant DB as Database
participant Store
participant Peers
Client->>Service: create/invite/update user (with groups)
Service->>DB: begin transaction
DB->>Store: save user record
DB->>Store: AddUserToGroup / RemoveUserFromGroup
Store-->>DB: ack
DB->>Store: StoreAutoGroups / update user.Groups
alt GroupsPropagationEnabled
Store->>Store: fetch affected peers
Store->>Peers: propagate added/removed group memberships
Peers-->>Store: ack
end
DB-->>Service: commit
Service-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
management/server/user.go (1)
743-779: Critical: group membership diffing is broken by callingStoreAutoGroupstoo earlyIn
processUserUpdate(line 748),StoreAutoGroups()is called immediately after settingupdatedUser.AutoGroups = update.AutoGroups. This clearsupdatedUser.AutoGroupsto an empty slice:updatedUser.AutoGroups = update.AutoGroups updatedUser.StoreAutoGroups() // clears AutoGroups to []string{}When
processUserGroupsUpdatecomputes the group diffs shortly after (line 782–783), it receives a user with an emptyAutoGroupsfield:removedGroups := util.Difference(oldUser.AutoGroups, updatedUser.AutoGroups) // all old groups addedGroups := util.Difference(updatedUser.AutoGroups, oldUser.AutoGroups) // always emptyThis causes:
- Any group addition to be silently ignored (empty
addedGroups)- Any group modification to remove all existing groups (all old groups in
removedGroups)Fix: Move
StoreAutoGroups()to afterprocessUserGroupsUpdate()completes its diff computation, so it operates on the intendedupdate.AutoGroupsvalues. Alternatively, skip the early call and rely onSaveUser(which already callsStoreAutoGroupsinternally) to persist the relational state.management/server/store/sql_store.go (1)
121-126: SqlStore wiring for GroupUser and auto-groups is functionally correct; clarify handling of group persistence in SaveUserKey observations:
NewSqlStoreauto‑migrates&types.GroupUser{}andSaveAccountproperly:
- Encrypts users after
StoreAutoGroups().- Omits
UsersG.Groupswhen persisting to avoid FK constraint violations.- Then explicitly upserts
group_userswithOnConflict(group_id,user_id).SaveUsers/SaveUsercallStoreAutoGroups()on copies before encrypting and persisting. GORM'sSave()will create/update group_user records present in the Groups slice, but will not delete orphaned ones (FullSaveAssociations is not used here, unlike SaveAccount).- All user retrieval paths preload
Groups, then callLoadAutoGroups()so callers seeAutoGroupspopulated andGroupscleared.- The PGX account path correctly fetches and buckets
group_usersby userID beforeLoadAutoGroups().Two behavioral details to clarify:
Redundant group persistence in SaveUser
processUserUpdatecallsStoreAutoGroups()thenSaveUser(), which persists groups via GORM'sSave().- Immediately after,
processUserGroupsUpdate()explicitly callsAddUserToGroup/RemoveUserFromGroupto handle membership changes.- The Save() call will create/update present groups but won't clean up orphaned records. Since all membership changes flow through the explicit APIs, the group persistence via SaveUser is redundant and inconsistent with SaveAccount's explicit handling. Consider whether SaveUser should use FullSaveAssociations (to fully sync groups) or omit group persistence entirely (if it's always managed separately).
RemoveUserFromGroupassumes globally unique group IDs
- Deletes by
(group_id, user_id)only, with no accountID filter. This assumes group IDs are unique across all accounts, which is safe if true but worth documenting.Overall, the SQL store migration is coherent and the functional flow is sound; these are design clarity points rather than correctness issues.
🤖 Fix all issues with AI agents
In @management/server/types/user.go:
- Around line 88-92: In StoreAutoGroups change the slice preallocation to use
the AutoGroups length: replace the current make([]*GroupUser, 0, len(u.Groups))
with make([]*GroupUser, 0, len(u.AutoGroups)); this ensures the loop over
u.AutoGroups in StoreAutoGroups (and related behavior in LoadAutoGroups/Copy)
preallocates the correct capacity and avoids unnecessary reallocations or
mismatches if AutoGroups and Groups differ.
In @management/server/user.go:
- Around line 143-158: In inviteNewUser, the transaction currently adds
auto-groups using the initiator's userID and redundantly calls am.Store.SaveUser
after the transaction; fix it by using the newly created user's ID (newUser.Id)
when calling tx.AddUserToGroup inside ExecuteInTransaction, remove the extra
am.Store.SaveUser(ctx, newUser) call outside the transaction, and ensure
newUser.AutoGroups is populated from invite.AutoGroups before returning so the
API response includes the assigned groups.
🧹 Nitpick comments (4)
management/server/user_test.go (1)
1344-1353: Check error fromGetAccountin SaveUser testYou now re-fetch the account with
manager.Store.GetAccount, but ignoreerr. If this ever fails, the test will proceed with a staleaccountand silently pass over a real store issue.Consider asserting on the error or removing the re-fetch if it’s not needed.
Proposed test fix
- account, err = manager.Store.GetAccount(context.Background(), account.Id) - - updated, err := manager.SaveUser(context.Background(), account.Id, tc.initiatorID, tc.update) + account, err = manager.Store.GetAccount(context.Background(), account.Id) + require.NoError(t, err) + + updated, err := manager.SaveUser(context.Background(), account.Id, tc.initiatorID, tc.update)management/server/account.go (1)
1405-1420: JWT group sync now correctly updates relational memberships and peersUsing
addNewGroups/removeOldGroupsto driveAddUserToGroup/RemoveUserFromGroup, then savinguser.AutoGroupsinside the same transaction, keeps the newGroupUsertable and the denormalized list in sync. The optional peer propagation underGroupsPropagationEnabledand singleIncrementNetworkSerialalso look consistent with the rest of the account update flow.Given typical per-user group counts, the per-group store calls are fine; if you ever see many groups per user, a batched store API could be a future optimization, but it’s not required now.
Also applies to: 1425-1447
management/server/migration/migration.go (1)
491-589: MakeCleanupOrphanedIDslogging generic, not auto_groups-specificThe migration logic itself looks solid, but the helper is generic while the messages are hard‑coded to groups/auto_groups:
- Line 529:
fmt.Errorf("fetch valid group IDs: %w", err)assumes the FK model is a group.- Line 587:
"Cleanup of orphaned auto_groups from table %s completed"hardcodes the column name.Consider parameterizing these messages by
columnName(and maybeS’s type) so the function can be safely reused for other JSON ID lists without confusing logs.management/server/user.go (1)
550-627:updateAccountPeersis now always set true for successful updatesInside
SaveOrAddUsers, after callingprocessUserUpdateyou ignore itsboolresult and unconditionally set:updateAccountPeers = trueThis means that as long as at least one user update succeeds (even if it doesn’t change groups or blocking status), you will:
- Increment the network serial.
- Call
UpdateAccountPeers.That’s probably more churn than necessary, especially for bulk updates that only touch metadata (roles, flags) without affecting peer routing.
Consider:
- Using the
boolreturned byprocessUserUpdate(OR‑ed across users) instead of hardcodingtrue.- Leaving the current behavior in place only if you explicitly want every successful user update to refresh peers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
management/server/account.gomanagement/server/account_test.gomanagement/server/migration/migration.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_test.gomanagement/server/store/sqlstore_bench_test.gomanagement/server/store/store.gomanagement/server/types/account.gomanagement/server/types/group.gomanagement/server/types/user.gomanagement/server/user.gomanagement/server/user_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
management/server/account.go (2)
shared/management/status/error.go (1)
Errorf(70-75)management/server/util/util.go (1)
Difference(4-16)
management/server/types/account.go (2)
management/server/types/user.go (1)
User(77-109)shared/management/http/api/types.gen.go (1)
User(1917-1960)
management/server/types/user.go (3)
management/server/types/group.go (1)
GroupUser(45-49)shared/management/http/api/types.gen.go (2)
PersonalAccessToken(1356-1374)User(1917-1960)management/server/types/personal_access_token.go (1)
PersonalAccessToken(29-40)
management/server/store/store.go (4)
management/server/migration/migration.go (2)
CleanupOrphanedIDs(494-589)MigrateJsonToTable(432-489)management/server/types/user.go (1)
User(77-109)shared/management/http/api/types.gen.go (2)
User(1917-1960)Group(482-501)management/server/types/group.go (2)
Group(15-37)GroupUser(45-49)
management/server/store/sql_store_test.go (1)
management/server/types/group.go (1)
GroupUser(45-49)
management/server/account_test.go (2)
management/server/permissions/modules/module.go (1)
Groups(8-8)management/server/types/group.go (1)
GroupUser(45-49)
management/server/store/sql_store.go (3)
management/server/peer/peer.go (1)
Peer(16-58)management/server/types/user.go (1)
User(77-109)management/server/types/group.go (3)
Group(15-37)GroupPeer(39-43)GroupUser(45-49)
management/server/store/sqlstore_bench_test.go (2)
management/server/permissions/modules/module.go (2)
Groups(8-8)Users(16-16)management/server/types/group.go (1)
GroupUser(45-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Build Cache
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: JS / Lint
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: release_ui
🔇 Additional comments (10)
management/server/store/sql_store_test.go (1)
1368-1368: LGTM! Test initialization updated for new GroupUsers field.The addition of
GroupUsers: []types.GroupUser{}to the Group struct literals is the necessary test maintenance to accommodate the new GroupUser association field introduced in the Group type.Also applies to: 1393-1393, 1402-1402
management/server/types/account.go (1)
90-90: No action needed. All code accessingUsersGproperly handles pointer semantics. The slice of pointers type is already correctly used throughout the codebase in range loops, assignments, appends, and GORM preloading statements.management/server/account_test.go (1)
1722-1733: Account copy test correctly exercises new GroupUser relationsWiring
User.GroupsandGroup.GroupUserswith matchingAccountID/UserID/GroupIDis consistent and gives good coverage thatAccount.Copypreserves the new relationship graph.Also applies to: 1747-1759
management/server/user_test.go (2)
341-352: User copy test now covers Groups associationInitializing
Groupswith aGroupUserinTestUser_CopykeepsvalidateStructandUser.Copyaligned with the new field and gives coverage for the new association. Looks good.
1648-1663: Including AutoGroups in current-user expectations is appropriateExtending
expectedResult.UserInfoto assertAutoGroups: []string{}for regular/admin and settings-blocked users keeps the tests aligned with the new API surface and enforces a consistent empty-slice contract for callers.Also applies to: 1670-1683, 1688-1702, 1711-1725
management/server/store/sqlstore_bench_test.go (3)
79-87: Slow-path now hydrates AutoGroups viaLoadAutoGroupsCalling
user.LoadAutoGroups()beforeuser.Copy()ensures the slow benchmark path reflects the new GroupUser-backed AutoGroups representation, keeping it comparable with the optimized implementations.
173-185: GORM-opt path correctly normalizes groups and fixes user pointer mappingNormalizing
AutoGroups/Groupsto empty slices avoids nil-vs-empty mismatches in equivalence tests, and assigningaccount.Users[user.Id] = useruses the actual user pointer instead of the loop variable’s address, preventing aliasing issues.
903-914: Pure-SQL path now uses the stored user pointer instead of&loop varUsing
user := account.UsersG[i]and storinguserdirectly intoaccount.Usersavoids taking the address of the loop variable and keeps user instances consistent with the other retrieval paths.management/server/store/store.go (1)
92-93: Store interface and migration wiring for user–group relations look consistentThe new Store methods and the pre/post-auto migration steps (cleanup + JSON→GroupUser) are coherent with the SqlStore implementation and the new GroupUser schema; no issues spotted here.
Also applies to: 355-357, 389-397
management/server/types/group.go (1)
31-32: [Your rewritten review comment text here]
[Exactly ONE classification tag]
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @management/server/store/sqlstore_bench_test.go:
- Around line 93-95: The code sets account.Groups[group.ID] = nil when a group
has no users, which inserts a nil value that can trigger nil pointer
dereferences; instead, remove the map entry or avoid inserting it: replace the
nil assignment with delete(account.Groups, group.ID) or simply omit adding
account.Groups[group.ID] for cases where group.GroupUsers is empty (refer to the
account.Groups map, group.ID key, and the group.GroupUsers check to locate the
code).
- Around line 621-626: The test currently skips equality when one of
oldVal/newVal is nil, allowing mismatches to pass; change the logic to fail if
one is nil and the other isn't by asserting both are nil or both are non-nil
(e.g., assert.True(t, (oldVal == nil && newVal == nil) || (oldVal != nil &&
newVal != nil), "Group with ID '%s' nil mismatch", key)); then, if both are
non-nil, sort.Strings(oldVal.Peers) and sort.Strings(newVal.Peers) and
assert.Equal(t, *oldVal, *newVal, "Group with ID '%s' should be equal", key);
keep the existing assert.True(t, ok, ...) for existence but ensure value
nil-mismatch is checked first.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/store/sqlstore_bench_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/store/sqlstore_bench_test.go (1)
management/server/types/group.go (3)
Group(15-37)GroupUser(45-49)GroupPeer(39-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release
- GitHub Check: JS / Lint
🔇 Additional comments (6)
management/server/store/sqlstore_bench_test.go (6)
85-85: LGTM - LoadAutoGroups integration.The call to
LoadAutoGroups()correctly populates the user's auto groups from the new GroupUser relationships before copying the user data.
182-186: LGTM - Proper empty slice initialization.Loading auto groups and initializing empty slices ensures consistent non-nil state, which is good defensive programming.
187-187: LGTM - Corrects loop variable address bug.Good fix! Since
account.UsersGis now a slice of pointers ([]*User), usinguserdirectly is correct. The old code taking&userwould have caused all map entries to reference the same loop variable address.
200-202: LGTM - Empty slice initialization.Properly initializes
GroupUsersto an empty slice to ensure non-nil state.
271-271: LGTM - GroupUser model migration.Correctly adds the new
GroupUsermodel to the migration list for test database setup.
914-914: LGTM - Correct pointer handling.Properly uses the pointer directly from
account.UsersG[i]since the slice now contains pointers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @management/server/store/sql_store.go:
- Around line 503-520: In SaveUser you open a transaction with
ExecuteInTransaction but call s.db directly, so DB operations aren't executed
within the transaction; change the saves to use the transaction parameter (tx)
or obtain the transactional *gorm.DB via the s.transaction() helper like
SaveAccount does — e.g., replace s.db.Omit("Groups").Save(...) and
s.db.Save(...) with tx.Omit("Groups").Save(...) / tx.Save(...) (or use
s.transaction(ctx) to get the transactional *gorm.DB) so both user and groups
are persisted inside the same transaction.
🧹 Nitpick comments (2)
management/server/store/sql_store.go (2)
2741-2759: Consider transaction context for AddUserToGroup.While the current implementation is correct for single operations, consider whether this method should accept an optional transaction context. This would allow callers to perform multiple group membership changes atomically. However, this may be deferred if the current design sufficiently meets requirements.
2761-2774: LGTM with same transaction consideration.The implementation correctly removes user-group associations and appropriately logs when the user wasn't in the group. The same transaction context consideration from
AddUserToGroupapplies here.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/store/sql_store.gomanagement/server/store/sql_store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/store/sql_store_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
🔇 Additional comments (5)
management/server/store/sql_store.go (5)
122-122: LGTM: GroupUser table migration added.The addition of
&types.GroupUser{}to AutoMigrate correctly introduces the new relational table for group-user associations.
2104-2118: LGTM: getGroupUsers implementation is correct.The new function follows the same pattern as
getGroupPeersand correctly fetchesGroupUserassociations for the given user IDs.
1664-1704: No changes required. The return type change from[]types.Userto[]*types.Useris correct and aligns with the declared type ofAccount.UsersGfield ([]*User). Both callers in the codebase handle the pointer semantics correctly with standard iteration and indexing patterns.
122-122: Data migration for GroupUser is properly handled.The
migratePostAuto()function explicitly backfills theGroupUsertable from the oldAutoGroupsJSON field usingmigration.MigrateJsonToTable[types.User](). This migration runs during database initialization (before the store is returned), so existing accounts are migrated automatically. The bidirectional sync viaLoadAutoGroups()/StoreAutoGroups()ensures backward compatibility for accounts with pending migrations.
207-229: FK constraint ordering is correctly handled.The code properly saves the account with
FullSaveAssociationswhich creates all groups (account.GroupsG) before the loop attempts to createGroupUserrecords. SinceGroupUserreferences bothUserID(created in the same FullSaveAssociations call) andGroupID(also created in FullSaveAssociations), the FK constraints are satisfied. TheOmit("UsersG.Groups")prevents premature GroupUser creation that might reference non-existent groups.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @management/server/types/user.go:
- Line 89: Update the struct field comment so it matches the actual field name
"Groups" (not "GroupUsers"); find the comment above the "Groups" field in
management/server/types/user.go and change the text to describe "Groups" (e.g.,
"Groups replaces old AutoGroups") so the comment accurately references the field
name.
- Around line 223-227: The current Copy() implementation shallow-copies the
slice u.Groups into groupUsers, leaving shared pointers; replace that with a
deep copy by iterating over u.Groups and creating new GroupUser instances (or
calling a GroupUser.Copy/Clone if one exists) and assigning each newly allocated
copy into groupUsers so the returned User has independent GroupUser objects;
update the logic around groupUsers in the Copy() method to allocate the slice
with the same length and populate it with deep-copied elements from u.Groups.
🧹 Nitpick comments (1)
management/server/types/user.go (1)
119-129: Consider validating AccountID and Id before creating GroupUser records.Lines 122-126 create
GroupUserobjects usingu.AccountIDandu.Idwithout checking if these fields are populated. If either field is empty, it could lead to database constraint violations or invalid records in the join table.🛡️ Suggested validation
func (u *User) StoreAutoGroups() { + if u.AccountID == "" || u.Id == "" { + // Either skip the operation or return an error + return + } u.Groups = make([]*GroupUser, 0, len(u.AutoGroups)) for _, groupID := range u.AutoGroups { u.Groups = append(u.Groups, &GroupUser{ AccountID: u.AccountID, GroupID: groupID, UserID: u.Id, }) } u.AutoGroups = []string{} }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/types/user.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/types/user.go (3)
management/server/types/group.go (1)
GroupUser(45-49)shared/management/http/api/types.gen.go (2)
PersonalAccessToken(1356-1374)User(1917-1960)management/server/types/personal_access_token.go (1)
PersonalAccessToken(29-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui_darwin
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/group_test.go (1)
396-418: Critical: Unreachable code due to early return.The return statement at line 400 makes lines 402-418 unreachable, leaving the test account incompletely initialized. Tests like
TestDefaultAccountManager_DeleteGroupexpect resources (routes, name server groups, policies, setup keys, users) to be present, but these are never populated due to the early return.🐛 Proposed fix to remove early return
account, err = am.Store.GetAccount(context.Background(), accountID) if err != nil { return nil, nil, err } - return am, account, nil account.Routes[routeResource.ID] = routeResource account.Routes[routePeerGroupResource.ID] = routePeerGroupResource account.NameServerGroups[nameServerGroup.ID] = nameServerGroup account.Policies = append(account.Policies, policy) account.SetupKeys[setupKey.Id] = setupKey account.Users[user.Id] = user err = am.Store.SaveAccount(context.Background(), account) if err != nil { return nil, nil, err } acc, err := am.Store.GetAccount(context.Background(), account.Id) if err != nil { return nil, nil, err } return am, acc, nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/group_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/store/sql_store.go (1)
467-490: SaveUsers may not properly handle user-group associations.
SaveUserscallsStoreAutoGroups()but uses a simpleOnConflict{UpdateAll: true}without explicitly managing thegroup_usersjunction table. This differs fromSaveUser(lines 503-526), which uses a transaction to delete old group associations and insert new ones.If users have group changes,
SaveUsersmay not update thegroup_userstable correctly, leading to stale associations.Consider applying the same transaction pattern used in
SaveUser:Suggested fix to handle group associations in SaveUsers
func (s *SqlStore) SaveUsers(ctx context.Context, users []*types.User) error { if len(users) == 0 { return nil } usersCopy := make([]*types.User, len(users)) for i, user := range users { userCopy := user.Copy() userCopy.Email = user.Email userCopy.Name = user.Name userCopy.StoreAutoGroups() if err := userCopy.EncryptSensitiveData(s.fieldEncrypt); err != nil { return fmt.Errorf("encrypt user: %w", err) } usersCopy[i] = userCopy } - result := s.db.Clauses(clause.OnConflict{UpdateAll: true}).Create(&usersCopy) - if result.Error != nil { - log.WithContext(ctx).Errorf("failed to save users to store: %s", result.Error) - return status.Errorf(status.Internal, "failed to save users to store") - } - return nil + return s.transaction(func(tx *gorm.DB) error { + result := tx.Omit("Groups").Clauses(clause.OnConflict{UpdateAll: true}).Create(&usersCopy) + if result.Error != nil { + return status.Errorf(status.Internal, "failed to save users to store: %v", result.Error) + } + + // Update group associations for each user + for _, userCopy := range usersCopy { + // Delete old associations + if err := tx.Delete(&types.GroupUser{}, "user_id = ?", userCopy.Id).Error; err != nil { + return status.Errorf(status.Internal, "failed to delete user groups: %v", err) + } + // Save new associations + if len(userCopy.Groups) > 0 { + if err := tx.Save(userCopy.Groups).Error; err != nil { + return status.Errorf(status.Internal, "failed to save user groups: %v", err) + } + } + } + return nil + }) }
🤖 Fix all issues with AI agents
In @management/server/store/sql_store.go:
- Around line 1677-1706: The callback passed to pgx.CollectRows returns a
pointer to the local variable u even on scan error, which can yield a
partially-initialized struct being collected; change the error path in the
CollectRows callback (the anonymous function that calls row.Scan) to return nil,
err instead of &u and add a short comment noting that returning nil on error
avoids collecting a broken User; keep the successful return as &u, nil.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/account.gomanagement/server/store/sql_store.go
💤 Files with no reviewable changes (1)
- management/server/account.go
🔇 Additional comments (2)
management/server/store/sql_store.go (2)
2748-2781: LGTM: New user-group management functions.The
AddUserToGroupandRemoveUserFromGroupfunctions are well-implemented:
- Idempotent
AddUserToGroupwithOnConflict{DoNothing: true}- Proper error handling and logging
RemoveUserFromGroupgracefully handles missing associations with a warningThese functions provide clean APIs for managing user-group relationships in the new separate table.
267-267: No action needed — the pointer semantics change is correct.Line 267 correctly changed from
append(account.UsersG, &user)toappend(account.UsersG, user). SinceUsersGis defined as[]User(a slice of values, not pointers) anduseris a pointer dereferenced from map iteration, appendinguserdirectly is the correct approach. The previous code taking the address of an already-dereferenced pointer was semantically incorrect.
# Conflicts: # management/server/migration/migration.go # management/server/store/store.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @management/server/migration/migration.go:
- Around line 639-640: The log message at the end of the cleanup routine
currently hardcodes "auto_groups" instead of using the function parameter;
update the Infof call that uses tableName to reference the columnName parameter
(e.g., replace the literal "auto_groups" with columnName) so the log reflects
the actual column being cleaned up and keeps the function generic.
🧹 Nitpick comments (3)
management/server/store/store.go (1)
92-93: Consider parameter consistency between AddUserToGroup and RemoveUserFromGroup.
AddUserToGrouprequiresaccountIDwhileRemoveUserFromGroupdoes not. IfaccountIDis used for validation or scoping in the add operation, the same might be beneficial for the remove operation to ensure consistency and prevent cross-account modifications. IfaccountIDis not needed for removal, consider documenting why the asymmetry exists.management/server/migration/migration.go (2)
578-582: Error message is not generic enough.Line 581 hardcodes "group IDs" in the error message, but this is a generic function that can clean up any type of orphaned references. Consider using a generic message like "fetch valid IDs" or dynamically construct the message using the type name.
📝 Suggested fix
var validIDs []string if err := tx.Model(fkModel).Select("id").Pluck("id", &validIDs).Error; err != nil { - return fmt.Errorf("fetch valid group IDs: %w", err) + return fmt.Errorf("fetch valid IDs from foreign key table: %w", err) }
590-600: Consider adding validation for JSON array content type.The function assumes the JSON column contains an array of strings but doesn't validate this. If the column contains an array of objects, numbers, or mixed types, the unmarshal will fail with a potentially confusing error. Consider validating the array element type or providing a more descriptive error message.
💡 Suggested enhancement
var list []string if err := json.Unmarshal([]byte(jsonValue), &list); err != nil { - log.WithContext(ctx).Warnf("Failed to unmarshal %s for id %v: %v", columnName, row["id"], err) + log.WithContext(ctx).Warnf("Failed to unmarshal %s as string array for id %v: %v", columnName, row["id"], err) continue }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
management/server/account.gomanagement/server/account_test.gomanagement/server/group_test.gomanagement/server/migration/migration.gomanagement/server/store/sql_store_test.gomanagement/server/store/store.gomanagement/server/user_test.go
💤 Files with no reviewable changes (1)
- management/server/account.go
🚧 Files skipped from review as they are similar to previous changes (2)
- management/server/account_test.go
- management/server/user_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
management/server/group_test.go (3)
management/server/store/store.go (1)
Store(51-214)management/server/permissions/modules/module.go (4)
Routes(15-15)Policies(14-14)SetupKeys(17-17)Users(16-16)route/route.go (1)
ID(48-48)
management/server/store/sql_store_test.go (1)
management/server/types/group.go (1)
GroupUser(45-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
🔇 Additional comments (5)
management/server/group_test.go (1)
396-411: Test setup correctly aligned with new GroupUser persistence model.The updated test sequence (create groups → fetch account → populate resources → save) ensures that group references exist before establishing foreign key relationships. This pattern correctly reflects the new relational integrity requirements introduced by the GroupUser model migration.
management/server/store/sql_store_test.go (1)
1375-1375: LGTM: GroupUsers field properly initialized in test fixtures.The empty GroupUsers slice initialization correctly aligns with the new relational model for groups. This ensures test groups are created with the expected structure.
Also applies to: 1400-1400, 1409-1409
management/server/store/store.go (2)
358-360: LGTM: Orphaned ID cleanup properly positioned in migration sequence.The
CleanupOrphanedIDsmigration correctly runs before auto-migration to ensure data integrity. This prevents foreign key constraint violations when the subsequent JSON-to-table migration creates GroupUser records.
400-408: LGTM: JSON-to-table migration correctly placed and implemented.The
MigrateJsonToTablemigration properly converts User.auto_groups JSON arrays into GroupUser relational records. Running this in post-auto migrations ensures the GroupUser table exists, and the priorCleanupOrphanedIDsstep ensures only valid group references are migrated.management/server/migration/migration.go (1)
543-641: Overall implementation is solid and well-structured.The
CleanupOrphanedIDsfunction is well-designed with proper:
- Table and column existence checks
- Transactional integrity
- Efficient lookup using a map for valid IDs
- Conditional updates (only when orphaned IDs are found)
- Appropriate logging at different levels
The function will effectively support the migration from JSON-based auto_groups to the relational GroupUser model.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
management/server/migration/migration.go (3)
543-546: Improve documentation clarity for the generic function.The comment mentions "the lists field" but the function is generic and operates on any JSON array column specified by
columnName. Consider rephrasing to clarify that it cleans up a JSON array column rather than a specific field name.📝 Suggested documentation improvement
-// CleanupOrphanedIDs removes non-existent IDs from the JSON array column. -// T is the type of the model that contains the list. -// This migration cleans up the lists field by removing IDs that no longer exist in the target table. +// CleanupOrphanedIDs removes non-existent IDs from a JSON array column. +// T is the type of the source model that contains the JSON array column. +// S is the type of the foreign key model used for validating ID existence. +// This migration cleans up the specified column by removing IDs that no longer exist in the foreign key table. func CleanupOrphanedIDs[T, S any](ctx context.Context, db *gorm.DB, columnName string) error {
578-582: Use generic error message for reusable code.The error message hardcodes "group IDs", but this function is generic and can clean up any type of ID references. Use a generic message to improve reusability.
♻️ Proposed fix
// Get all valid IDs from the fk table var validIDs []string if err := tx.Model(fkModel).Select("id").Pluck("id", &validIDs).Error; err != nil { - return fmt.Errorf("fetch valid group IDs: %w", err) + return fmt.Errorf("fetch valid IDs from foreign key table: %w", err) }
606-612: Use generic variable naming for reusable code.The variable
groupIDimplies a specific domain (groups), but this function is generic and can clean up any type of ID references. Use a more generic name likeidoritemID.♻️ Proposed fix
// Filter out non-existent IDs cleanedList := make([]string, 0, len(list)) - for _, groupID := range list { - if validIDMap[groupID] { - cleanedList = append(cleanedList, groupID) + for _, id := range list { + if validIDMap[id] { + cleanedList = append(cleanedList, id) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/migration/migration.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: release
- GitHub Check: Windows
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @management/server/account_test.go:
- Around line 1021-1054: genGroups returns incomplete types.Group objects
missing required fields (ID, AccountID, Issued) which causes DB constraint
violations when saving accounts; update genGroups to accept the parent account
ID (or take an account param) and set each group's ID to the map key, AccountID
to the provided account ID, and Issued to types.GroupIssuedAPI (or another valid
issued constant) for every entry so the generated groups are fully valid for
persistence.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/account_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/account_test.go (1)
management/server/types/group.go (2)
Group(15-37)GroupUser(45-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: JS / Lint
- GitHub Check: Darwin
- GitHub Check: release_ui_darwin
🔇 Additional comments (2)
management/server/account_test.go (2)
921-921: LGTM! Benchmark environment configuration is appropriate.Setting the store engine explicitly ensures the benchmark runs against PostgreSQL, providing consistent and targeted performance measurements.
1774-1780: LGTM! Test correctly validates new relational membership model.The additions of
User.Groups(lines 1774-1780) andGroup.GroupUsers(lines 1800-1806) properly test thatAccount.Copy()preserves the new relational group membership structure introduced by this migration.Also applies to: 1800-1806
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes / Data Integrity
Behavior Change
Tests
✏️ Tip: You can customize this high-level summary in your review settings.