From c16c1f4b15e1ccf8123c0b5e7e695efbd1f38312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Wed, 14 Dec 2022 11:26:15 +0100 Subject: [PATCH] Refactor: deduplicate job fetching code Deduplicate API implementation code to fetch a job from the persistence service. Almost no functional changes. Checking that the requested job UUID is actually a valid UUID is now consistently done on all fetches. This is not a functional change in normal Flamenco operations, where only valid UUIDs are used anyway. --- internal/manager/api_impl/jobs.go | 37 ++++++++---------- internal/manager/api_impl/jobs_query.go | 51 ++++++++++++++++++------- 2 files changed, 53 insertions(+), 35 deletions(-) diff --git a/internal/manager/api_impl/jobs.go b/internal/manager/api_impl/jobs.go index 63d2a906..ca4a7479 100644 --- a/internal/manager/api_impl/jobs.go +++ b/internal/manager/api_impl/jobs.go @@ -162,10 +162,9 @@ func (f *Flamenco) compileSubmittedJob(ctx context.Context, logger zerolog.Logge // SetJobStatus is used by the web interface to change a job's status. func (f *Flamenco) SetJobStatus(e echo.Context, jobID string) error { - logger := requestLogger(e) - ctx := e.Request().Context() - - logger = logger.With().Str("job", jobID).Logger() + logger := requestLogger(e).With(). + Str("job", jobID). + Logger() var statusChange api.SetJobStatusJSONRequestBody if err := e.Bind(&statusChange); err != nil { @@ -173,13 +172,10 @@ func (f *Flamenco) SetJobStatus(e echo.Context, jobID string) error { return sendAPIError(e, http.StatusBadRequest, "invalid format") } - dbJob, err := f.persist.FetchJob(ctx, jobID) - if err != nil { - if errors.Is(err, persistence.ErrJobNotFound) { - return sendAPIError(e, http.StatusNotFound, "no such job") - } - logger.Error().Err(err).Msg("error fetching job") - return sendAPIError(e, http.StatusInternalServerError, "error fetching job") + dbJob, err := f.fetchJob(e, logger, jobID) + if dbJob == nil { + // f.fetchJob already sent a response. + return err } logger = logger.With(). @@ -189,6 +185,7 @@ func (f *Flamenco) SetJobStatus(e echo.Context, jobID string) error { Logger() logger.Info().Msg("job status change requested") + ctx := e.Request().Context() err = f.stateMachine.JobStatusChange(ctx, dbJob, statusChange.Status, statusChange.Reason) if err != nil { logger.Error().Err(err).Msg("error changing job status") @@ -215,10 +212,9 @@ func (f *Flamenco) SetJobStatus(e echo.Context, jobID string) error { // SetJobPriority is used by the web interface to change a job's priority. func (f *Flamenco) SetJobPriority(e echo.Context, jobID string) error { - logger := requestLogger(e) - ctx := e.Request().Context() - - logger = logger.With().Str("job", jobID).Logger() + logger := requestLogger(e).With(). + Str("job", jobID). + Logger() var prioChange api.SetJobPriorityJSONRequestBody if err := e.Bind(&prioChange); err != nil { @@ -226,13 +222,10 @@ func (f *Flamenco) SetJobPriority(e echo.Context, jobID string) error { return sendAPIError(e, http.StatusBadRequest, "invalid format") } - dbJob, err := f.persist.FetchJob(ctx, jobID) - if err != nil { - if errors.Is(err, persistence.ErrJobNotFound) { - return sendAPIError(e, http.StatusNotFound, "no such job") - } - logger.Error().Err(err).Msg("error fetching job") - return sendAPIError(e, http.StatusInternalServerError, "error fetching job") + dbJob, err := f.fetchJob(e, logger, jobID) + if dbJob == nil { + // f.fetchJob already sent a response. + return err } logger = logger.With(). diff --git a/internal/manager/api_impl/jobs_query.go b/internal/manager/api_impl/jobs_query.go index 4751b4ae..74887d5f 100644 --- a/internal/manager/api_impl/jobs_query.go +++ b/internal/manager/api_impl/jobs_query.go @@ -2,34 +2,59 @@ package api_impl import ( + "context" "errors" - "fmt" "net/http" "github.com/labstack/echo/v4" + "github.com/rs/zerolog" "git.blender.org/flamenco/internal/manager/persistence" "git.blender.org/flamenco/internal/uuid" "git.blender.org/flamenco/pkg/api" ) +// fetchJob fetches the job from the database, and sends the appropriate error +// to the HTTP client if it cannot. Returns `nil` in the latter case, and the +// error returned can then be returned from the Echo handler function. +func (f *Flamenco) fetchJob(e echo.Context, logger zerolog.Logger, jobID string) (*persistence.Job, error) { + // TODO: use a timeout for fetching jobs. + // ctx, cancel := context.WithTimeout(e.Request().Context(), fetchJobTimeout) + // defer cancel() + ctx := e.Request().Context() + + if !uuid.IsValid(jobID) { + logger.Debug().Msg("invalid job ID received") + return nil, sendAPIError(e, http.StatusBadRequest, "job ID not valid") + } + + logger.Debug().Msg("fetching job") + dbJob, err := f.persist.FetchJob(ctx, jobID) + if err != nil { + switch { + case errors.Is(err, persistence.ErrJobNotFound): + return nil, sendAPIError(e, http.StatusNotFound, "no such job") + case errors.Is(err, context.DeadlineExceeded): + logger.Error().Err(err).Msg("timeout fetching job from database") + return nil, sendAPIError(e, http.StatusInternalServerError, "timeout fetching job from database") + default: + logger.Error().Err(err).Msg("error fetching job") + return nil, sendAPIError(e, http.StatusInternalServerError, "error fetching job") + } + } + + return dbJob, nil +} + func (f *Flamenco) FetchJob(e echo.Context, jobID string) error { logger := requestLogger(e).With(). Str("job", jobID). Logger() - if !uuid.IsValid(jobID) { - logger.Debug().Msg("invalid job ID received") - return sendAPIError(e, http.StatusBadRequest, "job ID not valid") - } - - logger.Debug().Msg("fetching job") - - ctx := e.Request().Context() - dbJob, err := f.persist.FetchJob(ctx, jobID) - if err != nil { - logger.Warn().Err(err).Msg("cannot fetch job") - return sendAPIError(e, http.StatusNotFound, fmt.Sprintf("job %+v not found", jobID)) + dbJob, err := f.fetchJob(e, logger, jobID) + if dbJob == nil { + // f.fetchJob already sent a response. + return err } apiJob := jobDBtoAPI(dbJob)