From 14452927a3419877eca94fc9abfd5c0171d80e38 Mon Sep 17 00:00:00 2001 From: Sebastian Spaink Date: Tue, 11 Aug 2026 08:13:48 -0500 Subject: [PATCH] fix(json): register provider and fix JSON/YAML parity gaps The BuiltinProvider entry for opa-builtins-json was commented out in its META-INF/services file, so ServiceLoader never discovered it and none of the twelve json/yaml builtins were reachable for consumers. Registering it exposed 16 failing compliance cases: - json.marshal_with_options ignored "pretty" and "prefix" and used Jackson's layout. Go composes prefix + MarshalIndent(v, prefix, indent): the prefix leads the document and repeats on every line, the separator is ": ", an empty container stays on one line, and an explicit "pretty": false disables indent and prefix. An unknown option key is a type error, which was not reported at all. - json.is_valid raised a type error for a non-string operand where Go returns false. - json.match_schema and json.verify_schema accepted a schema with an unknown "type". The validator silently never matches it, whereas Go rejects the schema, so the schema is now checked up front. Their error objects also used networknt's field names and wording rather than the desc/error/field/type shape Go returns. - json.patch could not address Rego sets. Sets serialize as arrays, so their locations are recorded before serializing: a member is then matched by value rather than index, an absent member appends, and the result is rebuilt as a set. Since the path segment is the member, an "add" whose value differs from the segment is undefined. yaml.unmarshal now reports Go's terse "yaml: line N: " instead of SnakeYAML's multi-line snippet. The residual wording difference is recorded in ComplianceTest's alternate-message map, as mapping every SnakeYAML diagnostic onto yaml.v2's phrasing is not tractable. Raises the evaluator's test heap. The jsonpatch/json_patch_tests fixture runs the whole upstream JSON Patch spec suite inside a single policy, and once json.patch resolves it needs more than Gradle's default 512m. This is a test-only limit, but it does say json.patch is memory-hungry on large inputs and is worth profiling separately. A path segment of digits that overflow an int is no longer read as an array index: it reached Integer.parseInt and threw NumberFormatException out of the builtin, where it should leave the call undefined. Reported by CodeQL on the pull request; the pre-existing resolver caught this, the new set-aware one did not. Also replaces the deprecated JsonNode.fields() with properties() and rejects an odd argument count in the test helper. Adds JsonPatchSetTest for the set behaviour and the index overflow. Four of its five cases fail against the unfixed code. Removes the twelve json/yaml entries from known-missing-builtins.txt (64 -> 52), leaving only builtins that are genuinely unimplemented. Signed-off-by: Sebastian Spaink --- .../opa/ast/builtin/impls/JsonBuiltins.java | 435 ++++++++++++++++-- ...licy_agent.opa.ast.builtin.BuiltinProvider | 2 +- .../ast/builtin/impls/JsonPatchSetTest.java | 135 ++++++ opa-evaluator/build.gradle.kts | 4 + .../opa/ir/ComplianceTest.java | 5 +- .../compliance/known-missing-builtins.txt | 25 +- 6 files changed, 551 insertions(+), 55 deletions(-) create mode 100644 opa-builtins/opa-builtins-json/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonPatchSetTest.java diff --git a/opa-builtins/opa-builtins-json/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonBuiltins.java b/opa-builtins/opa-builtins-json/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonBuiltins.java index 9e3d531a..1426c3a3 100644 --- a/opa-builtins/opa-builtins-json/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonBuiltins.java +++ b/opa-builtins/opa-builtins-json/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonBuiltins.java @@ -14,6 +14,7 @@ import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; +import com.fasterxml.jackson.dataformat.yaml.snakeyaml.error.MarkedYAMLException; import com.fasterxml.jackson.dataformat.yaml.YAMLGenerator; import com.github.fge.jsonpatch.JsonPatch; import com.github.fge.jsonpatch.JsonPatchException; @@ -183,37 +184,96 @@ public RegoString marshal_with_options(EvaluationContext ctx, RegoValue[] args) RegoValue input = args[0]; RegoObject options = getArg(args, 1, RegoObject.class); - try { - ObjectMapper mapper = JSON_MAPPER.copy(); - - // Check for indent option - RegoValue indentValue = options.getProperty(new RegoString("indent")); - if (indentValue instanceof RegoString) { - String indent = ((RegoString) indentValue).getValue(); - DefaultPrettyPrinter printer = new DefaultPrettyPrinter(); - // Use DefaultIndenter which accepts custom indent string - DefaultPrettyPrinter.Indenter indenter = - new DefaultIndenter(indent, DefaultIndenter.SYS_LF); - printer.indentArraysWith(indenter); - printer.indentObjectsWith(indenter); - mapper.setDefaultPrettyPrinter(printer); - mapper.enable(SerializationFeature.INDENT_OUTPUT); + // Go's implementation rejects unknown keys before looking at any of them. + for (RegoValue key : options.getProperties().keySet()) { + String name = key instanceof RegoString ? ((RegoString) key).getValue() : String.valueOf(key); + if (!MARSHAL_OPTION_KEYS.contains(name)) { + throw new TypeError("object contained unknown key \"" + name + "\""); } + } + + RegoValue prettyValue = options.getProperty(new RegoString("pretty")); + RegoValue indentValue = options.getProperty(new RegoString("indent")); + RegoValue prefixValue = options.getProperty(new RegoString("prefix")); - // Check for prefix option (for pretty printing) - RegoValue prefixValue = options.getProperty(new RegoString("prefix")); - if (prefixValue instanceof RegoString) { - // Prefix is typically used with indent for pretty printing - mapper.enable(SerializationFeature.INDENT_OUTPUT); + String indent = indentValue instanceof RegoString ? ((RegoString) indentValue).getValue() : "\t"; + String prefix = prefixValue instanceof RegoString ? ((RegoString) prefixValue).getValue() : ""; + + // "pretty" is only a default: supplying "indent" or "prefix" implies pretty output, but an + // explicit "pretty": false disables it and makes the other two options inert. + boolean pretty = indentValue != null || prefixValue != null; + if (prettyValue instanceof RegoBoolean) { + pretty = ((RegoBoolean) prettyValue).getValue(); + } + + try { + if (!pretty) { + return new RegoString(JSON_MAPPER.writeValueAsString(input)); } - String json = mapper.writeValueAsString(input); - return new RegoString(json); + // Go composes the result as prefix + json.MarshalIndent(v, prefix, indent), so the prefix + // leads the document and is repeated at the start of every subsequent line. + ObjectMapper mapper = JSON_MAPPER.copy(); + DefaultPrettyPrinter printer = new GoPrettyPrinter(prefix, indent); + mapper.setDefaultPrettyPrinter(printer); + mapper.enable(SerializationFeature.INDENT_OUTPUT); + + return new RegoString(prefix + mapper.writer(printer).writeValueAsString(input)); } catch (JsonProcessingException e) { throw new BuiltinError("json.marshal_with_options: " + e.getMessage()); } } + private static final Set MARSHAL_OPTION_KEYS = Set.of("pretty", "indent", "prefix"); + + /** + * Reproduces Go's json.MarshalIndent layout: newline-separated, each line indented by the + * prefix followed by the indent repeated per nesting level, and `": "` between key and value. + * Jackson's DefaultPrettyPrinter otherwise emits spaces around the colon and no line prefix. + */ + private static final class GoPrettyPrinter extends DefaultPrettyPrinter { + private static final long serialVersionUID = 1L; + + GoPrettyPrinter(String prefix, String indent) { + DefaultIndenter indenter = new DefaultIndenter(indent, DefaultIndenter.SYS_LF + prefix); + indentArraysWith(indenter); + indentObjectsWith(indenter); + _objectFieldValueSeparatorWithSpaces = ": "; + } + + private GoPrettyPrinter(GoPrettyPrinter base) { + super(base); + _objectFieldValueSeparatorWithSpaces = base._objectFieldValueSeparatorWithSpaces; + } + + @Override + public DefaultPrettyPrinter createInstance() { + return new GoPrettyPrinter(this); + } + + @Override + public void writeEndObject(JsonGenerator g, int nrOfEntries) throws java.io.IOException { + // Go emits {} for an empty object rather than opening a new indented line. Track the + // nesting level down as super would, or the enclosing containers close over-indented. + if (nrOfEntries == 0) { + --_nesting; + g.writeRaw('}'); + return; + } + super.writeEndObject(g, nrOfEntries); + } + + @Override + public void writeEndArray(JsonGenerator g, int nrOfValues) throws java.io.IOException { + if (nrOfValues == 0) { + --_nesting; + g.writeRaw(']'); + return; + } + super.writeEndArray(g, nrOfValues); + } + } + @OpaBuiltin( name = "json.unmarshal", description = "Deserializes the input string.", @@ -239,7 +299,11 @@ public RegoValue unmarshal(EvaluationContext ctx, RegoValue[] args) { result = @OpaType(name = "result", description = "`true` if `x` is valid JSON, `false` otherwise")) public RegoBoolean is_valid(EvaluationContext ctx, RegoValue[] args) { - String jsonInput = getArg(args, 0, RegoString.class).getValue(); + // Go returns false for a non-string operand rather than raising a type error. + if (!(args[0] instanceof RegoString)) { + return RegoBoolean.FALSE; + } + String jsonInput = ((RegoString) args[0]).getValue(); try { JSON_MAPPER.readTree(jsonInput); @@ -330,6 +394,205 @@ public RegoValue remove(EvaluationContext ctx, RegoValue[] args) { } } + /** + * Records the JSON Pointer of every set inside `value`. Sets serialize as arrays, so json.patch + * has to remember where they were: paths into a set address members by value rather than index, + * and the patched result has to be turned back into a set. + */ + private static void collectSetPaths(RegoValue value, String pointer, Set out) { + if (value instanceof RegoSet) { + out.add(pointer); + int i = 0; + for (RegoValue member : ((RegoSet) value).getValue()) { + collectSetPaths(member, pointer + "/" + i++, out); + } + } else if (value instanceof RegoArray) { + List values = ((RegoArray) value).getValue(); + for (int i = 0; i < values.size(); i++) { + collectSetPaths(values.get(i), pointer + "/" + i, out); + } + } else if (value instanceof RegoObject) { + for (Map.Entry e : ((RegoObject) value).getProperties().entrySet()) { + if (e.getKey() instanceof RegoString) { + String key = ((RegoString) e.getKey()).getValue(); + collectSetPaths(e.getValue(), pointer + "/" + escapePointerSegment(key), out); + } + } + } + } + + private static String escapePointerSegment(String segment) { + return segment.replace("~", "~0").replace("/", "~1"); + } + + /** + * Reads a path segment as an array index, returning null when it is not one. Digits that + * overflow an int are not a usable index either, so they are rejected rather than allowed to + * throw out of the builtin. + */ + private static Integer asArrayIndex(JsonNode segment) { + if (segment.isIntegralNumber()) { + return segment.canConvertToInt() ? Integer.valueOf(segment.asInt()) : null; + } + if (segment.isTextual() && segment.asText().matches("-?\\d+")) { + try { + return Integer.valueOf(segment.asText()); + } catch (NumberFormatException e) { + return null; + } + } + return null; + } + + /** + * Splits a patch path into segments. A path given as an array can carry non-string segments — + * a set member is addressed by its value, which may itself be an array or object — so segments + * stay as nodes rather than being flattened to a string up front. + */ + private List pathSegments(JsonNode pathNode) { + List segments = new ArrayList<>(); + if (pathNode.isArray()) { + for (JsonNode segment : pathNode) { + segments.add(segment); + } + return segments; + } + + String path = pathNode.isTextual() ? pathNode.asText() : pathNode.asText(); + if (path.startsWith("/")) { + path = path.substring(1); + } + if (path.isEmpty()) { + return segments; + } + for (String raw : path.split("/", -1)) { + segments.add(JSON_MAPPER.getNodeFactory().textNode(raw.replace("~1", "/").replace("~0", "~"))); + } + return segments; + } + + /** + * Resolves a patch path against the document, turning value lookups into indices. Arrays already + * allow addressing an element by value; a set additionally has no inherent order, so a member is + * located by deep equality and a member that is not present resolves to the append token. + */ + private PathResolution resolvePath(JsonNode pathNode, JsonNode document, Set setPaths) { + // Documents without sets keep the long-standing resolution untouched: set support only needs + // to change how a member is addressed, and this is by far the hotter path. + if (setPaths.isEmpty()) { + return new PathResolution(normalizeAndResolveJsonPointerPath(pathNode, document), null); + } + + StringBuilder resolved = new StringBuilder(); + String currentPointer = ""; + JsonNode current = document; + JsonNode appendedMember = null; + + for (JsonNode segment : pathSegments(pathNode)) { + boolean isSet = setPaths.contains(currentPointer); + String literal = + segment.isTextual() ? segment.asText() : segment.toString(); + + if (current != null && current.isArray()) { + int index = -1; + if (isSet) { + // A set has no order, so a member is addressed by its value. + for (int i = 0; i < current.size(); i++) { + if (current.get(i).equals(segment)) { + index = i; + break; + } + } + if (index < 0) { + // Absent member: for "add" this means append, which RFC 6902 spells "-". + resolved.append("/-"); + appendedMember = segment; + current = null; + currentPointer = null; + continue; + } + } else { + Integer parsed = asArrayIndex(segment); + if (parsed == null) { + // Not an index into a plain array. Keep the segment so the patch library reports it. + resolved.append('/').append(escapePointerSegment(literal)); + current = null; + currentPointer = null; + continue; + } + index = parsed; + } + + resolved.append('/').append(index); + current = index >= 0 && index < current.size() ? current.get(index) : null; + currentPointer = currentPointer == null ? null : currentPointer + "/" + index; + } else if (current != null && current.isObject()) { + resolved.append('/').append(escapePointerSegment(literal)); + current = current.get(literal); + currentPointer = currentPointer == null ? null : currentPointer + "/" + escapePointerSegment(literal); + } else { + resolved.append('/').append(escapePointerSegment(literal)); + current = null; + currentPointer = null; + } + } + + return new PathResolution(resolved.toString(), appendedMember); + } + + /** + * A resolved patch path. `appendedSetMember` is the addressed value when the path appends to a + * set, which lets `add` check that the value matches: in a set the path segment *is* the member, + * so `{"op": "add", "path": "foo/d", "value": "e"}` is incoherent and undefined in OPA. + */ + private static final class PathResolution { + private final String path; + private final JsonNode appendedSetMember; + + PathResolution(String path, JsonNode appendedSetMember) { + this.path = path; + this.appendedSetMember = appendedSetMember; + } + } + + /** Rebuilds a patched document, restoring the sets recorded by {@link #collectSetPaths}. */ + private RegoValue restoreSets(JsonNode node, String pointer, Set setPaths, boolean sorted) { + if (node.isArray() && setPaths.contains(pointer)) { + Set members = sorted ? new LinkedHashSet<>() : new HashSet<>(); + int i = 0; + for (JsonNode element : node) { + members.add(restoreSets(element, pointer + "/" + i++, setPaths, sorted)); + } + return new RegoSet(sorted, members); + } + if (node.isArray()) { + RegoArray array = new RegoArray(); + int i = 0; + for (JsonNode element : node) { + array.addValue(restoreSets(element, pointer + "/" + i++, setPaths, sorted)); + } + return array; + } + if (node.isObject()) { + RegoObject object = new RegoObject(); + for (Map.Entry e : node.properties()) { + object.setProp( + new RegoString(e.getKey()), + restoreSets( + e.getValue(), + pointer + "/" + escapePointerSegment(e.getKey()), + setPaths, + sorted)); + } + return object; + } + try { + return convertToRegoValue(JSON_MAPPER.treeToValue(node, Object.class)); + } catch (JsonProcessingException e) { + throw new BuiltinError("json.patch: " + e.getMessage()); + } + } + @OpaBuiltin( name = "json.patch", description = "Patches object according to RFC 6902 JSON Patch standard.", @@ -357,6 +620,10 @@ public RegoValue patch(EvaluationContext ctx, RegoValue[] args) { } JsonNode patchesNode = JSON_MAPPER.readTree(patchesJson); + // Sets serialize as arrays, so remember where they were before the document becomes JSON. + Set setPaths = new HashSet<>(); + collectSetPaths(object, "", setPaths); + // Normalize paths - ensure they start with "/" and convert array paths to strings // Also resolve array value lookups to indices if (patchesNode.isArray()) { @@ -398,8 +665,15 @@ public RegoValue patch(EvaluationContext ctx, RegoValue[] args) { return RegoUndefined.INSTANCE; } - String normalizedPath = normalizeAndResolveJsonPointerPath(pathNode, objectNode); - normalizedOp.put("path", normalizedPath); + PathResolution resolvedPath = resolvePath(pathNode, objectNode, setPaths); + // Appending to a set means the segment is the member, so an "add" whose value + // differs from the segment is incoherent and undefined. + if (resolvedPath.appendedSetMember != null + && "add".equals(op) + && !resolvedPath.appendedSetMember.equals(normalizedOp.get("value"))) { + return RegoUndefined.INSTANCE; + } + normalizedOp.put("path", resolvedPath.path); } // Also normalize "from" field for move/copy operations @@ -411,8 +685,7 @@ public RegoValue patch(EvaluationContext ctx, RegoValue[] args) { return RegoUndefined.INSTANCE; } - String normalizedFrom = normalizeAndResolveJsonPointerPath(fromNode, objectNode); - normalizedOp.put("from", normalizedFrom); + normalizedOp.put("from", resolvePath(fromNode, objectNode, setPaths).path); } normalizedPatches.add(normalizedOp); @@ -446,7 +719,10 @@ public RegoValue patch(EvaluationContext ctx, RegoValue[] args) { return RegoUndefined.INSTANCE; } - // Convert back to RegoValue + // Convert back to RegoValue, restoring any sets the document started with. + if (!setPaths.isEmpty()) { + return restoreSets(patched, "", setPaths, ctx.sortSets); + } Object result = JSON_MAPPER.treeToValue(patched, Object.class); return convertToRegoValue(result); } catch (BuiltinError e) { @@ -593,6 +869,41 @@ private String normalizeJsonPointerPath(JsonNode pathNode) { } } + /** JSON Schema's primitive type names, in the order Go lists them when rejecting a bad one. */ + private static final List VALID_SCHEMA_TYPES = + List.of("array", "boolean", "integer", "number", "null", "object", "string"); + + /** + * Walks a schema for `"type"` values that are not JSON Schema primitives. The validator accepts + * an unknown type and simply never matches, whereas Go rejects the schema outright, so the check + * has to happen before validation. Returns the Go-style complaint, or null when the schema is + * fine. + */ + private static String findInvalidSchemaType(JsonNode node) { + if (node == null) { + return null; + } + if (node.isObject()) { + JsonNode type = node.get("type"); + if (type != null && type.isTextual() && !VALID_SCHEMA_TYPES.contains(type.asText())) { + return "has a primitive type that is NOT VALID -- given: /" + + type.asText() + + "/ Expected valid values are:[" + + String.join(" ", VALID_SCHEMA_TYPES) + + "]"; + } + } + if (node.isObject() || node.isArray()) { + for (JsonNode child : node) { + String found = findInvalidSchemaType(child); + if (found != null) { + return found; + } + } + } + return null; + } + @OpaBuiltin( name = "json.match_schema", description = "Verifies the input matches the provided JSON schema.", @@ -632,6 +943,10 @@ public RegoValue match_schema(EvaluationContext ctx, RegoValue[] args) { } // Validate + String invalidType = findInvalidSchemaType(schemaNode); + if (invalidType != null) { + throw new BuiltinError(invalidType); + } SchemaRegistry registry = SchemaRegistry.withDefaultDialect(SpecificationVersion.DRAFT_7); Schema schema = registry.getSchema(schemaNode); List errors = schema.validate(documentNode); @@ -645,22 +960,58 @@ public RegoValue match_schema(EvaluationContext ctx, RegoValue[] args) { result.addValue(RegoBoolean.FALSE); RegoArray errorArray = new RegoArray(); for (Error error : errors) { + // Go surfaces gojsonschema's ResultError fields: type, field, desc and the combined + // "field: desc" as error. + String field = instanceLocationToField(error.getInstanceLocation()); + String desc = goStyleDescription(error); RegoObject errorObj = new RegoObject(); - errorObj.setProp(new RegoString("message"), new RegoString(error.getMessage())); - errorObj.setProp( - new RegoString("path"), new RegoString(error.getEvaluationPath().toString())); - errorObj.setProp(new RegoString("type"), new RegoString(error.getKeyword())); + errorObj.setProp(new RegoString("desc"), new RegoString(desc)); + errorObj.setProp(new RegoString("error"), new RegoString(field + ": " + desc)); + errorObj.setProp(new RegoString("field"), new RegoString(field)); + errorObj.setProp(new RegoString("type"), new RegoString(goStyleType(error.getKeyword()))); errorArray.addValue(errorObj); } result.addValue(errorArray); } return result; + } catch (BuiltinError e) { + throw e; } catch (Exception e) { throw new BuiltinError("json.match_schema: " + e.getMessage()); } } + /** gojsonschema names the root "(root)" and drops the leading slash from a pointer. */ + private static String instanceLocationToField(Object instanceLocation) { + String path = instanceLocation == null ? "" : instanceLocation.toString(); + if (path.isEmpty() || "/".equals(path)) { + return "(root)"; + } + return path.startsWith("/") ? path.substring(1).replace('/', '.') : path; + } + + /** gojsonschema reports the keyword as e.g. "invalid_type" for a "type" mismatch. */ + private static String goStyleType(String keyword) { + return "type".equals(keyword) ? "invalid_type" : keyword; + } + + /** + * networknt phrases a type mismatch as "string found, integer expected"; gojsonschema, which + * Go's json.match_schema returns, uses "Invalid type. Expected: integer, given: string". + */ + private static String goStyleDescription(Error error) { + String message = error.getMessage(); + if ("type".equals(error.getKeyword())) { + java.util.regex.Matcher m = + java.util.regex.Pattern.compile("^(\\S+) found, (\\S+) expected$").matcher(message); + if (m.matches()) { + return "Invalid type. Expected: " + m.group(2) + ", given: " + m.group(1); + } + } + return message; + } + @OpaBuiltin( name = "json.verify_schema", description = "Verifies the input is a valid JSON schema.", @@ -687,6 +1038,13 @@ public RegoValue verify_schema(EvaluationContext ctx, RegoValue[] args) { } // Try to create a schema - if it succeeds, it's valid + String invalidType = findInvalidSchemaType(schemaNode); + if (invalidType != null) { + RegoArray invalid = new RegoArray(); + invalid.addValue(RegoBoolean.FALSE); + invalid.addValue(new RegoString("jsonschema: " + invalidType)); + return invalid; + } SchemaRegistry registry = SchemaRegistry.withDefaultDialect(SpecificationVersion.DRAFT_7); registry.getSchema(schemaNode); @@ -760,11 +1118,24 @@ public RegoValue yamlUnmarshal(EvaluationContext ctx, RegoValue[] args) { try { Object parsed = YAML_MAPPER.readValue(yamlInput, Object.class); return convertToRegoValueFromYaml(parsed); + } catch (MarkedYAMLException e) { + // Go's yaml.v2 reports "yaml: line N: ". Jackson's default message is a multi-line + // snippet with carets, so rebuild the terse form from the structured fields. + throw new BuiltinError("yaml: " + describeYamlError(e)); } catch (IOException e) { throw new BuiltinError("yaml.unmarshal: " + e.getMessage()); } } + private static String describeYamlError(MarkedYAMLException e) { + String problem = e.getProblem() != null ? e.getProblem() : e.getMessage(); + if (e.getProblemMark() == null) { + return problem; + } + // SnakeYAML lines are 0-based; Go reports them 1-based. + return "line " + (e.getProblemMark().getLine() + 1) + ": " + problem; + } + /** * Converts a Java object (from Jackson YAML deserialization) to a RegoValue. In YAML, empty * values are treated as null, so we convert empty strings to RegoNull. diff --git a/opa-builtins/opa-builtins-json/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider b/opa-builtins/opa-builtins-json/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider index eb6b6f7c..fa467368 100644 --- a/opa-builtins/opa-builtins-json/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider +++ b/opa-builtins/opa-builtins-json/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider @@ -1 +1 @@ -#io.github.open_policy_agent.opa.ast.builtin.impls.JsonBuiltins +io.github.open_policy_agent.opa.ast.builtin.impls.JsonBuiltins diff --git a/opa-builtins/opa-builtins-json/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonPatchSetTest.java b/opa-builtins/opa-builtins-json/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonPatchSetTest.java new file mode 100644 index 00000000..33335776 --- /dev/null +++ b/opa-builtins/opa-builtins-json/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/JsonPatchSetTest.java @@ -0,0 +1,135 @@ +package io.github.open_policy_agent.opa.ast.builtin.impls; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; + +import io.github.open_policy_agent.opa.ast.types.RegoArray; +import io.github.open_policy_agent.opa.ast.types.RegoInt32; +import io.github.open_policy_agent.opa.ast.types.RegoObject; +import io.github.open_policy_agent.opa.ast.types.RegoSet; +import io.github.open_policy_agent.opa.ast.types.RegoString; +import io.github.open_policy_agent.opa.ast.types.RegoUndefined; +import io.github.open_policy_agent.opa.ast.types.RegoValue; +import io.github.open_policy_agent.opa.rego.EvaluationContext; +import java.util.LinkedHashSet; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +class JsonPatchSetTest { + + private final JsonBuiltins builtins = new JsonBuiltins(); + private final EvaluationContext ctx = new EvaluationContext.Builder().build(); + + private static RegoSet set(RegoValue... members) { + return new RegoSet(false, new LinkedHashSet<>(List.of(members))); + } + + /** Builds a patch operation from alternating key/value pairs. */ + private static RegoObject op(String... kv) { + if (kv.length % 2 != 0) { + throw new IllegalArgumentException("op() takes key/value pairs, got " + kv.length + " args"); + } + RegoObject o = new RegoObject(); + for (int i = 0; i + 1 < kv.length; i += 2) { + o.setProp(new RegoString(kv[i]), new RegoString(kv[i + 1])); + } + return o; + } + + @Test + @Timeout(10) + void addsMemberToSet() { + RegoObject doc = new RegoObject(); + doc.setProp(new RegoString("foo"), set(new RegoString("a"), new RegoString("b"))); + + RegoValue r = + builtins.patch( + ctx, + new RegoValue[] { + doc, new RegoArray(List.of(op("op", "add", "path", "foo/c", "value", "c"))) + }); + + RegoObject expected = new RegoObject(); + expected.setProp( + new RegoString("foo"), set(new RegoString("a"), new RegoString("b"), new RegoString("c"))); + assertEquals(expected, r); + } + + @Test + @Timeout(10) + void removesMemberFromSet() { + RegoObject doc = new RegoObject(); + doc.setProp( + new RegoString("foo"), set(new RegoString("a"), new RegoString("b"), new RegoString("c"))); + + RegoValue r = + builtins.patch( + ctx, + new RegoValue[] {doc, new RegoArray(List.of(op("op", "remove", "path", "foo/b")))}); + + RegoObject expected = new RegoObject(); + expected.setProp(new RegoString("foo"), set(new RegoString("a"), new RegoString("c"))); + assertEquals(expected, r); + } + + // In a set the path segment *is* the member, so adding "e" at ".../d" is incoherent. + @Test + @Timeout(10) + void addWithMismatchedMemberIsUndefined() { + RegoObject doc = new RegoObject(); + doc.setProp(new RegoString("foo"), set(new RegoString("a"), new RegoString("b"))); + + RegoValue r = + builtins.patch( + ctx, + new RegoValue[] { + doc, new RegoArray(List.of(op("op", "add", "path", "foo/d", "value", "e"))) + }); + + // RegoUndefined.equals() always returns false, so compare by identity. + assertSame(RegoUndefined.INSTANCE, r); + } + + // A digit-only segment that overflows an int is not a usable index. It used to reach + // Integer.parseInt and throw NumberFormatException out of the builtin instead of going undefined. + @Test + @Timeout(10) + void oversizedNumericIndexIsUndefined() { + RegoObject doc = new RegoObject(); + doc.setProp(new RegoString("members"), set(new RegoString("a"))); + doc.setProp(new RegoString("list"), new RegoArray(List.of(RegoInt32.of(1)))); + + RegoValue r = + builtins.patch( + ctx, + new RegoValue[] { + doc, + new RegoArray( + List.of(op("op", "remove", "path", "list/99999999999999999999"))) + }); + + assertSame(RegoUndefined.INSTANCE, r); + } + + @Test + @Timeout(10) + void addsToArrayNestedInSet() { + // doc := {[1]} — a set whose single member is the array [1] + RegoObject wrapper = new RegoObject(); + wrapper.setProp(new RegoString("x"), set(new RegoArray(List.of(RegoInt32.of(1))))); + + RegoObject patchOp = new RegoObject(); + patchOp.setProp(new RegoString("op"), new RegoString("add")); + patchOp.setProp( + new RegoString("path"), + new RegoArray(List.of(new RegoArray(List.of(RegoInt32.of(1))), RegoInt32.of(1)))); + patchOp.setProp(new RegoString("value"), RegoInt32.of(2)); + + RegoValue r = + builtins.patch( + ctx, new RegoValue[] {wrapper.getProperty("x"), new RegoArray(List.of(patchOp))}); + + assertEquals(set(new RegoArray(List.of(RegoInt32.of(1), RegoInt32.of(2)))), r); + } +} diff --git a/opa-evaluator/build.gradle.kts b/opa-evaluator/build.gradle.kts index 5e0911be..cac1138e 100644 --- a/opa-evaluator/build.gradle.kts +++ b/opa-evaluator/build.gradle.kts @@ -26,6 +26,10 @@ dependencies { tasks.test { useJUnitPlatform() + // The jsonpatch/json_patch_tests fixture evaluates the whole upstream JSON Patch spec suite + // inside one policy, building every case into comprehensions. That exceeds Gradle's default + // 512m test heap; 1g is enough today, so this leaves some headroom. + maxHeapSize = "2g" } // Apply a specific Java toolchain to ease working on different environments. 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 3a07463b..285359d1 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 @@ -90,7 +90,10 @@ public class ComplianceTest { "eval_type_error: strings.any_suffix_match: eval_type_error: operand 2 must be one of {string, set, array} but got number", "operand 0 must be array of strings but got array containing number" ), - "strings/any_prefix_match/type_error_strict", List.of("eval_type_error: strings.any_prefix_match: operand 0 must be array of strings but got array containing number") + "strings/any_prefix_match/type_error_strict", List.of("eval_type_error: strings.any_prefix_match: operand 0 must be array of strings but got array containing number"), + // SnakeYAML phrases the unterminated-flow-sequence diagnostic differently from Go's + // yaml.v2 ("did not find expected ',' or ']'"). The line number and shape match. + "jsonbuiltins/yaml unmarshal error", List.of("yaml: line 1: expected ',' or ']', but got ") ); static { diff --git a/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt b/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt index 9acde2d1..c86bc163 100644 --- a/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt +++ b/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt @@ -11,28 +11,11 @@ # 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 file is the fix, after which the parity failures the -# fixtures then expose need triage. +# and they are unreachable for consumers. Un-commenting the file is the fix, +# after which the parity failures the fixtures then expose need triage. # -# opa-builtins-crypto and opa-builtins-semver are done — registering them -# needed no parity fixes. The three below do: registering all of them at once -# fails 32 compliance cases (json 16, net 13, regex 3) and exhausts the default -# test heap, so they are being taken one module at a time. - -# 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 +# crypto and semver needed no parity fixes, json needed 16. The two modules +# below are being taken one at a time: net (13 failures) and regex (3). # opa-builtins-net net.cidr_contains