feat: add BulkCheck method in sync with permify go server #376
Conversation
📝 WalkthroughWalkthroughUpdates docker-compose to use permify v1.6.0, adds a new BulkCheck RPC and supporting messages to the Permission service proto, and expands gRPC client tests to include single-check, lookup, and bulk-check scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/grpc-clients.test.ts (3)
5-60: Consider adding error handling to prevent test hangs.The test uses promise chaining without
.catch()handlers. If any promise rejects, the test may hang instead of failing cleanly.🔎 Suggested improvement with error handling
it("permission client check", (done) => { // Initialize the Permify gRPC client let client = permify.grpc.newClient({ endpoint: "localhost:3478", cert: undefined, pk: undefined, certChain: undefined, insecure: true, }); // Define the schema let schema = ` entity user {} entity document { relation viewer @user action view = viewer } `; // Write the schema client.schema .write({ tenantId: "t1", schema: schema, }) .then((response1_1: permify.grpc.payload.SchemaWriteResponse) => { // Perform the permission check client.permission .check({ tenantId: "t1", metadata: { snapToken: "", schemaVersion: response1_1.schemaVersion, depth: 20, }, entity: { type: "document", id: "1", }, permission: "view", subject: { type: "user", id: "3", }, }) .then((response1_2: permify.grpc.payload.PermissionCheckResponse) => { // Verify the response expect(response1_2.can).toBe( permify.grpc.base.CheckResult.CHECK_RESULT_DENIED ); done(); - }); - }); + }) + .catch((err) => done(err)); + }) + .catch((err) => done(err)); });
184-290: Bulk check test logic looks good, but add error handling.The test correctly validates the bulk check functionality with both allowed and denied scenarios. However, it lacks error handling that could cause the test to hang on failure.
🔎 Add error handling to prevent test hangs
.then( ( response_bulk: permify.grpc.payload.PermissionBulkCheckResponse ) => { // Verify the response // Expecting 2 results expect(response_bulk.results.length).toBe(2); // First check should be ALLOWED because we added the tuple expect(response_bulk.results[0].can).toBe( permify.grpc.base.CheckResult.CHECK_RESULT_ALLOWED ); // Second check should be DENIED expect(response_bulk.results[1].can).toBe( permify.grpc.base.CheckResult.CHECK_RESULT_DENIED ); done(); } - ); + ) + .catch((err) => done(err)); }) - .then((_response_data: permify.grpc.payload.DataWriteResponse) => { + .catch((err) => done(err)); }) + .catch((err) => done(err)); });
294-301: Enhance stream handler to validate completeness and handle errors.The
handlefunction only checks that received IDs are in the expected list but doesn't verify that all expected IDs were received. It also doesn't handle stream errors.🔎 Improved handle function with completeness check
// Helper function to handle the stream response async function handle( res: AsyncIterable<permify.grpc.payload.PermissionLookupEntityStreamResponse>, expected: string[] ) { + const received = new Set<string>(); for await (const response of res) { expect(expected.includes(response.entityId)).toBe(true); + received.add(response.entityId); } + // Verify all expected items were received + expect(received.size).toBe(expected.length); + for (const id of expected) { + expect(received.has(id)).toBe(true); + } }proto/base/v1/service.proto (3)
153-169: Consider adding code samples for the new BulkCheck API.The BulkCheck RPC definition is correct, but unlike other methods (e.g., Check at lines 16-151), it lacks
x-codeSamplesin the OpenAPI annotations. Adding examples would improve API documentation consistency.Example code sample structure to add
You could add code samples similar to the Check method, showing how to use bulk check in Go, JavaScript/Node, and cURL. This would be added within the
openapiv2_operationextensions section.
938-941: Response structure is clean but lacks ordering documentation.Reusing
PermissionCheckResponsefor results is a good design choice. However, the comment should explicitly state that results correspond to request items by index.Enhanced documentation
// PermissionBulkCheckResponse is the response message for the BulkCheck method in the Permission service. message PermissionBulkCheckResponse { - // List of permission check responses corresponding to each request. + // List of permission check responses corresponding to each request item. + // Results are returned in the same order as the request items (results[i] corresponds to items[i]). repeated PermissionCheckResponse results = 1 [json_name = "results"]; }
918-925: Add documentation on partial failure handling and clarify ordering behavior.The
itemsfield lacks documentation on partial failure behavior and explicit ordering guarantees. While the response message implies results correspond to request order, the request message should clarify this explicitly. Additionally, document whether a single check failure causes the entire request to fail or if results are returned per-item.// List of permission check requests, maximum 100 items. // Results are returned in the same order as the request items. // If any individual check fails, the entire request fails. repeated PermissionBulkCheckRequestItem items = 3 [ json_name = "items", (validate.rules).repeated = { min_items: 1 max_items: 100 } ];
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/grpc/generated/base/v1/service.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
docker-compose.yamlproto/base/v1/service.protosrc/grpc-clients.test.ts
🔇 Additional comments (2)
proto/base/v1/service.proto (1)
874-898: LGTM - PermissionBulkCheckRequestItem structure is well-defined.The message structure correctly captures the essential per-item fields (entity, permission, subject) while context and arguments are shared at the request level. Validation rules are consistent with the single check API.
docker-compose.yaml (1)
3-4: Version bump aligns with the new bulk check feature.The update to v1.6.0 introduces the Bulk Check endpoint, enabling batch authorization checks for multiple tuples in a single request to reduce network overhead and latency. The port syntax change is cosmetic.
Previously, the test used a fixed 1000ms timeout to call `done()` after handling the stream response. This approach was error-prone as the stream processing time might vary, potentially causing flaky tests or unnecessary delays. Now, the test awaits the promise returned by `handle` (which resolves when stream processing completes) before calling `done()`. This ensures the test only finishes when the stream processing is actually complete, making the test more reliable and responsive to actual processing time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/grpc-clients.test.ts (5)
5-60: Good refactor to promise chaining, but add error handling.The refactor from nested callbacks to promise chaining improves readability. However, the test lacks error handling - if any promise rejects, the test will timeout rather than fail immediately with the actual error, making debugging harder.
🔎 Suggested error handling
.then((response1_2: permify.grpc.payload.PermissionCheckResponse) => { // Verify the response expect(response1_2.can).toBe( permify.grpc.base.CheckResult.CHECK_RESULT_DENIED ); done(); - }); + }) + .catch((err) => done(err)); });
62-179: Excellent fix for stream handling, but add error handling.The refactor to use the
handlefunction that returns a promise addresses the previous review comment about the unreliablesetTimeoutapproach. The test now properly waits for stream completion.However, add error handling to prevent test timeouts on failures.
🔎 Suggested error handling
// Handle the stream response and wait for completion handle(response2_3, ["1", "3", "4"]).then(() => { done(); - }); + }) + .catch((err) => done(err)); }); + }) + .catch((err) => done(err));
181-287: Well-structured bulk check test, but add error handling.The new bulk check test correctly validates the new BulkCheck functionality by testing both ALLOWED and DENIED cases. The test logic is sound and assertions are appropriate.
Add error handling to prevent timeouts on failures.
🔎 Suggested error handling
done(); } - ); + ) + .catch((err) => done(err)); }); + }) + .catch((err) => done(err));
291-298: Enhance stream validation to ensure all expected items are received.The current validation only checks that each received
entityIdis in the expected list, but doesn't verify that all expected IDs were actually received. If the stream returns only["1"], the test would pass even though["3", "4"]are missing.🔎 More comprehensive validation
async function handle( res: AsyncIterable<permify.grpc.payload.PermissionLookupEntityStreamResponse>, expected: string[] ) { + const received: string[] = []; for await (const response of res) { expect(expected.includes(response.entityId)).toBe(true); + received.push(response.entityId); } + // Ensure all expected IDs were received + expect(received.sort()).toEqual(expected.sort()); }
7-13: Consider extracting client initialization to reduce duplication.The client initialization code is duplicated across all three tests. While this provides clear test isolation, extracting it to a helper function or
beforeEachblock would improve maintainability.Example refactor
describe("clients test", () => { let client: ReturnType<typeof permify.grpc.newClient>; beforeEach(() => { client = permify.grpc.newClient({ endpoint: "localhost:3478", cert: undefined, pk: undefined, certChain: undefined, insecure: true, }); }); it("permission client check", (done) => { // Test implementation without client initialization // ... }); // ... other tests });Note: If tests need to be completely independent (e.g., for parallel execution), the current approach is fine.
Also applies to: 64-70, 183-189
|
Hi @tolgaozen, just wanted to check if there are any updates on this PR. Appreciate your time and feedback! |
1 participant
Summary
The Permify server recently introduced a bulk permission check API.
This PR updates the permify-node client to expose the same functionality.
What’s included
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.