From d64fb72f766c8ebf53444ce026256a2f8fe7525f Mon Sep 17 00:00:00 2001 From: "M. Morgan Aster" Date: Wed, 31 Jan 2024 18:26:25 -0500 Subject: [PATCH] TSPS-136 Require a JobControl object containing a job id when creating a new job (#52) --- common/openapi.yml | 7 +- .../controller/GlobalExceptionHandler.java | 52 +++++- .../controller/PipelinesApiController.java | 8 +- .../MissingRequiredFieldsException.java | 11 -- .../pipelines/common/utils/FlightUtils.java | 10 +- .../dependencies/stairway/JobService.java | 2 +- .../pipelines/service/ImputationService.java | 13 +- .../common/utils/FlightUtilsTest.java | 10 +- .../StairwayDatabaseConfigurationTest.java | 4 +- .../PipelinesApiControllerTest.java | 174 ++++++++++++++---- .../service/ImputationServiceMockTest.java | 2 +- .../service/ImputationServiceTest.java | 10 - 12 files changed, 222 insertions(+), 81 deletions(-) delete mode 100644 service/src/main/java/bio/terra/pipelines/common/exception/MissingRequiredFieldsException.java diff --git a/common/openapi.yml b/common/openapi.yml index f673a76d..d202c227 100644 --- a/common/openapi.yml +++ b/common/openapi.yml @@ -341,8 +341,9 @@ components: properties: id: description: >- - Unique identifier for the job. Caller-provided UUID. + Required unique identifier (UUID) for the job. type: string + format: uuid # In the future, notification configuration will also be part of JobControl. JobResult: @@ -361,12 +362,14 @@ components: description: | Object containing the user-provided information about a job request. type: object - required: [ pipelineVersion, pipelineInputs ] + required: [ jobControl, pipelineVersion, pipelineInputs ] properties: description: description: >- User-provided description of the job request. type: string + jobControl: + $ref: "#/components/schemas/JobControl" pipelineVersion: $ref: "#/components/schemas/PipelineVersion" pipelineInputs: diff --git a/service/src/main/java/bio/terra/pipelines/app/controller/GlobalExceptionHandler.java b/service/src/main/java/bio/terra/pipelines/app/controller/GlobalExceptionHandler.java index 1a541a03..a863f5d3 100644 --- a/service/src/main/java/bio/terra/pipelines/app/controller/GlobalExceptionHandler.java +++ b/service/src/main/java/bio/terra/pipelines/app/controller/GlobalExceptionHandler.java @@ -2,8 +2,11 @@ import bio.terra.common.exception.ErrorReportException; import bio.terra.pipelines.generated.model.ApiErrorReport; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import javax.validation.constraints.NotNull; +import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; @@ -33,9 +36,7 @@ public ResponseEntity errorReportHandler(ErrorReportException ex // -- validation exceptions - we don't control the exception raised @ExceptionHandler({ - MethodArgumentNotValidException.class, MethodArgumentTypeMismatchException.class, - HttpMessageNotReadableException.class, HttpRequestMethodNotSupportedException.class, IllegalArgumentException.class, NoHandlerFoundException.class @@ -56,6 +57,53 @@ public ResponseEntity validationExceptionHandler(Exception ex) { return new ResponseEntity<>(errorReport, HttpStatus.BAD_REQUEST); } + @ExceptionHandler({MethodArgumentNotValidException.class}) + public ResponseEntity argumentNotValidExceptionHandler( + MethodArgumentNotValidException ex) { + logger.debug("MethodArgumentNotValid exception caught by global exception handler", ex); + // For security reasons, we generally don't want to include the user's invalid (and potentially + // malicious) input in the error response, which also means we don't include the full exception. + // Instead, we return a generic error message about input validation. + List errors = new ArrayList<>(); + ex.getBindingResult() + .getFieldErrors() + .forEach( + error -> { + String fieldName = error.getField(); + String errorMessage = error.getDefaultMessage(); + errors.add(fieldName + " " + errorMessage); + }); + Collections.sort(errors); // sort alphabetically to make testing easier + String validationErrorMessage = + "Request could not be parsed or was invalid: " + String.join("; ", errors); + ApiErrorReport errorReport = + new ApiErrorReport() + .message(validationErrorMessage) + .statusCode(HttpStatus.BAD_REQUEST.value()); + return new ResponseEntity<>(errorReport, HttpStatus.BAD_REQUEST); + } + + @ExceptionHandler({HttpMessageNotReadableException.class}) + public ResponseEntity httpMessageNotReadableExceptionHandler( + HttpMessageNotReadableException ex) { + logger.debug( + "HttpMessageNotReadableException exception caught by global exception handler", ex); + + // Extract the top-level error message without the nested exceptions + String message = ex.getMessage(); + String validationErrorMessage = message; + final int tailIndex = StringUtils.indexOf(message, "; nested exception is"); + if (tailIndex != -1) { + validationErrorMessage = StringUtils.left(message, tailIndex); + } + + ApiErrorReport errorReport = + new ApiErrorReport() + .message(validationErrorMessage) + .statusCode(HttpStatus.BAD_REQUEST.value()); + return new ResponseEntity<>(errorReport, HttpStatus.BAD_REQUEST); + } + // Note we don't use @retryable yet, but we anticipate using it later so leaving it for now // Exception thrown by Spring Retry code when interrupted. @ExceptionHandler({BackOffInterruptedException.class}) diff --git a/service/src/main/java/bio/terra/pipelines/app/controller/PipelinesApiController.java b/service/src/main/java/bio/terra/pipelines/app/controller/PipelinesApiController.java index 9f949291..a339ab3c 100644 --- a/service/src/main/java/bio/terra/pipelines/app/controller/PipelinesApiController.java +++ b/service/src/main/java/bio/terra/pipelines/app/controller/PipelinesApiController.java @@ -119,6 +119,8 @@ public ResponseEntity createJob( @PathVariable("pipelineId") String pipelineId, @RequestBody ApiCreateJobRequestBody body) { final SamUser userRequest = getAuthenticatedInfo(); String userId = userRequest.getSubjectId(); + UUID jobId = body.getJobControl().getId(); + String description = body.getDescription(); String pipelineVersion = body.getPipelineVersion(); Object pipelineInputs = body.getPipelineInputs(); @@ -126,10 +128,10 @@ public ResponseEntity createJob( PipelinesEnum validatedPipelineId = validatePipelineId(pipelineId); logger.info( - "Creating {} pipeline job (version {}) for {} user {} with inputs {}", + "Creating {} pipeline (version {}) job (id {}) for user {} with inputs {}", pipelineId, pipelineVersion, - userRequest.getEmail(), + jobId, userId, pipelineInputs); @@ -142,7 +144,7 @@ public ResponseEntity createJob( createdJobUuid = imputationService.createImputationJob( - userId, description, pipelineVersion, pipelineInputs); + jobId, userId, description, pipelineVersion, pipelineInputs); } else { logger.error("Unknown validatedPipelineId {}", validatedPipelineId); throw new ApiException("An internal error occurred."); diff --git a/service/src/main/java/bio/terra/pipelines/common/exception/MissingRequiredFieldsException.java b/service/src/main/java/bio/terra/pipelines/common/exception/MissingRequiredFieldsException.java deleted file mode 100644 index 5222df73..00000000 --- a/service/src/main/java/bio/terra/pipelines/common/exception/MissingRequiredFieldsException.java +++ /dev/null @@ -1,11 +0,0 @@ -package bio.terra.pipelines.common.exception; - -import bio.terra.common.exception.InternalServerErrorException; - -/** When you can't get there from here, but somehow end up there */ -@SuppressWarnings("java:S110") // Disable "Inheritance tree of classes should not be too deep" -public class MissingRequiredFieldsException extends InternalServerErrorException { - public MissingRequiredFieldsException(String message) { - super(message); - } -} diff --git a/service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java b/service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java index c4ed8a0d..9170372f 100644 --- a/service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java +++ b/service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java @@ -1,6 +1,6 @@ package bio.terra.pipelines.common.utils; -import bio.terra.pipelines.common.exception.MissingRequiredFieldsException; +import bio.terra.common.exception.MissingRequiredFieldException; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; import bio.terra.pipelines.generated.model.ApiErrorReport; import bio.terra.stairway.FlightContext; @@ -76,7 +76,7 @@ public static T getInputParameterOrWorkingValue( public static void validateRequiredEntries(FlightMap flightMap, String... keys) { for (String key : keys) { if (null == flightMap.getRaw(key)) { - throw new MissingRequiredFieldsException( + throw new MissingRequiredFieldException( String.format("Required entry with key %s missing from flight map.", key)); } } @@ -87,7 +87,7 @@ public static FlightMap getResultMapRequired(FlightState flightState) { .getResultMap() .orElseThrow( () -> - new MissingRequiredFieldsException( + new MissingRequiredFieldException( String.format( "ResultMap is missing for flight %s", flightState.getFlightId()))); } @@ -127,7 +127,7 @@ public static String getFlightErrorMessage(FlightState flightState) { public static T getRequired(FlightMap flightMap, String key, Class tClass) { var value = flightMap.get(key, tClass); if (value == null) { - throw new MissingRequiredFieldsException("Missing required flight map key: " + key); + throw new MissingRequiredFieldException("Missing required flight map key: " + key); } return value; } @@ -144,7 +144,7 @@ public static T getRequired(FlightMap flightMap, String key, Class tClass public static T getRequired(FlightMap flightMap, String key, TypeReference typeReference) { var value = flightMap.get(key, typeReference); if (value == null) { - throw new MissingRequiredFieldsException("Missing required flight map key: " + key); + throw new MissingRequiredFieldException("Missing required flight map key: " + key); } return value; } diff --git a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java index e704858f..3f984e76 100644 --- a/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java +++ b/service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java @@ -75,7 +75,7 @@ protected UUID submit(Class flightClass, FlightMap parameterMa // behavior of flights. logger.warn("Received duplicate job ID: {}", jobIdString); throw new DuplicateJobIdException( - String.format("Received duplicate jobId %s", jobIdString), ex); + String.format("Received duplicate jobControl.id %s", jobIdString), ex); } catch (StairwayException stairwayEx) { throw new InternalStairwayException(stairwayEx); } catch (InterruptedException e) { diff --git a/service/src/main/java/bio/terra/pipelines/service/ImputationService.java b/service/src/main/java/bio/terra/pipelines/service/ImputationService.java index 3801b562..f1228b06 100644 --- a/service/src/main/java/bio/terra/pipelines/service/ImputationService.java +++ b/service/src/main/java/bio/terra/pipelines/service/ImputationService.java @@ -77,13 +77,17 @@ public class ImputationService { * @see ImputationJob */ public UUID createImputationJob( - String userId, String description, String pipelineVersion, Object pipelineInputs) { + UUID jobId, + String userId, + String description, + String pipelineVersion, + Object pipelineInputs) { logger.info("Create new imputation version {} job for user {}", pipelineVersion, userId); JobBuilder jobBuilder = jobService .newJob() - .jobId(createJobId()) + .jobId(jobId) .flightClass(RunImputationJobFlight.class) .addParameter(JobMapKeys.PIPELINE_ID.getKeyName(), PipelinesEnum.IMPUTATION) .addParameter(JobMapKeys.USER_ID.getKeyName(), userId) @@ -94,11 +98,6 @@ public UUID createImputationJob( return jobBuilder.submit(); } - // TSPS-136 will require that the user provide the job UUID, and should remove this method - protected UUID createJobId() { - return UUID.randomUUID(); - } - @Transactional public UUID writeJobToDb( UUID jobUuid, String userId, String pipelineVersion, Object pipelineInputs) { diff --git a/service/src/test/java/bio/terra/pipelines/common/utils/FlightUtilsTest.java b/service/src/test/java/bio/terra/pipelines/common/utils/FlightUtilsTest.java index 5bbf07b2..24612cef 100644 --- a/service/src/test/java/bio/terra/pipelines/common/utils/FlightUtilsTest.java +++ b/service/src/test/java/bio/terra/pipelines/common/utils/FlightUtilsTest.java @@ -3,7 +3,7 @@ import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.when; -import bio.terra.pipelines.common.exception.MissingRequiredFieldsException; +import bio.terra.common.exception.MissingRequiredFieldException; import bio.terra.pipelines.dependencies.stairway.JobMapKeys; import bio.terra.pipelines.dependencies.stairway.exception.InvalidResultStateException; import bio.terra.pipelines.generated.model.ApiErrorReport; @@ -103,7 +103,7 @@ void validateRequiredEntries_missingRequiredKey() { flightMap.put(requiredKey1, "value1"); assertThrows( - MissingRequiredFieldsException.class, + MissingRequiredFieldException.class, () -> FlightUtils.validateRequiredEntries(flightMap, requiredKey1, requiredKey2)); } @@ -122,7 +122,7 @@ void getResultMapRequired_missingResultMap() { FlightState flightState = new FlightState(); assertThrows( - MissingRequiredFieldsException.class, () -> FlightUtils.getResultMapRequired(flightState)); + MissingRequiredFieldException.class, () -> FlightUtils.getResultMapRequired(flightState)); } @Test @@ -165,7 +165,7 @@ void getRequired_class_fail() { FlightMap flightMap = new FlightMap(); assertThrows( - MissingRequiredFieldsException.class, + MissingRequiredFieldException.class, () -> FlightUtils.getRequired(flightMap, "key", String.class)); } @@ -186,7 +186,7 @@ void getRequired_typeRef_fail() { TypeReference typeReference = new TypeReference<>() {}; assertThrows( - MissingRequiredFieldsException.class, + MissingRequiredFieldException.class, () -> FlightUtils.getRequired(flightMap, "key", typeReference)); } diff --git a/service/src/test/java/bio/terra/pipelines/configuration/internal/StairwayDatabaseConfigurationTest.java b/service/src/test/java/bio/terra/pipelines/configuration/internal/StairwayDatabaseConfigurationTest.java index e1cff77d..fa934949 100644 --- a/service/src/test/java/bio/terra/pipelines/configuration/internal/StairwayDatabaseConfigurationTest.java +++ b/service/src/test/java/bio/terra/pipelines/configuration/internal/StairwayDatabaseConfigurationTest.java @@ -3,7 +3,7 @@ import static org.junit.jupiter.api.Assertions.*; import bio.terra.pipelines.app.configuration.internal.StairwayDatabaseConfiguration; -import bio.terra.pipelines.testutils.BaseTest; +import bio.terra.pipelines.testutils.BaseEmbeddedDbTest; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; @@ -13,7 +13,7 @@ "spring.main.lazy-initialization=true", "datasource.testWithEmbeddedDatabase=false" }) -class StairwayDatabaseConfigurationTest extends BaseTest { +class StairwayDatabaseConfigurationTest extends BaseEmbeddedDbTest { @Autowired StairwayDatabaseConfiguration stairwayDatabaseConfiguration; diff --git a/service/src/test/java/bio/terra/pipelines/controller/PipelinesApiControllerTest.java b/service/src/test/java/bio/terra/pipelines/controller/PipelinesApiControllerTest.java index 48f3ea08..14b04a2b 100644 --- a/service/src/test/java/bio/terra/pipelines/controller/PipelinesApiControllerTest.java +++ b/service/src/test/java/bio/terra/pipelines/controller/PipelinesApiControllerTest.java @@ -41,9 +41,12 @@ import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.http.MediaType; +import org.springframework.http.converter.HttpMessageNotReadableException; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; +import org.springframework.test.web.servlet.result.MockMvcResultMatchers; +import org.springframework.web.bind.MethodArgumentNotValidException; @ContextConfiguration(classes = {PipelinesApiController.class, GlobalExceptionHandler.class}) @WebMvcTest @@ -63,6 +66,7 @@ class PipelinesApiControllerTest { private final SamUser testUser = MockMvcUtils.TEST_SAM_USER; private final String testPipelineVersion = TestUtils.TEST_PIPELINE_VERSION_1; private final Object testPipelineInputs = TestUtils.TEST_PIPELINE_INPUTS; + private final UUID newJobId = TestUtils.TEST_NEW_UUID; @BeforeEach void beforeEach() { @@ -137,20 +141,19 @@ void getPipeline_badPipeline() throws Exception { .andExpect(status().isBadRequest()) .andExpect( result -> - assertTrue(result.getResolvedException() instanceof InvalidPipelineException)); + assertInstanceOf(InvalidPipelineException.class, result.getResolvedException())); } @Test void testCreateJobImputationPipelineRunning() throws Exception { String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); String description = "description for testCreateJobImputationPipelineRunning"; - String postBodyAsJson = createTestJobPostBody(description); - - UUID jobId = UUID.randomUUID(); // newJobId + UUID jobId = newJobId; + String postBodyAsJson = createTestJobPostBody(jobId.toString(), description); // the mocks when(imputationService.createImputationJob( - testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) + jobId, testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) .thenReturn(jobId); when(jobServiceMock.retrieveJob(jobId, testUser.getSubjectId())) .thenReturn( @@ -175,15 +178,14 @@ void testCreateJobImputationPipelineRunning() throws Exception { } @Test - void testCreateJobImputationPipelineNullDescription() throws Exception { + void testCreateJobImputationPipelineNoDescriptionOk() throws Exception { String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); - String postBodyAsJson = createTestJobPostBody(null); - - UUID jobId = UUID.randomUUID(); // newJobId + UUID jobId = newJobId; + String postBodyAsJson = createTestJobPostBody(jobId.toString(), ""); // the mocks when(imputationService.createImputationJob( - testUser.getSubjectId(), null, testPipelineVersion, testPipelineInputs)) + jobId, testUser.getSubjectId(), "", testPipelineVersion, testPipelineInputs)) .thenReturn(jobId); when(jobServiceMock.retrieveJob(jobId, testUser.getSubjectId())) .thenReturn( @@ -211,13 +213,12 @@ void testCreateJobImputationPipelineNullDescription() throws Exception { void testCreateJobImputationPipelineCompletedSuccess() throws Exception { String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); String description = "description for testCreateJobImputationPipelineCompletedSuccess"; - String postBodyAsJson = createTestJobPostBody(description); - - UUID jobId = UUID.randomUUID(); // newJobId + UUID jobId = newJobId; + String postBodyAsJson = createTestJobPostBody(jobId.toString(), description); // the mocks when(imputationService.createImputationJob( - testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) + jobId, testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) .thenReturn(jobId); when(jobServiceMock.retrieveJob(jobId, testUser.getSubjectId())) .thenReturn( @@ -241,26 +242,16 @@ void testCreateJobImputationPipelineCompletedSuccess() throws Exception { assertEquals(ApiJobReport.StatusEnum.SUCCEEDED, response.getJobReport().getStatus()); } - private String createTestJobPostBody(String description) throws JsonProcessingException { - ApiCreateJobRequestBody postBody = - new ApiCreateJobRequestBody() - .pipelineVersion(testPipelineVersion) - .pipelineInputs(testPipelineInputs) - .description(description); - return MockMvcUtils.convertToJsonString(postBody); - } - @Test void testCreateJobImputationPipelineCaseInsensitive() throws Exception { String pipelineIdString = "iMpUtAtIoN"; String description = "description for testCreateJobImputationPipelineCaseInsensitive"; - String postBodyAsJson = createTestJobPostBody(description); - - UUID jobId = UUID.randomUUID(); // newJobId + UUID jobId = newJobId; + String postBodyAsJson = createTestJobPostBody(jobId.toString(), description); // the mocks when(imputationService.createImputationJob( - testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) + jobId, testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) .thenReturn(jobId); when(jobServiceMock.retrieveJob(jobId, testUser.getSubjectId())) .thenReturn( @@ -287,8 +278,32 @@ void testCreateJobImputationPipelineCaseInsensitive() throws Exception { void testCreateJobBadPipeline() throws Exception { String pipelineIdString = "bad-pipeline-id"; String description = "description for testCreateJobBadPipeline"; - String postBodyAsJson = createTestJobPostBody(description); + UUID jobId = newJobId; + String postBodyAsJson = createTestJobPostBody(jobId.toString(), description); + + mockMvc + .perform( + post(String.format("/api/pipelines/v1alpha1/%s", pipelineIdString)) + .contentType(MediaType.APPLICATION_JSON) + .content(postBodyAsJson)) + .andExpect(status().isBadRequest()) + .andExpect( + result -> + assertInstanceOf(InvalidPipelineException.class, result.getResolvedException())); + } + + @Test + void testCreateJobMissingJobControl() throws Exception { + String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); + ApiCreateJobRequestBody postBody = + new ApiCreateJobRequestBody() + .pipelineVersion(testPipelineVersion) + .pipelineInputs(testPipelineInputs) + .description("description for testCreateJobMissingJobId"); + String postBodyAsJson = MockMvcUtils.convertToJsonString(postBody); + // Spring will catch the missing jobControl and invoke the GlobalExceptionHandler + // before it gets to the controller mockMvc .perform( post(String.format("/api/pipelines/v1alpha1/%s", pipelineIdString)) @@ -297,18 +312,105 @@ void testCreateJobBadPipeline() throws Exception { .andExpect(status().isBadRequest()) .andExpect( result -> - assertTrue(result.getResolvedException() instanceof InvalidPipelineException)); + assertInstanceOf( + MethodArgumentNotValidException.class, result.getResolvedException())) + .andExpect( + MockMvcResultMatchers.jsonPath("$.message") + .value("Request could not be parsed or was invalid: jobControl must not be null")); + } + + @Test + void testCreateJobMissingJobId() throws Exception { + String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); + ApiJobControl apiJobControl = new ApiJobControl(); + ApiCreateJobRequestBody postBody = + new ApiCreateJobRequestBody() + .jobControl(apiJobControl) + .pipelineVersion(testPipelineVersion) + .pipelineInputs(testPipelineInputs) + .description("description for testCreateJobMissingJobId"); + String postBodyAsJson = MockMvcUtils.convertToJsonString(postBody); + + // Spring will catch the missing job id and invoke the GlobalExceptionHandler + // before it gets to the controller + mockMvc + .perform( + post(String.format("/api/pipelines/v1alpha1/%s", pipelineIdString)) + .contentType(MediaType.APPLICATION_JSON) + .content(postBodyAsJson)) + .andExpect(status().isBadRequest()) + .andExpect( + result -> + assertInstanceOf( + MethodArgumentNotValidException.class, result.getResolvedException())) + .andExpect( + MockMvcResultMatchers.jsonPath("$.message") + .value( + "Request could not be parsed or was invalid: jobControl.id must not be null")); + } + + @Test + void testCreateJobMissingMultipleRequiredFields() throws Exception { + String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); + String stringifiedInputs = MockMvcUtils.convertToJsonString(testPipelineInputs); + String postBodyAsJson = + String.format( + "{\"pipelineInputs\":%s,\"description\":\"test description for testCreateJobMissingMultipleRequiredFields\"}", + stringifiedInputs); + + // Spring will catch the missing fields and invoke the GlobalExceptionHandler + // before it gets to the controller + mockMvc + .perform( + post(String.format("/api/pipelines/v1alpha1/%s", pipelineIdString)) + .contentType(MediaType.APPLICATION_JSON) + .content(postBodyAsJson)) + .andExpect(status().isBadRequest()) + .andExpect( + result -> + assertInstanceOf( + MethodArgumentNotValidException.class, result.getResolvedException())) + .andExpect( + MockMvcResultMatchers.jsonPath("$.message") + .value( + "Request could not be parsed or was invalid: jobControl must not be null; " + + "pipelineVersion must not be null")); + } + + @Test + void testCreateJobBadJobId() throws Exception { + String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); + String postBodyAsJson = + createTestJobPostBody("this-is-not-a-uuid", "description for testCreateJobMissingJobId"); + + // Spring will catch the non-uuid pipelineId and invoke the GlobalExceptionHandler + // before it gets to the controller + mockMvc + .perform( + post(String.format("/api/pipelines/v1alpha1/%s", pipelineIdString)) + .contentType(MediaType.APPLICATION_JSON) + .content(postBodyAsJson)) + .andExpect(status().isBadRequest()) + .andExpect( + result -> + assertInstanceOf( + HttpMessageNotReadableException.class, result.getResolvedException())) + .andExpect( + MockMvcResultMatchers.jsonPath("$.message") + .value( + "JSON parse error: Cannot deserialize value of type `java.util.UUID` from String \"this-is-not-a-uuid\": UUID has to be represented by standard 36-char representation")); } @Test void testCreateImputationJobStairwayError() throws Exception { String pipelineIdString = PipelinesEnum.IMPUTATION.getValue(); String description = "description for testCreateImputationJobStairwayError"; - String postBodyAsJson = createTestJobPostBody(description); + UUID jobId = newJobId; + String postBodyAsJson = createTestJobPostBody(jobId.toString(), description); // the mocks - one error that can happen is a MissingRequiredFieldException from Stairway when(imputationService.createImputationJob( - testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) + jobId, testUser.getSubjectId(), description, testPipelineVersion, testPipelineInputs)) .thenThrow(new InternalStairwayException("some message")); mockMvc @@ -385,6 +487,14 @@ void testGetPipelineJobs_badPipeline() throws Exception { .andExpect(status().isBadRequest()) .andExpect( result -> - assertTrue(result.getResolvedException() instanceof InvalidPipelineException)); + assertInstanceOf(InvalidPipelineException.class, result.getResolvedException())); + } + + private String createTestJobPostBody(String jobId, String description) + throws JsonProcessingException { + String stringifiedInputs = MockMvcUtils.convertToJsonString(testPipelineInputs); + return String.format( + "{\"jobControl\":{\"id\":\"%s\"},\"pipelineVersion\":\"%s\",\"pipelineInputs\":%s,\"description\":\"%s\"}", + jobId, testPipelineVersion, stringifiedInputs, description); } } diff --git a/service/src/test/java/bio/terra/pipelines/service/ImputationServiceMockTest.java b/service/src/test/java/bio/terra/pipelines/service/ImputationServiceMockTest.java index 82f2f9b3..768b2998 100644 --- a/service/src/test/java/bio/terra/pipelines/service/ImputationServiceMockTest.java +++ b/service/src/test/java/bio/terra/pipelines/service/ImputationServiceMockTest.java @@ -101,7 +101,7 @@ void testCreateJob_success() { // note this doesn't actually kick off a job UUID writtenUUID = imputationService.createImputationJob( - testUserId, "test description", testPipelineVersion, testPipelineInputs); + testUUID, testUserId, "test description", testPipelineVersion, testPipelineInputs); assertEquals(testUUID, writtenUUID); } } diff --git a/service/src/test/java/bio/terra/pipelines/service/ImputationServiceTest.java b/service/src/test/java/bio/terra/pipelines/service/ImputationServiceTest.java index b2b11f08..7b20563f 100644 --- a/service/src/test/java/bio/terra/pipelines/service/ImputationServiceTest.java +++ b/service/src/test/java/bio/terra/pipelines/service/ImputationServiceTest.java @@ -34,16 +34,6 @@ private ImputationJob createTestJobWithJobIdAndUser(UUID jobId, String userId) { return new ImputationJob(jobId, userId, testPipelineVersion); } - @Test - void testCreateJobId() { - UUID jobId1 = imputationService.createJobId(); - UUID jobId2 = imputationService.createJobId(); - - assertNotNull(jobId1); - assertNotNull(jobId2); - assertNotEquals(jobId1, jobId2); - } - @Test void testWriteJobToDb() { List jobsDefault = imputationJobsRepository.findAllByUserId(testUserId);