TSPS-123 Refactor to follow DSP's Async API best practices #48

mmorgantaylor · 2024-01-09T22:47:46Z

Description

This PR brings our service closer to being in line with DSP's Asynchronous API best practices doc. Remaining work for this goal that is not included in this PR is encapsulated in the following tickets:

TSPS-135: Implement GET imputation/result/{id} endpoint
TSPS-136: Require the caller to provide the job UUID
TSPS-150: Write tests for JobApiUtils

The plan of changes can be found in this doc.

The changes in this PR are as follows:

API endpoint and controller changes:

Create the following API endpoints:
- POST /api/pipelines/v1alpha1/{pipelineId} to create a new pipeline of type {pipelineId} e.g. imputation. The response to this request now contains a JobControl object (specified by the best practices doc) that contains the jobId, rather than just the jobId.
- GET /api/pipelines/v1alpha1/{pipelineId}/jobs to retrieve all pipeline-specific jobs requested by user.
- GET /api/job/v1alpha1/jobs to retrieve all jobs (across pipelines) requested by user
- GET /api/job/v1alpha1/jobs/{jobId} to retrieve a specified job
Note that the creation of a new job has moved from JobsApiController to PipelinesApiController, which checks which pipeline is requested (currently the only possible one is imputation) and then calls the appropriate service (ImputationService) to create the job.

Database changes:

The TSPS database's jobs table has been renamed to imputation_jobs (and future pipelines will get their own tables), and fields in this database that are captured in Stairway's database are removed (timestamps, status, and pipelineId). Currently, imputation_jobs stores the internal id, the jobId for the flight, the pipeline version, and the userId of the submitting user.
Note that the pipeline_inputs table remains as-is and is not imputation-specific. We may make the inputs table pipeline-specific in future but this was not needed in this PR.

Stairway-related changes:

Stairway classes refactored e.g. StairwayJobService and StairwayJobBuilder are now JobService and JobBuilder
Stairway JobBuilder now requires userId and pipelineId for all flights. It also requires a jobId (flightId) input, though the work to make the user supply this rather than the service is still to be completed in TSPS-136.
Stairway JobService now performs job lookups, both for getting all jobs and for getting specific jobs. It always checks that the jobs returned were submitted by the requesting user.
Add JobApiUtils class that contains useful methods for converting Stairway job objects (EnumeratedJobs, FlightStates, etc) to API response objects. Note that the mapFlightStatusToApi method groups the finer-grained Stairway statuses into three user-facing statuses for a job: RUNNING, SUCCEEDED, FAILED.

Other:

Create PipelinesEnum to encompass validated pipelines. Note that there is a test in PipelinesServiceTest to ensure that every pipeline in PipelinesEnum has a corresponding row in the pipelines repository pipelines table.

Jira Ticket

https://broadworkbench.atlassian.net/browse/TSPS-123

…his and PipelinesEnum in general

jsotobroad

were there any tests that you werent able to recapitulate with all of the refactoring you did or any parts of the code you wanted to test that felt overly difficult to test? The coverage looks great but was just curious.

jsotobroad · 2024-01-17T22:01:05Z

common/openapi.yml

          $ref: '#/components/responses/ServerError'
    post:
-      summary: Create new job request
+      summary: Create new pipeline job


just to make sure im on the right track, eventually this will require a jobid to be passed in, in which case we will also need to add a 202 reponse code when we do?

that's right - TSPS-136 covers requiring the jobid in the payload here. good catch about the 202 - that ought to be part of this PR, will add it.

jsotobroad · 2024-01-17T22:02:03Z

common/openapi.yml

          $ref: '#/components/responses/PermissionDenied'
-        '404':
+        404:


shouldnt need 403 or 404 here if we're just returning the list of jobs

good catch, thank you!

jsotobroad · 2024-01-17T22:09:33Z

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

@@ -58,4 +97,82 @@ static ApiPipeline pipelineToApi(Pipeline pipelineInfo) {
        .displayName(pipelineInfo.getDisplayName())
        .description(pipelineInfo.getDescription());
  }
+
+  // Pipelines jobs


probably worth expanding on this comment since its probably the most integral endpoint we have

added descriptions to each controller method below - is this what you had in mind, or is there still missing context?

jsotobroad · 2024-01-17T22:13:05Z

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

-    ApiGetJobResponse result = jobToApi(job);
-
+    logger.info("Retrieving jobId {} for userId {}", jobId, userId);
+    FlightState flightState = stairwayJobService.retrieveJob(jobId, userId);


how are we making sure that we are correctly handling 403s (dont have permission to see job id being asked for) and 404s (job id doesnt exist)

discussed in Slack - had been overly cautious with returning 404s instead of 403s, but this isn't necessary. should be returning 403s properly now.

service/src/main/java/bio/terra/pipelines/common/utils/PipelinesEnum.java

jsotobroad · 2024-01-17T22:41:32Z

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

@@ -19,9 +22,4 @@ public enum StairwayJobMapKeys {
  public String getKeyName() {
    return keyName;
  }
-
-  public static boolean isRequiredKey(String keyName) {


is this something that isnt useful to have? I can imagine a place where we want to get a list of required keys. Was it not being used before?

ugh, agreed, but the place I wanted to use it (when setting up a new flight) is written in a way that wasn't obvious how to use this. (it was not being used before; it existed in WSM but isn't used in that repo.) but I'm going to take another look, because this is a much clearer way to specify the required keys than what we have.

jsotobroad · 2024-01-17T22:44:20Z

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

+      FlightState result = stairwayComponent.get().getFlightState(jobId.toString());
+      // Note: after implementing TSPS-134, we can filter by flightId in enumerateJobs and remove
+      // the following check
+      if (!userId.equals(


so i wonder if this scenario shoudl return a job not found or a "forbidden" exception. Also at some point we will be doing these checks at the workspace level as opposed to the user level. Is that something we want to do now or do it in another round of refactors

re: checks at workspace level, I'd say make that change in the future if we do it, since that's only applicable if users are submitting data from a workspace, versus uploading the data in the no-BP model. or am i missing something?

jsotobroad · 2024-01-17T22:46:30Z

service/src/main/resources/db/changesets/20240104.yaml

+            - column:
+                name: time_completed
+            - column:
+                name: status


nice! we can clean up the pipelines table here if we want.

good call, done!

…at of response to createJob

…, remove request from jobmapkeys

…r more exceptions in JobService

jsotobroad · 2024-01-23T15:08:48Z

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

+          "Missing required field for flight construction: jobId");
+    }
+
+    for (JobMapKeys key : JobMapKeys.values()) {


It might be a bit more efficient if we change isRequiredKey to be collection of values. Then we can loop through that collection of values and check against jobParameterMap to see if they exist.

Also usually with validations like this you'd want to gather all failures at the same time as opposed to returning the first failure you see. I think this is pretty much a check to be internally consistent so probably not a big deal but when we start validing user inputs and stuff we'll want to do that instead.

great points, will do

jsotobroad · 2024-01-23T15:14:39Z

service/src/main/java/bio/terra/pipelines/service/ImputationService.java

+
+  // only used in tests
+  @VisibleForTesting
+  public ImputationJob getImputationJob(UUID jobId, String userId) {


do we need this function and the one below it if theyre only used for testing? cant we just call the imputationJobsRepository in the tests directly?

🤦 sure can, thanks!

jsotobroad · 2024-01-23T15:15:03Z

service/src/main/java/bio/terra/pipelines/service/ImputationService.java

+   *     following related classes:
+   * @see ImputationJob
+   */
+  @Transactional


does this need to be transactional. I'm assuming this is just doing one write to the stairway database

hmm at this time that's true. in future we might be doing more that might merit its being transactional, but happy to take this out for now

…ation

sonarqubecloud · 2024-01-23T16:55:50Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
88.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mmorgantaylor added 20 commits January 9, 2024 17:46

first pass at refactor, more still to do

64933bd

wip, controller test failing

bc0e4e3

fix pipelines api controller tests

938a136

add test for get all pipeline jobs endpoint

a83b0bb

add user isolation test for retrieveJob

99e546f

resolve a few code smells, more to do

be4b2e7

spotless apply

ebacc2c

fix some code smells

8f5f790

change switch case to if statement for now

9555f80

fix an instance of String pipelineId -> PipelinesEnum

b2d1061

more PipelinesEnum refactoring

dda5bfc

spotlessapply

7bc322c

remove unusued IngressConfiguration

4d20648

fix some comments

8580c7b

add descriptions of EnumerateJob(s) classes

48eca44

add method description for validatePipelineId

08a24ab

note that pipelineId is case-insensitive in openapi doc

ea23521

remove note about pipelineId being case insensitive

f9529d3

move validatePipelineId back to controller, update tests to support t…

93c6681

…his and PipelinesEnum in general

add case-insensitive test for createJob

4d2b389

jsotobroad reviewed Jan 17, 2024

View reviewed changes

mmorgantaylor added 9 commits January 19, 2024 15:48

properly allow a 202 response from createJob, return the correct form…

1850804

…at of response to createJob

more description of pipelines jobs in controller

7a2d264

better descriptions

5f12981

remove toy pipeline from db migration

08c7ee7

add back isRequiredKey() - but not used yet

0569360

add TODO so I don't forget to use isRequiredKey

b01dd8b

return 403 instead of 404 for unauthorized get job

9b601a2

refactor StairwayJobService to JobService etc

fcaacb4

clean up JobMapKeys

b037abd

mmorgantaylor added 3 commits January 22, 2024 13:05

update JobBuilder to use AddParameter, add description to api payload…

6a0324f

…, remove request from jobmapkeys

implement separate validateRequiredInputs before submit, add tests fo…

fc9b45d

…r more exceptions in JobService

code smell, add a test

a67bb4e

mmorgantaylor marked this pull request as ready for review January 23, 2024 14:24

remove some inappropriate api responses, add a test

41b91e9

jsotobroad reviewed Jan 23, 2024

View reviewed changes

mmorgantaylor added 5 commits January 23, 2024 10:21

add another jobs controller test

563bdce

remove unnecessary ImputationService methods and @transactional annot…

c1e6e95

…ation

refactor required keys check

fb68059

collect all missing fields before throwing error

e993ea6

require non-null, non empty values for required fields

3da8b58

jsotobroad approved these changes Jan 23, 2024

View reviewed changes

mmorgantaylor merged commit 1e55128 into main Jan 23, 2024
13 checks passed

mmorgantaylor deleted the TSPS-123_mma_async_api_best_practices branch January 23, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TSPS-123 Refactor to follow DSP's Async API best practices #48

TSPS-123 Refactor to follow DSP's Async API best practices #48

mmorgantaylor commented Jan 9, 2024 •

edited

Loading

jsotobroad left a comment

jsotobroad Jan 17, 2024

mmorgantaylor Jan 19, 2024

jsotobroad Jan 17, 2024

mmorgantaylor Jan 19, 2024

jsotobroad Jan 17, 2024

mmorgantaylor Jan 19, 2024

jsotobroad Jan 17, 2024

mmorgantaylor Jan 22, 2024

jsotobroad Jan 17, 2024

mmorgantaylor Jan 19, 2024 •

edited

Loading

jsotobroad Jan 17, 2024

mmorgantaylor Jan 19, 2024

jsotobroad Jan 17, 2024

mmorgantaylor Jan 19, 2024

jsotobroad Jan 23, 2024

mmorgantaylor Jan 23, 2024

jsotobroad Jan 23, 2024

mmorgantaylor Jan 23, 2024

jsotobroad Jan 23, 2024

mmorgantaylor Jan 23, 2024

sonarqubecloud bot commented Jan 23, 2024

TSPS-123 Refactor to follow DSP's Async API best practices #48

TSPS-123 Refactor to follow DSP's Async API best practices #48

Conversation

mmorgantaylor commented Jan 9, 2024 • edited Loading

Description

Jira Ticket

jsotobroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorgantaylor Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 23, 2024

Quality Gate passed

mmorgantaylor commented Jan 9, 2024 •

edited

Loading

mmorgantaylor Jan 19, 2024 •

edited

Loading