feat(graphql): optimize userBatchFn to handle system users without API calls and add tests for system user scenarios (#114)

Author: m-mizutani

pkg/controller/graphql/dataloader.go

@@ -114,15 +114,37 @@ func userBatchFn(slackClient interfaces.SlackClient) func(ctx context.Context, k
 	return func(ctx context.Context, keys []string) []*dataloader.Result[*graphql1.User] {
 		results := make([]*dataloader.Result[*graphql1.User], len(keys))
 
+		// Separate system users from regular Slack users
+		var slackUserIDs []string
+		slackUserIndices := make(map[int]string) // map[index]userID for Slack users
+
+		for i, key := range keys {
+			if key == string(types.SystemUserID) {
+				// Return system user immediately without calling Slack API
+				results[i] = &dataloader.Result[*graphql1.User]{
+					Data:  &graphql1.User{ID: key, Name: "System"},
+					Error: nil,
+				}
+			} else {
+				slackUserIDs = append(slackUserIDs, key)
+				slackUserIndices[i] = key
+			}
+		}
+
+		// If no Slack users to fetch, return early
+		if len(slackUserIDs) == 0 {
+			return results
+		}
+
 		if slackClient != nil {
 			// Use batch API to fetch all users at once
-			slackUsers, err := slackClient.GetUsersInfo(keys...)
+			slackUsers, err := slackClient.GetUsersInfo(slackUserIDs...)
 			if err != nil {
 				// Handle the error for debugging
-				errutil.Handle(ctx, goerr.Wrap(err, "failed to get users info from Slack", goerr.V("userIDs", keys)))
+				errutil.Handle(ctx, goerr.Wrap(err, "failed to get users info from Slack", goerr.V("userIDs", slackUserIDs)))
 				// If Slack API fails with user_not_found or similar errors, fallback to ID instead of propagating error
 				// This prevents the entire query from failing when some users don't exist in Slack
-				for i, id := range keys {
+				for i, id := range slackUserIndices {
 					results[i] = &dataloader.Result[*graphql1.User]{
 						Data:  &graphql1.User{ID: id, Name: id},
 						Error: nil,
@@ -163,13 +185,13 @@ func userBatchFn(slackClient interfaces.SlackClient) func(ctx context.Context, k
 				}
 
 				// Build results in the same order as keys
-				for i, key := range keys {
-					if user, found := userMap[key]; found {
+				for i, userID := range slackUserIndices {
+					if user, found := userMap[userID]; found {
 						results[i] = &dataloader.Result[*graphql1.User]{Data: user, Error: nil}
 					} else {
 						// User not found in Slack response, fallback to ID
 						results[i] = &dataloader.Result[*graphql1.User]{
-							Data:  &graphql1.User{ID: key, Name: key},
+							Data:  &graphql1.User{ID: userID, Name: userID},
 							Error: nil,
 						}
 					}
@@ -179,7 +201,7 @@ func userBatchFn(slackClient interfaces.SlackClient) func(ctx context.Context, k
 		}
 
 		// Fallback for when SlackClient is nil (not an error condition)
-		for i, id := range keys {
+		for i, id := range slackUserIndices {
 			results[i] = &dataloader.Result[*graphql1.User]{
 				Data:  &graphql1.User{ID: id, Name: id},
 				Error: nil,

pkg/controller/graphql/dataloader_test.go

@@ -418,6 +418,106 @@ func TestUserLoaderUseBatchMethod(t *testing.T) {
 	}
 }
 
+func TestSystemUserLoader(t *testing.T) {
+	t.Run("System user does not call Slack API", func(t *testing.T) {
+		repo, slackClient := setupTestData()
+		loaders := NewDataLoaders(repo, slackClient)
+		ctx := context.Background()
+
+		// Load system user
+		user, err := GetUserWithLoaders(ctx, loaders, string(types.SystemUserID))
+		gt.NoError(t, err)
+		gt.Equal(t, user.ID, string(types.SystemUserID))
+		gt.Equal(t, user.Name, "System")
+
+		// Verify Slack API was NOT called
+		getUsersInfoCalls := slackClient.GetUsersInfoCalls()
+		gt.Number(t, len(getUsersInfoCalls)).Equal(0)
+	})
+
+	t.Run("System user mixed with regular users", func(t *testing.T) {
+		repo, slackClient := setupTestData()
+		loaders := NewDataLoaders(repo, slackClient)
+		ctx := context.Background()
+
+		// Load system user and regular users concurrently
+		userIDs := []string{string(types.SystemUserID), "user1", "user2"}
+		results := make([]*graphql1.User, len(userIDs))
+		errors := make([]error, len(userIDs))
+
+		var wg sync.WaitGroup
+		for i, id := range userIDs {
+			wg.Add(1)
+			go func(index int, userID string) {
+				defer wg.Done()
+				u, err := GetUserWithLoaders(ctx, loaders, userID)
+				results[index] = u
+				errors[index] = err
+			}(i, id)
+		}
+		wg.Wait()
+
+		// Verify all loads returned correct data
+		for i, err := range errors {
+			gt.NoError(t, err)
+			gt.NotNil(t, results[i])
+			gt.Equal(t, results[i].ID, userIDs[i])
+		}
+
+		// Verify system user has correct name
+		gt.Equal(t, results[0].Name, "System")
+		gt.Equal(t, results[1].Name, "User One")
+		gt.Equal(t, results[2].Name, "User Two")
+
+		// Verify Slack API was called only for non-system users
+		getUsersInfoCalls := slackClient.GetUsersInfoCalls()
+		gt.Number(t, len(getUsersInfoCalls)).Greater(0)
+
+		// The batch call should only contain non-system users
+		if len(getUsersInfoCalls) > 0 {
+			calledUsers := getUsersInfoCalls[0].Users
+			for _, userID := range calledUsers {
+				gt.NotEqual(t, userID, string(types.SystemUserID))
+			}
+		}
+	})
+
+	t.Run("Multiple system users only", func(t *testing.T) {
+		repo, slackClient := setupTestData()
+		loaders := NewDataLoaders(repo, slackClient)
+		ctx := context.Background()
+
+		// Load multiple system users
+		userIDs := []string{string(types.SystemUserID), string(types.SystemUserID)}
+		results := make([]*graphql1.User, len(userIDs))
+		errors := make([]error, len(userIDs))
+
+		var wg sync.WaitGroup
+		for i, id := range userIDs {
+			wg.Add(1)
+			go func(index int, userID string) {
+				defer wg.Done()
+				u, err := GetUserWithLoaders(ctx, loaders, userID)
+				results[index] = u
+				errors[index] = err
+			}(i, id)
+		}
+		wg.Wait()
+
+		// Verify all loads returned correct data
+		for i, err := range errors {
+			gt.NoError(t, err)
+			gt.NotNil(t, results[i])
+			gt.Equal(t, results[i].ID, string(types.SystemUserID))
+			gt.Equal(t, results[i].Name, "System")
+		}
+
+		// Verify Slack API was NOT called at all
+		getUsersInfoCalls := slackClient.GetUsersInfoCalls()
+		gt.Number(t, len(getUsersInfoCalls)).Equal(0)
+	})
+}
+
 // Helper functions for testing with loaders
 func GetTicketWithLoaders(ctx context.Context, loaders *DataLoaders, ticketID types.TicketID) (*ticket.Ticket, error) {
 	thunk := loaders.TicketLoader.Load(ctx, ticketID)