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

introduces bulk check permission API #75

Merged
merged 1 commit into from
Aug 10, 2023
Merged

introduces bulk check permission API #75

merged 1 commit into from
Aug 10, 2023

Conversation

vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Jul 21, 2023

Introduces Bulk Check API, which allows issuing multiple CheckPermission requests at once

considerations:

  • does not reuse CheckPermissionRequest nor Response because a bulk check call requires a single consistency level and returns a single checked at zedtoken
  • gRPC map type does not support messages as key, so a repeated pair message is created
  • the response are pair items that include the request and the corresponding response
  • two different APIs are introduced: one that streams responses as they are computed, and another one that blocks until all checks are completed. A client-streaming version was discarded for now.

considerations:
- does not reuse CheckPermissionRequest nor Response
  because a bulk check call requires a single
  consistency level and returns a single checked at zedtoken
- gRPC map type does not support messages as key, so a repeated
  pair message is created
- the response are pair items that include the request and the
  corresponding response, which would be either an error or a response item
- two different APIs are introduced: one that streams responses
  as they are computed, and another one that blocks until all checks
  are completed. A client-streaming version was discarded for now.
@vroldanbet vroldanbet marked this pull request as ready for review July 27, 2023 19:32
@josephschorr josephschorr merged commit 206ed0a into main Aug 10, 2023
3 checks passed
@josephschorr josephschorr deleted the bulk-check branch August 10, 2023 17:45
}

message BulkCheckPermissionRequestItem {
ObjectReference resource = 1 [ (validate.rules).message.required = true ];
Copy link
Contributor

@ecordell ecordell Aug 10, 2023

Choose a reason for hiding this comment

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

Was there any thought given to having these components be exploded/repeatable as well?

Then I could do things like:
check(document:1#read@user:(1,2,3,4,5))
or
check(document:(1,2,3,4,5)#read@user:1)

as part of a larger bulk check?

Basically I assume that a lot of bulk-checking use-cases would be bulk checking with most parameters fixed, and just one being "bulk". This API requires that you repeat yourself a lot.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that this API fits the simplest possible client logic (e.g. resources.map(x => x.proto())). It definitely uses more bandwidth, but I think it's a worthy tradeoff for UX.

I'd love to see how it plays out as an experiment and we can change it when if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants