feat(1199): add bulk-permission check wrapper endpoint #2657
Conversation
WalkthroughAdds a Permission.BulkCheck RPC, server-side BulkCheck implementation, protobuf messages, generated validation, OpenAPI/Swagger docs, and integration tests that perform batched permission checks. Changes
Sequence DiagramsequenceDiagram
autonumber
actor Client
participant API as "API Gateway"
participant Server as "PermissionServer.BulkCheck"
participant Validator as "Message Validator"
participant CheckSvc as "Permission.Check"
participant Aggregator as "Result Aggregator"
Client->>API: POST /v1/tenants/{tenant_id}/permissions/bulk-check
API->>Server: BulkCheck(request)
Server->>Validator: validate tenant_id & items count (1–100)
alt invalid tenant_id/items
Server-->>Client: return validation error
else
par per-item (concurrent)
Server->>Validator: validate item
alt item invalid
Aggregator-->>Server: append DENIED result for item
else
Server->>CheckSvc: Check(per-item PermissionCheckRequest)
alt check success
CheckSvc-->>Aggregator: append item result
else
Aggregator-->>Server: append DENIED result for item
end
end
and continue
end
Server-->>Client: PermissionBulkCheckResponse(results[])
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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: 2
🧹 Nitpick comments (2)
internal/servers/permission_server.go (1)
55-61: Consider using the protobuf Validate() method for consistency.Unlike the
Checkmethod (line 34), which callsrequest.Validate(), this implementation manually validatestenant_id. IfPermissionBulkCheckRequesthas a generatedValidate()method (based on the protobuf validation rules at lines 904-908 in service.proto), use it for consistency and to leverage all protobuf validation rules.Apply this diff if the Validate() method exists:
- // Validate tenant_id - if request.GetTenantId() == "" { - err := status.Error(GetStatus(nil), "tenant_id is required") + v := request.Validate() + if v != nil { + err := status.Error(GetStatus(v), v.Error()) span.RecordError(err) span.SetStatus(otelCodes.Error, err.Error()) return nil, err } - - // Validate number of requests - if len(request.GetItems()) == 0 { - err := status.Error(GetStatus(nil), "at least one item is required") - span.RecordError(err) - span.SetStatus(otelCodes.Error, err.Error()) - return nil, err - } - - if len(request.GetItems()) > 100 { - err := status.Error(GetStatus(nil), "maximum 100 items allowed") - span.RecordError(err) - span.SetStatus(otelCodes.Error, err.Error()) - return nil, err - }proto/base/v1/service.proto (1)
900-941: Well-designed bulk request/response structure.The design appropriately shares common fields (tenant_id, metadata, context, arguments) across all items while allowing per-item variation in entity, permission, and subject. The validation rules correctly enforce the 1-100 item limit mentioned in the RPC description.
Note: Since
metadatais shared across all items, all checks in a bulk request will use the samesnap_token,schema_version, anddepth. This ensures consistency but means you cannot mix different consistency/versioning requirements in a single bulk request. Consider documenting this behavior in the field comment if it's not already clear to API users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/servers/permission_server.go(1 hunks)proto/base/v1/service.proto(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/servers/permission_server.go (1)
pkg/pb/base/v1/base.pb.go (1)
CheckResult_CHECK_RESULT_DENIED(38-38)
🪛 golangci-lint (2.5.0)
internal/servers/permission_server.go
[error] 51-51: : # github.com/Permify/permify/internal/servers
internal/servers/permission_server.go:51:71: undefined: v1.PermissionBulkCheckRequest
internal/servers/permission_server.go:51:104: undefined: v1.PermissionBulkCheckResponse
internal/servers/permission_server.go:121:13: undefined: v1.PermissionBulkCheckResponse
(typecheck)
🔇 Additional comments (4)
internal/servers/permission_server.go (2)
50-53: LGTM!The method signature and tracing setup follow the existing pattern used in other methods like
CheckandExpand.
78-119: Sequential processing is appropriate for this use case.The implementation correctly processes items sequentially and continues on per-item failures, ensuring the bulk operation is resilient. This design choice prioritizes consistency and error isolation over performance.
Note: Processing 100 items sequentially could impact latency. If performance becomes a concern, consider adding metrics to monitor bulk check duration and investigate optimization opportunities later.
proto/base/v1/service.proto (2)
153-169: LGTM!The BulkCheck RPC definition follows the existing patterns for other permission operations. The HTTP mapping and OpenAPI annotations are properly configured.
874-898: Well-structured message definition.The
PermissionBulkCheckRequestItemcorrectly extracts the per-check fields (entity, permission, subject) fromPermissionCheckRequest, with appropriate validation rules applied to each field.
| // Validate individual request | ||
| v := checkRequestItem.Validate() | ||
| if v != nil { | ||
| // Return error response for this check | ||
| results[i] = &v1.PermissionCheckResponse{ | ||
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | ||
| Metadata: &v1.PermissionCheckResponseMetadata{ | ||
| CheckCount: 0, | ||
| }, | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
Consider providing validation error details to callers.
When an item fails validation, the response only includes CHECK_RESULT_DENIED without any indication of what validation rule failed. This makes it difficult for API consumers to debug issues. Consider one of these approaches:
- Add an optional error message field to
PermissionCheckResponseto include validation errors - Log the validation error with the item index for easier server-side debugging
- Document that validation errors result in DENIED responses
If adding error details isn't feasible, at least add structured logging:
// Validate individual request
v := checkRequestItem.Validate()
if v != nil {
+ slog.ErrorContext(ctx, "item validation failed in bulk operation", "error", v.Error(), "index", i)
// Return error response for this check
results[i] = &v1.PermissionCheckResponse{📝 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.
| // Validate individual request | |
| v := checkRequestItem.Validate() | |
| if v != nil { | |
| // Return error response for this check | |
| results[i] = &v1.PermissionCheckResponse{ | |
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | |
| Metadata: &v1.PermissionCheckResponseMetadata{ | |
| CheckCount: 0, | |
| }, | |
| } | |
| continue | |
| } | |
| // Validate individual request | |
| v := checkRequestItem.Validate() | |
| if v != nil { | |
| slog.ErrorContext(ctx, "item validation failed in bulk operation", "error", v.Error(), "index", i) | |
| // Return error response for this check | |
| results[i] = &v1.PermissionCheckResponse{ | |
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | |
| Metadata: &v1.PermissionCheckResponseMetadata{ | |
| CheckCount: 0, | |
| }, | |
| } | |
| continue | |
| } |
🤖 Prompt for AI Agents
In internal/servers/permission_server.go around lines 82–93, when a request item
fails validation the code currently returns a DENIED result with no error
details; update handling to (1) add a validation error message into the response
(either by adding an optional ErrorMessage string on PermissionCheckResponse or
adding an Error field on PermissionCheckResponseMetadata) and populate it with
v.Error() (or v.Error() equivalent) for the failing item, and (2) add a
structured server log entry including the item index and the validation error
before continuing so server-side debugging is possible; ensure the proto change
(if adding a field) is backward-compatible/optional and update any
marshaling/use sites accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/api-reference/apidocs.swagger.json (2)
654-694: Endpoint structure is sound, but add code samples for consistency.The bulk-check endpoint follows the established API patterns and correctly references new request/response definitions. However, unlike other Permission endpoints (check, expand, lookup-entity, etc.), this endpoint lacks x-codeSamples. Consider adding examples for Go, Node.js, and cURL to align with documentation standards and aid API adoption.
1523-1552: BulkCheckBody structure is correct; consider adding validation constraints to the schema.The request body properly mirrors the single-check request pattern and reuses existing metadata and context types. However, the schema should ideally include
minItemsandmaxItemsconstraints on the items array to document the "maximum 100 requests" limit at the schema level, making the constraint discoverable to API clients and code generators."items": { "type": "array", "items": { "type": "object", "$ref": "#/definitions/PermissionBulkCheckRequestItem" }, "description": "List of permission check requests, maximum 100 items.", "minItems": 1, "maxItems": 100 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
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.gopkg/pb/base/v1/service_vtproto.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
docs/api-reference/apidocs.swagger.json(3 hunks)docs/api-reference/openapiv2/apidocs.swagger.json(3 hunks)pkg/pb/base/v1/service.pb.validate.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/pb/base/v1/service.pb.validate.go (1)
pkg/pb/base/v1/service.pb.go (9)
PermissionBulkCheckRequestItem(293-303)PermissionBulkCheckRequestItem(316-316)PermissionBulkCheckRequestItem(331-333)PermissionBulkCheckRequest(357-371)PermissionBulkCheckRequest(384-384)PermissionBulkCheckRequest(399-401)PermissionBulkCheckResponse(439-445)PermissionBulkCheckResponse(458-458)PermissionBulkCheckResponse(473-475)
🔇 Additional comments (7)
docs/api-reference/apidocs.swagger.json (2)
2561-2579: Per-item request definition is complete and consistent.PermissionBulkCheckRequestItem correctly captures the three required fields (entity, permission, subject) and aligns with the CheckBody structure.
2580-2593: Response definition properly aggregates individual check results.PermissionBulkCheckResponse correctly references PermissionCheckResponse array to aggregate per-item results. The structure allows clients to correlate each result with the corresponding request item by position.
docs/api-reference/openapiv2/apidocs.swagger.json (3)
654-694: Endpoint definition is well-structured and follows existing patterns.The bulk-check endpoint is properly positioned alphabetically and includes all necessary components: tenant_id path parameter, request body reference, and appropriate response schemas with error handling.
1521-1550: Request body definition aligns with existing check request patterns.The
BulkCheckBodycorrectly reusesPermissionCheckRequestMetadatafor consistency, includes anitemsarray with documented 100-item limit, and supports optionalcontextandargumentsfields matching the single check endpoint. The structure follows standard REST bulk operation conventions.
2545-2577: Response and item definitions properly support per-item result tracking.
PermissionBulkCheckRequestItemcontains the required entity, permission, and subject fields matching the single check operation, whilePermissionBulkCheckResponsereturns an array ofPermissionCheckResponseobjects—allowing clients to correlate results with individual requests.To ensure correctness, verify that the server-side implementation:
- Validates and enforces the 100-item maximum limit on the
itemsarray- Returns per-item results in the same order as the request items for client-side correlation
- Handles per-item errors gracefully (e.g., records denials individually rather than failing the entire request)
pkg/pb/base/v1/service.pb.validate.go (2)
727-931: Bulk item validation is consistent with existing single-check validatorsThe
PermissionBulkCheckRequestItemvalidation follows the same patterns asPermissionCheckRequest: requiredEntityandSubject, nested message validation, and length/regex constraints forPermissionwith proper aggregation viaValidateAll. No issues spotted here.
1208-1343: Bulk response validation correctly delegates to per-result validators
PermissionBulkCheckResponse.validatejust iteratesResultsand delegates to eachPermissionCheckResponse’sValidate/ValidateAll, aggregating errors intoPermissionBulkCheckResponseMultiError. This matches how other*Responsetypes are handled and looks sound.
| // Validate checks the field values on PermissionBulkCheckRequest with the | ||
| // rules defined in the proto definition for this message. If any rules are | ||
| // violated, the first error encountered is returned, or nil if there are no violations. | ||
| func (m *PermissionBulkCheckRequest) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on PermissionBulkCheckRequest with the | ||
| // rules defined in the proto definition for this message. If any rules are | ||
| // violated, the result is a list of violation errors wrapped in | ||
| // PermissionBulkCheckRequestMultiError, or nil if none found. | ||
| func (m *PermissionBulkCheckRequest) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *PermissionBulkCheckRequest) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| if len(m.GetTenantId()) > 128 { | ||
| err := PermissionBulkCheckRequestValidationError{ | ||
| field: "TenantId", | ||
| reason: "value length must be at most 128 bytes", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| if !_PermissionBulkCheckRequest_TenantId_Pattern.MatchString(m.GetTenantId()) { | ||
| err := PermissionBulkCheckRequestValidationError{ | ||
| field: "TenantId", | ||
| reason: "value does not match regex pattern \"^([a-zA-Z0-9_\\\\-@\\\\.:+]{1,128}|\\\\*)$\"", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| if m.GetMetadata() == nil { | ||
| err := PermissionBulkCheckRequestValidationError{ | ||
| field: "Metadata", | ||
| reason: "value is required", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetMetadata()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: "Metadata", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: "Metadata", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetMetadata()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return PermissionBulkCheckRequestValidationError{ | ||
| field: "Metadata", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if l := len(m.GetItems()); l < 1 || l > 100 { | ||
| err := PermissionBulkCheckRequestValidationError{ | ||
| field: "Items", | ||
| reason: "value must contain between 1 and 100 items, inclusive", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| for idx, item := range m.GetItems() { | ||
| _, _ = idx, item | ||
|
|
||
| if all { | ||
| switch v := interface{}(item).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: fmt.Sprintf("Items[%v]", idx), | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: fmt.Sprintf("Items[%v]", idx), | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(item).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return PermissionBulkCheckRequestValidationError{ | ||
| field: fmt.Sprintf("Items[%v]", idx), | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetContext()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: "Context", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: "Context", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetContext()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return PermissionBulkCheckRequestValidationError{ | ||
| field: "Context", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for idx, item := range m.GetArguments() { | ||
| _, _ = idx, item | ||
|
|
||
| if all { | ||
| switch v := interface{}(item).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: fmt.Sprintf("Arguments[%v]", idx), | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, PermissionBulkCheckRequestValidationError{ | ||
| field: fmt.Sprintf("Arguments[%v]", idx), | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(item).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return PermissionBulkCheckRequestValidationError{ | ||
| field: fmt.Sprintf("Arguments[%v]", idx), | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| if len(errors) > 0 { | ||
| return PermissionBulkCheckRequestMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // PermissionBulkCheckRequestMultiError is an error wrapping multiple | ||
| // validation errors returned by PermissionBulkCheckRequest.ValidateAll() if | ||
| // the designated constraints aren't met. | ||
| type PermissionBulkCheckRequestMultiError []error | ||
|
|
||
| // Error returns a concatenation of all the error messages it wraps. | ||
| func (m PermissionBulkCheckRequestMultiError) Error() string { | ||
| msgs := make([]string, 0, len(m)) | ||
| for _, err := range m { | ||
| msgs = append(msgs, err.Error()) | ||
| } | ||
| return strings.Join(msgs, "; ") | ||
| } | ||
|
|
||
| // AllErrors returns a list of validation violation errors. | ||
| func (m PermissionBulkCheckRequestMultiError) AllErrors() []error { return m } | ||
|
|
||
| // PermissionBulkCheckRequestValidationError is the validation error returned | ||
| // by PermissionBulkCheckRequest.Validate if the designated constraints aren't met. | ||
| type PermissionBulkCheckRequestValidationError struct { | ||
| field string | ||
| reason string | ||
| cause error | ||
| key bool | ||
| } | ||
|
|
||
| // Field function returns field value. | ||
| func (e PermissionBulkCheckRequestValidationError) Field() string { return e.field } | ||
|
|
||
| // Reason function returns reason value. | ||
| func (e PermissionBulkCheckRequestValidationError) Reason() string { return e.reason } | ||
|
|
||
| // Cause function returns cause value. | ||
| func (e PermissionBulkCheckRequestValidationError) Cause() error { return e.cause } | ||
|
|
||
| // Key function returns key value. | ||
| func (e PermissionBulkCheckRequestValidationError) Key() bool { return e.key } | ||
|
|
||
| // ErrorName returns error name. | ||
| func (e PermissionBulkCheckRequestValidationError) ErrorName() string { | ||
| return "PermissionBulkCheckRequestValidationError" | ||
| } | ||
|
|
||
| // Error satisfies the builtin error interface | ||
| func (e PermissionBulkCheckRequestValidationError) Error() string { | ||
| cause := "" | ||
| if e.cause != nil { | ||
| cause = fmt.Sprintf(" | caused by: %v", e.cause) | ||
| } | ||
|
|
||
| key := "" | ||
| if e.key { | ||
| key = "key for " | ||
| } | ||
|
|
||
| return fmt.Sprintf( | ||
| "invalid %sPermissionBulkCheckRequest.%s: %s%s", | ||
| key, | ||
| e.field, | ||
| e.reason, | ||
| cause) | ||
| } | ||
|
|
||
| var _ error = PermissionBulkCheckRequestValidationError{} | ||
|
|
||
| var _ interface { | ||
| Field() string | ||
| Reason() string | ||
| Key() bool | ||
| Cause() error | ||
| ErrorName() string | ||
| } = PermissionBulkCheckRequestValidationError{} | ||
|
|
||
| var _PermissionBulkCheckRequest_TenantId_Pattern = regexp.MustCompile("^([a-zA-Z0-9_\\-@\\.:+]{1,128}|\\*)$") |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Locate the proto-generated validation and proto-buf files
fd -t f "service.pb.go" "service.pb.validate.go"Repository: Permify/permify
<shell_result>
Repository: Permify/permify
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository Permify/permify: Clone operation failed
</shell_result>
Script executed:
#!/bin/bash
# Locate the proto-generated validation and proto-buf files
fd -t f "service.pb.go" "service.pb.validate.go"Repository: Permify/permify
<shell_result>
Repository: Permify/permify
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository Permify/permify: Clone operation failed
</shell_result>
Permify permify service.pb.go PermissionBulkCheckRequest TenantId
<web_search_result>
Do you want the exact generated Go struct/field (code snippet and file path), the proto message definition, or how to set TenantId when calling PermissionBulkCheckRequest?
</web_search_result>
Permify/permify github service proto TenantId validation constraints
<web_search_result>
TenantId constraints used in Permify APIs (service/proto level):
- Required. If not using multi-tenancy, default pre-inserted tenant id is "t1". [1]
- Pattern (regex): [a-zA-Z0-9-,]+. [1][2]
- Maximum length: 64 bytes. [1][2]
Sources: Permify API docs (List/Read Schema and other endpoints). [1] https://docs.permify.co/api-reference/schema/list-schema [2] https://docs.permify.co/api-reference/schema/read-schema
</web_search_result>
Permify permify service.proto TenantId validation "128" OR "64"
<web_search_result>
Permify validates tenant_id as maximum 64 bytes (pattern "[a-zA-Z0-9-,]+", max 64 bytes). See the API docs and generated client comments. [1][2]
Sources:
- Permify API docs — path parameter tenant_id: "must match the pattern "[a-zA-Z0-9-,]+", max 64 bytes." [1]
- Permify Java client README/example (client comment showing the same tenantId pattern and "max 64 bytes"). [2]
</web_search_result>
site:github.com Permify/permify "service.proto" tenant_id
<web_search_result>
I found that Permify’s proto-generated types include a tenant_id (TenantId) field (e.g., in base/v1 permission and relationship request messages). See Permify repo and generated protobuf Go package. [1] (github.com)
Sources:
- Permify main repo on GitHub. [1] (github.com)
- Generated Go protobuf types showing TenantId fields (pkg.go.dev mirror). [2] (pkg.go.dev)
Citations:
- 1: https://github.com/Permify/permify?utm_source=openai
- 2: https://github.com/Permify/permify?utm_source=openai
- 3: https://pkg.go.dev/github.com/adminium/permify/pkg/pb/base/v1?utm_source=openai
</web_search_result>
Based on my verification through official Permify API documentation and client libraries, I can confirm the core concern in the review comment:
Official Permify documentation consistently specifies:
- TenantId pattern:
[a-zA-Z0-9-,]+ - Maximum length: 64 bytes
The review comment accurately identifies that the validator code shown uses significantly broader constraints:
- Pattern:
^([a-zA-Z0-9_\\-@\\.:+]{1,128}|\\*)$(includes underscore, @, :, +, and wildcard) - Maximum length: 128 bytes (double the documented limit)
This is a legitimate discrepancy between the documented API contract and the validation rules shown in the code snippet.
TenantId rules in bulk request do not match official API documentation
In PermissionBulkCheckRequest.validate, TenantId is validated with:
len(TenantId) <= 128- Regex
^([a-zA-Z0-9_\\-@\\.:+]{1,128}|\\*)$
However, official Permify API documentation and client libraries specify:
- Maximum length: 64 bytes
- Pattern:
[a-zA-Z0-9-,]+
These are significantly different constraint sets. The validator allows double the length and a much broader character set (including _, @, :, +, and *) compared to documented rules.
The rest of the validator (required Metadata, 1–100 Items, and nested validation for Items, Context, and Arguments) appears correct and consistent.
Recommendation: Align the proto constraints with the documented API contract—either tighten the validation in the proto definition to match the 64-byte, [a-zA-Z0-9-,]+ pattern and regenerate, or update official documentation if the broader 128-byte rules are intentional.
|
|
||
| // Process each check request | ||
| results := make([]*v1.PermissionCheckResponse, len(request.GetItems())) | ||
| for i, checkRequestItem := range request.GetItems() { |
There was a problem hiding this comment.
hi @inabhi9, here it is running db checks sequentially, which can slow down bulk operations.
can we process these items concurrently using a worker pool or a similar concurrent aproach?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/servers/permission_server.go (2)
125-125: Fix formatting inconsistency (duplicate).Line 125 uses tabs for indentation while surrounding lines use spaces.
Apply this diff (same as previously suggested):
- Metadata: request.GetMetadata(), + Metadata: request.GetMetadata(),
104-117: Consider providing validation error details to callers (duplicate).When an item fails validation, the response includes only
CHECK_RESULT_DENIEDwithout indicating which validation rule failed, making debugging difficult for API consumers.As previously suggested, consider adding structured logging:
// Validate individual request v := checkRequestItem.Validate() if v != nil { + slog.ErrorContext(ctx, "item validation failed in bulk operation", "error", v.Error(), "index", index) // Return error response for this check resultChannel <- resultItem{(Note: This assumes the critical closure bug fix above is applied, so
indexis available as a parameter.)
🧹 Nitpick comments (1)
internal/servers/permission_server.go (1)
164-166: Optional: mutex may be unnecessary.Since each goroutine writes to a unique index in the
resultsslice (once the closure bug is fixed), the mutex on lines 164–166 is technically unnecessary. However, it's a safe defensive practice and won't impact performance given the 100-item limit.If you prefer to remove it for clarity:
- case response := <-resultChannel: - // If there's no error, we add the result to our response's Results map. - // We use a mutex to safely update the map since multiple goroutines may be writing to it concurrently. - mutex.Lock() - results[response.index] = response.response - mutex.Unlock() + case response := <-resultChannel: + results[response.index] = response.responseAnd remove the
mutexvariable declaration on line 94.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/servers/permission_server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/servers/permission_server.go (2)
pkg/pb/base/v1/base.pb.go (1)
CheckResult_CHECK_RESULT_DENIED(38-38)pkg/pb/base/v1/errors.pb.go (1)
ErrorCode_ERROR_CODE_CANCELLED(84-84)
🔇 Additional comments (4)
internal/servers/permission_server.go (4)
6-7: LGTM!The new imports (
errorsandsync) are appropriate for the concurrent bulk-check implementation.
52-68: LGTM!The method signature, tracing setup, and tenant_id validation follow established patterns from the existing
Checkmethod.
70-85: LGTM!The item count validation correctly enforces the 1–100 range as documented in the PR objectives.
174-177: LGTM!The final response construction correctly returns the aggregated results.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/servers/permission_server.go (2)
103-117: Log per-item validation failures (and optionally expose details) to aid debugging.When an individual item fails
checkRequestItem.Validate(), you silently map it toCHECK_RESULT_DENIEDwithout logging or surfacing why it failed. That makes debugging client issues with bulk requests difficult, especially when only one item in a large batch is malformed.At minimum, consider logging the validation error with the item index:
@@ - v := checkRequestItem.Validate() - if v != nil { - // Return error response for this check - resultChannel <- ResultChannel{ + v := checkRequestItem.Validate() + if v != nil { + slog.ErrorContext(ctx, "item validation failed in bulk operation", + "error", v.Error(), + "index", index, + ) + // Return error response for this check + resultChannel <- ResultChannel{If the API surface allows it, you might also add an optional error field to
PermissionCheckResponseor its metadata and populate it withv.Error()for these cases, as suggested in the earlier review.
120-128: FixMetadataindentation to match gofmt style.The
Metadatafield in the struct literal mixes spaces and a tab:Metadata: request.GetMetadata(),This is inconsistent with the surrounding fields and will be rewritten by
gofmt. Please normalize indentation:- Metadata: request.GetMetadata(), + Metadata: request.GetMetadata(),Running
gofmton the file should take care of this and any similar issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/servers/permission_server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/servers/permission_server.go (3)
internal/config/config.go (2)
Tracer(102-110)Permission(144-148)pkg/pb/base/v1/base.pb.go (1)
CheckResult_CHECK_RESULT_DENIED(38-38)pkg/pb/base/v1/errors.pb.go (1)
ErrorCode_ERROR_CODE_CANCELLED(84-84)
🔇 Additional comments (2)
internal/servers/permission_server.go (2)
80-81: Consider wiring bulk/concurrency limits to config instead of hard-coding100and unbounded goroutines.You currently enforce a fixed
maximum 100 items allowedand spawn one goroutine per item (bounded only by that constant). Giveninternal/config/config.goexposesPermission.BulkLimitandPermission.ConcurrencyLimit, consider using these config values instead of hardcoded limits. This would keep behavior controllable at runtime and aligned with deployments that need stricter limits.Also applies to: 87-99
87-95: Simplify result fan-out/fan-in: rename struct to use named fields and reconsider synchronization.The concurrency pattern has clarity and style issues:
type ResultChannel struct {int; *v1.PermissionCheckResponse}uses embedded builtin and pointer types, creating non-idiomatic accessors likeresponse.int. Use named fields (index int,response *v1.PermissionCheckResponse) for readability.- The
mutexprotectingresultsmay be unnecessary if the slice is only written by the main receive loop and no concurrent access occurs. Verify all writes toresultshappen exclusively in that loop.- The
WaitGroup+ deferred close is potentially optional given the buffering strategy (channel capacity equals item count, exactly one result per item, receive loop runslen(checkItems)times), but removing it requires confirming all goroutines complete their sends before the loop finishes.Suggested struct rename and field clarification:
type resultItem struct { index int response *v1.PermissionCheckResponse } resultCh := make(chan resultItem, len(checkItems))Then update all sends to use named field syntax:
resultCh <- resultItem{index: index, response: response}.Before removing the
mutexandWaitGroup, confirm thatresultsis written exclusively within the receive loop and that the buffering guarantees no goroutines attempt post-loop sends.
| // BulkCheck - Performs multiple authorization checks in a single request | ||
| func (r *PermissionServer) BulkCheck(ctx context.Context, request *v1.PermissionBulkCheckRequest) (*v1.PermissionBulkCheckResponse, error) { | ||
| // emptyResp is a default, empty response that we will return in case of an error or when the context is cancelled. | ||
| emptyResp := &v1.PermissionBulkCheckResponse{ | ||
| Results: make([]*v1.PermissionCheckResponse, 0), | ||
| } | ||
|
|
||
| ctx, span := internal.Tracer.Start(ctx, "permissions.bulk-check") | ||
| defer span.End() | ||
|
|
||
| // Validate tenant_id | ||
| if request.GetTenantId() == "" { | ||
| err := status.Error(GetStatus(nil), "tenant_id is required") | ||
| span.RecordError(err) | ||
| span.SetStatus(otelCodes.Error, err.Error()) | ||
| return nil, err | ||
| } | ||
|
|
||
| checkItems := request.GetItems() | ||
|
|
||
| // Validate number of requests | ||
| if len(checkItems) == 0 { | ||
| err := status.Error(GetStatus(nil), "at least one item is required") | ||
| span.RecordError(err) | ||
| span.SetStatus(otelCodes.Error, err.Error()) | ||
| return nil, err | ||
| } | ||
|
|
||
| if len(checkItems) > 100 { | ||
| err := status.Error(GetStatus(nil), "maximum 100 items allowed") | ||
| span.RecordError(err) | ||
| span.SetStatus(otelCodes.Error, err.Error()) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Fix error handling in BulkCheck to use proper gRPC status codes instead of GetStatus(nil).
The validation and cancellation error handling diverges from standard gRPC patterns:
- Validation branches (empty tenant_id, zero items, >100 items) call
status.Error(GetStatus(nil), "..."). SinceGetStatus(nil)returnscodes.OK, this is semantically incorrect—validation errors should usecodes.InvalidArgument. - On context cancellation, the code returns a non-nil response (
emptyResp) witherrors.New(...). This bypasses gRPC's error handling: when a handler returns both a response and error, gRPC sends only the error status to the client, and using a plain error maps tocodes.Unknowninstead ofcodes.Canceled.
Replace GetStatus(nil) calls with explicit codes.InvalidArgument, return nil for the response on all errors, and handle cancellation with codes.Canceled:
- if request.GetTenantId() == "" {
- err := status.Error(GetStatus(nil), "tenant_id is required")
+ if request.GetTenantId() == "" {
+ err := status.Error(codes.InvalidArgument, "tenant_id is required")
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
return nil, err
}
- if len(checkItems) == 0 {
- err := status.Error(GetStatus(nil), "at least one item is required")
+ if len(checkItems) == 0 {
+ err := status.Error(codes.InvalidArgument, "at least one item is required")
- if len(checkItems) > 100 {
- err := status.Error(GetStatus(nil), "maximum 100 items allowed")
+ if len(checkItems) > 100 {
+ err := status.Error(codes.InvalidArgument, "maximum 100 items allowed")
- case <-ctx.Done():
- return emptyResp, errors.New(v1.ErrorCode_ERROR_CODE_CANCELLED.String())
+ case <-ctx.Done():
+ err := status.Error(codes.Canceled, "request cancelled")
+ span.RecordError(err)
+ span.SetStatus(otelCodes.Error, err.Error())
+ return nil, errThe errors import can then be removed if no longer used elsewhere. This same pattern applies at lines 168–171.
📝 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.
| // BulkCheck - Performs multiple authorization checks in a single request | |
| func (r *PermissionServer) BulkCheck(ctx context.Context, request *v1.PermissionBulkCheckRequest) (*v1.PermissionBulkCheckResponse, error) { | |
| // emptyResp is a default, empty response that we will return in case of an error or when the context is cancelled. | |
| emptyResp := &v1.PermissionBulkCheckResponse{ | |
| Results: make([]*v1.PermissionCheckResponse, 0), | |
| } | |
| ctx, span := internal.Tracer.Start(ctx, "permissions.bulk-check") | |
| defer span.End() | |
| // Validate tenant_id | |
| if request.GetTenantId() == "" { | |
| err := status.Error(GetStatus(nil), "tenant_id is required") | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| return nil, err | |
| } | |
| checkItems := request.GetItems() | |
| // Validate number of requests | |
| if len(checkItems) == 0 { | |
| err := status.Error(GetStatus(nil), "at least one item is required") | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| return nil, err | |
| } | |
| if len(checkItems) > 100 { | |
| err := status.Error(GetStatus(nil), "maximum 100 items allowed") | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| return nil, err | |
| } | |
| // BulkCheck - Performs multiple authorization checks in a single request | |
| func (r *PermissionServer) BulkCheck(ctx context.Context, request *v1.PermissionBulkCheckRequest) (*v1.PermissionBulkCheckResponse, error) { | |
| // emptyResp is a default, empty response that we will return in case of an error or when the context is cancelled. | |
| emptyResp := &v1.PermissionBulkCheckResponse{ | |
| Results: make([]*v1.PermissionCheckResponse, 0), | |
| } | |
| ctx, span := internal.Tracer.Start(ctx, "permissions.bulk-check") | |
| defer span.End() | |
| // Validate tenant_id | |
| if request.GetTenantId() == "" { | |
| err := status.Error(codes.InvalidArgument, "tenant_id is required") | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| return nil, err | |
| } | |
| checkItems := request.GetItems() | |
| // Validate number of requests | |
| if len(checkItems) == 0 { | |
| err := status.Error(codes.InvalidArgument, "at least one item is required") | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| return nil, err | |
| } | |
| if len(checkItems) > 100 { | |
| err := status.Error(codes.InvalidArgument, "maximum 100 items allowed") | |
| span.RecordError(err) | |
| span.SetStatus(otelCodes.Error, err.Error()) | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
internal/servers/permission_server.go lines 52-85: validation branches currently
call status.Error(GetStatus(nil), ...) which resolves to codes.OK and is
incorrect; replace those with status.Error(codes.InvalidArgument, "...") and
return (nil, err) instead of returning a non-nil response, and ensure context
cancellation paths return status.Error(codes.Canceled, "...") (see same pattern
to fix at lines 168-171); after making these changes remove the unused errors
import if it’s no longer referenced.
|
@inabhi9 thank you! can we also add some integration tests for this api. please check /integrations-tests/usecases |
|
@ucatbas sure thing! I wanted some early feedback before adding tests. I see many other endpoints is proxy to engine's functions so not sure if I should move the implementation to engine. Btw, I found out you also created PR for the same issue and closed. Why it didn't get merge if may I ask? |
it was more of a team decision due to bandwidths. |
for this iteration, i think we can simply remove the network layer for bulk checks as in this PR. moving it into the engine will add more complexity and would require additional validation and optimization. maybe we can revisit that approach later if needed. i will be running some load tests on this version to understand whether that alternative is truly necessary or just over-engineering |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
integration-test/usecases/notion_test.go (1)
77-131: Same BulkCheck ordering assumption + unused context as other suites.This test also assumes response ordering matches request ordering (Line 128-130) and computes
contextTupleswithout using it (Line 94-99). Same flakiness/coverage concerns apply.If you confirm order is guaranteed by contract/implementation, great—otherwise please switch to an order-agnostic assertion strategy (or add a per-item correlation field if the API supports it). You can reuse the same verification script from the google-docs test comment to validate ordering + Context support.
integration-test/usecases/facebook_groups_test.go (1)
77-131: Consider extracting a shared helper for building BulkCheck items/expectations (and address ordering/context).Beyond the ordering assumption and unused
contextTuples(same as the other two files), this block is effectively duplicated 3x across suites. A small helper (e.g.,buildBulkCheckItemsAndExpected(shape)) would reduce drift and make future API changes (adding context/correlation ids) a one-liner.Before refactoring, please confirm BulkCheck result ordering + Context support (see the verification script in the google-docs test comment). If ordering isn’t guaranteed, refactoring into a helper will also make it much easier to implement a single order-agnostic comparison strategy.
integration-test/usecases/google_docs_test.go (1)
77-131: Remove unusedcontextTuplesvariable; Context is request-level and applied uniformly to all items.The test builds
contextTuples(lines 94–99) but never uses them. This is misleading because the proto definesContextat thePermissionBulkCheckRequestlevel (not per-item), and the server implementation passesrequest.GetContext()uniformly to all checks. Either remove the context-building block to avoid false coverage, or if per-item context is needed, verify the proto supports it before implementing it.Note: Response ordering is guaranteed. The server pre-allocates the results array and indexes each response via the index field in the channel, ensuring order preservation despite concurrent processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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)
🧰 Additional context used
🧬 Code graph analysis (3)
integration-test/usecases/google_docs_test.go (3)
pkg/tuple/tuple.go (2)
E(215-234)EAR(185-212)pkg/pb/base/v1/base.pb.go (2)
CheckResult_CHECK_RESULT_ALLOWED(36-36)CheckResult_CHECK_RESULT_DENIED(38-38)pkg/cmd/validate.go (1)
Depth(523-531)
integration-test/usecases/facebook_groups_test.go (4)
pkg/tuple/tuple.go (2)
E(215-234)EAR(185-212)pkg/schema/builder.go (1)
Relation(137-142)pkg/pb/base/v1/base.pb.go (2)
CheckResult_CHECK_RESULT_ALLOWED(36-36)CheckResult_CHECK_RESULT_DENIED(38-38)pkg/cmd/validate.go (1)
Depth(523-531)
integration-test/usecases/notion_test.go (2)
pkg/tuple/tuple.go (2)
E(215-234)EAR(185-212)pkg/pb/base/v1/base.pb.go (2)
CheckResult_CHECK_RESULT_ALLOWED(36-36)CheckResult_CHECK_RESULT_DENIED(38-38)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/servers/permission_server.go (2)
63-85: Fix validation error handling to use proper gRPC status codes.Lines 64, 74, and 81 still call
status.Error(GetStatus(nil), "...")for validation errors. As noted in previous reviews,GetStatus(nil)returnscodes.OK, which is semantically incorrect for validation failures. These should usecodes.InvalidArgument.Apply the changes suggested in the previous review to replace
GetStatus(nil)withcodes.InvalidArgumentfor all three validation branches.
173-174: Fix context cancellation to use proper gRPC status codes.Line 174 returns
errors.New(v1.ErrorCode_ERROR_CODE_CANCELLED.String())with a non-nil response. As noted in previous reviews, this bypasses gRPC's error handling and maps tocodes.Unknowninstead ofcodes.Canceled.Apply the changes suggested in the previous review to use
status.Error(codes.Canceled, "request cancelled")and return(nil, err).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/servers/permission_server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/servers/permission_server.go (2)
pkg/pb/base/v1/base.pb.go (1)
CheckResult_CHECK_RESULT_DENIED(38-38)pkg/pb/base/v1/errors.pb.go (1)
ErrorCode_ERROR_CODE_CANCELLED(84-84)
🔇 Additional comments (1)
internal/servers/permission_server.go (1)
87-151: Well-structured concurrent implementation.The concurrent processing approach effectively addresses the sequential bottleneck concern raised in previous reviews. The use of goroutines with proper synchronization (WaitGroup, buffered channel, mutex) is appropriate, and the goroutine closure correctly captures the loop index by parameter to avoid race conditions.
| resultChannel <- resultItem{ | ||
| index: index, | ||
| response: &v1.PermissionCheckResponse{ | ||
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | ||
| Metadata: &v1.PermissionCheckResponseMetadata{ | ||
| CheckCount: 0, | ||
| }, | ||
| }, | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Perform the check using existing Check function | ||
| checkRequest := &v1.PermissionCheckRequest{ | ||
| TenantId: request.GetTenantId(), | ||
| Subject: checkRequestItem.GetSubject(), | ||
| Entity: checkRequestItem.GetEntity(), | ||
| Permission: checkRequestItem.GetPermission(), | ||
| Metadata: request.GetMetadata(), | ||
| Context: request.GetContext(), | ||
| Arguments: request.GetArguments(), | ||
| } | ||
| response, err := r.invoker.Check(ctx, checkRequest) | ||
| if err != nil { | ||
| // Log error but don't fail the entire bulk operation | ||
| slog.ErrorContext(ctx, "check failed in bulk operation", "error", err.Error(), "index", index) | ||
| resultChannel <- resultItem{ | ||
| index: index, | ||
| response: &v1.PermissionCheckResponse{ | ||
| Can: v1.CheckResult_CHECK_RESULT_DENIED, | ||
| Metadata: &v1.PermissionCheckResponseMetadata{ | ||
| CheckCount: 1, | ||
| }, | ||
| }, | ||
| } | ||
| return |
There was a problem hiding this comment.
Inconsistent CheckCount values in error responses.
When validation fails (line 113), CheckCount is set to 0, but when the check operation fails (line 139), CheckCount is set to 1. This inconsistency may confuse API consumers about whether CheckCount represents checks attempted or checks completed successfully.
Consider standardizing to 0 for both cases (no successful check occurred) or documenting the semantic difference.
🤖 Prompt for AI Agents
In internal/servers/permission_server.go around lines 108 to 143, the error
responses in the bulk-check path use inconsistent CheckCount values
(validation-fail returns CheckCount: 0 while check-fail returns CheckCount: 1);
change the check-fail branch to return CheckCount: 0 as well (or vice-versa if
you prefer the other semantic) so both error paths consistently indicate no
successful checks, and update any related comments/tests to reflect the chosen
standard.
|
Thanks for the PR, @inabhi9. This feature was highly needed, great work! |
3 participants
This PR adds
/v1/tenants/{{tenant_id}}/permissions/bulk-checkendpoint to check permission in bulk.It's a simple wrapper around check invoker.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.