Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
any
andall
operators #21Add
any
andall
operators #21Changes from all commits
04dc037
fc98955
a20d5cf
053def9
878492e
e5b005d
5722e9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this alternative:
You evaluate the predicate on every member of the set, and collect all of the errors to be returned in a deterministic order? So in your example
[1, true].all(it like "foo")
you'd return the equivalent ofThe order in the list is based on a global order defined on Cedar values, e.g., that all Booleans come before all Longs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you could sort based on error type, and ignore the contents of the error message and the expression that accompanies it. I believe that matches the level of abstraction we have today when considering Cedar error messages in the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that using a list is a viable alternative because it forces the specification to impose an (arbitrary) order on values and/or error types. Then, every implementation has to enforce the same order.
If we want to go this route, how about returning a set of error messages instead?
Not that this a non-trivial change because it changes the return type of the evaluator from
Result<Value, Error>
toResult<Value, Set<Error>>
.Another alternative is to package this error set into the QuantifierError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying the result as a set feels like kicking the can; oftentimes sets are implemented as search trees, which require an ordering.
I feel that the loss in error specificity of the proposal is a potentially significant hit to the user experience. Without coupling errors with the actual expressions, we are still short of the best possible debugging experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can preserve the mapping: use a map from values to errors (instead of a set). That still avoids having to impose an arbitrary ordering in the specification, and allows implementations to use a HashMap if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug mode probably is another evaluator implementation which we have to prove/test is equivalent to the production one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is still the sticking point of this RFC. To recap:
The proposal in the RFC is that any error that results from
foo.any(...)
orfoo.all(...)
is emitted from the evaluator as aQuantifierError
with the location of the whole expression. The pro is a simple, efficient semantics, which admits short-circuiting when the first error occurs, and enjoys O(1) output size. The con is a lack of specific information about what element offoo
was the source of the error, and what the error actually is (e.g.,TypeError
). @cdisselkoen and I are worried about the impact on user experience.To overcome this con we could elect to package more details in the
QuantifierError
, e.g., as a map from element infoo
to the associated error. Doing this adds a bit more complexity to the implementation, but not much. It eliminates the ability to do short-circuiting in evaluation once an error, and makes output size O(n) for the policy in the worst case. But these downsides are minimal, to my mind. Using a map does not impose an order on Cedar values (which would have added a lot of pain/complexity). The worst case (every element in the set produces an error) is unlikely to happen in practice, and it won't affect the performance of non-erroring evaluation. All of this is worth it, to my mind, because it values customer experience. Good errors are extremely important.So: I propose we change the proposal to the above, and then accept it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds reasonable to me.
As an implementation note, the natural choice for the map from
foo
element to error would probably be a map fromExpr
to error. (Note thatExpr
implementsHash
so it can be put in an unordered hashmap.) But I'm a little less clear on what type the keys should be if you are calling.any()
or.all()
on a set that originates from the entity data orContext
.Expr
is still one option (we could easily convert theRestrictedExpr
s toExpr
and return them in that form), but RFC 34 proposes not keeping around theRestrictedExpr
by the time we get to evaluation. At that point you'd have to have the key beValue
, but those are notHash
.Value
s are totally ordered, so you can put them in a treemap, but then your concern above about appearing unordered is kind of a fiction. Moreover, ifValue
s are indeed the key type, they'd have to be made public -- something which is also not currently the case but also proposed in RFC 34.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an acceptable workaround to the above would be to have the key be the string representation of the
Value
; the string representation would be trivially total-ordered, hashable, and useful for debugging, while dodging the issues about whetherValue
is made public.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to keep the size of the evaluator output constant. Letting it grow linearly with input size (policies + request + entities) means that the authorizer's output is worst-case quadratic in input size. This is undesirable even if rare.
I've revised the RFC to relax the specification of
QuantifierError
so that it can include more information in the error message, as long as the additional information is computed deterministically and is bounded in size.