[OPIK-5599] [BE] Fix experiment items not displayed after evaluation suite deletion by thiagohora · Pull Request #6053 · comet-ml/opik

thiagohora · 2026-04-02T10:41:16Z

Details

When an evaluation suite (dataset with type = 'evaluation_suite') was deleted, any subsequent request to retrieve dataset items with experiment items for that suite returned 404. This happened because DatasetItemService.getItems() unconditionally called datasetService.findById(), which throws NotFoundException when the dataset no longer exists.

Root causes fixed:

DatasetItemService.getItems(): Removed the blocking findById() guard and replaced it with a reactive Mono.fromCallable wrapping a new verifyVisibilityIfExists() method. The new method only throws 404 if the dataset exists but fails the visibility check — it silently succeeds when the dataset is already deleted, allowing the request to proceed.
DatasetItemService.getItemsWithExperimentItems(): When getFallbackVersionId() returns empty (because all MySQL dataset versions were deleted alongside the suite), the code now falls back to the legacy dao.getItems() query so that experiment items stored in ClickHouse are still returned.
DatasetService: Added verifyVisibilityIfExists(UUID id, String workspaceId, Visibility visibility) — a visibility-only check that skips deleted datasets.

Change checklist

User facing
Documentation update

Issues

Resolves #
OPIK-5599

AI-WATERMARK

AI-WATERMARK: yes

Tools: Claude Code
Model(s): claude-sonnet-4-6
Scope: Bug fix implementation and tests
Human verification: Reviewed and tested

Testing

Regression test added: DatasetsResourceTest#find__whenEvaluationSuiteIsDeleted__thenExperimentItemsShouldStillBeRetrievable

Test scenario:

Create an evaluation suite
Create a trace and a dataset item linked to it
Create an experiment + experiment item linking the dataset item to the trace
Delete the evaluation suite
Assert that GET /datasets/{id}/items/experiments still returns the experiment item (HTTP 200)

mvn test -Dtest="DatasetsResourceTest#find__whenEvaluationSuiteIsDeleted__thenExperimentItemsShouldStillBeRetrievable"

All 6 tests in FindDatasetItemsWithExperimentItems pass.

Note on returned data: After deletion the dataset_items ClickHouse data is gone (items were stored in dataset_item_versions via the versioning feature). The legacy fallback query returns synthetic items built from experiment_items only, so traceId, spanId, source, description, evaluators, and executionPolicy are null on the returned dataset item. The experiment items themselves remain fully populated.

Documentation

No documentation changes required — this is a bug fix restoring previously expected behaviour.

…suite deletion When an evaluation suite (dataset) was deleted, subsequent requests for dataset items with experiment items returned 404 because DatasetItemService unconditionally called datasetService.findById(), which throws when the dataset no longer exists. Changes: - DatasetService: add verifyVisibilityIfExists() — only throws 404 when the dataset exists but fails the visibility check; silently succeeds when deleted - DatasetItemService.getItems(): replace the blocking findById() guard with a reactive Mono.fromCallable wrapping verifyVisibilityIfExists(), allowing requests to proceed after deletion - DatasetItemService.getItemsWithExperimentItems(): when getFallbackVersionId() returns empty (all versions deleted), fall back to the legacy dao.getItems() so experiment items are still returned from ClickHouse - DatasetsResourceTest: add regression test covering the deleted-suite scenario

github-actions · 2026-04-02T10:42:34Z

🔄 Test environment deployment process has started

Phase 1: Deploying base version 1.10.59-4794 (from main branch) if environment doesn't exist
Phase 2: Building new images from PR branch thiaghora/OPIK-5599-fix-experiment-items-display-after-suite-deletion
Phase 3: Will deploy newly built version after build completes

You can monitor the progress here.

github-actions · 2026-04-02T10:45:52Z

Backend Tests - Integration Group 15

25 files 25 suites 4m 53s ⏱️
243 tests 242 ✅ 0 💤 0 ❌ 1 🔥
242 runs 242 ✅ 0 💤 0 ❌

For more details on these errors, see this check.

Results for commit b37bb47.

♻️ This comment has been updated with latest results.

baz-reviewer · 2026-04-02T10:47:48Z

        return Mono.deferContextual(ctx -> {
            String workspaceId = ctx.get(RequestContext.WORKSPACE_ID);
+            Visibility visibility = ctx.get(RequestContext.VISIBILITY);


getItems reads RequestContext.VISIBILITY from the Reactor context but callers like DatasetExpansionService don't set it — should we default to a visibility (e.g. PRIVATE) when missing or ensure non-HTTP callers set VISIBILITY before calling getItems?

_{Finding type: Logical Bugs | Severity: 🔴 High}

Want Baz to fix this for you? Activate Fixer

Other fix methods

Prompt for AI Agents:

Before applying, verify this suggestion against the current code. In apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemService.java around lines 1164-1169, the getItems method unconditionally calls ctx.get(RequestContext.VISIBILITY) which throws when VISIBILITY is not present in the Reactor context. Change this to safely obtain a visibility with a default (for example, Visibility.PRIVATE) when the key is missing (e.g., use ctx.getOrDefault(RequestContext.VISIBILITY, Visibility.PRIVATE) or check containsKey and default). Ensure the rest of the method uses this defaulted visibility. Optionally, add/update unit tests and document that non-HTTP callers should also set VISIBILITY if a non-default behavior is required.

I guess our tests are just mocking this service, this wasn't noticed in the local testing. Worth to check, Thiago.

Commit aefebe6 addressed this comment by updating DatasetExpansionService to write the entire RequestContext (via AsyncUtils.setRequestContext) before calling DatasetItemService.getItems. That helper copies VISIBILITY along with workspace/user info, so non-HTTP callers now supply the visibility required by getItems.

Good catch! Fixed in commit aefebe6 — DatasetExpansionService now uses AsyncUtils.setRequestContext(ctx, requestContext) which copies all context keys including VISIBILITY, so non-HTTP callers always supply the required visibility before calling getItems.

Thanks for the update—per DatasetExpansionService (around the new .contextWrite(ctx -> AsyncUtils.setRequestContext(ctx, requestContext)) in getItems), the full RequestContext, including VISIBILITY, is now propagated before calling DatasetItemService#getItems, so non-HTTP callers no longer miss that value.

github-actions · 2026-04-02T10:50:31Z

Backend Tests - Integration Group 1

411 tests 411 ✅ 13m 40s ⏱️
23 suites 0 💤
23 files 0 ❌

Results for commit f9f70fb.

♻️ This comment has been updated with latest results.

CometActions · 2026-04-02T10:50:36Z

✅ Test environment is now available!

To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml)

Access Information

URL: https://pr-6053.dev.comet.com
Cluster: comet-ml-development
Namespace: pr-6053
Version: 1.10.59-6053-merge-1765
Application logs: View in Grafana

The deployment has completed successfully and the version has been verified.

ldaugusto

Some points to check before merge

ldaugusto · 2026-04-02T11:17:00Z

        return Mono.deferContextual(ctx -> {
            String workspaceId = ctx.get(RequestContext.WORKSPACE_ID);
+            Visibility visibility = ctx.get(RequestContext.VISIBILITY);


I guess our tests are just mocking this service, this wasn't noticed in the local testing. Worth to check, Thiago.

- DatasetExpansionService: use AsyncUtils.setRequestContext to ensure VISIBILITY is set in Reactor context - DatasetService.verifyVisibilityIfExists: add debug log documenting the silent-pass when dataset not found - DatasetsResourceTest: fix pre-existing typos; use assertPageAndContent to also assert traceId on returned experiment items

ldaugusto

Thanks for addressing those points!

…ing to null When dataset items are queried via LEFT JOIN after the dataset is deleted, ClickHouse returns the epoch (1970-01-01T00:00:00Z) for DateTime fields where no row matched. Map epoch instants to null in DatasetItemResultMapper so callers receive null instead of the sentinel epoch value. Update assertPageAndContent to handle null timestamps in expected items, branching between asserting null vs asserting isAfter.

Make nullIfEpoch package-private in DatasetItemResultMapper and use it in DatasetItemVersionDAO, which has the same LEFT JOIN queries against deleted dataset tables that can return epoch timestamps.

thiagohora requested a review from a team as a code owner April 2, 2026 10:41

github-actions bot assigned thiagohora Apr 2, 2026

github-actions bot added java Pull requests that update Java code Backend tests Including test files, or tests related like configuration. labels Apr 2, 2026

thiagohora added the test-environment Deploy Opik adhoc environment label Apr 2, 2026

baz-reviewer bot reviewed Apr 2, 2026

View reviewed changes

Revision 2: Fix data field assertion in deleted evaluation suite test

5b18d2b

ldaugusto reviewed Apr 2, 2026

View reviewed changes

thiagohora added 2 commits April 2, 2026 14:01

Revision 3: Document silent-pass intent in verifyVisibilityIfExists

d7d569d

thiagohora requested a review from ldaugusto April 2, 2026 12:09

ldaugusto previously approved these changes Apr 2, 2026

View reviewed changes

thiagohora dismissed ldaugusto’s stale review via f9f70fb April 2, 2026 12:32

baz-reviewer bot reviewed Apr 2, 2026

View reviewed changes

Comment thread apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemResultMapper.java

Revision 2: Apply nullIfEpoch in DatasetItemVersionDAO

b37bb47

Make nullIfEpoch package-private in DatasetItemResultMapper and use it in DatasetItemVersionDAO, which has the same LEFT JOIN queries against deleted dataset tables that can return epoch timestamps.

thiagohora requested a review from ldaugusto April 2, 2026 12:53

ldaugusto approved these changes Apr 2, 2026

View reviewed changes

thiagohora merged commit 21ae163 into main Apr 2, 2026
70 of 73 checks passed

thiagohora deleted the thiaghora/OPIK-5599-fix-experiment-items-display-after-suite-deletion branch April 2, 2026 13:11

Conversation

thiagohora commented Apr 2, 2026

Details

Change checklist

Issues

AI-WATERMARK

Testing

Documentation

Uh oh!

github-actions bot commented Apr 2, 2026

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backend Tests - Integration Group 15

Uh oh!

baz-reviewer bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

ldaugusto Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

baz-reviewer bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

thiagohora Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

baz-reviewer bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backend Tests - Integration Group 1

Uh oh!

CometActions commented Apr 2, 2026

Access Information

Uh oh!

ldaugusto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldaugusto Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

ldaugusto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 2, 2026 •

edited

Loading

github-actions bot commented Apr 2, 2026 •

edited

Loading