feat(server): remove multiple user-members from workspace #1281
Conversation
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…-server/remove-multiple-user-members
WalkthroughThe pull request updates the Changes
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (4)
server/schemas/workspace.graphql (2)
67-70: Add type description for better documentation.Consider adding a description to document the purpose and usage of this input type.
+""" +Input type for removing multiple users from a workspace simultaneously. +""" input RemoveMultipleUsersFromWorkspaceInput { workspaceId: ID! userIds: [ID!]! }
67-70: Consider implementing safeguards for bulk operations.The new mutation enables removing multiple users at once, which could impact performance with large user lists. Consider:
- Implementing a maximum limit on the number of users that can be removed in a single operation
- Adding rate limiting to prevent abuse
- Documenting these limitations in the schema
Also applies to: 111-113, 130-130
server/internal/adapter/gql/resolver_workspace.go (2)
129-132: Consider adding input validation for empty user list.While the implementation is correct, it would be beneficial to add validation for empty userIds list to fail fast and provide a clear error message.
userIds, err := util.TryMap(input.UserIds, gqlmodel.ToID[accountdomain.User]) if err != nil { return nil, err } +if len(userIds) == 0 { + return nil, errors.New("at least one user ID must be provided") +}
122-123: Add documentation explaining bulk operation benefits.Consider adding a comment explaining that this mutation optimizes performance by handling multiple user removals in a single operation, reducing the number of database transactions.
-// RemoveMultipleUsersFromWorkspace is the resolver for the removeMultipleUsersFromWorkspace field. +// RemoveMultipleUsersFromWorkspace is the resolver for the removeMultipleUsersFromWorkspace field. +// It optimizes the removal of multiple users from a workspace by handling them in a single operation, +// reducing the number of database transactions compared to removing users individually.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- server/go.mod (1 hunks)
- server/internal/adapter/gql/generated.go (17 hunks)
- server/internal/adapter/gql/gqlmodel/models_gen.go (1 hunks)
- server/internal/adapter/gql/resolver_workspace.go (2 hunks)
- server/schemas/workspace.graphql (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/go.mod
🧰 Additional context used
🔇 Additional comments (10)
server/schemas/workspace.graphql (1)
130-130: Add mutation description and verify permission handling.
- Add a description to document the mutation's purpose and requirements.
- Ensure the resolver implementation validates that the requesting user has sufficient permissions to remove multiple users.
+ """ + Remove multiple users from a workspace. Requires OWNER or MAINTAINER role. + """ removeMultipleUsersFromWorkspace(input: RemoveMultipleUsersFromWorkspaceInput!): RemoveMultipleMembersFromWorkspacePayloadserver/internal/adapter/gql/resolver_workspace.go (2)
16-16: LGTM: Import addition is appropriate.The util package import is correctly placed and necessary for the new TryMap functionality.
122-140: LGTM: Well-structured implementation of bulk user removal.The implementation follows the established patterns in the codebase and properly handles error cases.
server/internal/adapter/gql/gqlmodel/models_gen.go (3)
823-825: LGTM! Well-structured payload type.The payload type follows GraphQL conventions and maintains consistency with similar workspace member management operations.
827-830: LGTM! Well-structured input type.The input type is well-designed for bulk operations:
- Appropriate field types for workspace and user IDs
- Follows GraphQL naming conventions
- Maintains consistency with existing single-user removal input type
823-830: Verify GraphQL schema implementation.The new types look good and align with the PR objectives. Let's verify the corresponding mutation implementation.
✅ Verification successful
Let me try a different search strategy to locate the GraphQL schema files and related implementations.
Let me check the workspace schema file specifically since we found it in the previous search.
Based on the schema inspection, I can now provide a final response.
GraphQL schema implementation is complete and correct
The workspace schema file shows that the new mutation
removeMultipleUsersFromWorkspaceis properly defined with:
- Correct input type
RemoveMultipleUsersFromWorkspaceInput- Correct payload type
RemoveMultipleMembersFromWorkspacePayload- Proper integration in the Mutation type extension
The types in the generated Go code (
models_gen.go) correctly match the GraphQL schema definition, confirming proper schema-to-code generation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the mutation definition and any related workspace member removal types # Expected: Find the mutation definition that uses these new types # Search for related GraphQL type definitions and mutations rg -t go "type.*RemoveMultiple.*Workspace" --glob "*.graphqls" rg -t go "mutation.*removeMultipleUsersFromWorkspace" --glob "*.graphqls"Length of output: 438
Script:
#!/bin/bash # First, let's find all GraphQL schema files fd -e graphql -e graphqls # Then search for workspace-related mutations in the codebase rg -g '!node_modules' -g '!vendor' "mutation.*workspace" # Also search for the resolver implementation ast-grep --pattern 'func (*_) RemoveMultipleUsersFromWorkspace($$$) { $$$ }'Length of output: 5250
Script:
#!/bin/bash # Check the workspace schema file for the new mutation cat server/schemas/workspace.graphql # Also search for the resolver implementation with a different pattern ast-grep --pattern 'type mutationResolver struct { $$$ }' # And check for any implementation files related to workspace mutations rg -g '*.go' "func.*Workspace.*Resolver" -A 2Length of output: 3796
server/internal/adapter/gql/generated.go (4)
22252-22307: Ensure proper error handling in the resolver middleware.In the
_Mutation_removeMultipleUsersFromWorkspacefunction, verify that all potential errors are appropriately handled and that the context is correctly managed throughout the resolver middleware.
450-450: 🛠️ Refactor suggestionConsider renaming the mutation for consistency.
The function
RemoveMultipleUsersFromWorkspaceis added to theMutationstruct. To maintain naming consistency with other functions handling workspace members, consider renaming it toRemoveMultipleMembersFromWorkspace.Apply this diff to rename the function:
- RemoveMultipleUsersFromWorkspace func(childComplexity int, input gqlmodel.RemoveMultipleUsersFromWorkspaceInput) int + RemoveMultipleMembersFromWorkspace func(childComplexity int, input gqlmodel.RemoveMultipleUsersFromWorkspaceInput) intLikely invalid or redundant comment.
8029-8049: 🛠️ Refactor suggestionSimplify argument parsing in the resolver function.
The function
field_Mutation_removeMultipleUsersFromWorkspace_argsInputcontains redundant null checks and error handling that can be streamlined to improve readability.Apply this diff to simplify the function:
func (ec *executionContext) field_Mutation_removeMultipleUsersFromWorkspace_argsInput( ctx context.Context, rawArgs map[string]interface{}, ) (gqlmodel.RemoveMultipleUsersFromWorkspaceInput, error) { - _, ok := rawArgs["input"] - if !ok { - var zeroVal gqlmodel.RemoveMultipleUsersFromWorkspaceInput - return zeroVal, nil - } - ctx = graphql.WithPathContext(ctx, graphql.NewPathWithField("input")) - if tmp, ok := rawArgs["input"]; ok { - return ec.unmarshalNRemoveMultipleUsersFromWorkspaceInput2githubᚗcomᚋreearthᚋreearthᚑcmsᚋserverᚋinternalᚋadapterᚋgqlᚋgqlmodelᚐRemoveMultipleUsersFromWorkspaceInput(ctx, tmp) - } - - var zeroVal gqlmodel.RemoveMultipleUsersFromWorkspaceInput - return zeroVal, nil + tmp := rawArgs["input"] + return ec.unmarshalNRemoveMultipleUsersFromWorkspaceInput2githubᚗcomᚋreearthᚋreearthᚑcmsᚋserverᚋinternalᚋadapterᚋgqlᚋgqlmodelᚐRemoveMultipleUsersFromWorkspaceInput(ctx, tmp) }Likely invalid or redundant comment.
2796-2807: Verify the complexity calculation for the new mutation.Ensure that the complexity function for
RemoveMultipleUsersFromWorkspaceaccurately reflects the computational cost of the operation, especially if removing multiple users could have a significant impact on performance.Run the following script to check for any potential performance issues:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
server/schemas/workspace.graphql (1)
124-125: Consider adding a limit to batch operationsWhile the mutation is well-structured, consider adding a maximum limit to the number of users that can be removed in a single operation to prevent potential performance issues or abuse.
You could add a directive or documentation comment specifying the maximum allowed users, for example:
""" Remove multiple users from a workspace. Maximum 50 users per operation. """ removeMultipleMembersFromWorkspace(input: RemoveMultipleMembersFromWorkspaceInput!): RemoveMultipleMembersFromWorkspacePayloadserver/internal/adapter/gql/resolver_workspace.go (1)
107-125: Recommend adding unit tests for the new mutationTo ensure that
RemoveMultipleMembersFromWorkspacefunctions correctly and handles all edge cases, consider adding unit tests covering scenarios such as:
- Removing multiple users successfully.
- Attempting to remove users who are not members of the workspace.
- Handling invalid or malformed user IDs.
- Passing an empty list of user IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- server/internal/adapter/gql/generated.go (24 hunks)
- server/internal/adapter/gql/gqlmodel/models_gen.go (1 hunks)
- server/internal/adapter/gql/resolver_workspace.go (3 hunks)
- server/schemas/workspace.graphql (3 hunks)
🧰 Additional context used
🔇 Additional comments (16)
server/internal/adapter/gql/generated.go (5)
6691-6693: LGTM: Input type definition is well-structured.The input type correctly defines the required fields:
workspaceId: ID! (non-null workspace identifier)userIds: [ID!]! (non-null array of non-null user identifiers)
Line range hint
22143-22168: LGTM: Resolver implementation follows best practices.The resolver implementation:
- Properly handles context propagation
- Uses middleware for consistent error handling
- Returns appropriate null values on errors
6753-6754: Verify mutation ordering in schema.The new mutation is correctly placed near other workspace-related mutations, maintaining good schema organization.
415-472: Verify complexity calculation for large member lists.The complexity calculation for the mutation should account for the number of members being removed to prevent DoS attacks.
#!/bin/bash # Search for complexity calculation logic rg "func.*complexity.*RemoveMultipleMembersFromWorkspace"
37926-37953: Verify error handling for empty userIds array.The input validation looks good, but we should verify that the resolver handles the case where
userIdsis an empty array, as the schema allows it.server/schemas/workspace.graphql (3)
62-64: LGTM: Input type is well-structuredThe input type is properly defined with non-nullable array of IDs, which ensures type safety and clear contract.
102-104: Improve type naming consistency across the schemaThe renaming of
RemoveMemberFromWorkspacePayloadtoRemoveIntegrationFromWorkspacePayloadimproves clarity by being more specific about the entity being removed.
106-108: Consider aligning type naming with input typeThe payload type uses "Members" while the input type uses "Users". This inconsistency was previously flagged and should be addressed.
server/internal/adapter/gql/gqlmodel/models_gen.go (5)
819-821: LGTM! Improved type naming for better clarity.The rename from
RemoveMemberFromWorkspacePayloadtoRemoveIntegrationFromWorkspacePayloadbetter reflects its specific purpose of handling integration removal operations.
823-826: LGTM! Well-structured input type for bulk operations.The new input type is well-designed with clear field names and appropriate types for handling multiple user removals in a single operation.
828-831: LGTM! Consistent payload structure.The payload type follows the established pattern in the codebase and appropriately returns the updated workspace state after the bulk removal operation.
832-834: LGTM! Clean code reorganization.The type was moved without any structural changes, improving code organization.
819-834: Verify the impact of type changes across the codebase.Since this is a generated file with significant type changes, we should verify:
- All references to
RemoveMemberFromWorkspacePayloadhave been updated to useRemoveIntegrationFromWorkspacePayload- No lingering references to the removed
RemoveUserFromWorkspaceInputtype exist✅ Verification successful
Let me try a different approach to verify these changes by searching for both old and new type names, and also check the GraphQL schema files which might contain these definitions.
This should help us understand:
- Where the new types are being used
- If there are any GraphQL schema definitions that need to be checked
- Any resolver implementations that might be related to these operations
Based on the search results, I can now provide a conclusive response about the type changes:
Type changes are correctly implemented across the codebase
The search results show that:
- There are no lingering references to the old types (
RemoveMemberFromWorkspacePayloadandRemoveUserFromWorkspaceInput)- The new types (
RemoveIntegrationFromWorkspacePayloadandRemoveMultipleMembersFromWorkspaceInput) are properly defined and used in the codebase- The GraphQL schema files confirm these are valid type definitions with corresponding resolver implementations
The changes appear to be part of a broader refactoring where:
- Single user removal was replaced with batch member removal functionality (
RemoveMultipleMembersFromWorkspaceInput)- Integration-specific payload type was introduced (
RemoveIntegrationFromWorkspacePayload)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to old types echo "Checking for old payload type references..." rg "RemoveMemberFromWorkspacePayload" --type go echo "Checking for references to removed input type..." rg "RemoveUserFromWorkspaceInput" --type goLength of output: 295
Script:
#!/bin/bash # Search for both old and new types in Go files and GraphQL schema files echo "Searching for RemoveIntegrationFromWorkspacePayload..." rg "RemoveIntegrationFromWorkspacePayload" -A 2 --type go echo "Searching for RemoveMultipleMembersFromWorkspaceInput..." rg "RemoveMultipleMembersFromWorkspaceInput" -A 2 --type go echo "Checking GraphQL schema files..." fd -e graphql -e gql -x cat {} echo "Checking resolver implementations..." ast-grep --pattern 'func (*_) Remove$$$Workspace'Length of output: 46071
server/internal/adapter/gql/resolver_workspace.go (3)
16-16: Approved: Necessary import addedThe import statement for
github.com/reearth/reearthx/utilis required for theutil.TryMapfunction used below.
Line range hint
128-139: Approved: Updated return type forRemoveIntegrationFromWorkspaceThe return type has been correctly updated to
*gqlmodel.RemoveIntegrationFromWorkspacePayload, aligning with the new payload structure.
Line range hint
128-139: Verify all references toRemoveIntegrationFromWorkspacehandle the new return typeSince the return type has changed, ensure that any code invoking this method is updated to handle
*gqlmodel.RemoveIntegrationFromWorkspacePayloadto prevent potential runtime errors.Run the following script to find all usages of
RemoveIntegrationFromWorkspace:✅ Verification successful
Let me analyze the findings. The
rgsearch shows all references toRemoveIntegrationFromWorkspacein the codebase. Let's verify the test file to see how it's being used and ensure it's handling the return type correctly.
After analyzing the test file, I can now generate the final response since I have all the necessary information. The test file shows that the GraphQL mutation is being used correctly with the expected return type.
The return type change is properly handled across the codebase
The verification shows that:
- The implementation in
resolver_workspace.gocorrectly returns*gqlmodel.RemoveIntegrationFromWorkspacePayload- The interface in
generated.gocorrectly declares the return type- The test in
gql_workspace_test.goproperly handles the return type by expecting and asserting on the workspace field in the payload🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of RemoveIntegrationFromWorkspace in the codebase. # Expected result: All usages should be compatible with the new return type. rg -A 3 -B 3 'RemoveIntegrationFromWorkspace\(' --type goLength of output: 3921
Script:
#!/bin/bash # Check the test implementation to verify how it handles the return type cat server/e2e/gql_workspace_test.goLength of output: 15858
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/e2e/gql_workspace_test.go (1)
274-275: Consider using sub-tests and improving assertionsThe test has been updated to handle multiple users, but could benefit from some improvements:
- The test name
TestRemoveUserFromWorkspaceshould be updated toTestRemoveMultipleMembersFromWorkspaceto reflect its new functionality.- Consider using sub-tests for better organization and clarity:
- Success case: removing multiple existing users
- Error case: removing non-existent users
- Add assertions to verify the state for both users after removal
Here's a suggested improvement:
-func TestRemoveUserFromWorkspace(t *testing.T) { +func TestRemoveMultipleMembersFromWorkspace(t *testing.T) { e, _, ar := StartServerWithRepos(t, &app.Config{}, true, baseSeederWorkspace) - w, err := ar.Workspace.FindByID(context.Background(), wId2) - assert.Nil(t, err) - assert.True(t, w.Members().HasUser(uId3)) - - uIds := accountdomain.UserIDList{uId2, uId3} - query := fmt.Sprintf(`mutation { removeMultipleMembersFromWorkspace(input: {workspaceId: "%s", userIds: ["%s"]}){ workspace{ id } }}`, wId2, strings.Join(uIds.Strings(), "\", \"")) - request := GraphQLRequest{ - Query: query, - } - jsonData, err := json.Marshal(request) - if err != nil { - assert.Nil(t, err) - } + t.Run("success_removing_multiple_users", func(t *testing.T) { + // Verify initial state + w, err := ar.Workspace.FindByID(context.Background(), wId2) + assert.Nil(t, err) + assert.True(t, w.Members().HasUser(uId2)) + assert.True(t, w.Members().HasUser(uId3)) + + // Remove multiple users + uIds := accountdomain.UserIDList{uId2, uId3} + query := fmt.Sprintf(`mutation { removeMultipleMembersFromWorkspace(input: {workspaceId: "%s", userIds: ["%s"]}){ workspace{ id } }}`, wId2, strings.Join(uIds.Strings(), "\", \"")) + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + assert.Nil(t, err) + + // Execute request + e.POST("/api/graphql"). + WithHeader("authorization", "Bearer test"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect(). + Status(http.StatusOK) + + // Verify final state + w, err = ar.Workspace.FindByID(context.Background(), wId2) + assert.Nil(t, err) + assert.False(t, w.Members().HasUser(uId2)) + assert.False(t, w.Members().HasUser(uId3)) + }) + + t.Run("error_removing_nonexistent_users", func(t *testing.T) { + uIds := accountdomain.UserIDList{uId2, uId3} + query := fmt.Sprintf(`mutation { removeMultipleMembersFromWorkspace(input: {workspaceId: "%s", userIds: ["%s"]}){ workspace{ id } }}`, wId2, strings.Join(uIds.Strings(), "\", \"")) + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + assert.Nil(t, err) + + o := e.POST("/api/graphql"). + WithHeader("authorization", "Bearer test"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect(). + Status(http.StatusOK). + JSON(). + Object() + + o.Value("errors").Array().Value(0).Object().Value("message"). + IsEqual("specified users do not exist in the workspace") + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/e2e/gql_workspace_test.go (2 hunks)
🔇 Additional comments (1)
server/e2e/gql_workspace_test.go (1)
8-8: LGTM!The
stringspackage import is correctly placed and necessary for thestrings.Join()usage in the test.
Overview
This PR introduces a new mutation removeMultipleUsersFromWorkspace in the workspace schema. The mutation allows for the removal of multiple user-members from a workspace in a single operation.
Summary by CodeRabbit
New Features
Bug Fixes
These updates streamline user management processes and improve the overall functionality of the workspace management system.