From 527605965fbea65a19bd0ed42baa04c333e6267b Mon Sep 17 00:00:00 2001 From: James Mulcahy Date: Sat, 22 Aug 2026 00:51:26 +0000 Subject: [PATCH] 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(), } }