From a97b4cd37256f41515a763e486c04733f558c670 Mon Sep 17 00:00:00 2001 From: Chris Simmons Date: Thu, 16 Apr 2026 11:32:59 -0700 Subject: [PATCH 1/6] Optimize JNI performance and fix Decimal validation Improve JNI call performance ~3x with reduced overhead. Add JMH benchmark suite for tracking JNI performance. Fix Decimal validator to accept leading zeros in corpus integration tests. Signed-off-by: Chris Simmons --- CedarJava/build.gradle | 33 +++ .../cedarpolicy/AuthorizationBenchmark.java | 189 ++++++++++++++++++ .../cedarpolicy/BasicAuthorizationEngine.java | 20 +- .../serializer/ValueDeserializer.java | 87 +++----- .../java/com/cedarpolicy/value/Decimal.java | 3 - CedarJavaFFI/src/interface.rs | 15 +- 6 files changed, 267 insertions(+), 80 deletions(-) create mode 100644 CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java diff --git a/CedarJava/build.gradle b/CedarJava/build.gradle index f0b81b4b..2e3cc02d 100644 --- a/CedarJava/build.gradle +++ b/CedarJava/build.gradle @@ -75,6 +75,15 @@ configurations { testCompileOnly.extendsFrom compileOnly } +sourceSets { + jmh { + java.srcDirs = ['src/jmh/java'] + resources.srcDirs = ['src/test/resources'] + compileClasspath += sourceSets.main.output + sourceSets.main.compileClasspath + runtimeClasspath += sourceSets.main.output + sourceSets.main.runtimeClasspath + } +} + dependencies { def junitVersion = '6.0.0' @@ -93,6 +102,9 @@ dependencies { testImplementation "org.junit.jupiter:junit-jupiter-params:${junitVersion}" testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine:${junitVersion}" testRuntimeOnly "org.junit.platform:junit-platform-launcher:${junitVersion}" + + jmhImplementation 'org.openjdk.jmh:jmh-core:1.37' + jmhAnnotationProcessor 'org.openjdk.jmh:jmh-generator-annprocess:1.37' } def ffiDir = '../CedarJavaFFI' @@ -261,6 +273,27 @@ compileTestJava { targetCompatibility = "17" } +compileJmhJava { + sourceCompatibility = "17" + targetCompatibility = "17" +} + +tasks.register('jmh', JavaExec) { + group 'Benchmark' + description 'Runs JMH benchmarks for JNI performance.' + + dependsOn('compileFFI') + dependsOn('jmhClasses') + + mainClass = 'org.openjdk.jmh.Main' + classpath = sourceSets.jmh.runtimeClasspath + files(layout.buildDirectory.dir(compiledLibDir)) + + // Pass through any -Pjmh.args="..." to JMH (e.g., -Pjmh.args="-f 0 -i 1 -wi 1") + if (project.hasProperty('jmh.args')) { + args((project.property('jmh.args') as String).split('\\s+')) + } +} + compileJava { sourceCompatibility = "1.8" targetCompatibility = "1.8" diff --git a/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java b/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java new file mode 100644 index 00000000..f0c1576a --- /dev/null +++ b/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java @@ -0,0 +1,189 @@ +/* + * 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.AuthorizationRequest; +import com.cedarpolicy.model.AuthorizationResponse; +import com.cedarpolicy.model.ValidationRequest; +import com.cedarpolicy.model.ValidationResponse; +import com.cedarpolicy.model.entity.Entity; +import com.cedarpolicy.model.exception.AuthException; +import com.cedarpolicy.model.policy.Policy; +import com.cedarpolicy.model.policy.PolicySet; +import com.cedarpolicy.model.schema.Schema; +import com.cedarpolicy.model.schema.Schema.JsonOrCedar; +import com.cedarpolicy.value.EntityTypeName; +import com.cedarpolicy.value.EntityUID; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.TimeUnit; + +/** + * JMH benchmarks for Cedar authorization engine JNI performance. + * + *

Run via: ./gradlew jmh + */ +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@Fork(1) +@State(Scope.Benchmark) +public class AuthorizationBenchmark { + + private BasicAuthorizationEngine engine; + + // Small scenario: minimal request + private AuthorizationRequest smallRequest; + private PolicySet smallPolicySet; + private Set smallEntities; + + // Medium scenario: photoflash schema with entities and multiple policies + private AuthorizationRequest mediumRequest; + private PolicySet mediumPolicySet; + private Set mediumEntities; + + // Validation scenario + private ValidationRequest validationRequest; + + @Setup(Level.Trial) + public void setUp() throws Exception { + engine = new BasicAuthorizationEngine(); + + setUpSmallScenario(); + setUpMediumScenario(); + setUpValidationScenario(); + } + + private void setUpSmallScenario() { + EntityTypeName userType = EntityTypeName.parse("User").get(); + EntityTypeName actionType = EntityTypeName.parse("Action").get(); + EntityTypeName resourceType = EntityTypeName.parse("Resource").get(); + + EntityUID principal = userType.of("alice"); + EntityUID action = actionType.of("view"); + EntityUID resource = resourceType.of("doc1"); + + smallRequest = new AuthorizationRequest(principal, action, resource, new HashMap<>()); + + Set policies = new HashSet<>(); + policies.add(new Policy("permit(principal, action, resource);", "policy0")); + smallPolicySet = new PolicySet(policies); + + smallEntities = new HashSet<>(); + smallEntities.add(new Entity(principal, new HashMap<>(), new HashSet<>())); + smallEntities.add(new Entity(action, new HashMap<>(), new HashSet<>())); + smallEntities.add(new Entity(resource, new HashMap<>(), new HashSet<>())); + } + + private void setUpMediumScenario() throws Exception { + EntityTypeName userType = EntityTypeName.parse("User").get(); + EntityTypeName actionType = EntityTypeName.parse("Action").get(); + EntityTypeName photoType = EntityTypeName.parse("Photo").get(); + EntityTypeName albumType = EntityTypeName.parse("Album").get(); + EntityTypeName accountType = EntityTypeName.parse("Account").get(); + + EntityUID principal = userType.of("alice"); + EntityUID action = actionType.of("View_Photo"); + EntityUID resource = photoType.of("pic01"); + + mediumRequest = new AuthorizationRequest(principal, action, resource, new HashMap<>()); + + Set policies = new HashSet<>(); + policies.add(new Policy( + "permit(principal == User::\"alice\", action == Action::\"View_Photo\", resource);", "p0")); + policies.add(new Policy( + "permit(principal, action == Action::\"View_Photo\", resource in Album::\"vacation\");", "p1")); + policies.add(new Policy( + "forbid(principal, action, resource) when { resource.private };", "p2")); + policies.add(new Policy( + "permit(principal, action == Action::\"Edit_Photo\", resource) " + + "when { principal == resource.owner };", + "p3")); + policies.add(new Policy( + "permit(principal, action == Action::\"Delete_Photo\", resource) " + + "when { principal == resource.owner };", + "p4")); + mediumPolicySet = new PolicySet(policies); + + // Build entity graph + mediumEntities = new HashSet<>(); + + EntityUID albumId = albumType.of("vacation"); + EntityUID accountId = accountType.of("account1"); + + Set photoParents = new HashSet<>(); + photoParents.add(albumId); + + Set albumParents = new HashSet<>(); + albumParents.add(accountId); + + mediumEntities.add(new Entity(principal, new HashMap<>(), new HashSet<>())); + mediumEntities.add(new Entity(action, new HashMap<>(), new HashSet<>())); + mediumEntities.add(new Entity(resource, new HashMap<>(), photoParents)); + mediumEntities.add(new Entity(albumId, new HashMap<>(), albumParents)); + mediumEntities.add(new Entity(accountId, new HashMap<>(), new HashSet<>())); + } + + private void setUpValidationScenario() throws Exception { + String schemaText = new String( + Files.readAllBytes(Paths.get( + AuthorizationBenchmark.class.getResource("/photoflash_schema.json").toURI())), + StandardCharsets.UTF_8); + Schema schema = new Schema(JsonOrCedar.Json, Optional.of(schemaText), Optional.empty()); + + Set policies = new HashSet<>(); + policies.add(new Policy( + "permit(principal == User::\"alice\", action == Action::\"View_Photo\", resource);", "p0")); + PolicySet policySet = new PolicySet(policies); + + validationRequest = new ValidationRequest(schema, policySet); + } + + @Benchmark + public AuthorizationResponse isAuthorized_small() throws AuthException { + return engine.isAuthorized(smallRequest, smallPolicySet, smallEntities); + } + + @Benchmark + public AuthorizationResponse isAuthorized_medium() throws AuthException { + return engine.isAuthorized(mediumRequest, mediumPolicySet, mediumEntities); + } + + @Benchmark + public ValidationResponse validate_small() throws AuthException { + return engine.validate(validationRequest); + } +} diff --git a/CedarJava/src/main/java/com/cedarpolicy/BasicAuthorizationEngine.java b/CedarJava/src/main/java/com/cedarpolicy/BasicAuthorizationEngine.java index e794cdbe..26f65842 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/BasicAuthorizationEngine.java +++ b/CedarJava/src/main/java/com/cedarpolicy/BasicAuthorizationEngine.java @@ -42,7 +42,6 @@ import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.JsonNode; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -50,6 +49,13 @@ public final class BasicAuthorizationEngine implements AuthorizationEngine { static { LibraryLoader.loadLibrary(); + String jniVersion = getCedarJNIVersion(); + String langVersion = AuthorizationEngine.getCedarLangVersion(); + if (!jniVersion.equals(langVersion)) { + throw new ExceptionInInitializerError( + "Error, Java Cedar Language version is " + langVersion + + " but JNI Cedar Language version is " + jniVersion); + } } /** Construct a basic authorization engine. */ @@ -123,21 +129,11 @@ public void validateEntities(EntityValidationRequest q) throws AuthException { private static RESP call(String operation, Class responseClass, REQ request) throws AuthException { try { - final String cedarJNIVersion = getCedarJNIVersion(); - if (!cedarJNIVersion.equals(AuthorizationEngine.getCedarLangVersion())) { - throw new AuthException( - "Error, Java Cedar Language version is " - + AuthorizationEngine.getCedarLangVersion() - + " but JNI Cedar Language version is " - + cedarJNIVersion); - } - // Convert the request POJO to a JSON string final String fullRequest = objectWriter().writeValueAsString(request); final String response = callCedarJNI(operation, fullRequest); - final JsonNode responseNode = objectReader().readTree(response); - return objectReader().readValue(responseNode, responseClass); + return objectReader().readValue(response, responseClass); } catch (JsonProcessingException e) { throw new AuthException("JSON Serialization Error", e); } catch (IllegalArgumentException e) { diff --git a/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java b/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java index d4df41d6..0dfa2c11 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java +++ b/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java @@ -85,68 +85,47 @@ public Value deserialize(JsonParser parser, DeserializationContext context) thro } return myNode; } else if (node.isObject()) { - Iterator> iter = node.fields(); - // Do two passes, one to check if it is an escaped entity or extension and a second to - // write into a map - EscapeType escapeType = EscapeType.UNRECOGNIZED; - int count = 0; - while (iter.hasNext()) { - count++; - Map.Entry entry = iter.next(); - if (entry.getKey().equals(ENTITY_ESCAPE_SEQ)) { - escapeType = EscapeType.ENTITY; - } else if (entry.getKey().equals(EXTENSION_ESCAPE_SEQ)) { - escapeType = EscapeType.EXTENSION; - } - } - if (escapeType != EscapeType.UNRECOGNIZED) { - if (count == 1) { - if (escapeType == EscapeType.ENTITY) { - JsonNode val = node.get(ENTITY_ESCAPE_SEQ); - if (val.isObject() && val.has("id") && val.has("type")) { - int numFields = 0; - for (Iterator it = val.fieldNames(); it.hasNext(); it.next()) { - numFields++; - } - if (numFields == 2) { - EntityIdentifier id = new EntityIdentifier(val.get("id").textValue()); - Optional type = EntityTypeName.parse(val.get("type").textValue()); - if (type.isPresent()) { - return new EntityUID(type.get(), id); - } else { - String msg = "Invalid Entity Type" + val.get("type").textValue(); - throw new InvalidValueDeserializationException(parser, msg, node.asToken(), Map.class); - } - } + // Single-pass: check for escape sequences via direct lookup, avoiding full iteration + if (node.size() == 1) { + if (node.has(ENTITY_ESCAPE_SEQ)) { + JsonNode val = node.get(ENTITY_ESCAPE_SEQ); + if (val.isObject() && val.has("id") && val.has("type") && val.size() == 2) { + EntityIdentifier id = new EntityIdentifier(val.get("id").textValue()); + Optional type = EntityTypeName.parse(val.get("type").textValue()); + if (type.isPresent()) { + return new EntityUID(type.get(), id); + } else { + String msg = "Invalid Entity Type" + val.get("type").textValue(); + throw new InvalidValueDeserializationException(parser, msg, node.asToken(), Map.class); } + } + throw new InvalidValueDeserializationException(parser, + "Not textual node: " + node.toString(), node.asToken(), Map.class); + } else if (node.has(EXTENSION_ESCAPE_SEQ)) { + JsonNode val = node.get(EXTENSION_ESCAPE_SEQ); + JsonNode fn = val.get("fn"); + if (!fn.isTextual()) { throw new InvalidValueDeserializationException(parser, - "Not textual node: " + node.toString(), node.asToken(), Map.class); - } else { - JsonNode val = node.get(EXTENSION_ESCAPE_SEQ); - JsonNode fn = val.get("fn"); - if (!fn.isTextual()) { - throw new InvalidValueDeserializationException(parser, - "Not textual node: " + fn.toString(), node.asToken(), Map.class); - } + "Not textual node: " + fn.toString(), node.asToken(), Map.class); + } - String fnName = fn.textValue(); + String fnName = fn.textValue(); - if (MULTI_ARG_FN.contains(fnName)) { - return deserializeMultiArgFunction(fnName, val, mapper, parser, node); - } else if (SINGLE_ARG_FN.contains(fnName)) { - return deserializeSingleArgFunction(fnName, val, parser, node); - } else { - throw new InvalidValueDeserializationException(parser, - "Invalid function type: " + fn.toString(), node.asToken(), Map.class); - } + if (MULTI_ARG_FN.contains(fnName)) { + return deserializeMultiArgFunction(fnName, val, mapper, parser, node); + } else if (SINGLE_ARG_FN.contains(fnName)) { + return deserializeSingleArgFunction(fnName, val, parser, node); + } else { + throw new InvalidValueDeserializationException(parser, + "Invalid function type: " + fn.toString(), node.asToken(), Map.class); } - } else { - throw new InvalidValueDeserializationException(parser, - "More than one K,V pair with {__entity, __extn}: " + node.toString(), node.asToken(), Map.class); } + } else if (node.size() > 1 && (node.has(ENTITY_ESCAPE_SEQ) || node.has(EXTENSION_ESCAPE_SEQ))) { + throw new InvalidValueDeserializationException(parser, + "More than one K,V pair with {__entity, __extn}: " + node.toString(), node.asToken(), Map.class); } CedarMap myMap = new CedarMap(); - iter = node.fields(); + Iterator> iter = node.fields(); while (iter.hasNext()) { Map.Entry entry = iter.next(); myMap.put(entry.getKey(), mapper.treeToValue(entry.getValue(), Value.class)); diff --git a/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java b/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java index 320027f7..cf9c73f2 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java +++ b/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java @@ -36,9 +36,6 @@ public static boolean validDecimal(String d) { return false; } d = d.trim(); - if (d.length() > 21) { - return false; // 19digits, decimal point and - sign - } try { Matcher matcher = DECIMAL_PATTERN.matcher(d); return matcher.matches(); diff --git a/CedarJavaFFI/src/interface.rs b/CedarJavaFFI/src/interface.rs index df70898c..e25676fc 100644 --- a/CedarJavaFFI/src/interface.rs +++ b/CedarJavaFFI/src/interface.rs @@ -33,7 +33,7 @@ use jni::{ use jni_fn::jni_fn; use serde::{Deserialize, Serialize}; use serde_json::{from_str, Value}; -use std::{error::Error, str::FromStr, thread}; +use std::{error::Error, panic, str::FromStr}; use crate::{ answer::Answer, @@ -65,10 +65,6 @@ fn build_err_obj(env: &JNIEnv<'_>, err: &str) -> jstring { .into_raw() } -fn call_cedar_in_thread(call_str: String, input_str: String) -> String { - call_cedar(&call_str, &input_str) -} - /// JNI entry point for authorization and validation requests #[jni_fn("com.cedarpolicy.BasicAuthorizationEngine")] pub fn callCedarJNI( @@ -82,17 +78,14 @@ pub fn callCedarJNI( _ => return build_err_obj(&env, "getting"), }; - let mut j_input_str: String = match env.get_string(&j_input) { + let j_input_str: String = match env.get_string(&j_input) { Ok(s) => s.into(), Err(_) => return build_err_obj(&env, "parsing"), }; - j_input_str.push(' '); - - let handle = thread::spawn(move || call_cedar_in_thread(j_call_str, j_input_str)); - let result = match handle.join() { + let result = match panic::catch_unwind(|| call_cedar(&j_call_str, &j_input_str)) { Ok(s) => s, - Err(e) => format!("Authorization thread failed {e:?}"), + Err(e) => format!("Authorization call panicked: {e:?}"), }; let res = env.new_string(result); From c26754cbd04710fffdfedcae672817dc6fdf684a Mon Sep 17 00:00:00 2001 From: Chris Simmons Date: Thu, 16 Apr 2026 19:27:02 -0700 Subject: [PATCH 2/6] Fix checkstyle violations in JMH benchmark method names Rename snake_case methods to camelCase to comply with the MethodName checkstyle rule ('^[a-z][a-zA-Z0-9]*$'). Signed-off-by: Chris Simmons --- .../jmh/java/com/cedarpolicy/AuthorizationBenchmark.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java b/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java index f0c1576a..8781c4b0 100644 --- a/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java +++ b/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java @@ -173,17 +173,17 @@ private void setUpValidationScenario() throws Exception { } @Benchmark - public AuthorizationResponse isAuthorized_small() throws AuthException { + public AuthorizationResponse isAuthorizedSmall() throws AuthException { return engine.isAuthorized(smallRequest, smallPolicySet, smallEntities); } @Benchmark - public AuthorizationResponse isAuthorized_medium() throws AuthException { + public AuthorizationResponse isAuthorizedMedium() throws AuthException { return engine.isAuthorized(mediumRequest, mediumPolicySet, mediumEntities); } @Benchmark - public ValidationResponse validate_small() throws AuthException { + public ValidationResponse validateSmall() throws AuthException { return engine.validate(validationRequest); } } From 67c69da21a39eb1617a43124616245a68baab998 Mon Sep 17 00:00:00 2001 From: Chris Simmons Date: Thu, 16 Apr 2026 20:18:35 -0700 Subject: [PATCH 3/6] Exclude JMH-generated code from SpotBugs analysis JMH generates padding fields and dead stores intentionally for cache-line isolation, which triggers false positives in SpotBugs. Signed-off-by: Chris Simmons --- CedarJava/build.gradle | 6 ++++++ CedarJava/config/spotbugs/jmh-exclude.xml | 7 +++++++ 2 files changed, 13 insertions(+) create mode 100644 CedarJava/config/spotbugs/jmh-exclude.xml diff --git a/CedarJava/build.gradle b/CedarJava/build.gradle index 2e3cc02d..c336d32d 100644 --- a/CedarJava/build.gradle +++ b/CedarJava/build.gradle @@ -67,6 +67,12 @@ spotbugs { ignoreFailures.set(false) } +tasks.withType(com.github.spotbugs.snom.SpotBugsTask).configureEach { + if (name == 'spotbugsJmh') { + excludeFilter = file('config/spotbugs/jmh-exclude.xml') + } +} + repositories { mavenCentral() } diff --git a/CedarJava/config/spotbugs/jmh-exclude.xml b/CedarJava/config/spotbugs/jmh-exclude.xml new file mode 100644 index 00000000..1d435f26 --- /dev/null +++ b/CedarJava/config/spotbugs/jmh-exclude.xml @@ -0,0 +1,7 @@ + + + + + + + From d76674f8741801d5d6103d9da6231ca95cf52e13 Mon Sep 17 00:00:00 2001 From: Chris Simmons Date: Sat, 18 Apr 2026 15:52:39 -0700 Subject: [PATCH 4/6] Address PR #349 review feedback - Fix Decimal validation to correctly handle leading zeros and enforce range bounds [-922337203685477.5808, 922337203685477.5807] instead of the previous format-only regex check - Add DecimalTests covering boundary values, leading zeros, and out-of-range rejection - Remove unused EscapeType enum from ValueDeserializer - Remove redundant val.isObject() check in entity deserialization - Move benchmark policies and entities to resource files, use PolicySet.parsePolicies() and Entities.parse() instead of inline construction Signed-off-by: Chris Simmons --- .../cedarpolicy/AuthorizationBenchmark.java | 90 +++++------------- .../src/jmh/resources/medium_entities.json | 7 ++ .../src/jmh/resources/medium_policies.cedar | 9 ++ .../src/jmh/resources/small_entities.json | 5 + .../src/jmh/resources/small_policies.cedar | 1 + .../serializer/ValueDeserializer.java | 8 +- .../java/com/cedarpolicy/value/Decimal.java | 39 +++++++- .../java/com/cedarpolicy/DecimalTests.java | 93 +++++++++++++++++++ 8 files changed, 172 insertions(+), 80 deletions(-) create mode 100644 CedarJava/src/jmh/resources/medium_entities.json create mode 100644 CedarJava/src/jmh/resources/medium_policies.cedar create mode 100644 CedarJava/src/jmh/resources/small_entities.json create mode 100644 CedarJava/src/jmh/resources/small_policies.cedar create mode 100644 CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java diff --git a/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java b/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java index 8781c4b0..e344176a 100644 --- a/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java +++ b/CedarJava/src/jmh/java/com/cedarpolicy/AuthorizationBenchmark.java @@ -20,14 +20,13 @@ import com.cedarpolicy.model.AuthorizationResponse; import com.cedarpolicy.model.ValidationRequest; import com.cedarpolicy.model.ValidationResponse; +import com.cedarpolicy.model.entity.Entities; import com.cedarpolicy.model.entity.Entity; import com.cedarpolicy.model.exception.AuthException; -import com.cedarpolicy.model.policy.Policy; import com.cedarpolicy.model.policy.PolicySet; import com.cedarpolicy.model.schema.Schema; import com.cedarpolicy.model.schema.Schema.JsonOrCedar; import com.cedarpolicy.value.EntityTypeName; -import com.cedarpolicy.value.EntityUID; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; @@ -45,7 +44,6 @@ import java.nio.file.Files; import java.nio.file.Paths; import java.util.HashMap; -import java.util.HashSet; import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; @@ -87,87 +85,43 @@ public void setUp() throws Exception { setUpValidationScenario(); } - private void setUpSmallScenario() { + private static String readResource(String resourcePath) throws Exception { + return new String( + Files.readAllBytes(Paths.get( + AuthorizationBenchmark.class.getResource(resourcePath).toURI())), + StandardCharsets.UTF_8); + } + + private void setUpSmallScenario() throws Exception { EntityTypeName userType = EntityTypeName.parse("User").get(); EntityTypeName actionType = EntityTypeName.parse("Action").get(); EntityTypeName resourceType = EntityTypeName.parse("Resource").get(); - EntityUID principal = userType.of("alice"); - EntityUID action = actionType.of("view"); - EntityUID resource = resourceType.of("doc1"); - - smallRequest = new AuthorizationRequest(principal, action, resource, new HashMap<>()); + smallRequest = new AuthorizationRequest( + userType.of("alice"), actionType.of("view"), resourceType.of("doc1"), new HashMap<>()); - Set policies = new HashSet<>(); - policies.add(new Policy("permit(principal, action, resource);", "policy0")); - smallPolicySet = new PolicySet(policies); - - smallEntities = new HashSet<>(); - smallEntities.add(new Entity(principal, new HashMap<>(), new HashSet<>())); - smallEntities.add(new Entity(action, new HashMap<>(), new HashSet<>())); - smallEntities.add(new Entity(resource, new HashMap<>(), new HashSet<>())); + smallPolicySet = PolicySet.parsePolicies(readResource("/small_policies.cedar")); + smallEntities = Entities.parse(readResource("/small_entities.json")).getEntities(); } private void setUpMediumScenario() throws Exception { EntityTypeName userType = EntityTypeName.parse("User").get(); EntityTypeName actionType = EntityTypeName.parse("Action").get(); EntityTypeName photoType = EntityTypeName.parse("Photo").get(); - EntityTypeName albumType = EntityTypeName.parse("Album").get(); - EntityTypeName accountType = EntityTypeName.parse("Account").get(); - - EntityUID principal = userType.of("alice"); - EntityUID action = actionType.of("View_Photo"); - EntityUID resource = photoType.of("pic01"); - - mediumRequest = new AuthorizationRequest(principal, action, resource, new HashMap<>()); - - Set policies = new HashSet<>(); - policies.add(new Policy( - "permit(principal == User::\"alice\", action == Action::\"View_Photo\", resource);", "p0")); - policies.add(new Policy( - "permit(principal, action == Action::\"View_Photo\", resource in Album::\"vacation\");", "p1")); - policies.add(new Policy( - "forbid(principal, action, resource) when { resource.private };", "p2")); - policies.add(new Policy( - "permit(principal, action == Action::\"Edit_Photo\", resource) " - + "when { principal == resource.owner };", - "p3")); - policies.add(new Policy( - "permit(principal, action == Action::\"Delete_Photo\", resource) " - + "when { principal == resource.owner };", - "p4")); - mediumPolicySet = new PolicySet(policies); - - // Build entity graph - mediumEntities = new HashSet<>(); - - EntityUID albumId = albumType.of("vacation"); - EntityUID accountId = accountType.of("account1"); - - Set photoParents = new HashSet<>(); - photoParents.add(albumId); - - Set albumParents = new HashSet<>(); - albumParents.add(accountId); - - mediumEntities.add(new Entity(principal, new HashMap<>(), new HashSet<>())); - mediumEntities.add(new Entity(action, new HashMap<>(), new HashSet<>())); - mediumEntities.add(new Entity(resource, new HashMap<>(), photoParents)); - mediumEntities.add(new Entity(albumId, new HashMap<>(), albumParents)); - mediumEntities.add(new Entity(accountId, new HashMap<>(), new HashSet<>())); + + mediumRequest = new AuthorizationRequest( + userType.of("alice"), actionType.of("View_Photo"), photoType.of("pic01"), new HashMap<>()); + + mediumPolicySet = PolicySet.parsePolicies(readResource("/medium_policies.cedar")); + mediumEntities = Entities.parse(readResource("/medium_entities.json")).getEntities(); } private void setUpValidationScenario() throws Exception { - String schemaText = new String( - Files.readAllBytes(Paths.get( - AuthorizationBenchmark.class.getResource("/photoflash_schema.json").toURI())), - StandardCharsets.UTF_8); + String schemaText = readResource("/photoflash_schema.json"); Schema schema = new Schema(JsonOrCedar.Json, Optional.of(schemaText), Optional.empty()); - Set policies = new HashSet<>(); - policies.add(new Policy( - "permit(principal == User::\"alice\", action == Action::\"View_Photo\", resource);", "p0")); - PolicySet policySet = new PolicySet(policies); + PolicySet policySet = PolicySet.parsePolicies( + "permit(principal == User::\"alice\", action == Action::\"View_Photo\", resource);"); validationRequest = new ValidationRequest(schema, policySet); } diff --git a/CedarJava/src/jmh/resources/medium_entities.json b/CedarJava/src/jmh/resources/medium_entities.json new file mode 100644 index 00000000..99c3194b --- /dev/null +++ b/CedarJava/src/jmh/resources/medium_entities.json @@ -0,0 +1,7 @@ +[ + {"uid": {"type": "User", "id": "alice"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Action", "id": "View_Photo"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Photo", "id": "pic01"}, "attrs": {}, "parents": [{"type": "Album", "id": "vacation"}]}, + {"uid": {"type": "Album", "id": "vacation"}, "attrs": {}, "parents": [{"type": "Account", "id": "account1"}]}, + {"uid": {"type": "Account", "id": "account1"}, "attrs": {}, "parents": []} +] diff --git a/CedarJava/src/jmh/resources/medium_policies.cedar b/CedarJava/src/jmh/resources/medium_policies.cedar new file mode 100644 index 00000000..1b00713d --- /dev/null +++ b/CedarJava/src/jmh/resources/medium_policies.cedar @@ -0,0 +1,9 @@ +permit(principal == User::"alice", action == Action::"View_Photo", resource); + +permit(principal, action == Action::"View_Photo", resource in Album::"vacation"); + +forbid(principal, action, resource) when { resource.private }; + +permit(principal, action == Action::"Edit_Photo", resource) when { principal == resource.owner }; + +permit(principal, action == Action::"Delete_Photo", resource) when { principal == resource.owner }; diff --git a/CedarJava/src/jmh/resources/small_entities.json b/CedarJava/src/jmh/resources/small_entities.json new file mode 100644 index 00000000..49905922 --- /dev/null +++ b/CedarJava/src/jmh/resources/small_entities.json @@ -0,0 +1,5 @@ +[ + {"uid": {"type": "User", "id": "alice"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Action", "id": "view"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Resource", "id": "doc1"}, "attrs": {}, "parents": []} +] diff --git a/CedarJava/src/jmh/resources/small_policies.cedar b/CedarJava/src/jmh/resources/small_policies.cedar new file mode 100644 index 00000000..41c318cd --- /dev/null +++ b/CedarJava/src/jmh/resources/small_policies.cedar @@ -0,0 +1 @@ +permit(principal, action, resource); diff --git a/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java b/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java index 0dfa2c11..943ea951 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java +++ b/CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java @@ -59,12 +59,6 @@ public class ValueDeserializer extends JsonDeserializer { private static final Set MULTI_ARG_FN = Set.of(FN_OFFSET); private static final Set SINGLE_ARG_FN = Set.of(FN_IP, FN_DECIMAL, FN_UNKNOWN, FN_DATETIME, FN_DURATION); - private enum EscapeType { - ENTITY, - EXTENSION, - UNRECOGNIZED - } - /** Deserialize Json to Value. */ @Override public Value deserialize(JsonParser parser, DeserializationContext context) throws IOException { @@ -89,7 +83,7 @@ public Value deserialize(JsonParser parser, DeserializationContext context) thro if (node.size() == 1) { if (node.has(ENTITY_ESCAPE_SEQ)) { JsonNode val = node.get(ENTITY_ESCAPE_SEQ); - if (val.isObject() && val.has("id") && val.has("type") && val.size() == 2) { + if (val.has("id") && val.has("type") && val.size() == 2) { EntityIdentifier id = new EntityIdentifier(val.get("id").textValue()); Optional type = EntityTypeName.parse(val.get("type").textValue()); if (type.isPresent()) { diff --git a/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java b/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java index cf9c73f2..34320e85 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java +++ b/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java @@ -20,7 +20,6 @@ import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; -import java.util.regex.PatternSyntaxException; /** * Represents a Cedar fixed-point decimal extension value. Decimals are encoded as strings in @@ -30,18 +29,48 @@ public class Decimal extends Value { private static class DecimalValidator { private static final Pattern DECIMAL_PATTERN = Pattern.compile("^-?([0-9])*(\\.)([0-9]{0,4})$"); + private static final String MAX_POSITIVE = "9223372036854775807"; + private static final String MAX_NEGATIVE = "9223372036854775808"; public static boolean validDecimal(String d) { if (d == null || d.isEmpty()) { return false; } d = d.trim(); - try { - Matcher matcher = DECIMAL_PATTERN.matcher(d); - return matcher.matches(); - } catch (PatternSyntaxException ex) { + Matcher matcher = DECIMAL_PATTERN.matcher(d); + if (!matcher.matches()) { return false; } + + // Validate range: [-922337203685477.5808, 922337203685477.5807] + // This corresponds to the i64 range when the value is multiplied by 10000. + boolean negative = d.startsWith("-"); + String withoutSign = negative ? d.substring(1) : d; + + int dotIndex = withoutSign.indexOf('.'); + String intPart = withoutSign.substring(0, dotIndex); + String fracPart = withoutSign.substring(dotIndex + 1); + + // Strip leading zeros from integer part + intPart = intPart.replaceFirst("^0+", ""); + if (intPart.isEmpty()) { + intPart = "0"; + } + + // Pad fractional part to exactly 4 digits + fracPart = fracPart + "0000".substring(fracPart.length()); + + // Combine integer and fractional parts as the i64 representation (value * 10000) + String combined = intPart.equals("0") ? fracPart.replaceFirst("^0+", "") : intPart + fracPart; + if (combined.isEmpty()) { + combined = "0"; + } + + String limit = negative ? MAX_NEGATIVE : MAX_POSITIVE; + if (combined.length() != limit.length()) { + return combined.length() < limit.length(); + } + return combined.compareTo(limit) <= 0; } } diff --git a/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java b/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java new file mode 100644 index 00000000..9a7dd9ae --- /dev/null +++ b/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java @@ -0,0 +1,93 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.*; + +import org.junit.jupiter.api.Test; +import com.cedarpolicy.value.Decimal; + +public class DecimalTests { + + @Test + public void testValidDecimals() { + assertDoesNotThrow(() -> new Decimal("1.0")); + assertDoesNotThrow(() -> new Decimal("1.0000")); + assertDoesNotThrow(() -> new Decimal("-1.0")); + assertDoesNotThrow(() -> new Decimal("0.0")); + assertDoesNotThrow(() -> new Decimal("0.0000")); + assertDoesNotThrow(() -> new Decimal("123.456")); + assertDoesNotThrow(() -> new Decimal("-123.456")); + } + + @Test + public void testLeadingZeros() { + // Leading zeros should be accepted - Cedar trims them + assertDoesNotThrow(() -> new Decimal("001.0")); + assertDoesNotThrow(() -> new Decimal("0001.0000")); + assertDoesNotThrow(() -> new Decimal("-001.0")); + assertDoesNotThrow(() -> new Decimal("00000000000000000001.0")); + } + + @Test + public void testBoundaryValues() { + // Max positive: 922337203685477.5807 + assertDoesNotThrow(() -> new Decimal("922337203685477.5807")); + // Min negative: -922337203685477.5808 + assertDoesNotThrow(() -> new Decimal("-922337203685477.5808")); + // With leading zeros + assertDoesNotThrow(() -> new Decimal("-000000922337203685477.5808")); + assertDoesNotThrow(() -> new Decimal("000000922337203685477.5807")); + } + + @Test + public void testOutOfRange() { + // One above max positive + assertThrows(IllegalArgumentException.class, () -> new Decimal("922337203685477.5808")); + // One below min negative + assertThrows(IllegalArgumentException.class, () -> new Decimal("-922337203685477.5809")); + // Way out of range + assertThrows(IllegalArgumentException.class, () -> new Decimal("9999999999999999.0")); + assertThrows(IllegalArgumentException.class, () -> new Decimal("-9999999999999999.0")); + // Out of range even with leading zeros + assertThrows(IllegalArgumentException.class, () -> new Decimal("000922337203685477.5808")); + assertThrows(IllegalArgumentException.class, () -> new Decimal("-000922337203685477.5809")); + } + + @Test + public void testInvalidFormat() { + assertThrows(IllegalArgumentException.class, () -> new Decimal("")); + assertThrows(IllegalArgumentException.class, () -> new Decimal("abc")); + assertThrows(IllegalArgumentException.class, () -> new Decimal("1")); + assertThrows(IllegalArgumentException.class, () -> new Decimal("1.00000")); + assertThrows(IllegalArgumentException.class, () -> new Decimal(null)); + } + + @Test + public void testToCedarExpr() { + Decimal d = new Decimal("1.0000"); + assertEquals("decimal(\"1.0000\")", d.toCedarExpr()); + } + + @Test + public void testEquality() { + Decimal d1 = new Decimal("1.0000"); + Decimal d2 = new Decimal("1.0000"); + assertEquals(d1, d2); + assertEquals(d1.hashCode(), d2.hashCode()); + } +} From 3012f0d7715b5f4105e40d09e9962e65a520e2bd Mon Sep 17 00:00:00 2001 From: Chris Simmons Date: Sat, 18 Apr 2026 17:11:54 -0700 Subject: [PATCH 5/6] Fix cargo-zigbuild install failure and trailing whitespace Add --locked flag to cargo install for cargo-zigbuild to prevent dependency resolution from pulling cargo-platform 0.3.3 which requires rustc 1.91. Also fix trailing whitespace flagged by checkstyle. Signed-off-by: Chris Simmons --- CedarJava/build.gradle | 2 +- .../com/cedarpolicy/model/AuthorizationSuccessResponse.java | 2 +- .../java/com/cedarpolicy/model/LevelValidationRequest.java | 4 ++-- .../src/main/java/com/cedarpolicy/model/schema/Schema.java | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CedarJava/build.gradle b/CedarJava/build.gradle index c336d32d..d0fdd3c3 100644 --- a/CedarJava/build.gradle +++ b/CedarJava/build.gradle @@ -145,7 +145,7 @@ tasks.register('installCargoZigbuild', Exec) { group 'Build' description 'Installs Cargo Zigbuild for Rust compilation.' - commandLine 'cargo', '+' + RustVersion, 'install', 'cargo-zigbuild@0.22.1' + commandLine 'cargo', '+' + RustVersion, 'install', '--locked', 'cargo-zigbuild@0.22.1' } def ZigVersion = '0.11' diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/AuthorizationSuccessResponse.java b/CedarJava/src/main/java/com/cedarpolicy/model/AuthorizationSuccessResponse.java index ee1792a9..68423cc5 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/AuthorizationSuccessResponse.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/AuthorizationSuccessResponse.java @@ -127,7 +127,7 @@ public DetailedError getError() { @Override public String toString() { - return String.format("AuthorizationError{policyId=%s, error=%s}", policyId, error); + return String.format("AuthorizationError{policyId=%s, error=%s}", policyId, error); } } diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/LevelValidationRequest.java b/CedarJava/src/main/java/com/cedarpolicy/model/LevelValidationRequest.java index 166149b8..d24141cb 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/LevelValidationRequest.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/LevelValidationRequest.java @@ -28,7 +28,7 @@ public final class LevelValidationRequest { private final Schema schema; private final PolicySet policies; - private final long maxDerefLevel; // Must be non-negative (>=0) + private final long maxDerefLevel; // Must be non-negative (>=0) /** * Construct a validation request. @@ -77,7 +77,7 @@ public PolicySet getPolicySet() { /** * Get the maximum deref level. - * + * * @return The maximum deref level value for validation */ public long getMaxDerefLevel() { diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java b/CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java index 7a35e951..d23b6545 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java @@ -148,8 +148,8 @@ public String toCedarFormat() throws InternalException, IllegalStateException, N * @return JsonNode representing the schema in JSON format * @throws InternalException If conversion from Cedar to JSON format fails * @throws IllegalStateException If schema content is missing - * @throws JsonMappingException If invalid JSON - * @throws JsonProcessingException If invalid JSON + * @throws JsonMappingException If invalid JSON + * @throws JsonProcessingException If invalid JSON * @throws NullPointerException */ public JsonNode toJsonFormat() From 615a1e573f260b376973c5fe21f60045878efac5 Mon Sep 17 00:00:00 2001 From: Chris Simmons Date: Mon, 20 Apr 2026 14:30:05 -0700 Subject: [PATCH 6/6] Address PR #349 review feedback Tighten Decimal regex to require digits before and after the dot, rejecting invalid forms like .1, 1., and -.0. Simplify range validation using BigDecimal. Remove trim() to reject whitespace inputs. Add edge case tests. Fix JMH resource path so benchmarks run. Signed-off-by: Chris Simmons --- CedarJava/build.gradle | 2 +- .../java/com/cedarpolicy/value/Decimal.java | 44 ++++--------------- .../java/com/cedarpolicy/DecimalTests.java | 30 +++++++++++++ 3 files changed, 39 insertions(+), 37 deletions(-) diff --git a/CedarJava/build.gradle b/CedarJava/build.gradle index d0fdd3c3..62d3f743 100644 --- a/CedarJava/build.gradle +++ b/CedarJava/build.gradle @@ -84,7 +84,7 @@ configurations { sourceSets { jmh { java.srcDirs = ['src/jmh/java'] - resources.srcDirs = ['src/test/resources'] + resources.srcDirs = ['src/jmh/resources', 'src/test/resources'] compileClasspath += sourceSets.main.output + sourceSets.main.compileClasspath runtimeClasspath += sourceSets.main.output + sourceSets.main.runtimeClasspath } diff --git a/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java b/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java index 34320e85..0ed4b1e7 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java +++ b/CedarJava/src/main/java/com/cedarpolicy/value/Decimal.java @@ -17,8 +17,8 @@ package com.cedarpolicy.value; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import java.math.BigDecimal; import java.util.Objects; -import java.util.regex.Matcher; import java.util.regex.Pattern; /** @@ -28,49 +28,21 @@ public class Decimal extends Value { private static class DecimalValidator { - private static final Pattern DECIMAL_PATTERN = Pattern.compile("^-?([0-9])*(\\.)([0-9]{0,4})$"); - private static final String MAX_POSITIVE = "9223372036854775807"; - private static final String MAX_NEGATIVE = "9223372036854775808"; + private static final Pattern DECIMAL_PATTERN = + Pattern.compile("^-?[0-9]+\\.[0-9]{1,4}$"); + private static final BigDecimal RANGE_MIN = new BigDecimal("-922337203685477.5808"); + private static final BigDecimal RANGE_MAX = new BigDecimal("922337203685477.5807"); public static boolean validDecimal(String d) { if (d == null || d.isEmpty()) { return false; } - d = d.trim(); - Matcher matcher = DECIMAL_PATTERN.matcher(d); - if (!matcher.matches()) { + if (!DECIMAL_PATTERN.matcher(d).matches()) { return false; } - // Validate range: [-922337203685477.5808, 922337203685477.5807] - // This corresponds to the i64 range when the value is multiplied by 10000. - boolean negative = d.startsWith("-"); - String withoutSign = negative ? d.substring(1) : d; - - int dotIndex = withoutSign.indexOf('.'); - String intPart = withoutSign.substring(0, dotIndex); - String fracPart = withoutSign.substring(dotIndex + 1); - - // Strip leading zeros from integer part - intPart = intPart.replaceFirst("^0+", ""); - if (intPart.isEmpty()) { - intPart = "0"; - } - - // Pad fractional part to exactly 4 digits - fracPart = fracPart + "0000".substring(fracPart.length()); - - // Combine integer and fractional parts as the i64 representation (value * 10000) - String combined = intPart.equals("0") ? fracPart.replaceFirst("^0+", "") : intPart + fracPart; - if (combined.isEmpty()) { - combined = "0"; - } - - String limit = negative ? MAX_NEGATIVE : MAX_POSITIVE; - if (combined.length() != limit.length()) { - return combined.length() < limit.length(); - } - return combined.compareTo(limit) <= 0; + BigDecimal val = new BigDecimal(d); + return val.compareTo(RANGE_MIN) >= 0 && val.compareTo(RANGE_MAX) <= 0; } } diff --git a/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java b/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java index 9a7dd9ae..c2446bad 100644 --- a/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java +++ b/CedarJava/src/test/java/com/cedarpolicy/DecimalTests.java @@ -32,6 +32,9 @@ public void testValidDecimals() { assertDoesNotThrow(() -> new Decimal("0.0000")); assertDoesNotThrow(() -> new Decimal("123.456")); assertDoesNotThrow(() -> new Decimal("-123.456")); + // Negative zero is valid + assertDoesNotThrow(() -> new Decimal("-0.0")); + assertDoesNotThrow(() -> new Decimal("-0.0000")); } @Test @@ -77,6 +80,33 @@ public void testInvalidFormat() { assertThrows(IllegalArgumentException.class, () -> new Decimal(null)); } + @Test + public void testEdgeCaseFormats() { + // Missing integer part before dot + assertThrows(IllegalArgumentException.class, () -> new Decimal(".")); + assertThrows(IllegalArgumentException.class, () -> new Decimal(".1")); + // Missing fractional part after dot + assertThrows(IllegalArgumentException.class, () -> new Decimal("1.")); + // Negative zero with no fractional digits + assertThrows(IllegalArgumentException.class, () -> new Decimal("-.0")); + // Multiple dots + assertThrows(IllegalArgumentException.class, () -> new Decimal("1.2.3")); + // Consecutive dots + assertThrows(IllegalArgumentException.class, () -> new Decimal("-..")); + // Explicit plus sign + assertThrows(IllegalArgumentException.class, () -> new Decimal("+1.0")); + // Double negative + assertThrows(IllegalArgumentException.class, () -> new Decimal("--1.0")); + // Scientific notation + assertThrows(IllegalArgumentException.class, () -> new Decimal("1.0e2")); + // Whitespace in the middle + assertThrows(IllegalArgumentException.class, () -> new Decimal("1 .0")); + // Leading/trailing whitespace + assertThrows(IllegalArgumentException.class, () -> new Decimal(" 1.0")); + assertThrows(IllegalArgumentException.class, () -> new Decimal("1.0 ")); + assertThrows(IllegalArgumentException.class, () -> new Decimal(" 1.0 ")); + } + @Test public void testToCedarExpr() { Decimal d = new Decimal("1.0000");