Skip to content

Add support for new schema syntax - #142

Merged
khieta merged 4 commits into
mainfrom
khieta/add-new-schema-fmt
May 10, 2024
Merged

Add support for new schema syntax#142
khieta merged 4 commits into
mainfrom
khieta/add-new-schema-fmt

Conversation

@khieta

@khietakhieta commented May 9, 2024

Copy link
Copy Markdown

Issue #, if available:

Resolves#130

Description of changes:

@khietakhieta changed the title first passAdd support for new schema syntaxMay 9, 2024
Comment threadCedarJava/src/main/java/com/cedarpolicy/AuthorizationEngine.java Outdated
Comment threadCedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java Outdated
Comment threadCedarJavaFFI/src/interface.rs Outdated
Comment threadCedarJavaFFI/src/interface.rs Outdated
let schema_string = String::from(schema_jstring);
match Schema::from_str_natural(&schema_string) {
Err(e) => Err(Box::new(e)),
Ok(_) => Ok(JValueGen::Object(env.new_string("Success")?.into())),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Returns the string Success on success, but on the Java side, Schema.parse() seems to expect it to return the schema text?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, that is indeed not doing the right thing (and testing didn't catch it since we don't have any tests that do something with a parsed schema). Will fix in next commit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Curious why return the schema contents? Java already had a string containing the schema contents, I think?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, Java already has the string -- the only new information "returned" by the parsing function is whether an exception was thrown or not. Looking at it again, it likely saves some time not to pass back the string, so I'll revert to that.

@khieta

Copy link
Copy Markdown
Author

It turned out to not be too difficult to add support for the new integration tests, so I added that change to this PR.

"tests/example_use_cases/2c.json",
"tests/example_use_cases/3a.json",
"tests/example_use_cases/3b.json",
"tests/example_use_cases/3c.json",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] Can we keep the comment explaining why 4c isn't run?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I deleted it because that test was not moved over to https://github.com/cedar-policy/cedar-integration-tests -- do you think we should maintain this test in our integration test repo even if it's not used?

@khieta
khieta merged commit 73de8ca into mainMay 10, 2024
@khieta
khieta deleted the khieta/add-new-schema-fmt branch May 10, 2024 14:44
Sign up for freeto 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.

Add support for the new schema syntax

3 participants

@khieta@cdisselkoen@andrewmwells-amazon