Implement UUID builtins - #155
Conversation
Signed-off-by: Aayush Tiwari <aayushtiwari1001@gmail.com>
sspaink
left a comment
sspaink
left a comment
There was a problem hiding this comment.
Thank you for the work on this!
| return buffer.array(); | ||
| } | ||
|
|
||
| private String deterministicVersion4Uuid(String key) { |
There was a problem hiding this comment.
SHA-256(key) makes this a hash of the key, not a UUID generator. Upstream internal/uuid.New reads 16 random bytes from bctx.Seed and forces the version/variant bits; the key is only a per-query cache key (bctx.Cache), so the contract is "consistent throughout a query evaluation" — not identical in every query, process and machine, and not derivable from the key by anyone who can guess it.
Closest parity with upstream is to keep this method's shape and swap the digest for a static final SecureRandom:
byte[] bytes = new byte[16];
RANDOM.nextBytes(bytes);
bytes[6] = (byte) ((bytes[6] & 0x0f) | 0x40);
bytes[8] = (byte) ((bytes[8] & 0x3f) | 0x80);That matches internal/uuid.New's bit-fiddling exactly, and UUID.toString() produces the same lowercase canonical form as its fmt.Sprintf("%x-%x-%x-%x-%x", ...). See the note on line 80 for the caching half of this.
| result = @OpaType(type = "string", name = "uuid", description = "RFC 4122 UUID"), | ||
| nondeterministic = true) | ||
| public RegoString rfc4122(EvaluationContext ctx, RegoValue[] args) { | ||
| if (ctx != null && ctx.getNdBuiltinCache() != null) { |
There was a problem hiding this comment.
Upstream caches uuid in bctx.Cache — a plain map built fresh per query (v1/topdown/query.go:424), no bound, no eviction — so k pins the value inside one evaluation and nothing beyond it. NdBuiltinCache is inter-query-shaped (LRU cap + stale-entry eviction), so once it's wired up the same key would return the same UUID to every request until an eviction, where upstream mints a fresh one per query. Note this branch is inert today, so the same-key stability actually comes from the digest on line 206.
A small per-evaluation map would match upstream; alternatively split rfc4122 into a follow-up and merge uuid.parse here, which needs none of this.
| String input = getArg(args, 0, RegoString.class).getValue(); | ||
| UUID uuid = parseUuid(input); | ||
| if (uuid == null) { | ||
| return RegoUndefined.INSTANCE; |
There was a problem hiding this comment.
Builtins signal undefined by returning null — that's what invokeBuiltin checks (ir/Evaluator.java:918). A RegoUndefined is stored as an ordinary value instead, so p := uuid.parse("bad") comes out as {"p": null} rather than undefined.
| return RegoUndefined.INSTANCE; | |
| return null; |
| } | ||
| } | ||
|
|
||
| private String normalize(String input) { |
There was a problem hiding this comment.
UUID.fromString is lenient about field widths, so this defines results where upstream is undefined — verified UUID.fromString("1-1-1-1-1") → 00000001-0001-0001-0001-000000000001, which google/uuid.Parse (used upstream via internal/uuid) rejects. Worth switching on length like upstream (36 / 38 / 45 / 32) and regex-checking the canonical form before fromString.
|
|
||
| if (version == 2) { | ||
| result.setProperty("domain", new RegoString(domain(bytes[9]))); | ||
| result.setProperty("id", RegoInt32.of(ByteBuffer.wrap(bytes, 0, 4).getInt())); |
There was a problem hiding this comment.
id is unsigned upstream (int(u.ID()) over a uint32), but ByteBuffer.getInt() is signed — ffffffff-48b9-21ee-b200-325096b39f47 gives -1 here vs 4294967295 in OPA.
| result.setProperty("id", RegoInt32.of(ByteBuffer.wrap(bytes, 0, 4).getInt())); | |
| result.setProperty( | |
| "id", new RegoBigInt(Integer.toUnsignedLong(ByteBuffer.wrap(bytes, 0, 4).getInt()))); |
| case 2: | ||
| return "Org"; | ||
| default: | ||
| return "Invalid"; |
There was a problem hiding this comment.
Upstream's internal/uuid wraps google/uuid, and its Domain.String() returns "Domain" + N for values outside 0–2, not "Invalid".
| return "Invalid"; | |
| return "Domain" + Byte.toUnsignedInt(value); |
|
|
||
| private UUID parseUuid(String input) { | ||
| String normalized = normalize(input); | ||
| if (normalized == null) { |
There was a problem hiding this comment.
Dead branch — normalize never returns null.
| description = "Parses a UUID string into its RFC 4122 metadata.", | ||
| categories = {"uuid"}, | ||
| args = {@OpaType(type = "string", name = "uuid", description = "UUID string to parse")}, | ||
| result = @OpaType(type = "object", name = "result", description = "parsed UUID metadata")) |
There was a problem hiding this comment.
Upstream declares uuid.parse's result as types.NewObject(nil, types.NewDynamicProperty(types.S, types.A)), so this should carry @OpaDynamic(keyType = "string", valueType = "any") — as OpaBuiltins.runtime does — rather than an object with no properties.
Signed-off-by: Aayush Tiwari <aayushtiwari1001@gmail.com>
2 participants
Summary
Implements the UUID builtins requested in #134:
uuid.parse, including RFC 4122 metadata for version 1 and 2 UUIDsuuid.rfc4122, including stable repeated values for the same key during evaluationValidation
.\gradlew.bat --no-daemon --console=plain :opa-evaluator:test --tests io.github.open_policy_agent.opa.ast.builtin.impls.UUIDBuiltinsTest.\gradlew.bat --no-daemon --console=plain :opa-evaluator:test --tests io.github.open_policy_agent.opa.ast.builtin.impls.UUIDBuiltinsTest --tests io.github.open_policy_agent.opa.ast.builtin.CapabilitiesGeneratorTest :opa-evaluator:checkstyleMain :opa-evaluator:checkstyleTestI also tried
:opa-evaluator:check; it did not complete cleanly because of unrelated existing local failures inGoTimeLayoutConverterTestandFileSystemBundleLoaderTest.Fixes #134.