feat: add support for list of permissions in lookup-entity #1465
Conversation
|
Someone is attempting to deploy a commit to the permify Team on Vercel. A member of the Team first needs to authorize it. |
…ithub.com:ankitsheoran1/permify into support-for-array-of-permissions-in-lookup-entity pull from origin and merge master
|
Hi @ankitsheoran1, thank your for you contribution! I have updated the last case you added for linked schemas and now it is passing. |
WalkthroughThe changes introduce new endpoints and modify existing ones in the Permify API, significantly enhancing the handling of permissions. Two new endpoints, 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: 2
Outside diff range and nitpick comments (3)
internal/schema/linkedSchema.go (1)
90-123: Consider performance improvements.The function looks good overall. Here are a few suggestions to further improve it:
Return early if an error is encountered in any goroutine, instead of waiting for all of them to finish. This can be done by checking the
errChanin a separate goroutine.Use
sync.Mapinstead of a regular map with a mutex for thevisitedmap. This provides better performance for concurrent access.Limit the number of concurrent goroutines to a reasonable number to avoid spawning too many goroutines for a large number of targets. This can be done using a semaphore or a fixed-size worker pool.
pkg/development/development.go (1)
421-424: Approve the changes to support multiple permissions and improve entity ID organization.The modifications to the
RunWithShapefunction, specifically the change from a single permission string to a slice of permissions in thePermissionLookupEntityRequeststruct and the introduction of theGetEntityIds()map structure, enhance the flexibility and organization of the permission lookup process. These changes allow for checking multiple permissions simultaneously and improve the retrieval of entity IDs based on permissions.Consider the following suggestions for further improvement:
Add unit tests to cover the new functionality and ensure the correctness of the permission lookups with multiple permissions.
Evaluate the performance implications of the
isSameArrayfunction for large arrays. If necessary, explore more efficient comparison methods, such as using sets or optimized sorting algorithms, to handle large-scale comparisons.Also applies to: 438-440
pkg/cmd/validate.go (1)
354-356: LGTM!The change from
PermissiontoPermissionsis a step towards supporting multiple permissions in the future, which aligns with the goal of enhancing permission handling flexibility.Consider fully utilizing the
Permissionsarray by allowing multiple permission strings to be specified, when the requirement arises.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pkg/pb/base/v1/service.pb.gois excluded by!**/*.pb.go
Files selected for processing (17)
- docs/api-reference/apidocs.swagger.json (5 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (5 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/engines/bulk.go (4 hunks)
- internal/engines/lookup.go (10 hunks)
- internal/engines/lookup_test.go (58 hunks)
- internal/engines/massEntityFilter.go (3 hunks)
- internal/engines/schemaBasedEntityFilter.go (13 hunks)
- internal/invoke/invoke.go (2 hunks)
- internal/schema/linkedSchema.go (10 hunks)
- internal/schema/linkedSchema_test.go (18 hunks)
- pkg/cmd/validate.go (2 hunks)
- pkg/development/development.go (2 hunks)
- pkg/pb/base/v1/service.pb.validate.go (6 hunks)
- proto/base/v1/service.proto (4 hunks)
Additional comments not posted (76)
internal/engines/massEntityFilter.go (3)
30-30: Verify the usage of the newpermissionChecksparameter.The addition of the
permissionChecksparameter to theEntityFiltermethod signature is a valid change. It suggests that the method will now perform permission checks based on the providedpermissionChecksmap.Please ensure that the
permissionChecksparameter is being used correctly throughout the rest of the method implementation. Verify that the permission checks are being performed as intended and that there are no inconsistencies or errors in how thepermissionChecksmap is being accessed and utilized.
75-75: LGTM!The
permissionChecksparameter is being correctly passed to thePublishmethod of thepublisherobject. This ensures that the permission checks will be performed on the entity before publishing it.
102-102: LGTM!The
permissionChecksparameter is being correctly passed to thePublishmethod of thepublisherobject. This ensures that the permission checks will be performed on the entity before publishing it.integration-test/usecases/notion_test.go (1)
114-124: LGTM!The changes to the
LookupEntityrequest and response validation logic are consistent with the new support for multiple permissions per entity. The test case has been updated to accurately reflect the new structure of the response data, ensuring the robustness and accuracy of the test.integration-test/usecases/google_docs_test.go (1)
114-124: LGTM! The changes enhance the functionality and robustness of the test.The modifications to the
PermissionLookupEntityRequestand the response handling logic introduce several benefits:
Support for multiple permissions: By changing the
Permissionfield toPermissionsas a slice of strings, the test can now handle scenarios where multiple permissions need to be specified.Improved response validation: The updated logic checks if the response contains entity IDs associated with the specific permission. If the permission exists, it extracts the corresponding IDs into a new variable,
responseIdsForPermission, which is then compared against the expected values. This ensures a more accurate and comprehensive validation of the response based on the granted permissions.These changes align with the overall objective of enhancing the functionality to support a list of permissions and provide a more flexible and robust testing approach.
integration-test/usecases/facebook_groups_test.go (3)
114-116: Approve the change to support a list of permissions.The modification from
PermissiontoPermissionsaligns with the objective of supporting a list of permissions in thelookup-entityfeature. Accepting a slice of strings provides flexibility to specify multiple permissions in thePermissionLookupEntityRequest, enhancing the functionality of entity filtering.
120-124: Approve the change to retrieve entity IDs for a specific permission.The introduction of the
responseIdsForPermissionvariable and the logic to retrieve the entity IDs for a specific permission enhances the test case. It ensures that the test accurately verifies the expected entity IDs based on the permission context, accommodating scenarios where the response may contain IDs for multiple permissions. This change improves the robustness and precision of the test case.
Line range hint
1-200: Remaining code looks good.The remaining code in the file consists of well-structured and comprehensive test cases for the Facebook Groups sample. It covers various scenarios related to entity filtering, subject filtering, and permission checks. The code follows a consistent pattern and utilizes helper functions effectively. No further changes are necessary for the remaining code.
internal/engines/bulk.go (4)
57-57: LGTM!The addition of the
permissionparameter to thecallbackfunction signature enhances the information provided during the callback execution, aligning with the goal of improving permission handling.
79-79: Looks good!Updating the
NewBulkCheckerfunction to accept the modifiedcallbacksignature ensures proper initialization of theBulkCheckerwith the enhanced callback.
223-225: Great improvement!Passing the specific
permissionto the callback and differentiating between entity and subject checks enhances the granularity of information provided and allows for more effective handling of permission checks based on the type of check being performed. These changes align with the goal of improving permission handling.
264-279: Efficient handling of multiple permissions!The modifications to the
Publishmethod to iterate over multiple permissions and check them against thepermissionChecksmap improve the efficiency of handling multiple permissions in a single publish operation. This ensures that only valid permissions are processed, preventing unnecessary processing of invalid permissions.internal/engines/schemaBasedEntityFilter.go (4)
39-39: LGTM! The changes enhance the efficiency and robustness of the permission checking process.The introduction of the
permissionChecksparameter and the updated logic for checking if an entity reference matches the subject are valuable improvements:
- The
permissionChecksmap helps avoid duplicate permission evaluations, improving overall efficiency.- Iterating over all entity references and breaking the loop upon a match ensures that unnecessary checks are prevented, enhancing the robustness of the matching process.
Great work on these enhancements!
Also applies to: 42-55
118-118: The changes improve the utilization of thepermissionChecksmap and enhance the pagination logic.The updates to the
relationEntrancemethod are beneficial:
- Accepting the
permissionChecksparameter allows the method to utilize the thread-safe map for tracking previously checked permissions, avoiding duplicate evaluations.- The updated pagination logic ensures that the correct pagination settings are applied based on the type of the entity references provided. This improves the efficiency of querying relationships by using cursor pagination with sorting by "entity_id" when appropriate.
These changes enhance the overall performance and effectiveness of the method.
Also applies to: 144-149, 183-183
203-204: The updates to thetupleToUserSetEntrancemethod mirror the improvements made to therelationEntrancemethod.The changes to the
tupleToUserSetEntrancemethod are similar to those made to therelationEntrancemethod and provide the same benefits:
- Utilizing the
permissionChecksmap to avoid duplicate permission evaluations.- Enhancing the pagination logic to apply the correct settings based on the type of the entity references provided, improving the efficiency of querying relationships.
These updates strengthen the overall performance and effectiveness of the method.
Also applies to: 231-236, 270-270
285-285: The updates to thelmethod improve the utilization of thepermissionChecksmap and enhance the publishing logic.The changes to the
lmethod are beneficial:
- Accepting the
permissionChecksparameter allows the method to utilize the thread-safe map for tracking previously checked permissions, avoiding duplicate evaluations.- The updated publishing logic ensures that the found entity is published only if it matches one of the requested entity references. This prevents unnecessary publishing of entities and improves the accuracy of the results.
These modifications enhance the overall performance and effectiveness of the method.
Also applies to: 308-316, 323-333
internal/engines/lookup.go (4)
89-98: The updated error handling logic for validating permissions looks good!I like how the code now iterates over all permissions provided in the request and validates each one using the schema walker. Only the valid permissions are proceeded with, which is a more robust approach.
The handling of the
schema.ErrUnimplementederror usingMassEntityFilterand the handling of other errors by returning them directly is a clean way to manage different error scenarios.Also applies to: 110-124
288-309: The changes to handle theschema.ErrUnimplementederror inLookupSubjectlook good!Using the
MassSubjectFilterwith a callback function to store thesubjectIDif it passes the permission check is a clean way to handle the scenario when the schema walk is unimplemented.Falling back to the
SchemaBasedSubjectFilterwhen there are no errors during the permission check walk ensures that the function works correctly for both implemented and unimplemented schema walk scenarios.
Line range hint
168-178: The changes to stream results for each permission look good!The updated callback function now sends the
entityID,permission, andtokenfor each entity that passes the permission check, which allows for streaming results per permission.Performing the permission check walk for each permission separately and handling errors based on the error type is a good approach to manage the streaming logic for different scenarios.
To verify that the
LookupEntityStreamfunction streams results for each permission correctly, you can use the following script:Also applies to: 194-234
Verification successful
The changes to
LookupEntityStreamfunction have been implemented correctly and stream results for each permission as expected.The verification process confirms that:
- The callback function correctly handles streaming of results for each entity and permission.
- The permission check is performed separately for each permission, allowing for individual processing and error handling.
- Proper error handling is in place for both the schema walk and the entity filtering process.
These changes ensure that the function can effectively stream results for each permission, handling various scenarios and potential errors appropriately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `LookupEntityStream` streams results for each permission correctly. # Test: Search for the updated callback function signature and logic. # Expect: The callback should send `entityID`, `permission`, and `token` for each entity that passes the check. rg --type go -A 10 $'callback := func\\(entityID, permission string, token string\\)' # Test: Search for the permission check walk performed for each permission. # Expect: The walk should be performed for each permission separately. rg --type go -A 20 $'for _, permission := range request\\.GetPermissions\\(\\)'Length of output: 4383
54-54: LGTM! The changes to handle multiple permissions look good.The updated logic to use a map structure
entityIDsByPermissionfor associating permissions with entity IDs is a cleaner approach. The callback function has been appropriately modified to handle the additionalpermissionparameter.To verify that the
LookupEntityfunction handles multiple permissions correctly, you can use the following script:Also applies to: 64-71
Verification successful
Verification successful: Multiple permissions handling implemented correctly
The changes to the
LookupEntityfunction have been verified to correctly handle multiple permissions:
- The
entityIDsByPermissionmap is properly initialized to store entity IDs for each permission.- The callback function signature has been updated to include the
permissionparameter.- The implementation uses a mutex for thread-safe access to the shared map.
- Entity IDs are correctly associated with their respective permissions in the map.
- The streaming response implementation includes the permission in each response.
These changes ensure that the function can process and store entity IDs for multiple permissions separately and efficiently.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `LookupEntity` handles multiple permissions correctly. # Test: Search for the `entityIDsByPermission` map usage. # Expect: The map should be initialized and used to store entity IDs by permission. rg --type go -A 5 $'entityIDsByPermission := make\\(map\\[string\\]\\*base\\.EntityIds\\)' # Test: Search for the updated callback function signature and logic. # Expect: The callback should accept `entityID`, `permission`, and `token` parameters and store entity IDs in the map. rg --type go -A 10 $'callback := func\\(entityID, permission string, token string\\)'Length of output: 1921
internal/schema/linkedSchema.go (4)
Line range hint
141-176: LGTM!The function correctly handles different reference types and recursively finds entrances while avoiding cycles.
Line range hint
192-226: LGTM!The function correctly finds entrances for relation references, handling both direct and indirect entrances by recursively searching child relations.
Line range hint
246-323: LGTM!The function correctly finds entrances for different types of leaf references, recursively searching through tuple set and computed user set relations as needed.
Line range hint
340-360: LGTM!The function correctly finds entrances for rewrite references by recursively searching each child and combining the results.
pkg/cmd/validate.go (2)
367-371: LGTM!The result validation logic correctly handles both the presence and absence of returned entities, ensuring that the actual results match the expected values.
377-378: LGTM!The updated failure logging correctly accesses the IDs from the permission key in the result, aligning with the new result structure.
Including the query, expected values, and actual values in the failure message is helpful for debugging purposes.
internal/invoke/invoke.go (2)
248-248: LGTM!The change from
"permission"to"permissions"and fromStringValuetoStringSliceValueaccurately reflects the shift to handling multiple permissions simultaneously. This improves the granularity of the logged data and enables better tracking and analysis of permissions associated with entity lookups.
295-295: LGTM!The change from
"permission"to"permissions"and fromStringValuetoStringSliceValueinLookupEntityStreamis consistent with the modification made inLookupEntity. It accurately reflects the shift to handling multiple permissions simultaneously for the streaming variant of the lookup. This improves the granularity of the logged data and enables better tracking and analysis of permissions associated with entity lookup streams.internal/schema/linkedSchema_test.go (15)
188-192: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
254-258: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
324-328: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
381-385: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
470-474: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
519-523: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
570-574: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
629-633: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
679-683: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
731-735: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
773-777: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
827-831: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
872-876: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
925-929: LGTM!The test case has been updated to match the new
RelationshipLinkedEntrancesfunction signature that accepts a slice of*base.RelationReference.
959-1037: Great job adding a new test case!The new test case (Case 19) is a valuable addition to the test suite. It covers a scenario where the
RelationshipLinkedEntrancesfunction is expected to return linked entrances for both documents and organizations based on the provided schema and relationship references.The test case is well-structured, easy to understand, and verifies the expected behavior of the function.
internal/engines/lookup_test.go (18)
Line range hint
71-178: LGTM!The addition of the
permissionsfield in thefilterstruct and the corresponding changes to iterate over the permissions in the test case improve the robustness of the test by allowing validation of multiple permissions in a single test case.
Line range hint
180-324: Looks good!The test case follows the same structure as
Drive Sample: Case 1and tests the lookup entity functionality with a different set of test data.
Line range hint
325-465: LGTM!The test case is structured similarly to the previous test cases and tests the lookup entity functionality with a different set of test data.
Line range hint
466-606: Looks good!The test case is structured similarly to the previous test cases and tests the lookup entity functionality with a different set of test data.
Line range hint
608-732: LGTM!The test case is structured similarly to the previous test cases and tests the lookup entity functionality with a different set of test data.
Line range hint
734-880: Looks good!The test case is structured similarly to the previous test cases but includes contextual data. It tests the lookup entity functionality with a different set of test data.
Line range hint
882-1070: LGTM!The test case is structured similarly to the previous test cases and tests the lookup entity functionality with a larger set of test data.
1072-1232: Looks good!The test case is structured similarly to the previous test cases and tests the lookup entity functionality with invalid and duplicate permissions to ensure the system handles such scenarios correctly.
1234-1336: LGTM!The test case is structured similarly to the previous test cases and tests the lookup entity functionality with all invalid permissions to ensure the system handles such scenarios correctly and returns an error.
Line range hint
1472-1651: Looks good!The test case is structured similarly to the drive sample test cases and tests the lookup entity functionality with the Facebook group sample schema and a different set of test data.
Line range hint
1653-1888: LGTM!The test case is structured similarly to the previous Facebook group sample test case but includes contextual data. It tests the lookup entity functionality with a different set of test data.
Line range hint
1890-2027: Looks good!The test case is structured similarly to the previous Facebook group sample test cases and tests the lookup entity functionality with pagination by making multiple requests with a continuous token and page size.
Line range hint
2029-2178: LGTM!The test case is structured similarly to the previous Facebook group sample pagination test case and tests the lookup entity functionality with pagination and a different set of test data.
Line range hint
2229-2370: Looks good!The test case is structured similarly to the previous sample test cases and tests the lookup entity functionality with the Google Docs sample schema and a different set of test data.
Line range hint
2372-2581: LGTM!The test case is structured similarly to the previous Google Docs sample test case and tests the lookup entity functionality with a different set of test data.
Line range hint
2583-2699: Looks good!The test case is structured similarly to the previous Google Docs sample test cases and tests the lookup entity functionality with a larger set of test data.
Line range hint
2701-2833: LGTM!The test case is structured similarly to the previous Google Docs sample test cases and tests the lookup entity functionality with pagination by making multiple requests with a continuous token and page size.
Line range hint
2890-3018: Looks good!The test case is structured similarly to the previous sample test cases and tests the lookup entity functionality with the Weekday sample schema and a different set of test data, including relationships, attributes, and filters.
proto/base/v1/service.proto (6)
1088-1089: LGTM:entity_idsfield type changed tomap<string, EntityIds>.This allows for a more organized representation of entity identifiers associated with specific keys.
1095-1098: Looks good: NewEntityIdsmessage introduced.Encapsulating the list of entity IDs in a separate message improves clarity and structure of the response.
1108-1109: Approved: Newpermissionfield added.Specifying the permission associated with the
entity_idenhances the response's informational content.
1026-1035: Skipping duplicate review.The
PermissionLookupEntityRequestfunction and its changes have already been reviewed earlier in this file.
1135-1135: Breaking change:entity_referencefield changed torepeated RelationReference entity_references.This allows including multiple entity references in the request, broadening the filtering capabilities.
Verify all usages of the
entity_referencefield have been updated to useentity_references:Verification successful
Verification successful:
entity_referencefield correctly updated toentity_referencesThe codebase search confirms that the
entity_referencefield has been consistently updated toentity_referencesas a repeated RelationReference. The change is present in proto/base/v1/service.proto:repeated RelationReference entity_references = 3 [json_name = "entity_references"];No occurrences of the old
entity_referencefield were found, indicating a complete update across the proto files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `entity_reference` field have been updated to `entity_references`. # Test: Search for the old `entity_reference` field usage. Expect: No results. rg --type proto 'entity_reference = ' # Test: Search for the new `repeated RelationReference entity_references` field usage. Expect: Only valid usages. rg --type proto -A 5 'repeated RelationReference entity_references'Length of output: 169
Script:
#!/bin/bash # Description: Search for both old and new field names in proto files # Find all proto files proto_files=$(fd -e proto) # Search for 'entity_reference' (old field name) echo "Searching for 'entity_reference':" rg 'entity_reference\s*=' -n --context 2 $proto_files echo "\n---\n" # Search for 'entity_references' (new field name) echo "Searching for 'entity_references':" rg 'entity_references\s*=' -n --context 2 $proto_filesLength of output: 975
1026-1035: Breaking change:permissionfield replaced withrepeated string permissions.The change enhances flexibility by allowing multiple permissions per request. The updated validation rules and permission pattern are reasonable.
Verify all usages of the
permissionfield have been updated to usepermissions:Verification successful
Verification successful:
permissionfield correctly replaced withrepeated string permissionsThe change has been implemented as expected in
proto/base/v1/service.proto. No instances of the oldpermissionfield were found in any proto files, and the newpermissionsfield is only present in the file under review.
- The new field includes the correct validation rules (min_items: 1, max_items: 100, pattern: "^[a-zA-Z_][a-zA-Z0-9_]*$").
- No other proto files appear to use either the old or new field, suggesting this change is isolated.
While proto files have been verified, it's recommended to manually review any non-proto files (e.g., Go, Python, or JavaScript) that might reference this field to ensure complete update coverage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `permission` field have been updated to `permissions`. # Test: Search for the old `permission` field usage. Expect: No results. rg --type proto 'permission: "?[a-zA-Z_]{1,64}"?' # Test: Search for the new `repeated string permissions` field usage. Expect: Only valid usages. rg --type proto -A 5 'repeated string permissions'Length of output: 164
Script:
#!/bin/bash # Description: Verify all usages of the `permission` field have been updated to `permissions` in proto files. # Test: Search for the old `permission` field usage. Expect: No results. echo "Searching for old 'permission' field usage:" fd -e proto | xargs rg 'permission: "?[a-zA-Z_]{1,64}"?' # Test: Search for the new `repeated string permissions` field usage. Expect: Only valid usages. echo "Searching for new 'permissions' field usage:" fd -e proto | xargs rg -A 5 'repeated string permissions'Length of output: 710
docs/api-reference/openapiv2/apidocs.swagger.json (3)
2052-2064: LGTM!The new
EntityIdsobject definition looks good. It provides a clean way to represent a list of entity IDs in API responses.
2626-2629: LGTM!The new
permissionproperty inPermissionLookupEntityStreamResponselooks good. It provides useful context about the permission associated with each streamed entity ID.
2606-2608: Verify theentity_idsproperty change across the codebase.Changing
entity_idsfrom an array of strings to an object withEntityIdsvalues is a good refactor for better structure. However, ensure that this change has been consistently updated across the codebase, especially in API clients that parse this response.Run the following script to verify references to the
entity_idsproperty:Verification successful
Approve the
entity_idschange, server-side implementation is consistentThe change of
entity_idsfrom an array to a map has been consistently implemented in the server-side code. ThePermissionLookupEntityResponsestruct and theLookupEntityfunction both use the new map structure correctly. However, as client-side code was not found in this repository, it's recommended to ensure that any separate client implementations are updated to handle the new response format.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `entity_ids` property have been updated to handle the new object structure. # Test: Search for accesses to the `entity_ids` property. Expect: Only references that handle it as an object. rg --type go --type js --type ts --type python --type java $'response\.entity_ids' -A 5Length of output: 119
Script:
#!/bin/bash # Description: Search for references to `entity_ids` in potential client code # Search for "entity_ids" in code files, excluding common non-code extensions rg "entity_ids" --type-add 'code:*.{js,ts,jsx,tsx,go,java,kt,swift,cs,cpp,h}' -t code -C 5 # Search specifically for object-like access to entity_ids rg "entity_ids\[" --type-add 'code:*.{js,ts,jsx,tsx,go,java,kt,swift,cs,cpp,h}' -t code -C 5 # Search for any mention of PermissionLookupEntityResponse rg "PermissionLookupEntityResponse" --type-add 'code:*.{js,ts,jsx,tsx,go,java,kt,swift,cs,cpp,h}' -t code -C 5Length of output: 33939
docs/api-reference/apidocs.swagger.json (2)
2287-2291: Breaking API change:permissionis nowpermissionsarray inPermissionLookupEntityRequest.This change allows clients to specify multiple permissions in the entity lookup request, providing more flexibility.
Verify that this change from
permissiontopermissionsis consistently applied across all relevant API definitions, documentation and client SDKs.Specifically, check:
- Other API endpoints that accept
PermissionLookupEntityRequest- API reference documentation
- Client SDK method signatures in all languages
- OpenAPI spec in this file
2640-2643: Breaking API change:permissionis nowpermissionsarray inPermissionLookupEntityStreamResponse.This change is consistent with the modification made to
PermissionLookupEntityRequest. It allows the stream response to include multiple permissions for each entity ID.Verify that this change from
permissiontopermissionsis consistently applied across all relevant API definitions, documentation and client SDKs.Specifically, check:
- Other API endpoints that return
PermissionLookupEntityStreamResponse- API reference documentation
- Client SDK method signatures and response types in all languages
- OpenAPI spec in this file
pkg/pb/base/v1/service.pb.validate.go (4)
1570-1605: Validation looks good for thePermissionsfield.The validation logic comprehensively checks the
Permissionsfield by:
- Enforcing the number of permissions to be within a reasonable range of 1 to 100.
- Limiting the length of each permission string to 64 bytes.
- Validating each permission string against a well-defined regex pattern.
The combination of these checks should ensure the integrity and validity of the
Permissionsdata.
1781-1781: The updated regex pattern for validating permission strings looks good.The new pattern
^[a-zA-Z_][a-zA-Z0-9_]*$enforces a reasonable format for permission names:
- The permission must start with a letter or underscore.
- It can only contain letters, digits, and underscores.
- It must have at least one character (empty strings are not allowed).
Allowing an initial underscore supports common naming conventions for constants and special identifiers.
1925-1970: The added validation for theEntityIdsfield inPermissionLookupEntityResponselooks good.The validation logic comprehensively handles the
EntityIdsmap by:
- Iterating over the map entries in a deterministic order by sorting the keys.
- Validating each embedded message value using the available
ValidateAllandValidatemethods.- Collecting any validation errors and returning them as a structured
EntityIdsValidationErrortype.The support for both
ValidateAllandValidatemethods allows the embedded message types to define their own granular validation logic.Using a structured error type provides rich error reporting, including the specific map key that failed validation.
This validation follows the established patterns in the codebase and ensures the integrity of the
EntityIdsdata.
2054-2152: The addition of theEntityIdstype and its associated validation error types looks good.The
EntityIdstype follows the validation pattern used for other message types in the codebase:
- It defines
ValidateandValidateAllmethods for performing validation checks.- The
validatemethod is a shared implementation used by both validation methods.- Associated validation error types
EntityIdsMultiErrorandEntityIdsValidationErrorare defined to represent validation failures.Although there are currently no specific validation rules defined for
EntityIds, the presence of the validation methods and error types allows for easy extension in the future.The structured validation error types (
EntityIdsMultiErrorandEntityIdsValidationError) provide detailed information about validation failures, including the field, reason, cause, and key.This addition aligns with the existing validation architecture in the codebase.
| "permissions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Update API clients to use an array of permission names.
Changing the permission property to an array is a good enhancement to allow checking multiple permissions in a single request.
Please ensure that all API clients are updated to provide an array of permission names when calling the LookupEntity API.
| "permissions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Update streaming API clients to handle multiple permissions per entity ID.
Changing the permission property to an array is a good enhancement to allow checking multiple permissions in a single streaming request.
Please ensure that all streaming API clients are updated to handle multiple permissions associated with each entity ID in the response stream.
|
Commit history is a bit confusing as I had to pass all changes to master in another branch first to preserve @ankitsheoran1's contributions. Please squash merge once everything is ready. |
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (20)
internal/servers/permissionServer.go (2)
112-131: LGTM! Consider adding a brief method description.The
LookupEntitiesmethod is well-implemented and consistent with other methods in the file. It correctly handles tracing, request validation, invocation, and error handling.Consider adding a brief description of the method's purpose, similar to other methods in the file. For example:
// LookupEntities - Performs a bulk lookup of entities for multiple permissions
133-152: LGTM! Consider adding a brief method description.The
LookupEntitiesStreammethod is well-implemented and consistent with other streaming methods in the file. It correctly handles tracing, request validation, invocation, and error handling for the streaming scenario.Consider adding a brief description of the method's purpose, similar to other methods in the file. For example:
// LookupEntitiesStream - Performs a streaming bulk lookup of entities for multiple permissionsinternal/engines/utils.go (1)
397-409: LGTM! Consider adding a comment for clarity.The
ConvertToPermissionsLookupEntityRequestfunction correctly maps all fields from the input to the output, including the conversion of the singlePermissionto a slice ofPermissions. This implementation maintains backwards compatibility while supporting the new feature of multiple permissions.Consider adding a brief comment explaining the purpose of this function, especially highlighting the conversion of the single permission to a slice. For example:
// ConvertToPermissionsLookupEntityRequest converts a PermissionLookupEntityRequest to a PermissionsLookupEntityRequest, // maintaining backwards compatibility by converting the single Permission to a slice of Permissions. func ConvertToPermissionsLookupEntityRequest(req *base.PermissionLookupEntityRequest) *base.PermissionsLookupEntityRequest { // ... (existing implementation) }docs/api-reference/openapiv2/apidocs.swagger.json (1)
2165-2176: Fix typo in EntityIds definition.There's a minor typo in the
EntityIdsdefinition:- "title": "entitiy Ids" + "title": "entity Ids"Please correct the spelling of "entity" in the title.
internal/engines/lookup.go (3)
174-179: Consider initializing 'entrances' more efficientlyWhen building the
entrancesslice for multiple permissions, you can initialize the slice with a predefined capacity to improve performance.Modify the initialization as follows:
entrances := make([]*base.Entrance, 0, len(request.GetPermissions())) for _, permission := range request.GetPermissions() { entrances = append(entrances, &base.Entrance{ Type: request.GetEntityType(), Value: permission, }) }
Line range hint
222-230: Ensure proper error handling in callback functionIn the
LookupEntityStreamfunction's callback, if an error occurs while sending the response, it silently fails due to thereturnstatement inside the callback. This may lead to unintended behavior.Consider propagating the error to ensure that the stream is properly handled:
callback := func(entityID, permission, token string) { err := server.Send(&base.PermissionLookupEntityStreamResponse{ EntityId: entityID, - Permission: permission, ContinuousToken: token, }) // If there is an error in sending the response, the function will return if err != nil { - return + // Handle the error appropriately, e.g., log or set an error variable + // You might want to cancel the context or handle according to your error strategy } }🧰 Tools
🪛 GitHub Check: Test with Coverage
[failure] 225-225:
unknown field Permission in struct literal of type basev1.PermissionLookupEntityStreamResponse
Line range hint
367-404: Consider optimizing 'LookupSubject' for concurrency and performanceIn the
LookupSubjectfunction, while the mutex protects concurrent writes to thesubjectIDsslice, consider using concurrent-safe data structures or channels if you expect high concurrency.You can explore the use of buffered channels or concurrent maps to improve performance under heavy load.
internal/engines/entityFilter.go (7)
38-38: Update function documentation to include the newpermissionChecksparameter.The function
EntityFilternow accepts a new parameterpermissionChecks, but the function documentation does not mention it. Please update the comment to explain the purpose and usage of this parameter.
220-220: Update function documentation to include the newpermissionChecksparameter.The function
relationEntrancenow accepts a new parameterpermissionChecks, but the function documentation does not mention it. Please update the comment to explain the purpose and usage of this parameter.
317-318: Update function documentation to include the newpermissionChecksparameter.The function
tupleToUserSetEntrancenow accepts a new parameterpermissionChecks, but the function documentation does not mention it. Please update the comment to explain the purpose and usage of this parameter.
409-409: Update function documentation to include the newpermissionChecksparameter.The function
ltnow accepts a new parameterpermissionChecks, but the function documentation does not mention it. Please update the comment to explain the purpose and usage of this parameter.
Line range hint
204-209: Possible variable capture issue in goroutine.Within the loop, the variable
currentis used inside the goroutine. Sincecurrentmay change before the goroutine executes, this could lead to incorrect values being used. To ensure the correct value is captured, assigncurrentto a new variable inside the loop.Apply this diff to fix the issue:
for it.HasNext() { current, ok := it.GetNext() if !ok { break } + curr := current g.Go(func() error { return engine.la(ctx, request, &base.Entity{ Type: entrance.TargetEntrance.GetType(), - Id: current.GetEntity().GetId(), + Id: curr.GetEntity().GetId(), }, visits, g, publisher, entityChecks) }) }
Line range hint
296-301: Possible variable capture issue in goroutine.The variable
currentused inside the goroutine may change before the goroutine runs, leading to incorrect behavior. Capture the current value by assigning it to a local variable.Apply this diff to fix the issue:
for it.HasNext() { current, ok := it.GetNext() if !ok { break } + curr := current g.Go(func() error { return engine.lt(ctx, request, &base.EntityAndRelation{ Entity: &base.Entity{ Type: curr.GetEntity().GetType(), Id: curr.GetEntity().GetId(), }, Relation: curr.GetRelation(), }, visits, g, publisher, permissionChecks) }) }
Line range hint
394-399: Possible variable capture issue in goroutine.There's a potential variable capture issue with
currentinside the goroutine. Assigncurrentto a new variable within the loop to safely capture its value.Apply this diff to fix the issue:
for it.HasNext() { current, ok := it.GetNext() if !ok { break } + curr := current g.Go(func() error { return engine.lt(ctx, request, &base.EntityAndRelation{ Entity: &base.Entity{ Type: entrance.TargetEntrance.GetType(), - Id: current.GetEntity().GetId(), + Id: curr.GetEntity().GetId(), }, Relation: entrance.TargetEntrance.GetValue(), }, visits, g, publisher, permissionChecks) }) }internal/invoke/invoke.go (3)
52-53: Add documentation comments for new interface methods.Consider adding comments for the newly added methods
LookupEntitiesandLookupEntitiesStreamin theLookupinterface to enhance code readability and maintainability.
373-373: Assign result to the named return variableresponse.Since
responseis a named return variable, you can assign the result directly to it instead of declaring a new variableresp. This enhances code readability.Apply this diff:
- resp, err := invoker.lo.LookupEntities(ctx, request) + response, err = invoker.lo.LookupEntities(ctx, request)
420-420: Simplify error handling inLookupEntitiesStream.The variable
respis assigned but not used elsewhere. Since the method returns an error, you can directly return the result ofinvoker.lo.LookupEntitiesStream(...).Apply this diff:
- resp := invoker.lo.LookupEntitiesStream(ctx, request, server) ... - return resp + err = invoker.lo.LookupEntitiesStream(ctx, request, server) ... + return errOr simplify further:
- resp := invoker.lo.LookupEntitiesStream(ctx, request, server) ... - return resp + return invoker.lo.LookupEntitiesStream(ctx, request, server)proto/base/v1/service.proto (2)
395-396: Clarify method description forLookupEntitiesThe description "It is used to retrieve entities by its identifier." may not accurately reflect the purpose of the
LookupEntitiesmethod, which retrieves entities based on permissions for a given subject. Consider updating the description for clarity.
1343-1343: Fix typo in commentThere's a typo in the comment: "entitiy Ids" should be "entity Ids".
Apply this diff to fix the typo:
- // entitiy Ids + // entity Idspkg/pb/base/v1/service.pb.validate.go (1)
Line range hint
1611-2663: Avoid Modifying Generated Code DirectlyThis file is auto-generated by
protoc-gen-validate(as indicated by the comment at the top:// Code generated by protoc-gen-validate. DO NOT EDIT.). Any manual changes made here will be overwritten the next time the code is regenerated. To introduce changes, please modify the corresponding.protofiles and regenerate the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
pkg/pb/base/v1/health.pb.gois excluded by!**/*.pb.gopkg/pb/base/v1/health_grpc.pb.gois excluded by!**/*.pb.gopkg/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 (13)
- docs/api-reference/apidocs.swagger.json (4 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (4 hunks)
- internal/engines/bulk.go (5 hunks)
- internal/engines/entityFilter.go (18 hunks)
- internal/engines/lookup.go (9 hunks)
- internal/engines/utils.go (1 hunks)
- internal/invoke/invoke.go (4 hunks)
- internal/schema/linkedSchema.go (11 hunks)
- internal/servers/permissionServer.go (1 hunks)
- pkg/cmd/validate.go (1 hunks)
- pkg/pb/base/v1/health.pb.validate.go (0 hunks)
- pkg/pb/base/v1/service.pb.validate.go (8 hunks)
- proto/base/v1/service.proto (5 hunks)
💤 Files with no reviewable changes (1)
- pkg/pb/base/v1/health.pb.validate.go
🧰 Additional context used
🪛 GitHub Check: Test with Coverage
internal/engines/lookup.go
[failure] 67-67:
undefined: entityIDsByPermission
[failure] 69-69:
undefined: entityIDsByPermission
[failure] 71-71:
undefined: entityIDsByPermission
[failure] 225-225:
unknown field Permission in struct literal of type basev1.PermissionLookupEntityStreamResponse
🔇 Additional comments (20)
internal/servers/permissionServer.go (1)
112-152: Summary: New methods align well with PR objectivesThe addition of
LookupEntitiesandLookupEntitiesStreammethods to thePermissionServerstruct successfully implements the PR objective of adding support for a list of permissions in lookup-entity. These methods maintain consistency with the existing codebase in terms of structure, error handling, and tracing.The changes enhance the server's capability to handle bulk permission lookups, both for single response and streaming scenarios, which should improve the flexibility and efficiency of permission management in the system.
internal/engines/utils.go (2)
396-396: LGTM! Clear and informative comment.The added comment for the
IsRelationalfunction provides a clear and accurate description of its purpose. It helps developers understand the function's role in determining whether a permission corresponds to a relational attribute in the provided entity definition.
Line range hint
1-409: Overall assessment: Changes align with PR objectives and improve code quality.The changes in this file, particularly the addition of the
ConvertToPermissionsLookupEntityRequestfunction, directly support the PR objective of adding support for a list of permissions in lookup-entity. The function provides a clean way to convert single permission requests to multi-permission requests, maintaining backwards compatibility.The comment update for the
IsRelationalfunction improves code documentation, which enhances overall code quality and maintainability.These changes are well-implemented and contribute positively to the codebase.
docs/api-reference/openapiv2/apidocs.swagger.json (7)
819-875: LGTM! New endpoint for lookup-entities is well-structured.The new endpoint
/v1/tenants/{tenant_id}/permissions/lookup-entitiesis correctly defined and follows the structure of other endpoints in the file. The request bodyLookupEntitiesBodyand responsePermissionsLookupEntityResponseare properly referenced.
2389-2393: Update API clients to use an array of permission names.The
permissionsfield in theLookupEntitiesBodyhas been changed to an array of strings, allowing multiple permissions to be specified in a single request. This is a good enhancement that provides more flexibility.
876-936: LGTM! New streaming endpoint for lookup-entities is well-structured.The new streaming endpoint
/v1/tenants/{tenant_id}/permissions/lookup-entities-streamis correctly defined and follows the structure of other streaming endpoints in the file. The request bodyLookupEntitiesStreamBodyand responsePermissionsLookupEntityStreamResponseare properly referenced.
2434-2438: Update streaming API clients to handle multiple permissions per entity ID.The
permissionsfield in theLookupEntitiesStreamBodyhas been changed to an array of strings, allowing multiple permissions to be specified in a single streaming request. This change is consistent with the non-streaming endpoint and provides more flexibility.
2915-2931: LGTM! PermissionsLookupEntityResponse structure supports multiple permissions.The
PermissionsLookupEntityResponsestructure has been updated to support multiple permissions per entity. Theentity_idsfield is now a map where the key is likely the permission name, and the value is anEntityIdsobject containing the list of entity IDs that have that permission.
2932-2948: LGTM! PermissionsLookupEntityStreamResponse structure supports multiple permissions.The
PermissionsLookupEntityStreamResponsestructure has been updated to include apermissionfield, which allows the stream to return results for multiple permissions. This is consistent with the changes made to support multiple permissions in the lookup-entities feature.
Line range hint
1-3551: Overall, the changes to support multiple permissions in lookup-entity operations are well-implemented.The new endpoints
/v1/tenants/{tenant_id}/permissions/lookup-entitiesand/v1/tenants/{tenant_id}/permissions/lookup-entities-streamhave been correctly added to the Swagger/OpenAPI specification. The changes to support multiple permissions are consistent across the new endpoints and related structures.Key points:
- Both new endpoints use an array of permissions in their request bodies, allowing multiple permissions to be checked in a single request.
- The response structures (
PermissionsLookupEntityResponseandPermissionsLookupEntityStreamResponse) have been updated to accommodate results for multiple permissions.- The changes align well with the PR objectives of adding support for a list of permissions in lookup-entity operations.
There's a minor typo in the
EntityIdsdefinition that should be corrected, but it doesn't affect the functionality of the API.These changes will require updates to API clients to take advantage of the new multiple permission checking capability.
docs/api-reference/apidocs.swagger.json (2)
819-936: Excellent addition of new permission lookup endpoints!The introduction of
/v1/tenants/{tenant_id}/permissions/lookup-entitiesand/v1/tenants/{tenant_id}/permissions/lookup-entities-streamendpoints significantly enhances the API's capabilities for permission management and entity lookups. These additions offer both synchronous and streaming options, providing flexibility for various client needs and potentially improving performance for large result sets.
Line range hint
937-1010: Approved update to the existing lookup-entity endpoint.The modification of the
/v1/tenants/{tenant_id}/permissions/lookup-entityendpoint, particularly the change in response structure toPermissionLookupEntityResponse, aligns well with the newly added endpoints. This update likely improves the consistency and functionality of the API.To ensure this change doesn't break existing integrations, please verify backward compatibility:
✅ Verification successful
[/run_scripts]
[/run_scripts]
Verified: No breaking changes detected in PermissionLookupEntityResponse.
The
PermissionLookupEntityResponsedefinition remains unchanged, ensuring that the update to the/v1/tenants/{tenant_id}/permissions/lookup-entityendpoint does not introduce any backward compatibility issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any breaking changes in the PermissionLookupEntityResponse diff_output=$(ast-grep --lang json --pattern '$_PermissionLookupEntityResponse: {$$$}' | diff - <(ast-grep --lang json --pattern '$_PermissionLookupEntityResponse: {$$$}' docs/api-reference/apidocs.swagger.json)) if [ -n "$diff_output" ]; then echo "Potential breaking changes detected in PermissionLookupEntityResponse:" echo "$diff_output" else echo "No breaking changes detected in PermissionLookupEntityResponse." fiLength of output: 1349
Script:
#!/bin/bash # Extract PermissionLookupEntityResponse from the current branch git show origin/main:docs/api-reference/apidocs.swagger.json | jq '.definitions.PermissionLookupEntityResponse' > /tmp/PermissionLookupEntityResponse_main.json # Extract PermissionLookupEntityResponse from the PR branch jq '.definitions.PermissionLookupEntityResponse' docs/api-reference/apidocs.swagger.json > /tmp/PermissionLookupEntityResponse_pr.json # Compare the two definitions diff_output=$(diff /tmp/PermissionLookupEntityResponse_main.json /tmp/PermissionLookupEntityResponse_pr.json) if [ -n "$diff_output" ]; then echo "PermissionLookupEntityResponse has been modified:" echo "$diff_output" else echo "No changes detected in PermissionLookupEntityResponse." fiLength of output: 2777
Script:
#!/bin/bash # Determine the default branch name default_branch=$(git remote show origin | awk '/HEAD branch/ {print $NF}') echo "Default branch detected: $default_branch" # Check if PermissionLookupEntityResponse exists in the default branch git show origin/$default_branch:docs/api-reference/apidocs.swagger.json > /tmp/apidocs_main.swagger.json 2>/dev/null if [ $? -ne 0 ]; then echo "Failed to retrieve docs/api-reference/apidocs.swagger.json from origin/$default_branch." echo "Please ensure the default branch name is correct and the file exists." exit 1 fi # Extract PermissionLookupEntityResponse from the default branch jq '.definitions.PermissionLookupEntityResponse' /tmp/apidocs_main.swagger.json > /tmp/PermissionLookupEntityResponse_main.json 2>/dev/null if [ $? -ne 0 ]; then echo "PermissionLookupEntityResponse does not exist in origin/$default_branch." echo "This indicates that it is a new addition and should be backward-compatible." exit 0 fi # Extract PermissionLookupEntityResponse from the PR branch jq '.definitions.PermissionLookupEntityResponse' docs/api-reference/apidocs.swagger.json > /tmp/PermissionLookupEntityResponse_pr.json 2>/dev/null if [ $? -ne 0 ]; then echo "PermissionLookupEntityResponse is missing in the PR branch." exit 1 fi # Compare the two definitions diff_output=$(diff /tmp/PermissionLookupEntityResponse_main.json /tmp/PermissionLookupEntityResponse_pr.json) if [ -n "$diff_output" ]; then echo "PermissionLookupEntityResponse has been modified:" echo "$diff_output" else echo "No changes detected in PermissionLookupEntityResponse." fiLength of output: 693
internal/engines/bulk.go (2)
79-79: Verify all usages ofNewBulkCheckerare updated to match the new signatureThe
NewBulkCheckerfunction now accepts apermissionparameter in itscallbackfunction argument. Please ensure that all calls toNewBulkCheckerare updated accordingly.Use the following script to find all calls to
NewBulkCheckerand review their arguments:#!/bin/bash # Description: Find all invocations of `NewBulkChecker`. rg --type go 'NewBulkChecker\(' -A 3
57-57: Ensure all callback invocations match the new signatureThe
callbackfunction inBulkCheckernow includes an additionalpermissionparameter. Please verify that all invocations of this callback are updated accordingly throughout the codebase.You can use the following script to find and review all invocations of the
callbackfunction:internal/schema/linkedSchema.go (2)
7-7: Approved: Importing thesyncpackage for concurrency controlThe addition of the
syncpackage is appropriate for handling concurrency with mutexes and wait groups introduced in the code.
375-388: Error handling infindEntranceRewritedoes not check for all possible error returnsIn the recursive calls within
findEntranceRewrite, errors returned byfindEntranceRewriteandfindEntranceLeafare checked, but processing continues even if an error is encountered in one of the iterations.Run the following script to identify any unhandled errors:
internal/engines/lookup.go (2)
129-210: Verify the correctness of the new 'LookupEntities' functionThe new
LookupEntitiesfunction introduces support for multiple permissions. Ensure that all logic correctly handles multiple permissions and that concurrent access to shared resources is properly synchronized.To verify that the function works as intended, consider reviewing unit tests or adding additional tests that cover multiple permissions scenarios.
280-339: Review the new 'LookupEntitiesStream' function for consistency and correctnessThe new
LookupEntitiesStreamfunction appears well-structured and aligns with the intended functionality of streaming entities for multiple permissions. Ensure that all edge cases are handled and that proper synchronization is maintained.internal/engines/entityFilter.go (1)
Line range hint
450-461: Ensure recursion terminates to prevent potential infinite loops.The recursive call to
engine.EntityFiltercould lead to infinite recursion if the termination conditions are not properly enforced. Verify that thevisitsmap and other mechanisms correctly prevent infinite loops.Run the following script to check for cycles in the call graph:
pkg/pb/base/v1/service.pb.validate.go (1)
Line range hint
1611-2663: Validation Logic Appears CorrectThe added validation functions for
PermissionsLookupEntityRequest,PermissionsLookupEntityResponse, andPermissionsLookupEntityStreamResponsecorrectly implement the necessary checks for field lengths, required fields, and regex patterns according to the proto definitions.
internal/engines/bulk.go
Outdated
| bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct) | ||
| bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct) |
There was a problem hiding this comment.
Remove duplicate callback invocation
Similarly, the bc.callback function is called twice with the same arguments on lines 226 and 227. Please remove the duplicate call.
Apply this diff to remove the duplicated line:
226 bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct)
-227 bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct)📝 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.
| bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct) | |
| bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct) | |
| bc.callback(listCopy[processedIndex].Request.GetSubject().GetId(), req.Request.GetPermission(), ct) |
internal/engines/bulk.go
Outdated
| bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct) | ||
| bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct) |
There was a problem hiding this comment.
Remove duplicate callback invocation
The bc.callback function is called twice with the same arguments on lines 223 and 224. This appears to be an unintentional duplication. Please remove the duplicate invocation.
Apply this diff to remove the duplicated line:
223 bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct)
-224 bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct)📝 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.
| bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct) | |
| bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct) | |
| bc.callback(listCopy[processedIndex].Request.GetEntity().GetId(), req.Request.GetPermission(), ct) |
| Context: context, | ||
| }, | ||
| Result: result, | ||
| func (s *BulkEntityPublisher) Publish(entity *base.Entity, metadata *base.PermissionCheckRequestMetadata, context *base.Context, result base.CheckResult, permissionChecks *VisitsMap) { |
There was a problem hiding this comment.
💡 Codebase verification
Update Required: Publish Method Invocations Missing New Parameter
Several invocations of the Publish method do not include the new permissionChecks *VisitsMap parameter:
-
internal/engines/massSubjectFilter.go
- Lines where
publisher.Publish(&base.Subject{is called.
- Lines where
-
internal/engines/lookup.go
- Lines where
publisher.Publish(&base.Subject{is called.
- Lines where
These calls need to be updated to include the permissionChecks argument to match the new method signature.
🔗 Analysis chain
Verify all invocations of Publish are updated to the new signature
The Publish method now includes a new parameter permissionChecks *VisitsMap. Please ensure that all invocations of this method are updated accordingly.
Use the following script to find all calls to Publish and review their parameter lists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all invocations of `BulkEntityPublisher.Publish` and check their parameter lists.
rg --type go 'BulkEntityPublisher\)\.Publish\(' -A 1
Length of output: 2423
Script:
#!/bin/bash
# Description: Find all invocations of `Publish` method and check their parameter lists.
rg --type go '\.Publish\(' -A 1
Length of output: 1228
internal/schema/linkedSchema.go
Outdated
| mu.Lock() | ||
| mu.Lock() |
There was a problem hiding this comment.
Duplicate mu.Lock() calls causing potential deadlock
The mutex mu is locked twice consecutively at lines 143 and 144 without an intervening unlock. This will cause a deadlock as the second mu.Lock() call waits indefinitely for the mutex to be unlocked.
Proposed fix: Remove the duplicate mu.Lock() call
func (g *LinkedSchemaGraph) findEntrance(target, source *base.Entrance, visited map[string]struct{}, mu *sync.Mutex) ([]*LinkedEntrance, error) {
key := utils.Key(target.GetType(), target.GetValue())
- mu.Lock()
mu.Lock()
if _, ok := visited[key]; ok {
mu.Unlock()
return nil, nil
}
visited[key] = struct{}{}
mu.Unlock()📝 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.
| mu.Lock() | |
| mu.Lock() | |
| mu.Lock() |
| func (g *LinkedSchemaGraph) LinkedEntrances(targets []*base.Entrance, source *base.Entrance) ([]*LinkedEntrance, error) { | ||
| visited := map[string]struct{}{} | ||
| var mu sync.Mutex | ||
| var wg sync.WaitGroup | ||
| errChan := make(chan error, len(targets)) | ||
| done := make(chan struct{}) | ||
| totalEntries := make([]*LinkedEntrance, 0) | ||
| for _, target := range targets { | ||
| wg.Add(1) | ||
| go func(target *base.Entrance) { | ||
| defer wg.Done() | ||
| entries, err := g.findEntrance(target, source, visited, &mu) | ||
| if err != nil { | ||
| errChan <- err | ||
| } | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| totalEntries = append(totalEntries, entries...) | ||
| }(target) | ||
| } | ||
|
|
||
| return entries, nil | ||
| go func() { | ||
| wg.Wait() | ||
| close(done) | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| // All goroutines finished | ||
| return totalEntries, nil | ||
| case err := <-errChan: | ||
| // Return on the first error encountered | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential goroutine leak and unhandled errors in LinkedEntrances
When an error occurs in one of the goroutines and is sent to errChan, the select statement returns immediately. However, the other goroutines continue to run and may attempt to send errors to errChan, which could lead to a panic if the channel is unbuffered or closed. Additionally, these goroutines may continue processing unnecessarily.
Suggestion: Implement context cancellation to manage goroutines
Consider using a context.Context to signal cancellation to all goroutines when an error is encountered. This ensures that all goroutines terminate gracefully upon error, preventing resource leaks and unintended behavior.
Apply this diff to integrate context cancellation:
+import (
+ "context"
+)
func (g *LinkedSchemaGraph) LinkedEntrances(targets []*base.Entrance, source *base.Entrance) ([]*LinkedEntrance, error) {
+ ctx, cancel := context.WithCancel(context.Background())
visited := map[string]struct{}{}
var mu sync.Mutex
var wg sync.WaitGroup
errChan := make(chan error, len(targets))
done := make(chan struct{})
totalEntries := make([]*LinkedEntrance, 0)
for _, target := range targets {
wg.Add(1)
go func(target *base.Entrance) {
defer wg.Done()
+ select {
+ case <-ctx.Done():
+ return
+ default:
+ }
entries, err := g.findEntrance(target, source, visited, &mu)
if err != nil {
errChan <- err
+ cancel()
return
}
mu.Lock()
defer mu.Unlock()
totalEntries = append(totalEntries, entries...)
}(target)
}
go func() {
wg.Wait()
close(done)
}()
select {
case <-done:
// All goroutines finished
return totalEntries, nil
case err := <-errChan:
// Return on the first error encountered
+ cancel()
return nil, err
}
}📝 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.
| func (g *LinkedSchemaGraph) LinkedEntrances(targets []*base.Entrance, source *base.Entrance) ([]*LinkedEntrance, error) { | |
| visited := map[string]struct{}{} | |
| var mu sync.Mutex | |
| var wg sync.WaitGroup | |
| errChan := make(chan error, len(targets)) | |
| done := make(chan struct{}) | |
| totalEntries := make([]*LinkedEntrance, 0) | |
| for _, target := range targets { | |
| wg.Add(1) | |
| go func(target *base.Entrance) { | |
| defer wg.Done() | |
| entries, err := g.findEntrance(target, source, visited, &mu) | |
| if err != nil { | |
| errChan <- err | |
| } | |
| mu.Lock() | |
| defer mu.Unlock() | |
| totalEntries = append(totalEntries, entries...) | |
| }(target) | |
| } | |
| return entries, nil | |
| go func() { | |
| wg.Wait() | |
| close(done) | |
| }() | |
| select { | |
| case <-done: | |
| // All goroutines finished | |
| return totalEntries, nil | |
| case err := <-errChan: | |
| // Return on the first error encountered | |
| return nil, err | |
| } | |
| import ( | |
| "context" | |
| ) | |
| func (g *LinkedSchemaGraph) LinkedEntrances(targets []*base.Entrance, source *base.Entrance) ([]*LinkedEntrance, error) { | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| visited := map[string]struct{}{} | |
| var mu sync.Mutex | |
| var wg sync.WaitGroup | |
| errChan := make(chan error, len(targets)) | |
| done := make(chan struct{}) | |
| totalEntries := make([]*LinkedEntrance, 0) | |
| for _, target := range targets { | |
| wg.Add(1) | |
| go func(target *base.Entrance) { | |
| defer wg.Done() | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| default: | |
| } | |
| entries, err := g.findEntrance(target, source, visited, &mu) | |
| if err != nil { | |
| errChan <- err | |
| cancel() | |
| return | |
| } | |
| mu.Lock() | |
| defer mu.Unlock() | |
| totalEntries = append(totalEntries, entries...) | |
| }(target) | |
| } | |
| go func() { | |
| wg.Wait() | |
| close(done) | |
| }() | |
| select { | |
| case <-done: | |
| // All goroutines finished | |
| return totalEntries, nil | |
| case err := <-errChan: | |
| // Return on the first error encountered | |
| cancel() | |
| return nil, err | |
| } | |
| } |
proto/base/v1/service.proto
Outdated
| "str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionLookupEntityRequest{\n" | ||
| " Metadata: &v1.PermissionLookupEntityRequestMetadata{\n" | ||
| " SnapToken: \"\",\n" | ||
| " SchemaVersion: \"\",\n" | ||
| " Depth: 50,\n" | ||
| " },\n" | ||
| " EntityType: \"document\",\n" | ||
| " Permissions: [\"view\"],\n" | ||
| " Subject: &v1.Subject{\n" | ||
| " Type: \"user\",\n" | ||
| " Id: \"1\",\n" | ||
| " },\n" | ||
| " PageSize: 20,\n" | ||
| " ContinuousToken: \"\",\n" | ||
| "})\n\n" |
There was a problem hiding this comment.
Incorrect request type in Go code sample for LookupEntitiesStream
In the Go code sample for LookupEntitiesStream, the request is of type &v1.PermissionLookupEntityRequest, but it should be &v1.PermissionsLookupEntityRequest to match the method signature.
Apply this diff to fix the issue:
-str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionLookupEntityRequest{
+str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionsLookupEntityRequest{📝 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.
| "str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionLookupEntityRequest{\n" | |
| " Metadata: &v1.PermissionLookupEntityRequestMetadata{\n" | |
| " SnapToken: \"\",\n" | |
| " SchemaVersion: \"\",\n" | |
| " Depth: 50,\n" | |
| " },\n" | |
| " EntityType: \"document\",\n" | |
| " Permissions: [\"view\"],\n" | |
| " Subject: &v1.Subject{\n" | |
| " Type: \"user\",\n" | |
| " Id: \"1\",\n" | |
| " },\n" | |
| " PageSize: 20,\n" | |
| " ContinuousToken: \"\",\n" | |
| "})\n\n" | |
| "str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionsLookupEntityRequest{\n" | |
| " Metadata: &v1.PermissionLookupEntityRequestMetadata{\n" | |
| " SnapToken: \"\",\n" | |
| " SchemaVersion: \"\",\n" | |
| " Depth: 50,\n" | |
| " },\n" | |
| " EntityType: \"document\",\n" | |
| " Permissions: [\"view\"],\n" | |
| " Subject: &v1.Subject{\n" | |
| " Type: \"user\",\n" | |
| " Id: \"1\",\n" | |
| " },\n" | |
| " PageSize: 20,\n" | |
| " ContinuousToken: \"\",\n" | |
| "})\n\n" |
proto/base/v1/service.proto
Outdated
| " Metadata: &v1.PermissionLookupEntityRequestMetadata{\n" | ||
| " SnapToken: \"\",\n" | ||
| " SchemaVersion: \"\",\n" | ||
| " Depth: 50,\n" | ||
| " },\n" | ||
| " EntityType: \"document\",\n" | ||
| " Permissions: [\"view\"],\n" | ||
| " Subject: &v1.Subject{\n" | ||
| " Type: \"user\",\n" | ||
| " Id: \"1\",\n" | ||
| " },\n" | ||
| " PageSize: 20,\n" | ||
| " ContinuousToken: \"\",\n" | ||
| "})\n\n" | ||
| "// handle stream response\n" | ||
| "for {\n" | ||
| " res, err := str.Recv()\n\n" | ||
| " if err == io.EOF {\n" | ||
| " break\n" | ||
| " }\n\n" | ||
| " // res.EntityId\n" | ||
| "}" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update code sample to access new permission field
The PermissionsLookupEntityStreamResponse message now includes a permission field. Please update the code sample to handle this field accordingly.
Apply this change to the code sample:
- // res.EntityId
+ // res.EntityId, res.Permission📝 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.
| " Metadata: &v1.PermissionLookupEntityRequestMetadata{\n" | |
| " SnapToken: \"\",\n" | |
| " SchemaVersion: \"\",\n" | |
| " Depth: 50,\n" | |
| " },\n" | |
| " EntityType: \"document\",\n" | |
| " Permissions: [\"view\"],\n" | |
| " Subject: &v1.Subject{\n" | |
| " Type: \"user\",\n" | |
| " Id: \"1\",\n" | |
| " },\n" | |
| " PageSize: 20,\n" | |
| " ContinuousToken: \"\",\n" | |
| "})\n\n" | |
| "// handle stream response\n" | |
| "for {\n" | |
| " res, err := str.Recv()\n\n" | |
| " if err == io.EOF {\n" | |
| " break\n" | |
| " }\n\n" | |
| " // res.EntityId\n" | |
| "}" | |
| } | |
| " Metadata: &v1.PermissionLookupEntityRequestMetadata{\n" | |
| " SnapToken: \"\",\n" | |
| " SchemaVersion: \"\",\n" | |
| " Depth: 50,\n" | |
| " },\n" | |
| " EntityType: \"document\",\n" | |
| " Permissions: [\"view\"],\n" | |
| " Subject: &v1.Subject{\n" | |
| " Type: \"user\",\n" | |
| " Id: \"1\",\n" | |
| " },\n" | |
| " PageSize: 20,\n" | |
| " ContinuousToken: \"\",\n" | |
| "})\n\n" | |
| "// handle stream response\n" | |
| "for {\n" | |
| " res, err := str.Recv()\n\n" | |
| " if err == io.EOF {\n" | |
| " break\n" | |
| " }\n\n" | |
| " // res.EntityId, res.Permission\n" | |
| "}" | |
| } |
| "client.permission.lookupEntities({\n" | ||
| " tenantId: \"t1\",\n" | ||
| " metadata: {\n" | ||
| " snapToken: \"\",\n" | ||
| " schemaVersion: \"\",\n" | ||
| " depth: 20\n" | ||
| " },\n" | ||
| " entity_type: \"document\",\n" | ||
| " permissions: [\"edit\"],\n" | ||
| " subject: {\n" | ||
| " type: \"user\",\n" | ||
| " id: \"1\"\n" | ||
| " },\n" | ||
| " page_size: 20,\n" | ||
| " continuous_token: \"\"\n" | ||
| "}).then((response) => {\n" | ||
| " console.log(response.entity_ids)\n" | ||
| "})" |
There was a problem hiding this comment.
Incorrect metadata type in JavaScript code sample
In the JavaScript code sample for LookupEntities, the metadata field should be of type PermissionsLookupEntityRequestMetadata to match the request message.
Apply this diff to fix the issue:
- metadata: {
+ metadata: new PermissionsLookupEntityRequestMetadata({Committable suggestion was skipped due to low confidence.
proto/base/v1/service.proto
Outdated
| value: {string_value: | ||
| "const permify = require(\"@permify/permify-node\");\n" | ||
| "const {PermissionsLookupEntityStreamResponse} = require(\"@permify/permify-node/dist/src/grpc/generated/base/v1/service\");\n\n" | ||
| "function main() {\n" | ||
| " const client = new permify.grpc.newClient({\n" | ||
| " endpoint: \"localhost:3478\",\n" | ||
| " });\n\n" | ||
| " let res = client.permission.lookupEntitiesStream({\n" | ||
| " metadata: {\n" | ||
| " snapToken: \"\",\n" | ||
| " schemaVersion: \"\",\n" | ||
| " depth: 20\n" | ||
| " },\n" | ||
| " entityType: \"document\",\n" | ||
| " permissions: [\"view\"],\n" | ||
| " subject: {\n" | ||
| " type: \"user\",\n" | ||
| " id: \"1\"\n" | ||
| " },\n" | ||
| " page_size: 20,\n" | ||
| " continuous_token: \"\"\n" | ||
| " });\n\n" | ||
| " handle(res);\n" | ||
| "}\n\n" | ||
| "async function handle(res: AsyncIterable<PermissionsLookupEntityStreamResponse>) {\n" | ||
| " for await (const response of res) {\n" | ||
| " // response.entityId\n" | ||
| " }\n" | ||
| "}" | ||
| } |
There was a problem hiding this comment.
Incorrect request type and response handling in JavaScript code sample
In the JavaScript code sample for LookupEntitiesStream, the request type should be PermissionsLookupEntityRequest. Additionally, consider updating the response handling to access the new permission field in PermissionsLookupEntityStreamResponse.
Apply these changes:
- let res = client.permission.lookupEntitiesStream({
+ let res = client.permission.lookupEntitiesStream(new PermissionsLookupEntityRequest({
...
- // response.entityId
+ // response.entityId, response.permissionCommittable suggestion was skipped due to low confidence.
| if l := len(m.GetPermissions()); l < 1 || l > 100 { | ||
| err := PermissionsLookupEntityRequestValidationError{ | ||
| field: "Permissions", | ||
| reason: "value must contain between 1 and 100 items, inclusive", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| for idx, item := range m.GetPermissions() { | ||
| _, _ = idx, item | ||
|
|
||
| if len(item) > 64 { | ||
| err := PermissionsLookupEntityRequestValidationError{ | ||
| field: fmt.Sprintf("Permissions[%v]", idx), | ||
| reason: "value length must be at most 64 bytes", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| if !_PermissionsLookupEntityRequest_Permissions_Pattern.MatchString(item) { | ||
| err := PermissionsLookupEntityRequestValidationError{ | ||
| field: fmt.Sprintf("Permissions[%v]", idx), | ||
| reason: "value does not match regex pattern \"^[a-zA-Z_][a-zA-Z0-9_]*$\"", | ||
| } | ||
| if !all { | ||
| return err | ||
| } | ||
| errors = append(errors, err) | ||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Regex Patterns for Permission Validations
Different regex patterns are used for validating permissions in various parts of the codebase, which could lead to inconsistent validation behaviors across similar fields. It's recommended to align these patterns unless there's a specific reason for the differences.
🔗 Analysis chain
Inconsistent Regex Patterns for Permission Validations
The regex pattern used for validating each permission in PermissionsLookupEntityRequest is:
var _PermissionsLookupEntityRequest_Permissions_Pattern = regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9_]*$")However, the pattern used in PermissionLookupEntityRequest for the Permission field is:
var _PermissionLookupEntityRequest_Permission_Pattern = regexp.MustCompile("^[a-zA-Z_]{1,64}$")This inconsistency might lead to different validation behaviors for similar fields. For consistency and to prevent confusion, consider aligning the regex patterns used across permission validations unless there is a specific reason for the difference.
Run the following script to list all regex patterns used in permission validations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all regex patterns used in permission validations.
# Search for regex patterns in validation code for permissions.
rg 'var _.*Permission.*Pattern = regexp.MustCompile' -A 2
Length of output: 3655
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- internal/engines/lookup.go (10 hunks)
🔇 Additional comments (2)
internal/engines/lookup.go (2)
Line range hint
1-461: Overall assessment: Good implementation with room for minor improvementsThe changes successfully implement support for multiple permissions in lookup operations. The code is well-structured and follows good practices. Consider the following points for further improvement:
- Ensure consistent usage of
permissionChecksvariable across methods.- Enhance error handling, particularly in
ExecuteRequestscalls and stream operations.- Maintain consistency in method signatures and request types when dealing with multiple permissions.
These improvements will enhance the robustness and maintainability of the code.
87-88: Consider the usage ofpermissionChecksvariableThe
permissionChecksvariable is declared and passed to theEntityFiltermethod, but it's not used within theLookupEntitymethod. Consider whether this variable is necessary or if its usage is missing in the current implementation.Also applies to: 107-107
| err = checker.ExecuteRequests(size) // Execute the collected requests in parallel | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling for ExecuteRequests
While the current error handling is functional, it might be beneficial to provide more context when an error occurs. Consider wrapping the error with additional information about the operation being performed.
err = checker.ExecuteRequests(size) // Execute the collected requests in parallel
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to execute bulk permission checks: %w", err)
}This change would make debugging easier by providing more context about where the error occurred.
📝 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.
| err = checker.ExecuteRequests(size) // Execute the collected requests in parallel | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = checker.ExecuteRequests(size) // Execute the collected requests in parallel | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to execute bulk permission checks: %w", err) | |
| } |
|
|
||
| // LookupEntityStream performs a permission check on a set of entities and streams the results | ||
| // containing the IDs of the entities that have the requested permission. | ||
| func (engine *LookupEngine) LookupEntityStream(ctx context.Context, request *base.PermissionLookupEntityRequest, server base.Permission_LookupEntityStreamServer) (err error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating the request type for consistency
The method signature is still using PermissionLookupEntityRequest, which suggests a single permission. Given that the implementation now supports multiple permissions, consider updating the request type to PermissionsLookupEntityRequest for consistency with the new functionality.
-func (engine *LookupEngine) LookupEntityStream(ctx context.Context, request *base.PermissionLookupEntityRequest, server base.Permission_LookupEntityStreamServer) (err error) {
+func (engine *LookupEngine) LookupEntityStream(ctx context.Context, request *base.PermissionsLookupEntityRequest, server base.Permission_LookupEntityStreamServer) (err error) {This change would make the method signature more accurately reflect its capability to handle multiple permissions.
📝 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.
| // LookupEntityStream performs a permission check on a set of entities and streams the results | |
| // containing the IDs of the entities that have the requested permission. | |
| func (engine *LookupEngine) LookupEntityStream(ctx context.Context, request *base.PermissionLookupEntityRequest, server base.Permission_LookupEntityStreamServer) (err error) { | |
| // LookupEntityStream performs a permission check on a set of entities and streams the results | |
| // containing the IDs of the entities that have the requested permission. | |
| func (engine *LookupEngine) LookupEntityStream(ctx context.Context, request *base.PermissionsLookupEntityRequest, server base.Permission_LookupEntityStreamServer) (err error) { |
| callback := func(entityID, permission, token string) { | ||
| err := server.Send(&base.PermissionsLookupEntityStreamResponse{ | ||
| EntityId: entityID, | ||
| Permission: permission, | ||
| ContinuousToken: token, | ||
| }) | ||
| // If there is an error in sending the response, the function will return | ||
| if err != nil { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in the callback function
The current implementation ignores any error that occurs during the Send operation. Consider propagating this error to allow proper handling at a higher level.
callback := func(entityID, permission, token string) {
err := server.Send(&base.PermissionsLookupEntityStreamResponse{
EntityId: entityID,
Permission: permission,
ContinuousToken: token,
})
- // If there is an error in sending the response, the function will return
if err != nil {
- return
+ // Propagate the error
+ panic(err) // This will be recovered in ExecuteRequests
}
}Also, update the ExecuteRequests call to handle this panic:
-err = checker.ExecuteRequests(size)
+err = func() (err error) {
+ defer func() {
+ if r := recover(); r != nil {
+ err = fmt.Errorf("error sending stream response: %v", r)
+ }
+ }()
+ return checker.ExecuteRequests(size)
+}()This change ensures that any errors during the stream send operation are properly handled and reported.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
internal/engines/bulk.go (2)
268-283: LGTM with suggestion: BulkEntityPublisher.Publish updated for multiple permissionsThe
Publishmethod has been successfully updated to handle multiple permissions per entity, which aligns with the PR objective. The use ofpermissionChecks.AddER(entity, permission)appears to prevent duplicate permission checks, improving efficiency.Suggestion for improvement:
Consider adding logging or telemetry for skipped permissions to aid in debugging and monitoring. For example:if !permissionChecks.AddER(entity, permission) { // Log or emit telemetry for skipped permission // log.Debug("Skipped duplicate permission check", "entity", entity.GetId(), "permission", permission) continue }This addition would provide visibility into how often permissions are skipped due to duplication.
Line range hint
286-321: Update BulkSubjectPublisher for consistencyThe
BulkSubjectPublisherhas not been updated to support multiple permissions, unlike theBulkEntityPublisher. This inconsistency could lead to unexpected behavior or limitations when performing subject-based permission checks.To maintain consistency and fully support the list of permissions feature, consider updating
BulkSubjectPublishersimilarly toBulkEntityPublisher:
- Change the
requestfield type to*base.PermissionsLookupSubjectRequest.- Update the
NewBulkSubjectPublisherfunction signature accordingly.- Modify the
Publishmethod to handle multiple permissions:func (s *BulkSubjectPublisher) Publish(subject *base.Subject, metadata *base.PermissionCheckRequestMetadata, context *base.Context, result base.CheckResult, permissionChecks *VisitsMap) { for _, permission := range s.request.GetPermissions() { if !permissionChecks.AddSR(subject, permission) { continue } s.bulkChecker.RequestChan <- BulkCheckerRequest{ Request: &base.PermissionCheckRequest{ TenantId: s.request.GetTenantId(), Metadata: metadata, Entity: s.request.GetEntity(), Permission: permission, Subject: subject, Context: context, }, Result: result, } } }These changes will ensure that both entity and subject-based permission checks support multiple permissions consistently.
internal/schema/linkedSchema.go (1)
Potential Goroutine Leak Detected
The goroutine initiated at line 111 is missing a
wg.Done()call. This omission prevents theWaitGroupfrom being notified of the goroutine's completion, potentially leading to a goroutine leak.
- Location:
internal/schema/linkedSchema.goat line 111Please add
wg.Done()within the goroutine to ensure proper synchronization and prevent leaks.🔗 Analysis chain
Line range hint
1-394: Summary of LinkedSchemaGraph concurrency implementationThe changes introduce concurrency to improve the performance of the
LinkedEntrancesmethod, which is a positive step. However, there are several critical issues that need to be addressed:
- The deadlock in the
findEntrancemethod due to double locking of the mutex.- Potential goroutine leaks and inefficient error handling in the
LinkedEntrancesmethod.- Possible contention due to long-held mutex locks when appending to
totalEntries.Please prioritize fixing these issues, especially the deadlock, to ensure the correct and efficient operation of the concurrent implementation. After addressing these concerns, consider further optimizations such as using a context for cancellation and exploring lock-free data structures for better performance.
To ensure that the deadlock issue is resolved and that the concurrency is implemented correctly throughout the file, please run the following verification script:
This script will help identify any remaining instances of double mutex locking, inconsistent mutex usage, and potential goroutine leaks.
Please run the following verification scripts to further ensure there are no goroutine leaks and to assess mutex usage for potential contention:
Please run the following revised verification scripts to accurately identify any goroutine leaks and mutex locking issues:
Please run the following verification script to examine the code around line 111 and verify the proper use of
wg.Done()within the goroutine:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the removal of double mutex locking and check for consistent mutex usage # Check for double mutex locking echo "Checking for double mutex locking:" rg --type go 'mu\.Lock\(\).*mu\.Lock\(\)' internal/schema/linkedSchema.go # Check for consistent mutex parameter passing echo "Checking for consistent mutex parameter passing:" rg --type go 'func \(g \*LinkedSchemaGraph\) \w+\(.*\*sync\.Mutex\)' internal/schema/linkedSchema.go # Check for potential goroutine leaks echo "Checking for potential goroutine leaks:" rg --type go 'go func\(' internal/schema/linkedSchema.go -A 10 -B 2 echo "Verification complete. Please review the output for any remaining issues."Length of output: 1835
Script:
# #!/bin/bash # Check for goroutines without proper synchronization (e.g., missing wg.Done()) echo "Checking for goroutines without proper synchronization (missing wg.Done()):" rg --type go 'go func\(' internal/schema/linkedSchema.go | while read -r line; do context=$(echo "$line" | cut -d':' -f1-2) file=$(echo "$context" | cut -d':' -f1) lineno=$(echo "$context" | cut -d':' -f2) # Check the next 10 lines for wg.Done() if ! rg -A10 '^' --line-number=$lineno internal/schema/linkedSchema.go | tail -n+2 | grep -q 'wg\.Done\(\)'; then echo "Potential goroutine leak at $context" fi done # Check for mutexes that are locked without being promptly unlocked echo "Checking for mutexes locked without immediate unlock:" rg --type go 'mu\.Lock\(\)' internal/schema/linkedSchema.go -n | while read -r line; do lineno=$(echo "$line" | cut -d':' -f2) # Check the next 10 lines for mu.Unlock() if ! rg -A10 "^" --line-number=$lineno internal/schema/linkedSchema.go | tail -n+2 | grep -q 'mu\.Unlock\(\)'; then echo "Mutex lock without unlock near line $lineno" fi done echo "Verification complete. Please review the output for any remaining issues."Length of output: 2076
Script:
# #!/bin/bash # Check for goroutines without proper synchronization (e.g., missing wg.Done()) echo "Checking for goroutines without proper synchronization (missing wg.Done()):" rg --type go 'go\s+func\s*\(' internal/schema/linkedSchema.go --with-filename --line-number | while IFS=: read -r file lineno line; do # Extract the line number and check the next 10 lines for wg.Done() done_line=$((lineno + 10)) # Use sed to extract lines from current line to done_line sed -n "${lineno},${done_line}p" "$file" | grep -q 'wg\.Done()' if [ $? -ne 0 ]; then echo "Potential goroutine leak in $file at line $lineno" fi done # Check for mutexes that are locked without being promptly unlocked echo "Checking for mutexes locked without immediate unlock:" rg --type go 'mu\.Lock\(\)' internal/schema/linkedSchema.go --with-filename --line-number | while IFS=: read -r file lineno line; do # Extract the line number and check the next 10 lines for mu.Unlock() done_line=$((lineno + 10)) # Use sed to extract lines from current line to done_line sed -n "${lineno},${done_line}p" "$file" | grep -q 'mu\.Unlock()' if [ $? -ne 0 ]; then echo "Mutex lock without unlock in $file at line $lineno" fi done echo "Verification complete. Please review the output for any remaining issues."Length of output: 1414
Script:
# #!/bin/bash # Extract code context around the potential goroutine leak at line 111 echo "Extracting code context around line 111 to verify goroutine usage:" sed -n '106,116p' internal/schema/linkedSchema.go echo "Verification complete. Please review the output for any remaining issues."Length of output: 492
internal/engines/lookup.go (5)
74-74: Refactor for consistency with multi-permission supportThe changes in this function appear to be adapting it for consistency with the new multi-permission support:
- The publisher now uses
ConvertToPermissionsLookupEntityRequest, which likely converts the single permission request to a multi-permission format.- A new
permissionChecksVisitsMap has been added.- The
Entrancesfield is now a list, although it still contains only a single entrance.- The
permissionChecksmap is now passed to the EntityFilter call.These changes maintain backwards compatibility while aligning the function with the new multi-permission structure. However, consider adding a comment explaining why these changes were made to a single-permission function for clarity.
Also applies to: 86-87, 96-100, 106-106
126-205: Enhance error handling and consider performance optimization in LookupEntitiesThe new
LookupEntitiesfunction looks well-structured and correctly implements multi-permission support. However, consider the following improvements:
- Error Handling: Enhance error handling in the callback function. Currently, errors in sending responses are silently ignored.
callback := func(entityID, permission, token string) { mu.Lock() defer mu.Unlock() if _, exists := entityIDsByPermission[permission]; !exists { entityIDsByPermission[permission] = &base.EntityIds{Ids: []string{}} } entityIDsByPermission[permission].Ids = append(entityIDsByPermission[permission].Ids, entityID) ct = token + // Consider how to handle errors, maybe by using a channel to communicate errors back to the main function }
Performance: For large numbers of permissions, consider using a more efficient data structure than a slice for
entrances.Consistency: Consider extracting common code between
LookupEntityandLookupEntitiesinto a shared helper function to reduce duplication.
217-217: Consistent adaptation for multi-permission supportThe changes in
LookupEntityStreamare consistent with those made inLookupEntity:
- The callback function signature has been updated to include the
permissionparameter.- The publisher now uses
ConvertToPermissionsLookupEntityRequest.- A
permissionChecksVisitsMap has been added.- The
Entrancesfield is now a list with a single entrance.- The
permissionChecksmap is now passed to the EntityFilter call.These changes maintain backwards compatibility while aligning the function with the new multi-permission structure. However, consider adding a comment explaining the reason for these changes in a single-permission function for clarity.
Also applies to: 232-232, 242-242, 252-256, 261-261
276-333: Enhance error handling and consider performance optimization in LookupEntitiesStreamThe new
LookupEntitiesStreamfunction is well-structured and correctly implements multi-permission support with streaming. However, consider the following improvements:
- Error Handling: Enhance error handling in the callback function. Currently, errors in sending responses are silently ignored, which could lead to incomplete results without any indication of failure.
callback := func(entityID, permission, token string) { err := server.Send(&base.PermissionsLookupEntityStreamResponse{ EntityId: entityID, Permission: permission, ContinuousToken: token, }) if err != nil { - return + // Consider how to handle errors, maybe by using a channel to communicate errors back to the main function + // or by implementing a retry mechanism } }
Performance: For large numbers of permissions, consider using a more efficient data structure than a slice for
entrances.Consistency: Consider extracting common code between
LookupEntityStreamandLookupEntitiesStreaminto a shared helper function to reduce duplication.Cancellation: Consider adding a check for context cancellation in the main loop to allow for early termination if the client disconnects.
Line range hint
1-465: Overall assessment: Well-implemented multi-permission support with room for minor improvementsThe changes to
internal/engines/lookup.gosuccessfully implement multi-permission support for entity lookups. New functionsLookupEntitiesandLookupEntitiesStreamhave been added, and existing functions have been modified for consistency. The implementation is generally well-structured and maintains backwards compatibility.Recommendations for further improvement:
- Enhance error handling in callback functions, especially in streaming functions.
- Consider performance optimizations for handling large numbers of permissions.
- Extract common code into shared helper functions to reduce duplication.
- Add comments explaining the changes made to single-permission functions for clarity.
- Consider implementing context cancellation checks in streaming functions.
These improvements would enhance the robustness and maintainability of the code.
internal/engines/entityFilter.go (4)
37-37: Clarify the parameter comment forpermissionChecksThe comment for
permissionCheckshas grammatical issues and could be clearer. Consider rephrasing it to enhance readability. Suggested:"A thread-safe map to check if the same permission has already been checked for an entity."
218-218: Clarify the parameter comment forpermissionChecksSimilar to line 37, the comment for
permissionCheckscould be clearer. Suggested rephrasing:"A thread-safe map to check if the same permission has already been checked for an entity."
316-316: Clarify the parameter comment forpermissionChecksAgain, improve the comment for clarity. Suggested:
"A thread-safe map to check if the same permission has already been checked for an entity."
407-407: Clarify the parameter comment forpermissionChecksEnsure consistent and clear comments for
permissionChecks. Suggested:"A thread-safe map to check if the same permission has already been checked for an entity."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- internal/engines/bulk.go (6 hunks)
- internal/engines/entityFilter.go (18 hunks)
- internal/engines/lookup.go (11 hunks)
- internal/schema/linkedSchema.go (11 hunks)
🔇 Additional comments (12)
internal/engines/bulk.go (3)
57-57: LGTM: Enhanced permission handling in BulkCheckerThe changes to the
BulkCheckerstruct andNewBulkCheckerfunction signature improve the granularity of permission handling. The addition of thepermissionparameter to the callback function allows for more detailed processing of individual permissions, which aligns well with the PR objective of supporting a list of permissions.Also applies to: 79-79
225-226: LGTM: Consistent callback updates in ExecuteRequestsThe
ExecuteRequestsmethod has been properly updated to include the permission parameter in the callback invocations for both BULK_ENTITY and BULK_SUBJECT types. This change ensures consistency with the new callback signature and maintains the distinction between entity and subject processing.Also applies to: 228-229
253-253: LGTM: BulkEntityPublisher updated for multiple permissions supportThe
BulkEntityPublisherstruct andNewBulkEntityPublisherfunction have been updated to usePermissionsLookupEntityRequestinstead ofPermissionLookupEntityRequest. This change reflects the support for multiple permissions, aligning with the PR's objective. The implementation is correct and maintains backward compatibility.Also applies to: 259-259
internal/schema/linkedSchema.go (3)
165-167: Consistent mutex usage in method callsThe changes in these lines correctly pass the mutex to the related methods, maintaining consistency with the updated method signatures.
Also applies to: 183-183
203-203: Consistent method signature updatesThe method signatures for
findRelationEntrance,findEntranceLeaf, andfindEntranceRewritehave been consistently updated to include the mutex parameter. This change aligns with the concurrency implementation in theLinkedEntrancesmethod.Also applies to: 260-260, 374-374
293-293: Consistent mutex usage in nested method callsThe changes in these lines correctly pass the mutex to the nested
findEntrance,findEntranceRewrite, andfindEntranceLeafmethod calls. This ensures that the concurrency control is maintained throughout the recursive calls.Also applies to: 318-318, 379-379, 384-384
internal/engines/lookup.go (2)
Line range hint
63-67: Update callback function signatureThe callback function signature has been updated to include a
permissionparameter. This change aligns with the new multi-permission support.
361-361: Consistent update to callback function signatureThe callback function signature in
LookupSubjecthas been updated to include thepermissionparameter. This change is consistent with the updates made to other functions and supports the new multi-permission functionality.While this function still handles a single permission, the change maintains consistency across the codebase and prepares for potential future multi-permission support in subject lookup.
internal/engines/entityFilter.go (4)
Line range hint
89-105: Ensure consistent parameter naming across functionsThe functions
relationEntrance,tupleToUserSetEntrance, andltuse the parameterpermissionChecks *VisitsMap, whileattributeEntranceandlauseentityChecks *VisitsMap. For consistency and to avoid confusion, consider standardizing the parameter name across all functions unless there is a specific reason for the distinction.
164-169: Refactor duplicated pagination logic into a helper functionThe pagination initialization logic between lines 164–169, 256–261, and 354–359 is identical. Refactoring this code into a shared helper function would reduce duplication and improve maintainability.
Also applies to: 256-261, 354-359
471-471: Ensure consistent parameter naming across functionsThe function
lauses the parameterentityChecks *VisitsMapinstead ofpermissionChecks *VisitsMap. For consistency and to avoid confusion, consider standardizing the parameter name.
Line range hint
448-459: Recursive call seems correctly structuredThe recursive call to
engine.EntityFilterwithinltis properly set up with the correct parameters, ensuring that the recursive logic functions as intended.
| "github.com/Permify/permify/pkg/dsl/utils" | ||
| base "github.com/Permify/permify/pkg/pb/base/v1" | ||
| "sync" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve concurrency implementation and error handling
The concurrency implementation is a good step towards improving performance, but there are a few areas that could be enhanced:
- Error handling: Currently, if an error occurs, other goroutines continue running unnecessarily. Consider using a context for cancellation.
- Mutex usage: The mutex is locked for the entire duration of appending to
totalEntries, which might cause contention. Consider using a concurrent slice or minimizing the lock duration. - Channel management: The
errChanis not closed, which could lead to a goroutine leak if an error occurs.
Here's a suggested improvement:
import (
"context"
"sync"
)
func (g *LinkedSchemaGraph) LinkedEntrances(targets []*base.Entrance, source *base.Entrance) ([]*LinkedEntrance, error) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
var mu sync.Mutex
var wg sync.WaitGroup
errChan := make(chan error, 1)
totalEntries := make([]*LinkedEntrance, 0)
for _, target := range targets {
wg.Add(1)
go func(target *base.Entrance) {
defer wg.Done()
entries, err := g.findEntrance(ctx, target, source, &mu)
if err != nil {
select {
case errChan <- err:
cancel()
default:
}
return
}
mu.Lock()
totalEntries = append(totalEntries, entries...)
mu.Unlock()
}(target)
}
go func() {
wg.Wait()
close(errChan)
}()
select {
case err := <-errChan:
return nil, err
case <-ctx.Done():
return totalEntries, nil
}
}This implementation uses a context for cancellation, minimizes the mutex lock duration, and properly closes the error channel.
Also applies to: 90-123
| func (g *LinkedSchemaGraph) findEntrance(target, source *base.Entrance, visited map[string]struct{}, mu *sync.Mutex) ([]*LinkedEntrance, error) { | ||
| key := utils.Key(target.GetType(), target.GetValue()) | ||
| mu.Lock() | ||
|
|
||
| if _, ok := visited[key]; ok { | ||
| mu.Unlock() | ||
| return nil, nil | ||
| } | ||
| visited[key] = struct{}{} | ||
| mu.Unlock() |
There was a problem hiding this comment.
Fix critical deadlock issue in findEntrance method
There's a critical issue in the findEntrance method that will cause a deadlock:
The mutex is locked twice consecutively (lines 143-144) without an intervening unlock. This will cause the second Lock() call to wait indefinitely, resulting in a deadlock.
Apply this diff to fix the issue:
func (g *LinkedSchemaGraph) findEntrance(target, source *base.Entrance, visited map[string]struct{}, mu *sync.Mutex) ([]*LinkedEntrance, error) {
key := utils.Key(target.GetType(), target.GetValue())
- mu.Lock()
mu.Lock()
if _, ok := visited[key]; ok {
mu.Unlock()
return nil, nil
}
visited[key] = struct{}{}
mu.Unlock()This change removes the redundant mu.Lock() call, preventing the deadlock.
📝 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.
| func (g *LinkedSchemaGraph) findEntrance(target, source *base.Entrance, visited map[string]struct{}, mu *sync.Mutex) ([]*LinkedEntrance, error) { | |
| key := utils.Key(target.GetType(), target.GetValue()) | |
| mu.Lock() | |
| if _, ok := visited[key]; ok { | |
| mu.Unlock() | |
| return nil, nil | |
| } | |
| visited[key] = struct{}{} | |
| mu.Unlock() | |
| func (g *LinkedSchemaGraph) findEntrance(target, source *base.Entrance, visited map[string]struct{}, mu *sync.Mutex) ([]*LinkedEntrance, error) { | |
| key := utils.Key(target.GetType(), target.GetValue()) | |
| mu.Lock() | |
| if _, ok := visited[key]; ok { | |
| mu.Unlock() | |
| return nil, nil | |
| } | |
| visited[key] = struct{}{} | |
| mu.Unlock() |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
proto/base/v1/service.proto (2)
394-395: Update method comments for clarityThe comments for the
LookupEntitiesmethod could be more precise. Consider updating them to reflect that this method handles multiple entities and permissions.Apply this change to improve the comments:
- // LookupEntity method receives a PermissionsLookupEntityRequest and returns a PermissionsLookupEntityResponse. - // It is used to retrieve entities by its identifier. + // LookupEntities method receives a PermissionsLookupEntityRequest and returns a PermissionsLookupEntityResponse. + // It is used to retrieve multiple entities based on a list of permissions.
572-573: Update code sample to access newpermissionfieldThe
PermissionsLookupEntityStreamResponsemessage now includes apermissionfield. Please update the code sample to handle this field accordingly.Apply this change to the code sample:
- // res.EntityId + // res.EntityId, res.Permissiondocs/api-reference/openapiv2/apidocs.swagger.json (2)
876-936: Approve new streaming endpoint and suggest documentation updates.The new
lookup-entities-streamendpoint is a valuable addition to the API, allowing efficient handling of large result sets. The structure is consistent with the non-streaming version and includes bothentity_idandpermissionin each streamed response, which is helpful for clients processing the stream.To ensure smooth adoption of this new endpoint:
- Update the API documentation to include best practices for consuming streaming responses.
- Provide code samples in various languages demonstrating how to efficiently process the streamed data.
- Consider adding guidance on when to use the streaming endpoint versus the non-streaming endpoint based on expected result size or performance characteristics.
Line range hint
1-3451: Summary of API changes and recommendations for implementation.The changes to the Permify API introduce new capabilities and improve efficiency, particularly with the addition of the lookup-entities and lookup-entities-stream endpoints. These modifications allow for checking multiple permissions in a single request and provide more flexible response structures. However, they also introduce some breaking changes that will require updates to client implementations.
Key points to address:
- The new endpoints and response structures offer improved flexibility and efficiency.
- Existing clients will need to be updated to handle the new request and response formats.
- API documentation should be thoroughly updated to reflect these changes.
To ensure a smooth transition and maintain backwards compatibility:
- Consider implementing API versioning (e.g., v1 vs v2) to allow gradual adoption of the new endpoints and structures.
- Provide comprehensive migration guides and updated code samples for all supported languages and frameworks.
- Implement a deprecation strategy for the old endpoints, if applicable, giving clients ample time to migrate.
- Update any server-side SDK libraries to support both old and new structures during the transition period.
- Monitor API usage after deployment to identify any clients struggling with the transition and offer targeted support.
These steps will help ensure that the valuable improvements introduced in this update are adopted smoothly by your API consumers.
docs/api-reference/apidocs.swagger.json (1)
Line range hint
2173-2962: New definitions for entity lookup endpoints: Approved with a suggestionThe new definitions (
LookupEntitiesBody,LookupEntitiesStreamBody,PermissionsLookupEntityResponse,PermissionsLookupEntityStreamResponse, andEntityIds) are well-structured and provide comprehensive support for the new entity lookup endpoints. They include necessary fields for request metadata, filtering, pagination, and appropriate response structures.One minor suggestion for consistency:
Consider renaming the
entitiy Idsfield in theEntityIdsdefinition toentity_idsfor consistency with the naming convention used in other parts of the API (e.g.,entity_type,entity_id)."EntityIds": { "type": "object", "properties": { "ids": { "type": "array", "items": { "type": "string" }, - "title": "entitiy Ids" + "title": "entity_ids" } } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
- docs/api-reference/apidocs.swagger.json (4 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (4 hunks)
- pkg/pb/base/v1/service.pb.validate.go (8 hunks)
- proto/base/v1/service.proto (5 hunks)
🧰 Additional context used
🔇 Additional comments (10)
proto/base/v1/service.proto (8)
396-517: Approve new LookupEntities RPC methodThe implementation of the new
LookupEntitiesRPC method looks good. It enhances the API by allowing batch permission lookups, which can improve efficiency for clients needing to check multiple permissions at once.
520-627: Approve new LookupEntitiesStream RPC methodThe implementation of the new
LookupEntitiesStreamRPC method is a valuable addition. It provides an efficient way to handle large datasets by streaming the results, which is particularly useful for clients dealing with a large number of entities and permissions.
1229-1301: Approve new PermissionsLookupEntityRequest messageThe new
PermissionsLookupEntityRequestmessage is well-structured and appropriately supports the newLookupEntitiesmethod. The change from a single permission to a list of permissions (line 1259) is a good design choice for the new functionality.
1332-1340: Approve new PermissionsLookupEntityResponse messageThe new
PermissionsLookupEntityResponsemessage structure is well-designed. The change from a simple list of entity IDs to a map of string toEntityIds(line 1336) allows for more flexible and informative responses, likely grouping entities by permissions. This aligns well with the new functionality of looking up multiple permissions at once.
1359-1368: Approve new PermissionsLookupEntityStreamResponse messageThe new
PermissionsLookupEntityStreamResponsemessage is well-structured for streaming responses. The inclusion of bothentity_idandpermissionfields (lines 1361 and 1364) in each response allows for a clear association between entities and their permissions, which is crucial for the new streaming functionality.
425-440:⚠️ Potential issueCorrect metadata type in Go code sample
In the Go code sample for
LookupEntities, theMetadatafield is initialized with&v1.PermissionLookupEntityRequestMetadata, but it should be&v1.PermissionsLookupEntityRequestMetadatato match the request type.Apply this change to fix the inconsistency:
- Metadata: &v1.PermissionLookupEntityRequestMetadata{ + Metadata: &v1.PermissionsLookupEntityRequestMetadata{ SnapToken: "", SchemaVersion: "", Depth: 20, },Likely invalid or redundant comment.
551-565:⚠️ Potential issueCorrect request type in Go code sample for
LookupEntitiesStreamIn the Go code sample for
LookupEntitiesStream, the request is of type&v1.PermissionsLookupEntityRequest, but it should be&v1.PermissionsLookupEntityRequestto match the method signature.Apply this change to fix the issue:
-str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionLookupEntityRequest{ +str, err := client.Permission.LookupEntitiesStream(context.Background(), &v1.PermissionsLookupEntityRequest{Likely invalid or redundant comment.
1243-1245: 🛠️ Refactor suggestionInconsistent metadata message naming
In
PermissionsLookupEntityRequest, themetadatafield is of typePermissionLookupEntityRequestMetadata. For consistency, consider renaming the metadata type toPermissionsLookupEntityRequestMetadata.Apply this diff to rename the metadata message:
-message PermissionLookupEntityRequestMetadata { +message PermissionsLookupEntityRequestMetadata {And update the field type accordingly:
-PermissionLookupEntityRequestMetadata metadata = 2 [ +PermissionsLookupEntityRequestMetadata metadata = 2 [Likely invalid or redundant comment.
docs/api-reference/apidocs.swagger.json (1)
819-936: New endpoints for permission-based entity lookups: ApprovedThe addition of
/v1/tenants/{tenant_id}/permissions/lookup-entitiesand/v1/tenants/{tenant_id}/permissions/lookup-entities-streamendpoints enhances the API's capabilities for permission-based entity lookups. These new endpoints provide both synchronous and streaming options, allowing for efficient handling of large result sets. The implementation is consistent with other endpoints in the API and follows best practices.pkg/pb/base/v1/service.pb.validate.go (1)
1611-1975: Validation Functions for 'PermissionsLookupEntityRequest' Implemented CorrectlyThe generated validation functions for
PermissionsLookupEntityRequestand related types are correctly implemented. They ensure that all required fields are present, string lengths are within specified limits, and regex patterns are correctly applied. Embedded messages are also properly validated.
| "/v1/tenants/{tenant_id}/permissions/lookup-entities": { | ||
| "post": { | ||
| "summary": "lookup entities", | ||
| "operationId": "permissions.lookupEntities", | ||
| "responses": { | ||
| "200": { | ||
| "description": "A successful response.", | ||
| "schema": { | ||
| "$ref": "#/definitions/PermissionsLookupEntityResponse" | ||
| } | ||
| }, | ||
| "default": { | ||
| "description": "An unexpected error response.", | ||
| "schema": { | ||
| "$ref": "#/definitions/Status" | ||
| } | ||
| } | ||
| }, | ||
| "parameters": [ | ||
| { | ||
| "name": "tenant_id", | ||
| "description": "Identifier of the tenant, if you are not using multi-tenancy (have only one tenant) use pre-inserted tenant \u003ccode\u003et1\u003c/code\u003e for this field. Required, and must match the pattern \\“[a-zA-Z0-9-,]+\\“, max 64 bytes.", | ||
| "in": "path", | ||
| "required": true, | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "name": "body", | ||
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/LookupEntitiesBody" | ||
| } | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "Permission" | ||
| ], | ||
| "x-codeSamples": [ | ||
| { | ||
| "label": "go", | ||
| "lang": "go", | ||
| "source": "cr, err := client.Permission.LookupEntities(context.Background(), \u0026v1.PermissionsLookupEntityRequest{\n TenantId: \"t1\",\n Metadata: \u0026v1.PermissionLookupEntityRequestMetadata{\n SnapToken: \"\",\n SchemaVersion: \"\",\n Depth: 20,\n },\n EntityType: \"document\",\n Permissions: [\"edit\"],\n Subject: \u0026v1.Subject{\n Type: \"user\",\n Id: \"1\",\n }\n PageSize: 20,\n ContinuousToken: \"\",\n})" | ||
| }, | ||
| { | ||
| "label": "node", | ||
| "lang": "javascript", | ||
| "source": "client.permission.lookupEntities({\n tenantId: \"t1\",\n metadata: {\n snapToken: \"\",\n schemaVersion: \"\",\n depth: 20\n },\n entity_type: \"document\",\n permissions: [\"edit\"],\n subject: {\n type: \"user\",\n id: \"1\"\n },\n page_size: 20,\n continuous_token: \"\"\n}).then((response) =\u003e {\n console.log(response.entity_ids)\n})" | ||
| }, | ||
| { | ||
| "label": "cURL", | ||
| "lang": "curl", | ||
| "source": "curl --location --request POST 'localhost:3476/v1/tenants/{tenant_id}/permissions/lookup-entities' \\\n--header 'Content-Type: application/json' \\\n--data-raw '{\n \"metadata\":{\n \"snap_token\": \"\",\n \"schema_version\": \"\",\n \"depth\": 20\n },\n \"entity_type\": \"document\",\n \"permissions\": [\"edit\"],\n \"subject\": {\n \"type\":\"user\",\n \"id\":\"1\"\n },\n \"page_size\": 20,\n \"continuous_token\": \"\",\n}'" | ||
| } | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update API clients to handle multiple permissions and new response structure.
The new lookup-entities endpoint introduces two significant changes:
- The
permissionsfield in the request body is now an array, allowing multiple permissions to be checked in a single request. - The response structure uses a map for
entity_ids, where each key is a permission and the value is an array of entity IDs.
These changes enhance the API's flexibility but require updates to API clients. Please ensure that:
- All API clients are updated to provide an array of permission names when calling this endpoint.
- Client-side logic is adjusted to handle the new response structure, where entity IDs are grouped by permission.
Consider adding a migration guide or code samples to help API consumers adapt to these changes.
| "EntityIds": { | ||
| "type": "object", | ||
| "properties": { | ||
| "ids": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "title": "entitiy Ids" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update API documentation and clients for modified response structures.
Several changes have been made to existing structures:
PermissionsLookupEntityResponsenow uses a map forentity_ids, where each key is a permission and the value is anEntityIdsobject containing an array of entity IDs.PermissionsLookupEntityStreamResponseincludes apermissionfield, allowing clients to identify which permission each streamed entity corresponds to.- A new
EntityIdsstructure has been added to support the new response format.
These changes enhance the API's flexibility and provide more detailed information in responses. However, they will require updates to existing client implementations.
To ensure a smooth transition for API consumers:
- Update all API documentation to reflect these changes, including request and response examples.
- Provide migration guides for updating client code to handle the new response structures.
- Consider versioning the API (e.g., v2) to allow for a gradual transition if these changes are breaking for existing clients.
- Update any server-side SDK libraries to accommodate these new structures.
Also applies to: 2915-2948
| "PermissionsLookupEntityResponse": { | ||
| "type": "object", | ||
| "properties": { | ||
| "entity_ids": { | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "$ref": "#/definitions/EntityIds" | ||
| }, | ||
| "description": "List of identifiers for entities that match the lookup." | ||
| }, | ||
| "continuous_token": { | ||
| "type": "string", | ||
| "description": "continuous_token is a string that can be used to paginate and retrieve the next set of results." | ||
| } | ||
| }, | ||
| "description": "PermissionLookupEntityResponse is the response message for the LookupEntity method in the Permission service." | ||
| }, |
There was a problem hiding this comment.
Clarification needed: Changes to PermissionLookupEntityResponse structure
The PermissionLookupEntityResponse definition has been modified, changing the entity_ids field from an array of strings to an object with EntityIds as values. While this change allows for a more structured response, it raises some concerns:
- The new structure might break backward compatibility for existing clients expecting an array of strings.
- The purpose of the object structure is not clear from the definition. What do the keys in this object represent?
- The field name
entity_idssuggests a simple list, which is no longer accurate.
Please provide clarification on the following points:
- What is the rationale behind this structural change?
- What do the keys in the
entity_idsobject represent? - How should existing clients handle this change?
- Should the field name be updated to better reflect its new structure?
Consider either reverting this change to maintain backward compatibility or providing a clear migration path and updating the field name to reflect its new structure.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
docs/api-reference/openapiv2/apidocs.swagger.json (4)
768-824: New endpoint structure looks good, consider enhancing documentation.The new
/v1/tenants/{tenant_id}/permissions/lookup-entitiesendpoint is well-structured and consistent with other endpoints in the API. The use of a map forentity_idsin the response (PermissionsLookupEntityResponse) provides improved flexibility.Consider enhancing the documentation for this endpoint, particularly explaining the new response structure with the
entity_idsmap. This will help API consumers understand and utilize the new format effectively.
825-885: Streaming endpoint looks good, consider adding usage example.The new
/v1/tenants/{tenant_id}/permissions/lookup-entities-streamendpoint is well-structured and provides an efficient way to handle large datasets. The response structure withentity_id,permission, andcontinuous_tokenis clear and useful.To improve the documentation, consider adding an example of how to consume the streaming response. This could include a code snippet demonstrating how to iterate over the stream and process each entity-permission pair. This addition would greatly assist developers in implementing clients for this endpoint.
2856-2872: PermissionsLookupEntityResponse structure improved, consider enhancing documentation.The
PermissionsLookupEntityResponsestructure has been updated to provide a more detailed and structured response. Theentity_idsfield is now a map where each key is a permission and the value is anEntityIdsobject. This change allows for better association of entity IDs with specific permissions.To improve clarity for API consumers:
- Consider adding a more detailed description of the
entity_idsfield, explaining the structure of the map and how it relates permissions to sets of entity IDs.- Provide an example of the response structure in the documentation to illustrate how the data is organized.
These additions will help developers better understand and utilize the new response format.
2149-2160: EntityIds structure is good, but description could be improved.The new
EntityIdsstructure is simple and serves its purpose well. It appropriately groups entity IDs together, which is useful for the updatedPermissionsLookupEntityResponse.To improve clarity:
- Consider updating the description of the
EntityIdsstructure to something like: "EntityIds represents a collection of entity identifiers associated with a specific permission."- For the
idsfield, you could use: "An array of entity identifiers that have the associated permission."These changes would provide more context about the purpose and usage of this structure within the API.
docs/api-reference/apidocs.swagger.json (3)
825-885: New endpoint added:/v1/tenants/{tenant_id}/permissions/lookup-entities-streamThis new endpoint provides a streaming version of the entity lookup functionality. The implementation looks good and is consistent with the non-streaming version. Here are some observations:
- The endpoint uses POST method, which is appropriate for complex queries.
- It's designed for streaming responses, which is reflected in the response schema.
- The response schema references a new definition
PermissionsLookupEntityStreamResponse.- The request body references a new definition
LookupEntitiesStreamBody.- The endpoint is properly tagged under "Permission" category.
- Code samples are provided for Go and Node.js, but cURL is missing.
Consider adding a cURL example for consistency with other endpoints and to provide a complete set of examples for API users.
2157-2168: New definition added:EntityIdsA new
EntityIdsdefinition has been added to support the new lookup-entities endpoints. The structure looks appropriate:
- It contains a single property
idswhich is an array of strings.- The property is correctly titled as "entity Ids".
However, there's a minor typo in the title.
Fix the typo in the title of the
idsproperty. Change "entitiy Ids" to "entity Ids".
2409-2453: New definition added:LookupEntitiesStreamBodyThis definition is identical to
LookupEntitiesBody, which is appropriate as it represents the request body for the streaming version of the lookup-entities endpoint. However, the description at the end of the definition is incorrect.Update the description at the end of the
LookupEntitiesStreamBodydefinition. Change "PermissionsLookupEntityRequest is the request message for the LookupEntities method in the Permission service." to "PermissionsLookupEntityStreamRequest is the request message for the LookupEntitiesStream method in the Permission service."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
pkg/pb/base/v1/health.pb.gois excluded by!**/*.pb.gopkg/pb/base/v1/health_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
- docs/api-reference/apidocs.swagger.json (4 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (4 hunks)
- pkg/pb/base/v1/health.pb.validate.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
docs/api-reference/apidocs.swagger.json (5)
768-824: New endpoint added:/v1/tenants/{tenant_id}/permissions/lookup-entitiesThis new endpoint has been added to the Permify API for looking up entities based on permissions. The implementation looks good and follows the structure of other endpoints in the API. Here are some observations:
- The endpoint uses POST method, which is appropriate for complex queries.
- It includes proper response codes (200 for success, default for errors).
- The response schema references a new definition
PermissionsLookupEntityResponse.- The request body references a new definition
LookupEntitiesBody.- The endpoint is properly tagged under "Permission" category.
- Code samples are provided for Go, Node.js, and cURL, which is helpful for API users.
2364-2408: New definition added:LookupEntitiesBodyThis new definition represents the request body for the lookup-entities endpoint. The structure is well-defined and includes all necessary fields:
metadata: ReferencesPermissionLookupEntityRequestMetadata.entity_type: String field for the type of entity to lookup.permissions: Array of strings for the permissions to check.subject: References theSubjectdefinition.context: References theContextdefinition.scope: A map of entity types toStringArrayValue.page_size: Integer for pagination.continuous_token: String for pagination.All fields have appropriate descriptions, and the structure is consistent with other similar definitions in the API.
2870-2886: Modified definition:PermissionsLookupEntityResponseThe
PermissionsLookupEntityResponsedefinition has been modified to change the structure of theentity_idsfield. The changes look good and provide more flexibility:
entity_idsis now an object withEntityIdsas values, instead of an array of strings.- This change allows for a more structured response, potentially grouping entity IDs by some key.
- The
continuous_tokenfield remains unchanged.
2887-2903: New definition added:PermissionsLookupEntityStreamResponseThis new definition represents the response for the streaming version of the lookup-entities endpoint. The structure is appropriate for a streaming response:
entity_id: A single string representing an entity ID.permission: The name of the permission for which the entity has access.continuous_token: For pagination in streaming responses.This structure allows for sending individual entities as they are found, which is suitable for streaming.
Line range hint
1-3458: Overall assessment: Well-implemented API enhancementsThe changes to the Permify API specification are well-designed and implemented. The new endpoints for entity lookup, including the streaming version, add valuable functionality to the API. The supporting structures and definitions are consistent with the existing API design and follow good practices.
Key points:
- Two new endpoints added:
/v1/tenants/{tenant_id}/permissions/lookup-entitiesand/v1/tenants/{tenant_id}/permissions/lookup-entities-stream.- New definitions added to support these endpoints:
EntityIds,LookupEntitiesBody,LookupEntitiesStreamBody, andPermissionsLookupEntityStreamResponse.- Existing definition
PermissionsLookupEntityResponsemodified to provide more flexibility.The changes maintain the overall structure and consistency of the API. There are only a few minor issues related to documentation and typos, which can be easily addressed.
Great work on expanding the API's capabilities while maintaining consistency and following best practices!
pkg/pb/base/v1/health.pb.validate.go (1)
1-1: Generated Code—Do Not Manually EditThe file
health.pb.validate.gois auto-generated byprotoc-gen-validate, as indicated by the comment on line 1. Manual edits to this file are not recommended since they will be overwritten during regeneration. Ensure that any changes or validation rules are applied in the.protofiles so they are correctly reflected here.
| "LookupEntitiesBody": { | ||
| "type": "object", | ||
| "properties": { | ||
| "metadata": { | ||
| "$ref": "#/definitions/PermissionLookupEntityRequestMetadata", | ||
| "description": "Metadata associated with this request, required." | ||
| }, | ||
| "entity_type": { | ||
| "type": "string", | ||
| "description": "Type of the entity to lookup, required, must start with a letter and can include alphanumeric and underscore, max 64 bytes." | ||
| }, | ||
| "permissions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "description": "Name of the permission to check, required, must start with a letter and can include alphanumeric and underscore, max 64 bytes." | ||
| }, | ||
| "subject": { | ||
| "$ref": "#/definitions/Subject", | ||
| "description": "Subject for which to check the permission, required." | ||
| }, | ||
| "context": { | ||
| "$ref": "#/definitions/Context", | ||
| "description": "Context associated with this request." | ||
| }, | ||
| "scope": { | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "$ref": "#/definitions/StringArrayValue" | ||
| }, | ||
| "description": "Scope: A map that associates entity types with lists of identifiers. Each entry\nhelps filter requests by specifying which entities are relevant to the operation." | ||
| }, | ||
| "page_size": { | ||
| "type": "integer", | ||
| "format": "int64", | ||
| "description": "page_size is the number of entities to be returned in the response.\nThe value should be between 1 and 100." | ||
| }, | ||
| "continuous_token": { | ||
| "type": "string", | ||
| "description": "continuous_token is an optional parameter used for pagination.\nIt should be the value received in the previous response." | ||
| } | ||
| }, | ||
| "description": "PermissionsLookupEntityRequest is the request message for the LookupEntities method in the Permission service." | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent permissions Field Definition Detected.
The permissions field in LookupEntitiesBody is inconsistently defined as both an object and an array of strings. This discrepancy can lead to confusion and potential errors in API usage.
- Action Required: Ensure that the
permissionsfield is uniformly defined across all documentation and implementation. If the intended type is an array of strings, update all instances to reflect this change consistently.
🔗 Analysis chain
LookupEntitiesBody structure updated correctly.
The LookupEntitiesBody structure has been appropriately updated to support multiple permissions. The change from a single permission to an array of permissions (permissions field) is a significant improvement, allowing for more flexible and efficient permission checks.
Let's verify that the documentation for the permissions field accurately reflects its new array type:
This command will help us confirm that the documentation correctly describes the permissions field as an array of strings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the documentation of the permissions field in LookupEntitiesBody
grep -A 3 '"permissions": {' docs/api-reference/openapiv2/apidocs.swagger.json
Length of output: 436
| // Validate checks the field values on HealthCheckRequest 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 *HealthCheckRequest) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on HealthCheckRequest 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 | ||
| // HealthCheckRequestMultiError, or nil if none found. | ||
| func (m *HealthCheckRequest) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *HealthCheckRequest) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| // no validation rules for Service | ||
|
|
||
| if len(errors) > 0 { | ||
| return HealthCheckRequestMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validation Methods Lack Implementation for HealthCheckRequest
The Validate, ValidateAll, and validate methods for HealthCheckRequest currently do not enforce any validation rules, as indicated by the placeholder comment // no validation rules for Service on line 60. If validation is required for fields within HealthCheckRequest, consider adding validation constraints in the .proto definition. This will enable protoc-gen-validate to generate the appropriate validation logic.
| // Validate checks the field values on HealthCheckResponse 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 *HealthCheckResponse) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on HealthCheckResponse 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 | ||
| // HealthCheckResponseMultiError, or nil if none found. | ||
| func (m *HealthCheckResponse) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *HealthCheckResponse) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| // no validation rules for Status | ||
|
|
||
| if len(errors) > 0 { | ||
| return HealthCheckResponseMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validation Methods Lack Implementation for HealthCheckResponse
Similarly, the Validate, ValidateAll, and validate methods for HealthCheckResponse do not enforce any validation rules, as noted by the comment // no validation rules for Status on line 164. If there are fields in HealthCheckResponse that require validation, please specify the necessary validation rules in the .proto file. This will allow protoc-gen-validate to generate the required validation logic automatically.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests