go-sdk icon indicating copy to clipboard operation
go-sdk copied to clipboard

replace errgroup with conc pool in WriteExecute method

Open AltuisticIsopod opened this issue 1 month ago • 6 comments

Replace errgroup with conc pool in WriteExecute method

Description

  • Replace errgroup with conc pool for parallel write/delete operations
  • Use errors.As() instead of type assertion for authentication error detection
  • Handle wrapped errors from conc pool using errors.As() in both client and test
  • Add nil check before dereferencing singleResponse to prevent panics
  • Update test to use errors.As() for proper error unwrapping

This PR only moved "Write" to use Conc

References

https://github.com/openfga/go-sdk/issues/193

Review Checklist

  • [x] I have clicked on "allow edits by maintainers".
  • [x] I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • [x] The correct base branch is being used, if not main
  • [x] I have added tests to validate that the change in functionality is working as expected

AltuisticIsopod avatar Oct 17 '25 21:10 AltuisticIsopod

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: AltuisticIsopod / name: Lalit Sudhir (3915e47da00d9106bb99091c24fcd7f124b6ee53, dfeff0b3d2d025d0c25ff854939376f92138ce96)
  • :white_check_mark: login: SoulPancake / name: Anurag Bandyopadhyay (1e88ab5c1920262069a1802627b3658937b543d3)

[!IMPORTANT]

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The 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

Cohort / File(s) Summary
Batch processing parallelism refactor
client/client.go
Replaced error-group concurrent pattern with pool-based workers (pool.NewWithResults with WithContext and WithMaxGoroutines) for WriteExecute and Delete chunk processing; switched error handling to use errors.As for FgaApiAuthenticationError classification
Public API surface expansion
client/client.go
Added four new public methods to SdkClient interface: SetAuthorizationModelId(string) error, GetAuthorizationModelId() (string, error), SetStoreId(string) error, GetStoreId() (string, error); implemented on OpenFgaClient with ULID format validation and config storage
Error handling improvements
client/client.go, client/client_test.go
Added standard errors package import and replaced direct type assertions with errors.As checks for robust FgaApiAuthenticationError detection across multiple code paths

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)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "replace errgroup with conc pool in WriteExecute method" directly aligns with the primary technical objective stated in the PR description. The title accurately identifies the main refactoring work: replacing the concurrent error-group approach with a conc pool-based approach for parallel operations. While the changeset includes additional modifications such as new public API methods (SetAuthorizationModelId, GetAuthorizationModelId, SetStoreId, GetStoreId) and error handling improvements using errors.As(), the title captures the core technical change that drives the majority of the code review effort. The title is specific, clear, and would enable a teammate reviewing the git history to understand the primary refactoring being performed.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 17 '25 21:10 coderabbitai[bot]

  • Removed manual slice management and indexing for write/delete operations
  • Used conc pool's automatic result collection instead of manual assignment

AltuisticIsopod avatar Oct 18 '25 00:10 AltuisticIsopod

Hey @rhamzeh, I have replaced errgroup with conc for Write. Can you review and let me know if you find any issues. Can you please add Hacktoberfest label to this issue? Thank you.

AltuisticIsopod avatar Oct 18 '25 00:10 AltuisticIsopod

Hi @AltuisticIsopod, same as with https://github.com/openfga/sdk-generator/pull/653, we'll assign someone to review your PR. Thanks.

dyeam0 avatar Nov 03 '25 04:11 dyeam0

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 33.88%. Comparing base (8ebd110) to head (3915e47).

:x: Your project status has failed because the head coverage (33.88%) 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.90%   33.88%   -0.03%     
==========================================
  Files         111      111              
  Lines       10614    10610       -4     
==========================================
- Hits         3599     3595       -4     
  Misses       6663     6663              
  Partials      352      352              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Nov 03 '25 05:11 codecov-commenter