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

@@ -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)
+	logger := requestLogger(e).With().
-	ctx := e.Request().Context()
+		Str("job", jobID).
-
+		Logger()
-	logger = logger.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)
+	dbJob, err := f.fetchJob(e, logger, jobID)
-	if err != nil {
+	if dbJob == nil {
-		if errors.Is(err, persistence.ErrJobNotFound) {
+		// f.fetchJob already sent a response.
-			return sendAPIError(e, http.StatusNotFound, "no such job")
+		return err
-		}
-		logger.Error().Err(err).Msg("error fetching job")
-		return sendAPIError(e, http.StatusInternalServerError, "error fetching job")
 	}
 
 	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)
+	logger := requestLogger(e).With().
-	ctx := e.Request().Context()
+		Str("job", jobID).
-
+		Logger()
-	logger = logger.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)
+	dbJob, err := f.fetchJob(e, logger, jobID)
-	if err != nil {
+	if dbJob == nil {
-		if errors.Is(err, persistence.ErrJobNotFound) {
+		// f.fetchJob already sent a response.
-			return sendAPIError(e, http.StatusNotFound, "no such job")
+		return err
-		}
-		logger.Error().Err(err).Msg("error fetching job")
-		return sendAPIError(e, http.StatusInternalServerError, "error fetching job")
 	}
 
 	logger = logger.With().

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) {
+	dbJob, err := f.fetchJob(e, logger, jobID)
-		logger.Debug().Msg("invalid job ID received")
+	if dbJob == nil {
-		return sendAPIError(e, http.StatusBadRequest, "job ID not valid")
+		// f.fetchJob already sent a response.
-	}
+		return err
-
-	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))
 	}
 
 	apiJob := jobDBtoAPI(dbJob)