Skip to content

Comments

Adds annotation support for Policy#296

Merged
muditchaudhary merged 9 commits intocedar-policy:mainfrom
muditchaudhary:addsPolicyAnnotations
Feb 25, 2025
Merged

Adds annotation support for Policy#296
muditchaudhary merged 9 commits intocedar-policy:mainfrom
muditchaudhary:addsPolicyAnnotations

Conversation

@muditchaudhary
Copy link
Contributor

Description

This PR implements annotation getters for the Policy class, providing access to both static policy and template policy annotations.

Key Changes

  • Implements a Java HashMap in CedarJavaFFI for annotation management and JNI communication
  • Adds annotation access methods to Policy class:
    • getAnnotation(String key)
    • getAnnotations()
  • Implements lazy loading pattern for annotations:
    • Annotations initialize via JNI only on first getter call
    • Subsequent calls utilize cached annotations
  • Adds test coverage for:
    • New FFI methods
    • CedarJavaFFI HashMap implementation
    • Policy annotation getters

Implementation Details

  1. Handling Static Policies and Templates

    • Uses try-catch approach to attempt static policy annotation retrieval first
    • Falls back to template annotation retrieval on failure (errorMessage.contains("expected a static policy"))
    • Current implementation relies on error message parsing (precedent in PolicyTests). Limited by information exposed by cedar-policy's ParseError (tracking in #979)
    • Alternative: Completely consume ParseError for static policies (as in PR Exposes policy effect for static policies and templates #275)
    • Temporary workaround until Policy and Template class separation
  2. Lazy Loading Strategy

    • Loads annotations on-demand instead of in Policy constructor to preserve backward compatibility by avoiding addition of InternalException in constructor signature.

jtbrain and others added 9 commits February 14, 2025 20:08
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
@muditchaudhary muditchaudhary marked this pull request as ready for review February 18, 2025 20:58
*
* @throws InternalException if there is an error loading or parsing the annotations
*/
private void ensureAnnotationsLoaded() throws InternalException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we can't include this snippet of code that should be run once per instance in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snippet throws an InternalException, but the current constructor doesn't declare this exception. Moving this code into the constructor would require us to either add throws InternalException to the constructor signature, which would be a breaking change for existing users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good reason for this major version at least 👍

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

looks reasonable

@muditchaudhary muditchaudhary merged commit 21d7f45 into cedar-policy:main Feb 25, 2025
4 checks passed
@muditchaudhary muditchaudhary mentioned this pull request Mar 4, 2025
2 tasks
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.

4 participants