feat: bulk permission check api is added #1681
Conversation
WalkthroughThe changes introduce a new API endpoint Changes
Possibly related PRs
Suggested reviewers
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: 10
🧹 Outside diff range and nitpick comments (3)
integration-test/usecases/google_docs_test.go (1)
77-146: LGTM! The new test case is well-structured and consistent with existing tests.The new "Google Docs Sample: Bulk Checks" test case effectively tests the bulk permission check functionality. It maintains consistency with existing test patterns and provides good coverage of the new feature.
However, there are a few suggestions for improvement:
- Consider making the depth configurable or add a comment explaining the choice of 100:
Metadata: &base.PermissionCheckRequestMetadata{ SchemaVersion: initialGoogleDocsSchemaVersion, SnapToken: initialGoogleDocsSnapToken, - Depth: 100, + Depth: 100, // TODO: Explain why 100 or make configurable },
- Add separate error handling for the
BulkCheckcall:bulkRes, err := permissionClient.BulkCheck(ctx, &base.BulkPermissionCheckRequest{ TenantId: "google-docs", Checks: checks, }) -Expect(err).ShouldNot(HaveOccurred()) +Expect(err).ShouldNot(HaveOccurred(), "BulkCheck call failed: %v", err) +Expect(bulkRes).ShouldNot(BeNil(), "BulkCheck response is nil")
- Optimize the result validation loop for better efficiency:
-for idx, check := range scenario.Checks { - expectedResults := check.Assertions - for _, expected := range expectedResults { - exp := base.CheckResult_CHECK_RESULT_ALLOWED - if !expected { - exp = base.CheckResult_CHECK_RESULT_DENIED - } - - Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can) - } -} +for idx, result := range bulkRes.Results { + expectedResult := base.CheckResult_CHECK_RESULT_ALLOWED + if !scenario.Checks[idx].Assertions[result.Permission] { + expectedResult = base.CheckResult_CHECK_RESULT_DENIED + } + Expect(result.Can).Should(Equal(expectedResult), "Permission result mismatch for check %d, permission %s, expected %v, got %v", idx, result.Permission, expectedResult, result.Can) +}These changes will improve the test case's robustness and efficiency.
docs/api-reference/openapiv2/apidocs.swagger.json (1)
654-711: Endpoint structure looks good, but some improvements needed.The new bulk permission check endpoint is well-structured and consistent with other endpoints in the API. However, there are a few areas for improvement:
- The description could be more detailed to provide better context for API users.
- The code samples in the
x-codeSamplessection are currently empty. These should be filled with actual examples to help developers understand how to use the endpoint.- Consider adding more language examples to the
x-codeSamplessection for broader language support.To enhance the endpoint definition, consider applying these changes:
- Expand the description to include more details about the bulk check operation, its use cases, and any limitations.
- Fill in the code samples for Go, Node.js, and cURL to demonstrate how to use the endpoint.
- Add examples for other popular languages like Python, Java, or Ruby in the
x-codeSamplessection.internal/servers/permissionServer.go (1)
49-51: Add detailed documentation for theBulkCheckmethodThe
BulkCheckmethod currently has a brief comment. Consider adding more comprehensive documentation to explain the method's purpose, parameters, expected behavior, and return values. This will improve maintainability and help other developers understand the code better.Here's how you could enhance the documentation:
-// Bulk Check - Performs Bulk Authorization Checks +// BulkCheck performs bulk authorization checks for a list of permission checks. +// It validates the request, processes each permission check, and returns a bulk response containing the results. +// Parameters: +// - ctx: The context for controlling cancellation and timeouts. +// - request: The BulkPermissionCheckRequest containing tenant ID and a list of checks. +// Returns: +// - *v1.BulkPermissionCheckResponse: The response containing results for each permission check. +// - error: An error if the bulk check operation fails. func (r *PermissionServer) BulkCheck(ctx context.Context, request *v1.BulkPermissionCheckRequest) (*v1.BulkPermissionCheckResponse, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/pb/base/v1/service.pb.gois excluded by!**/*.pb.gopkg/pb/base/v1/service.pb.gw.gois excluded by!**/*.pb.gw.gopkg/pb/base/v1/service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
- docs/api-reference/apidocs.swagger.json (3 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (3 hunks)
- integration-test/usecases/facebook_groups_test.go (1 hunks)
- integration-test/usecases/google_docs_test.go (1 hunks)
- integration-test/usecases/notion_test.go (1 hunks)
- internal/servers/permissionServer.go (1 hunks)
- pkg/pb/base/v1/service.pb.validate.go (1 hunks)
- proto/base/v1/service.proto (4 hunks)
🧰 Additional context used
🔇 Additional comments (14)
docs/api-reference/apidocs.swagger.json (5)
654-711: New bulk check endpoint looks good!The new
/v1/tenants/{tenant_id}/permissions/bulk-checkendpoint is well-defined and consistent with existing API patterns. It correctly uses the POST method, includes appropriate parameters, and is properly tagged. The summary and description clearly explain its purpose.However, we should review the referenced request (
BulkCheckBody) and response (BulkPermissionCheckResponse) structures to ensure they are properly defined.
1540-1553: BulkCheckBody structure is well-definedThe
BulkCheckBodyrequest structure for the bulk check operation is properly defined. It contains a single required fieldcheckswhich is an array ofSinglePermissionCheckobjects, allowing for multiple permission checks in a single request. The structure and its description are clear and consistent with the API's existing patterns.
1554-1567: BulkPermissionCheckResponse structure is appropriateThe
BulkPermissionCheckResponsestructure for the bulk check operation response is well-defined. It contains a single fieldresultswhich is an array ofSinglePermissionCheckResponseobjects, allowing for multiple permission check results to be returned. The structure and its description are clear and consistent with the API's existing patterns.
3126-3184: SinglePermissionCheck and SinglePermissionCheckResponse structures are well-designedThe
SinglePermissionCheckandSinglePermissionCheckResponsestructures are properly defined and consistent with the existing API patterns. They include all necessary fields for performing and responding to individual permission checks within the bulk operation. The inclusion of anindexfield in both structures ensures that responses can be correctly matched to their corresponding requests.These structures effectively reuse the existing permission check logic while adapting it for bulk operations, promoting consistency and maintainability in the API.
Line range hint
654-3184: Summary: Bulk permission check feature is a valuable addition to the APIThe introduction of the bulk permission check endpoint (
/v1/tenants/{tenant_id}/permissions/bulk-check) and its associated structures (BulkCheckBody,BulkPermissionCheckResponse,SinglePermissionCheck, andSinglePermissionCheckResponse) is a well-implemented and valuable addition to the Permify API.These changes allow clients to perform multiple permission checks in a single request, which can significantly improve efficiency for applications that need to check multiple permissions simultaneously. The new structures and endpoint are consistent with existing API patterns and best practices, ensuring that the API remains cohesive and easy to use.
Overall, this feature enhances the API's functionality without disrupting its existing structure or usage patterns.
proto/base/v1/service.proto (5)
155-163: NewBulkCheckRPC method is correctly definedThe
BulkCheckmethod is properly added to thePermissionservice with the correct request and response types. The HTTP mapping aligns with the existing API design.
962-1010:SinglePermissionCheckmessage is well-definedThe
SinglePermissionCheckmessage correctly encapsulates the details required for individual permission checks within a bulk request. The fields are consistent with those inPermissionCheckRequest.
1013-1026:BulkPermissionCheckResponsemessage is correctly structuredThe
BulkPermissionCheckResponsemessage appropriately includes a list ofSinglePermissionCheckResponseresults corresponding to each permission check performed.
1029-1045:SinglePermissionCheckResponsemessage is correctly definedThe
SinglePermissionCheckResponseprovides the result of a single permission check within the bulk response, including the index to correlate with the corresponding request.
Line range hint
69-151: Updated OpenAPI examples enhance API documentationThe new examples in Go, JavaScript, and cURL for the
Checkmethod provide clearer guidance on how to use the API. The inclusion of therelationfield in the subject reflects changes in the request structure.pkg/pb/base/v1/service.pb.validate.go (4)
727-832: Validation methods forBulkPermissionCheckRequestare correctly implementedThe validation methods for
BulkPermissionCheckRequestcomprehensively check all required fields, enforce length and pattern constraints onTenantId, limit the number ofChecksto 100, and ensure eachSinglePermissionCheckis validated properly.
909-1025:SinglePermissionCheckvalidation methods are thorough and accurateThe validation methods for
SinglePermissionCheckappropriately validateIndex, ensure required fields (Metadata,Entity,Permission,Subject) are present, enforce length and pattern constraints onPermission, and validate embedded messages.
1229-1312: Implementation ofBulkPermissionCheckResponsevalidation is correctThe validation methods for
BulkPermissionCheckResponsecorrectly enforce the maximum number ofResultsand validate eachSinglePermissionCheckResponse.
1388-1457:SinglePermissionCheckResponsevalidation methods are properly implementedThe validation methods for
SinglePermissionCheckResponseensure thatIndexis non-negative and thatMetadatais valid.
internal/servers/permissionServer.go
Outdated
| response, err := r.invoker.Check(ctx, singleRequest) | ||
| if err != nil { | ||
| // Log and record the error for each failed check | ||
| span.RecordError(err) | ||
| span.SetStatus(otelCodes.Error, err.Error()) | ||
| slog.ErrorContext(ctx, err.Error()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include error details in the response for failed checks
When an error occurs during a permission check, the response sets Can to CHECK_RESULT_DENIED without providing error details. Including error information can help clients understand why a check failed and handle errors more effectively.
You could modify the error handling as follows:
// Perform the permission check
response, err := r.invoker.Check(ctx, singleRequest)
if err != nil {
// Log and record the error for each failed check
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
slog.ErrorContext(ctx, err.Error())
// Add the failure response with index
results[i] = &v1.SinglePermissionCheckResponse{
Can: v1.CheckResult_CHECK_RESULT_DENIED,
Metadata: &v1.PermissionCheckResponseMetadata{
+ CheckError: err.Error(), // Assuming you have a field to include error details
+ // Alternatively, add a separate field for error messages
},
Index: int32(i),
}
} else {
// Successful check response, attach the index
results[i] = &v1.SinglePermissionCheckResponse{
Can: response.Can,
Metadata: response.Metadata,
Index: int32(i),
}
}Make sure to update the SinglePermissionCheckResponse and PermissionCheckResponseMetadata structures to include an error field if they don't already have one.
Committable suggestion was skipped due to low confidence.
internal/servers/permissionServer.go
Outdated
| for i, check := range checks { | ||
| // Create individual PermissionCheckRequest for each item | ||
| singleRequest := &v1.PermissionCheckRequest{ | ||
| TenantId: request.TenantId, | ||
| Metadata: check.Metadata, | ||
| Entity: check.Entity, | ||
| Permission: check.Permission, | ||
| Subject: check.Subject, | ||
| Context: check.Context, | ||
| Arguments: check.Arguments, | ||
| } | ||
|
|
||
| // Perform the permission check | ||
| response, err := r.invoker.Check(ctx, singleRequest) | ||
| if err != nil { | ||
| // Log and record the error for each failed check | ||
| span.RecordError(err) | ||
| span.SetStatus(otelCodes.Error, err.Error()) | ||
| slog.ErrorContext(ctx, err.Error()) | ||
|
|
||
| // Add the failure response with index | ||
| results[i] = &v1.SinglePermissionCheckResponse{ | ||
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | ||
| Metadata: &v1.PermissionCheckResponseMetadata{}, | ||
| Index: int32(i), | ||
| } | ||
| } else { | ||
| // Successful check response, attach the index | ||
| results[i] = &v1.SinglePermissionCheckResponse{ | ||
| Can: response.Can, | ||
| Metadata: response.Metadata, | ||
| Index: int32(i), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle context cancellation to ensure timely termination
Currently, the loop does not check if the context has been canceled. If the client cancels the request or if a timeout occurs, the server might continue processing all checks unnecessarily. To handle this, check the context's cancellation status within the loop.
You can add a context check like this:
for i, check := range checks {
+ // Check if the context is canceled
+ select {
+ case <-ctx.Done():
+ // Context canceled, return the error
+ return nil, ctx.Err()
+ default:
+ // Continue processing
+ }
// Create individual PermissionCheckRequest for each item
singleRequest := &v1.PermissionCheckRequest{
// fields...
}
// Rest of the code...
}This ensures that the server responds promptly to context cancellations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, check := range checks { | |
| // Create individual PermissionCheckRequest for each item | |
| singleRequest := &v1.PermissionCheckRequest{ | |
| TenantId: request.TenantId, | |
| Metadata: check.Metadata, | |
| Entity: check.Entity, | |
| Permission: check.Permission, | |
| Subject: check.Subject, | |
| Context: check.Context, | |
| Arguments: check.Arguments, | |
| } | |
| // Perform the permission check | |
| response, err := r.invoker.Check(ctx, singleRequest) | |
| if err != nil { | |
| // Log and record the error for each failed check | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| slog.ErrorContext(ctx, err.Error()) | |
| // Add the failure response with index | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | |
| Metadata: &v1.PermissionCheckResponseMetadata{}, | |
| Index: int32(i), | |
| } | |
| } else { | |
| // Successful check response, attach the index | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: response.Can, | |
| Metadata: response.Metadata, | |
| Index: int32(i), | |
| } | |
| } | |
| } | |
| for i, check := range checks { | |
| // Check if the context is canceled | |
| select { | |
| case <-ctx.Done(): | |
| // Context canceled, return the error | |
| return nil, ctx.Err() | |
| default: | |
| // Continue processing | |
| } | |
| // Create individual PermissionCheckRequest for each item | |
| singleRequest := &v1.PermissionCheckRequest{ | |
| TenantId: request.TenantId, | |
| Metadata: check.Metadata, | |
| Entity: check.Entity, | |
| Permission: check.Permission, | |
| Subject: check.Subject, | |
| Context: check.Context, | |
| Arguments: check.Arguments, | |
| } | |
| // Perform the permission check | |
| response, err := r.invoker.Check(ctx, singleRequest) | |
| if err != nil { | |
| // Log and record the error for each failed check | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| slog.ErrorContext(ctx, err.Error()) | |
| // Add the failure response with index | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | |
| Metadata: &v1.PermissionCheckResponseMetadata{}, | |
| Index: int32(i), | |
| } | |
| } else { | |
| // Successful check response, attach the index | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: response.Can, | |
| Metadata: response.Metadata, | |
| Index: int32(i), | |
| } | |
| } | |
| } |
🛠️ Refactor suggestion
Consider processing checks concurrently to improve performance
The BulkCheck method processes permission checks sequentially in a loop. For a large number of checks, this could become a performance bottleneck. Since each permission check is independent, you could process them concurrently using goroutines and synchronization mechanisms to improve throughput.
Here's a possible refactor using goroutines and a sync.WaitGroup:
+import (
+ "sync"
+)
func (r *PermissionServer) BulkCheck(ctx context.Context, request *v1.BulkPermissionCheckRequest) (*v1.BulkPermissionCheckResponse, error) {
ctx, span := tracer.Start(ctx, "permissions.bulk-check")
defer span.End()
// Validate the incoming request
v := request.Validate()
if v != nil {
return nil, status.Error(GetStatus(v), v.Error())
}
checks := request.Checks
results := make([]*v1.SinglePermissionCheckResponse, len(checks))
+ var wg sync.WaitGroup
+ var mu sync.Mutex
+ wg.Add(len(checks))
for i, check := range checks {
+ go func(i int, check *v1.SinglePermissionCheck) {
+ defer wg.Done()
// Create individual PermissionCheckRequest for each item
singleRequest := &v1.PermissionCheckRequest{
TenantId: request.TenantId,
Metadata: check.Metadata,
Entity: check.Entity,
Permission: check.Permission,
Subject: check.Subject,
Context: check.Context,
Arguments: check.Arguments,
}
// Perform the permission check
response, err := r.invoker.Check(ctx, singleRequest)
if err != nil {
// Log and record the error for each failed check
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
slog.ErrorContext(ctx, err.Error())
// Add the failure response with index
+ mu.Lock()
results[i] = &v1.SinglePermissionCheckResponse{
Can: v1.CheckResult_CHECK_RESULT_DENIED,
Metadata: &v1.PermissionCheckResponseMetadata{},
Index: int32(i),
+ // Optionally include error details if needed
}
+ mu.Unlock()
} else {
// Successful check response, attach the index
+ mu.Lock()
results[i] = &v1.SinglePermissionCheckResponse{
Can: response.Can,
Metadata: response.Metadata,
Index: int32(i),
}
+ mu.Unlock()
}
+ }(i, check)
}
+ wg.Wait()
// Return the bulk response
return &v1.BulkPermissionCheckResponse{
Results: results,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, check := range checks { | |
| // Create individual PermissionCheckRequest for each item | |
| singleRequest := &v1.PermissionCheckRequest{ | |
| TenantId: request.TenantId, | |
| Metadata: check.Metadata, | |
| Entity: check.Entity, | |
| Permission: check.Permission, | |
| Subject: check.Subject, | |
| Context: check.Context, | |
| Arguments: check.Arguments, | |
| } | |
| // Perform the permission check | |
| response, err := r.invoker.Check(ctx, singleRequest) | |
| if err != nil { | |
| // Log and record the error for each failed check | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| slog.ErrorContext(ctx, err.Error()) | |
| // Add the failure response with index | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | |
| Metadata: &v1.PermissionCheckResponseMetadata{}, | |
| Index: int32(i), | |
| } | |
| } else { | |
| // Successful check response, attach the index | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: response.Can, | |
| Metadata: response.Metadata, | |
| Index: int32(i), | |
| } | |
| } | |
| } | |
| import ( | |
| "sync" | |
| ) | |
| func (r *PermissionServer) BulkCheck(ctx context.Context, request *v1.BulkPermissionCheckRequest) (*v1.BulkPermissionCheckResponse, error) { | |
| ctx, span := tracer.Start(ctx, "permissions.bulk-check") | |
| defer span.End() | |
| // Validate the incoming request | |
| v := request.Validate() | |
| if v != nil { | |
| return nil, status.Error(GetStatus(v), v.Error()) | |
| } | |
| checks := request.Checks | |
| results := make([]*v1.SinglePermissionCheckResponse, len(checks)) | |
| var wg sync.WaitGroup | |
| var mu sync.Mutex | |
| wg.Add(len(checks)) | |
| for i, check := range checks { | |
| go func(i int, check *v1.SinglePermissionCheck) { | |
| defer wg.Done() | |
| // Create individual PermissionCheckRequest for each item | |
| singleRequest := &v1.PermissionCheckRequest{ | |
| TenantId: request.TenantId, | |
| Metadata: check.Metadata, | |
| Entity: check.Entity, | |
| Permission: check.Permission, | |
| Subject: check.Subject, | |
| Context: check.Context, | |
| Arguments: check.Arguments, | |
| } | |
| // Perform the permission check | |
| response, err := r.invoker.Check(ctx, singleRequest) | |
| if err != nil { | |
| // Log and record the error for each failed check | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| slog.ErrorContext(ctx, err.Error()) | |
| // Add the failure response with index | |
| mu.Lock() | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | |
| Metadata: &v1.PermissionCheckResponseMetadata{}, | |
| Index: int32(i), | |
| // Optionally include error details if needed | |
| } | |
| mu.Unlock() | |
| } else { | |
| // Successful check response, attach the index | |
| mu.Lock() | |
| results[i] = &v1.SinglePermissionCheckResponse{ | |
| Can: response.Can, | |
| Metadata: response.Metadata, | |
| Index: int32(i), | |
| } | |
| mu.Unlock() | |
| } | |
| }(i, check) | |
| } | |
| wg.Wait() | |
| // Return the bulk response | |
| return &v1.BulkPermissionCheckResponse{ | |
| Results: results, | |
| }, nil | |
| } |
Ensure individual checks are properly validated
While the overall BulkPermissionCheckRequest is validated, individual SinglePermissionCheck items may have invalid data. To prevent potential runtime errors, consider adding validation for each SinglePermissionCheck within the loop.
You could incorporate validation like this:
for i, check := range checks {
+ // Validate the individual check
+ v := check.Validate()
+ if v != nil {
+ // Handle invalid check
+ results[i] = &v1.SinglePermissionCheckResponse{
+ Can: v1.CheckResult_CHECK_RESULT_DENIED,
+ Metadata: &v1.PermissionCheckResponseMetadata{},
+ Index: int32(i),
+ // Optionally include error details if needed
+ }
+ continue
+ }
// Create individual PermissionCheckRequest for each item
singleRequest := &v1.PermissionCheckRequest{
TenantId: request.TenantId,
Metadata: check.Metadata,
Entity: check.Entity,
Permission: check.Permission,
Subject: check.Subject,
Context: check.Context,
Arguments: check.Arguments,
}
// Rest of the code...
}Committable suggestion was skipped due to low confidence.
| for idx, check := range scenario.Checks { | ||
| expectedResults := check.Assertions | ||
| for _, expected := range expectedResults { | ||
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | ||
| if !expected { | ||
| exp = base.CheckResult_CHECK_RESULT_DENIED | ||
| } | ||
| Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can) | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct the indexing mismatch when validating bulk check results
There is a potential mismatch between the indices of the bulkRes.Results array and the expected results from scenario.Checks. Since multiple permission checks are appended to checks for each scenario (one for each permission in check.Assertions), the total number of results may exceed the number of scenarios.
Currently, the code uses idx from the outer loop to index bulkRes.Results[idx], which may not align correctly with the corresponding result. This can lead to incorrect validation of permission results.
Suggested Fix:
Introduce a separate index to accurately track the position in bulkRes.Results. Here's how you can modify the code:
// Iterate over results and validate them
+ resultIdx := 0
for _, check := range scenario.Checks {
- expectedResults := check.Assertions
- for _, expected := range expectedResults {
+ for permission, expected := range check.Assertions {
exp := base.CheckResult_CHECK_RESULT_ALLOWED
if !expected {
exp = base.CheckResult_CHECK_RESULT_DENIED
}
- Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can)
+ Expect(bulkRes.Results[resultIdx].Can).Should(Equal(exp), "Permission result mismatch at result index %d for permission '%s', expected %v, got %v", resultIdx, permission, exp, bulkRes.Results[resultIdx].Can)
+ resultIdx++
}
}Explanation:
- Initialize
resultIdxbefore the loops to keep track of the current index inbulkRes.Results. - Use
for permission, expected := range check.Assertionsto iterate over each permission and its expected result. - Replace
bulkRes.Results[idx]withbulkRes.Results[resultIdx]to correctly access the corresponding result. - Increment
resultIdxafter each expectation to move to the next result.
This ensures that each expected permission result is correctly matched with the corresponding result from the bulk check response.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for idx, check := range scenario.Checks { | |
| expectedResults := check.Assertions | |
| for _, expected := range expectedResults { | |
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | |
| if !expected { | |
| exp = base.CheckResult_CHECK_RESULT_DENIED | |
| } | |
| Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can) | |
| } | |
| } | |
| resultIdx := 0 | |
| for _, check := range scenario.Checks { | |
| for permission, expected := range check.Assertions { | |
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | |
| if !expected { | |
| exp = base.CheckResult_CHECK_RESULT_DENIED | |
| } | |
| Expect(bulkRes.Results[resultIdx].Can).Should(Equal(exp), "Permission result mismatch at result index %d for permission '%s', expected %v, got %v", resultIdx, permission, exp, bulkRes.Results[resultIdx].Can) | |
| resultIdx++ | |
| } | |
| } |
| It("Facebook Groups Sample: Bulk Checks", func() { | ||
| for _, scenario := range shapes.InitialFacebookGroupsShape.Scenarios { | ||
| var checks []*base.SinglePermissionCheck | ||
| for _, check := range scenario.Checks { | ||
|
|
||
| entity, err := tuple.E(check.Entity) | ||
| if err != nil { | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| } | ||
|
|
||
| ear, err := tuple.EAR(check.Subject) | ||
| if err != nil { | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| } | ||
|
|
||
| subject := &base.Subject{ | ||
| Type: ear.GetEntity().GetType(), | ||
| Id: ear.GetEntity().GetId(), | ||
| Relation: ear.GetRelation(), | ||
| } | ||
|
|
||
| var contextTuples []*base.Tuple | ||
|
|
||
| for _, t := range check.Context.Tuples { | ||
| tup, err := tuple.Tuple(t) | ||
| if err != nil { | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| } | ||
|
|
||
| contextTuples = append(contextTuples, tup) | ||
| } | ||
|
|
||
| for permission, _ := range check.Assertions { | ||
|
|
||
| checks = append(checks, &base.SinglePermissionCheck{ | ||
| Metadata: &base.PermissionCheckRequestMetadata{ | ||
| SchemaVersion: initialFacebookGroupsSchemaVersion, | ||
| SnapToken: initialFacebookGroupsSnapToken, | ||
| Depth: 100, | ||
| }, | ||
| Context: &base.Context{ | ||
| Tuples: contextTuples, | ||
| }, | ||
| Entity: entity, | ||
| Permission: permission, | ||
| Subject: subject, | ||
| }) | ||
| } | ||
| } | ||
| // Perform the bulk check | ||
| bulkRes, err := permissionClient.BulkCheck(ctx, &base.BulkPermissionCheckRequest{ | ||
| TenantId: "facebook-groups", | ||
| Checks: checks, | ||
| }) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| // Iterate over results and validate them | ||
| for idx, check := range scenario.Checks { | ||
| expectedResults := check.Assertions | ||
| for _, expected := range expectedResults { | ||
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | ||
| if !expected { | ||
| exp = base.CheckResult_CHECK_RESULT_DENIED | ||
| } | ||
| Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor common code into helper functions to reduce duplication
The "Bulk Checks" test case duplicates several blocks of code from the "Checks" test case, including processing entities, subjects, context tuples, and constructing permission checks. To improve maintainability and readability, consider extracting these repeated code segments into reusable helper functions.
| for idx, check := range scenario.Checks { | ||
| expectedResults := check.Assertions | ||
| for _, expected := range expectedResults { | ||
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | ||
| if !expected { | ||
| exp = base.CheckResult_CHECK_RESULT_DENIED | ||
| } | ||
| Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix result indexing during bulk check validation
The current validation loop assumes a one-to-one correspondence between scenario.Checks and bulkRes.Results, which is incorrect because multiple SinglePermissionChecks are added per check (one per permission). This can lead to mismatches when validating the results.
To fix this, introduce a separate index resultIdx to correctly iterate over bulkRes.Results and ensure each permission's result is validated against the expected value.
Apply this diff to fix the indexing:
+ resultIdx := 0
for idx, check := range scenario.Checks {
expectedResults := check.Assertions
- for _, expected := range expectedResults {
+ for permission, expected := range expectedResults {
exp := base.CheckResult_CHECK_RESULT_ALLOWED
if !expected {
exp = base.CheckResult_CHECK_RESULT_DENIED
}
- Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can)
+ Expect(bulkRes.Results[resultIdx].Can).Should(Equal(exp), "Permission result mismatch for check %d, permission %s, expected %v, got %v", idx, permission, exp, bulkRes.Results[resultIdx].Can)
+ resultIdx++
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for idx, check := range scenario.Checks { | |
| expectedResults := check.Assertions | |
| for _, expected := range expectedResults { | |
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | |
| if !expected { | |
| exp = base.CheckResult_CHECK_RESULT_DENIED | |
| } | |
| Expect(bulkRes.Results[idx].Can).Should(Equal(exp), "Permission result mismatch for check %d, expected %v, got %v", idx, exp, bulkRes.Results[idx].Can) | |
| } | |
| } | |
| resultIdx := 0 | |
| for idx, check := range scenario.Checks { | |
| expectedResults := check.Assertions | |
| for permission, expected := range expectedResults { | |
| exp := base.CheckResult_CHECK_RESULT_ALLOWED | |
| if !expected { | |
| exp = base.CheckResult_CHECK_RESULT_DENIED | |
| } | |
| Expect(bulkRes.Results[resultIdx].Can).Should(Equal(exp), "Permission result mismatch for check %d, permission %s, expected %v, got %v", idx, permission, exp, bulkRes.Results[resultIdx].Can) | |
| resultIdx++ | |
| } | |
| } |
| extensions: { | ||
| key: "x-codeSamples" | ||
| value: { | ||
| list_value: { | ||
| values: { | ||
| struct_value: { | ||
| fields: { | ||
| key: "lang" | ||
| value: {string_value: "go"} | ||
| } | ||
| fields: { | ||
| key: "label" | ||
| value: {string_value: "go"} | ||
| } | ||
| fields: { | ||
| key: "source" | ||
| value: {string_value:""} | ||
| } | ||
| } | ||
| } | ||
| values: { | ||
| struct_value: { | ||
| fields: { | ||
| key: "lang" | ||
| value: {string_value: "javascript"} | ||
| } | ||
| fields: { | ||
| key: "label" | ||
| value: {string_value: "node"} | ||
| } | ||
| fields: { | ||
| key: "source" | ||
| value: {string_value:""} | ||
| } | ||
| } |
There was a problem hiding this comment.
Add code samples to the OpenAPI annotations for the BulkCheck method
The OpenAPI annotations for the BulkCheck method include placeholders for code samples in Go, JavaScript, and cURL, but the source fields are empty. Providing code samples will enhance the API documentation and help users understand how to use the new endpoint.
| // PermissionCheck defines a single permission check within a bulk request. | ||
| message SinglePermissionCheck { | ||
| // The index of this permission check within a bulk request, required. | ||
| int32 index = 1 [ | ||
| json_name = "index", | ||
| (validate.rules).int32.gte = 0, | ||
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | ||
| description: "The index of this permission check in the bulk request. Required." | ||
| } | ||
| ]; | ||
|
|
||
| // Metadata associated with this request, required. | ||
| PermissionCheckRequestMetadata metadata = 2 [ | ||
| json_name = "metadata", | ||
| (validate.rules).message.required = true | ||
| ]; | ||
|
|
||
| // Entity on which the permission needs to be checked, required. | ||
| Entity entity = 3 [ | ||
| json_name = "entity", | ||
| (validate.rules).message.required = true, | ||
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {example: "\"repository:1\""} | ||
| ]; | ||
|
|
||
| // Name of the permission or relation, required, must start with a letter and can include alphanumeric and underscore, max 64 bytes. | ||
| string permission = 4 [ | ||
| json_name = "permission", | ||
| (validate.rules).string = { | ||
| pattern: "^[a-zA-Z_]{1,64}$" | ||
| max_bytes: 64 | ||
| ignore_empty: false | ||
| }, | ||
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {description: "The action the user wants to perform on the resource"} | ||
| ]; | ||
|
|
||
| // Subject for which the permission needs to be checked, required. | ||
| Subject subject = 5 [ | ||
| json_name = "subject", | ||
| (validate.rules).message.required = true | ||
| ]; | ||
|
|
||
| // Context associated with this request. | ||
| Context context = 6 [ | ||
| json_name = "context", | ||
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {description: "Contextual data that can be dynamically added to permission check requests. See details on [Contextual Data](../../operations/contextual-tuples)"} | ||
| ]; | ||
|
|
||
| // Additional arguments associated with this request. | ||
| repeated Argument arguments = 7 [json_name = "arguments"]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reusing existing message types to avoid duplication
The SinglePermissionCheck and SinglePermissionCheckResponse messages are structurally similar to the existing PermissionCheckRequest and PermissionCheckResponse messages. Consider reusing or extending these existing messages to reduce code duplication and maintain consistency.
Also applies to: 1029-1045
| // BulkPermissionCheckRequest is the request message for the BulkCheck method in the Permission service. | ||
| message BulkPermissionCheckRequest { | ||
| // Identifier of the tenant, required, and must match the pattern "[a-zA-Z0-9-,]+", max 64 bytes. | ||
| string tenant_id = 1 [ | ||
| json_name = "tenant_id", | ||
| (validate.rules).string = { | ||
| pattern: "^([a-zA-Z0-9_\\-@\\.:+]{1,128}|\\*)$" | ||
| max_bytes: 128 | ||
| ignore_empty: false | ||
| }, | ||
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | ||
| description: "Identifier of the tenant, if you are not using multi-tenancy (have only one tenant) use pre-inserted tenant <code>t1</code> for this field. Required, and must match the pattern \\“[a-zA-Z0-9-,]+\\“, max 64 bytes." | ||
| } | ||
| ]; | ||
|
|
||
| // A list of permission checks to perform, required. | ||
| repeated SinglePermissionCheck checks = 3 [ | ||
| json_name = "checks", | ||
| (validate.rules).repeated = { | ||
| min_items: 0 | ||
| max_items: 100 | ||
| items: { | ||
| message: {required: true} | ||
| } | ||
| } | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Enforce at least one permission check in BulkPermissionCheckRequest
The checks field allows for zero items due to min_items: 0 in the validation rules. Since the checks field is required and the purpose of the request is to perform permission checks, consider setting min_items to 1 to ensure that at least one permission check is provided.
Apply this diff to fix the validation rule:
(validate.rules).repeated = {
- min_items: 0
+ min_items: 1
max_items: 100
items: {
message: {required: true}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // BulkPermissionCheckRequest is the request message for the BulkCheck method in the Permission service. | |
| message BulkPermissionCheckRequest { | |
| // Identifier of the tenant, required, and must match the pattern "[a-zA-Z0-9-,]+", max 64 bytes. | |
| string tenant_id = 1 [ | |
| json_name = "tenant_id", | |
| (validate.rules).string = { | |
| pattern: "^([a-zA-Z0-9_\\-@\\.:+]{1,128}|\\*)$" | |
| max_bytes: 128 | |
| ignore_empty: false | |
| }, | |
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | |
| description: "Identifier of the tenant, if you are not using multi-tenancy (have only one tenant) use pre-inserted tenant <code>t1</code> for this field. Required, and must match the pattern \\“[a-zA-Z0-9-,]+\\“, max 64 bytes." | |
| } | |
| ]; | |
| // A list of permission checks to perform, required. | |
| repeated SinglePermissionCheck checks = 3 [ | |
| json_name = "checks", | |
| (validate.rules).repeated = { | |
| min_items: 0 | |
| max_items: 100 | |
| items: { | |
| message: {required: true} | |
| } | |
| } | |
| ]; | |
| } | |
| // BulkPermissionCheckRequest is the request message for the BulkCheck method in the Permission service. | |
| message BulkPermissionCheckRequest { | |
| // Identifier of the tenant, required, and must match the pattern "[a-zA-Z0-9-,]+", max 64 bytes. | |
| string tenant_id = 1 [ | |
| json_name = "tenant_id", | |
| (validate.rules).string = { | |
| pattern: "^([a-zA-Z0-9_\\-@\\.:+]{1,128}|\\*)$" | |
| max_bytes: 128 | |
| ignore_empty: false | |
| }, | |
| (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { | |
| description: "Identifier of the tenant, if you are not using multi-tenancy (have only one tenant) use pre-inserted tenant <code>t1</code> for this field. Required, and must match the pattern \\"[a-zA-Z0-9-,]+\\", max 64 bytes." | |
| } | |
| ]; | |
| // A list of permission checks to perform, required. | |
| repeated SinglePermissionCheck checks = 3 [ | |
| json_name = "checks", | |
| (validate.rules).repeated = { | |
| min_items: 1 | |
| max_items: 100 | |
| items: { | |
| message: {required: true} | |
| } | |
| } | |
| ]; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (14)
internal/engines/balancer/balancer.go (2)
36-39: Add documentation and implement the BulkCheck method
- Consider adding documentation for the
BulkCheckmethod to explain its purpose and expected behavior.- The current implementation with
panic("unimplemented")is a placeholder. Remember to implement this method to handle bulk permission checks.- When implementing, consider how this method will interact with other parts of the
Balancerstruct, such asschemaReader,checker, andclient.Here's a suggested structure for the method with documentation:
// BulkCheck performs multiple permission checks in a single call. // It implements the invoke.Check interface for bulk operations. func (c *Balancer) BulkCheck(ctx context.Context, request *base.BulkPermissionCheckRequest) (*base.BulkPermissionCheckResponse, error) { // TODO: Implement bulk permission checking logic // Consider using c.schemaReader for entity definitions // and c.client for distributed checks if necessary return nil, fmt.Errorf("BulkCheck not yet implemented") }
36-39: Consider performance and potential refactoring for BulkCheck implementationWhen implementing the
BulkCheckmethod, consider the following:
- Performance implications of handling multiple permission checks in a single call. You may need to optimize database queries or use batch processing techniques.
- Potential for refactoring common logic between
CheckandBulkCheckmethods to avoid code duplication.- Error handling strategy for cases where some checks succeed while others fail within the same bulk request.
Consider creating helper functions for common operations that both
CheckandBulkCheckcan use. For example:func (c *Balancer) performSingleCheck(ctx context.Context, request *base.PermissionCheckRequest) (*base.PermissionCheckResponse, error) { // Extract common logic from the existing Check method } func (c *Balancer) BulkCheck(ctx context.Context, request *base.BulkPermissionCheckRequest) (*base.BulkPermissionCheckResponse, error) { responses := make([]*base.SinglePermissionCheckResponse, len(request.Checks)) for i, check := range request.Checks { // Use the common logic for each individual check response, err := c.performSingleCheck(ctx, check) // Handle the response and error appropriately } // Combine individual responses into a BulkPermissionCheckResponse }internal/engines/cache/cache.go (2)
38-41: Extend caching strategy and telemetry for bulk operations.While adding the
BulkCheckmethod is a good start, consider the following improvements to fully support bulk operations:
Extend the caching strategy to handle bulk requests efficiently. This might involve creating a new method for bulk cache operations or adapting the existing
getCheckKeyandsetCheckKeymethods.Add new telemetry metrics for bulk operations. For example:
- Bulk cache hit rate
- Bulk operation duration
- Number of permissions checked in bulk operations
Update the
NewCheckEngineWithCachefunction to initialize any new fields required for bulk operations.Here's an example of how you might add new telemetry metrics:
type CheckEngineWithCache struct { // ... existing fields ... bulkCacheCounter api.Int64Counter bulkCacheHitDurationHistogram api.Int64Histogram bulkOperationDurationHistogram api.Int64Histogram } func NewCheckEngineWithCache( // ... existing parameters ... ) invoke.Check { return &CheckEngineWithCache{ // ... existing initializations ... bulkCacheCounter: telemetry.NewCounter(meter, "bulk_cache_check_count", "Number of bulk permission cached checks performed"), bulkCacheHitDurationHistogram: telemetry.NewHistogram(meter, "bulk_cache_hit_duration", "microseconds", "Duration of bulk cache hits in microseconds"), bulkOperationDurationHistogram: telemetry.NewHistogram(meter, "bulk_operation_duration", "microseconds", "Duration of bulk operations in microseconds"), } }Remember to use these new metrics in your
BulkCheckimplementation.
Line range hint
1-41: Summary: Bulk check implementation needs completion and integrationThe addition of the
BulkCheckmethod is a good start towards implementing the bulk permission check API. However, there are several areas that need attention to fully realize this feature:
- The
BulkCheckmethod needs to be implemented, leveraging the existingCheckmethod and caching infrastructure.- The caching strategy should be extended or adapted to efficiently handle bulk operations.
- Telemetry and metrics should be added for bulk operations to maintain observability.
- The
NewCheckEngineWithCachefunction may need updates to initialize new fields related to bulk operations.Next steps:
- Implement the
BulkCheckmethod with proper error handling and response formatting.- Extend the caching strategy for bulk operations.
- Add telemetry for bulk operations.
- Update tests to cover the new bulk check functionality.
- Ensure that the implementation aligns with the PR objectives of adding a bulk permission check API.
Consider creating a separate
BulkCheckEngineWithCachestruct if the bulk check logic becomes complex, to maintain separation of concerns and keep the codebase clean.internal/engines/lookup.go (4)
64-70: LGTM! Consider adding a comment for clarity.The changes to the callback function improve flexibility and prepare for bulk operations. The use of
BulkEntityCallbackParamsis a good approach for structured parameter passing.Consider adding a brief comment explaining the purpose of the
BulkEntityCallbackParamsstruct for better code readability:callback := func(params ...interface{}) { mu.Lock() // Safeguard access to the shared slice with a mutex defer mu.Unlock() // Ensure the lock is released after appending the ID + // Extract entity information from BulkEntityCallbackParams if resp, ok := params[0].(*BulkEntityCallbackParams); ok { entityIDs = append(entityIDs, resp.entityID) ct = resp.token } }
133-143: LGTM! Consider improving error handling.The changes to the callback function are consistent with the
LookupEntitymethod and maintain the existing streaming functionality while adapting to the new parameter structure.Consider improving the error handling by logging the error before returning:
callback := func(params ...interface{}) { if resp, ok := params[0].(*BulkEntityCallbackParams); ok { err := server.Send(&base.PermissionLookupEntityStreamResponse{ EntityId: resp.entityID, ContinuousToken: resp.token, }) // If there is an error in sending the response, the function will return if err != nil { + // Log the error for debugging purposes + log.Printf("Error sending entity stream response: %v", err) return } } }
205-212: LGTM! Consider extracting the callback logic.The changes to the callback function are consistent with the other methods and maintain the existing functionality while adapting to the new parameter structure.
Consider extracting the callback logic into a separate function for better code organization and reusability:
+func appendSubjectID(subjectIDs *[]string, continuousToken *string, params ...interface{}) { + if resp, ok := params[0].(*BulkSubjectCallbackParams); ok { + *subjectIDs = append(*subjectIDs, resp.subjectID) + *continuousToken = resp.token + } +} // Callback function to handle the results of permission checks. // If an entity passes the permission check, its ID is stored in the subjectIDs slice. callback := func(params ...interface{}) { mu.Lock() // Lock to prevent concurrent modification of the slice. defer mu.Unlock() // Unlock after the ID is appended. - - if resp, ok := params[0].(*BulkSubjectCallbackParams); ok { - subjectIDs = append(subjectIDs, resp.subjectID) - ct = resp.token - } + appendSubjectID(&subjectIDs, &ct, params...) }This refactoring improves code readability and makes it easier to reuse the logic if needed elsewhere in the codebase.
Line range hint
1-300: Overall, the changes look good and prepare the codebase for bulk operations.The modifications to
LookupEntity,LookupEntityStream, andLookupSubjectmethods are consistent and well-implemented. The introduction ofBulkEntityCallbackParamsandBulkSubjectCallbackParamsfor structured parameter passing is a good approach that improves code flexibility while maintaining existing functionality.Some minor suggestions for improvement have been made, including:
- Adding comments for clarity
- Improving error handling
- Extracting callback logic for better code organization
These changes align well with the PR objective of introducing a bulk permission check API. The modifications to these core methods lay the groundwork for efficient bulk operations.
As you continue to develop this feature, consider the following architectural points:
- Ensure that the bulk operations maintain good performance as the number of entities/subjects increases.
- Consider implementing rate limiting or pagination for very large bulk requests to prevent system overload.
- Document the new bulk operation capabilities in the API documentation to guide users on efficient usage.
internal/engines/check_test.go (2)
698-812: LGTM! Consider adding a comment explaining the bulk check process.The new test case "Github Sample: Case 4" is well-implemented and consistent with the existing test structure. It effectively tests the bulk permission check functionality, which aligns with the PR objectives.
Consider adding a brief comment before line 767 explaining the purpose of the bulk permission check and how it differs from individual checks. This would enhance code readability and make the test's intention clearer.
Line range hint
1499-1894: LGTM! Minor inconsistency in test case naming.The new test case "Exclusion Sample: Case 7" is well-implemented and consistent with the existing test structure. It effectively tests the bulk permission check functionality for the polymorphic relations scenario.
There's a minor inconsistency in the test case naming. The test is called "Exclusion Sample: Case 7" but it's within the "Polymorphic Relations Sample" context. Consider renaming it to "Polymorphic Relations Sample: Case 2" for consistency.
Similar to the previous suggestion, consider adding a brief comment before line 1572 explaining the purpose of the bulk permission check and how it differs from individual checks. This would enhance code readability and make the test's intention clearer.
internal/engines/bulk.go (2)
258-264: Potential deadlock due to mutex lockingIn the goroutine within
ExecuteRequests, the mutexmuis locked before callingbc.processLookupResults(...). IfprocessLookupResultsblocks or takes significant time, it could cause other goroutines to wait, reducing concurrency and performance.Consider minimizing the scope of the mutex lock or redesigning
processLookupResultsto avoid holding the lock during lengthy operations.
385-385: Remove or implement the emptyProcessResultmethodThe
ProcessResultmethod inBulkCheckPublisheris empty. If it's not needed, consider removing it to keep the codebase clean. If it's intended for future use, add a comment explaining its purpose.internal/invoke/invoke.go (1)
235-235: Update comment to match the metric being incrementedThe comment mentions increasing the lookup entity count, but the code increments
checkCounter, which tracks permission checks. Update the comment to accurately describe the operation.Apply this diff to fix the comment:
- // Increase the lookup entity count in the metrics. + // Increase the check count in the metrics.internal/engines/check.go (1)
907-943: Add unit tests for the newBulkCheckmethodThe
BulkCheckmethod introduces significant new functionality to theCheckEngine. To ensure reliability and prevent future regressions, it's important to include unit tests that cover:
- Multiple permission checks with varying outcomes
- Error handling when permissions checks fail
- Concurrency aspects to ensure thread safety
- Edge cases such as empty requests or invalid inputs
Would you like assistance in drafting unit tests for the
BulkCheckmethod or should we open a new GitHub issue to track the addition of these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- integration-test/specific/health_test.go (1 hunks)
- integration-test/usecases/usecase_test.go (1 hunks)
- internal/engines/balancer/balancer.go (1 hunks)
- internal/engines/bulk.go (7 hunks)
- internal/engines/cache/cache.go (1 hunks)
- internal/engines/check.go (2 hunks)
- internal/engines/check_test.go (3 hunks)
- internal/engines/lookup.go (3 hunks)
- internal/invoke/invoke.go (3 hunks)
- internal/servers/permissionServer.go (1 hunks)
- internal/storage/memory/schemaReader.go (0 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/memory/schemaReader.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/servers/permissionServer.go
🧰 Additional context used
🔇 Additional comments (5)
integration-test/specific/health_test.go (1)
17-17: Clarify the reason for changing the gRPC server addressThe gRPC server address has been changed from "permify:3478" to "localhost:3478". While this change might improve portability, it raises a few concerns:
- This modification doesn't seem directly related to the PR's main objective of adding a bulk permission check API. Could you explain the reasoning behind this change?
- Using "localhost" assumes the server will always run on the same machine as the test. Is this assumption valid across all your test environments, including CI/CD pipelines?
- Is this change consistent with other tests in the codebase? If not, should we update them as well?
Consider adding a comment explaining the rationale behind this change for future maintainers.
To ensure consistency across the codebase, let's check for other occurrences of gRPC server addresses:
✅ Verification successful
<!-- [monologue] [task review_file_1/1] Review file `integration-test/specific/health_test.go`. [observations] - The shell script found two instances where "localhost:3478" is used in `grpc.DialContext` within Go test files: 1. `integration-test/usecases/usecase_test.go` 2. `integration-test/specific/health_test.go` - Both instances use "localhost:3478", indicating consistency in the use of the gRPC server address across these tests. [analysis] - The change in `health_test.go` aligns with the usage in `usecase_test.go`, suggesting a standardized approach to specifying the gRPC server address as "localhost:3478" in integration tests. - This consistency addresses the concern about whether the change is mirrored in other parts of the codebase. - No further occurrences of different server addresses were found, which supports the decision to use "localhost" for portability. [actions] - Since the change is consistent with existing tests, the verification is successful. - No additional shell scripts are necessary at this point. -->The change to use "localhost:3478" is consistent with other tests in the codebase, ensuring standardized server addresses across integration tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for gRPC server addresses in Go files rg --type go 'grpc\.Dial.*:3478'Length of output: 359
internal/engines/balancer/balancer.go (1)
36-39: Verify integration of BulkCheck with existing codebaseAs you implement the
BulkCheckmethod, ensure proper integration with the existing codebase:
- Verify that the
NewCheckEngineWithBalancerfunction provides all necessary dependencies for bulk operations.- Check if any changes are needed in the
base.PermissionClientinterface or its implementation to support bulk operations.- Consider updating any relevant tests or adding new ones to cover the bulk check functionality.
To help verify the integration, you can run the following commands:
These commands will help identify any existing bulk operation methods and related tests, which can guide your implementation and integration of the
BulkCheckmethod.internal/engines/check_test.go (1)
Line range hint
1-2447: Overall, excellent addition of bulk permission check tests.The new test cases "Github Sample: Case 4" and "Exclusion Sample: Case 7" (which should be renamed to "Polymorphic Relations Sample: Case 2") are well-implemented and effectively test the new bulk permission check functionality. These additions align perfectly with the PR objectives of introducing a bulk permission check API.
The implementation is consistent across both new test cases and follows the established patterns in the existing test suite. This ensures that the new functionality is thoroughly tested in different scenarios (Github-like and Polymorphic relations).
These changes significantly enhance the test coverage for the new bulk permission check feature, which will help ensure its reliability and correctness.
internal/engines/bulk.go (1)
194-194:⚠️ Potential issueCorrect pointer increment syntax
The expression
*processedIndex++is invalid in Go. Incrementing the value pointed to by a pointer requires parentheses around the dereferenced pointer.Apply this fix:
-*processedIndex++ // Move to the next index for processing +(*processedIndex)++ // Move to the next index for processingThis change ensures proper syntax and increments the value correctly.
Likely invalid or redundant comment.
internal/engines/check.go (1)
931-931: Verify the argument passed topublisher.PublishIn
publisher.Publish(base.CheckResult_CHECK_RESULT_UNSPECIFIED), the argumentbase.CheckResult_CHECK_RESULT_UNSPECIFIEDis being passed. Please verify if this is the correct or intended value. Using an unspecified check result might lead to unintended behavior.To confirm the correct usage, you can inspect the
Publishmethod's expected arguments and check wherepublisher.Publishis used elsewhere in the codebase.
| @@ -49,7 +49,7 @@ var _ = BeforeSuite(func() { | |||
|
|
|||
| var err error | |||
| // Set up a connection to the server. | |||
| conn, err = grpc.DialContext(ctx, "permify:3478", grpc.WithTransportCredentials(insecure.NewCredentials())) | |||
| conn, err = grpc.DialContext(ctx, "localhost:3478", grpc.WithTransportCredentials(insecure.NewCredentials())) | |||
There was a problem hiding this comment.
💡 Codebase verification
Refactor Hardcoded Server Addresses in Tests
Multiple test files contain hardcoded server addresses, which can hinder flexibility and maintenance:
integration-test/specific/health_test.go:17integration-test/usecases/usecase_test.go:52internal/authn/oidc/authn_test.go:23internal/config/config_test.go:101internal/config/config_test.go:129
Recommendations:
-
Centralize Configuration:
- Introduce environment variables for all server addresses used in tests.
- Example implementation:
serverAddr := os.Getenv("PERMIFY_SERVER_ADDR") if serverAddr == "" { serverAddr = "localhost:3478" // Default value } conn, err := grpc.DialContext(ctx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))
-
Update All Test Files:
- Replace hardcoded addresses with the centralized configuration approach.
-
Ensure Consistency:
- Verify that all tests reference the environment variables or configuration files appropriately.
-
Validate in CI/CD:
- Ensure that the CI/CD pipeline sets the necessary environment variables to maintain test integrity across environments.
Implementing these changes will enhance the test suite's flexibility and ease of maintenance.
🔗 Analysis chain
Approve with suggestions: Consider using an environment variable for the server address.
The change from "permify:3478" to "localhost:3478" is noted. This modification might simplify local development and testing. However, it could potentially impact the portability of the tests across different environments.
To improve flexibility and maintain compatibility with various setups:
-
Consider using an environment variable for the server address. This allows easy configuration for different environments without code changes.
-
Verify that this change doesn't break the tests in your CI/CD pipeline or other environments where the server might not be on localhost.
Here's a suggested implementation using an environment variable:
- conn, err = grpc.DialContext(ctx, "localhost:3478", grpc.WithTransportCredentials(insecure.NewCredentials()))
+ serverAddr := os.Getenv("PERMIFY_SERVER_ADDR")
+ if serverAddr == "" {
+ serverAddr = "localhost:3478" // Default to localhost if not set
+ }
+ conn, err = grpc.DialContext(ctx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))To ensure this change doesn't introduce issues in other environments, please run the following verification:
This will help identify any other occurrences of hardcoded server addresses that might need to be updated for consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded server addresses in test files
echo "Checking for hardcoded server addresses in test files:"
rg --type go -n '(localhost|permify):[0-9]+' test
Length of output: 215
Script:
#!/bin/bash
# Description: Recursively search for hardcoded server addresses in Go test files
echo "Checking for hardcoded server addresses in Go test files:"
rg --type go --files | grep '_test.go$' | xargs rg -n '(localhost|permify):[0-9]+'
Length of output: 805
| // BulkCheck implements invoke.Check. | ||
| func (c *CheckEngineWithCache) BulkCheck(ctx context.Context, request *base.BulkPermissionCheckRequest) (response *base.BulkPermissionCheckResponse, err error) { | ||
| panic("unimplemented") | ||
| } |
There was a problem hiding this comment.
Implement the BulkCheck method to fulfill the PR objectives.
The BulkCheck method is currently unimplemented, which doesn't align with the PR objectives of adding a bulk permission check API. This could lead to runtime errors if the method is called.
To address this:
- Implement the bulk check logic, possibly leveraging the existing
Checkmethod for individual checks. - Consider caching strategies for bulk checks to optimize performance.
- Ensure error handling and proper response formatting.
- Add appropriate logging and telemetry.
Here's a skeleton to get started:
func (c *CheckEngineWithCache) BulkCheck(ctx context.Context, request *base.BulkPermissionCheckRequest) (response *base.BulkPermissionCheckResponse, err error) {
response = &base.BulkPermissionCheckResponse{
Results: make([]*base.PermissionCheckResponse, len(request.GetRequests())),
}
for i, singleRequest := range request.GetRequests() {
// Perform individual check, possibly using c.Check method
singleResponse, err := c.Check(ctx, singleRequest)
if err != nil {
// Handle error (you might want to continue checking other permissions)
response.Results[i] = &base.PermissionCheckResponse{
Can: base.CheckResult_CHECK_RESULT_DENIED,
Metadata: &base.PermissionCheckResponseMetadata{
CheckCount: 0,
},
}
} else {
response.Results[i] = singleResponse
}
}
return response, nil
}Consider optimizing this implementation for bulk operations, possibly by batching database queries or implementing a bulk caching strategy.
| @@ -54,7 +70,8 @@ type BulkChecker struct { | |||
|
|
|||
| // callback is a function that handles the result of each permission check. | |||
| // It is called with the entity ID and the result of the permission check (e.g., allowed or denied). | |||
| callback func(entityID, ct string) | |||
| // callback func(entityID, ct string) | |||
| callback func(params ...interface{}) | |||
There was a problem hiding this comment.
Avoid using ...interface{} in function signatures
Changing the callback field to func(params ...interface{}) reduces type safety and makes the code harder to understand and maintain. It's better to use explicit types in function signatures to leverage compile-time checks and improve code readability.
Consider defining a specific callback type based on the BulkCheckerType. For example:
// callback is a function that handles the result of each permission check.
// It is called with the entity ID and the result of the permission check (e.g., allowed or denied).
-// callback func(entityID, ct string)
-callback func(params ...interface{})
+callback func(params BulkCallbackParams)And define a dedicated interface for BulkCallbackParams:
type BulkCallbackParams interface{}Then, create concrete types that implement this interface for each check type.
| func (s *BulkCheckPublisher) Publish(result base.CheckResult) { | ||
| // Loop through all mapping requests inside s.request.Requests | ||
| for _, req := range s.request.Checks { | ||
| s.bulkChecker.RequestChan <- BulkCheckerRequest{ | ||
| Request: &base.PermissionCheckRequest{ | ||
| TenantId: s.request.GetTenantId(), // Manually set tenantID from the main request | ||
| Metadata: &base.PermissionCheckRequestMetadata{ | ||
| SnapToken: req.GetMetadata().GetSnapToken(), | ||
| SchemaVersion: req.GetMetadata().GetSchemaVersion(), | ||
| Depth: req.GetMetadata().GetDepth(), | ||
| }, | ||
| Entity: req.GetEntity(), // Use the specific request's entity | ||
| Permission: req.GetPermission(), // Use the specific request's permission | ||
| Subject: req.GetSubject(), // The subject passed into the function | ||
| Context: req.GetContext(), // The context passed into the function | ||
| }, | ||
| Result: result, // Pass the result into the bulk checker | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure correct association of results with requests in Publish
In BulkCheckPublisher.Publish, the same result is applied to all permission check requests, which may not be accurate if each check should have its own result.
Modify the Publish method to accept a slice of results corresponding to each check:
-func (s *BulkCheckPublisher) Publish(result base.CheckResult) {
+func (s *BulkCheckPublisher) Publish(results []base.CheckResult) {Then, iterate over both s.request.Checks and results:
for i, req := range s.request.Checks {
s.bulkChecker.RequestChan <- BulkCheckerRequest{
Request: &base.PermissionCheckRequest{
TenantId: s.request.GetTenantId(),
Metadata: req.GetMetadata(),
Entity: req.GetEntity(),
Permission: req.GetPermission(),
Subject: req.GetSubject(),
Context: req.GetContext(),
},
Result: results[i],
}
}This ensures each request is associated with its correct result.
| @@ -76,7 +93,7 @@ type BulkChecker struct { | |||
| // engine: the CheckEngine to use for permission checks | |||
| // callback: a callback function that handles the result of each permission check | |||
| // concurrencyLimit: the maximum number of concurrent permission checks | |||
| func NewBulkChecker(ctx context.Context, checker invoke.Check, typ BulkCheckerType, callback func(entityID, ct string), concurrencyLimit int) *BulkChecker { | |||
| func NewBulkChecker(ctx context.Context, checker invoke.Check, typ BulkCheckerType, callback func(params ...interface{}), concurrencyLimit int) *BulkChecker { | |||
There was a problem hiding this comment.
Use explicit callback types in NewBulkChecker
In the NewBulkChecker function, the callback parameter uses func(params ...interface{}), which lacks type safety. This can lead to runtime errors and makes it difficult for others to understand what parameters are expected.
Modify the function signature to use explicit types:
-func NewBulkChecker(ctx context.Context, checker invoke.Check, typ BulkCheckerType, callback func(params ...interface{}), concurrencyLimit int) *BulkChecker {
+func NewBulkChecker(ctx context.Context, checker invoke.Check, typ BulkCheckerType, callback func(params BulkCallbackParams), concurrencyLimit int) *BulkChecker {This change enhances type safety and clarifies the expected callback parameter.
Committable suggestion was skipped due to low confidence.
|
|
||
| var schemaVersion string | ||
| schemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.GetTenantId()) | ||
| fmt.Println(invoker.schemaReader.ReadSchema(ctx, request.GetTenantId(), schemaVersion)) |
There was a problem hiding this comment.
Avoid using fmt.Println for logging in production code
Using fmt.Println directly outputs to standard output, which is not suitable for production environments. It would be better to use a structured logging framework or remove this statement if it was intended for debugging purposes.
Apply this diff to remove the debug print statement:
- fmt.Println(invoker.schemaReader.ReadSchema(ctx, request.GetTenantId(), schemaVersion))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Println(invoker.schemaReader.ReadSchema(ctx, request.GetTenantId(), schemaVersion)) |
| } | ||
| } | ||
|
|
||
| fmt.Println(request.GetChecks()) |
There was a problem hiding this comment.
Avoid using fmt.Println for logging in production code
Directly printing to standard output using fmt.Println is generally discouraged in production code. Consider removing this statement or replacing it with appropriate logging.
Apply this diff to remove the debug print statement:
- fmt.Println(request.GetChecks())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Println(request.GetChecks()) |
| @@ -2,6 +2,7 @@ package invoke | |||
|
|
|||
| import ( | |||
| "context" | |||
| "fmt" | |||
There was a problem hiding this comment.
Remove unused import of fmt package
After removing the fmt.Println statements, the fmt package import is no longer needed and can be safely removed to clean up the imports.
Apply this diff to remove the unused import:
import (
"context"
- "fmt"
"time"Committable suggestion was skipped due to low confidence.
| resp, err := invoker.cc.BulkCheck(ctx, request) | ||
|
|
||
| duration := time.Since(start) | ||
| invoker.lookupEntityDurationHistogram.Record(ctx, duration.Microseconds()) |
There was a problem hiding this comment.
Use the correct histogram for recording BulkCheck duration
The duration is currently being recorded using invoker.lookupEntityDurationHistogram, which is intended for lookup entity operations. It should be recorded using invoker.checkDurationHistogram to accurately reflect BulkCheck operations.
Apply this diff to correct the histogram:
- invoker.lookupEntityDurationHistogram.Record(ctx, duration.Microseconds())
+ invoker.checkDurationHistogram.Record(ctx, duration.Microseconds())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| invoker.lookupEntityDurationHistogram.Record(ctx, duration.Microseconds()) | |
| invoker.checkDurationHistogram.Record(ctx, duration.Microseconds()) |
| callback := func(params ...interface{}) { | ||
| mu.Lock() // Safeguard access to the shared slice with a mutex | ||
| defer mu.Unlock() // Ensure the lock is released after appending the ID | ||
| if resp, ok := params[0].(*BulkCheckCallbackParams); ok { | ||
| results = append(results, &base.SinglePermissionCheckResponse{ | ||
| Index: int32(resp.index), | ||
| Can: resp.result, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure callback parameters are checked for length and type
In the callback function, there is an assumption that params[0] exists and is of type *BulkCheckCallbackParams. This can lead to a panic if params is empty or contains a different type. It's important to check the length of params and confirm the type before using it to prevent runtime errors.
Here's a suggested fix:
callback := func(params ...interface{}) {
mu.Lock() // Safeguard access to the shared slice with a mutex
defer mu.Unlock() // Ensure the lock is released after appending the ID
+ if len(params) == 0 {
+ // Handle the case when no parameters are passed
+ return
+ }
if resp, ok := params[0].(*BulkCheckCallbackParams); ok {
results = append(results, &base.SinglePermissionCheckResponse{
Index: int32(resp.index),
Can: resp.result,
})
+ } else {
+ // Handle the case when the parameter is of unexpected type
+ // Possibly log an error or return
+ return
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| callback := func(params ...interface{}) { | |
| mu.Lock() // Safeguard access to the shared slice with a mutex | |
| defer mu.Unlock() // Ensure the lock is released after appending the ID | |
| if resp, ok := params[0].(*BulkCheckCallbackParams); ok { | |
| results = append(results, &base.SinglePermissionCheckResponse{ | |
| Index: int32(resp.index), | |
| Can: resp.result, | |
| }) | |
| } | |
| } | |
| callback := func(params ...interface{}) { | |
| mu.Lock() // Safeguard access to the shared slice with a mutex | |
| defer mu.Unlock() // Ensure the lock is released after appending the ID | |
| if len(params) == 0 { | |
| // Handle the case when no parameters are passed | |
| return | |
| } | |
| if resp, ok := params[0].(*BulkCheckCallbackParams); ok { | |
| results = append(results, &base.SinglePermissionCheckResponse{ | |
| Index: int32(resp.index), | |
| Can: resp.result, | |
| }) | |
| } else { | |
| // Handle the case when the parameter is of unexpected type | |
| // Possibly log an error or return | |
| return | |
| } | |
| } |
Summary by CodeRabbit
New Features
/v1/tenants/{tenant_id}/permissions/bulk-check.Bug Fixes
Documentation
Tests