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

RFC-82: Implement Support for Cedar Tags #11

Closed
wants to merge 8 commits into from
Closed

Conversation

adowair
Copy link

@adowair adowair commented Dec 4, 2024

No description provided.

ast.Principal().HasTag(ast.String("key")),
testutil.OK,
},
{

Choose a reason for hiding this comment

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

It would be nice to have folding examples here ... maybe there needs to be some extra data dumped into the context or something so that's possible? e.g. context.entity and then have some entity defined in the entities.

Copy link

@philhassey philhassey left a comment

Choose a reason for hiding this comment

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

Looks good!

  • corpus tests
  • verify 100% coverage
  • update to work against latest

Once those are done, you can close this PR and make a new PR against the actual cedar-go repo for final review, etc.

@@ -33,10 +34,12 @@ func (e Entity) MarshalJSON() ([]byte, error) {
UID ImplicitlyMarshaledEntityUID `json:"uid"`
Parents []ImplicitlyMarshaledEntityUID `json:"parents"`
Attributes Record `json:"attrs"`
Tags Record `json:"tags"`

Choose a reason for hiding this comment

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

The Rust implementation actually elides the field if it's empty, (presumably for backwards compatibility?). To match the behavior, we could use the omitempty option here.

Copy link
Author

@adowair adowair Dec 4, 2024

Choose a reason for hiding this comment

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

That's great to know (and it fixes some incompatibilities even in our codebase 😄)

Since Tags is a struct, it cannot have an empty value, so omitempty won't be enough here. I could let Tags be *Record, which would solve this problem right away, but that possibly introduces a host of other problems of mutability.

Aside: a new tag omitzero will be introduced in Go v1.24, which was made to address this exact scenario (golang/go#45669) (commit).

Choose a reason for hiding this comment

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

Mmm, right. Thanks for the reminder.

@philhassey
Copy link

Also, update the README.md removing the line about how entity tags are missing.

This commit will make changes to product code (and not test code),
to always instantiate Env.Entities to types.EntityMap{}, if it
has not been instantiated. This is a quick fix to safeguard against
nil pointer access Env.Entities when Entities is not set.

Signed-off-by: Ali Dowair <[email protected]>
This commit will add a new test to ensure the partial evaluation
of tag methods on context functions correctly.

Signed-off-by: Ali Dowair <[email protected]>
This commit will update the corpus test bundle to latest, and copies
one of the added corpus tests to TestCorpusRelated, to catch tag
related corner cases easier.

Signed-off-by: Ali Dowair <[email protected]>
@adowair
Copy link
Author

adowair commented Dec 6, 2024

Superseded by cedar-policy#77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants