Skip to content

Commit 26042fc

Browse files
thiagohoraclaude
andcommitted
[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>
1 parent 0d3b353 commit 26042fc

3 files changed

Lines changed: 30 additions & 25 deletions

File tree

apps/opik-backend/src/main/java/com/comet/opik/domain/IdGenerator.java

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,16 +111,19 @@ public Mono<UUID> validateIdAsync(@NonNull UUID id, String resource) {
111111
}
112112

113113
@Override
114-
115-
public void validateIdForUpdate(@NonNull UUID id, String resource, String workspaceId) {
114+
public void validateIdNotInFuture(@NonNull UUID id, String resource) {
116115
IdGenerator.validateVersion(id, resource);
117-
uuidV7TimestampValidator.validateNotInFuture(id, resource, workspaceId);
116+
// Referenced/foreign ids are validated where the request-scoped workspace is not threaded
117+
// (e.g. the synchronous fail-fast batch pass), so the audit metric falls back to UNKNOWN here;
118+
// the batch's own ids still carry the workspace via validateId.
119+
uuidV7TimestampValidator.validateNotInFuture(id, resource, ErrorMetricsResolver.UNKNOWN);
118120
}
119121

120122
@Override
121-
public Mono<UUID> validateIdForUpdateAsync(@NonNull UUID id, String resource) {
123+
public Mono<UUID> validateIdNotInFutureAsync(@NonNull UUID id, String resource) {
122124
return Mono.deferContextual(ctx -> Mono.fromCallable(() -> {
123-
validateIdNotInFuture(id, resource, workspaceId(ctx));
125+
IdGenerator.validateVersion(id, resource);
126+
uuidV7TimestampValidator.validateNotInFuture(id, resource, workspaceId(ctx));
124127
return id;
125128
}));
126129
}

apps/opik-backend/src/main/java/com/comet/opik/domain/SpanService.java

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -375,15 +375,6 @@ public Mono<Long> create(@NonNull SpanBatch batch) {
375375

376376
List<Span> dedupedSpans = dedupSpans(batch.spans());
377377

378-
// Fail fast on invalid ids BEFORE any side effect below (auto-stripped attachment deletion, project
379-
// creation), so a rejected batch never mutates state.
380-
dedupedSpans.forEach(span -> {
381-
if (span.id() != null) {
382-
idGenerator.validateId(span.id(), SPAN_KEY);
383-
}
384-
validateSpanReferences(span.traceId(), span.parentSpanId());
385-
});
386-
387378
List<String> projectNames = dedupedSpans
388379
.stream()
389380
.map(Span::projectName)
@@ -401,7 +392,19 @@ public Mono<Long> create(@NonNull SpanBatch batch) {
401392
.filter(Objects::nonNull)
402393
.collect(Collectors.toSet());
403394

404-
return attachmentService.deleteAutoStrippedAttachments(SPAN, spanIds)
395+
// Fail fast on invalid ids BEFORE any side effect below (auto-stripped attachment deletion, project
396+
// creation), so a rejected batch never mutates state. Runs inside deferContextual so the audit
397+
// metric can attribute the batch's own ids to the request workspace.
398+
return Mono.deferContextual(validationCtx -> {
399+
String validationWorkspaceId = validationCtx.get(RequestContext.WORKSPACE_ID);
400+
dedupedSpans.forEach(span -> {
401+
if (span.id() != null) {
402+
idGenerator.validateId(span.id(), SPAN_KEY, validationWorkspaceId);
403+
}
404+
validateSpanReferences(span.traceId(), span.parentSpanId());
405+
});
406+
return attachmentService.deleteAutoStrippedAttachments(SPAN, spanIds);
407+
})
405408
.then(Mono.deferContextual(ctx -> {
406409
String workspaceId = ctx.get(RequestContext.WORKSPACE_ID);
407410
String workspaceName = ctx.getOrDefault(RequestContext.WORKSPACE_NAME, "");
@@ -410,7 +413,7 @@ public Mono<Long> create(@NonNull SpanBatch batch) {
410413
Mono<List<Span>> resolveProjects = Flux.fromIterable(projectNames)
411414
.flatMap(projectService::getOrCreate)
412415
.collectList()
413-
.map(projects -> bindSpanToProjectAndId(dedupedSpans, projects, workspaceId));
416+
.map(projects -> bindSpanToProjectAndId(dedupedSpans, projects));
414417

415418
return resolveProjects
416419
.flatMap(this::stripAttachmentsFromSpanBatch)
@@ -462,7 +465,7 @@ private void validateSpanReferences(UUID traceId, UUID parentSpanId) {
462465
idGenerator.validateIdNotInFutureIfPresent(parentSpanId, SPAN_PARENT_KEY);
463466
}
464467

465-
private List<Span> bindSpanToProjectAndId(List<Span> spans, List<Project> projects, String workspaceId) {
468+
private List<Span> bindSpanToProjectAndId(List<Span> spans, List<Project> projects) {
466469
Map<String, Project> projectPerName = projects.stream()
467470
.collect(Collectors.toMap(
468471
WorkspaceUtils::stripProjectName,
@@ -482,10 +485,9 @@ private List<Span> bindSpanToProjectAndId(List<Span> spans, List<Project> projec
482485
throw new IllegalStateException("Project not found: %s".formatted(span.projectName()));
483486
}
484487

488+
// Ids are already validated up-front in create(SpanBatch); generated ids are inherently valid.
485489
UUID id = span.id() == null ? idGenerator.generateId() : span.id();
486490

487-
idGenerator.validateId(id, SPAN_KEY, workspaceId);
488-
489491
return span.toBuilder().id(id).projectId(project.id()).build();
490492
})
491493
.toList();

apps/opik-backend/src/test/java/com/comet/opik/domain/IdGeneratorAsyncValidationTest.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717

1818
/**
1919
* Covers the reactive validation paths of {@link IdGenerator} (which the sync {@link
20-
* UuidV7TimestampValidatorTest} does not): {@code validateIdAsync} / {@code validateIdForUpdateAsync}
20+
* UuidV7TimestampValidatorTest} does not): {@code validateIdAsync} / {@code validateIdNotInFutureAsync}
2121
* resolve {@code workspaceId} from the Reactor context via {@code deferContextual}, falling back to
2222
* {@link com.comet.opik.infrastructure.metrics.ErrorMetricsResolver#UNKNOWN} when the context has no
2323
* {@link RequestContext#WORKSPACE_ID}. Both cases must preserve the accept/reject behavior.
@@ -76,14 +76,14 @@ void rejectAsyncRejectsFutureWithoutContext() {
7676
}
7777

7878
@Test
79-
@DisplayName("reject: validateIdForUpdateAsync rejects only too-far-future, accepts old ids")
79+
@DisplayName("reject: validateIdNotInFutureAsync rejects only too-far-future, accepts old ids")
8080
void rejectForUpdateAsync() {
8181
var oldId = idAt(Instant.now().minus(48, ChronoUnit.HOURS));
82-
StepVerifier.create(rejectGenerator.validateIdForUpdateAsync(oldId, RESOURCE).contextWrite(withWorkspace()))
82+
StepVerifier.create(rejectGenerator.validateIdNotInFutureAsync(oldId, RESOURCE).contextWrite(withWorkspace()))
8383
.expectNext(oldId)
8484
.verifyComplete();
8585
StepVerifier
86-
.create(rejectGenerator.validateIdForUpdateAsync(tooFarFutureId(), RESOURCE)
86+
.create(rejectGenerator.validateIdNotInFutureAsync(tooFarFutureId(), RESOURCE)
8787
.contextWrite(withWorkspace()))
8888
.expectError(InvalidUUIDException.class)
8989
.verify();
@@ -104,10 +104,10 @@ void auditAsyncNeverRejects() {
104104
}
105105

106106
@Test
107-
@DisplayName("audit: validateIdForUpdateAsync passes a too-far-future id through")
107+
@DisplayName("audit: validateIdNotInFutureAsync passes a too-far-future id through")
108108
void auditForUpdateAsyncNeverRejects() {
109109
var id = tooFarFutureId();
110-
StepVerifier.create(Mono.defer(() -> auditGenerator.validateIdForUpdateAsync(id, RESOURCE)))
110+
StepVerifier.create(Mono.defer(() -> auditGenerator.validateIdNotInFutureAsync(id, RESOURCE)))
111111
.expectNext(id)
112112
.verifyComplete();
113113
}

0 commit comments

Comments
 (0)