chore: improved validation and error responses

- Updated StatusCausesFromAPIStatus to also return causes from StatusReasonConflict
- Enhanced PauseActionWorkspaceHandler and CreateWorkspaceHandler to utilize IsEOFError for better error responses.
- Enhanced PauseActionWorkspaceHandler to ensure conflict_causes[] is present when Patch operation returns Invalid
- Introduced new test files for response error handling and validation helper functions.

Signed-off-by: Andy Stoneberg <[email protected]>

update

Signed-off-by: Andy Stoneberg <[email protected]>

Author: andyatmiami

workspaces/backend/api/helpers.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"mime"
 	"net/http"
 	"strings"
@@ -66,6 +67,13 @@ func (a *App) DecodeJSON(r *http.Request, v any) error {
 		if a.IsMaxBytesError(err) {
 			return err
 		}
+
+		// provide better error message for the case where the body is empty
+		// NOTE: io.EOF is only returned when the body is completely empty or contains only whitespace.
+		//       If there's any actual JSON content (even malformed), json.Decoder returns different errors.
+		if a.IsEOFError(err) {
+			return fmt.Errorf("request body was empty: %w", err)
+		}
 		return fmt.Errorf("error decoding JSON: %w", err)
 	}
 	return nil
@@ -77,6 +85,14 @@ func (a *App) IsMaxBytesError(err error) bool {
 	return errors.As(err, &maxBytesError)
 }
 
+// IsEOFError checks if the error is an EOF error (empty request body).
+// This returns true when the request body is completely empty, which happens when:
+// - Content-Length is 0, or
+// - The body stream ends immediately without any data (io.EOF)
+func (a *App) IsEOFError(err error) bool {
+	return errors.Is(err, io.EOF)
+}
+
 // ValidateContentType validates the Content-Type header of the request.
 // If this method returns false, the request has been handled and the caller should return immediately.
 // If this method returns true, the request has the correct Content-Type.

workspaces/backend/api/response_errors_test.go

@@ -0,0 +1,276 @@
+/*
+Copyright 2024.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+var _ = Describe("Error Response Functions", func() {
+
+	// Shared constants (used across multiple Describe blocks or in BeforeEach)
+	const (
+		testRequestPath   = "/test"
+		testRequestMethod = "GET"
+
+		// Used in both failedValidationResponse and Error response structure
+		testFieldName       = "name"
+		testNameRequiredMsg = "name is required"
+	)
+
+	var (
+		app *App
+		w   *httptest.ResponseRecorder
+		r   *http.Request
+	)
+
+	BeforeEach(func() {
+		app = &App{}
+		w = httptest.NewRecorder()
+		r = httptest.NewRequest(testRequestMethod, testRequestPath, http.NoBody)
+	})
+
+	Describe("conflictResponse", func() {
+		const (
+			httpStatusCodeConflict = http.StatusConflict
+
+			testGroupResourceGroup    = "test"
+			testGroupResourceResource = "tests"
+			testResourceName          = "test-name"
+			testConflictMsg           = "WorkspaceKind is used by 5 workspace(s)"
+		)
+
+		var httpStatusCodeConflictStr = strconv.Itoa(http.StatusConflict)
+
+		type testCase struct {
+			description string
+			err         error
+			k8sCauses   []metav1.StatusCause
+		}
+
+		conflictErr := apierrors.NewConflict(
+			schema.GroupResource{Group: testGroupResourceGroup, Resource: testGroupResourceResource},
+			testResourceName,
+			nil,
+		)
+
+		testCases := []testCase{
+			{
+				description: "should return 409 with k8s causes",
+				err:         conflictErr,
+				k8sCauses:   []metav1.StatusCause{{Type: metav1.CauseTypeFieldValueInvalid, Message: testConflictMsg}},
+			},
+			{
+				description: "should return 409 with error message when no k8s causes",
+				err:         conflictErr,
+				k8sCauses:   nil,
+			},
+			{
+				description: "should return 409 with empty conflict causes array",
+				err:         conflictErr,
+				k8sCauses:   []metav1.StatusCause{},
+			},
+		}
+
+		buildExpectedConflictResponse := func(err error, k8sCauses []metav1.StatusCause, codeStr string) ErrorResponse {
+			expectedConflictCauses := make([]ConflictError, len(k8sCauses))
+			for i, cause := range k8sCauses {
+				expectedConflictCauses[i] = ConflictError{
+					Origin:  OriginKubernetes,
+					Message: cause.Message,
+				}
+			}
+
+			// Determine message: if we have k8s causes, use generic message; otherwise use error message
+			var expectedMessage string
+			if len(k8sCauses) > 0 {
+				expectedMessage = errMsgKubernetesConflict
+			} else {
+				expectedMessage = err.Error()
+			}
+
+			// Empty slices become nil after JSON unmarshaling
+			var expectedConflictCausesForComparison []ConflictError
+			if len(expectedConflictCauses) > 0 {
+				expectedConflictCausesForComparison = expectedConflictCauses
+			} else {
+				expectedConflictCausesForComparison = nil
+			}
+
+			return ErrorResponse{
+				Code:    codeStr,
+				Message: expectedMessage,
+				Cause: &ErrorCause{
+					ConflictCauses: expectedConflictCausesForComparison,
+				},
+			}
+		}
+
+		for _, tc := range testCases {
+			It(tc.description, func() {
+				app.conflictResponse(w, r, tc.err, tc.k8sCauses)
+
+				Expect(w.Code).To(Equal(httpStatusCodeConflict))
+
+				var envelope ErrorEnvelope
+				unmarshalErr := json.Unmarshal(w.Body.Bytes(), &envelope)
+				Expect(unmarshalErr).NotTo(HaveOccurred())
+				Expect(envelope.Error).NotTo(BeNil())
+
+				expectedErrorResponse := buildExpectedConflictResponse(tc.err, tc.k8sCauses, httpStatusCodeConflictStr)
+				Expect(envelope.Error.ErrorResponse).To(Equal(expectedErrorResponse))
+			})
+		}
+	})
+
+	Describe("failedValidationResponse", func() {
+		const (
+			httpStatusCodeUnprocessableEntity = http.StatusUnprocessableEntity
+
+			testFieldKind      = "kind"
+			testFieldSpecName  = "spec.name"
+			testFieldSpecKind  = "spec.kind"
+			testInvalidKind    = "invalid-kind"
+			testKindInvalidMsg = "kind is invalid"
+		)
+
+		var httpStatusCodeUnprocessableEntityStr = strconv.Itoa(http.StatusUnprocessableEntity)
+
+		type testCase struct {
+			description string
+			msg         string
+			valErrs     field.ErrorList
+			k8sCauses   []metav1.StatusCause
+		}
+
+		buildExpectedValidationResponse := func(msg string, valErrs field.ErrorList, k8sCauses []metav1.StatusCause, codeStr string) ErrorResponse {
+			valErrsConverted := make([]ValidationError, len(valErrs))
+			for i, err := range valErrs {
+				valErrsConverted[i] = ValidationError{
+					Origin:  OriginInternal,
+					Type:    err.Type,
+					Field:   err.Field,
+					Message: err.ErrorBody(),
+				}
+			}
+
+			k8sCausesConverted := make([]ValidationError, len(k8sCauses))
+			for i, cause := range k8sCauses {
+				k8sCausesConverted[i] = ValidationError{
+					Origin:  OriginKubernetes,
+					Type:    field.ErrorType(cause.Type),
+					Field:   cause.Field,
+					Message: cause.Message,
+				}
+			}
+
+			expectedValidationErrors := make([]ValidationError, len(valErrsConverted)+len(k8sCausesConverted))
+			copy(expectedValidationErrors, valErrsConverted)
+			copy(expectedValidationErrors[len(valErrsConverted):], k8sCausesConverted)
+
+			var expectedValidationErrorsForComparison []ValidationError
+			if len(expectedValidationErrors) > 0 {
+				expectedValidationErrorsForComparison = expectedValidationErrors
+			} else {
+				expectedValidationErrorsForComparison = nil
+			}
+
+			return ErrorResponse{
+				Code:    codeStr,
+				Message: msg,
+				Cause: &ErrorCause{
+					ValidationErrors: expectedValidationErrorsForComparison,
+				},
+			}
+		}
+
+		testCases := []testCase{
+			{
+				description: "should return 422 with internal validation errors",
+				msg:         errMsgRequestBodyInvalid,
+				valErrs: field.ErrorList{
+					field.Required(field.NewPath(testFieldName), ""),
+					field.Invalid(field.NewPath(testFieldKind), testInvalidKind, testKindInvalidMsg),
+				},
+				k8sCauses: nil,
+			},
+			{
+				description: "should return 422 with k8s validation errors",
+				msg:         errMsgKubernetesValidation,
+				valErrs:     nil,
+				k8sCauses: []metav1.StatusCause{
+					{
+						Type:    metav1.CauseTypeFieldValueRequired,
+						Field:   testFieldSpecName,
+						Message: testNameRequiredMsg,
+					},
+					{
+						Type:    metav1.CauseTypeFieldValueInvalid,
+						Field:   testFieldSpecKind,
+						Message: testKindInvalidMsg,
+					},
+				},
+			},
+			{
+				description: "should return 422 with both internal and k8s validation errors",
+				msg:         errMsgRequestBodyInvalid,
+				valErrs: field.ErrorList{
+					field.Required(field.NewPath(testFieldName), ""),
+				},
+				k8sCauses: []metav1.StatusCause{
+					{
+						Type:    metav1.CauseTypeFieldValueInvalid,
+						Field:   testFieldSpecKind,
+						Message: testKindInvalidMsg,
+					},
+				},
+			},
+			{
+				description: "should return 422 with empty validation errors",
+				msg:         errMsgRequestBodyInvalid,
+				valErrs:     nil,
+				k8sCauses:   nil,
+			},
+		}
+
+		for _, tc := range testCases {
+			It(tc.description, func() {
+				app.failedValidationResponse(w, r, tc.msg, tc.valErrs, tc.k8sCauses)
+
+				Expect(w.Code).To(Equal(httpStatusCodeUnprocessableEntity))
+
+				var envelope ErrorEnvelope
+				err := json.Unmarshal(w.Body.Bytes(), &envelope)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(envelope.Error).NotTo(BeNil())
+
+				expectedErrorResponse := buildExpectedValidationResponse(tc.msg, tc.valErrs, tc.k8sCauses, httpStatusCodeUnprocessableEntityStr)
+				Expect(envelope.Error.ErrorResponse).To(Equal(expectedErrorResponse))
+			})
+		}
+	})
+})

workspaces/backend/api/workspace_actions_handler.go

@@ -79,6 +79,7 @@ func (a *App) PauseActionWorkspaceHandler(w http.ResponseWriter, r *http.Request
 			a.requestEntityTooLargeResponse(w, r, err)
 			return
 		}
+
 		//
 		// TODO: handle UnmarshalTypeError and return 422,
 		//       decode the paths which were failed to decode (included in the error)
@@ -121,7 +122,13 @@ func (a *App) PauseActionWorkspaceHandler(w http.ResponseWriter, r *http.Request
 			return
 		}
 		if errors.Is(err, repository.ErrWorkspaceInvalidState) {
-			a.conflictResponse(w, r, err, nil)
+			causes := helper.StatusCausesFromAPIStatus(err)
+			// The patch can fail due to a 'test' operation that ensures the workspace is in a valid state for the action.
+			// In these cases, Kubernetes returns an Invalid error but without StatusCauses.
+			// We prepend a StatusCause with the `ErrWorkspaceInvalidState` error message to provide context,
+			// but preserve any existing causes in case Kubernetes provides more details in the future.
+			causes = append([]metav1.StatusCause{{Message: err.Error()}}, causes...)
+			a.conflictResponse(w, r, err, causes)
 			return
 		}
 		a.serverErrorResponse(w, r, err)

workspaces/backend/api/workspaces_handler.go

@@ -227,6 +227,7 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps
 			a.requestEntityTooLargeResponse(w, r, err)
 			return
 		}
+
 		//
 		// TODO: handle UnmarshalTypeError and return 422,
 		//       decode the paths which were failed to decode (included in the error)

workspaces/backend/internal/helper/validation.go

@@ -29,8 +29,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-// StatusCausesFromAPIStatus extracts status causes from a Kubernetes apierrors.APIStatus validation error.
-// NOTE: we use this to convert them to our own validation error format.
+// StatusCausesFromAPIStatus extracts status causes from a Kubernetes apierrors.APIStatus error.
+// NOTE: we use this to convert them to our own validation/conflict error format.
 func StatusCausesFromAPIStatus(err error) []metav1.StatusCause {
 	// ensure this is an APIStatus error
 	var statusErr apierrors.APIStatus
@@ -40,9 +40,9 @@ func StatusCausesFromAPIStatus(err error) []metav1.StatusCause {
 		return nil
 	}
 
-	// only attempt to extract if the status is a validation error
+	// only attempt to extract causes if the status has details
 	errStatus := statusErr.Status()
-	if errStatus.Reason != metav1.StatusReasonInvalid {
+	if errStatus.Details == nil {
 		return nil
 	}