[OPIK-7352] [BE] reject non-UUIDv7 referenced ids on ingest - #7553
Merged
Conversation
Retention sweeps spans/traces via an id-range predicate that assumes the referenced id is a time-ordered UUIDv7 (SpanDAO orders by the trace_id range; TraceDAO by the id range). Ingest validated an entity's own id but not the ids it references, so a client could store a span with a non-UUIDv7 trace_id and have those rows ordered lexicographically instead of chronologically — deleted or retained against the wrong window. Validate referenced ids on ingest with a not-in-future policy (version 7 required, future-dated rejected, past allowed — referencing older entities such as late spans on old traces is legitimate). Renames validateIdForUpdate -> validateIdNotInFuture and reuses it for both the update paths and referenced ids; adds a public sync overload. Covered paths: span traceId/parentSpanId (create, batch, update, batch-update, and the experiment-items bulk path via batch create); single and batch feedback-score entity ids; trace/span/thread comment entity ids; attachment entity ids; dataset-item trace/span ids (batch, patch, from-traces, from-spans); annotation-queue item ids. Version enforcement is always on (mirrors own-id validation); the not-in-future check is gated by the existing uuidValidation kill-switch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends referenced-id validation to config-entity references (verify-then- enforce: confirmed zero non-UUIDv7 ids across prod projects, datasets, dataset_versions, prompts, prompt_versions, alerts, dashboards, webhooks, automation_rules, and guardrails, so enforcement rejects no existing data). Adds null-safe validateIdNotInFutureIfPresent(Async) helpers and validates: projectId across span/trace update + batch-update, feedback scores (single + batch), guardrails, assertion results, annotation queues, experiments, optimizations, alerts, automation-rule evaluators, prompts + prompt versions, dashboards, and thread open/close; feedback/guardrail sourceQueueId; guardrail secondaryId; experiment optimizationId + datasetVersionId; dataset-item datasetId + copy-from ids. Upgrades the guardrail/assertion referenced entityId from version-only to the shared not-in-future policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 40 skipped (no matching files changed)
|
Contributor
Backend Tests - Integration Group 6184 tests +9 181 ✅ +9 3m 22s ⏱️ -6s Results for commit 7df5573. ± Comparison against base commit 4dc6b0c. This pull request removes 10 and adds 19 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 12297 tests - 4 297 ✅ - 4 6m 2s ⏱️ -1s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 51 and adds 47 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 13325 tests - 17 323 ✅ - 17 4m 37s ⏱️ - 1m 1s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 39 and adds 22 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 8370 tests +1 368 ✅ - 1 5m 16s ⏱️ - 2m 33s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 70 and adds 71 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 16212 tests +13 212 ✅ +13 5m 34s ⏱️ - 1m 51s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 17 and adds 30 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 71 584 tests - 7 1 584 ✅ - 7 6m 57s ⏱️ -20s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 19 and adds 12 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Backend Tests - Integration Group 14650 tests ±0 650 ✅ ±0 7m 5s ⏱️ -10s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 41 and adds 41 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Backend Tests - Integration Group 11 39 files + 1 39 suites +1 7m 56s ⏱️ -33s Results for commit 7df5573. ± Comparison against base commit 4dc6b0c. This pull request removes 15 and adds 5 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 9520 tests +16 519 ✅ +16 10m 25s ⏱️ -29s Results for commit 7df5573. ± Comparison against base commit 4dc6b0c. This pull request removes 17 and adds 33 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Contributor
Backend Tests - Integration Group 41 823 tests +21 1 823 ✅ +21 9m 18s ⏱️ +2s Results for commit f1e48d3. ± Comparison against base commit 4dc6b0c. This pull request removes 49 and adds 70 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Refine scope to the rule: validate a referenced id iff the operation persists it to the entity's table. Drop validation where the id is not written by the operation: - Thread open/close: projectId only resolves/locates an already-existing thread (its project_id was written at trace ingestion), so nothing is persisted here. - Dataset item batch copy-from ids: only a read source for carry-forward rows, never stored as version lineage. All create/update paths that write the referenced id to a column keep their validation (spans trace_id/parent_span_id, feedback/comment/attachment/ guardrail/assertion entity ids, dataset-item trace/span + dataset ids, annotation-queue items, and projectId/optimizationId/datasetVersionId/ sourceQueueId on the rows that store them). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Existing tests assigned random v4 UUIDs to referenced ids that are now validated as UUIDv7 (guardrail secondaryId, experiment optimizationId / datasetVersionId). Switch that test data to generator.generate() so the tests exercise the intended paths (the invalid-version conflict test now uses a v7-but-nonexistent id, preserving its 409 expectation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d; fix test data Address reviewer + CI findings: - Remove projectId validation in GuardrailsService and AssertionResultService: both resolve the project by name and overwrite projectId with the resolved project.id() before persistence, so the client-supplied projectId is never stored — validating it only breaks clients that send arbitrary project ids. - Fix remaining test data to use current UUIDv7 (never v4 or future-dated) for validated referenced ids: span traceId, dataset-item trace/span ids, annotation-queue item ids, feedback sourceQueueId, alert projectId, automation-rule projectIds, experiment optimizationId. 404/409 negative tests use a v7-but-nonexistent id to preserve their intent. - Strengthen createWithOldTraceIdSucceeds to round-trip and assert the old traceId is persisted verbatim; add a batch non-v7 parentSpanId rejection test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…checks Addresses reviewer findings: - create(SpanBatch) validated traceId/parentSpanId inside bindSpanToProjectAndId, which runs AFTER deleteAutoStrippedAttachments + project getOrCreate — so a bad batch could delete attachments / create projects before failing 400. Move the id checks up front (before any side effect) so a rejected batch never mutates state. - Extract a shared validateSpanReferences(traceId, parentSpanId) helper used by every span write path (single/batch create, single/batch update) so the reference-id rules can't drift between them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
andrescrz
previously approved these changes
Jul 22, 2026
andrescrz
left a comment
andrescrz
left a comment
Member
There was a problem hiding this comment.
Some comments, but no blockers.
…er fixes Address reviewer feedback (andrescrz): - Drop UUIDv7 validation on referenced config ids that are already existence-checked (so a bad id fails with the proper 404/409, not 400): projectId on span/trace update, prompt, experiment, optimization, dashboard (all go through validateProjectIdExists / resolveProjectIdOrCreate / get); feedback-batch projectId (overwritten by the name-resolved project); datasetVersionId (FK-checked) and datasetId (findById). - Keep it where the ref is persisted WITHOUT an existence check, so a UUIDv4 orphan can't be ingested: alert projectId, annotation-queue projectId, automation-rule projectIds, experiment optimizationId, feedback sourceQueueId. - Move validateIdNotInFutureIfPresent(Async) bodies out of the IdGenerator interface into IdGeneratorImpl. - Tests: route added generators through TestIdGeneratorFactory (static final) instead of local TimeBasedEpochGenerator fields; parameterize the span invalid-reference tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
andrescrz
approved these changes
Jul 22, 2026
thiagohora
added a commit
that referenced
this pull request
Jul 22, 2026
…d validation Resolve the semantic overlap after merging main (#7553): unify IdGenerator on the merged API — implement validateIdNotInFuture / validateIdNotInFutureAsync (and the IfPresent variants) and drop the superseded validateIdForUpdate*. Own-id validateId stays workspace-tagged for the audit metric; referenced-id validateIdNotInFuture falls back to UNKNOWN on the sync path and resolves the workspace from the reactive context on the async path. Keep #7553's fail-fast (validate before side effects) for span batches, now run inside deferContextual so the batch's own ids attribute to the request workspace; bindSpanToProjectAndId no longer re-validates. Trace batches keep validating in bindTraceToProjectAndId with the workspace threaded through. Update the async test to the renamed methods. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
added a commit
that referenced
this pull request
Jul 22, 2026
bindTraceToProjectAndId validated own ids only after deleteAutoStrippedAttachments and project getOrCreate, so a bad trace id in a batch still mutated state before failing (unlike spans, which #7553 validates fail-fast). Hoist trace own-id validation into a leading deferContextual that runs before any side effect and attributes the audit metric to the request workspace, and stop re-validating in bindTraceToProjectAndId — mirroring create(SpanBatch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
added a commit
that referenced
this pull request
Jul 24, 2026
…on validation (#7573) * [OPIK-7402] [BE] Add audit (shadow / log-only) mode to UUIDv7 ingestion validation Adds a third `auditOnly` state to uuidValidation on top of the `enabled` kill-switch. When enabled=true and auditOnly=true, out-of-window UUIDv7 ids are counted and logged but NOT rejected, so clients emitting them surface in real time without breaking ingestion. Effective mode: enabled=false -> disabled; enabled=true & auditOnly=true -> audit; enabled=true & auditOnly=false -> reject (HTTP 400, unchanged). - New UuidValidationMetrics records opik.ingestion.uuid_v7.rejected in audit mode, tagged mode=audit + reason + resource + workspace_id (shares the instrument with InvalidUUIDExceptionMapper's reject-path counter). - Validator gains resource/workspaceId params; audit branch emits + logs instead of throwing. Scope is the window check; the always-on NOT_V7 version check is unchanged. - workspaceId is read from the reactive context on the async paths and threaded through the batch bind on the sync path. - Unit test covers disabled/reject/audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [OPIK-7402] [BE] Tag the reject path with mode=reject on the shared counter Address review: InvalidUUIDExceptionMapper recorded opik.ingestion.uuid_v7.rejected without a mode label, so the shared counter had series with and without mode and a `mode=reject` query would miss enforced rejections. Add mode=reject on the reject path (workspace_id stays audit-only, as the mapper has no threaded workspace), and align the UuidValidationMetrics contract javadoc: mode is always present; workspace_id is audit-only; the reject path additionally carries http_route. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [OPIK-7402] [BE] Address review: async test coverage, log prefix, trimmed javadoc - Add IdGeneratorAsyncValidationTest covering validateIdAsync / validateIdForUpdateAsync reject+audit behavior, with and without RequestContext.WORKSPACE_ID in the reactive context (the deferContextual workspace lookup + UNKNOWN fallback). - Keep a fixed, searchable prefix on the audit log line and move the variable fields to the end. - Trim the IdGenerator#validateId javadoc to validation semantics + InvalidUUIDException; keep workspaceId threading (the sync/batch path is the primary ingestion route where per-workspace attribution is the goal). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [OPIK-7402] [BE] Reconcile audit mode with merged OPIK-7352 foreign-id validation Resolve the semantic overlap after merging main (#7553): unify IdGenerator on the merged API — implement validateIdNotInFuture / validateIdNotInFutureAsync (and the IfPresent variants) and drop the superseded validateIdForUpdate*. Own-id validateId stays workspace-tagged for the audit metric; referenced-id validateIdNotInFuture falls back to UNKNOWN on the sync path and resolves the workspace from the reactive context on the async path. Keep #7553's fail-fast (validate before side effects) for span batches, now run inside deferContextual so the batch's own ids attribute to the request workspace; bindSpanToProjectAndId no longer re-validates. Trace batches keep validating in bindTraceToProjectAndId with the workspace threaded through. Update the async test to the renamed methods. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [OPIK-7402] [BE] Make trace batch validation symmetric with spans bindTraceToProjectAndId validated own ids only after deleteAutoStrippedAttachments and project getOrCreate, so a bad trace id in a batch still mutated state before failing (unlike spans, which #7553 validates fail-fast). Hoist trace own-id validation into a leading deferContextual that runs before any side effect and attributes the audit metric to the request workspace, and stop re-validating in bindTraceToProjectAndId — mirroring create(SpanBatch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [OPIK-7402] [BE] Address review: centralize counter, simplify async, tidy nits - Centralize the opik.ingestion.uuid_v7.rejected counter and its label constants in UuidValidationMetrics; InvalidUUIDExceptionMapper now injects it and delegates via recordReject(reason, httpRoute), dropping its duplicate counter/constants. - Drop the redundant nested Mono.fromCallable inside deferContextual on the async validation paths (validate has no blocking work and there is no subscribeOn, so it ran on the subscribing thread either way; deferContextual already maps a thrown exception to onError). - Let resource fall back to unknown like workspaceId (relax @nonnull); the metric recorder defaults blank values. - Test: make the id generators static final and rename to constant case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [OPIK-7402] [BE] Review: centralize IdGenerator + workspace overloads; factor metric assembly - IdGenerator: single core validateIdNotInFuture(id, resource, workspaceId); the 2-arg form and the async form delegate to it (no more duplicated version+window logic), and a workspace-parameterized overload is offered for callers that carry the request workspace. Same for validateIdNotInFutureIfPresent. The many config-entity callers keep the 2-arg form, defaulting to unknown by choice. - UuidValidationMetrics: factor the shared counter/mode/reason assembly into a private record(...) helper; recordAudit/recordReject only supply their path-specific tags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Retention sweeps spans/traces via an id-range predicate that assumes the referenced id is a time-ordered UUIDv7 (
SpanDAOorders spans by thetrace_idrange;TraceDAOby theidrange). Ingest validated an entity's own id but not the ids it references, so a client could store e.g. a span with a non-UUIDv7trace_idand have those rows ordered lexicographically instead of chronologically — deleted or retained against the wrong window. This validates referenced ids on ingest.projectIdon thread open/close, whoseproject_idwas written at trace ingestion) or as a read source (dataset copy-from ids) are not validated.validateIdForUpdate→validateIdNotInFutureand reuses it for both the update paths and referenced ids; adds null-safevalidateIdNotInFutureIfPresent(Async)helpers.traceId/parentSpanId(create, batch, update, batch-update, experiment-items bulk); feedback-score entity ids (single + batch) +sourceQueueId; trace/span/thread comment entity ids; attachment entity ids; guardrailentityId+secondaryId; assertionentityId; dataset-itemtraceId/spanId+ batchdatasetId; annotation-queue item ids; experimentprojectId/optimizationId/datasetVersionId; andprojectIdon span/trace update + batch-update, optimization, alert, dashboard, prompt (+ version), annotation-queue, guardrail, assertion, feedback batch, automation-rule evaluators.uuidValidationkill-switch (off in prod), so this ships dormant beyond the version check.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn -o compile/test-compile/spotless:applyclean.SpansResourceTestcases (single and batch create): reject non-v7traceId(400), reject future-datedtraceId(400), reject non-v7parentSpanId(400), and accept an old (past) v7traceId(spans on old traces are valid). These exercise the sharedvalidateIdNotInFuturemechanism used by every path here.Documentation
No user-facing docs. The referenced-id validation policy (version 7 + not-in-future, past allowed) is documented in the
IdGeneratorjavadoc.