Validate StreamRetryPolicy at processor entry#56
Validate StreamRetryPolicy at processor entry#56aleksandar-apostolov wants to merge 1 commit intodevelopfrom
StreamRetryPolicy at processor entry#56Conversation
- Extract requireValid() as internal member on StreamRetryPolicy - Call policy.requireValid() at the top of StreamRetryProcessor.retry() - Add @IntRange annotations to constructor and factory method params - Fix error message: "minRetries must be > 0" (was incorrectly "≥ 0") - Update test to expect IllegalArgumentException from requireValid() instead of IllegalStateException from unreachable fallthrough
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
aleksandar-apostolov
changed the title
StreamRetryPolicy at processor entry
WalkthroughThe changes refactor the validation mechanism in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt (1)
32-38:⚠️ Potential issue | 🟡 MinorUpdate the KDoc to match the new
minRetries > 0invariant.The docs still say
minRetries ≥ 0and that0disables retries, but the annotations andrequireValid()now reject0.📝 Proposed KDoc update
- * * `minRetries ≥ 0` + * * `minRetries > 0` * * `maxRetries ≥ minRetries` * * `0 ≤ minBackoffMills ≤ maxBackoffMills` * - * `@param` minRetries Minimum number of retry attempts (not counting the original call). Set to `0` - * to disable retries. + * `@param` minRetries Minimum number of retry attempts (not counting the original call). Must be + * greater than `0`.As per coding guidelines, “Keep processor/queue behaviour documented via KDoc or dedicated docs when semantics evolve”.
Also applies to: 53-57, 234-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt` around lines 32 - 38, The KDoc for StreamRetryPolicy is out of date: change the invariant and parameter descriptions to reflect that minRetries must be > 0 (not ≥ 0) and that 0 is no longer allowed (so you cannot disable retries by setting 0); update all occurrences referencing `minRetries ≥ 0` and “Set to `0` to disable retries” (including the blocks around lines referencing the class and the other affected KDoc sections) to state the new invariant `minRetries > 0` and explain the intended semantics, and ensure the comment mentions `requireValid()` enforces this constraint so callers understand the validation behavior.
🧹 Nitpick comments (1)
stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt (1)
205-225: Add@IntRangeannotations tocustom()parameters for consistency with other factory methods.All three preset factories (
exponential,linear,fixed) use@IntRangeannotations on their numeric parameters, butcustom()lacks them. This removes IDE-level validation hints for callers and creates an inconsistent API surface.Apply annotations
public fun custom( - minRetries: Int, - maxRetries: Int, - minBackoffMills: Long, - maxBackoffMills: Long, - initialDelayMillis: Long, + `@IntRange`(from = 1) minRetries: Int, + `@IntRange`(from = 1) maxRetries: Int, + `@IntRange`(from = 0) minBackoffMills: Long, + `@IntRange`(from = 0) maxBackoffMills: Long, + `@IntRange`(from = 0) initialDelayMillis: Long, giveUp: (Int, Throwable) -> Boolean = { retry, _ -> retry > maxRetries }, nextDelay: (Int, Long) -> Long, ): StreamRetryPolicy =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt` around lines 205 - 225, The custom() factory is missing `@IntRange` annotations on its numeric parameters, which makes it inconsistent with exponential/linear/fixed; update the custom(...) signature in StreamRetryPolicy to add the same `@IntRange`(from=..., to=...) annotations used by the other factories for minRetries, maxRetries, minBackoffMills, maxBackoffMills and initialDelayMillis (import androidx.annotation.IntRange if needed) so callers get IDE validation; keep the rest of the signature (giveUp, nextDelay, return StreamRetryPolicy and requireValid()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt`:
- Around line 234-245: The internal function requireValid in StreamRetryPolicy
lacks an explicit return type required by explicit API mode; update its
signature to declare a Unit return type (i.e., change fun requireValid() to fun
requireValid(): Unit) so the compiler accepts the internal API, leaving the
function body unchanged; ensure you modify the declaration in the
StreamRetryPolicy class where requireValid is defined.
---
Outside diff comments:
In
`@stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt`:
- Around line 32-38: The KDoc for StreamRetryPolicy is out of date: change the
invariant and parameter descriptions to reflect that minRetries must be > 0 (not
≥ 0) and that 0 is no longer allowed (so you cannot disable retries by setting
0); update all occurrences referencing `minRetries ≥ 0` and “Set to `0` to
disable retries” (including the blocks around lines referencing the class and
the other affected KDoc sections) to state the new invariant `minRetries > 0`
and explain the intended semantics, and ensure the comment mentions
`requireValid()` enforces this constraint so callers understand the validation
behavior.
---
Nitpick comments:
In
`@stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.kt`:
- Around line 205-225: The custom() factory is missing `@IntRange` annotations on
its numeric parameters, which makes it inconsistent with
exponential/linear/fixed; update the custom(...) signature in StreamRetryPolicy
to add the same `@IntRange`(from=..., to=...) annotations used by the other
factories for minRetries, maxRetries, minBackoffMills, maxBackoffMills and
initialDelayMillis (import androidx.annotation.IntRange if needed) so callers
get IDE validation; keep the rest of the signature (giveUp, nextDelay, return
StreamRetryPolicy and requireValid()) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3ed3655-8ee3-4cda-9bec-32c47dc9d93d
📒 Files selected for processing (3)
stream-android-core/src/main/java/io/getstream/android/core/api/model/retry/StreamRetryPolicy.ktstream-android-core/src/main/java/io/getstream/android/core/internal/processing/StreamRetryProcessorImpl.ktstream-android-core/src/test/java/io/getstream/android/core/internal/processing/StreamRetryProcessorImplTest.kt
|
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
|
The 21.7% duplication is pre-existing structural duplication from the factory pattern — Extracting a shared config class would hurt call-site ergonomics ( Same pattern exists in |
2 participants
Goal
Defense-in-depth: validate
StreamRetryPolicyinvariants at the point of consumption (StreamRetryProcessor.retry()), not just at creation time.Addresses a review observation from GetStream/stream-video-android#1645 (by @rahul-lohra): the processor's
while (attempt <= policy.maxRetries)loop silently skips execution ifmaxRetries < 1, falling through to a confusingIllegalStateException("Check your policy"). While factory methods already validate viarequireValid(), the processor itself should guard against invalid policies as well.Implementation
StreamRetryPolicy.requireValid()— extracted from the privatevalidate()companion extension into aninternalmember function. Single source of truth for invariant checks, callable from both factory methods and the processor.StreamRetryProcessor.retry()— callspolicy.requireValid()at entry. Invalid policies now fail immediately with a clearIllegalArgumentExceptionmessage instead of silently skipping the retry loop.@IntRangeannotations — added to constructor and all factory method parameters for IDE-level early detection."minRetries must be ≥ 0"corrected to"minRetries must be > 0"(therequirecheck is> 0, not>= 0).Testing
StreamRetryProcessorImplTest— themaxRetries = 0test now expectsIllegalArgumentExceptionwith message"maxRetries must be"instead ofIllegalStateExceptionwith"Check your policy".StreamRetryPolicyTestandStreamRetryProcessorImplTesttests pass.Summary by CodeRabbit
Refactor
Chores