Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: subject filtering pagination #1710

Merged
merged 1 commit into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/api-reference/apidocs.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"info": {
"title": "Permify API",
"description": "Permify is an open source authorization service for creating fine-grained and scalable authorization systems.",
"version": "v1.1.5",
"version": "v1.1.6",
"contact": {
"name": "API Support",
"url": "https://github.com/Permify/permify/issues",
Expand Down
2 changes: 1 addition & 1 deletion docs/api-reference/openapiv2/apidocs.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"info": {
"title": "Permify API",
"description": "Permify is an open source authorization service for creating fine-grained and scalable authorization systems.",
"version": "v1.1.5",
"version": "v1.1.6",
"contact": {
"name": "API Support",
"url": "https://github.com/Permify/permify/issues",
Expand Down
22 changes: 22 additions & 0 deletions internal/engines/lookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,14 +240,36 @@ func (engine *LookupEngine) LookupSubject(ctx context.Context, request *base.Per
}, nil
}

start := ""

// Sort the IDs
sort.Strings(ids)

// Handle continuous token if present
if request.GetContinuousToken() != "" {
var t database.ContinuousToken
t, err := utils.EncodedContinuousToken{Value: request.GetContinuousToken()}.Decode()
if err != nil {
return nil, err
}
start = t.(utils.ContinuousToken).Value
}

// Since the incoming 'ids' are already filtered based on the continuous token,
// there is no need to decode or handle the continuous token manually.
// The startIndex is initialized to 0.
startIndex := 0

if start != "" {
// Locate the position in the sorted list where the ID equals or exceeds the token value
for i, id := range ids {
if id >= start {
startIndex = i
break
}
}
}

// Convert size to int for compatibility with startIndex
pageSize := int(size)

Expand Down
183 changes: 183 additions & 0 deletions internal/engines/lookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4570,4 +4570,187 @@ var _ = Describe("lookup-entity-engine", func() {
}
})
})

exampleSchemaSubjectFilter := `
entity user {
attribute first_name string
}

entity org {
relation admin @user
relation perms @group_perms
}

entity project {
relation parent @org
relation admin @user
relation perms @group_perms

permission project_edit = admin or parent.admin or perms.project_edit
permission project_view = project_edit or perms.project_view
permission edit = perms.project_edit
}

entity group {
relation member @user
}

entity group_perms {
relation members @group

attribute can_project_edit boolean
attribute can_project_view boolean

permission project_edit = can_project_edit and members.member
permission project_view = (can_project_view and members.member) or project_edit
permission edit = can_project_edit
}
`

Context("Sample: Subject Filter", func() {
It("Sample: Case 1", func() {
db, err := factories.DatabaseFactory(
config.Database{
Engine: "memory",
},
)

Expect(err).ShouldNot(HaveOccurred())

conf, err := newSchema(exampleSchemaSubjectFilter)
Expect(err).ShouldNot(HaveOccurred())

schemaWriter := factories.SchemaWriterFactory(db)
err = schemaWriter.WriteSchema(context.Background(), conf)

Expect(err).ShouldNot(HaveOccurred())

type filter struct {
subjectReference string
entity string
assertions map[string][]string
}

tests := struct {
relationships []string
attributes []string
filters []filter
}{
relationships: []string{
"project:project_1#perms@group_perms:group_perms_1",
"project:project_2#perms@group_perms:group_perms_2",

"group_perms:group_perms_1#members@group:group_1",
"group:group_1#member@user:user_1",
"group:group_1#member@user:user_3",
"group:group_1#member@user:user_4",

"group:group_2#member@user:user_2",
},
attributes: []string{
"group_perms:group_perms_1$can_project_view|boolean:true",
"group_perms:group_perms_1$can_project_edit|boolean:true",
},
filters: []filter{
{
subjectReference: "user",
entity: "project:project_1",
assertions: map[string][]string{
"project_view": {"user_1", "user_3", "user_4"},
"edit": {"user_1", "user_3", "user_4"},
},
},
{
subjectReference: "user",
entity: "group_perms:group_perms_1",
assertions: map[string][]string{
"edit": {"user_1", "user_2", "user_3", "user_4"},
},
},
},
}

// filters

schemaReader := factories.SchemaReaderFactory(db)
dataReader := factories.DataReaderFactory(db)
dataWriter := factories.DataWriterFactory(db)

checkEngine := NewCheckEngine(schemaReader, dataReader)

lookupEngine := NewLookupEngine(
checkEngine,
schemaReader,
dataReader,
)

invoker := invoke.NewDirectInvoker(
schemaReader,
dataReader,
checkEngine,
nil,
lookupEngine,
nil,
)

checkEngine.SetInvoker(invoker)

var tuples []*base.Tuple

for _, relationship := range tests.relationships {
t, err := tuple.Tuple(relationship)
Expect(err).ShouldNot(HaveOccurred())
tuples = append(tuples, t)
}

var attributes []*base.Attribute

for _, attr := range tests.attributes {
a, err := attribute.Attribute(attr)
Expect(err).ShouldNot(HaveOccurred())
attributes = append(attributes, a)
}

_, err = dataWriter.Write(context.Background(), "t1", database.NewTupleCollection(tuples...), database.NewAttributeCollection(attributes...))
Expect(err).ShouldNot(HaveOccurred())

for _, filter := range tests.filters {
entity, err := tuple.E(filter.entity)
Expect(err).ShouldNot(HaveOccurred())

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))
Comment on lines +4721 to +4751
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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))

}
}
})
})
})
6 changes: 3 additions & 3 deletions internal/engines/subjectFilter.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,15 +429,15 @@ func (engine *SubjectFilter) subjectFilterDirectRelation(
// NewContextualRelationships() creates a ContextualRelationships instance from tuples in the request.
// QueryRelationships() then uses the filter to find and return matching relationships.
var cti *database.TupleIterator
cti, err = storageContext.NewContextualTuples(request.GetContext().GetTuples()...).QueryRelationships(filter, database.NewCursorPagination(database.Cursor(request.GetContinuousToken()), database.Sort("subject_id")))
cti, err = storageContext.NewContextualTuples(request.GetContext().GetTuples()...).QueryRelationships(filter, database.NewCursorPagination())
if err != nil {
return subjectFilterEmpty(), err
Comment on lines +432 to 434
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}

// Query the relationships for the entity in the request.
// TupleFilter helps in filtering out the relationships for a specific entity and a permission.
var rit *database.TupleIterator
rit, err = engine.dataReader.QueryRelationships(ctx, request.GetTenantId(), filter, request.GetMetadata().GetSnapToken(), database.NewCursorPagination(database.Cursor(request.GetContinuousToken()), database.Sort("subject_id")))
rit, err = engine.dataReader.QueryRelationships(ctx, request.GetTenantId(), filter, request.GetMetadata().GetSnapToken(), database.NewCursorPagination())
if err != nil {
return subjectFilterEmpty(), err
Comment on lines +440 to 442
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}
Expand Down Expand Up @@ -695,7 +695,7 @@ func subjectFilterUnion(ctx context.Context, functions []SubjectFilterFunction,
encounteredWildcard = true
// Collect any additional IDs alongside "<>", treat them as exclusions
for _, id := range d.resp {
if id != "<>" && !slices.Contains(excludedIds, id) {
if id != ALL && !slices.Contains(excludedIds, id) {
excludedIds = append(excludedIds, id)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var Identifier = ""
*/
const (
// Version is the last release of the Permify (e.g. v0.1.0)
Version = "v1.1.5"
Version = "v1.1.6"
)

// Function to create a single line of the ASCII art with centered content and color
Expand Down
3 changes: 3 additions & 0 deletions internal/storage/postgres/dataReader.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,9 @@ func (r *DataReader) HeadSnapshot(ctx context.Context, tenantID string) (token.S
return nil, utils.HandleError(ctx, span, err, base.ErrorCode_ERROR_CODE_SQL_BUILDER)
}

// TODO: To optimize this query, create the following index concurrently to avoid table locks:
// CREATE INDEX CONCURRENTLY idx_transactions_tenant_id_id ON transactions(tenant_id, id DESC);

// Execute the query and retrieve the highest transaction ID.
err = r.database.ReadPool.QueryRow(ctx, query, args...).Scan(&xid)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/pb/base/v1/openapi.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion proto/base/v1/openapi.proto
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {
info: {
title: "Permify API";
description: "Permify is an open source authorization service for creating fine-grained and scalable authorization systems.";
version: "v1.1.5";
version: "v1.1.6";
contact: {
name: "API Support";
url: "https://github.com/Permify/permify/issues";
Expand Down