-
Notifications
You must be signed in to change notification settings - Fork 21
fix: handle empty auth models list in readlatest auth model #265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdded defensive programming to prevent Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.javaThanks 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
============================================
+ Coverage 37.12% 37.14% +0.01%
- Complexity 1195 1196 +1
============================================
Files 192 192
Lines 7472 7474 +2
Branches 862 863 +1
============================================
+ Hits 2774 2776 +2
Misses 4568 4568
Partials 130 130 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/dev/openfga/sdk/api/client/model/ClientReadAuthorizationModelResponse.java (1)
37-40: Consider also guarding against a null authorizationModels listThe new check avoids
IndexOutOfBoundsExceptionfor empty lists, which is good. Ifresponse.getAuthorizationModels()can ever benull(e.g., field missing or older server),models.isEmpty()would still throw aNullPointerException. A small tweak would make this fully defensive:- List<dev.openfga.sdk.api.model.AuthorizationModel> models = response.getAuthorizationModels(); - if (!models.isEmpty()) { + List<dev.openfga.sdk.api.model.AuthorizationModel> models = response.getAuthorizationModels(); + if (models != null && !models.isEmpty()) { clientResponse.setAuthorizationModel(models.get(0)); }This preserves existing behavior for non-null lists while handling both empty and absent lists safely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/dev/openfga/sdk/api/client/model/ClientReadAuthorizationModelResponse.java(1 hunks)src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (1)
src/main/java/dev/openfga/sdk/constants/FgaConstants.java (1)
FgaConstants(19-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
- GitHub Check: Test and Build OpenFGA (17)
- GitHub Check: Test and Build OpenFGA (11)
- GitHub Check: Test and Build OpenFGA (21)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (1)
749-764: Empty-list regression test looks correct and consistentThe test accurately exercises the empty
authorization_modelscase, verifies the correct URL and call count, and asserts the new contract (authorizationModelis null rather than throwing). Fits well with existing test patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where readLatestAuthorizationModel() would throw an IndexOutOfBoundsException when attempting to access the first element of an empty authorization models list. The fix adds a null-safety check before accessing the list, and includes a test case to verify the behavior.
Key changes:
- Added empty list check in
ClientReadAuthorizationModelResponse.latestOf()to prevent IndexOutOfBoundsException - Added test case
readLatestAuthorizationModelTest_emptyListto verify null is returned when no models exist
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/main/java/dev/openfga/sdk/api/client/model/ClientReadAuthorizationModelResponse.java | Added null-safety check before accessing the first authorization model from the list |
| src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java | Added test case to verify empty list handling returns null authorization model |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.