fix(depth-check): correctly handle 'depth not enough' error condition #1707
Conversation
WalkthroughThe changes involve updating the version number from "v1.1.4" to "v1.1.5" across multiple files associated with the Permify API documentation and versioning. Additionally, modifications were made to the permission checking mechanisms in the code, including the introduction of a new helper function for metadata responses, updates to ensure thread safety in concurrent operations, and the removal of outdated functions. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
internal/invoke/invoke.go (2)
197-198: LGTM: Thread-safe check count incrementThe use of
atomic.AddInt32for incrementing the check count ensures thread-safety in concurrent scenarios. This is an improvement over the previous non-atomic operation.Consider updating the comment to reflect the atomic nature of the operation:
- // increaseCheckCount increments the CheckCount value in the response metadata by 1. + // Atomically increment the CheckCount value in the response metadata by 1.
Line range hint
1-458: Summary: Improved thread-safety in Check methodThe changes in this file focus on enhancing thread-safety in the
Checkmethod of theDirectInvokerstruct. The introduction of atomic operations for depth decrement and check count increment ensures that these operations are performed safely in concurrent scenarios. These modifications are well-implemented and align with best practices for concurrent programming.Consider reviewing other methods in this file (and related files) that might benefit from similar thread-safety improvements, especially if they are likely to be called concurrently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
- docs/api-reference/apidocs.swagger.json (1 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (1 hunks)
- internal/engines/check.go (18 hunks)
- internal/info.go (1 hunks)
- internal/invoke/invoke.go (3 hunks)
- internal/invoke/utils.go (1 hunks)
- proto/base/v1/openapi.proto (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- docs/api-reference/apidocs.swagger.json
- docs/api-reference/openapiv2/apidocs.swagger.json
- internal/info.go
- proto/base/v1/openapi.proto
🧰 Additional context used
🔇 Additional comments (12)
internal/invoke/invoke.go (2)
5-5: LGTM: Import of sync/atomic packageThe addition of the
sync/atomicpackage import is appropriate for the subsequent changes involving atomic operations in theCheckmethod.
179-179: LGTM: Thread-safe depth decrementThe use of
atomic.AddInt32for decrementing the depth ensures thread-safety in concurrent scenarios. This is an improvement over the previous non-atomic operation.internal/engines/check.go (8)
64-66: LGTM: Improved consistency in metadata creationThe change to use
emptyResponseMetadata()instead of directly instantiatingbase.PermissionCheckResponseMetadata{}is a good improvement. It centralizes the creation of empty response metadata, making the code more consistent and easier to maintain.
116-117: LGTM: Consistent use of emptyResponseMetadata()The change to use
emptyResponseMetadata()here is consistent with the previous changes and maintains the improved pattern for creating empty response metadata.
Line range hint
272-282: LGTM: Consistent metadata handling in error casesThe changes in this segment consistently use
emptyResponseMetadata()for error cases, which aligns with the overall improvement in metadata handling throughout the file.
Line range hint
302-326: LGTM: Consistent metadata handling in checkDirectRelationThe changes in the
checkDirectRelationmethod consistently useemptyResponseMetadata(), which aligns with the overall improvement in metadata handling and maintains consistency throughout the file.
Line range hint
354-504: LGTM: Consistent metadata handling across multiple methodsThe changes in this segment demonstrate a consistent approach to metadata handling across multiple methods, including
checkTupleToUserSet,checkComputedUserSet, andcheckDirectAttribute. This improves the overall consistency and maintainability of the code.
Line range hint
536-631: LGTM: Consistent metadata handling in checkDirectCallThe changes in the
checkDirectCallmethod consistently useemptyResponseMetadata(), which aligns with the overall improvement in metadata handling and maintains consistency throughout the file.
Line range hint
639-911: LGTM: Consistent metadata handling and new emptyResponseMetadata functionThe changes in this final segment complete the consistent use of
emptyResponseMetadata()throughout the file. The addition of theemptyResponseMetadata()function at the end provides a centralized point for creating empty metadata, which improves maintainability and consistency.The new function is simple and correctly initializes the
CheckCountto 0:func emptyResponseMetadata() *base.PermissionCheckResponseMetadata { return &base.PermissionCheckResponseMetadata{ CheckCount: 0, } }This implementation is correct and aligns with the usage throughout the file.
Line range hint
1-911: Overall: Excellent improvement in metadata handlingThe changes made throughout this file significantly improve the consistency and maintainability of the code by introducing and consistently using the
emptyResponseMetadata()function. This refactoring provides a single point of control for creating empty metadata, which will make future modifications easier and reduce the likelihood of inconsistencies.Key improvements:
- Consistent metadata creation across all methods
- Centralized empty metadata creation with
emptyResponseMetadata()- Improved readability and maintainability
These changes align well with best practices for code organization and will make the codebase more robust and easier to maintain in the future.
internal/invoke/utils.go (2)
5-5: Importingsync/atomicfor atomic operationsThe inclusion of the
"sync/atomic"package is necessary to perform atomic operations on theDepthfield, ensuring thread safety in concurrent environments.
12-14: Using atomic operation to prevent data races onDepthSwitching to
atomic.LoadInt32(&request.GetMetadata().Depth)ensures that theDepthfield is read atomically. This prevents potential data races whenDepthmay be accessed or modified concurrently by multiple goroutines.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes