Complete Cedar Policy AST Implementation#3
Conversation
kostyantynv
left a comment
kostyantynv
left a comment
There was a problem hiding this comment.
Thanks Bec, looks great. A few minor comments left for your discretion. Feel free to merge.
rsvp/policy-ast/src/main/java/uq/pac/rsvp/policy/ast/schema/attribute/AttributeType.java
Outdated
Show resolved
Hide resolved
rsvp/policy-ast/src/main/java/uq/pac/rsvp/policy/ast/schema/attribute/EntityOrCommonType.java
Outdated
Show resolved
Hide resolved
rsvp/policy-ast/src/main/java/uq/pac/rsvp/policy/ast/schema/attribute/PrimitiveType.java
Outdated
Show resolved
Hide resolved
rsvp/policy-ast/src/main/java/uq/pac/rsvp/policy/ast/visitor/SchemaVisitor.java
Show resolved
Hide resolved
rebecca-odonoghue
force-pushed
the
policy-ast
branch
from
9956a08 to
8c6075b
Compare
|
@kostyantynv I have added the new visitors as we discussed, more info can be found in README. I haven't written any tests for them yet, but hopefully they are okay. I will write some more tests now and once that is done we can merge the PR. @davmac314 FYI these are the pending AST changes, if you have any feedback or requests based on your usage. |
Looks fine to me |
|
Oh if the API changes again though could you please bump the version number (in |
* Fix immutable fields * Fix .gitignore * Re-add Guava dependency
rebecca-odonoghue
force-pushed
the
policy-ast
branch
from
5c59812 to
b02fcf5
Compare
|
@kostyantynv I've noticed that the behaviour of "required" will be wrong because record attributes are required by default, and there is no difference between omitting the property in JSON or explicitly setting it to false. So, we will need to check the JSON to see if "required" is specified or not, rather than replying on gson for this. For now, I am leaving it as-is. I'm happy for this to be merged whenever, but you might want to look at the new changes first. Note that the merge options have changed, even though it has "rebase and merge" as an option, I don't think it will need to actually rebase. |
| import uq.pac.rsvp.policy.ast.visitor.SchemaComputationVisitor; | ||
| import uq.pac.rsvp.policy.ast.visitor.SchemaVisitor; | ||
|
|
||
| public class EntityOrCommonType extends AttributeType { |
There was a problem hiding this comment.
Based on what we have discovered so far about cedar types, let's require a schema to reason about resolved types. This needs to be broken into an entity type and a common type (tracking a its record)
| @@ -0,0 +1,109 @@ | |||
| package uq.pac.rsvp.policy.ast.schema; | |||
There was a problem hiding this comment.
I understand that this corresponds to Entity definition, so perhaps it should be named as such, otherwise it looks like an ad-hoc type outside of the attribute hierarchy. Especially, considering that in a resolved there is an entity type. On a similar note, we'd need a definition for a common type as well
| @@ -0,0 +1,79 @@ | |||
| package uq.pac.rsvp.policy.ast.schema.attribute; | |||
There was a problem hiding this comment.
That would be more convenient if we could split primitive types in respective sub-classes, e.g., StringPrimitiveType etc
3 participants
I've relocated the library from
rsvp/libtorsvp/policy-astin anticipation of bringing in the datalog stuff.I've also moved the git files that were generated in
rsvpinto the root directory because that's my preference.I also renamed the artifact produced by the library to
uq.pac.rsvp:policy-ast:1.0.0because I think it makes more sense given the new project structure.I am still writing tests and need to rebase onto main, but I am opening the PR so people can comment on the implementation if they want to.