diff --git a/AGENTS.md b/AGENTS.md index ade86188..1b3e670c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,10 +55,14 @@ Use the Gradle wrapper. Java **17** toolchain. ## Testing expectations - Unit tests use JUnit 5 (`useJUnitPlatform()`). -- Builtin behavior is additionally covered by OPA compliance fixtures. Note that - the harness may silently skip cases for unimplemented builtins (a missing - function is not a hard failure), so a newly-added builtin needs its own - assertion, not just a fixture that "passes." +- Builtin behavior is additionally covered by OPA compliance fixtures. A fixture + whose builtin the SDK cannot resolve fails `ComplianceTest` unless that builtin + is listed in + `opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt`, so an + unimplemented builtin can no longer make its cases pass silently. The list is a + ratchet: implementing a builtin (or registering its `BuiltinProvider`) means + deleting its line in the same change, since the suite also fails on entries no + fixture reports as missing. - Add or update tests for the behavior you change; verify against OPA parity for builtins. diff --git a/CHANGELOG.md b/CHANGELOG.md index b3dba37d..80675833 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +### Build and CI + +- Fail the compliance suite on fixtures whose builtin cannot be resolved, instead + of skipping them silently, and run it for `opa-builtins` changes + ## 0.3.0 This release raises the minimum Java version to 17, adds the `opa-proto` module diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 95af7d26..c07d16c4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,8 +16,13 @@ When contributing, please consider the following pointers: and `builtin_metadata.json` / `capabilities.json`). When implementing or fixing a builtin, verify the corresponding Go behavior for edge cases (key/JWK types, error vs. `false`, strict-mode behavior, pre-hashing, etc.) rather than - guessing. Note the compliance-test harness may silently skip cases for - unimplemented builtins, so a new builtin needs its own explicit assertion. + guessing. The compliance-test harness skips a case only when the builtin it + calls is listed in + `opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt`, so + implementing a builtin means deleting its line there in the same change. + Behavior the upstream fixtures do not cover still needs its own test — and + consider contributing the missing case to OPA, so every implementation is held + to it. - **Public APIs:** This SDK is meant to be embedded, so keep the public surface minimal. Prefer package-private types and methods; only make something `public` when consumers genuinely need it. A published API is a long-term commitment. diff --git a/opa-evaluator/src/test/java/io/github/open_policy_agent/opa/ir/ComplianceTest.java b/opa-evaluator/src/test/java/io/github/open_policy_agent/opa/ir/ComplianceTest.java index eebd085a..3a07463b 100644 --- a/opa-evaluator/src/test/java/io/github/open_policy_agent/opa/ir/ComplianceTest.java +++ b/opa-evaluator/src/test/java/io/github/open_policy_agent/opa/ir/ComplianceTest.java @@ -32,9 +32,13 @@ import org.skyscreamer.jsonassert.JSONAssert; import java.io.ByteArrayInputStream; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; +import java.io.UncheckedIOException; import java.net.URISyntaxException; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -47,7 +51,9 @@ import java.util.ServiceLoader; import java.util.Set; import java.util.Stack; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -58,10 +64,21 @@ public class ComplianceTest { Pattern.compile("^[A-Za-z0-9_-]+\\.[A-Za-z0-9_-]*\\.[A-Za-z0-9_-]*$"); private static final PolicyReader POLICY_READER = ServiceLoader.load(PolicyReader.class).findFirst().orElseThrow(); + private static final String KNOWN_MISSING_RESOURCE = "compliance/known-missing-builtins.txt"; private static String complianceDir; - private static Set missingFunctions = ConcurrentHashMap.newKeySet(); - private static boolean ignoreFunctionNotFoundTests = true; + /** + * Builtins the fixtures call but this SDK cannot resolve. A case that calls one of these is + * skipped; a case calling any other unresolved builtin fails, so an unimplemented builtin can no + * longer make its fixtures pass silently. + */ + private static final Set knownMissingBuiltins = loadKnownMissingBuiltins(); + + /** Builtins from {@link #knownMissingBuiltins} actually hit during this run. */ + private static final Set missingFunctions = ConcurrentHashMap.newKeySet(); + + private static final AtomicInteger casesExecuted = new AtomicInteger(); + private static final AtomicInteger casesDiscovered = new AtomicInteger(); /** * Alternate acceptable error message substrings for tests where the Java IR evaluator produces a @@ -96,15 +113,55 @@ public class ComplianceTest { @AfterAll public static void reportMissingFunctions() { if (!missingFunctions.isEmpty()) { - String report = buildMissingFunctionsReport(); - // TODO: add missing builtins - System.err.println(report); + System.err.println(buildMissingFunctionsReport()); + } + // A filtered run (e.g. --tests "...ComplianceTest$some case") only reaches a subset of the + // fixtures, so entries could look stale when they are simply not exercised. Only ratchet when + // every discovered case ran. + if (casesExecuted.get() != casesDiscovered.get()) { + return; + } + Set stale = new TreeSet<>(knownMissingBuiltins); + stale.removeAll(missingFunctions); + if (!stale.isEmpty()) { + fail( + "These builtins are listed in " + + KNOWN_MISSING_RESOURCE + + " but no compliance case reported them missing — they are either implemented now or" + + " no longer covered by a fixture. Remove them from the list: " + + stale); } } + /** + * Reads the known-missing builtin list. One name per line; blank lines and {@code #} comments are + * ignored. + */ + private static Set loadKnownMissingBuiltins() { + URL resource = ComplianceTest.class.getClassLoader().getResource(KNOWN_MISSING_RESOURCE); + if (resource == null) { + throw new IllegalStateException(KNOWN_MISSING_RESOURCE + " not found on the classpath"); + } + Set names = new TreeSet<>(); + try (BufferedReader reader = + new BufferedReader(new InputStreamReader(resource.openStream(), StandardCharsets.UTF_8))) { + reader + .lines() + .map(line -> line.replaceFirst("#.*", "").trim()) + .filter(line -> !line.isEmpty()) + .forEach(names::add); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return Set.copyOf(names); + } + private static String buildMissingFunctionsReport() { StringBuilder sb = new StringBuilder(); sb.append("\n=== Missing Builtin Report ===\n"); + sb.append("Cases skipped for builtins listed in ") + .append(KNOWN_MISSING_RESOURCE) + .append(":\n"); sb.append("Total missing builtins: ").append(missingFunctions.size()).append("\n"); missingFunctions.stream().sorted().forEach(fn -> sb.append(" - ").append(fn).append("\n")); sb.append("================================\n"); @@ -123,6 +180,7 @@ public static Stream getComplianceTestData() throws IOException { JsonNode root = mapper.readTree(f); List cases = new ArrayList<>(); root.get("cases").forEach(cases::add); + casesDiscovered.addAndGet(cases.size()); return cases.stream() .map(c -> new Object[]{ c.get("note").asText("unknown"), c @@ -343,6 +401,7 @@ private static RegoValue jsonNodeToRegoValue( @ParameterizedTest(name = "{0}") @MethodSource("getComplianceTestData") public void testEvaluate(String caseName, JsonNode root) { + casesExecuted.incrementAndGet(); try { ObjectMapper mapper = new ObjectMapper().registerModule(new io.github.open_policy_agent.opa.jackson.RegoValueModule()); @@ -422,11 +481,14 @@ public void testEvaluate(String caseName, JsonNode root) { JsonNode want = root.get("want_result"); if (want == null) { - JsonNode wantError = root.get("want_error"); - if (wantError != null) { - fail("error wanted but not thrown: " + wantError.asText()); + if (root.has("want_error") || root.has("want_error_code")) { + fail( + "error wanted but not thrown: " + + (root.has("want_error") + ? root.get("want_error").asText() + : root.get("want_error_code").asText())); } - System.out.println("no want_result for: " + caseName); + fail("case declares neither want_result nor want_error, so it asserts nothing"); return; } @@ -448,15 +510,21 @@ public void testEvaluate(String caseName, JsonNode root) { } } } catch (OpaException re) { - // Track missing functions for reporting + // A builtin the SDK does not resolve makes the case unrunnable. Skip it only when the + // builtin is a known gap, so that an unlisted one fails instead of passing silently. if (re instanceof FunctionNotFoundError) { String functionName = (String) re.getContext().get("name"); - if (functionName != null) { - missingFunctions.add(functionName); - } - if (ignoreFunctionNotFoundTests) { - return; + if (functionName == null || !knownMissingBuiltins.contains(functionName)) { + fail( + "builtin '" + + functionName + + "' could not be resolved, so this case asserted nothing. Implement the" + + " builtin (and register its provider in META-INF/services), or add it to " + + KNOWN_MISSING_RESOURCE + + " with a reason."); } + missingFunctions.add(functionName); + return; } if (root.get("want_error_code") == null && root.get("want_error") == null) { diff --git a/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt b/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt new file mode 100644 index 00000000..4a197ebd --- /dev/null +++ b/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt @@ -0,0 +1,117 @@ +# Builtins that the OPA compliance fixtures exercise but this SDK does not +# resolve at evaluation time. ComplianceTest skips a case only when the builtin +# it calls is listed here, and fails the case otherwise — so a fixture can no +# longer pass silently just because its builtin is absent (see AGENTS.md). +# +# The list is a ratchet: ComplianceTest also fails when an entry here is no +# longer missing, so implementing a builtin means deleting its line in the same +# change. Format is one builtin name per line; `#` starts a comment. + +# --- Implemented, but never registered --------------------------------------- +# These live in opa-builtins sub-modules and are listed as supported in +# opa-builtins/README.md, but the BuiltinProvider entry in each module's +# META-INF/services file is commented out, so ServiceLoader never finds them +# and they are unreachable for consumers. Tracked separately from this list; +# un-commenting the five files is the fix, after which the parity failures the +# fixtures then expose need triage. + +# opa-builtins-crypto +crypto.hmac.equal +crypto.hmac.md5 +crypto.hmac.sha1 +crypto.hmac.sha256 +crypto.hmac.sha512 +crypto.md5 +crypto.sha1 +crypto.sha256 + +# opa-builtins-json +json.filter +json.is_valid +json.marshal +json.marshal_with_options +json.match_schema +json.patch +json.remove +json.unmarshal +json.verify_schema +yaml.is_valid +yaml.marshal +yaml.unmarshal + +# opa-builtins-net +net.cidr_contains +net.cidr_contains_matches +net.cidr_expand +net.cidr_intersects +net.cidr_is_valid +net.cidr_merge +net.lookup_ip_addr + +# opa-builtins-regex +regex.find_all_string_submatch_n +regex.find_n +regex.globs_match +regex.is_valid +regex.match +regex.replace +regex.split +regex.template_match + +# opa-builtins-semver +semver.compare +semver.is_valid + +# --- Not implemented anywhere in the SDK ------------------------------------- + +bits.and +bits.lsh +bits.negate +bits.or +bits.rsh +bits.xor + +crypto.parse_private_keys +crypto.x509.parse_and_verify_certificates +crypto.x509.parse_certificate_request +crypto.x509.parse_certificates +crypto.x509.parse_keypair +crypto.x509.parse_rsa_private_key + +glob.match +glob.quote_meta + +graph.reachable +graph.reachable_paths + +graphql.is_valid +graphql.parse +graphql.parse_and_verify +graphql.parse_query +graphql.parse_schema +graphql.schema_is_valid + +http.send + +internal.template_string + +io.jwt.verify_eddsa + +rego.parse_module + +strings.render_template +strings.split_n + +units.parse +units.parse_bytes + +urlquery.decode +urlquery.decode_object +urlquery.encode +urlquery.encode_object + +uuid.parse +uuid.rfc4122 + +# Delete this line along with the walk implementation in #141. +walk diff --git a/tools/policy/pr-check/pr_check.rego b/tools/policy/pr-check/pr_check.rego index 87eab19f..16f0061a 100644 --- a/tools/policy/pr-check/pr_check.rego +++ b/tools/policy/pr-check/pr_check.rego @@ -9,6 +9,17 @@ build_change_files := [ changes["opa-evaluator"] if { some changed_file in input startswith(changed_file.filename, "opa-evaluator/") +} else if { + # ComplianceTest lives in opa-evaluator but resolves builtins from the opa-builtins sub-modules + # over the BuiltinProvider SPI, and fails on any fixture whose builtin it cannot resolve. A + # builtins-only change (implementing a builtin, or registering its provider) therefore has to + # re-run the compliance suite. + some changed_file in input + startswith(changed_file.filename, "opa-builtins/") +} else if { + # An OPA version bump here regenerates the compliance fixtures the suite reads. + some changed_file in input + startswith(changed_file.filename, "tools/generate-compliance-tests/") } else if { some changed_file in input changed_file.filename in build_change_files diff --git a/tools/policy/pr-check/pr_check_test.rego b/tools/policy/pr-check/pr_check_test.rego index 07e8b813..23507496 100644 --- a/tools/policy/pr-check/pr_check_test.rego +++ b/tools/policy/pr-check/pr_check_test.rego @@ -68,6 +68,15 @@ test_builtins_change_triggers_builtins if { pr_check.changes["opa-builtins"] with input as example_builtins_changelist } +# The compliance suite that gates builtin coverage runs in opa-evaluator. +test_builtins_change_triggers_evaluator if { + pr_check.changes["opa-evaluator"] with input as example_builtins_changelist +} + +test_generator_change_triggers_evaluator if { + pr_check.changes["opa-evaluator"] with input as example_generator_changelist +} + test_slf4j_change_triggers_slf4j if { pr_check.changes["opa-slf4j"] with input as example_slf4j_changelist }