fix: subject filtering pagination #1710
Conversation
WalkthroughThis pull request primarily updates the version number of the Permify API documentation and related files from "v1.1.5" to "v1.1.6". The changes span several files, including API documentation and internal components. Additionally, enhancements to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
internal/engines/lookup.go (2)
248-257: LGTM: Proper handling of continuous token.The implementation correctly handles the continuous token when present. It decodes the token and extracts the start value, which is then used for pagination. The error handling for the decoding process is also implemented.
Consider wrapping the error returned from the decoding process to provide more context:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to decode continuous token: %w", err) }
263-271: LGTM: Correct implementation of pagination starting point.The code correctly determines the starting index for pagination based on the
startvalue. It handles both cases whenstartis empty and when it contains a value.Consider using a binary search instead of a linear search for better performance with large lists:
if start != "" { startIndex = sort.Search(len(ids), func(i int) bool { return ids[i] >= start }) }This change would improve the time complexity from O(n) to O(log n) for finding the starting index.
internal/engines/subjectFilter.go (2)
Line range hint
120-122: Fix typo in function namesubjectfFlterComputedAttributeThe function name
subjectfFlterComputedAttributeat lines 120-122 contains a typo with an extra 'f' in 'Filter'. This can lead to confusion and potential issues when referencing the function.Apply this diff to correct the function name:
-func (engine *SubjectFilter) subjectfFlterComputedAttribute( +func (engine *SubjectFilter) subjectFilterComputedAttribute( request *base.PermissionLookupSubjectRequest, ca *base.ComputedAttribute, ) SubjectFilterFunction {Ensure all references to this function are updated accordingly.
Line range hint
153-154: Correct function call tosubjectFilterComputedAttributeIn the switch case at lines 153-154, the function
subjectfFlterComputedAttributeis called, which includes a typo. It should besubjectFilterComputedAttributeto match the corrected function name.Apply this diff to correct the function call:
case *base.Leaf_ComputedAttribute: - return engine.subjectfFlterComputedAttribute(request, op.ComputedAttribute) + return engine.subjectFilterComputedAttribute(request, op.ComputedAttribute)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
- docs/api-reference/apidocs.swagger.json (1 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (1 hunks)
- internal/engines/lookup.go (1 hunks)
- internal/engines/lookup_test.go (1 hunks)
- internal/engines/subjectFilter.go (2 hunks)
- internal/info.go (1 hunks)
- internal/storage/postgres/dataReader.go (1 hunks)
- proto/base/v1/openapi.proto (1 hunks)
✅ Files skipped from review due to trivial changes (5)
- docs/api-reference/apidocs.swagger.json
- docs/api-reference/openapiv2/apidocs.swagger.json
- internal/info.go
- internal/storage/postgres/dataReader.go
- proto/base/v1/openapi.proto
🧰 Additional context used
🔇 Additional comments (4)
internal/engines/lookup.go (1)
243-244: LGTM: Initialization ofstartvariable.The initialization of the
startvariable as an empty string is a good practice. It provides a default value and will be used later for pagination handling.internal/engines/subjectFilter.go (1)
698-700: Properly handle wildcard exclusions insubjectFilterUnionIn lines 698-700 within the
subjectFilterUnionfunction, the conditionif id != ALL && !slices.Contains(excludedIds, id)correctly ensures that theALLwildcard is not added to the list of excluded IDs. This prevents logical errors where the wildcard could mistakenly be treated as an exclusion identifier.internal/engines/lookup_test.go (2)
4659-4661: 🛠️ Refactor suggestionConsider adding missing
project_editpermission check inproject_view.In the
project_viewpermission definition for theprojectentity, it might be appropriate to include a direct check forproject_editto ensure consistency and completeness.Apply this diff to include the
project_editpermission:permission project_view = (project_edit or perms.project_view) +permission project_edit = admin or parent.admin or perms.project_editLikely invalid or redundant comment.
4667-4667: Verify the inclusion of all relevant users ingroup_permspermissions.The
editpermission ingroup_permsincludes users who are not members ofgroup_perms_1. Ensure that this is intentional and that all necessary users are accounted for.Run the following script to check membership:
✅ Verification successful
Verified: All users with
editpermissions are correctly included ingroup_perms_1.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all users with the `edit` permission are members of `group_perms_1`. # Expected: Only users who are members of `group_perms_1` should have the `edit` permission. grep -rn 'group_perms:group_perms_1#members@group:' ./internal/engines/lookup_test.goLength of output: 146
Script:
#!/bin/bash # Description: Verify that all users with the `edit` permission are members of `group_1`. # Users to verify users=("user_1" "user_2" "user_3" "user_4") # Check each user in group_1 for user in "${users[@]}"; do grep -q "$user" ./internal/engines/lookup_test.go && echo "$user is a member of group_1" || echo "$user is NOT a member of group_1" doneLength of output: 637
| cti, err = storageContext.NewContextualTuples(request.GetContext().GetTuples()...).QueryRelationships(filter, database.NewCursorPagination()) | ||
| if err != nil { | ||
| return subjectFilterEmpty(), err |
There was a problem hiding this comment.
Handle pagination tokens for scalability in QueryRelationships
At lines 432-434, you're using database.NewCursorPagination() without handling pagination tokens in the QueryRelationships call for contextual tuples. Without managing pagination tokens or limits, there may be issues when dealing with large datasets, potentially leading to performance bottlenecks or incomplete data retrieval.
Consider modifying the pagination to handle tokens and set appropriate limits. This could involve passing pagination parameters from the request or defining default limits to ensure efficient data retrieval.
-cti, err = storageContext.NewContextualTuples(request.GetContext().GetTuples()...).QueryRelationships(filter, database.NewCursorPagination())
+pagination := database.NewCursorPagination()
+if request.GetContinuousToken() != "" {
+ pagination.Token = request.GetContinuousToken()
+}
+cti, err = storageContext.NewContextualTuples(request.GetContext().GetTuples()...).QueryRelationships(filter, pagination)Also, ensure that the ContinuousToken is updated appropriately in the response to support continued pagination.
Committable suggestion was skipped due to low confidence.
| rit, err = engine.dataReader.QueryRelationships(ctx, request.GetTenantId(), filter, request.GetMetadata().GetSnapToken(), database.NewCursorPagination()) | ||
| if err != nil { | ||
| return subjectFilterEmpty(), err |
There was a problem hiding this comment.
Ensure consistent pagination handling in data reader queries
At lines 440-442, similar to the previous comment, you're using database.NewCursorPagination() in the QueryRelationships call for the data reader without managing pagination tokens or limits. Consistent pagination handling across both contextual tuples and data reader queries is crucial for predictable and efficient data access.
Consider implementing pagination handling as suggested in the previous comment to ensure consistency. Here's an example:
-pagination := database.NewCursorPagination()
+pagination := database.NewCursorPagination()
+if request.GetContinuousToken() != "" {
+ pagination.Token = request.GetContinuousToken()
+}
rit, err = engine.dataReader.QueryRelationships(ctx, request.GetTenantId(), filter, request.GetMetadata().GetSnapToken(), pagination)This ensures that both sources respect the pagination parameters, providing a consistent interface for clients.
Committable suggestion was skipped due to low confidence.
| for permission, res := range filter.assertions { | ||
|
|
||
| ct := "" | ||
|
|
||
| var ids []string | ||
|
|
||
| for { | ||
| response, err := invoker.LookupSubject(context.Background(), &base.PermissionLookupSubjectRequest{ | ||
| TenantId: "t1", | ||
| SubjectReference: tuple.RelationReference(filter.subjectReference), | ||
| Entity: entity, | ||
| Permission: permission, | ||
| Metadata: &base.PermissionLookupSubjectRequestMetadata{ | ||
| SnapToken: token.NewNoopToken().Encode().String(), | ||
| SchemaVersion: "", | ||
| }, | ||
| ContinuousToken: ct, | ||
| PageSize: 2, | ||
| }) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| ids = append(ids, response.GetSubjectIds()...) | ||
|
|
||
| ct = response.GetContinuousToken() | ||
|
|
||
| if ct == "" { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| Expect(ids).Should(Equal(res)) |
There was a problem hiding this comment.
Ensure pagination logic handles all subjects correctly.
In the loop for retrieving subject IDs, consider handling potential scenarios where the number of subjects exceeds the page size to avoid missing any subjects due to pagination issues.
Apply this diff to adjust the pagination handling:
for {
response, err := invoker.LookupSubject(context.Background(), &base.PermissionLookupSubjectRequest{
TenantId: "t1",
SubjectReference: tuple.RelationReference(filter.subjectReference),
Entity: entity,
Permission: permission,
Metadata: &base.PermissionLookupSubjectRequestMetadata{
SnapToken: token.NewNoopToken().Encode().String(),
SchemaVersion: "",
},
ContinuousToken: ct,
- PageSize: 2,
+ PageSize: 100, // Increase page size to handle more subjects
})
Expect(err).ShouldNot(HaveOccurred())
ids = append(ids, response.GetSubjectIds()...)
ct = response.GetContinuousToken()
if ct == "" {
break
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for permission, res := range filter.assertions { | |
| ct := "" | |
| var ids []string | |
| for { | |
| response, err := invoker.LookupSubject(context.Background(), &base.PermissionLookupSubjectRequest{ | |
| TenantId: "t1", | |
| SubjectReference: tuple.RelationReference(filter.subjectReference), | |
| Entity: entity, | |
| Permission: permission, | |
| Metadata: &base.PermissionLookupSubjectRequestMetadata{ | |
| SnapToken: token.NewNoopToken().Encode().String(), | |
| SchemaVersion: "", | |
| }, | |
| ContinuousToken: ct, | |
| PageSize: 2, | |
| }) | |
| Expect(err).ShouldNot(HaveOccurred()) | |
| ids = append(ids, response.GetSubjectIds()...) | |
| ct = response.GetContinuousToken() | |
| if ct == "" { | |
| break | |
| } | |
| } | |
| Expect(ids).Should(Equal(res)) | |
| for permission, res := range filter.assertions { | |
| ct := "" | |
| var ids []string | |
| for { | |
| response, err := invoker.LookupSubject(context.Background(), &base.PermissionLookupSubjectRequest{ | |
| TenantId: "t1", | |
| SubjectReference: tuple.RelationReference(filter.subjectReference), | |
| Entity: entity, | |
| Permission: permission, | |
| Metadata: &base.PermissionLookupSubjectRequestMetadata{ | |
| SnapToken: token.NewNoopToken().Encode().String(), | |
| SchemaVersion: "", | |
| }, | |
| ContinuousToken: ct, | |
| PageSize: 100, // Increase page size to handle more subjects | |
| }) | |
| Expect(err).ShouldNot(HaveOccurred()) | |
| ids = append(ids, response.GetSubjectIds()...) | |
| ct = response.GetContinuousToken() | |
| if ct == "" { | |
| break | |
| } | |
| } | |
| Expect(ids).Should(Equal(res)) |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores