Skip to content

fix(CedarJavaFFI): exhaustive match broken on EntitiesError update #366

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.

Sign up for GitHub

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

Merged
mark-creamer-amazon merged 2 commits into from
Aug 11, 2026
+50 −7
Merged

fix(CedarJavaFFI): exhaustive match broken on EntitiesError update #366

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bee2b4
fix(CedarJavaFFI): performing non exhaustive match on entities error …
mark-creamer-amazon Aug 3, 2026
21fb9ba
fix: support policyFormat and schemaFormat in shared integration tests
mark-creamer-amazon Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions CedarJavaFFI/src/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,12 +506,16 @@ pub fn validate_entities(input: &str) -> serde_json::Result<Answer> {

match CedarEntities::from_json_value(validate_entity_call.entities, Some(&schema)) {
Err(error) => {
// Unwrap only the variants whose own `Display` impl summarizes instead of
// delegating, so the caller keeps the specific inner diagnostic. The rest
// are `#[error(transparent)]` or already interpolate their source, so the
// catch-all preserves their detail and keeps this compiling when upstream
// adds a variant.
let err_message = match error {
EntitiesError::Serialization(err) => err.to_string(),
EntitiesError::Deserialization(err) => err.to_string(),
EntitiesError::Duplicate(err) => err.to_string(),
EntitiesError::TransitiveClosureError(err) => err.to_string(),
EntitiesError::InvalidEntity(err) => err.to_string(),
err => err.to_string(),

@lianah lianah Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If someone adds a new EntitiesError that has it's own Display summarizing the issue without interpolating this would not break cedar-java compilation but it would silently drop the inner detailed error message right? I wonder if a compilation error forcing us to fix this is not preferable to silently dropping the inner error message.

@teruokun teruokun Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The issue you have is that right now, your consumers are the ones broken since your own package doesn't lock down to compatible versions in any way and this is why libraries with Enums they plan to update often add a non-exhaustive directive to ensure proper compatibility guarantees are kept. In other words, you're violating core sem-ver principles in Rust where you're actually breaking on minor version updates to your underlying dependencies but don't constrain your dependencies for your consumed libraries to match.

So the ask would be either to:

  1. Add such constraints as either an upper version boundary or precise version locking similar to the cedar-policy packages themselves (using "=VERSION" or <MAJOR.minor for the last minor version you know your release is compatible with.
  2. Make the package flexible enough to accommodate additions that aren't considered "breaking" for the purposes of Rust semver

Otherwise you're just making all of your consumers have to lock or constrain otherwise transitive dependencies that they don't directly use. I'm not saying it isn't an available tool, but it's effectively passing breakages to your consumers for reporting them much like this one which doesn't engender trust if that isn't well-understood at outset.

@john-h-kastner-aws john-h-kastner-aws Aug 6, 2026 •
edited
Loading

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.

The new error message variant shouldn't have made it into the release. We have cargo-semver-checks to guard against this, but it's not perfect.

Probably the best option for now is to roll forward, leaving the new variant in place and patching the Java as propsed here. Adding #[non_exhaustive] is also breaking so unfortunately we can't just patch that in.

@mark-creamer-amazon mark-creamer-amazon Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed with the above! In that case, are we aligned on this change to patch Cedar Java?

};
Ok(Answer::fail_bad_request(vec![err_message]))
}
Expand Down
Loading