Auto-paginated project users #38
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/provider/data_source_openai_project_user.go (2)
🔇 Additional comments (3)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/client/client.go (1)
1859-1897: Guard against empty cursors to prevent infinite pagination loops.If the API ever returns
has_more = truebut omitslast_id,afterwill stay unchanged and we’ll keep re-fetching the same page forever. Please add a fallback (e.g. break whenlast_id == ""or derive the cursor from the last element) so the loop always progresses or aborts safely.internal/provider/resource_openai_project_user.go (1)
155-179: Reuse fixed cursor handling in retry path.The duplicate pagination in the retry branch needs the same empty-
LastIDguard; otherwise a missing cursor will loop forever right after analready existserror. Please apply the same fix there.
🧹 Nitpick comments (1)
internal/provider/data_source_openai_project_user.go (1)
118-124: Usestrings.EqualFoldinstead of fmt.Sprintf for email match.
fmt.Sprintf("%s", ...)is flagged by staticcheckS1025. Replace the redundant formatting with a case-insensitive comparison, e.g.strings.EqualFold(user.Email, email), to satisfy CI and make the intent clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/client/client.go(1 hunks)internal/provider/data_source_openai_project_user.go(3 hunks)internal/provider/data_source_openai_project_users.go(1 hunks)internal/provider/resource_openai_project_user.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/client/client.go (1)
internal/provider/provider.go (1)
OpenAIClient(27-31)
internal/provider/resource_openai_project_user.go (2)
internal/client/client.go (1)
ProjectUser(160-166)internal/provider/provider.go (1)
GetOpenAIClientWithAdminKey(83-110)
internal/provider/data_source_openai_project_user.go (2)
internal/client/client.go (1)
ProjectUser(160-166)internal/provider/provider.go (1)
GetOpenAIClientWithAdminKey(83-110)
🪛 GitHub Actions: CI
internal/provider/data_source_openai_project_user.go
[error] 120-120: S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
🪛 GitHub Check: golangci
internal/provider/data_source_openai_project_user.go
[failure] 120-120:
S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
| } | ||
|
|
||
| // Look for the user with matching email in this page (case insensitive) | ||
| for _, user := range userList.Data { | ||
| if user.Email == email || (len(user.Email) == len(email) && | ||
| fmt.Sprintf("%s", user.Email) == fmt.Sprintf("%s", email)) { | ||
| tflog.Debug(ctx, fmt.Sprintf("Found user with email %s in project %s on page %d", email, projectID, pageCount)) | ||
| return &user, true, nil | ||
| } | ||
| } | ||
|
|
||
| // Check if there are more pages | ||
| hasMore = userList.HasMore | ||
| if hasMore && userList.LastID != "" { | ||
| after = userList.LastID | ||
| } | ||
| } | ||
|
|
||
| tflog.Debug(ctx, fmt.Sprintf("User with email %s not found in project %s after checking %d pages", email, projectID, pageCount)) | ||
| return nil, false, nil |
There was a problem hiding this comment.
Break out when the API omits LastID.
To keep pagination resilient, bail when HasMore is true but LastID comes back empty (or compute the cursor from the last element). Without that, the loop may keep fetching the same page.
🧰 Tools
🪛 GitHub Actions: CI
[error] 120-120: S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
🪛 GitHub Check: golangci
[failure] 120-120:
S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
| // Automatic pagination - fetch all users with default batch size | ||
| const batchSize = 100 | ||
| tflog.Debug(ctx, fmt.Sprintf("Fetching all users in project %s with batch size: %d", projectID, batchSize)) | ||
|
|
||
| var allUsers []map[string]interface{} | ||
| var after string | ||
| hasMore := true | ||
| pageCount := 0 | ||
|
|
||
| // Paginate through all results | ||
| for hasMore { | ||
| pageCount++ | ||
| tflog.Debug(ctx, fmt.Sprintf("Fetching page %d for project %s (after: %s)", pageCount, projectID, after)) | ||
|
|
||
| usersList, err := c.ListProjectUsers(projectID, after, batchSize) | ||
| if err != nil { | ||
| tflog.Error(ctx, fmt.Sprintf("Error listing users (page %d): %v", pageCount, err)) | ||
| return diag.Errorf("Error listing users in project (page %d): %s", pageCount, err) | ||
| } | ||
|
|
||
| // Transform the users into a format appropriate for the schema | ||
| users := make([]map[string]interface{}, 0, len(usersList.Data)) | ||
| for _, user := range usersList.Data { | ||
| userData := map[string]interface{}{ | ||
| "id": user.ID, | ||
| "email": user.Email, | ||
| "role": user.Role, | ||
| "added_at": user.AddedAt, | ||
| // Transform the users from this page into a format appropriate for the schema | ||
| for _, user := range usersList.Data { | ||
| userData := map[string]interface{}{ | ||
| "id": user.ID, | ||
| "email": user.Email, | ||
| "role": user.Role, | ||
| "added_at": user.AddedAt, | ||
| } | ||
| allUsers = append(allUsers, userData) | ||
| } | ||
|
|
||
| // Check if there are more pages | ||
| hasMore = usersList.HasMore | ||
| if hasMore && usersList.LastID != "" { | ||
| after = usersList.LastID | ||
| } | ||
| users = append(users, userData) | ||
| } | ||
|
|
||
| tflog.Debug(ctx, fmt.Sprintf("Fetched %d total users for project %s across %d pages", len(allUsers), projectID, pageCount)) | ||
|
|
There was a problem hiding this comment.
Handle missing last_id to avoid re-fetching the same page.
If HasMore comes back true but LastID is empty, after remains unchanged and the loop repeats the same request indefinitely. Please bail out (or derive the cursor from the last record) when usersList.LastID == "" so pagination can’t spin forever.
| const batchSize = 100 | ||
| tflog.Debug(ctx, fmt.Sprintf("Finding user %s in project %s with pagination", userID, projectID)) | ||
|
|
||
| var after string | ||
| hasMore := true | ||
| pageCount := 0 | ||
|
|
||
| for hasMore { | ||
| pageCount++ | ||
| tflog.Debug(ctx, fmt.Sprintf("Fetching page %d for project %s (after: %s)", pageCount, projectID, after)) | ||
|
|
||
| userList, err := clientInstance.ListProjectUsers(projectID, after, batchSize) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("error fetching project users (page %d): %w", pageCount, err) | ||
| } | ||
|
|
||
| // Look for the user in this page | ||
| for _, user := range userList.Data { | ||
| if user.ID == userID { | ||
| tflog.Debug(ctx, fmt.Sprintf("Found user %s in project %s on page %d", userID, projectID, pageCount)) | ||
| return &user, true, nil | ||
| } | ||
| } | ||
|
|
||
| // Check if there are more pages | ||
| hasMore = userList.HasMore | ||
| if hasMore && userList.LastID != "" { | ||
| after = userList.LastID | ||
| } | ||
| } | ||
|
|
||
| tflog.Debug(ctx, fmt.Sprintf("User %s not found in project %s after checking %d pages", userID, projectID, pageCount)) | ||
| return nil, false, nil | ||
| } |
There was a problem hiding this comment.
Protect pagination loop from empty cursors.
Same cursor issue here: when HasMore is true yet LastID is blank, after never advances and we re-request the same page. Please guard against that case by breaking or extracting the ID from the last element before looping again.
🤖 Prompt for AI Agents
In internal/provider/resource_openai_project_user.go around lines 68 to 101 the
pagination loop can re-request the same page when userList.HasMore is true but
userList.LastID is empty; guard against that by: if HasMore is true and LastID
is non-empty set after = LastID, otherwise if HasMore is true but LastID is
empty then if userList.Data is non-empty set after to the ID of the last element
in userList.Data, else break the loop (no more progress); this ensures the
cursor advances or the loop exits instead of looping forever.
Currently it works only with first 20 users. This PR fixes it, including this issue #36
Summary by CodeRabbit
New Features
Bug Fixes