feat: add migration to AddUserMember interactor [FLOW-BE-34] #74
Conversation
|
Warning Rate limit exceeded@akiyatomohiro has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 32 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughThis pull request adds new domain types, builders, and interfaces to support role and permission management. It introduces new ID types, helper functions, and list/set constructs for roles and permittables. Additionally, it provides in-memory and MongoDB-backed implementations for managing these entities. The workspace business logic has been enhanced with functions to ensure that users receive the appropriate maintainer role during member addition. Temporary comments indicate that many of these changes are slated for removal once the permission check migration is complete. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant W as Workspace
participant RR as RoleRepo
participant PR as PermittableRepo
U->>W: AddUserMember(userID)
W->>RR: getMaintainerRole(ctx)
RR-->>W: return maintainerRole
W->>PR: ensureUserHasMaintainerRole(ctx, userID, maintainerRole)
alt User already has role
PR-->>W: Role exists
else
PR-->>W: Created new Permittable with role
end
W-->>U: Member added with maintainer role
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (19)
account/accountusecase/accountinteractor/workspace.go (4)
148-152: Temporary TODO comment.These lines, which retrieve the maintainer role during user addition, are flagged for removal once the permission check migration is finalized. Track these changes so they don’t remain in production code beyond the planned timeframe.
159-162: Ensure consistent naming in TODO.This supplemental step, which ensures a user has the maintainer role, references a second method (
ensureUserHasMaintainerRole). The name is clear and consistent with the precedinggetMaintainerRole, but remember to remove or rename them after the migration.
415-452: Proactively handle concurrency for role creation.The
getMaintainerRolefunction checks existing roles before creating a new one, which is correct. However, in a high-concurrency environment, it might lead to duplicate creation if multiple transactions run in parallel. Consider adding repository-level checks or unique constraints to avoid duplicates.
489-500: Use a more descriptive name forhasRole.“
hasRole” is a clear name, but to align with typical naming in domain-driven design, you might prefer something likepermitsRoleorgrantsRole. This is a minor suggestion, but it can help unify naming across the domain.account/accountdomain/permittable/permittable.go (1)
38-43: Add validation to EditRoleIDs method.Consider adding validation to ensure roleIDs are valid before assignment:
- Check for nil roleIDs
- Validate individual role IDs
- Consider returning an error instead of silently accepting invalid input
-func (p *Permittable) EditRoleIDs(roleIDs accountdomain.RoleIDList) { +func (p *Permittable) EditRoleIDs(roleIDs accountdomain.RoleIDList) error { if p == nil { - return + return errors.New("permittable is nil") } + if roleIDs == nil { + return errors.New("roleIDs cannot be nil") + } + for _, rid := range roleIDs { + if rid.IsNil() { + return errors.New("invalid role ID in list") + } + } p.roleIDs = roleIDs + return nil }account/accountinfrastructure/accountmongo/mongodoc/role.go (1)
22-28: Add validation to NewRole function.Consider validating the input Role before creating a document:
func NewRole(g role.Role) (*RoleDocument, string) { + if g.ID().IsNil() { + return nil, "" + } + if g.Name() == "" { + return nil, "" + } id := g.ID().String() return &RoleDocument{ ID: id, Name: g.Name(), }, id }account/accountdomain/permittable/builder.go (3)
1-1: Consider adding more context to the TODO comment.The TODO comment should include more context about why this file needs to be deleted and what will replace it after the migration.
-// TODO: Delete this file once the permission check migration is complete. +// TODO: Delete this file once the permission check migration is complete. +// This is a temporary implementation that will be replaced by the new permission system. +// Related issue: <add issue link>
18-26: Consider improving error handling in Build().The current implementation returns the same
ErrInvalidIDfor bothidanduserIDvalidation failures. This makes it harder to debug which field caused the validation to fail.func (b *Builder) Build() (*Permittable, error) { if b.p.id.IsNil() { - return nil, ErrInvalidID + return nil, fmt.Errorf("invalid permittable id: %w", ErrInvalidID) } if b.p.userID.IsNil() { - return nil, ErrInvalidID + return nil, fmt.Errorf("invalid user id: %w", ErrInvalidID) } return b.p, nil }
28-34: Consider improving panic message in MustBuild().The current implementation panics with the raw error, which might not provide enough context about where the panic occurred.
func (b *Builder) MustBuild() *Permittable { u, err := b.Build() if err != nil { - panic(err) + panic(fmt.Sprintf("failed to build permittable: %v", err)) } return u }account/accountinfrastructure/accountmongo/mongodoc/permittable.go (2)
27-30: Consider pre-allocating roleIds slice with exact capacity.The current implementation creates a new slice and appends to it. For better performance, consider allocating the slice with the exact size needed.
- roleIds := make([]string, 0, len(p.RoleIDs())) - for _, r := range p.RoleIDs() { - roleIds = append(roleIds, r.String()) + roleIds := make([]string, len(p.RoleIDs())) + for i, r := range p.RoleIDs() { + roleIds[i] = r.String()
39-64: Consider consolidating error handling in Model().The current implementation has repetitive error handling patterns. Consider using a helper function to reduce duplication.
+func handleIDConversion[T any](convert func(string) (T, error), id string) (T, error) { + result, err := convert(id) + if err != nil { + return result, fmt.Errorf("failed to convert ID %s: %w", id, err) + } + return result, nil +} func (d *PermittableDocument) Model() (*permittable.Permittable, error) { if d == nil { return nil, nil } - uid, err := accountdomain.PermittableIDFrom(d.ID) + uid, err := handleIDConversion(accountdomain.PermittableIDFrom, d.ID) if err != nil { return nil, err } - userId, err := user.IDFrom(d.UserID) + userId, err := handleIDConversion(user.IDFrom, d.UserID) if err != nil { return nil, err }account/accountinfrastructure/accountmemory/role.go (1)
34-43: Consider optimizing slice allocations in Find methods.Both
FindAllandFindByIDscould benefit from more efficient slice handling.func (r *Role) FindAll(ctx context.Context) (role.List, error) { r.lock.Lock() defer r.lock.Unlock() - res := make(role.List, 0, len(r.data)) + res := make(role.List, len(r.data)) + i := 0 for _, v := range r.data { - res = append(res, v) + res[i] = v + i++ } return res, nil }Also applies to: 56-67
account/accountdomain/id.go (1)
39-39: Enhance the TODO comment with migration details.The TODO comment should provide more context about the migration plan and what will replace these types.
-// TODO: Delete the below once the permission check migration is complete. +// TODO: Delete the below once the permission check migration is complete. +// These types are temporary and will be replaced by the new permission system. +// Migration tracking issue: <add issue link> +// Target completion date: <add date>account/accountinfrastructure/accountmemory/permittable.go (2)
16-19: Consider adding indices for better lookup performance.The current implementation uses a single map indexed by
PermittableID, requiring O(n) scans for user ID lookups. Consider adding a secondary index map forUserIDto improve lookup performance.type Permittable struct { lock sync.Mutex data map[accountdomain.PermittableID]*permittable.Permittable + // Secondary index for faster user ID lookups + userIndex map[user.ID]*permittable.Permittable }
48-67: Optimize FindByUserIDs implementation.The current implementation has nested loops which could be inefficient for large datasets. Consider using a map to track found items and break early when all requested IDs are found.
func (p *Permittable) FindByUserIDs(ctx context.Context, userIDs user.IDList) (permittable.List, error) { p.lock.Lock() defer p.lock.Unlock() + // Use map for tracking found IDs + found := make(map[user.ID]bool) results := make(permittable.List, 0, len(userIDs)) - for _, userID := range userIDs { - for _, perm := range p.data { - if perm.UserID() == userID { - results = append(results, perm) - break - } + + for _, perm := range p.data { + if userIDs.Has(perm.UserID()) { + results = append(results, perm) + found[perm.UserID()] = true + // Break early if all IDs are found + if len(found) == len(userIDs) { + break + } } }account/accountinfrastructure/accountmongo/role.go (2)
16-19: Consider adding TTL index for temporary migration data.Since this is temporary migration code (as indicated by the TODO), consider adding a TTL index to automatically clean up old data.
var ( roleIndexes = []string{} - roleUniqueIndexes = []string{"id", "name"} + roleUniqueIndexes = []string{"id", "name"} + roleTTLIndexes = []string{"created_at"} )
31-33: Add TTL index creation to Init method.Update the Init method to create TTL indexes for automatic cleanup of temporary migration data.
func (r *Role) Init(ctx context.Context) error { - return createIndexes(ctx, r.client, roleIndexes, roleUniqueIndexes) + if err := createIndexes(ctx, r.client, roleIndexes, roleUniqueIndexes); err != nil { + return err + } + return r.client.CreateIndex(ctx, mongox.Index{ + Key: bson.D{{Key: "created_at", Value: 1}}, + Options: options.Index().SetExpireAfterSeconds(7 * 24 * 60 * 60), // 7 days + }) }account/accountinfrastructure/accountmongo/permittable.go (2)
18-21: Add index for roleids field to optimize FindByRoleID queries.The FindByRoleID method performs queries on the roleids field, but there's no index for it.
var ( - newPermittableIndexes = []string{} + newPermittableIndexes = []string{"roleids"} newPermittableUniqueIndexes = []string{"id", "userid"} )
49-53: Optimize FindByRoleID query for better performance.The current query might not use the index effectively. Consider using $elemMatch for better performance with array fields.
func (r *Permittable) FindByRoleID(ctx context.Context, roleId accountdomain.RoleID) (permittable.List, error) { return r.find(ctx, bson.M{ - "roleids": bson.M{"$in": []string{roleId.String()}}, + "roleids": bson.M{"$elemMatch": bson.M{"$eq": roleId.String()}}, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
account/accountdomain/id.go(1 hunks)account/accountdomain/permittable/builder.go(1 hunks)account/accountdomain/permittable/id.go(1 hunks)account/accountdomain/permittable/list.go(1 hunks)account/accountdomain/permittable/permittable.go(1 hunks)account/accountdomain/role/builder.go(1 hunks)account/accountdomain/role/id.go(1 hunks)account/accountdomain/role/list.go(1 hunks)account/accountdomain/role/role.go(1 hunks)account/accountinfrastructure/accountmemory/container.go(1 hunks)account/accountinfrastructure/accountmemory/permittable.go(1 hunks)account/accountinfrastructure/accountmemory/role.go(1 hunks)account/accountinfrastructure/accountmongo/mongodoc/permittable.go(1 hunks)account/accountinfrastructure/accountmongo/mongodoc/role.go(1 hunks)account/accountinfrastructure/accountmongo/permittable.go(1 hunks)account/accountinfrastructure/accountmongo/role.go(1 hunks)account/accountusecase/accountinteractor/workspace.go(3 hunks)account/accountusecase/accountinteractor/workspace_test.go(10 hunks)account/accountusecase/accountrepo/container.go(1 hunks)account/accountusecase/accountrepo/permittable.go(1 hunks)account/accountusecase/accountrepo/role.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- account/accountdomain/role/list.go
- account/accountdomain/permittable/list.go
🔇 Additional comments (19)
account/accountusecase/accountinteractor/workspace.go (2)
5-5: Use caution with new imports.The newly introduced imports (
fmt,permittable,role,log) make sense for the role-handling logic and logging. Ensure that this expanded dependency set does not introduce conflicts or overshadow existing naming.Also applies to: 8-10, 16-16
454-487: Ensure robust error propagation inensureUserHasMaintainerRole.
- If the
FindByUserIDcall returns a transient error, you currently exit with that error. Ensure caller code can handle or retry.- If partial creation fails (e.g.,
Permittable.Savefails), the function stops but leaves the code in an intermediate state. Consider rolling back or verifying that partial states are acceptable.account/accountusecase/accountinteractor/workspace_test.go (3)
9-10: Imports for new role and permittable logic.The test now includes
permittableandrolereferences, along withaccountrepo. This is consistent with your domain changes. Just confirm that these tests won’t conflict with more advanced mocking frameworks or stubs if introduced.Also applies to: 16-17
957-960: Minor expansions in existing tests.The expansions in these lines appear to extend coverage for workspace membership, ensuring correct handling of additional roles. Keep test scenarios minimal and focus on the essential logic to avoid test bloat.
Also applies to: 967-967, 974-974, 1007-1007, 1060-1061, 1069-1071, 1127-1128
1323-1554: Comprehensive test coverage for migration logic.
TestWorkspace_AddMember_Migrationcovers scenario-based user role assignment, including edge cases like existing roles, multiple roles, or duplicate creations. This thorough approach is helpful for verifying domain correctness.assertPermittablesAndRolesis well-written, verifying both the presence of a “maintainer” role and the integrity of associated user IDs.- Consider including concurrency or cross-transaction tests if the domain expects parallel usage.
Overall, these tests robustly confirm the migration logic.
account/accountdomain/role/id.go (1)
1-19: Temporary file note & type alias usage.
- The comment indicates this file is temporary for the permission check migration. Revisit and remove or merge it once the migration is complete to minimize fragmentation.
- The type alias and exposed variables (e.g.,
ErrInvalidID,NewID, etc.) mirror the underlying domain definitions, which is a neat approach.- Keep an eye on future changes to
accountdomain.RoleIDto ensure these aliases stay in sync.No issues found; changes look straightforward.
account/accountdomain/permittable/id.go (2)
1-2: Ensure migration completion criteria is documented.The TODO comment indicates this file is temporary, but the criteria for completing the permission check migration and safely removing this file should be documented.
Consider:
- What conditions must be met to consider the migration complete?
- Are there any dependencies that need to be updated before removal?
- What tests should pass to verify safe removal?
7-17: LGTM! Clean type alias and variable declarations.The code provides a well-structured wrapper around
accountdomaintypes and functions, making them easily accessible within thepermittablepackage context.account/accountinfrastructure/accountmemory/container.go (1)
12-13: LGTM! Clean container initialization with new fields.The
RoleandPermittablefields are properly initialized using their respective factory functions. The TODO comments clearly indicate these are temporary additions for the migration.account/accountdomain/role/role.go (1)
7-14: LGTM! Clean error handling and struct definition.The code properly defines the error for empty names and encapsulates the role properties.
account/accountusecase/accountrepo/role.go (1)
12-18: LGTM! Well-defined repository interface.The
Roleinterface provides a comprehensive set of methods for role management with proper context handling and error returns.account/accountusecase/accountrepo/permittable.go (2)
13-18: Well-designed interface for permission management.The interface follows Go best practices with:
- Context support for cancellation
- Clear method signatures
- Appropriate error handling
- Consistent return types
1-1:❓ Verification inconclusive
Verify the migration timeline.
Please ensure there's a tracking issue or project milestone for the permission check migration to avoid this temporary code becoming permanent.
🏁 Script executed:
#!/bin/bash # Search for related migration issues or TODOs rg -i "permission.*check.*migration|TODO.*permission"Length of output: 3604
Action: Confirm and Track Permission Check Migration
The// TODO: Delete this file once the permission check migration is complete.comment inaccount/accountusecase/accountrepo/permittable.gois consistent with similar annotations throughout the repository. Please ensure that a dedicated tracking issue or project milestone is established for the permission check migration. This will help guarantee that the temporary code is removed once the migration is complete and prevent it from inadvertently becoming permanent.account/accountdomain/role/builder.go (1)
13-21: Good validation in Build method.The validation logic properly checks for:
- Non-nil ID
- Non-empty name
- Returns appropriate errors
account/accountdomain/permittable/permittable.go (1)
17-36: Well-implemented defensive programming with nil checks.The methods consistently handle nil receivers and return appropriate zero values, preventing nil pointer dereferences.
account/accountinfrastructure/accountmongo/mongodoc/role.go (1)
30-44: Well-implemented Model conversion with proper error handling.The Model method:
- Handles nil receiver
- Validates ID conversion
- Uses builder pattern effectively
- Returns appropriate errors
account/accountdomain/permittable/builder.go (1)
51-54: Consider validating roleIDs input.The
RoleIDsmethod accepts a slice without any validation. Consider adding checks for nil or empty roleIDs.func (b *Builder) RoleIDs(roleIDs []accountdomain.RoleID) *Builder { + if roleIDs == nil { + roleIDs = make([]accountdomain.RoleID, 0) + } b.p.roleIDs = roleIDs return b }account/accountinfrastructure/accountmemory/role.go (1)
69-75: Consider adding validation in Save method.The
Savemethod doesn't validate the role before saving. Consider adding checks for nil or invalid role ID.func (r *Role) Save(ctx context.Context, rl role.Role) error { r.lock.Lock() defer r.lock.Unlock() + if rl.ID().IsNil() { + return fmt.Errorf("cannot save role with nil ID") + } + r.data[rl.ID()] = &rl return nil }account/accountdomain/id.go (1)
41-46: LGTM! Well-structured type definitions.The type definitions and their Type() methods follow the established pattern in the file, maintaining consistency with existing types.
akiyatomohiro
changed the title
ref:
https://eukarya-inc.slack.com/archives/C064TUMGNDA/p1739512520086599?thread_ts=1739394205.301159&cid=C064TUMGNDA
Summary by CodeRabbit
New Features
PermittableandRoleto standardize permission and role management.Tests