Skip to content

Fix missing/incorrect @OpaBuiltin annotations for union and object.union#157

Merged
sspaink merged 1 commit into
open-policy-agent:mainfrom
ML-dev-crypto:fix-137-union-object-union-metadata
Jul 16, 2026
Merged

Fix missing/incorrect @OpaBuiltin annotations for union and object.union#157
sspaink merged 1 commit into
open-policy-agent:mainfrom
ML-dev-crypto:fix-137-union-object-union-metadata

Conversation

@ML-dev-crypto

@ML-dev-crypto ML-dev-crypto commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Both union and object.union are already fully implemented and functionally
correct registered in their respective builtins() maps and dispatched
properly at runtime, so calling either from a Rego policy already works today.
The actual gap is in their @OpaBuiltin metadata annotations, which are used
to generate capabilities.json and docs. This PR fixes that metadata rather
than reimplementing the builtins, so the diff is smaller than the issue's
"missing builtins" framing might suggest.

Issues

#137

Changes

  • Added the missing @OpaBuiltin annotation to union (SetBuiltins.java).
    It previously had none, so it was falling back to a minimal
    auto-generated capabilities entry with no description or arg types.
  • Replaced object.union's @OpaBuiltin annotation (ObjectBuiltins.java).
    It was copy-pasted from object.keys wrong name, wrong single-arg
    signature, wrong description. This also meant two methods in the same
    file both declared name = "object.keys", an ambiguous collision since
    annotation-scanning order over declared methods isn't guaranteed by the
    JVM. Fixing the name resolves the collision as a side effect.
  • Swapped a raw (RegoSet) args[0] cast for the existing getArg helper
    in union, so a bad input type throws a TypeError instead of a raw
    ClassCastException consistent with intersection/and/or in the
    same file.

Testing

  • ./gradlew :opa-evaluator:test — full suite passes, including all
    compliance cases under TestData/v1/union/ and TestData/v1/objectunion/.
  • ./gradlew checkstyleMain checkstyleTest pmdMain pmdTest — clean, no
    violations in either changed file.
  • 11 pre-existing, unrelated failures were present both with and without
    this change (verified via git stash): 9 in FileSystemBundleLoaderTest
    (Windows-specific InvalidPathException, this repo's CI only runs
    ubuntu-latest) and 2 in GoTimeLayoutConverterTest (unrelated to
    bundle/builtin code).

@ML-dev-crypto
ML-dev-crypto requested a review from a team as a code owner July 14, 2026 10:40

@sspaink sspaink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata-only PR — verified against OPA's capabilities.json / v1/ast/builtins.go and by running this repo's CapabilitiesGenerator on this branch and diffing the emitted JSON for both builtins. The collision fix is correct (object.keys keeps its own annotation on the keys method) and the getArg swap is fine. Two annotation-shape issues remain; both confirmed against the actually-generated capabilities output.

@ML-dev-crypto
ML-dev-crypto force-pushed the fix-137-union-object-union-metadata branch from d035a74 to eacd5b8 Compare July 15, 2026 18:00
@ML-dev-crypto

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review! Good catch on both points. I've updated the object.union result annotation to use type = "object" with the appropriate dynamic key/value types, and simplified the union argument annotation to @OpaDynamic(type = "set") to match the existing convention.

@sspaink sspaink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sspaink
sspaink merged commit 0c73094 into open-policy-agent:main Jul 16, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants