From 8948103957824ff95bea28178ae7c3d6608a7009 Mon Sep 17 00:00:00 2001 From: Sebastian Frick Date: Thu, 26 Feb 2026 19:42:44 +0100 Subject: [PATCH] fix(production): N+1-Query in traceForward durch Level-by-Level BFS ersetzen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit traceForward aus BatchTraceabilityService in BatchRepository verschoben. Statt einer Query pro Knoten (N+1) wird jetzt eine IN(:parentIds)-Query pro Tiefenebene ausgeführt (max. maxDepth Queries statt N). --- .../domain/production/BatchRepository.java | 2 + .../production/BatchTraceabilityService.java | 40 +--- .../persistence/JdbcBatchRepository.java | 42 +++- .../stub/StubBatchRepository.java | 6 + .../BatchTraceabilityServiceFuzzTest.java | 29 ++- .../BatchTraceabilityServiceTest.java | 185 +++++------------- 6 files changed, 130 insertions(+), 174 deletions(-) diff --git a/backend/src/main/java/de/effigenix/domain/production/BatchRepository.java b/backend/src/main/java/de/effigenix/domain/production/BatchRepository.java index a1fb89e..a22842d 100644 --- a/backend/src/main/java/de/effigenix/domain/production/BatchRepository.java +++ b/backend/src/main/java/de/effigenix/domain/production/BatchRepository.java @@ -31,5 +31,7 @@ public interface BatchRepository { Result> findByInputBatchId(BatchId inputBatchId); + Result> traceForward(BatchId startBatchId, int maxDepth); + Result save(Batch batch); } diff --git a/backend/src/main/java/de/effigenix/domain/production/BatchTraceabilityService.java b/backend/src/main/java/de/effigenix/domain/production/BatchTraceabilityService.java index b857cc2..94b3c36 100644 --- a/backend/src/main/java/de/effigenix/domain/production/BatchTraceabilityService.java +++ b/backend/src/main/java/de/effigenix/domain/production/BatchTraceabilityService.java @@ -2,11 +2,7 @@ package de.effigenix.domain.production; import de.effigenix.shared.common.Result; -import java.util.ArrayDeque; -import java.util.ArrayList; -import java.util.HashSet; import java.util.List; -import java.util.Set; public class BatchTraceabilityService { @@ -33,35 +29,15 @@ public class BatchTraceabilityService { } } - List result = new ArrayList<>(); - Set visited = new HashSet<>(); - visited.add(startBatchId.value()); - - record BfsEntry(BatchId batchId, int depth) {} - var queue = new ArrayDeque(); - queue.add(new BfsEntry(startBatchId, 0)); - - while (!queue.isEmpty()) { - var entry = queue.poll(); - if (entry.depth() >= maxDepth) { - continue; - } - - switch (batchRepository.findByInputBatchId(entry.batchId())) { - case Result.Failure(var err) -> - { return Result.failure(new BatchError.RepositoryFailure(err.message())); } - case Result.Success(var children) -> { - int childDepth = entry.depth() + 1; - for (Batch child : children) { - if (visited.add(child.id().value())) { - result.add(new TracedBatch(child, childDepth)); - queue.add(new BfsEntry(child.id(), childDepth)); - } - } - } - } + if (maxDepth <= 0) { + return Result.success(List.of()); } - return Result.success(result); + return switch (batchRepository.traceForward(startBatchId, maxDepth)) { + case Result.Failure(var err) -> + Result.failure(new BatchError.RepositoryFailure(err.message())); + case Result.Success(var traced) -> + Result.success(traced); + }; } } diff --git a/backend/src/main/java/de/effigenix/infrastructure/production/persistence/JdbcBatchRepository.java b/backend/src/main/java/de/effigenix/infrastructure/production/persistence/JdbcBatchRepository.java index 7e32f56..1351327 100644 --- a/backend/src/main/java/de/effigenix/infrastructure/production/persistence/JdbcBatchRepository.java +++ b/backend/src/main/java/de/effigenix/infrastructure/production/persistence/JdbcBatchRepository.java @@ -17,6 +17,8 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.time.LocalDate; import java.time.OffsetDateTime; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Optional; @@ -210,6 +212,44 @@ public class JdbcBatchRepository implements BatchRepository { } } + @Override + public Result> traceForward(BatchId startBatchId, int maxDepth) { + try { + String startId = startBatchId.value(); + var results = new ArrayList(); + var visited = new HashSet(); + visited.add(startId); + + var currentLevel = List.of(startId); + for (int depth = 1; depth <= maxDepth && !currentLevel.isEmpty(); depth++) { + int currentDepth = depth; + var children = jdbc.sql(""" + SELECT DISTINCT b.* + FROM batches b + JOIN batch_consumptions bc ON b.id = bc.batch_id + WHERE bc.input_batch_id IN (:parentIds) + """) + .param("parentIds", currentLevel) + .query(this::mapBatchRow) + .list(); + + var nextLevel = new ArrayList(); + for (Batch child : children) { + if (visited.add(child.id().value())) { + results.add(new TracedBatch(child, currentDepth)); + nextLevel.add(child.id().value()); + } + } + currentLevel = nextLevel; + } + + return Result.success(results); + } catch (Exception e) { + logger.trace("Database error in traceForward", e); + return Result.failure(new RepositoryError.DatabaseError(e.getMessage())); + } + } + @Override public Result save(Batch batch) { try { @@ -294,7 +334,7 @@ public class JdbcBatchRepository implements BatchRepository { .query((rs, rowNum) -> rs.getString("id")) .list(); - var existingIdSet = new java.util.HashSet<>(existingIds); + var existingIdSet = new HashSet<>(existingIds); for (Consumption c : batch.consumptions()) { if (!existingIdSet.contains(c.id().value())) { diff --git a/backend/src/main/java/de/effigenix/infrastructure/stub/StubBatchRepository.java b/backend/src/main/java/de/effigenix/infrastructure/stub/StubBatchRepository.java index 23c6ced..961e71d 100644 --- a/backend/src/main/java/de/effigenix/infrastructure/stub/StubBatchRepository.java +++ b/backend/src/main/java/de/effigenix/infrastructure/stub/StubBatchRepository.java @@ -6,6 +6,7 @@ import de.effigenix.domain.production.BatchNumber; import de.effigenix.domain.production.BatchRepository; import de.effigenix.domain.production.BatchStatus; import de.effigenix.domain.production.RecipeId; +import de.effigenix.domain.production.TracedBatch; import de.effigenix.shared.common.RepositoryError; import de.effigenix.shared.common.Result; import org.springframework.context.annotation.Profile; @@ -77,6 +78,11 @@ public class StubBatchRepository implements BatchRepository { return Result.failure(STUB_ERROR); } + @Override + public Result> traceForward(BatchId startBatchId, int maxDepth) { + return Result.failure(STUB_ERROR); + } + @Override public Result save(Batch batch) { return Result.failure(STUB_ERROR); diff --git a/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceFuzzTest.java b/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceFuzzTest.java index c6238ae..be886a0 100644 --- a/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceFuzzTest.java +++ b/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceFuzzTest.java @@ -96,7 +96,7 @@ class BatchTraceabilityServiceFuzzTest { /** * In-memory BatchRepository that serves a pre-built graph. - * Only findById and findByInputBatchId are implemented (the rest throw). + * Implements traceForward with BFS (mirrors the contract of the recursive CTE). */ private static class InMemoryGraphRepository implements BatchRepository { @@ -122,6 +122,33 @@ class BatchTraceabilityServiceFuzzTest { return Result.success(children.stream().map(BatchTraceabilityServiceFuzzTest::makeBatch).toList()); } + @Override + public Result> traceForward(BatchId startBatchId, int maxDepth) { + List result = new ArrayList<>(); + Set visited = new HashSet<>(); + visited.add(startBatchId.value()); + + record BfsEntry(String batchId, int depth) {} + var queue = new ArrayDeque(); + queue.add(new BfsEntry(startBatchId.value(), 0)); + + while (!queue.isEmpty()) { + var entry = queue.poll(); + if (entry.depth() >= maxDepth) continue; + + var children = adjacency.getOrDefault(entry.batchId(), List.of()); + int childDepth = entry.depth() + 1; + for (String childId : children) { + if (visited.add(childId)) { + result.add(new TracedBatch(makeBatch(childId), childDepth)); + queue.add(new BfsEntry(childId, childDepth)); + } + } + } + + return Result.success(result); + } + // --- Remaining methods are not used by BatchTraceabilityService --- @Override public Result> findAll() { throw new UnsupportedOperationException(); } diff --git a/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceTest.java b/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceTest.java index 61673e4..de6ef1e 100644 --- a/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceTest.java +++ b/backend/src/test/java/de/effigenix/domain/production/BatchTraceabilityServiceTest.java @@ -56,88 +56,6 @@ class BatchTraceabilityServiceTest { @DisplayName("traceForward") class TraceForward { - @Test - @DisplayName("should return empty list when no downstream batches exist") - void should_ReturnEmptyList_When_NoDownstreamBatches() { - var startId = BatchId.of("start-batch"); - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start-batch")))); - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of())); - - var result = service.traceForward(startId); - - assertThat(result.isSuccess()).isTrue(); - assertThat(result.unsafeGetValue()).isEmpty(); - } - - @Test - @DisplayName("should return direct downstream batches at depth 1") - void should_ReturnDirectDownstream_AtDepth1() { - var startId = BatchId.of("start-batch"); - var child1 = sampleBatch("child-1"); - var child2 = sampleBatch("child-2"); - - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start-batch")))); - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of(child1, child2))); - when(batchRepository.findByInputBatchId(BatchId.of("child-1"))).thenReturn(Result.success(List.of())); - when(batchRepository.findByInputBatchId(BatchId.of("child-2"))).thenReturn(Result.success(List.of())); - - var result = service.traceForward(startId); - - assertThat(result.isSuccess()).isTrue(); - var traced = result.unsafeGetValue(); - assertThat(traced).hasSize(2); - assertThat(traced).allMatch(t -> t.depth() == 1); - assertThat(traced).extracting(t -> t.batch().id().value()) - .containsExactlyInAnyOrder("child-1", "child-2"); - } - - @Test - @DisplayName("should traverse multi-level chain with correct depths") - void should_TraverseMultiLevel_WithCorrectDepths() { - var startId = BatchId.of("start"); - var level1 = sampleBatch("level-1"); - var level2 = sampleBatch("level-2"); - var level3 = sampleBatch("level-3"); - - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start")))); - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of(level1))); - when(batchRepository.findByInputBatchId(BatchId.of("level-1"))).thenReturn(Result.success(List.of(level2))); - when(batchRepository.findByInputBatchId(BatchId.of("level-2"))).thenReturn(Result.success(List.of(level3))); - when(batchRepository.findByInputBatchId(BatchId.of("level-3"))).thenReturn(Result.success(List.of())); - - var result = service.traceForward(startId); - - assertThat(result.isSuccess()).isTrue(); - var traced = result.unsafeGetValue(); - assertThat(traced).hasSize(3); - assertThat(traced.get(0).batch().id().value()).isEqualTo("level-1"); - assertThat(traced.get(0).depth()).isEqualTo(1); - assertThat(traced.get(1).batch().id().value()).isEqualTo("level-2"); - assertThat(traced.get(1).depth()).isEqualTo(2); - assertThat(traced.get(2).batch().id().value()).isEqualTo("level-3"); - assertThat(traced.get(2).depth()).isEqualTo(3); - } - - @Test - @DisplayName("should detect cycles and terminate without endless loop") - void should_DetectCycles_AndTerminate() { - var startId = BatchId.of("start"); - var child = sampleBatch("child"); - - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start")))); - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of(child))); - // child references back to start — cycle - when(batchRepository.findByInputBatchId(BatchId.of("child"))) - .thenReturn(Result.success(List.of(sampleBatch("start")))); - - var result = service.traceForward(startId); - - assertThat(result.isSuccess()).isTrue(); - var traced = result.unsafeGetValue(); - assertThat(traced).hasSize(1); - assertThat(traced.get(0).batch().id().value()).isEqualTo("child"); - } - @Test @DisplayName("should fail with BatchNotFound when start batch does not exist") void should_FailWithBatchNotFound_When_StartBatchDoesNotExist() { @@ -148,26 +66,7 @@ class BatchTraceabilityServiceTest { assertThat(result.isFailure()).isTrue(); assertThat(result.unsafeGetError()).isInstanceOf(BatchError.BatchNotFound.class); - } - - @Test - @DisplayName("should stop at max depth limit") - void should_StopAtMaxDepth() { - var startId = BatchId.of("start"); - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start")))); - - var level1 = sampleBatch("level-1"); - var level2 = sampleBatch("level-2"); - - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of(level1))); - when(batchRepository.findByInputBatchId(BatchId.of("level-1"))).thenReturn(Result.success(List.of(level2))); - - var result = service.traceForward(startId, 2); - - assertThat(result.isSuccess()).isTrue(); - var traced = result.unsafeGetValue(); - assertThat(traced).hasSize(2); - assertThat(traced).extracting(TracedBatch::depth).containsExactly(1, 2); + verify(batchRepository, never()).traceForward(any(), anyInt()); } @Test @@ -181,46 +80,40 @@ class BatchTraceabilityServiceTest { assertThat(result.isFailure()).isTrue(); assertThat(result.unsafeGetError()).isInstanceOf(BatchError.RepositoryFailure.class); + verify(batchRepository, never()).traceForward(any(), anyInt()); } @Test - @DisplayName("should fail with RepositoryFailure when findByInputBatchId returns error") - void should_FailWithRepositoryFailure_When_FindByInputBatchIdReturnsError() { + @DisplayName("should delegate to repository traceForward when start batch exists") + void should_DelegateToRepository_When_StartBatchExists() { var startId = BatchId.of("start"); when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start")))); - when(batchRepository.findByInputBatchId(startId)) - .thenReturn(Result.failure(new RepositoryError.DatabaseError("timeout"))); - var result = service.traceForward(startId); - - assertThat(result.isFailure()).isTrue(); - assertThat(result.unsafeGetError()).isInstanceOf(BatchError.RepositoryFailure.class); - } - - @Test - @DisplayName("should deduplicate shared child in diamond graph") - void should_DeduplicateSharedChild_InDiamondGraph() { - // A → B, A → C, B → D, C → D ⇒ D appears only once - var startId = BatchId.of("A"); - var batchB = sampleBatch("B"); - var batchC = sampleBatch("C"); - var batchD = sampleBatch("D"); - - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("A")))); - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of(batchB, batchC))); - when(batchRepository.findByInputBatchId(BatchId.of("B"))).thenReturn(Result.success(List.of(batchD))); - when(batchRepository.findByInputBatchId(BatchId.of("C"))).thenReturn(Result.success(List.of(sampleBatch("D")))); - when(batchRepository.findByInputBatchId(BatchId.of("D"))).thenReturn(Result.success(List.of())); + var traced = List.of( + new TracedBatch(sampleBatch("child-1"), 1), + new TracedBatch(sampleBatch("child-2"), 1) + ); + when(batchRepository.traceForward(startId, BatchTraceabilityService.DEFAULT_MAX_DEPTH)) + .thenReturn(Result.success(traced)); var result = service.traceForward(startId); assertThat(result.isSuccess()).isTrue(); - var traced = result.unsafeGetValue(); - assertThat(traced).extracting(t -> t.batch().id().value()) - .containsExactlyInAnyOrder("B", "C", "D"); - // D must appear only once despite being reachable from B and C - assertThat(traced.stream().filter(t -> t.batch().id().value().equals("D")).count()) - .isEqualTo(1); + assertThat(result.unsafeGetValue()).hasSize(2); + verify(batchRepository).traceForward(startId, BatchTraceabilityService.DEFAULT_MAX_DEPTH); + } + + @Test + @DisplayName("should pass custom maxDepth to repository") + void should_PassCustomMaxDepth_ToRepository() { + var startId = BatchId.of("start"); + when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start")))); + when(batchRepository.traceForward(startId, 5)).thenReturn(Result.success(List.of())); + + var result = service.traceForward(startId, 5); + + assertThat(result.isSuccess()).isTrue(); + verify(batchRepository).traceForward(startId, 5); } @Test @@ -233,23 +126,35 @@ class BatchTraceabilityServiceTest { assertThat(result.isSuccess()).isTrue(); assertThat(result.unsafeGetValue()).isEmpty(); + verify(batchRepository, never()).traceForward(any(), anyInt()); } @Test - @DisplayName("should not include start batch in result") - void should_NotIncludeStartBatch_InResult() { + @DisplayName("should fail with RepositoryFailure when traceForward returns error") + void should_FailWithRepositoryFailure_When_TraceForwardReturnsError() { var startId = BatchId.of("start"); - var child = sampleBatch("child"); - when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start")))); - when(batchRepository.findByInputBatchId(startId)).thenReturn(Result.success(List.of(child))); - when(batchRepository.findByInputBatchId(BatchId.of("child"))).thenReturn(Result.success(List.of())); + when(batchRepository.traceForward(startId, BatchTraceabilityService.DEFAULT_MAX_DEPTH)) + .thenReturn(Result.failure(new RepositoryError.DatabaseError("timeout"))); + + var result = service.traceForward(startId); + + assertThat(result.isFailure()).isTrue(); + assertThat(result.unsafeGetError()).isInstanceOf(BatchError.RepositoryFailure.class); + } + + @Test + @DisplayName("should return empty list when no downstream batches exist") + void should_ReturnEmptyList_When_NoDownstreamBatches() { + var startId = BatchId.of("start-batch"); + when(batchRepository.findById(startId)).thenReturn(Result.success(Optional.of(sampleBatch("start-batch")))); + when(batchRepository.traceForward(startId, BatchTraceabilityService.DEFAULT_MAX_DEPTH)) + .thenReturn(Result.success(List.of())); var result = service.traceForward(startId); assertThat(result.isSuccess()).isTrue(); - assertThat(result.unsafeGetValue()).extracting(t -> t.batch().id().value()) - .doesNotContain("start"); + assertThat(result.unsafeGetValue()).isEmpty(); } } }