Skip to content

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

Open
mark-creamer-amazon wants to merge 1 commit into
cedar-policy:mainfrom
mark-creamer-amazon:fix-entities-error-enumeration
Open

fix(CedarJavaFFI): exhaustive match broken on EntitiesError update#366
mark-creamer-amazon wants to merge 1 commit into
cedar-policy:mainfrom
mark-creamer-amazon:fix-entities-error-enumeration

Conversation

@mark-creamer-amazon

Copy link
Copy Markdown
Contributor

Problem

validate_entities in CedarJavaFFI/src/interface.rs matches exhaustively on cedar_policy::entities_errors::EntitiesError, enumerating all five variants with no wildcard arm. cedar-policy-core 4.12.0 added a sixth variant, InvalidEntityStructure, so the crate no longer compiles:

error[E0004]: non-exhaustive patterns: `EntitiesError::InvalidEntityStructure(_)` not covered
   --> src/interface.rs:509:37
    |
509 |             let err_message = match error {
    |                                     ^^^^^ pattern `EntitiesError::InvalidEntityStructure(_)` not covered
    |
note: `EntitiesError` defined here
   --> cedar-policy-core-4.12.0/src/entities/err.rs:24:1
    |
24  | pub enum EntitiesError {
    | ^^^^^^^^^^^^^^^^^^^^^^
...
50  |     InvalidEntityStructure(#[from] InvalidEntityStructureError),
    |     ---------------------- not covered

error: could not compile `cedar-java-ffi` (lib) due to 1 previous error

main is affected because CedarJavaFFI/Cargo.toml sources cedar from branch = "main", which now resolves to 4.12.0. Release branches such as release/4.10.x track frozen cedar branches and are unaffected.

Reproduce with a clean clone of main (cedar 4.12.0 requires rustc ≥ 1.89):

cd CedarJavaFFI && cargo +1.89 check

Fix

Keep explicit arms only for the variants whose own Display impl summarizes rather than delegating, and add a catch-all for the rest:

let err_message = match error {
    EntitiesError::Deserialization(err) => err.to_string(),
    EntitiesError::TransitiveClosureError(err) => err.to_string(),
    EntitiesError::InvalidEntity(err) => err.to_string(),
    err => err.to_string(),
};

Those three carry #[error("...")] messages that drop the inner error ("error during entity deserialization", "transitive closure computation/enforcement error", "entity does not conform to the schema"), so unwrapping them preserves the specific diagnostic returned to Java callers. Serialization interpolates its source via {0}, and Duplicate and InvalidEntityStructure are #[error(transparent)], so the catch-all loses no detail for them. These attributes are identical in 4.10.0 and 4.12.0.

This keeps the FFI compiling when new variants are added upstream, while retaining the detail the explicit match was there to provide.

Testing

cargo +1.89 check against cedar-policy-core 4.12.0 (efb14530) — compiles cleanly; the two remaining warnings are pre-existing on main.

Note for maintainers

EntitiesError is publicly re-exported as cedar_policy::entities_errors::EntitiesError and is not marked #[non_exhaustive], so adding a variant in 4.12.0 was a breaking change in a minor release. Maybe we should consider marking it #[non_exhaustive] in the cedar repo to prevent recurrence for downstream consumers?

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

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.

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

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.

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?

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