TSPS-136 Require a JobControl object containing a job id when creatin…

…g a new job (#52)

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:

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<ApiErrorReport> 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<ApiErrorReport> validationExceptionHandler(Exception ex) {
     return new ResponseEntity<>(errorReport, HttpStatus.BAD_REQUEST);
   }
 
+  @ExceptionHandler({MethodArgumentNotValidException.class})
+  public ResponseEntity<ApiErrorReport> 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<String> 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<ApiErrorReport> 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})

service/src/main/java/bio/terra/pipelines/app/controller/PipelinesApiController.java

@@ -119,17 +119,19 @@ public ResponseEntity<ApiCreateJobResponse> 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();
 
     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<ApiCreateJobResponse> 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.");

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> 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> T getRequired(FlightMap flightMap, String key, Class<T> 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> T getRequired(FlightMap flightMap, String key, Class<T> tClass
   public static <T> T getRequired(FlightMap flightMap, String key, TypeReference<T> 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;
   }

service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobService.java

@@ -75,7 +75,7 @@ protected UUID submit(Class<? extends Flight> 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) {

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) {

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<Object> typeReference = new TypeReference<>() {};
 
     assertThrows(
-        MissingRequiredFieldsException.class,
+        MissingRequiredFieldException.class,
         () -> FlightUtils.getRequired(flightMap, "key", typeReference));
   }
 

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;