Skip to content
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 38 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
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 Jan 9, 2024
bc0e4e3
wip, controller test failing
mmorgantaylor Jan 10, 2024
938a136
fix pipelines api controller tests
mmorgantaylor Jan 10, 2024
a83b0bb
add test for get all pipeline jobs endpoint
mmorgantaylor Jan 10, 2024
99e546f
add user isolation test for retrieveJob
mmorgantaylor Jan 10, 2024
be4b2e7
resolve a few code smells, more to do
mmorgantaylor Jan 10, 2024
ebacc2c
spotless apply
mmorgantaylor Jan 10, 2024
8f5f790
fix some code smells
mmorgantaylor Jan 10, 2024
9555f80
change switch case to if statement for now
mmorgantaylor Jan 10, 2024
b2d1061
fix an instance of String pipelineId -> PipelinesEnum
mmorgantaylor Jan 11, 2024
dda5bfc
more PipelinesEnum refactoring
mmorgantaylor Jan 16, 2024
7bc322c
spotlessapply
mmorgantaylor Jan 16, 2024
4d20648
remove unusued IngressConfiguration
mmorgantaylor Jan 16, 2024
8580c7b
fix some comments
mmorgantaylor Jan 16, 2024
48eca44
add descriptions of EnumerateJob(s) classes
mmorgantaylor Jan 16, 2024
08a24ab
add method description for validatePipelineId
mmorgantaylor Jan 16, 2024
ea23521
note that pipelineId is case-insensitive in openapi doc
mmorgantaylor Jan 16, 2024
f9529d3
remove note about pipelineId being case insensitive
mmorgantaylor Jan 16, 2024
93c6681
move validatePipelineId back to controller, update tests to support t…
mmorgantaylor Jan 17, 2024
4d2b389
add case-insensitive test for createJob
mmorgantaylor Jan 17, 2024
1850804
properly allow a 202 response from createJob, return the correct form…
mmorgantaylor Jan 19, 2024
7a2d264
more description of pipelines jobs in controller
mmorgantaylor Jan 19, 2024
5f12981
better descriptions
mmorgantaylor Jan 19, 2024
08c7ee7
remove toy pipeline from db migration
mmorgantaylor Jan 19, 2024
0569360
add back isRequiredKey() - but not used yet
mmorgantaylor Jan 22, 2024
b01dd8b
add TODO so I don't forget to use isRequiredKey
mmorgantaylor Jan 22, 2024
9b601a2
return 403 instead of 404 for unauthorized get job
mmorgantaylor Jan 22, 2024
fcaacb4
refactor StairwayJobService to JobService etc
mmorgantaylor Jan 22, 2024
b037abd
clean up JobMapKeys
mmorgantaylor Jan 22, 2024
6a0324f
update JobBuilder to use AddParameter, add description to api payload…
mmorgantaylor Jan 22, 2024
fc9b45d
implement separate validateRequiredInputs before submit, add tests fo…
mmorgantaylor Jan 22, 2024
a67bb4e
code smell, add a test
mmorgantaylor Jan 23, 2024
41b91e9
remove some inappropriate api responses, add a test
mmorgantaylor Jan 23, 2024
563bdce
add another jobs controller test
mmorgantaylor Jan 23, 2024
c1e6e95
remove unnecessary ImputationService methods and @Transactional annot…
mmorgantaylor Jan 23, 2024
fb68059
refactor required keys check
mmorgantaylor Jan 23, 2024
e993ea6
collect all missing fields before throwing error
mmorgantaylor Jan 23, 2024
3da8b58
require non-null, non empty values for required fields
mmorgantaylor Jan 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
316 changes: 212 additions & 104 deletions common/openapi.yml

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package bio.terra.pipelines.app.common;

import bio.terra.pipelines.common.utils.PipelinesEnum;
import io.micrometer.core.instrument.Metrics;
import lombok.AccessLevel;
import lombok.NoArgsConstructor;
Expand All @@ -20,9 +21,10 @@ public class MetricsUtils {
*
* @param pipelineId - pipelineId that was run e.g. "imputation"
*/
public static void incrementPipelineRun(String pipelineId) {
public static void incrementPipelineRun(PipelinesEnum pipelineId) {
Metrics.globalRegistry
.counter(String.format("%s.pipeline.run.count", NAMESPACE), PIPELINE_TAG, pipelineId)
.counter(
String.format("%s.pipeline.run.count", NAMESPACE), PIPELINE_TAG, pipelineId.getValue())
.increment();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public ResponseEntity<ApiErrorReport> errorReportHandler(ErrorReportException ex
}

// -- validation exceptions - we don't control the exception raised
// TODO add JobNotFoundException method here - see TSPS-9
@ExceptionHandler({
MethodArgumentNotValidException.class,
MethodArgumentTypeMismatchException.class,
Expand Down
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;
}
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,14 @@
package bio.terra.pipelines.app.controller;

import bio.terra.common.exception.ApiException;
import bio.terra.common.iam.SamUser;
import bio.terra.common.iam.SamUserFactory;
import bio.terra.pipelines.app.common.MetricsUtils;
import bio.terra.pipelines.app.configuration.external.SamConfiguration;
import bio.terra.pipelines.db.entities.Job;
import bio.terra.pipelines.db.exception.PipelineNotFoundException;
import bio.terra.pipelines.dependencies.stairway.StairwayJobService;
import bio.terra.pipelines.dependencies.stairway.model.EnumeratedJobs;
import bio.terra.pipelines.generated.api.JobsApi;
import bio.terra.pipelines.generated.model.*;
import bio.terra.pipelines.service.ImputationService;
import bio.terra.pipelines.service.JobsService;
import bio.terra.pipelines.service.PipelinesService;
import bio.terra.stairway.FlightState;
import io.swagger.annotations.Api;
import java.util.List;
import java.util.UUID;
import javax.servlet.http.HttpServletRequest;
import org.slf4j.Logger;
Expand All @@ -23,7 +18,6 @@
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestBody;

/** Jobs controller */
@Controller
Expand All @@ -32,24 +26,18 @@ public class JobsApiController implements JobsApi {
private final SamConfiguration samConfiguration;
private final SamUserFactory samUserFactory;
private final HttpServletRequest request;
private final JobsService jobsService;
private final PipelinesService pipelinesService;
private final ImputationService imputationService;
private final StairwayJobService stairwayJobService;

@Autowired
public JobsApiController(
SamConfiguration samConfiguration,
SamUserFactory samUserFactory,
HttpServletRequest request,
JobsService jobsService,
PipelinesService pipelinesService,
ImputationService imputationService) {
StairwayJobService stairwayJobService) {
this.samConfiguration = samConfiguration;
this.samUserFactory = samUserFactory;
this.request = request;
this.jobsService = jobsService;
this.pipelinesService = pipelinesService;
this.imputationService = imputationService;
this.stairwayJobService = stairwayJobService;
}

private static final Logger logger = LoggerFactory.getLogger(JobsApiController.class);
Expand All @@ -61,93 +49,23 @@ private SamUser getAuthenticatedInfo() {
// -- Jobs --

@Override
public ResponseEntity<ApiPostJobResponse> createJob(
@PathVariable("pipelineId") String pipelineId, @RequestBody ApiPostJobRequestBody body) {
public ResponseEntity<ApiJobReport> getJob(@PathVariable("jobId") UUID jobId) {
final SamUser userRequest = getAuthenticatedInfo();
String userId = userRequest.getSubjectId();
String pipelineVersion = body.getPipelineVersion();
Object pipelineInputs = body.getPipelineInputs();

if (!pipelinesService.pipelineExists(pipelineId)) {
throw new PipelineNotFoundException(
String.format("Requested pipeline %s not found.", pipelineId));
}

logger.info(
"Creating {} pipeline job (version {}) for {} user {} with inputs {}",
pipelineId,
pipelineVersion,
userRequest.getEmail(),
userId,
pipelineInputs);

// TODO assuming we will write outputs back to source workspace, we will need to check user
// permissions for write access to the workspace - explore interceptors

UUID createdJobUuid =
jobsService.createJob(userId, pipelineId, pipelineVersion, pipelineInputs);
if (createdJobUuid == null) {
logger.error("New {} pipeline job creation failed.", pipelineId);
throw new ApiException("An internal error occurred.");
}

// eventually we'll expand this out to kick off the imputation pipeline flight but for
// now this is good enough.
imputationService.queryForWorkspaceApps();

ApiPostJobResponse createdJobResponse = new ApiPostJobResponse();
createdJobResponse.setJobId(createdJobUuid.toString());
logger.info("Created {} job {}", pipelineId, createdJobUuid);
MetricsUtils.incrementPipelineRun(pipelineId);

return new ResponseEntity<>(createdJobResponse, HttpStatus.OK);
}

@Override
public ResponseEntity<ApiGetJobResponse> getJob(
@PathVariable("pipelineId") String pipelineId, @PathVariable("jobId") UUID jobId) {
final SamUser userRequest = getAuthenticatedInfo();
String userId = userRequest.getSubjectId();
Job job = jobsService.getJob(userId, pipelineId, jobId);
ApiGetJobResponse result = jobToApi(job);

logger.info("Retrieving jobId {} for userId {}", jobId, userId);
FlightState flightState = stairwayJobService.retrieveJob(jobId, userId);
Copy link
Collaborator

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)

Copy link
Collaborator Author

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.

ApiJobReport result = JobApiUtils.mapFlightStateToApiJobReport(flightState);
return new ResponseEntity<>(result, HttpStatus.OK);
}

@Override
public ResponseEntity<ApiGetJobsResponse> getJobs(@PathVariable("pipelineId") String pipelineId) {
public ResponseEntity<ApiGetJobsResponse> getAllJobs(Integer limit, String pageToken) {
final SamUser userRequest = getAuthenticatedInfo();
String userId = userRequest.getSubjectId();
List<Job> jobList = jobsService.getJobs(userId, pipelineId);
ApiGetJobsResponse result = jobsToApi(jobList);

logger.info("Retrieving all jobs for userId {}", userId);
EnumeratedJobs enumeratedJobs =
stairwayJobService.enumerateJobs(userId, limit, pageToken, null);
ApiGetJobsResponse result = JobApiUtils.mapEnumeratedJobsToApi(enumeratedJobs);
return new ResponseEntity<>(result, HttpStatus.OK);
}

static ApiGetJobResponse jobToApi(Job job) {
ApiGetJobResponse apiGetJobResponse =
new ApiGetJobResponse()
.jobId(job.getJobId().toString())
.userId(job.getUserId())
.pipelineId(job.getPipelineId())
.pipelineVersion(job.getPipelineVersion())
.timeSubmitted(job.getTimeSubmitted().toString())
.status(job.getStatus());
if (job.getTimeCompleted() != null) {
apiGetJobResponse.setTimeCompleted(job.getTimeCompleted().toString());
}
return apiGetJobResponse;
}

static ApiGetJobsResponse jobsToApi(List<Job> jobList) {
ApiGetJobsResponse apiResult = new ApiGetJobsResponse();

for (Job job : jobList) {
var apiJob = jobToApi(job);

apiResult.add(apiJob);
}

return apiResult;
}
}
Loading
Loading