Move Validation before the Pipeline

src/main/java/de/medizininformatikinitiative/torch/management/OperationOutcomeCreator.java

@@ -11,7 +11,7 @@ public static OperationOutcome createOperationOutcome(String jobId, Throwable th
         OperationOutcome.OperationOutcomeIssueComponent issueComponent = new OperationOutcome.OperationOutcomeIssueComponent();
         issueComponent.setSeverity(OperationOutcome.IssueSeverity.FATAL);
         issueComponent.setCode(createIssueType(throwable));
-        issueComponent.setDiagnostics(throwable.getClass().getSimpleName() + ": " + throwable.getMessage());
+        issueComponent.setDiagnostics(throwable.getMessage());
         operationOutcome.addIssue(issueComponent);
         return operationOutcome;
     }

src/main/java/de/medizininformatikinitiative/torch/rest/FhirController.java

@@ -1,6 +1,8 @@
 package de.medizininformatikinitiative.torch.rest;
 
 import ca.uhn.fhir.context.FhirContext;
+import de.medizininformatikinitiative.torch.exceptions.ValidationException;
+import de.medizininformatikinitiative.torch.service.CrtdlValidatorService;
 import de.medizininformatikinitiative.torch.service.ExtractDataService;
 import de.medizininformatikinitiative.torch.util.ResultFileManager;
 import org.hl7.fhir.r4.model.OperationOutcome;
@@ -45,15 +47,17 @@ public class FhirController {
     private final ExtractDataParametersParser extractDataParametersParser;
     private final ExtractDataService extractDataService;
     private final String baseUrl;
+    private final CrtdlValidatorService validator;
 
     @Autowired
     public FhirController(FhirContext fhirContext, ResultFileManager resultFileManager,
-                          ExtractDataParametersParser parser, ExtractDataService extractDataService, @Value("${torch.base.url}") String baseUrl) {
+                          ExtractDataParametersParser parser, ExtractDataService extractDataService, @Value("${torch.base.url}") String baseUrl, CrtdlValidatorService validator) {
         this.fhirContext = requireNonNull(fhirContext);
         this.resultFileManager = requireNonNull(resultFileManager);
         this.extractDataParametersParser = requireNonNull(parser);
         this.extractDataService = requireNonNull(extractDataService);
         this.baseUrl = baseUrl;
+        this.validator = requireNonNull(validator);
     }
 
     private Mono<ServerResponse> getGlobalStatus(ServerRequest serverRequest) {
@@ -90,10 +94,13 @@ public Mono<ServerResponse> handleExtractData(ServerRequest request) {
                 .switchIfEmpty(Mono.error(new IllegalArgumentException("Empty request body")))
                 .map(extractDataParametersParser::parseParameters)
                 .flatMap(parameters -> {
-                    Mono<Void> jobMono = extractDataService
-                            .startJob(parameters.crtdl(), parameters.patientIds(), jobId);
-
-                    // Launch it asynchronously
+                    Mono<Void> jobMono;
+                    try {
+                        jobMono = extractDataService
+                                .startJob(validator.validate(parameters.crtdl()), parameters.patientIds(), jobId);
+                    } catch (ValidationException e) {
+                        return Mono.error(new IllegalArgumentException(e.getMessage()));
+                    }
                     jobMono.subscribe();
                     return ServerResponse.accepted()
                             .header("Content-Location",

src/main/java/de/medizininformatikinitiative/torch/service/ExtractDataService.java

@@ -1,7 +1,7 @@
 package de.medizininformatikinitiative.torch.service;
 
 import de.medizininformatikinitiative.torch.exceptions.ValidationException;
-import de.medizininformatikinitiative.torch.model.crtdl.Crtdl;
+import de.medizininformatikinitiative.torch.model.crtdl.annotated.AnnotatedCrtdl;
 import de.medizininformatikinitiative.torch.util.ResultFileManager;
 import org.hl7.fhir.r4.model.OperationOutcome;
 import org.springframework.http.HttpStatus;
@@ -18,24 +18,19 @@
 public class ExtractDataService {
 
     private final ResultFileManager resultFileManager;
-    private final CrtdlValidatorService validatorService;
     private final CrtdlProcessingService processingService;
 
     public ExtractDataService(ResultFileManager resultFileManager,
-                              CrtdlValidatorService validatorService,
                               CrtdlProcessingService processingService) {
         this.resultFileManager = requireNonNull(resultFileManager);
-        this.validatorService = requireNonNull(validatorService);
         this.processingService = requireNonNull(processingService);
     }
 
-    public Mono<Void> startJob(Crtdl crtdl, List<String> patientIds, String jobId) {
+    public Mono<Void> startJob(AnnotatedCrtdl crtdl, List<String> patientIds, String jobId) {
         resultFileManager.setStatus(jobId, HttpStatus.ACCEPTED);
 
         return resultFileManager.initJobDir(jobId)
-                .then(Mono.fromCallable(() -> validatorService.validate(crtdl))
-                        .subscribeOn(Schedulers.boundedElastic()))
-                .flatMap(validated -> processingService.process(validated, jobId, patientIds))
+                .then(processingService.process(crtdl, jobId, patientIds))
                 .doOnSuccess(v -> resultFileManager.setStatus(jobId, HttpStatus.OK))
                 .doOnError(e -> handleJobError(jobId, e))
                 .onErrorResume(e -> Mono.empty());

src/main/java/de/medizininformatikinitiative/torch/util/ResultFileManager.java

@@ -77,7 +77,7 @@ public Map<String, HttpStatus> getJobStatusMap() {
         return jobStatusMap;
     }
 
-    public void loadExistingResults() {
+    private void loadExistingResults() {
         try (Stream<Path> jobDirs = Files.list(resultsDirPath)) {
             jobDirs.filter(Files::isDirectory)
                     .forEach(jobDir -> {

src/test/java/de/medizininformatikinitiative/torch/FhirControllerIT.java

@@ -17,11 +17,7 @@
 import de.numcodex.sq2cql.Translator;
 import de.numcodex.sq2cql.model.structured_query.StructuredQuery;
 import org.hl7.fhir.r4.model.OperationOutcome;
-import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Nested;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.api.*;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
@@ -31,11 +27,7 @@
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.boot.test.web.client.TestRestTemplate;
-import org.springframework.http.HttpEntity;
-import org.springframework.http.HttpHeaders;
-import org.springframework.http.HttpMethod;
-import org.springframework.http.MediaType;
-import org.springframework.http.ResponseEntity;
+import org.springframework.http.*;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.ActiveProfiles;
 import org.springframework.web.client.HttpStatusCodeException;
@@ -181,7 +173,7 @@ void testCoreMustHave() throws IOException, ValidationException {
         fis.close();
     }
 
-    void testExecutor(String filePath, String url, HttpHeaders headers, int expectedFinalCode) {
+    void testExecutor(String filePath, String url, HttpHeaders headers) {
         TestRestTemplate restTemplate = new TestRestTemplate();
         try {
             String fileContent = Files.readString(Paths.get(filePath), StandardCharsets.UTF_8);
@@ -194,7 +186,7 @@ void testExecutor(String filePath, String url, HttpHeaders headers, int expected
                 assertThat(durationSecondsSince(start)).isLessThan(1);
                 List<String> locations = response.getHeaders().get("Content-Location");
                 assertThat(locations).hasSize(1);
-                pollStatusEndpoint(restTemplate, headers, locations.getFirst(), expectedFinalCode);
+                pollStatusEndpoint(restTemplate, headers, locations.getFirst(), 200);
                 clearDirectory(locations.getFirst().substring(locations.getFirst().lastIndexOf('/')));
             } catch (HttpStatusCodeException e) {
                 logger.error("HTTP Status code error: {}", e.getStatusCode(), e);
@@ -282,7 +274,7 @@ class Endpoint {
         void validObservation(String parametersFile) {
             HttpHeaders headers = new HttpHeaders();
             headers.add("content-type", "application/fhir+json");
-            testExecutor(parametersFile, "http://localhost:" + port + "/fhir/$extract-data", headers, 200);
+            testExecutor(parametersFile, "http://localhost:" + port + "/fhir/$extract-data", headers);
         }
 
         @Test
@@ -299,12 +291,18 @@ void emptyRequestBodyReturnsBadRequest() {
             assertThat(response.getBody()).contains("Empty request body");
         }
 
-        @ParameterizedTest
-        @ValueSource(strings = {"src/test/resources/CRTDL_Parameters/Parameters_invalid_CRTDL.json"})
-        void invalidCRTDLReturnsValidationException(String parametersFile) {
+        @Test
+        void invalidCRTDLReturnsBadRequest() throws IOException {
+            TestRestTemplate restTemplate = new TestRestTemplate();
             HttpHeaders headers = new HttpHeaders();
             headers.add("content-type", "application/fhir+json");
-            testExecutor(parametersFile, "http://localhost:" + port + "/fhir/$extract-data", headers, 400);
+            String fileContent = Files.readString(Paths.get("src/test/resources/CRTDL_Parameters/Parameters_invalid_CRTDL.json"), StandardCharsets.UTF_8);
+            HttpEntity<String> entity = new HttpEntity<>(fileContent, headers);
+            long start = System.nanoTime();
+            ResponseEntity<String> response = restTemplate.exchange("http://localhost:" + port + "/fhir/$extract-data", HttpMethod.POST, entity, String.class);
+            assertThat(response.getStatusCode().value()).isEqualTo(400);
+            assertThat(durationSecondsSince(start)).isLessThan(1);
         }
+
     }
 }

src/test/java/de/medizininformatikinitiative/torch/rest/FhirControllerTest.java

@@ -1,7 +1,9 @@
 package de.medizininformatikinitiative.torch.rest;
 
 import ca.uhn.fhir.context.FhirContext;
+import de.medizininformatikinitiative.torch.exceptions.ValidationException;
 import de.medizininformatikinitiative.torch.model.crtdl.ExtractDataParameters;
+import de.medizininformatikinitiative.torch.service.CrtdlValidatorService;
 import de.medizininformatikinitiative.torch.service.ExtractDataService;
 import de.medizininformatikinitiative.torch.util.CrtdlFactory;
 import de.medizininformatikinitiative.torch.util.ResultFileManager;
@@ -24,15 +26,13 @@
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.*;
 import static org.mockito.Mockito.when;
 
 @ExtendWith(MockitoExtension.class)
 class FhirControllerTest {
 
-    private final static String BASE_URL = "http://base-url";
+    private static final String BASE_URL = "http://base-url";
 
     @Mock
     ResultFileManager resultFileManager;
@@ -43,12 +43,15 @@ class FhirControllerTest {
     @Mock
     ExtractDataService extractDataService;
 
+    @Mock
+    CrtdlValidatorService validator;
+
     WebTestClient client;
 
     @BeforeEach
     void setup() {
         FhirContext fhirContext = FhirContext.forR4();
-        FhirController fhirController = new FhirController(fhirContext, resultFileManager, extractDataParametersParser, extractDataService, BASE_URL);
+        FhirController fhirController = new FhirController(fhirContext, resultFileManager, extractDataParametersParser, extractDataService, BASE_URL, validator);
         client = WebTestClient.bindToRouterFunction(fhirController.queryRouter()).build();
     }
 
@@ -180,4 +183,21 @@ void blankRequestBodyTriggersBadRequest() {
                     .jsonPath("$.resourceType").isEqualTo("OperationOutcome");
         }
     }
+
+    @Nested
+    class Validator {
+        @Test
+        void invalidCrtdlTriggersBadRequest() throws ValidationException {
+            ExtractDataParameters params = new ExtractDataParameters(CrtdlFactory.empty(), Collections.emptyList());
+            when(extractDataParametersParser.parseParameters(any())).thenReturn(params);
+            when(validator.validate(any())).thenThrow(new ValidationException("Invalid CRTDL"));
+
+            var response = client.post().uri("/fhir/$extract-data").contentType(MediaType.APPLICATION_JSON).bodyValue("{}").exchange();
+
+            response.expectStatus().isBadRequest().expectHeader().contentType("application/fhir+json")
+                    .expectBody()
+                    .jsonPath("$.resourceType").isEqualTo("OperationOutcome")
+                    .jsonPath("$.issue[0].diagnostics").isEqualTo("Invalid CRTDL");
+        }
+    }
 }

src/test/java/de/medizininformatikinitiative/torch/service/ExtractDataServiceTest.java

@@ -1,7 +1,6 @@
 package de.medizininformatikinitiative.torch.service;
 
 import de.medizininformatikinitiative.torch.exceptions.ValidationException;
-import de.medizininformatikinitiative.torch.model.crtdl.Crtdl;
 import de.medizininformatikinitiative.torch.model.crtdl.annotated.AnnotatedCrtdl;
 import de.medizininformatikinitiative.torch.util.ResultFileManager;
 import org.hl7.fhir.r4.model.OperationOutcome;
@@ -24,84 +23,65 @@
 class ExtractDataServiceTest {
 
     private ResultFileManager resultFileManager;
-    private CrtdlValidatorService validatorService;
     private CrtdlProcessingService processingService;
     private ExtractDataService service;
-    private Crtdl crtdl; // mocked
     private AnnotatedCrtdl annotatedCrtdl;
 
     @BeforeEach
     void setUp() {
         resultFileManager = mock(ResultFileManager.class);
-        validatorService = mock(CrtdlValidatorService.class);
         processingService = mock(CrtdlProcessingService.class);
-        crtdl = mock(Crtdl.class);
         annotatedCrtdl = mock(AnnotatedCrtdl.class);
-        service = new ExtractDataService(resultFileManager, validatorService, processingService);
+        service = new ExtractDataService(resultFileManager, processingService);
 
         when(resultFileManager.initJobDir(anyString())).thenReturn(Mono.empty());
         when(resultFileManager.saveErrorToJson(anyString(), any(OperationOutcome.class), any(HttpStatus.class)))
                 .thenReturn(Mono.empty());
     }
 
     @Test
-    void startJob_success() throws ValidationException {
-        when(validatorService.validate(crtdl)).thenReturn(annotatedCrtdl);
+    void startJob_success() {
         when(processingService.process(any(), anyString(), anyList())).thenReturn(Mono.empty());
 
-        StepVerifier.create(service.startJob(crtdl, List.of("p1"), "job-ok"))
+        StepVerifier.create(service.startJob(annotatedCrtdl, List.of("p1"), "job-ok"))
                 .verifyComplete();
 
         verify(resultFileManager).setStatus("job-ok", HttpStatus.ACCEPTED);
         verify(resultFileManager).initJobDir("job-ok");
-        verify(validatorService).validate(crtdl);
         verify(processingService).process(eq(annotatedCrtdl), eq("job-ok"), eq(List.of("p1")));
         verify(resultFileManager).setStatus("job-ok", HttpStatus.OK);
     }
 
     @Test
-    void startJob_illegalArgumentError() throws ValidationException {
-        when(validatorService.validate(crtdl)).thenReturn(annotatedCrtdl);
+    void startJob_illegalArgumentError() {
         when(processingService.process(any(), anyString(), anyList()))
                 .thenReturn(Mono.error(new IllegalArgumentException("bad arg")));
 
-        StepVerifier.create(service.startJob(crtdl, List.of("p1"), "job-bad-arg"))
+        StepVerifier.create(service.startJob(annotatedCrtdl, List.of("p1"), "job-bad-arg"))
                 .verifyComplete();
 
         verify(resultFileManager).saveErrorToJson(eq("job-bad-arg"), any(OperationOutcome.class), eq(HttpStatus.BAD_REQUEST));
     }
 
     @Test
-    void startJob_validationExceptionError() throws ValidationException {
-        when(validatorService.validate(crtdl)).thenReturn(annotatedCrtdl);
+    void startJob_validationExceptionError() {
         when(processingService.process(any(), anyString(), anyList()))
                 .thenReturn(Mono.error(new ValidationException("validation failed")));
 
-        StepVerifier.create(service.startJob(crtdl, List.of("p1"), "job-validation-fail"))
+        StepVerifier.create(service.startJob(annotatedCrtdl, List.of("p1"), "job-validation-fail"))
                 .verifyComplete();
 
         verify(resultFileManager).saveErrorToJson(eq("job-validation-fail"), any(OperationOutcome.class), eq(HttpStatus.BAD_REQUEST));
     }
 
     @Test
-    void startJob_runtimeError() throws ValidationException {
-        when(validatorService.validate(crtdl)).thenReturn(annotatedCrtdl);
+    void startJob_runtimeError() {
         when(processingService.process(any(), anyString(), anyList()))
                 .thenReturn(Mono.error(new RuntimeException("unexpected")));
 
-        StepVerifier.create(service.startJob(crtdl, List.of("p1"), "job-runtime-fail"))
+        StepVerifier.create(service.startJob(annotatedCrtdl, List.of("p1"), "job-runtime-fail"))
                 .verifyComplete();
 
         verify(resultFileManager).saveErrorToJson(eq("job-runtime-fail"), any(OperationOutcome.class), eq(HttpStatus.INTERNAL_SERVER_ERROR));
     }
-
-    @Test
-    void startJob_validatorThrows() throws ValidationException {
-        when(validatorService.validate(crtdl)).thenThrow(new ValidationException("invalid"));
-
-        StepVerifier.create(service.startJob(crtdl, List.of("p1"), "job-validator-throw"))
-                .verifyComplete();
-
-        verify(resultFileManager).saveErrorToJson(eq("job-validator-throw"), any(OperationOutcome.class), eq(HttpStatus.BAD_REQUEST));
-    }
 }