refactor(rest-api): address review feedback on Task model

Three follow-ups from thossain-nv on PR #2288:

- Inline BuildAPITaskOptions as APIGetTasksRequest.TaskOptions, since
  the function only ever consumed a single request and was already
  conceptually a getter on it.
- Move parseTaskReportV1 into APITaskReportV1.UnmarshalJSON so the
  custom Flow snake_case decoder is reachable via the standard
  encoding/json entry point. The marshal path stays camelCase via
  struct tags; the doc comment flags the resulting asymmetry.
- Rename WithReport to WithTaskReport so future model option
  constructors for other API types (machine, rack, ...) can coexist
  in this shared package without colliding on a generic "Report" name.

Signed-off-by: Kun Zhao <kunzhao@nvidia.com>

Author: kunzhao-nv

rest-api/api/pkg/api/handler/task.go

@@ -183,7 +183,7 @@ func (gth GetTaskHandler) Handle(c echo.Context) error {
 		return cutil.NewAPIErrorResponse(c, http.StatusNotFound, "Task not found", nil)
 	}
 
-	apiTask := model.NewAPITask(tasks[0], model.WithReport())
+	apiTask := model.NewAPITask(tasks[0], model.WithTaskReport())
 
 	logger.Info().Msg("finishing API handler")
 
@@ -354,7 +354,7 @@ func (cth CancelTaskHandler) Handle(c echo.Context) error {
 		return cutil.NewAPIErrorResponse(c, code, fmt.Sprintf("Failed to execute Task cancellation workflow on Site: %s", unwrapErr), nil)
 	}
 
-	apiTask := model.NewAPITask(flowResponse.GetTask(), model.WithReport())
+	apiTask := model.NewAPITask(flowResponse.GetTask(), model.WithTaskReport())
 
 	logger.Info().Msg("finishing API handler")
 	return c.JSON(http.StatusAccepted, apiTask)
@@ -529,7 +529,7 @@ func (h GetRackTasksHandler) Handle(c echo.Context) error {
 		return cutil.NewAPIErrorResponse(c, code, fmt.Sprintf("Failed to execute workflow to retrieve all Rack Tasks: %s", unwrapErr), nil)
 	}
 
-	taskOpts := model.BuildAPITaskOptions(apiRequest)
+	taskOpts := apiRequest.TaskOptions()
 	apiTasks := make([]*model.APITask, 0, len(flowResponse.GetTasks()))
 	for _, t := range flowResponse.GetTasks() {
 		apiTasks = append(apiTasks, model.NewAPITask(t, taskOpts...))
@@ -715,7 +715,7 @@ func (h GetTrayTasksHandler) Handle(c echo.Context) error {
 		return cutil.NewAPIErrorResponse(c, code, fmt.Sprintf("Failed to execute workflow to retrieve all Tray Tasks: %s", unwrapErr), nil)
 	}
 
-	taskOpts := model.BuildAPITaskOptions(apiRequest)
+	taskOpts := apiRequest.TaskOptions()
 	apiTasks := make([]*model.APITask, 0, len(flowResponse.GetTasks()))
 	for _, t := range flowResponse.GetTasks() {
 		apiTasks = append(apiTasks, model.NewAPITask(t, taskOpts...))

rest-api/api/pkg/api/model/task.go

@@ -132,27 +132,26 @@ type flowTaskReportV1Step struct {
 	Error               string                `json:"error,omitempty"`
 }
 
-// parseTaskReportV1 decodes the JSON Flow writes into Task.report and
-// translates it into the REST-facing APITaskReportV1 (snake_case ->
-// camelCase). Returns nil for empty input. Returns nil if the payload
-// fails to parse or carries a version other than 1: omitting the field
-// is preferable to surfacing a stale/wrong shape behind a v1 contract.
-func parseTaskReportV1(raw string) *APITaskReportV1 {
-	if raw == "" {
-		return nil
-	}
+// UnmarshalJSON decodes Flow's snake_case wire format for Task.report
+// into the REST-facing APITaskReportV1. The marshal path is asymmetric:
+// json.Marshal walks the struct tags and emits camelCase per the REST
+// surface, while UnmarshalJSON intentionally only accepts Flow's
+// snake_case input. Round-tripping a marshalled APITaskReportV1 through
+// json.Unmarshal will therefore drop most fields; callers that need a
+// round-trippable form should decode into a map. Returns an error for
+// any payload that fails to parse or carries a version other than 1, so
+// FromProto can drop the field rather than surface an off-contract shape.
+func (r *APITaskReportV1) UnmarshalJSON(data []byte) error {
 	var src flowTaskReportV1
-	if err := json.Unmarshal([]byte(raw), &src); err != nil {
-		return nil
+	if err := json.Unmarshal(data, &src); err != nil {
+		return err
 	}
 	if src.Version != 1 {
-		return nil
-	}
-	out := &APITaskReportV1{
-		Version: src.Version,
-		Error:   src.Error,
-		Stages:  make([]APITaskReportV1Stage, 0, len(src.Stages)),
+		return fmt.Errorf("unsupported task report version %d", src.Version)
 	}
+	r.Version = src.Version
+	r.Error = src.Error
+	r.Stages = make([]APITaskReportV1Stage, 0, len(src.Stages))
 	for _, s := range src.Stages {
 		dstStage := APITaskReportV1Stage{
 			Number:     s.Number,
@@ -174,9 +173,9 @@ func parseTaskReportV1(raw string) *APITaskReportV1 {
 				Error:               p.Error,
 			})
 		}
-		out.Stages = append(out.Stages, dstStage)
+		r.Stages = append(r.Stages, dstStage)
 	}
-	return out
+	return nil
 }
 
 // APITaskOption configures optional fields populated on an APITask.
@@ -188,26 +187,17 @@ type apiTaskOptions struct {
 	withReport bool
 }
 
-// WithReport populates APITask.Report by decoding Task.report as
+// WithTaskReport populates APITask.Report by decoding Task.report as
 // APITaskReportV1. Without this option, Report is left nil and is
 // omitted from the JSON response. A malformed or non-v1 payload also
 // yields nil so the response never carries an off-contract shape.
-func WithReport() APITaskOption {
+// Named WithTaskReport rather than WithReport so future option
+// constructors for other API types (machine, rack, ...) can coexist
+// in this package without a naming collision.
+func WithTaskReport() APITaskOption {
 	return func(o *apiTaskOptions) { o.withReport = true }
 }
 
-// BuildAPITaskOptions translates the opt-in toggles on an
-// APIGetTasksRequest into the APITaskOption slice the list-task
-// handlers pass to NewAPITask. Centralized so new opt-in fields
-// only need wiring in one place rather than in each list handler.
-func BuildAPITaskOptions(req APIGetTasksRequest) []APITaskOption {
-	var opts []APITaskOption
-	if req.IncludeReport {
-		opts = append(opts, WithReport())
-	}
-	return opts
-}
-
 func (t *APITask) FromProto(task *flowv1.Task, opts ...APITaskOption) {
 	if task == nil {
 		return
@@ -233,7 +223,12 @@ func (t *APITask) FromProto(task *flowv1.Task, opts ...APITaskOption) {
 	t.Created = task.GetCreatedAt().AsTime().UTC()
 	t.Updated = task.GetUpdatedAt().AsTime().UTC()
 	if o.withReport {
-		t.Report = parseTaskReportV1(task.GetReport())
+		if raw := task.GetReport(); raw != "" {
+			var r APITaskReportV1
+			if err := json.Unmarshal([]byte(raw), &r); err == nil {
+				t.Report = &r
+			}
+		}
 	}
 }
 
@@ -283,6 +278,19 @@ func (r *APIGetTasksRequest) Validate() error {
 	return nil
 }
 
+// TaskOptions returns the APITaskOption slice the list-task handlers
+// pass to NewAPITask, translating the opt-in toggles on this request
+// (IncludeReport today, more later) into the corresponding option
+// constructors. Centralized here so new opt-in fields wire up in one
+// place rather than in each list handler.
+func (r *APIGetTasksRequest) TaskOptions() []APITaskOption {
+	var opts []APITaskOption
+	if r.IncludeReport {
+		opts = append(opts, WithTaskReport())
+	}
+	return opts
+}
+
 // QueryValues returns query parameters that participate in deterministic
 // workflow ID hashing, including pagination fields so concurrent requests
 // for different pages do not reuse the same workflow execution.

rest-api/api/pkg/api/model/task_test.go

@@ -187,7 +187,7 @@ func TestNewAPITask_Report(t *testing.T) {
 		assert.Nil(t, result.Report, "Report must default to nil so the JSON field is omitted")
 	})
 
-	t.Run("WithReport decodes a v1 payload into the typed struct with camelCase keys", func(t *testing.T) {
+	t.Run("WithTaskReport decodes a v1 payload into the typed struct with camelCase keys", func(t *testing.T) {
 		body := `{
 			"version": 1,
 			"stages": [
@@ -214,7 +214,7 @@ func TestNewAPITask_Report(t *testing.T) {
 			Report: body,
 		}
 
-		result := NewAPITask(task, WithReport())
+		result := NewAPITask(task, WithTaskReport())
 
 		require.NotNil(t, result.Report)
 		assert.Equal(t, 1, result.Report.Version)
@@ -239,50 +239,51 @@ func TestNewAPITask_Report(t *testing.T) {
 		assert.NotContains(t, step, "component_type")
 	})
 
-	t.Run("WithReport on empty proto report yields nil", func(t *testing.T) {
+	t.Run("WithTaskReport on empty proto report yields nil", func(t *testing.T) {
 		task := &flowv1.Task{
 			Id:     &flowv1.UUID{Id: "task-rep-3"},
 			Status: flowv1.TaskStatus_TASK_STATUS_PENDING,
 		}
 
-		result := NewAPITask(task, WithReport())
+		result := NewAPITask(task, WithTaskReport())
 
 		assert.Nil(t, result.Report, "Empty proto report must not surface as an empty JSON value")
 	})
 
-	t.Run("WithReport on malformed JSON yields nil", func(t *testing.T) {
+	t.Run("WithTaskReport on malformed JSON yields nil", func(t *testing.T) {
 		task := &flowv1.Task{
 			Id:     &flowv1.UUID{Id: "task-rep-4"},
 			Status: flowv1.TaskStatus_TASK_STATUS_RUNNING,
 			Report: `{`,
 		}
 
-		result := NewAPITask(task, WithReport())
+		result := NewAPITask(task, WithTaskReport())
 
 		assert.Nil(t, result.Report, "Malformed report must not surface as a partial struct")
 	})
 
-	t.Run("WithReport on non-v1 payload yields nil", func(t *testing.T) {
+	t.Run("WithTaskReport on non-v1 payload yields nil", func(t *testing.T) {
 		task := &flowv1.Task{
 			Id:     &flowv1.UUID{Id: "task-rep-5"},
 			Status: flowv1.TaskStatus_TASK_STATUS_RUNNING,
 			Report: `{"version":2,"stages":[]}`,
 		}
 
-		result := NewAPITask(task, WithReport())
+		result := NewAPITask(task, WithTaskReport())
 
 		assert.Nil(t, result.Report, "v2+ payload must not be exposed behind the v1 contract")
 	})
 }
 
-func TestBuildAPITaskOptions(t *testing.T) {
+func TestAPIGetTasksRequest_TaskOptions(t *testing.T) {
 	t.Run("default request yields no options", func(t *testing.T) {
-		opts := BuildAPITaskOptions(APIGetTasksRequest{SiteID: "s"})
-		assert.Empty(t, opts)
+		req := APIGetTasksRequest{SiteID: "s"}
+		assert.Empty(t, req.TaskOptions())
 	})
 
-	t.Run("includeReport=true yields WithReport()", func(t *testing.T) {
-		opts := BuildAPITaskOptions(APIGetTasksRequest{SiteID: "s", IncludeReport: true})
+	t.Run("includeReport=true yields WithTaskReport()", func(t *testing.T) {
+		req := APIGetTasksRequest{SiteID: "s", IncludeReport: true}
+		opts := req.TaskOptions()
 		require.Len(t, opts, 1)
 
 		// The option must decode Task.report when the proto report is non-empty.