fix(CedarJavaFFI): exhaustive match broken on EntitiesError update - #366
fix(CedarJavaFFI): exhaustive match broken on EntitiesError update#366mark-creamer-amazon wants to merge 1 commit into
Conversation
…in validate_entities
| EntitiesError::Duplicate(err) => err.to_string(), | ||
| EntitiesError::TransitiveClosureError(err) => err.to_string(), | ||
| EntitiesError::InvalidEntity(err) => err.to_string(), | ||
| err => err.to_string(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Add such constraints as either an upper version boundary or precise version locking similar to the cedar-policy packages themselves (using
"=VERSION"or<MAJOR.minorfor the last minor version you know your release is compatible with. - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed with the above! In that case, are we aligned on this change to patch Cedar Java?
Problem
validate_entitiesinCedarJavaFFI/src/interface.rsmatches exhaustively oncedar_policy::entities_errors::EntitiesError, enumerating all five variants with no wildcard arm.cedar-policy-core4.12.0 added a sixth variant,InvalidEntityStructure, so the crate no longer compiles:mainis affected becauseCedarJavaFFI/Cargo.tomlsources cedar frombranch = "main", which now resolves to 4.12.0. Release branches such asrelease/4.10.xtrack frozen cedar branches and are unaffected.Reproduce with a clean clone of
main(cedar 4.12.0 requires rustc ≥ 1.89):Fix
Keep explicit arms only for the variants whose own
Displayimpl summarizes rather than delegating, and add a catch-all for the rest: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}, andDuplicateandInvalidEntityStructureare#[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
EntitiesErroris publicly re-exported ascedar_policy::entities_errors::EntitiesErrorand 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?