replace errgroup with conc pool in WriteExecute method #249
Conversation
|
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes replace concurrent error-group parallelism with a pool-based approach in batch write and delete operations, add four new public methods to the SdkClient interface for managing authorization model and store IDs, and improve error handling by using the errors.As package for safer type assertions across the client and test code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes The changes introduce multiple distinct patterns: a significant parallelism refactor in critical write/delete paths requiring verification of pool-worker error flow, new public API methods with straightforward validation logic, and consistent but distributed error handling improvements across two files that each require separate reasoning. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (3)
client/client_test.go (1)
1738-1740: Align this test with errors.As for consistencyUse errors.As here too to avoid panics on wrapped errors.
- if _, ok := err.(openfga.FgaApiAuthenticationError); !ok { - t.Fatalf("Expected an api auth error") - } + var authErr openfga.FgaApiAuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("Expected an api auth error") + }client/client.go (2)
1792-1823: Cancel peers on first auth errorPools don’t cancel sibling tasks; previous errgroup did via context. Add WithCancelOnError so an auth failure stops in-flight work.
- writePool := pool.NewWithResults[*ClientWriteResponse]().WithContext(request.GetContext()).WithMaxGoroutines(int(maxParallelReqs)) + writePool := pool.NewWithResults[*ClientWriteResponse](). + WithContext(request.GetContext()). + WithMaxGoroutines(int(maxParallelReqs)). + WithCancelOnError() - deletePool := pool.NewWithResults[*ClientWriteResponse]().WithContext(request.GetContext()).WithMaxGoroutines(int(maxParallelReqs)) + deletePool := pool.NewWithResults[*ClientWriteResponse](). + WithContext(request.GetContext()). + WithMaxGoroutines(int(maxParallelReqs)). + WithCancelOnError()Also applies to: 1837-1872
1830-1834: Use deleteChunkSize when chunking deletesCurrently uses writeChunkSize; same value today, but confusing.
- end := int(math.Min(float64(i+writeChunkSize), float64(len(request.GetBody().Deletes)))) + end := int(math.Min(float64(i+deleteChunkSize), float64(len(request.GetBody().Deletes))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/client.go(5 hunks)client/client_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/client_test.go (1)
errors.go (1)
FgaApiAuthenticationError(47-60)
client/client.go (1)
errors.go (1)
FgaApiAuthenticationError(47-60)
🔇 Additional comments (2)
client/client_test.go (1)
1779-1781: Good switch to errors.As for auth error unwrappingSafer and matches wrapped errors from conc pool. LGTM.
Also applies to: 1798-1800
client/client.go (1)
1811-1819: Nice: errors.As + nil-check avoid panics and classify auth errors correctlyThis correctly bubbles auth errors while still aggregating non-auth failures per item. LGTM.
Also applies to: 1857-1866
|
|
Hey @rhamzeh, I have replaced errgroup with conc for Write. Can you review and let me know if you find any issues. |
|
Hi @AltuisticIsopod, same as with openfga/sdk-generator#653, we'll assign someone to review your PR. Thanks. |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (33.62%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 33.58% 33.62% +0.04%
==========================================
Files 115 115
Lines 9808 9814 +6
==========================================
+ Hits 3294 3300 +6
Misses 6252 6252
Partials 262 262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…isticIsopod/go-sdk into task-Move_Write_to_use_conc
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the WriteExecute method to use github.com/sourcegraph/conc/pool instead of golang.org/x/sync/errgroup for parallel processing, and updates error handling to use errors.As() instead of type assertions for checking authentication errors.
Key changes:
- Replaced
errgroupwithpool.NewWithResultsfor managing concurrent write and delete operations - Updated authentication error checking from type assertions (
err.(Type)) toerrors.As()for better error unwrapping support - Updated corresponding test cases to use the new error checking pattern
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| client/client.go | Refactored WriteExecute to use conc pool library for concurrent operations and updated error handling to use errors.As() |
| client/client_test.go | Updated test cases to use errors.As() for authentication error checking |
Comments suppressed due to low confidence (1)
client/client.go:1860
- The logic assumes
writeResponsesanddeleteResponsescontain non-nil pointers, but the pool returns[]*ClientWriteResponse. If any goroutine returnsnilfor the response (which happens in lines 1801 and 1842 when authentication errors occur), this will cause a nil pointer dereference when accessing.Writesor.Deletes. Add nil checks before dereferencing:if writeResponse != nil { response.Writes = append(...) }
for _, writeResponse := range writeResponses {
response.Writes = append(response.Writes, writeResponse.Writes...)
}
for _, deleteResponse := range deleteResponses {
response.Deletes = append(response.Deletes, deleteResponse.Deletes...)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SoulPancake
left a comment
SoulPancake
left a comment
There was a problem hiding this comment.
LG for WriteExecute, Can you possibly also do this for ClientBatchCheck? @AltuisticIsopod
|
Sure, I will do that. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var authErr fgaSdk.FgaApiAuthenticationError | ||
| // If an error was returned then it will be an authentication error so we want to return | ||
| if errors.As(err, &authErr) { |
There was a problem hiding this comment.
The comment states "If an error was returned then it will be an authentication error" but this is inaccurate. The errors.As check only handles authentication errors, but other types of errors could also be returned from client.WriteExecute. Non-authentication errors would still pass through and return singleResponse (which could be nil) with err=nil, leading to potential nil pointer dereferences when iterating over writeResponses.
| var authErr fgaSdk.FgaApiAuthenticationError | |
| // If an error was returned then it will be an authentication error so we want to return | |
| if errors.As(err, &authErr) { | |
| if err != nil { |
| // If an error was returned then it will be an authentication error so we want to return | ||
| if errors.As(err, &authErr) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference. If client.WriteExecute returns a non-authentication error with singleResponse=nil, the code at line 1799 will return nil for the error (since errors.As will be false), but singleResponse will be nil. This nil response will then be dereferenced at line 1854 causing a panic. The code should check if err is not nil (for any error type) and return it, or ensure singleResponse is not nil before returning it.
| } | |
| } | |
| if err != nil { | |
| return nil, err | |
| } |
| var authErr fgaSdk.FgaApiAuthenticationError | ||
| if errors.As(err, &authErr) { |
There was a problem hiding this comment.
Potential nil pointer dereference. If client.WriteExecute returns a non-authentication error with singleResponse=nil, the code will return nil with no error (since errors.As will be false for non-authentication errors). This nil response will then be dereferenced at line 1858 causing a panic. The code should check if err is not nil (for any error type) and return it, or ensure singleResponse is not nil before returning it.
| var authErr fgaSdk.FgaApiAuthenticationError | |
| if errors.As(err, &authErr) { | |
| if err != nil { |
| // If an error was returned then it will be an authentication error so we want to return | ||
| if errors.As(err, &authErr) { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
The comment states "If an error was returned then it will be an authentication error" but this is inaccurate. The errors.As check only handles authentication errors, but other types of errors could also be returned from client.CheckExecute. Non-authentication errors would pass through to be stored in the Error field of the response, but singleResponse could be nil, leading to a potential nil pointer dereference at line 2276.
| // If an error was returned then it will be an authentication error so we want to return | |
| if errors.As(err, &authErr) { | |
| return nil, err | |
| } | |
| // If the error is an authentication error, return it immediately. | |
| if errors.As(err, &authErr) { | |
| return nil, err | |
| } | |
| // If any other error occurred and singleResponse is nil, avoid nil pointer dereference. | |
| if err != nil && singleResponse == nil { | |
| return &batchCheckResult{ | |
| Index: index, | |
| Response: ClientBatchCheckClientSingleResponse{ | |
| Request: checkBody, | |
| ClientCheckResponse: ClientCheckResponse{}, // zero value | |
| Error: err, | |
| }, | |
| }, nil | |
| } |
| return &batchCheckResult{ | ||
| Index: index, | ||
| Response: ClientBatchCheckClientSingleResponse{ | ||
| Request: checkBody, | ||
| ClientCheckResponse: *singleResponse, |
There was a problem hiding this comment.
Potential nil pointer dereference. If client.CheckExecute returns a non-authentication error with singleResponse=nil, the code will dereference singleResponse at line 2276 causing a panic. The code should check if singleResponse is nil before dereferencing it, especially since the err is being stored in the response for non-authentication errors.
| return &batchCheckResult{ | |
| Index: index, | |
| Response: ClientBatchCheckClientSingleResponse{ | |
| Request: checkBody, | |
| ClientCheckResponse: *singleResponse, | |
| var clientCheckResponse fgaSdk.ClientCheckResponse | |
| if singleResponse != nil { | |
| clientCheckResponse = *singleResponse | |
| } | |
| return &batchCheckResult{ | |
| Index: index, | |
| Response: ClientBatchCheckClientSingleResponse{ | |
| Request: checkBody, | |
| ClientCheckResponse: clientCheckResponse, |
|
Hi @SoulPancake , The earlier codebase was only checking for errors of type fgaSdk.FgaApiAuthenticationError. So, I followed the same and I am only catching those errors. |
|
@AltuisticIsopod |
4 participants
Replace errgroup with conc pool in WriteExecute method
Description
This PR only moved "Write" to use Conc
References
#193
Review Checklist
main