fix: apply per-request min_wait_in_ms to wait, not max_retry - #320
SoulPancake merged 1 commit into
Conversation
RetryParams(max_retry=0, min_wait_in_ms=100) currently becomes 101 attempts because the overlay copies min_wait onto max_retry. Fixes openfga#319
|
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe asynchronous and synchronous API clients now apply per-request minimum-wait overrides to ChangesPer-request retry parameter overrides
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The retry override correction appears ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The correction makes per-request retry limits work as intended, preventing excess API calls during retryable failures. The reviewed paths do not change authentication or which endpoints callers can reach. Deployment context remains unavailable. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
SoulPancake
left a comment
There was a problem hiding this comment.
Thanks a lot @pieramarchesini
LGTM
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (69.99%) 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 #320 +/- ##
==========================================
+ Coverage 69.93% 69.99% +0.05%
==========================================
Files 142 142
Lines 10774 10774
==========================================
+ Hits 7535 7541 +6
+ Misses 3239 3233 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #319.
Bug
Applying per-request
_retry_paramsinApiClient.call_apicopiesmin_wait_in_msontomax_retry:RetryParams.__init__always setsmin_wait_in_ms(default 100), so any per-request overlay overwrites the retry count with the wait-in-ms value.Intended: 1 attempt (
range(0 + 1)).Actual: 101 attempts (
range(100 + 1)).Same copy-paste is in
openfga_sdk/sync/api_client.py. Introduced in fb55350 (feat: improved handling of retries (#188)). Still present onmain/ v0.10.4.Existing 5xx tests only set
configuration.retry_params, so they never hit this overlay.Fix
Assign
min_wait_in_msontomin_wait_in_msin both async and sync clients. That is the only correct mapping for the three overlay fields (max_retry,min_wait_in_ms,max_wait_in_sec).These files are not in
.openapi-generator/FILESand are not marked generated, so this repo is the right place (no sdk-generator PR).Test plan
AssertionError: 101 != 1test_500_error/test_500_error_retrystill passruff check/ruff formatclean on the touched filesSummary by CodeRabbit