From 527605965fbea65a19bd0ed42baa04c333e6267b Mon Sep 17 00:00:00 2001 From: James Mulcahy Date: Sat, 22 Aug 2026 00:51:26 +0000 Subject: [PATCH 1/5] Surface structured parse diagnostics from PolicySet.parsePolicies Parse failures are reduced to format!("Internal JNI Error: {e}") in jni_failed, discarding the miette diagnostic cedar produced: the source span, the tokens the parser expected, and the help text. ParseErrors' Display also prints only its first error, so subsequent errors are lost. Add PolicyParseException, a subclass of InternalException carrying List - the same representation the validation path already returns. parsePoliciesJni downcasts ParseErrors and converts each ParseError via the existing From<&E: miette::Diagnostic> impl. If building the richer exception fails for any reason the generic path is used, so a parse error can never become a different kind of failure. Existing catch (InternalException) blocks are unaffected. Two message details change deliberately: the "Internal JNI Error: " prefix is dropped, since it describes the binding rather than the policy and reads as a library fault rather than a typo the caller can fix; and getErrors() carries one entry per parse error rather than a single entry for the whole document, which is what its plural contract always implied. Signed-off-by: James Mulcahy --- .../model/exception/PolicyParseException.java | 84 +++++++++++++++ .../PolicyParseDiagnosticsTests.java | 102 ++++++++++++++++++ CedarJavaFFI/src/interface.rs | 85 ++++++++++++++- 3 files changed, 267 insertions(+), 4 deletions(-) create mode 100644 CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java create mode 100644 CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java new file mode 100644 index 00000000..a228630b --- /dev/null +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java @@ -0,0 +1,84 @@ +/* + * Copyright Cedar Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.cedarpolicy.model.exception; + +import com.cedarpolicy.CedarJson; +import com.cedarpolicy.model.DetailedError; +import com.fasterxml.jackson.core.type.TypeReference; +import java.util.Collections; +import java.util.List; + +/** + * Thrown when Cedar policy text fails to parse, carrying the structured diagnostics Cedar + * produced for each error. + * + *

Cedar reports parse failures as {@code miette} diagnostics: a message, the source span + * of the offending token, the tokens the parser expected there, and often help text. Prior + * to this type those were flattened to a single {@code Display} string, so callers saw + * "unexpected token `::`" with no indication of where in the policy it occurred, and every + * error after the first was discarded. {@link #getDetailedErrors()} returns the full set, + * one {@link DetailedError} per parse error, in the order Cedar reported them. + * + *

Extends {@link InternalException} so existing {@code catch} blocks are unaffected. + * Two message details differ from the generic error path, deliberately: the + * {@code "Internal JNI Error: "} prefix is dropped, because it describes the binding rather + * than the policy and reads as a library fault rather than a typo in the caller's input; + * and {@link #getErrors()} carries one entry per parse error rather than a single entry for + * the whole document, which is what its plural contract always implied. + */ +public class PolicyParseException extends InternalException { + + private static final TypeReference> ERROR_LIST = + new TypeReference>() {}; + + private final transient List detailedErrors; + + /** + * Construct from the JSON array of {@code DetailedError} the native layer serialises. + * + * @param messages one message per parse error, for {@link #getErrors()} + * @param detailedErrorsJson JSON array of {@code DetailedError}; if it cannot be read, + * the exception still carries {@code messages} and {@link #getDetailedErrors()} + * returns empty, so a serialisation change can never turn a parse error into a + * different failure + */ + public PolicyParseException(String[] messages, String detailedErrorsJson) { + super(messages); + this.detailedErrors = readDetailedErrors(detailedErrorsJson); + } + + private static List readDetailedErrors(String json) { + if (json == null || json.isEmpty()) { + return List.of(); + } + try { + List parsed = CedarJson.objectReader().forType(ERROR_LIST).readValue(json); + return parsed == null ? List.of() : List.copyOf(parsed); + } catch (Exception e) { + return List.of(); + } + } + + /** + * The structured diagnostics for each parse error, including source spans and help text. + * + * @return the diagnostics, or an empty list if none could be recovered + */ + public List getDetailedErrors() { + return Collections.unmodifiableList(detailedErrors); + } +} diff --git a/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java b/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java new file mode 100644 index 00000000..22f46f65 --- /dev/null +++ b/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java @@ -0,0 +1,102 @@ +/* + * Copyright Cedar Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.cedarpolicy; + +import com.cedarpolicy.model.DetailedError; +import com.cedarpolicy.model.exception.InternalException; +import com.cedarpolicy.model.exception.PolicyParseException; +import com.cedarpolicy.model.policy.PolicySet; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Parse failures carry Cedar's structured diagnostics, not just a flattened string. */ +public class PolicyParseDiagnosticsTests { + + @Test + public void parseFailureCarriesSourceSpanAndExpectedTokens() { + // An entity literal in the action slot: the scope needs `action == ...`. + String src = "forbid(principal, Foo::Action::\"Read\", resource);"; + PolicyParseException e = + assertThrows(PolicyParseException.class, () -> PolicySet.parsePolicies(src)); + + List details = e.getDetailedErrors(); + assertEquals(1, details.size()); + DetailedError error = details.get(0); + assertTrue(error.message.contains("unexpected token `::`"), error.message); + + assertEquals(1, error.sourceLocations.size()); + DetailedError.SourceLabel span = error.sourceLocations.get(0); + // The span must cover the offending `::`, so callers can underline it. + assertEquals(src.indexOf("::"), span.start); + assertEquals(src.indexOf("::") + 2, span.end); + assertTrue(span.label.orElse("").contains("expected"), span.label.toString()); + } + + @Test + public void everyParseErrorIsReportedNotJustTheFirst() { + // ParseErrors' Display prints only the first error, so the flattened path reported + // one string for the whole document. Both accessors now carry all of them. + String src = "forbid(principal, Foo::Action::\"A\", resource);\n" + + "permit(principal, action, resource) when { 1 + };"; + PolicyParseException e = + assertThrows(PolicyParseException.class, () -> PolicySet.parsePolicies(src)); + + assertEquals(2, e.getDetailedErrors().size()); + assertEquals(2, e.getErrors().size()); + } + + @Test + public void messagesDropTheInternalJniErrorPrefix() { + // That prefix describes the binding, not the policy: reading "Internal JNI Error" + // for an ordinary typo suggests a library fault rather than a fixable mistake. + PolicyParseException e = assertThrows(PolicyParseException.class, + () -> PolicySet.parsePolicies("forbid(principal, Foo::Action::\"Read\", resource);")); + + assertEquals("Internal error: unexpected token `::`", e.getMessage()); + assertEquals(List.of("unexpected token `::`"), e.getErrors()); + } + + @Test + public void helpTextSurvivesWhenCedarSuppliesIt() { + PolicyParseException e = assertThrows(PolicyParseException.class, + () -> PolicySet.parsePolicies("permit(principle, action, resource);")); + + DetailedError error = e.getDetailedErrors().get(0); + assertTrue(error.help.isPresent(), "expected help text for an invalid scope variable"); + assertTrue(error.help.get().contains("principal"), error.help.get()); + } + + @Test + public void remainsCatchableAsInternalException() { + // PolicyParseException extends InternalException so existing callers keep working. + InternalException e = assertThrows(InternalException.class, + () -> PolicySet.parsePolicies("permit(principal, action, resource)")); + assertFalse(e.getErrors().isEmpty()); + } + + @Test + public void validPolicySetStillParses() { + org.junit.jupiter.api.Assertions.assertDoesNotThrow( + () -> PolicySet.parsePolicies("permit(principal, action, resource);")); + } +} diff --git a/CedarJavaFFI/src/interface.rs b/CedarJavaFFI/src/interface.rs index b03841e6..f57a2420 100644 --- a/CedarJavaFFI/src/interface.rs +++ b/CedarJavaFFI/src/interface.rs @@ -22,13 +22,13 @@ use cedar_policy::ffi::{ }; use cedar_policy::{ ffi::{is_authorized_json_str, validate_json_str}, - Authorizer, Entities as CedarEntities, EntityUid, Policy, PolicySet, Request, Schema, SlotId, - Template, + Authorizer, Entities as CedarEntities, EntityUid, ParseErrors, Policy, PolicySet, Request, + Schema, SlotId, Template, }; use cedar_policy_formatter::{policies_str_to_pretty, Config}; use dashmap::DashMap; use jni::{ - objects::{JClass, JObject, JString, JValueGen, JValueOwned}, + objects::{JClass, JObject, JString, JThrowable, JValueGen, JValueOwned}, sys::{jstring, jvalue}, JNIEnv, }; @@ -534,6 +534,77 @@ struct JavaInterfaceCall { arguments: String, } +/// Throw a `PolicyParseException` carrying Cedar's structured diagnostics for each parse +/// error. +/// +/// `jni_failed` reduces any error to `format!("Internal JNI Error: {e}")`, which for a parse +/// failure discards everything `miette` recorded — the source span, the tokens the parser +/// expected, the help text — and, because `ParseErrors`' `Display` prints only its first +/// error, every subsequent error too. Parse failures are the errors a policy author is most +/// likely to hit and the ones where that detail matters most, so they are additionally +/// converted to `DetailedError` (the same representation the validation path already +/// returns) and handed to Java intact. +/// +/// The `"Internal JNI Error: "` prefix is dropped for these: it describes the binding +/// rather than the policy, and reading "Internal error" for an ordinary typo suggests a +/// library fault rather than something the caller can fix. `getErrors()` likewise carries +/// one entry per parse error instead of a single entry for the whole document. +fn throw_parse_errors(env: &mut JNIEnv<'_>, errs: &ParseErrors) { + if env.exception_check().unwrap_or_default() { + return; // An exception is already in flight; let it propagate. + } + let details: Vec = errs.iter().map(DetailedError::from).collect(); + let messages: Vec = details.iter().map(|d| d.message.clone()).collect(); + let details_json = serde_json::to_string(&details).unwrap_or_default(); + + // Fall back to the generic path if any part of building the richer exception fails, so + // a parse error is never silently turned into a different kind of failure. + match build_parse_exception(env, &messages, &details_json) { + Ok(exception) => { + if env.throw(exception).is_err() { + throw_internal(env, errs); + } + } + Err(_) => throw_internal(env, errs), + } +} + +/// Build `PolicyParseException(String[] messages, String detailedErrorsJson)`. +fn build_parse_exception<'a>( + env: &mut JNIEnv<'a>, + messages: &[String], + details_json: &str, +) -> Result> { + let string_class = env.find_class("java/lang/String")?; + let messages_array = + env.new_object_array(messages.len() as i32, &string_class, JObject::null())?; + for (i, message) in messages.iter().enumerate() { + let jmessage = env.new_string(message)?; + env.set_object_array_element(&messages_array, i as i32, jmessage)?; + } + let jdetails = env.new_string(details_json)?; + let exception = env.new_object( + "com/cedarpolicy/model/exception/PolicyParseException", + "([Ljava/lang/String;Ljava/lang/String;)V", + &[ + JValueGen::Object(&messages_array), + JValueGen::Object(&jdetails), + ], + )?; + Ok(JThrowable::from(exception)) +} + +/// Throw a plain `InternalException`, exactly as `jni_failed` would have. +fn throw_internal(env: &mut JNIEnv<'_>, errs: &ParseErrors) { + // We have to unwrap here as we're doing exception handling + // If we don't have the heap space to create an exception, the only valid move is ending the process + env.throw_new( + "com/cedarpolicy/model/exception/InternalException", + format!("Internal JNI Error: {errs}"), + ) + .unwrap(); +} + fn jni_failed(env: &mut JNIEnv<'_>, e: &dyn Error) -> jvalue { // If we already generated an exception, then let that go up the stack // Otherwise, generate a cedar InternalException and return null @@ -655,7 +726,13 @@ fn policy_set_to_json_internal<'a>( #[jni_fn("com.cedarpolicy.model.policy.PolicySet")] pub fn parsePoliciesJni<'a>(mut env: JNIEnv<'a>, _: JClass, policies_jstr: JString<'a>) -> jvalue { match parse_policies_internal(&mut env, policies_jstr) { - Err(e) => jni_failed(&mut env, e.as_ref()), + Err(e) => match e.downcast_ref::() { + Some(parse_errors) => { + throw_parse_errors(&mut env, parse_errors); + JValueOwned::Object(JObject::null()).as_jni() + } + None => jni_failed(&mut env, e.as_ref()), + }, Ok(policies_set) => policies_set.as_jni(), } } From 20cc7c3e8a625b215d871c8d0a690fe18f282944 Mon Sep 17 00:00:00 2001 From: James Mulcahy Date: Tue, 15 Sep 2026 23:46:37 +0000 Subject: [PATCH 2/5] Preserve InternalException.getMessage() for parse failures Review feedback on #367: dropping the "Internal JNI Error: " prefix from getMessage() is a breaking change. Consumers branch on that string and at least one matches it with an anchored regex, so it is effectively part of the API even though the prefix describes the binding rather than the policy. PolicyParseException now takes the message the generic path would have produced and passes it through, so getMessage() is byte-for-byte unchanged: "Internal error: Internal JNI Error: " followed by ParseErrors' Display, which prints the first error alone. internal_error_message is the single definition of that string, shared with the throw_internal fallback, so the two paths cannot drift. The added detail is reached through the accessors instead. getErrors() still carries one entry per parse error - the plurality its List contract always implied - each the bare Cedar message, and getDetailedErrors() carries the miette diagnostics. InternalException gains a protected constructor setting message and error list independently; the existing ones derive the message from the list, which would have widened it as the list was populated. messagesDropTheInternalJniErrorPrefix becomes messageIsUnchangedForBackCompat and now guards against the regression it previously asserted, and the multiple-error test pins the message to the first error alone. Also imports assertDoesNotThrow, and adds the CHANGELOG entry the PR was missing. Signed-off-by: James Mulcahy Co-Authored-By: Claude Opus 5 (1M context) --- CedarJava/CHANGELOG.md | 1 + .../model/exception/InternalException.java | 16 +++++++++++ .../model/exception/PolicyParseException.java | 28 +++++++++++-------- .../PolicyParseDiagnosticsTests.java | 18 ++++++++---- CedarJavaFFI/src/interface.rs | 27 ++++++++++++------ 5 files changed, 65 insertions(+), 25 deletions(-) diff --git a/CedarJava/CHANGELOG.md b/CedarJava/CHANGELOG.md index 283fbd26..81d7c07c 100644 --- a/CedarJava/CHANGELOG.md +++ b/CedarJava/CHANGELOG.md @@ -8,6 +8,7 @@ * Added Offset function support [#331](https://github.com/cedar-policy/cedar-java/pull/331) * Added PolicySet to JSON conversion API [#329](https://github.com/cedar-policy/cedar-java/pull/329) * Added Cedar Schema support for Entity Validation [#332](https://github.com/cedar-policy/cedar-java/pull/332) +* Added `PolicyParseException`, thrown by `PolicySet.parsePolicies` when policy text fails to parse. It is a subclass of `InternalException`, so existing `catch` blocks and `getMessage()` are unaffected, and adds `getDetailedErrors()` returning Cedar's structured diagnostics - source span, expected tokens, and help text - for each error. `getErrors()` now carries one entry per parse error rather than a single entry for the whole document [#367](https://github.com/cedar-policy/cedar-java/pull/367) ## 4.3.1 ### Added diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java index 16c34fd2..ba955ef4 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java @@ -41,6 +41,22 @@ public InternalException(String[] errors) { this.errors = new ArrayList<>(Arrays.asList(errors)); } + /** + * Internal exception whose message is not derived from its error list. + * + *

The other constructors build the message by joining {@code errors}, which ties the + * two together: a more finely split list necessarily changes the message. Subclasses that + * report each underlying error separately while keeping the message they have always + * produced use this constructor to set the two independently. + * + * @param error the message, prefixed as in {@link #InternalException(String)} + * @param errors the individual error messages, for {@link #getErrors()} + */ + protected InternalException(String error, List errors) { + super("Internal error: " + error); + this.errors = new ArrayList<>(errors); + } + /** * Get errors. * diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java index a228630b..415cc574 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java @@ -19,6 +19,7 @@ import com.cedarpolicy.CedarJson; import com.cedarpolicy.model.DetailedError; import com.fasterxml.jackson.core.type.TypeReference; +import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -33,12 +34,15 @@ * error after the first was discarded. {@link #getDetailedErrors()} returns the full set, * one {@link DetailedError} per parse error, in the order Cedar reported them. * - *

Extends {@link InternalException} so existing {@code catch} blocks are unaffected. - * Two message details differ from the generic error path, deliberately: the - * {@code "Internal JNI Error: "} prefix is dropped, because it describes the binding rather - * than the policy and reads as a library fault rather than a typo in the caller's input; - * and {@link #getErrors()} carries one entry per parse error rather than a single entry for - * the whole document, which is what its plural contract always implied. + *

Extends {@link InternalException} so existing {@code catch} blocks are unaffected, and + * {@link #getMessage()} is byte-for-byte what the generic error path produced: callers are + * known to branch on that string and to match it with anchored regexes, so it is treated as + * part of the API and left alone. The new detail is reached through the accessors instead. + * + *

{@link #getErrors()} does change: it now carries one entry per parse error rather than + * a single entry for the whole document, which is what its plural contract always implied, + * and each entry is the bare Cedar message without the {@code "Internal JNI Error: "} + * prefix, which described the binding rather than any one error. */ public class PolicyParseException extends InternalException { @@ -50,14 +54,16 @@ public class PolicyParseException extends InternalException { /** * Construct from the JSON array of {@code DetailedError} the native layer serialises. * + * @param message the message, which the native layer builds exactly as the generic + * error path does so that {@link #getMessage()} is unchanged * @param messages one message per parse error, for {@link #getErrors()} * @param detailedErrorsJson JSON array of {@code DetailedError}; if it cannot be read, - * the exception still carries {@code messages} and {@link #getDetailedErrors()} - * returns empty, so a serialisation change can never turn a parse error into a - * different failure + * the exception still carries {@code message} and {@code messages}, and + * {@link #getDetailedErrors()} returns empty, so a serialisation change can never + * turn a parse error into a different failure */ - public PolicyParseException(String[] messages, String detailedErrorsJson) { - super(messages); + public PolicyParseException(String message, String[] messages, String detailedErrorsJson) { + super(message, Arrays.asList(messages)); this.detailedErrors = readDetailedErrors(detailedErrorsJson); } diff --git a/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java b/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java index 22f46f65..a52fcdc1 100644 --- a/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java +++ b/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java @@ -24,6 +24,7 @@ import java.util.List; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -63,16 +64,22 @@ public void everyParseErrorIsReportedNotJustTheFirst() { assertEquals(2, e.getDetailedErrors().size()); assertEquals(2, e.getErrors().size()); + // The message stays what Display gave it - the first error alone - so populating the + // list cannot widen the string that existing callers match on. + assertEquals("Internal error: Internal JNI Error: " + e.getErrors().get(0), e.getMessage()); } @Test - public void messagesDropTheInternalJniErrorPrefix() { - // That prefix describes the binding, not the policy: reading "Internal JNI Error" - // for an ordinary typo suggests a library fault rather than a fixable mistake. + public void messageIsUnchangedForBackCompat() { + // Callers branch on getMessage() and match it with anchored regexes, so the string + // stays exactly as the generic error path wrote it, "Internal JNI Error: " and all. + // The added detail is reached through the accessors instead. PolicyParseException e = assertThrows(PolicyParseException.class, () -> PolicySet.parsePolicies("forbid(principal, Foo::Action::\"Read\", resource);")); - assertEquals("Internal error: unexpected token `::`", e.getMessage()); + assertEquals("Internal error: Internal JNI Error: unexpected token `::`", e.getMessage()); + // getErrors() entries are the bare Cedar messages: the prefix described the binding + // rather than any one error, and is meaningless once the list is per-error. assertEquals(List.of("unexpected token `::`"), e.getErrors()); } @@ -96,7 +103,6 @@ public void remainsCatchableAsInternalException() { @Test public void validPolicySetStillParses() { - org.junit.jupiter.api.Assertions.assertDoesNotThrow( - () -> PolicySet.parsePolicies("permit(principal, action, resource);")); + assertDoesNotThrow(() -> PolicySet.parsePolicies("permit(principal, action, resource);")); } } diff --git a/CedarJavaFFI/src/interface.rs b/CedarJavaFFI/src/interface.rs index f57a2420..80273e12 100644 --- a/CedarJavaFFI/src/interface.rs +++ b/CedarJavaFFI/src/interface.rs @@ -545,10 +545,11 @@ struct JavaInterfaceCall { /// converted to `DetailedError` (the same representation the validation path already /// returns) and handed to Java intact. /// -/// The `"Internal JNI Error: "` prefix is dropped for these: it describes the binding -/// rather than the policy, and reading "Internal error" for an ordinary typo suggests a -/// library fault rather than something the caller can fix. `getErrors()` likewise carries -/// one entry per parse error instead of a single entry for the whole document. +/// The message is deliberately left as `jni_failed` would have written it, prefix and all: +/// callers are known to branch on `getMessage()` and to match it with anchored regexes, so +/// it is part of the API. `getErrors()` does gain one entry per parse error instead of a +/// single entry for the whole document, and those entries carry the bare Cedar message, +/// since the prefix describes the binding rather than any one error. fn throw_parse_errors(env: &mut JNIEnv<'_>, errs: &ParseErrors) { if env.exception_check().unwrap_or_default() { return; // An exception is already in flight; let it propagate. @@ -556,10 +557,11 @@ fn throw_parse_errors(env: &mut JNIEnv<'_>, errs: &ParseErrors) { let details: Vec = errs.iter().map(DetailedError::from).collect(); let messages: Vec = details.iter().map(|d| d.message.clone()).collect(); let details_json = serde_json::to_string(&details).unwrap_or_default(); + let message = internal_error_message(errs); // Fall back to the generic path if any part of building the richer exception fails, so // a parse error is never silently turned into a different kind of failure. - match build_parse_exception(env, &messages, &details_json) { + match build_parse_exception(env, &message, &messages, &details_json) { Ok(exception) => { if env.throw(exception).is_err() { throw_internal(env, errs); @@ -569,9 +571,10 @@ fn throw_parse_errors(env: &mut JNIEnv<'_>, errs: &ParseErrors) { } } -/// Build `PolicyParseException(String[] messages, String detailedErrorsJson)`. +/// Build `PolicyParseException(String message, String[] messages, String detailedErrorsJson)`. fn build_parse_exception<'a>( env: &mut JNIEnv<'a>, + message: &str, messages: &[String], details_json: &str, ) -> Result> { @@ -582,11 +585,13 @@ fn build_parse_exception<'a>( let jmessage = env.new_string(message)?; env.set_object_array_element(&messages_array, i as i32, jmessage)?; } + let jmessage = env.new_string(message)?; let jdetails = env.new_string(details_json)?; let exception = env.new_object( "com/cedarpolicy/model/exception/PolicyParseException", - "([Ljava/lang/String;Ljava/lang/String;)V", + "(Ljava/lang/String;[Ljava/lang/String;Ljava/lang/String;)V", &[ + JValueGen::Object(&jmessage), JValueGen::Object(&messages_array), JValueGen::Object(&jdetails), ], @@ -594,13 +599,19 @@ fn build_parse_exception<'a>( Ok(JThrowable::from(exception)) } +/// The message `jni_failed` writes for `errs`. The sole definition of that string, so the +/// richer exception and the fallback below can never drift apart. +fn internal_error_message(errs: &ParseErrors) -> String { + format!("Internal JNI Error: {errs}") +} + /// Throw a plain `InternalException`, exactly as `jni_failed` would have. fn throw_internal(env: &mut JNIEnv<'_>, errs: &ParseErrors) { // We have to unwrap here as we're doing exception handling // If we don't have the heap space to create an exception, the only valid move is ending the process env.throw_new( "com/cedarpolicy/model/exception/InternalException", - format!("Internal JNI Error: {errs}"), + internal_error_message(errs), ) .unwrap(); } From c1eb82d744852ac238d28d77d84d46ecbae3e9d0 Mon Sep 17 00:00:00 2001 From: James Mulcahy Date: Wed, 16 Sep 2026 18:37:04 +0000 Subject: [PATCH 3/5] Document how to index a DetailedError source span The spans Cedar reports are UTF-8 byte offsets, but nothing said so beyond "in bytes" on the two fields, and the obvious way to use them -- source.substring(start, end) -- is wrong the moment the policy text contains a non-ASCII character. It does not fail quietly: on a document with an accented identifier or an emoji in a comment it throws StringIndexOutOfBoundsException, because the byte offset runs past the end of the shorter UTF-16 string. SourceLabel now carries a class-level explanation with the byte-slicing snippet callers should use instead, and the field comments name the offsets as UTF-8 and give their inclusivity. It also records three other properties that are not apparent from the types, all confirmed against the parser: - offsets are absolute within the whole parsed text rather than relative to the enclosing policy, so they stay usable when several policies are parsed together, and they carry no policy identity of their own; - a span may be empty, which is what an unterminated string literal produces, so a renderer must not assume a character to underline; - a span covers the unexpected token, which for a missing operand is the token that followed it, possibly on a later line. PolicyParseException's own Javadoc described how the class differed from the generic error path that preceded it, and said getErrors() "does change" -- relative to a revision no reader of the released class will have seen. Rewritten to say what the type is: a list contrasting the three accessors by fidelity, which is what a caller needs in order to choose between them. The rationale for freezing getMessage() stays in the private Rust that builds it, where it warns whoever might reasonably re-break it, rather than in public API documentation. Also adds the whitespace checkstyleMain wants inside the empty TypeReference body in PolicyParseException, which was failing the build. Signed-off-by: James Mulcahy Co-Authored-By: Claude Opus 5 (1M context) --- .../com/cedarpolicy/model/DetailedError.java | 36 +++++++++++++++++-- .../model/exception/InternalException.java | 6 ++-- .../model/exception/PolicyParseException.java | 35 +++++++++--------- 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java b/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java index 19a0d5fb..0fd032df 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java @@ -38,7 +38,7 @@ public class DetailedError { /** Severity */ @JsonProperty("severity") public final Optional severity; - /** Source labels (ranges) */ + /** Source labels (ranges); see {@link SourceLabel} for how to index them */ @JsonProperty("sourceLocations") public final ImmutableList sourceLocations; /** Related errors */ @@ -84,14 +84,44 @@ public enum Severity { Error, } + /** + * A region of the source text an error refers to, so callers can underline it. + * + *

The offsets are UTF-8 byte offsets, not {@code String} indices. Cedar produces + * them by counting bytes, while {@link String#substring(int, int)} counts UTF-16 chars. The + * two coincide only while the source is pure ASCII; a single non-ASCII character anywhere + * earlier in the document - an accented identifier, a non-Latin string literal, an emoji in + * a comment - shifts them apart, and slicing the {@code String} directly then either + * extracts the wrong region or throws {@link StringIndexOutOfBoundsException}. Slice the + * source's UTF-8 bytes instead: + * + * 

{@code
+     * byte[] bytes = source.getBytes(StandardCharsets.UTF_8);
+     * String offending = new String(bytes, label.start, label.end - label.start, StandardCharsets.UTF_8);
+     * }
+ * + *

Offsets are absolute within the whole text that was parsed, not relative to the policy + * containing the error, so they remain directly usable when several policies are parsed + * together. They carry no policy identity of their own: mapping an offset back to a + * particular policy statement is left to the caller. + * + *

A region may be empty ({@code start == end}), which happens when there is no extent to + * highlight - an unterminated string literal, for instance, reports the position the lexer + * stopped at. Renderers should treat zero width as a single caret rather than assuming at + * least one character to underline. + * + *

For a parse error the region covers the unexpected token, which is not always where a + * reader would place the mistake: a missing operand is reported at the token that followed + * it, possibly on a later line. + */ public static final class SourceLabel { /** Text of the label (if any) */ @JsonProperty("label") public final Optional label; - /** Start of the source location (in bytes) */ + /** Start of the source location, as a UTF-8 byte offset, inclusive. See {@link SourceLabel}. */ @JsonProperty("start") public final int start; - /** End of the source location (in bytes) */ + /** End of the source location, as a UTF-8 byte offset, exclusive. See {@link SourceLabel}. */ @JsonProperty("end") public final int end; diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java index ba955ef4..bcee3aff 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java @@ -45,9 +45,9 @@ public InternalException(String[] errors) { * Internal exception whose message is not derived from its error list. * *

The other constructors build the message by joining {@code errors}, which ties the - * two together: a more finely split list necessarily changes the message. Subclasses that - * report each underlying error separately while keeping the message they have always - * produced use this constructor to set the two independently. + * two together: splitting the list more finely necessarily changes the message. Subclasses + * that report each underlying error separately, but summarise them differently in the + * message, use this constructor to set the two independently. * * @param error the message, prefixed as in {@link #InternalException(String)} * @param errors the individual error messages, for {@link #getErrors()} diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java index 415cc574..2f3f2a8d 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java @@ -27,35 +27,36 @@ * Thrown when Cedar policy text fails to parse, carrying the structured diagnostics Cedar * produced for each error. * - *

Cedar reports parse failures as {@code miette} diagnostics: a message, the source span - * of the offending token, the tokens the parser expected there, and often help text. Prior - * to this type those were flattened to a single {@code Display} string, so callers saw - * "unexpected token `::`" with no indication of where in the policy it occurred, and every - * error after the first was discarded. {@link #getDetailedErrors()} returns the full set, - * one {@link DetailedError} per parse error, in the order Cedar reported them. + *

Cedar reports a parse failure as one or more {@code miette} diagnostics: a message, the + * source span of the offending token, the tokens the parser expected there, and often help + * text. A single document may fail in several places, and every failure is reported. * - *

Extends {@link InternalException} so existing {@code catch} blocks are unaffected, and - * {@link #getMessage()} is byte-for-byte what the generic error path produced: callers are - * known to branch on that string and to match it with anchored regexes, so it is treated as - * part of the API and left alone. The new detail is reached through the accessors instead. + *

Three accessors describe the same failure at increasing fidelity: * - *

{@link #getErrors()} does change: it now carries one entry per parse error rather than - * a single entry for the whole document, which is what its plural contract always implied, - * and each entry is the bare Cedar message without the {@code "Internal JNI Error: "} - * prefix, which described the binding rather than any one error. + *

    + *
  • {@link #getMessage()} - one human-readable line, describing the first error only. Its + * wording is treated as part of this class's compatibility surface, so it is the least + * informative of the three and the safest to match on. + *
  • {@link #getErrors()} - one message per parse error, in the order Cedar reported them. + * These are Cedar's messages alone, without the prefix {@link #getMessage()} carries. + *
  • {@link #getDetailedErrors()} - the full diagnostic for each error, and the only + * accessor that reports where in the source the error occurred. See {@link DetailedError}. + *
+ * + *

Extends {@link InternalException}, so callers that catch the general parse-or-evaluate + * failure are unaffected and need not know this type exists. */ public class PolicyParseException extends InternalException { private static final TypeReference> ERROR_LIST = - new TypeReference>() {}; + new TypeReference>() { }; private final transient List detailedErrors; /** * Construct from the JSON array of {@code DetailedError} the native layer serialises. * - * @param message the message, which the native layer builds exactly as the generic - * error path does so that {@link #getMessage()} is unchanged + * @param message the value {@link #getMessage()} reports * @param messages one message per parse error, for {@link #getErrors()} * @param detailedErrorsJson JSON array of {@code DetailedError}; if it cannot be read, * the exception still carries {@code message} and {@code messages}, and From 8632241296637241837beee5049ef1bc6b9f370d Mon Sep 17 00:00:00 2001 From: James Mulcahy Date: Thu, 17 Sep 2026 01:23:47 +0000 Subject: [PATCH 4/5] Satisfy SpotBugs in PolicyParseException SpotBugs flagged the blanket `catch (Exception)` in readDetailedErrors (REC_CATCH_EXCEPTION). Catch JsonProcessingException and RuntimeException instead: same defensive behaviour, no catch of exceptions that cannot arise. Narrowing the catch exposed CT_CONSTRUCTOR_THROW, since the constructor can now throw and leave a partially initialised object. Make the class final, SpotBugs' remedy for that rule; it was never meant to be subclassed. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: James Mulcahy --- .../cedarpolicy/model/exception/PolicyParseException.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java index 2f3f2a8d..1e1d85f6 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java @@ -18,6 +18,7 @@ import com.cedarpolicy.CedarJson; import com.cedarpolicy.model.DetailedError; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import java.util.Arrays; import java.util.Collections; @@ -46,7 +47,7 @@ *

Extends {@link InternalException}, so callers that catch the general parse-or-evaluate * failure are unaffected and need not know this type exists. */ -public class PolicyParseException extends InternalException { +public final class PolicyParseException extends InternalException { private static final TypeReference> ERROR_LIST = new TypeReference>() { }; @@ -75,7 +76,7 @@ private static List readDetailedErrors(String json) { try { List parsed = CedarJson.objectReader().forType(ERROR_LIST).readValue(json); return parsed == null ? List.of() : List.copyOf(parsed); - } catch (Exception e) { + } catch (JsonProcessingException | RuntimeException e) { return List.of(); } } From 7bdffd7d1859ce44f3697c7a0459dfddbe266129 Mon Sep 17 00:00:00 2001 From: James Mulcahy Date: Fri, 18 Sep 2026 17:48:22 +0000 Subject: [PATCH 5/5] Guard throw_internal against a pending Java exception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `?` in `build_parse_exception` is a jni-rs call that can fail because the JVM threw — `NoClassDefFoundError` if the loaded `.jar` predates the `.so`, `OutOfMemoryError` from `new_string` or `new_object_array`. jni-rs detects the exception but leaves it pending, so the `Err(_) => throw_internal(...)` fallback ran with an exception in flight. There `throw_new` resolves its class through `find_class`, which refuses to call JNI in that state and returns `Error::JavaException`; unwrapping that panicked, and unwinding out of the `extern "C"` boundary `jni_fn` generates would abort the JVM. Guard on `exception_check` as `jni_failed` does. Returning is also the better behaviour: the pending error says more about the failure than an `InternalException` naming a parse error would, and it reaches the caller when the native method returns. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: James Mulcahy --- CedarJavaFFI/src/interface.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CedarJavaFFI/src/interface.rs b/CedarJavaFFI/src/interface.rs index 80273e12..b5c27932 100644 --- a/CedarJavaFFI/src/interface.rs +++ b/CedarJavaFFI/src/interface.rs @@ -607,6 +607,19 @@ fn internal_error_message(errs: &ParseErrors) -> String { /// Throw a plain `InternalException`, exactly as `jni_failed` would have. fn throw_internal(env: &mut JNIEnv<'_>, errs: &ParseErrors) { + // Guard on a pending exception exactly as `jni_failed` does. This is not only for safety + // but is also the more useful behaviour: whatever the JVM already threw (an + // `OutOfMemoryError`, or a `NoClassDefFoundError` from a `.jar` that predates the `.so`) + // says far more about the failure than an `InternalException` naming a parse error would, + // and it propagates to the caller when this native method returns. + // + // The guard is also what makes the `unwrap` below sound. jni-rs refuses to make JNI calls + // while an exception is pending, so `throw_new` would fail its internal `find_class` and + // return `Error::JavaException`; unwrapping that panics, and unwinding out of the + // `extern "C"` boundary that `jni_fn` generates would abort the JVM. + if env.exception_check().unwrap_or_default() { + return; + } // We have to unwrap here as we're doing exception handling // If we don't have the heap space to create an exception, the only valid move is ending the process env.throw_new(