Surface structured parse diagnostics from PolicySet.parsePolicies

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.
+ *
+ * <p>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.
+ *
+ * <p>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<List<DetailedError>> ERROR_LIST =
+            new TypeReference<List<DetailedError>>() {};
+
+    private final transient List<DetailedError> 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<DetailedError> readDetailedErrors(String json) {
+        if (json == null || json.isEmpty()) {
+            return List.of();
+        }
+        try {
+            List<DetailedError> 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<DetailedError> getDetailedErrors() {
+        return Collections.unmodifiableList(detailedErrors);
+    }
+}

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<DetailedError> 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);"));
+    }
+}

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<DetailedError> = errs.iter().map(DetailedError::from).collect();
+    let messages: Vec<String> = 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<JThrowable<'a>> {
+    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::<ParseErrors>() {
+            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(),
     }
 }