diff --git a/src/main/java/it/cnr/isti/workflow/manager/controllers/ExecutionsController.java b/src/main/java/it/cnr/isti/workflow/manager/controllers/ExecutionsController.java index 4380870..78b2ad4 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/controllers/ExecutionsController.java +++ b/src/main/java/it/cnr/isti/workflow/manager/controllers/ExecutionsController.java @@ -421,9 +421,11 @@ public class ExecutionsController { @Operation(summary = "Provides human interaction output", description = "Provides the value for a waiting human interaction step.") public ExecutionView provideInteractionValue(@PathVariable String executionId, @PathVariable String nodeId, @PathVariable String fieldName, @RequestBody String value, + @RequestParam(required = false) Integer iteration, @AuthenticationPrincipal LoginEntity userDetails) { - return ExecutionView.fromExecution( - executionService.setInteractionValue(visibleExecution(executionId, userDetails).getId(), nodeId, fieldName, value)); + String id = visibleExecution(executionId, userDetails).getId(); + requireRound(id, nodeId, iteration); + return ExecutionView.fromExecution(executionService.setInteractionValue(id, nodeId, fieldName, value)); } @PutMapping(path = "{executionId}/node/{nodeId}/evaluation", consumes = "application/json") @@ -433,7 +435,9 @@ public class ExecutionsController { + "scale the criterion declares, in which case nothing is recorded.") public ExecutionView submitEvaluation(@PathVariable String executionId, @PathVariable String nodeId, @RequestBody @jakarta.validation.Valid HumanEvaluationRequest request, + @RequestParam(required = false) Integer iteration, @AuthenticationPrincipal LoginEntity userDetails) { + requireRound(visibleExecution(executionId, userDetails).getId(), nodeId, iteration); Map interaction = new LinkedHashMap<>(request.verdict()); if (request.notes() != null) { interaction.put(HumanEvaluationBlockFactory.NOTES_FIELD, request.notes()); @@ -454,7 +458,9 @@ public class ExecutionsController { description = "Adds files - screenshots, recordings, exports - to a waiting HumanEvaluation step. They " + "accumulate across calls and leave on the node's evidence output once the judgement is submitted.") public ExecutionView attachEvaluationEvidence(@PathVariable String executionId, @PathVariable String nodeId, - @RequestParam List files, @AuthenticationPrincipal LoginEntity userDetails) { + @RequestParam List files, @RequestParam(required = false) Integer iteration, + @AuthenticationPrincipal LoginEntity userDetails) { + requireRound(visibleExecution(executionId, userDetails).getId(), nodeId, iteration); List stored = new ArrayList<>(); try { for (MultipartFile file : files) { @@ -477,12 +483,30 @@ public class ExecutionsController { + "node configured to keep it hidden. The execution records the reveal, so a judgement made after " + "it can be told from one made blind - which is the whole point of hiding it.") public ExecutionView revealEvaluationReference(@PathVariable String executionId, @PathVariable String nodeId, + @RequestParam(required = false) Integer iteration, @AuthenticationPrincipal LoginEntity userDetails) { - return ExecutionView.fromExecution(executionService.setInteractionValues( - visibleExecution(executionId, userDetails).getId(), nodeId, + String id = visibleExecution(executionId, userDetails).getId(); + requireRound(id, nodeId, iteration); + return ExecutionView.fromExecution(executionService.setInteractionValues(id, nodeId, Map.of(HumanEvaluationExecutor.REFERENCE_REVEALED_FIELD, Boolean.TRUE))); } + /** + * Refuses an answer meant for another round of a loop. A node in a loop asks the same question + * once per round, and an answer given in one tab while another has already moved the loop on + * would otherwise be taken as the answer to a draft its author never saw. + */ + private void requireRound(String executionId, String nodeId, Integer iteration) { + if (iteration == null) { + return; + } + var step = executionService.getExecution(executionId).getContext().getSteps().get(nodeId); + if (step != null && step.getIteration() != iteration) { + throw new ResponseStatusException(HttpStatus.CONFLICT, "This answer is for round " + iteration + + " of the loop, but the node is now on round " + step.getIteration() + ". Reload to see what it asks now"); + } + } + @PutMapping(path = "{executionId}/authorizations") @Operation(summary = "Provides execution authorization", description = "Stores the selected saved credential reference for a provider authorization required by an " diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionContextView.java b/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionContextView.java index e531d50..0b9b77c 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionContextView.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionContextView.java @@ -4,6 +4,7 @@ package it.cnr.isti.workflow.manager.executions.api; +import it.cnr.isti.workflow.manager.executions.persistence.ExecutionStepSnapshot; import java.util.Map; import java.util.stream.Collectors; @@ -36,6 +37,8 @@ public class ExecutionContextView { private java.util.List warnings; private java.util.List outcomes; private Map steps; + /** Loop steps as each earlier round left them, oldest first; the steps above are the current round. */ + private java.util.List stepHistory; private ExecutionStatus status; private java.util.List waitingSteps; @@ -62,6 +65,7 @@ public class ExecutionContextView { ExecutionStepView::fromStep, (left, right) -> right, java.util.LinkedHashMap::new))) + .stepHistory(context.getStepHistory()) .status(context.getStatus()) .waitingSteps(context.getWaitingSteps()) .build(); diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionStepView.java b/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionStepView.java index 6fb1386..d9d62ca 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionStepView.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/api/ExecutionStepView.java @@ -30,6 +30,8 @@ public class ExecutionStepView { private List inputs; private List outputs; private StepStatus status; + /** Which round of its loop the step is on; 1 for a step in no loop. */ + private int iteration; private StepSkipReason skipReason; private boolean simulated; private String activeInnerExecutionId; @@ -44,6 +46,7 @@ public class ExecutionStepView { .inputs(step.getInputs()) .outputs(step.getOutputs()) .status(step.getStatus()) + .iteration(step.getIteration()) .skipReason(step.getSkipReason()) .simulated(step.isSimulated()) .activeInnerExecutionId(step.getContainerContinuation() == null diff --git a/src/test/java/it/cnr/isti/workflow/manager/controllers/ExecutionControllerTest.java b/src/test/java/it/cnr/isti/workflow/manager/controllers/ExecutionControllerTest.java index e187b71..d6f50f4 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/controllers/ExecutionControllerTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/controllers/ExecutionControllerTest.java @@ -312,7 +312,7 @@ public class ExecutionControllerTest { ExecutionView execution = waitingEvaluation(reviewBlock); execution = executionsController.submitEvaluation(execution.getId(), reviewBlock.getId(), - new HumanEvaluationRequest(Map.of("works", "fail", "clarity", "2"), "Checkout breaks"), testUser()); + new HumanEvaluationRequest(Map.of("works", "fail", "clarity", "2"), "Checkout breaks"), null, testUser()); assertEquals(Map.of("works", "fail", "clarity", "2"), resultOf(execution, reviewBlock.getId(), "verdict")); assertEquals("fail", resultOf(execution, reviewBlock.getId(), "works")); @@ -331,10 +331,10 @@ public class ExecutionControllerTest { .build()); ExecutionView execution = waitingEvaluation(reviewBlock); - executionsController.revealEvaluationReference(execution.getId(), reviewBlock.getId(), testUser()); + executionsController.revealEvaluationReference(execution.getId(), reviewBlock.getId(), null, testUser()); execution = executionsController.submitEvaluation(execution.getId(), reviewBlock.getId(), - new HumanEvaluationRequest(Map.of("works", "pass"), ""), testUser()); + new HumanEvaluationRequest(Map.of("works", "pass"), ""), null, testUser()); assertEquals(false, resultOf(execution, reviewBlock.getId(), "blind")); } @@ -353,7 +353,7 @@ public class ExecutionControllerTest { ResponseStatusException failure = assertThrows(ResponseStatusException.class, () -> executionsController.submitEvaluation(execution.getId(), reviewBlock.getId(), - new HumanEvaluationRequest(Map.of("works", "almost"), ""), testUser())); + new HumanEvaluationRequest(Map.of("works", "almost"), ""), null, testUser())); assertEquals(HttpStatus.BAD_REQUEST, failure.getStatusCode()); assertTrue(failure.getReason().contains("works")); @@ -436,6 +436,7 @@ public class ExecutionControllerTest { reviewBlock.getId(), "output", "Approved", + null, testUser()); waitForExecutionStatus(executionObject, ExecutionStatus.SUCCESS); @@ -808,7 +809,7 @@ public class ExecutionControllerTest { waitForExecutionStatus(executionObject, ExecutionStatus.WAITING); executionsController.provideInteractionValue(executionObject.getId(), reviewBlock.getId(), "output", "Approved", - testUser()); + null, testUser()); waitForExecutionStatus(executionObject, ExecutionStatus.SUCCESS); } @@ -1015,7 +1016,7 @@ public class ExecutionControllerTest { org.junit.jupiter.api.Assertions.assertEquals(ExecutionStatus.WAITING, resumed.getContext().getStatus()); resumed = executionsController.provideInteractionValue(executionId, reviewBlock.getId(), "output", "Approved", - testUser()); + null, testUser()); waitForExecutionStatus(resumed, ExecutionStatus.SUCCESS); } diff --git a/src/test/java/it/cnr/isti/workflow/manager/executions/LoopExecutionTest.java b/src/test/java/it/cnr/isti/workflow/manager/executions/LoopExecutionTest.java index b9db23d..d026e71 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/executions/LoopExecutionTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/executions/LoopExecutionTest.java @@ -6,6 +6,7 @@ package it.cnr.isti.workflow.manager.executions; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Duration; @@ -17,8 +18,12 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.TestConfiguration; import org.springframework.context.annotation.Bean; +import org.springframework.http.HttpStatus; import org.springframework.test.context.TestPropertySource; +import org.springframework.web.server.ResponseStatusException; +import it.cnr.isti.workflow.manager.auth.repo.LoginEntity; +import it.cnr.isti.workflow.manager.controllers.ExecutionsController; import it.cnr.isti.workflow.manager.blocks.Block; import it.cnr.isti.workflow.manager.blocks.configurations.ConditionalBlockConfiguration; import it.cnr.isti.workflow.manager.blocks.configurations.EndBlockConfiguration; @@ -88,9 +93,14 @@ class LoopExecutionTest { } } + private static final LoginEntity USER = new LoginEntity("testuser", "testpassword"); + @Autowired ExecutionsService executionsService; + @Autowired + ExecutionsController executionsController; + @Autowired LLMBlockFactory llmBlockFactory; @@ -215,7 +225,7 @@ class LoopExecutionTest { .connection(connection(review, "approve", approved, EndBlockFactory.INPUT_NAME)) .build(); - ExecutionObject execution = executionsService.createExecution("Reviewed until approved", flow); + ExecutionObject execution = executionsService.createExecution("Reviewed until approved", flow, USER.getUsername()); executionsService.prepareInput(execution.getId(), draft.getId(), "topic", "idea"); executionsService.startExecution(execution.getId()); @@ -226,6 +236,16 @@ class LoopExecutionTest { } awaitStepWaiting(execution.getId(), review.getId(), 3); + // An answer sent from a page still showing round 2 must not land on round 3's draft. + String executionId = execution.getId(); + ResponseStatusException stale = assertThrows(ResponseStatusException.class, + () -> executionsController.provideInteractionValue(executionId, review.getId(), + HumanDecisionBlockFactory.CHOICE_FIELD, "approve", 2, USER)); + assertEquals(HttpStatus.CONFLICT, stale.getStatusCode()); + assertEquals(StepStatus.WAITING_FOR_INTERACTION, + executionsService.getExecution(executionId).getContext().getSteps().get(review.getId()).getStatus(), + "the refused answer left the question open"); + // Read back from storage in the middle of the loop: the round it is on, and the rounds // before, have to survive the trip. executionsService.clearInMemoryExecutions();