-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TSPS-123 Refactor to follow DSP's Async API best practices #48
Merged
Merged
Changes from 20 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
64933bd
first pass at refactor, more still to do
mmorgantaylor bc0e4e3
wip, controller test failing
mmorgantaylor 938a136
fix pipelines api controller tests
mmorgantaylor a83b0bb
add test for get all pipeline jobs endpoint
mmorgantaylor 99e546f
add user isolation test for retrieveJob
mmorgantaylor be4b2e7
resolve a few code smells, more to do
mmorgantaylor ebacc2c
spotless apply
mmorgantaylor 8f5f790
fix some code smells
mmorgantaylor 9555f80
change switch case to if statement for now
mmorgantaylor b2d1061
fix an instance of String pipelineId -> PipelinesEnum
mmorgantaylor dda5bfc
more PipelinesEnum refactoring
mmorgantaylor 7bc322c
spotlessapply
mmorgantaylor 4d20648
remove unusued IngressConfiguration
mmorgantaylor 8580c7b
fix some comments
mmorgantaylor 48eca44
add descriptions of EnumerateJob(s) classes
mmorgantaylor 08a24ab
add method description for validatePipelineId
mmorgantaylor ea23521
note that pipelineId is case-insensitive in openapi doc
mmorgantaylor f9529d3
remove note about pipelineId being case insensitive
mmorgantaylor 93c6681
move validatePipelineId back to controller, update tests to support t…
mmorgantaylor 4d2b389
add case-insensitive test for createJob
mmorgantaylor 1850804
properly allow a 202 response from createJob, return the correct form…
mmorgantaylor 7a2d264
more description of pipelines jobs in controller
mmorgantaylor 5f12981
better descriptions
mmorgantaylor 08c7ee7
remove toy pipeline from db migration
mmorgantaylor 0569360
add back isRequiredKey() - but not used yet
mmorgantaylor b01dd8b
add TODO so I don't forget to use isRequiredKey
mmorgantaylor 9b601a2
return 403 instead of 404 for unauthorized get job
mmorgantaylor fcaacb4
refactor StairwayJobService to JobService etc
mmorgantaylor b037abd
clean up JobMapKeys
mmorgantaylor 6a0324f
update JobBuilder to use AddParameter, add description to api payload…
mmorgantaylor fc9b45d
implement separate validateRequiredInputs before submit, add tests fo…
mmorgantaylor a67bb4e
code smell, add a test
mmorgantaylor 41b91e9
remove some inappropriate api responses, add a test
mmorgantaylor 563bdce
add another jobs controller test
mmorgantaylor c1e6e95
remove unnecessary ImputationService methods and @Transactional annot…
mmorgantaylor fb68059
refactor required keys check
mmorgantaylor e993ea6
collect all missing fields before throwing error
mmorgantaylor 3da8b58
require non-null, non empty values for required fields
mmorgantaylor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
136 changes: 136 additions & 0 deletions
136
service/src/main/java/bio/terra/pipelines/app/controller/JobApiUtils.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
package bio.terra.pipelines.app.controller; | ||
|
||
import bio.terra.common.exception.ErrorReportException; | ||
import bio.terra.pipelines.dependencies.stairway.StairwayJobMapKeys; | ||
import bio.terra.pipelines.dependencies.stairway.exception.InvalidResultStateException; | ||
import bio.terra.pipelines.dependencies.stairway.model.EnumeratedJob; | ||
import bio.terra.pipelines.dependencies.stairway.model.EnumeratedJobs; | ||
import bio.terra.pipelines.generated.model.ApiErrorReport; | ||
import bio.terra.pipelines.generated.model.ApiGetJobsResponse; | ||
import bio.terra.pipelines.generated.model.ApiJobReport; | ||
import bio.terra.stairway.FlightMap; | ||
import bio.terra.stairway.FlightState; | ||
import bio.terra.stairway.FlightStatus; | ||
import java.time.Instant; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class JobApiUtils { | ||
|
||
JobApiUtils() {} | ||
|
||
public static ApiGetJobsResponse mapEnumeratedJobsToApi(EnumeratedJobs enumeratedJobs) { | ||
|
||
// Convert the result to API-speak | ||
List<ApiJobReport> apiJobList = new ArrayList<>(); | ||
for (EnumeratedJob enumeratedJob : enumeratedJobs.getResults()) { | ||
ApiJobReport jobReport = mapFlightStateToApiJobReport(enumeratedJob.getFlightState()); | ||
apiJobList.add(jobReport); | ||
} | ||
|
||
return new ApiGetJobsResponse() | ||
.pageToken(enumeratedJobs.getPageToken()) | ||
.totalResults(enumeratedJobs.getTotalResults()) | ||
.results(apiJobList); | ||
} | ||
|
||
public static ApiJobReport mapFlightStateToApiJobReport(FlightState flightState) { | ||
FlightMap inputParameters = flightState.getInputParameters(); | ||
String description = | ||
inputParameters.get(StairwayJobMapKeys.DESCRIPTION.getKeyName(), String.class); | ||
FlightStatus flightStatus = flightState.getFlightStatus(); | ||
String submittedDate = flightState.getSubmitted().toString(); | ||
ApiJobReport.StatusEnum jobStatus = mapFlightStatusToApi(flightStatus); | ||
|
||
String completedDate = null; | ||
HttpStatus statusCode = HttpStatus.ACCEPTED; | ||
|
||
if (jobStatus != ApiJobReport.StatusEnum.RUNNING) { | ||
// If the job is completed, the JobReport should include a result code indicating success or | ||
// failure. For failed jobs, this code is the error code. For successful jobs, this is the | ||
// code specified by the flight if present, or a default of 200 if not. | ||
completedDate = | ||
flightState | ||
.getCompleted() | ||
.map(Instant::toString) | ||
.orElseThrow( | ||
() -> new InvalidResultStateException("No completed time for completed flight")); | ||
switch (jobStatus) { | ||
case FAILED -> { | ||
int errorCode = | ||
flightState | ||
.getException() | ||
.map(e -> buildApiErrorReport(e).getStatusCode()) | ||
.orElseThrow( | ||
() -> | ||
new InvalidResultStateException( | ||
String.format( | ||
"Flight %s failed with no exception reported", | ||
flightState.getFlightId()))); | ||
statusCode = HttpStatus.valueOf(errorCode); | ||
} | ||
case SUCCEEDED -> { | ||
FlightMap resultMap = | ||
flightState.getResultMap().orElseThrow(InvalidResultStateException::noResultMap); | ||
statusCode = resultMap.get(StairwayJobMapKeys.STATUS_CODE.getKeyName(), HttpStatus.class); | ||
if (statusCode == null) { | ||
statusCode = HttpStatus.OK; | ||
} | ||
} | ||
default -> throw new IllegalStateException( | ||
"Cannot get status code of flight in unknown state " + jobStatus); | ||
} | ||
} | ||
|
||
return new ApiJobReport() | ||
.id(flightState.getFlightId()) | ||
.description(description) | ||
.status(jobStatus) | ||
.statusCode(statusCode.value()) | ||
.submitted(submittedDate) | ||
.completed(completedDate) | ||
.resultURL(resultUrlFromFlightState(flightState)); | ||
} | ||
|
||
private static ApiJobReport.StatusEnum mapFlightStatusToApi(FlightStatus flightStatus) { | ||
switch (flightStatus) { | ||
case RUNNING, QUEUED, WAITING, READY, READY_TO_RESTART: | ||
return ApiJobReport.StatusEnum.RUNNING; | ||
case SUCCESS: | ||
return ApiJobReport.StatusEnum.SUCCEEDED; | ||
case ERROR, FATAL: | ||
return ApiJobReport.StatusEnum.FAILED; | ||
default: | ||
return ApiJobReport.StatusEnum.FAILED; | ||
} | ||
} | ||
|
||
public static ApiErrorReport buildApiErrorReport(Exception exception) { | ||
if (exception instanceof ErrorReportException errorReport) { | ||
return new ApiErrorReport() | ||
.message(errorReport.getMessage()) | ||
.statusCode(errorReport.getStatusCode().value()) | ||
.causes(errorReport.getCauses()); | ||
} else { | ||
return new ApiErrorReport() | ||
.message(exception.getMessage()) | ||
.statusCode(HttpStatus.INTERNAL_SERVER_ERROR.value()) | ||
.causes(null); | ||
} | ||
} | ||
|
||
private static String resultUrlFromFlightState(FlightState flightState) { | ||
String resultPath = | ||
flightState | ||
.getInputParameters() | ||
.get(StairwayJobMapKeys.RESULT_PATH.getKeyName(), String.class); | ||
if (resultPath == null) { | ||
resultPath = ""; | ||
} | ||
// TSPS-135 will implement the GET result endpoint, at which point this path should be created | ||
return resultPath; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed in Slack - had been overly cautious with returning 404s instead of 403s, but this isn't necessary. should be returning 403s properly now.