From 54870fb7d6bb18d69c73557abc2c1e7ff6a8e1b0 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Thu, 3 Oct 2024 11:41:12 +0200 Subject: [PATCH] [OPIK-159] Experiment delete endpoint --- .../v1/priv/ExperimentsResource.java | 15 ++++ .../com/comet/opik/domain/ExperimentDAO.java | 33 +++++++++ .../comet/opik/domain/ExperimentItemDAO.java | 32 +++++++++ .../comet/opik/domain/ExperimentService.java | 7 ++ .../v1/priv/ExperimentsResourceTest.java | 71 +++++++++++++++++++ 5 files changed, 158 insertions(+) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ExperimentsResource.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ExperimentsResource.java index 9ce949c9..4fc7e479 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ExperimentsResource.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ExperimentsResource.java @@ -31,6 +31,7 @@ import jakarta.validation.constraints.Min; import jakarta.validation.constraints.NotNull; import jakarta.ws.rs.Consumes; +import jakarta.ws.rs.DELETE; import jakarta.ws.rs.DefaultValue; import jakarta.ws.rs.GET; import jakarta.ws.rs.POST; @@ -132,6 +133,20 @@ public Response create( return Response.created(uri).build(); } + @DELETE + @Path("/{id}") + @Operation(operationId = "deleteExperimentById", summary = "Delete experiment", description = "Delete experiment", responses = { + @ApiResponse(responseCode = "204", description = "No content")}) + public Response deleteExperimentById(@PathParam("id") UUID id) { + + log.info("Deleting experiment by id '{}'", id); + experimentService.delete(id) + .contextWrite(ctx -> setRequestContext(ctx, requestContext)) + .block(); + log.info("Deleted experiment by id '{}'", id); + return Response.noContent().build(); + } + // Experiment Item Resources @GET diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java index c467b0ff..7dee28c8 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java @@ -22,11 +22,13 @@ import org.stringtemplate.v4.ST; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.publisher.SignalType; import java.math.BigDecimal; import java.time.Instant; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -362,6 +364,13 @@ AND ilike(name, CONCAT('%', :name, '%')) ; """; + private static final String DELETE_BY_ID = """ + DELETE FROM experiments + WHERE id = :id + AND workspace_id = :workspace_id + ; + """; + private final @NonNull ConnectionFactory connectionFactory; Mono insert(@NonNull Experiment experiment) { @@ -526,4 +535,28 @@ public Flux getExperimentWorkspaces(@NonNull Set e row.get("workspace_id", String.class), row.get("id", UUID.class)))); } + + public Mono delete(UUID id) { + Preconditions.checkArgument(Objects.nonNull(id), "Argument 'id' must not be null"); + + log.info("Deleting experiment by id '{}'", id); + + return Mono.from(connectionFactory.create()) + .flatMapMany(connection -> delete(id, connection)) + .reduce(Long::sum) + .doFinally(signalType -> { + if (signalType == SignalType.ON_COMPLETE) { + log.info("Deleted experiment by id '{}'", id); + } + }); + } + + private Publisher delete(UUID id, Connection connection) { + + var statement = connection.createStatement(DELETE_BY_ID) + .bind("id", id); + + return makeFluxContextAware(bindWorkspaceIdToFlux(statement)) + .flatMap(Result::getRowsUpdated); + } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentItemDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentItemDAO.java index d437a7d3..9942fe63 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentItemDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentItemDAO.java @@ -16,10 +16,12 @@ import org.stringtemplate.v4.ST; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.publisher.SignalType; import java.time.Instant; import java.util.Collection; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.UUID; @@ -117,6 +119,13 @@ INSERT INTO experiment_items ( ; """; + public static final String DELETE_BY_EXPERIMENT_ID = """ + DELETE FROM experiment_items + WHERE experiment_id = :experiment_id + AND workspace_id = :workspace_id + ; + """; + private final @NonNull ConnectionFactory connectionFactory; public Flux findExperimentSummaryByDatasetIds(Collection datasetIds) { @@ -260,4 +269,27 @@ private Publisher delete(Set ids, Connection connection) return makeFluxContextAware(bindWorkspaceIdToFlux(statement)); } + + public Mono deleteByExperimentId(UUID id) { + Preconditions.checkArgument(Objects.nonNull(id), "Argument 'id' must not be null"); + + log.info("Deleting experiment items by experiment id '{}'", id); + + return Mono.from(connectionFactory.create()) + .flatMapMany(connection -> deleteByExperimentId(id, connection)) + .reduce(0L, Long::sum) + .doFinally(signalType -> { + if (signalType == SignalType.ON_COMPLETE) { + log.info("Deleted experiment items by experiment id '{}'", id); + } + }); + } + + private Publisher deleteByExperimentId(UUID id, Connection connection) { + Statement statement = connection.createStatement(DELETE_BY_EXPERIMENT_ID) + .bind("experiment_id", id); + + return makeFluxContextAware(bindWorkspaceIdToFlux(statement)) + .flatMap(Result::getRowsUpdated); + } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentService.java index 3e06bac9..efe50cbd 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentService.java @@ -32,6 +32,7 @@ public class ExperimentService { private final @NonNull ExperimentDAO experimentDAO; + private final @NonNull ExperimentItemDAO experimentItemDAO; private final @NonNull DatasetService datasetService; private final @NonNull IdGenerator idGenerator; private final @NonNull NameGenerator nameGenerator; @@ -147,4 +148,10 @@ public Mono validateExperimentWorkspace(@NonNull String workspaceId, @N return experimentDAO.getExperimentWorkspaces(experimentIds) .all(experimentWorkspace -> workspaceId.equals(experimentWorkspace.workspaceId())); } + + public Mono delete(@NonNull UUID id) { + return experimentDAO.delete(id) + .then(Mono.defer(() -> experimentItemDAO.deleteByExperimentId(id))) + .then(); + } } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ExperimentsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ExperimentsResourceTest.java index f2cf8a0f..49d3e3c9 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ExperimentsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ExperimentsResourceTest.java @@ -1626,6 +1626,77 @@ void getById() { } } + @Nested + @TestInstance(TestInstance.Lifecycle.PER_CLASS) + class DeleteExperimentItems { + + @Test + void deleteExperimentById__whenExperimentExists__thenReturnNoContent() { + var experiment = podamFactory.manufacturePojo(Experiment.class); + + createAndAssert(experiment, API_KEY, TEST_WORKSPACE); + + deleteExperimentAndAssert(experiment.id(), API_KEY, TEST_WORKSPACE); + } + + private void deleteExperimentAndAssert(UUID id, String apiKey, String workspaceName) { + try (var actualResponse = client.target(getExperimentsPath()) + .path(id.toString()) + .request() + .header(HttpHeaders.AUTHORIZATION, apiKey) + .header(WORKSPACE_HEADER, workspaceName) + .delete()) { + + assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(204); + } + + getExperimentAndAssertNotFound(id, apiKey, workspaceName); + } + + private void getExperimentAndAssertNotFound(UUID id, String apiKey, String workspaceName) { + try (var actualResponse = client.target(getExperimentsPath()) + .path(id.toString()) + .request() + .header(HttpHeaders.AUTHORIZATION, apiKey) + .header(WORKSPACE_HEADER, workspaceName) + .get()) { + + assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(404); + } + } + + @Test + void deleteExperimentById__whenExperimentDoesNotExist__thenReturnNoContent() { + var experiment = podamFactory.manufacturePojo(Experiment.class); + + getExperimentAndAssertNotFound(experiment.id(), API_KEY, TEST_WORKSPACE); + + deleteExperimentAndAssert(experiment.id(), API_KEY, TEST_WORKSPACE); + } + + @Test + void deleteExperimentById__whenExperimentHasItems__thenReturnNoContent() { + var experiment = podamFactory.manufacturePojo(Experiment.class); + + createAndAssert(experiment, API_KEY, TEST_WORKSPACE); + + var experimentItems = PodamFactoryUtils.manufacturePojoList(podamFactory, ExperimentItem.class).stream() + .map(experimentItem -> experimentItem.toBuilder().experimentId(experiment.id()).build()) + .collect(Collectors.toUnmodifiableSet()); + + var createRequest = ExperimentItemsBatch.builder().experimentItems(experimentItems).build(); + + createAndAssert(createRequest, API_KEY, TEST_WORKSPACE); + + deleteExperimentAndAssert(experiment.id(), API_KEY, TEST_WORKSPACE); + + experimentItems + .stream() + .parallel() + .forEach(experimentItem -> getAndAssertNotFound(experimentItem.id(), API_KEY, TEST_WORKSPACE)); + } + } + @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) class StreamExperimentItems {