From 2dc45fda20ba8dc43f608fa1bf2962b0582db6ed Mon Sep 17 00:00:00 2001 From: Mallory Hill Date: Wed, 29 Jul 2026 12:18:18 -0400 Subject: [PATCH] HYPERFLEET-1369 - refactor: Flatten handlerConfig --- AGENTS.md | 11 +- CLAUDE.md | 10 +- CONTRIBUTING.md | 2 +- pkg/handlers/CLAUDE.md | 122 +++++- pkg/handlers/framework.go | 289 ------------ pkg/handlers/framework_test.go | 1 - pkg/handlers/helpers.go | 142 +++++- pkg/handlers/helpers_test.go | 408 +++++++++++++++++ pkg/handlers/name_validation_handler_test.go | 94 ---- pkg/handlers/resource_handler.go | 380 ++++++++-------- pkg/handlers/resource_handler_test.go | 65 +++ pkg/handlers/resource_status_handler.go | 72 +-- pkg/handlers/root_resource_handler.go | 436 ++++++++++--------- pkg/handlers/validation.go | 2 + 14 files changed, 1175 insertions(+), 859 deletions(-) delete mode 100755 pkg/handlers/framework.go delete mode 100755 pkg/handlers/framework_test.go create mode 100644 pkg/handlers/helpers_test.go delete mode 100644 pkg/handlers/name_validation_handler_test.go diff --git a/AGENTS.md b/AGENTS.md index cd40fb5f..62736281 100755 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,8 +84,7 @@ cmd/hyperfleet-api/ # Entry point + subcommands (serve, migrate) environments/ # Environment configs (development, unit_testing, etc.) pkg/ api/openapi/ # GENERATED — models + embedded spec (never edit) - handlers/ # HTTP handlers using handlerConfig pipeline - framework.go # handle/handleGet/handleList/handleDelete pipeline + handlers/ # HTTP handler pattern, validation and error handling services/ # Service interfaces + sqlXxxService implementations dao/ # DAO interfaces + sqlXxxDao implementations db/ # SessionFactory, transaction middleware, migrations @@ -125,14 +124,6 @@ Use constructor functions from `pkg/errors/errors.go`: `NotFound()`, `Validation Use `pkg/logger/` — `logger.Info(ctx, "msg")`, `logger.With(ctx, "key", val).Error("msg")`. Never use `fmt.Println` or `log.Print`. -### Handlers - -Use `handlerConfig` pipeline from `pkg/handlers/framework.go`: - -- `handle(w, r, cfg, status)` — unmarshal → validate → action → respond -- `handleGet/handleList/handleDelete` — no-body variants -- Validation: `func() *errors.ServiceError`; Action: `func() (interface{}, *errors.ServiceError)` - ### Services Interface + `sql*Service` struct. Constructor injection of DAOs. Return `*errors.ServiceError`. Add `//go:generate mockgen` directive for mocks. diff --git a/CLAUDE.md b/CLAUDE.md index bb4ff7c6..a3b31e90 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,10 +65,10 @@ Use the structured logging API — never `fmt.Println` or `log.Print`: - Chainable: `logger.With(ctx, "key", value).WithError(err).Error("failed")` ### Handler Pipeline -Handlers use the `handlerConfig` pipeline — Reference: `pkg/handlers/framework.go` -- `handle()` — full pipeline: unmarshal body → validate → action → respond -- `handleGet()` / `handleList()` / `handleDelete()` — no body variants -- Validation functions return `*errors.ServiceError`; action functions return `(interface{}, *errors.ServiceError)` +HTTP handlers (ResourceHandler, RootResourceHandler) are thin orchestration layers: +- Reference: `pkg/handlers/` +- Decode and validate requests, call services, marshal responses, handle errors +- No business logic — that lives in `pkg/services/` ### Service Pattern Interface + `sql*Service` implementation with constructor injection: @@ -117,7 +117,7 @@ No per-entity Go code needed. See `plugins/CLAUDE.md` for details. Subdirectories contain context-specific guidance that loads when you work in those areas: -- `pkg/handlers/CLAUDE.md` — Handler pipeline and handlerConfig patterns +- `pkg/handlers/CLAUDE.md` — Handler patterns, validation, and error handling - `pkg/services/CLAUDE.md` — Service interface and status aggregation patterns - `pkg/dao/CLAUDE.md` — DAO interface, session access, and rollback patterns - `pkg/db/CLAUDE.md` — SessionFactory and transaction middleware diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 477d48c0..64fa4817 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,7 +62,7 @@ hyperfleet-api/ │ ├── dao/ # Data access layer (database interactions) │ ├── db/ # Database session factory, migrations, transaction middleware │ ├── errors/ # RFC 9457 Problem Details error model -│ ├── handlers/ # HTTP request handlers using handlerConfig pipeline +│ ├── handlers/ # HTTP handler pattern, validation and error handling │ ├── logger/ # Structured logging (slog-based) │ ├── presenters/ # Response presenters (DAO models → API responses) │ └── services/ # Business logic layer (status aggregation, validation) diff --git a/pkg/handlers/CLAUDE.md b/pkg/handlers/CLAUDE.md index 46d9952a..d699e8d7 100644 --- a/pkg/handlers/CLAUDE.md +++ b/pkg/handlers/CLAUDE.md @@ -1,35 +1,115 @@ # Claude Code Guidelines for Handlers -Handlers use the `handlerConfig` pipeline defined in `framework.go`. +## Request Processing -## Pipeline Functions +Handlers follow a standard pipeline: -- `handle(w, r, cfg, httpStatus)` — Full pipeline: unmarshal `cfg.MarshalInto` from request body → run `cfg.Validate` functions → execute `cfg.Action` → write response -- `handleGet(w, r, cfg)` — No body read, action only, returns 200 -- `handleList(w, r, cfg)` — No body read, action only, returns 200 -- `handleDelete(w, r, cfg)` — No body read, action only, returns 204 -- `handleCreateWithNoContent(w, r, cfg)` — Body read + validate, returns 204 if action returns nil +1. **Decode and validate** — Use `decodeAndValidate(r, &req, validateFuncs)` to unmarshal request body and run validation + - Pass `"strict"` as 4th parameter for PATCH requests only to reject unknown/immutable fields via `DisallowUnknownFields()` + - Validation functions return `*errors.ServiceError` on failure +2. **Convert** — Use `presenters.Convert*()` to convert request types to internal models +3. **Call service** — Pass converted models to service layer methods +4. **Present** — Use `presenters.Present*()` to convert internal models to response types +5. **Write response** — Use `writeJSONResponse(w, r, statusCode, data)` or `handleError(r, w, err)` -## handlerConfig Fields +## Handler Types -- `MarshalInto`: pointer to struct for JSON unmarshalling (e.g., `&openapi.ResourceCreateRequest{}`) -- `Validate`: slice of `func() *errors.ServiceError` — each returns nil on success -- `Action`: `func() (interface{}, *errors.ServiceError)` — core business logic -- `ErrorHandler`: optional custom error handler (rarely used) +- **ResourceHandler** — Handles both flat (`/api/hyperfleet/v1/clusters`) and owner-nested (`/api/hyperfleet/v1/clusters/{parent_id}/nodepools`) routes for a single entity kind + - Extract parent_id to determine route type - `r.PathValue("parent_id")` + - See struct comment for safety invariant (plugins must register correctly) +- **RootResourceHandler** — Handles kind-agnostic routes (`/api/hyperfleet/v1/resources`, `/api/hyperfleet/v1/resources/{id}`) + - Resolves entity kind dynamically via `registry.Get(kind)` + - Handles adapter status operations (`/api/hyperfleet/v1/resources/{id}/statuses`) -## Patterns +## Validation Patterns -- Validation helpers check fields on the unmarshalled struct and return `errors.Validation("reason")` on failure -- Use `presenters.Convert*()` to convert request types to internal models before calling services -- Use `presenters.Present*()` to convert internal models to response types -- Handler structs hold service interfaces — never access DAOs directly from handlers -- Extract path params via `r.PathValue("id")` (stdlib `net/http.ServeMux`, Go 1.22+) +Validation functions close over the request struct and return `func() *errors.ServiceError`: -## Error Responses +```go +validateFuncs := []validate{ + validateKind(&req, "Kind", "kind", expectedKind), + validateName(&req, "Name", "name", minLen, maxLen), + validateSpec(&req, "Spec", "spec"), + validateLabels(&req, "Labels"), +} +``` -Errors are written via `handleError()` in `framework.go` which calls `err.AsProblemDetails()` for RFC 9457 format. The trace ID comes from `r.Header.Get("X-Request-Id")`. +Common validators (in `validation.go`): +- `validateKind(req, fieldName, jsonName, expectedKind)` — Ensures `kind` matches descriptor +- `validateName(req, fieldName, jsonName, minLen, maxLen)` — Name length constraints +- `validateSpec(req, fieldName, jsonName)` — Spec is non-nil +- `validateLabels(req, fieldName)` — Labels follow RFC 1123 rules +- `validatePatchRequest(req)` — At least one field is set +- `validateNotEmpty(req, fieldName, jsonName)` — String field is non-empty +- `validateMaxLength(req, fieldName, jsonName, maxLen)` — String length constraint -## Related CLAUDE.md Files +## Owner Verification + +For nested routes, verify the child belongs to the parent: + +```go +if err := h.checkOwnership(r, id); err != nil { + handleError(r, w, err) + return +} +``` + +This is a no-op for flat routes (no `parent_id` in path). + +## Error Handling + +Use `handleError(r, w, err)` to write RFC 9457 Problem Details responses: +- Logs errors with structured fields (trace ID, error code, HTTP status) +- Automatically extracts trace ID from request context +- Sets `instance` to the request path +- Error constructor functions (from `pkg/errors`) + + +## Path Parameters + +Extract route parameters with r.PathValue + +```go +id := r.PathValue("id") +// Extract parent_id +if parentID := r.PathValue("parent_id"); parentID != "" { + if _, svcErr := h.resourceService.GetByOwner(ctx, h.descriptor.Kind, id, parentID); svcErr != nil { + handleError(r, w, svcErr) + return + } +} +``` + +## Field Filtering + +Apply `?fields=` query parameter filtering: + +```go +result, svcErr := applyFieldFilter(r, presenters.PresentResource(resource)) +if svcErr != nil { + handleError(r, w, svcErr) + return +} + +``` + +For list responses: +```go +if listArgs.Fields != nil { + filtered, svcErr := presenters.SliceFilter(listArgs.Fields, result) + if svcErr != nil { + handleError(r, w, svcErr) + return // REQUIRED — omitting this causes a double-write + } + writeJSONResponse(w, r, http.StatusOK, filtered) + return // REQUIRED — prevents falling through to the unfiltered write below +} +writeJSONResponse(w, r, http.StatusOK, result) +``` + +## Related Files - `pkg/services/CLAUDE.md` — Service layer patterns - `pkg/errors/CLAUDE.md` — Error constructors and codes +- `helpers.go` — `writeJSONResponse`,`decodeAndValidate`, `handleError`, `applyFieldFilter` implementations +- `validation.go` — Validator functions (`validateKind`, `validateName`, `validateSpec`, etc.) \ No newline at end of file diff --git a/pkg/handlers/framework.go b/pkg/handlers/framework.go deleted file mode 100755 index 8f5cb414..00000000 --- a/pkg/handlers/framework.go +++ /dev/null @@ -1,289 +0,0 @@ -package handlers - -import ( - "bytes" - "encoding/json" - goerrors "errors" - "io" - "net/http" - "reflect" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/presenters" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/response" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" -) - -// handlerConfig defines the common things each REST controller must do. -// The corresponding handle() func runs the basic handlerConfig. -// This is not meant to be an HTTP framework or anything larger than simple CRUD in handlers. -// -// MarshalInto is a pointer to the object to hold the unmarshaled JSON. -// Validate is a list of validation function that run in order, returning fast on the first error. -// Action is the specific logic a handler must take (e.g, find an object, save an object) -// ErrorHandler is the way errors are returned to the client -type handlerConfig struct { - MarshalInto interface{} - Action httpAction - ErrorHandler errorHandlerFunc - Validate []validate - StrictUnmarshal bool -} - -type validate func() *errors.ServiceError -type errorHandlerFunc func(r *http.Request, w http.ResponseWriter, err *errors.ServiceError) -type httpAction func() (interface{}, *errors.ServiceError) - -func handleError(r *http.Request, w http.ResponseWriter, err *errors.ServiceError) { - traceID, _ := logger.GetRequestID(r.Context()) - instance := r.URL.Path - - // Log with RFC 9457 code format - if err.HTTPCode >= 400 && err.HTTPCode <= 499 { - logger.With(r.Context(), - "code", err.RFC9457Code, - "http_code", err.HTTPCode, - "reason", err.Reason).Info("Client error response") - } else { - logger.With(r.Context(), - "code", err.RFC9457Code, - "http_code", err.HTTPCode, - "reason", err.Reason).Error("Server error response") - } - - response.WriteProblemDetailsResponse(w, r, err.HTTPCode, err.AsProblemDetails(instance, traceID)) -} - -// applyFieldFilter applies field filtering to a presented resource based on the ?fields query parameter. -// If no fields are specified, it returns the original presented resource. -// If fields are specified, it filters the resource and returns only the requested fields. -// Note: This function only validates the fields parameter, not pagination parameters, -// to avoid rejecting irrelevant query params on single-resource GET endpoints. -func applyFieldFilter(r *http.Request, presented interface{}) (interface{}, *errors.ServiceError) { - fields := ensureIDField(normalizeList(r.URL.Query()["fields"])) - if fields != nil { - filtered, filterErr := presenters.FilterSingle(fields, presented) - if filterErr != nil { - return nil, filterErr - } - return filtered, nil - } - return presented, nil -} - -func handle(w http.ResponseWriter, r *http.Request, cfg *handlerConfig, httpStatus int) { - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } - - bodyBytes, err := io.ReadAll(r.Body) - if err != nil { - handleError(r, w, errors.MalformedRequest("Unable to read request body: %s", err)) - return - } - - if cfg.StrictUnmarshal { - dec := json.NewDecoder(bytes.NewReader(bodyBytes)) - dec.DisallowUnknownFields() - if err := dec.Decode(cfg.MarshalInto); err != nil { - if typeErr := cleanTypeMismatchError(err); typeErr != nil { - handleError(r, w, typeErr) - return - } - handleError(r, w, errors.BadRequest("Unknown or immutable field in request body: %s", err)) - return - } - } else { - if err := json.Unmarshal(bodyBytes, &cfg.MarshalInto); err != nil { - if typeErr := cleanTypeMismatchError(err); typeErr != nil { - handleError(r, w, typeErr) - return - } - handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) - return - } - } - - for _, v := range cfg.Validate { - err := v() - if err != nil { - cfg.ErrorHandler(r, w, err) - return - } - } - - result, serviceErr := cfg.Action() - - switch { - case serviceErr != nil: - cfg.ErrorHandler(r, w, serviceErr) - default: - writeJSONResponse(w, r, httpStatus, result) - } - -} - -func handleSoftDelete(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { - httpStatus := http.StatusAccepted - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } - for _, v := range cfg.Validate { - err := v() - if err != nil { - cfg.ErrorHandler(r, w, err) - return - } - } - - result, serviceErr := cfg.Action() - - switch { - case serviceErr != nil: - cfg.ErrorHandler(r, w, serviceErr) - default: - writeJSONResponse(w, r, httpStatus, result) - } - -} - -func cleanTypeMismatchError(err error) *errors.ServiceError { - var typeErr *json.UnmarshalTypeError - if !goerrors.As(err, &typeErr) { - return nil - } - return errors.Validation("field '%s' must be %s", typeErr.Field, describeJSONKind(typeErr.Type.Kind())) -} - -func describeJSONKind(k reflect.Kind) string { - switch k { - case reflect.Map, reflect.Struct: - return "an object" - case reflect.Slice, reflect.Array: - return "an array" - case reflect.String: - return "a string" - case reflect.Bool: - return "a boolean" - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, - reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, - reflect.Float32, reflect.Float64: - return "a number" - default: - // Fallback for kinds json.UnmarshalTypeError.Type should never report - // here (e.g. Ptr, Interface) — avoids mislabeling as a number. - return "the correct type" - } -} - -func handleGet(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } - - result, serviceErr := cfg.Action() - switch serviceErr { - case nil: - writeJSONResponse(w, r, http.StatusOK, result) - default: - cfg.ErrorHandler(r, w, serviceErr) - } -} - -func handleList(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } - - results, serviceError := cfg.Action() - if serviceError != nil { - cfg.ErrorHandler(r, w, serviceError) - return - } - writeJSONResponse(w, r, http.StatusOK, results) -} - -// handleForceDelete reads the request body, unmarshals it into cfg.MarshalInto, -// runs validators, executes the action, and returns 204 No Content on success. -func handleForceDelete(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } - - bytes, err := io.ReadAll(r.Body) - if err != nil { - handleError(r, w, errors.MalformedRequest("Unable to read request body: %s", err)) - return - } - - err = json.Unmarshal(bytes, &cfg.MarshalInto) - if err != nil { - if typeErr := cleanTypeMismatchError(err); typeErr != nil { - handleError(r, w, typeErr) - return - } - handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) - return - } - - for _, v := range cfg.Validate { - err := v() - if err != nil { - cfg.ErrorHandler(r, w, err) - return - } - } - - _, serviceErr := cfg.Action() - if serviceErr != nil { - cfg.ErrorHandler(r, w, serviceErr) - return - } - - w.WriteHeader(http.StatusNoContent) -} - -// handleCreateWithNoContent handles create requests that may return 204 No Content -// If action returns (nil, nil), it means a no-op and returns 204 No Content -// Otherwise, it returns 201 Created with the result -func handleCreateWithNoContent(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { - if cfg.ErrorHandler == nil { - cfg.ErrorHandler = handleError - } - - bytes, err := io.ReadAll(r.Body) - if err != nil { - handleError(r, w, errors.MalformedRequest("Unable to read request body: %s", err)) - return - } - - err = json.Unmarshal(bytes, &cfg.MarshalInto) - if err != nil { - if typeErr := cleanTypeMismatchError(err); typeErr != nil { - handleError(r, w, typeErr) - return - } - handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) - return - } - - for _, v := range cfg.Validate { - err := v() - if err != nil { - cfg.ErrorHandler(r, w, err) - return - } - } - - result, serviceErr := cfg.Action() - - switch { - case serviceErr != nil: - cfg.ErrorHandler(r, w, serviceErr) - case result == nil: - // No-op case: return 204 No Content - w.WriteHeader(http.StatusNoContent) - default: - writeJSONResponse(w, r, http.StatusCreated, result) - } -} diff --git a/pkg/handlers/framework_test.go b/pkg/handlers/framework_test.go deleted file mode 100755 index 5ac8282f..00000000 --- a/pkg/handlers/framework_test.go +++ /dev/null @@ -1 +0,0 @@ -package handlers diff --git a/pkg/handlers/helpers.go b/pkg/handlers/helpers.go index acecac89..72db7c3a 100755 --- a/pkg/handlers/helpers.go +++ b/pkg/handlers/helpers.go @@ -2,9 +2,18 @@ package handlers import ( "encoding/json" + goerrors "errors" + "io" "net/http" + "reflect" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/presenters" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/response" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) func writeJSONResponse(w http.ResponseWriter, r *http.Request, code int, payload interface{}) { @@ -38,4 +47,135 @@ func writeJSONResponse(w http.ResponseWriter, r *http.Request, code int, payload } } -// Prepare a 'list' of non-db-backed resources +// decodeAndValidate unmarshals the request body and runs validation functions. +// Pass "strict" to reject unknown fields (use for PATCH to catch immutable field changes). +func decodeAndValidate(r *http.Request, req any, validateFuncs []validate, decodeType ...string) *errors.ServiceError { + dec := json.NewDecoder(r.Body) + // Add "strict" decodeType to reject unknown fields in request body + if len(decodeType) > 0 { + if decodeType[0] == "strict" { + dec.DisallowUnknownFields() + } else { + return errors.GeneralError("invalid decode mode: %s (must be 'strict')", decodeType[0]) + } + } + + // Default behavior is to silently ignore unknown fields + if err := dec.Decode(req); err != nil { + if err == io.EOF { + return errors.MalformedRequest("Request body is required but was empty") + } + if typeErr := cleanTypeMismatchError(err); typeErr != nil { + return typeErr + } + return errors.MalformedRequest("Invalid request format: %s", err) + } + + for _, validateFunc := range validateFuncs { + if svcErr := validateFunc(); svcErr != nil { + return svcErr + } + } + return nil +} + +func handleError(r *http.Request, w http.ResponseWriter, err *errors.ServiceError) { + traceID, _ := logger.GetRequestID(r.Context()) + instance := r.URL.Path + + // Log with RFC 9457 code format + if err.HTTPCode >= 400 && err.HTTPCode <= 499 { + logger.With(r.Context(), + "code", err.RFC9457Code, + "http_code", err.HTTPCode, + "reason", err.Reason).Info("Client error response") + } else { + logger.With(r.Context(), + "code", err.RFC9457Code, + "http_code", err.HTTPCode, + "reason", err.Reason).Error("Server error response") + } + + response.WriteProblemDetailsResponse(w, r, err.HTTPCode, err.AsProblemDetails(instance, traceID)) +} + +// applyFieldFilter applies field filtering to a presented resource based on the ?fields query parameter. +// If no fields are specified, it returns the original presented resource. +// If fields are specified, it filters the resource and returns only the requested fields. +// Note: This function only validates the fields parameter, not pagination parameters, +// to avoid rejecting irrelevant query params on single-resource GET endpoints. +func applyFieldFilter(r *http.Request, presented interface{}) (interface{}, *errors.ServiceError) { + fields := ensureIDField(normalizeList(r.URL.Query()["fields"])) + if fields != nil { + filtered, filterErr := presenters.FilterSingle(fields, presented) + if filterErr != nil { + return nil, filterErr + } + return filtered, nil + } + return presented, nil +} + +func cleanTypeMismatchError(err error) *errors.ServiceError { + var typeErr *json.UnmarshalTypeError + if !goerrors.As(err, &typeErr) { + return nil + } + return errors.Validation("field '%s' must be %s", typeErr.Field, describeJSONKind(typeErr.Type.Kind())) +} + +func describeJSONKind(k reflect.Kind) string { + switch k { + case reflect.Map, reflect.Struct: + return "an object" + case reflect.Slice, reflect.Array: + return "an array" + case reflect.String: + return "a string" + case reflect.Bool: + return "a boolean" + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return "a number" + default: + // Fallback for kinds json.UnmarshalTypeError.Type should never report + // here (e.g. Ptr, Interface) — avoids mislabeling as a number. + return "the correct type" + } +} + +func convertResourcePatch(req *openapi.ResourcePatchRequest) *api.ResourcePatch { + patch := &api.ResourcePatch{} + if req.Spec != nil { + patch.Spec = *req.Spec + } + if req.Labels != nil { + patch.Labels = *req.Labels + } + if req.References != nil { + patch.References = *req.References + } + return patch +} + +// extractReferences unwraps the optional references pointer from an API request. +// Returns nil when no references are supplied (nil pointer), or the map value. +func extractReferences(refs *api.ReferenceMap) api.ReferenceMap { + if refs == nil { + return nil + } + return *refs +} + +// childCreateRejection returns a validation error indicating the resource must be +// created via its parent's nested route. +func childCreateRejection(descriptor registry.EntityDescriptor) *errors.ServiceError { + parent := registry.MustGet(descriptor.ParentKind) + svcErr := errors.Validation( + "kind %q is a child kind; create it via /%s/{id}/%s", + descriptor.Kind, parent.Plural, descriptor.Plural, + ) + svcErr.HTTPCode = http.StatusUnprocessableEntity + return svcErr +} diff --git a/pkg/handlers/helpers_test.go b/pkg/handlers/helpers_test.go new file mode 100644 index 00000000..076dbc17 --- /dev/null +++ b/pkg/handlers/helpers_test.go @@ -0,0 +1,408 @@ +package handlers + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + + . "github.com/onsi/gomega" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" +) + +func TestWriteJSONResponse(t *testing.T) { + RegisterTestingT(t) + + type testPayload struct { + ID string `json:"id"` + Name string `json:"name"` + } + + t.Run("writes JSON response with correct headers", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + payload := testPayload{ID: "999999", Name: "test"} + + writeJSONResponse(w, r, http.StatusOK, payload) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) + Expect(w.Header().Get("Vary")).To(Equal("Authorization")) + + var response testPayload + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).To(BeNil()) + Expect(response.ID).To(Equal("999999")) + Expect(response.Name).To(Equal("test")) + }) + + t.Run("handles nil payload correctly", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + + writeJSONResponse(w, r, http.StatusNoContent, nil) + + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) + Expect(w.Body.Len()).To(Equal(0)) + }) + + t.Run("sets correct status code for different responses", func(t *testing.T) { + testCases := []struct { + name string + statusCode int + }{ + {"200 OK", http.StatusOK}, + {"201 Created", http.StatusCreated}, + {"202 Accepted", http.StatusAccepted}, + {"204 No Content", http.StatusNoContent}, + {"400 Bad Request", http.StatusBadRequest}, + {"404 Not Found", http.StatusNotFound}, + {"500 Internal Server Error", http.StatusInternalServerError}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + + writeJSONResponse(w, r, tc.statusCode, testPayload{ID: "test"}) + + Expect(w.Code).To(Equal(tc.statusCode)) + }) + } + }) + + t.Run("handles empty payload", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + + writeJSONResponse(w, r, http.StatusOK, map[string]any{}) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Body.String()).To(Equal("{}")) + }) + + t.Run("handles arrays", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + payload := []testPayload{ + {ID: "1", Name: "first"}, + {ID: "2", Name: "second"}, + } + + writeJSONResponse(w, r, http.StatusOK, payload) + + Expect(w.Code).To(Equal(http.StatusOK)) + var response []testPayload + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).To(BeNil()) + Expect(response).To(HaveLen(2)) + Expect(response[0].ID).To(Equal("1")) + Expect(response[1].ID).To(Equal("2")) + }) +} +func TestDecodeAndValidate(t *testing.T) { + RegisterTestingT(t) + + const validJSON = `{"name":"test","value":42}` + const unknownFieldJSON = `{"name":"test","value":42,"unknown":"field"}` + type testRequest struct { + Name string `json:"name"` + Value int `json:"value"` + } + + t.Run("default decodeType - valid JSON", func(t *testing.T) { + body := validJSON + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + var validateCalled bool + validateFuncs := []validate{ + func() *errors.ServiceError { + validateCalled = true + return nil + }, + } + + err := decodeAndValidate(r, &req, validateFuncs) + + Expect(err).To(BeNil()) + Expect(req.Name).To(Equal("test")) + Expect(req.Value).To(Equal(42)) + Expect(validateCalled).To(BeTrue()) + }) + + t.Run("default decodeType - allows unknown fields", func(t *testing.T) { + body := unknownFieldJSON // includes "unknown":"field" which will be silently discarded + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}) + + Expect(err).To(BeNil()) + Expect(req.Name).To(Equal("test")) + Expect(req.Value).To(Equal(42)) + // The "unknown" field is silently ignored by json.Unmarshal (no error, not stored anywhere) + }) + + t.Run("undefined decodeType - accepts unknown field JSON", func(t *testing.T) { + body := unknownFieldJSON + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}, "unknown") + + Expect(err).NotTo(BeNil()) + Expect(err.Reason).To(ContainSubstring("invalid decode mode: unknown (must be 'strict')")) + }) + + t.Run("strict mode - rejects unknown fields", func(t *testing.T) { + body := unknownFieldJSON + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}, "strict") + + Expect(err).NotTo(BeNil()) + Expect(err.HTTPCode).To(Equal(http.StatusBadRequest)) + Expect(err.Reason).To(ContainSubstring("Invalid request format")) + }) + + t.Run("strict mode - accepts valid JSON without extra fields", func(t *testing.T) { + body := validJSON + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}, "strict") + + Expect(err).To(BeNil()) + Expect(req.Name).To(Equal("test")) + Expect(req.Value).To(Equal(42)) + }) + + t.Run("malformed JSON returns error", func(t *testing.T) { + body := `{"name":"test",invalid}` + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}) + + Expect(err).NotTo(BeNil()) + Expect(err.HTTPCode).To(Equal(http.StatusBadRequest)) + Expect(err.Reason).To(ContainSubstring("Invalid request format")) + }) + + t.Run("type mismatch returns validation error", func(t *testing.T) { + body := `{"name":"test","value":"not-a-number"}` + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}) + + Expect(err).NotTo(BeNil()) + Expect(err.HTTPCode).To(Equal(http.StatusBadRequest)) + Expect(err.Reason).To(ContainSubstring("must be a number")) + }) + + t.Run("validation function fails", func(t *testing.T) { + body := validJSON + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body)) + var req testRequest + validateFuncs := []validate{ + func() *errors.ServiceError { + return errors.Validation("name is too short") + }, + } + + err := decodeAndValidate(r, &req, validateFuncs) + + Expect(err).NotTo(BeNil()) + Expect(err.Reason).To(ContainSubstring("name is too short")) + }) + + t.Run("empty body returns error", func(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/test", strings.NewReader("")) + var req testRequest + + err := decodeAndValidate(r, &req, []validate{}) + + Expect(err).NotTo(BeNil()) + Expect(err.HTTPCode).To(Equal(http.StatusBadRequest)) + Expect(err.Reason).To(ContainSubstring("Request body is required but was empty")) + }) +} + +func TestCleanTypeMismatchError(t *testing.T) { + RegisterTestingT(t) + + type testStruct struct { + Name string `json:"name"` + Count int `json:"count"` + Flag bool `json:"flag"` + } + + t.Run("string type mismatch", func(t *testing.T) { + body := `{"name":123}` + var req testStruct + err := json.Unmarshal([]byte(body), &req) + + svcErr := cleanTypeMismatchError(err) + + Expect(svcErr).NotTo(BeNil()) + Expect(svcErr.Reason).To(ContainSubstring("'name' must be a string")) + }) + + t.Run("number type mismatch", func(t *testing.T) { + body := `{"count":"not-a-number"}` + var req testStruct + err := json.Unmarshal([]byte(body), &req) + + svcErr := cleanTypeMismatchError(err) + + Expect(svcErr).NotTo(BeNil()) + Expect(svcErr.Reason).To(ContainSubstring("'count' must be a number")) + }) + + t.Run("boolean type mismatch", func(t *testing.T) { + body := `{"flag":"true"}` + var req testStruct + err := json.Unmarshal([]byte(body), &req) + + svcErr := cleanTypeMismatchError(err) + + Expect(svcErr).NotTo(BeNil()) + Expect(svcErr.Reason).To(ContainSubstring("'flag' must be a boolean")) + }) + + t.Run("non-type-mismatch error returns nil", func(t *testing.T) { + body := `{invalid json}` + var req testStruct + err := json.Unmarshal([]byte(body), &req) + + svcErr := cleanTypeMismatchError(err) + + Expect(svcErr).To(BeNil()) + }) + + t.Run("nil error returns nil", func(t *testing.T) { + svcErr := cleanTypeMismatchError(nil) + + Expect(svcErr).To(BeNil()) + }) +} + +func TestDescribeJSONKind(t *testing.T) { + RegisterTestingT(t) + + tests := []struct { + expected string + kind reflect.Kind + }{ + {"an object", reflect.Map}, + {"an object", reflect.Struct}, + {"an array", reflect.Slice}, + {"an array", reflect.Array}, + {"a string", reflect.String}, + {"a boolean", reflect.Bool}, + {"a number", reflect.Int}, + {"a number", reflect.Int8}, + {"a number", reflect.Int16}, + {"a number", reflect.Int32}, + {"a number", reflect.Int64}, + {"a number", reflect.Uint}, + {"a number", reflect.Uint8}, + {"a number", reflect.Uint16}, + {"a number", reflect.Uint32}, + {"a number", reflect.Uint64}, + {"a number", reflect.Float32}, + {"a number", reflect.Float64}, + {"the correct type", reflect.Invalid}, + } + + for _, tt := range tests { + t.Run(tt.kind.String(), func(t *testing.T) { + result := describeJSONKind(tt.kind) + Expect(result).To(Equal(tt.expected)) + }) + } +} + +func TestApplyFieldFilter(t *testing.T) { + RegisterTestingT(t) + + type testResource struct { + ID string `json:"id"` + Name string `json:"name"` + Metadata string `json:"metadata"` + } + + presented := testResource{ + ID: "123", + Name: "test", + Metadata: "2026-07-28", + } + + t.Run("no fields parameter returns original", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + + result, err := applyFieldFilter(r, presented) + + Expect(err).To(BeNil()) + Expect(result).To(Equal(presented)) + }) + + t.Run("fields parameter filters correctly", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test?fields=id,name", nil) + + result, err := applyFieldFilter(r, presented) + + Expect(err).To(BeNil()) + filtered, ok := result.(map[string]any) + Expect(ok).To(BeTrue()) + Expect(filtered).To(HaveKey("id")) + Expect(filtered).To(HaveKey("name")) + Expect(filtered).NotTo(HaveKey("metadata")) + }) + + t.Run("invalid fields parameter returns error", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test?fields=nonexistent", nil) + + result, err := applyFieldFilter(r, presented) + + Expect(err).NotTo(BeNil()) + Expect(err.HTTPCode).To(Equal(http.StatusBadRequest)) + Expect(result).To(BeNil()) + }) +} + +func TestHandleError(t *testing.T) { + RegisterTestingT(t) + + t.Run("writes problem details response", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + svcErr := errors.NotFound("resource not found") + + handleError(r, w, svcErr) + + Expect(w.Code).To(Equal(http.StatusNotFound)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/problem+json")) + + var response map[string]interface{} + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).To(BeNil()) + Expect(response["detail"]).To(ContainSubstring("resource not found")) + }) + + t.Run("sets HTTP status code correctly", func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/test", nil) + w := httptest.NewRecorder() + svcErr := errors.Validation("validation failed") + + handleError(r, w, svcErr) + + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) +} diff --git a/pkg/handlers/name_validation_handler_test.go b/pkg/handlers/name_validation_handler_test.go deleted file mode 100644 index fc6b0c72..00000000 --- a/pkg/handlers/name_validation_handler_test.go +++ /dev/null @@ -1,94 +0,0 @@ -package handlers - -import ( - "encoding/json" - "fmt" - "net/http" - "net/http/httptest" - "strings" - "testing" - "time" - - . "github.com/onsi/gomega" - "go.uber.org/mock/gomock" - "gorm.io/datatypes" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" -) - -func newNameValidationHandler( - t *testing.T, - ctrl *gomock.Controller, - descriptor registry.EntityDescriptor, -) (*ResourceHandler, *services.MockResourceService) { - t.Helper() - mockResourceSvc := services.NewMockResourceService(ctrl) - handler := NewResourceHandler(descriptor, mockResourceSvc) - return handler, mockResourceSvc -} - -func TestResourceHandler_Create_NameValidation(t *testing.T) { - tests := []struct { - name string - inputName string - nameMinLen int - nameMaxLen int - wantStatus int - }{ - {"too short", "ab", 3, 53, http.StatusBadRequest}, - {"too long", "toolongname", 3, 5, http.StatusBadRequest}, - {"at min length", "abc", 3, 53, http.StatusCreated}, - {"at max length", "abcde", 3, 5, http.StatusCreated}, - {"no validation when zero lengths", "x", 0, 0, http.StatusCreated}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - RegisterTestingT(t) - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - descriptor := registry.EntityDescriptor{ - Kind: "TestEntity", - Plural: "testentities", - NameMinLen: tt.nameMinLen, - NameMaxLen: tt.nameMaxLen, - } - handler, mockSvc := newNameValidationHandler(t, ctrl, descriptor) - - if tt.wantStatus == http.StatusCreated { - now := time.Now() - mockSvc.EXPECT().Create( - gomock.Any(), "TestEntity", gomock.AssignableToTypeOf(&api.Resource{}), gomock.Any(), - ).Return(&api.Resource{ - Meta: api.Meta{ID: "test-id", CreatedTime: now, UpdatedTime: now}, - Kind: "TestEntity", - Name: tt.inputName, - Href: "/api/hyperfleet/v1/testentities/test-id", - Spec: datatypes.JSON(`{"key":"value"}`), - Generation: 1, - CreatedBy: "system@hyperfleet.local", - UpdatedBy: "system@hyperfleet.local", - }, nil) - } - - body := fmt.Sprintf(`{"kind":"TestEntity","name":%q,"spec":{"key":"value"}}`, tt.inputName) - req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/testentities", strings.NewReader(body)) - req.Header.Set("Content-Type", "application/json") - rr := httptest.NewRecorder() - - handler.Create(rr, req) - Expect(rr.Code).To(Equal(tt.wantStatus)) - - if tt.wantStatus == http.StatusCreated { - var resp openapi.Resource - err := json.Unmarshal(rr.Body.Bytes(), &resp) - Expect(err).NotTo(HaveOccurred()) - Expect(resp.Name).To(Equal(tt.inputName)) - } - }) - } -} diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index d8a4840f..999f98de 100644 --- a/pkg/handlers/resource_handler.go +++ b/pkg/handlers/resource_handler.go @@ -11,8 +11,14 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" ) -// ResourceHandler serves both flat and owner-nested routes for one entity kind. -// Each verb branches on whether parent_id is present in the path. +// ResourceHandler serves both flat and owner-nested routes for a single entity +// kind. Every method branches on whether "parent_id" is present via +// r.PathValue("parent_id") rather than dispatching statically per route. This is only correct because +// plugins/entities/plugin.go guarantees the invariant: a nested (ParentKind != "") +// descriptor is registered exclusively under a {parent_id} subrouter, and a flat +// descriptor never is. If that registration is ever bypassed — e.g. a nested kind +// wired to a flat route — these branches take the wrong path silently (Create +// would skip setting owner references instead of erroring). type ResourceHandler struct { service services.ResourceService descriptor registry.EntityDescriptor @@ -30,169 +36,192 @@ func NewResourceHandler( func (h *ResourceHandler) Create(w http.ResponseWriter, r *http.Request) { var req openapi.ResourceCreateRequest - cfg := &handlerConfig{ - MarshalInto: &req, - Validate: []validate{ - validateKind(&req, "Kind", "kind", h.descriptor.Kind), - validateName(&req, "Name", "name", h.descriptor.NameMinLen, h.descriptor.NameMaxLen), - validateSpec(&req, "Spec", "spec"), - validateLabels(&req, "Labels"), - }, - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - - parentID := r.PathValue("parent_id") - if parentID == "" && h.descriptor.ParentKind != "" { - return nil, childCreateRejection(h.descriptor) - } - - var resource *api.Resource - var err error - if parentID != "" { - parent, svcErr := h.service.Get(ctx, h.descriptor.ParentKind, parentID) - if svcErr != nil { - return nil, svcErr - } - resource, err = presenters.ConvertResourceWithOwner(&req, parent.ID, parent.Kind, parent.Href) - } else { - resource, err = presenters.ConvertResource(&req) - } - if err != nil { - return nil, errors.GeneralError("failed to convert resource: %v", err) - } - - refs := extractReferences(req.References) - resource, svcErr := h.service.Create(ctx, h.descriptor.Kind, resource, refs) - if svcErr != nil { - return nil, svcErr - } - return presenters.PresentResource(resource), nil - }, + validateFuncs := []validate{ + validateKind(&req, "Kind", "kind", h.descriptor.Kind), + validateName(&req, "Name", "name", h.descriptor.NameMinLen, h.descriptor.NameMaxLen), + validateSpec(&req, "Spec", "spec"), + validateLabels(&req, "Labels"), } - handle(w, r, cfg, http.StatusCreated) + if err := decodeAndValidate(r, &req, validateFuncs); err != nil { + handleError(r, w, err) + return + } + + ctx := r.Context() + + parentID := r.PathValue("parent_id") + if parentID == "" && h.descriptor.ParentKind != "" { + handleError(r, w, childCreateRejection(h.descriptor)) + return + } + + var resource *api.Resource + var convErr error + if parentID != "" { + parent, err := h.service.Get(ctx, h.descriptor.ParentKind, parentID) + if err != nil { + handleError(r, w, err) + return + } + resource, convErr = presenters.ConvertResourceWithOwner(&req, parent.ID, parent.Kind, parent.Href) + } else { + resource, convErr = presenters.ConvertResource(&req) + } + if convErr != nil { + handleError(r, w, errors.GeneralError("failed to convert resource: %v", convErr)) + return + } + + refs := extractReferences(req.References) + resource, err := h.service.Create(ctx, h.descriptor.Kind, resource, refs) + if err != nil { + handleError(r, w, err) + return + } + + writeJSONResponse(w, r, http.StatusCreated, presenters.PresentResource(resource)) } func (h *ResourceHandler) Get(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - id := r.PathValue("id") - - parentID, err := h.parentIDIfExists(r) - if err != nil { - return nil, err - } - - var resource *api.Resource - if parentID != "" { - resource, err = h.service.GetByOwner(ctx, h.descriptor.Kind, id, parentID) - } else { - resource, err = h.service.Get(ctx, h.descriptor.Kind, id) - } - if err != nil { - return nil, err - } - - return applyFieldFilter(r, presenters.PresentResource(resource)) - }, + ctx := r.Context() + id := r.PathValue("id") + + parentID, err := h.parentIDIfExists(r) + if err != nil { + handleError(r, w, err) + return } - handleGet(w, r, cfg) + + var resource *api.Resource + if parentID != "" { + resource, err = h.service.GetByOwner(ctx, h.descriptor.Kind, id, parentID) + } else { + resource, err = h.service.Get(ctx, h.descriptor.Kind, id) + } + if err != nil { + handleError(r, w, err) + return + } + + result, err := applyFieldFilter(r, presenters.PresentResource(resource)) + if err != nil { + handleError(r, w, err) + return + } + + writeJSONResponse(w, r, http.StatusOK, result) } func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - - parentID, err := h.parentIDIfExists(r) - if err != nil { - return nil, err - } - - listArgs, err := parseListParams(r.URL.Query()) - if err != nil { - return nil, err - } - - var resources api.ResourceList - var paging *api.PagingMeta - if parentID != "" { - resources, paging, err = h.service.ListByOwner(ctx, h.descriptor.Kind, parentID, listArgs) - } else { - resources, paging, err = h.service.List(ctx, h.descriptor.Kind, listArgs) - } - if err != nil { - return nil, err - } - - result := presenters.PresentResourceList(resources, paging) - if listArgs.Fields != nil { - return presenters.SliceFilter(listArgs.Fields, result) - } - return result, nil - }, + ctx := r.Context() + + parentID, err := h.parentIDIfExists(r) + if err != nil { + handleError(r, w, err) + return + } + + listArgs, err := parseListParams(r.URL.Query()) + if err != nil { + handleError(r, w, err) + return + } + + var resources api.ResourceList + var paging *api.PagingMeta + if parentID != "" { + resources, paging, err = h.service.ListByOwner(ctx, h.descriptor.Kind, parentID, listArgs) + } else { + resources, paging, err = h.service.List(ctx, h.descriptor.Kind, listArgs) + } + + if err != nil { + handleError(r, w, err) + return } - handleList(w, r, cfg) + + presented := presenters.PresentResourceList(resources, paging) + if listArgs.Fields != nil { + filtered, err := presenters.SliceFilter(listArgs.Fields, presented) + if err != nil { + handleError(r, w, err) + return + } + writeJSONResponse(w, r, http.StatusOK, filtered) + return + } + writeJSONResponse(w, r, http.StatusOK, presented) } func (h *ResourceHandler) Patch(w http.ResponseWriter, r *http.Request) { var req openapi.ResourcePatchRequest - cfg := &handlerConfig{ - MarshalInto: &req, - StrictUnmarshal: true, - Validate: []validate{ - validatePatchRequest(&req), - validateLabels(&req, "Labels"), - }, - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - - if err := h.checkOwnership(r, id); err != nil { - return nil, err - } - - patch := convertResourcePatch(&req) - resource, err := h.service.Patch(r.Context(), h.descriptor.Kind, id, patch) - if err != nil { - return nil, err - } - return presenters.PresentResource(resource), nil - }, + validateFuncs := []validate{ + validatePatchRequest(&req), + validateLabels(&req, "Labels"), + } + if err := decodeAndValidate(r, &req, validateFuncs, "strict"); err != nil { + handleError(r, w, err) + return + } + + id := r.PathValue("id") + ctx := r.Context() + if err := h.checkOwnership(r, id); err != nil { + handleError(r, w, err) + return + } + + patch := convertResourcePatch(&req) + resource, err := h.service.Patch(ctx, h.descriptor.Kind, id, patch) + if err != nil { + handleError(r, w, err) + return } - handle(w, r, cfg, http.StatusOK) + + writeJSONResponse(w, r, http.StatusOK, presenters.PresentResource(resource)) } func (h *ResourceHandler) Delete(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - - if err := h.checkOwnership(r, id); err != nil { - return nil, err - } - - resource, err := h.service.Delete(r.Context(), h.descriptor.Kind, id) - if err != nil { - return nil, err - } - return presenters.PresentResource(resource), nil - }, + id := r.PathValue("id") + ctx := r.Context() + if err := h.checkOwnership(r, id); err != nil { + handleError(r, w, err) + return + } + + resource, err := h.service.Delete(ctx, h.descriptor.Kind, id) + if err != nil { + handleError(r, w, err) + return } - handleSoftDelete(w, r, cfg) + + writeJSONResponse(w, r, http.StatusAccepted, presenters.PresentResource(resource)) } -// parentIDIfExists returns the parent_id if the parent exists, "" for flat -// routes, or a 404 if parent_id is present but the parent is missing. -func (h *ResourceHandler) parentIDIfExists(r *http.Request) (string, *errors.ServiceError) { - parentID := r.PathValue("parent_id") - if parentID == "" { - return "", nil +func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) { + var req openapi.ForceDeleteRequest + validateFuncs := []validate{ + validateNotEmpty(&req, "Reason", "reason"), + validateMaxLength(&req, "Reason", "reason", maxReasonLength), } - _, err := h.service.Get(r.Context(), h.descriptor.ParentKind, parentID) - if err != nil { - return "", err + if err := decodeAndValidate(r, &req, validateFuncs); err != nil { + handleError(r, w, err) + return } - return parentID, nil + + id := r.PathValue("id") + ctx := r.Context() + if err := h.checkOwnership(r, id); err != nil { + handleError(r, w, err) + return + } + + if err := h.service.ForceDelete(ctx, h.descriptor.Kind, id, req.Reason); err != nil { + handleError(r, w, err) + return + } + + w.WriteHeader(http.StatusNoContent) } // checkOwnership verifies id belongs to parent_id, checking the parent first so @@ -210,61 +239,16 @@ func (h *ResourceHandler) checkOwnership(r *http.Request, id string) *errors.Ser return nil } -// childCreateRejection returns a validation error indicating the resource must be -// created via its parent's nested route. -func childCreateRejection(descriptor registry.EntityDescriptor) *errors.ServiceError { - parent := registry.MustGet(descriptor.ParentKind) - svcErr := errors.Validation( - "kind %q is a child kind; create it via /%s/{id}/%s", - descriptor.Kind, parent.Plural, descriptor.Plural, - ) - svcErr.HTTPCode = http.StatusUnprocessableEntity - return svcErr -} - -func convertResourcePatch(req *openapi.ResourcePatchRequest) *api.ResourcePatch { - patch := &api.ResourcePatch{} - if req.Spec != nil { - patch.Spec = *req.Spec - } - if req.Labels != nil { - patch.Labels = *req.Labels - } - if req.References != nil { - patch.References = *req.References - } - return patch -} - -// extractReferences unwraps the optional references pointer from an API request. -// Returns nil when no references are supplied (nil pointer), or the map value. -func extractReferences(refs *api.ReferenceMap) api.ReferenceMap { - if refs == nil { - return nil +// parentIDIfExists returns the parent_id if the parent exists, "" for flat +// routes, or a 404 if parent_id is present but the parent is missing. +func (h *ResourceHandler) parentIDIfExists(r *http.Request) (string, *errors.ServiceError) { + parentID := r.PathValue("parent_id") + if parentID == "" { + return "", nil } - return *refs -} - -func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) { - var req openapi.ForceDeleteRequest - cfg := &handlerConfig{ - MarshalInto: &req, - Validate: []validate{ - validateNotEmpty(&req, "Reason", "reason"), - validateMaxLength(&req, "Reason", "reason", maxReasonLength), - }, - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - - if err := h.checkOwnership(r, id); err != nil { - return nil, err - } - - if err := h.service.ForceDelete(r.Context(), h.descriptor.Kind, id, req.Reason); err != nil { - return nil, err - } - return nil, nil - }, + _, err := h.service.Get(r.Context(), h.descriptor.ParentKind, parentID) + if err != nil { + return "", err } - handleForceDelete(w, r, cfg) + return parentID, nil } diff --git a/pkg/handlers/resource_handler_test.go b/pkg/handlers/resource_handler_test.go index 742d0aeb..beadbb72 100644 --- a/pkg/handlers/resource_handler_test.go +++ b/pkg/handlers/resource_handler_test.go @@ -1272,3 +1272,68 @@ func TestResourceHandler_Create_ChildKindWithoutParent_Returns422(t *testing.T) Expect(rr.Code).To(Equal(http.StatusUnprocessableEntity)) Expect(rr.Body.String()).To(ContainSubstring("/channels/{id}/versions")) } + +func TestResourceHandler_Create_NameValidation(t *testing.T) { + tests := []struct { + name string + inputName string + nameMinLen int + nameMaxLen int + wantStatus int + }{ + {"too short", "ab", 3, 53, http.StatusBadRequest}, + {"too long", "toolongname", 3, 5, http.StatusBadRequest}, + {"at min length", "abc", 3, 53, http.StatusCreated}, + {"at max length", "abcde", 3, 5, http.StatusCreated}, + {"valid name with zero lengths", "valid-name", 0, 0, http.StatusCreated}, + {"exceeds dbNameMaxLen fallback", "a" + strings.Repeat("b", 100), 0, 0, http.StatusBadRequest}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + descriptor := registry.EntityDescriptor{ + Kind: "TestEntity", + Plural: "testentities", + NameMinLen: tt.nameMinLen, + NameMaxLen: tt.nameMaxLen, + } + mockResourceSvc := services.NewMockResourceService(ctrl) + handler := NewResourceHandler(descriptor, mockResourceSvc) + + if tt.wantStatus == http.StatusCreated { + now := time.Now() + mockResourceSvc.EXPECT().Create( + gomock.Any(), "TestEntity", gomock.AssignableToTypeOf(&api.Resource{}), gomock.Any(), + ).Return(&api.Resource{ + Meta: api.Meta{ID: "test-id", CreatedTime: now, UpdatedTime: now}, + Kind: "TestEntity", + Name: tt.inputName, + Href: "/api/hyperfleet/v1/testentities/test-id", + Spec: datatypes.JSON(`{"key":"value"}`), + Generation: 1, + CreatedBy: "system@hyperfleet.local", + UpdatedBy: "system@hyperfleet.local", + }, nil) + } + + body := fmt.Sprintf(`{"kind":"TestEntity","name":%q,"spec":{"key":"value"}}`, tt.inputName) + req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/testentities", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + handler.Create(rr, req) + Expect(rr.Code).To(Equal(tt.wantStatus)) + + if tt.wantStatus == http.StatusCreated { + var resp openapi.Resource + err := json.Unmarshal(rr.Body.Bytes(), &resp) + Expect(err).NotTo(HaveOccurred()) + Expect(resp.Name).To(Equal(tt.inputName)) + } + }) + } +} diff --git a/pkg/handlers/resource_status_handler.go b/pkg/handlers/resource_status_handler.go index d75aa047..173d6336 100644 --- a/pkg/handlers/resource_status_handler.go +++ b/pkg/handlers/resource_status_handler.go @@ -37,54 +37,60 @@ func NewResourceStatusHandler( // List returns all adapter statuses for a resource with pagination. // Verifies ownership when parent_id is present in the route. func (h *ResourceStatusHandler) List(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - id := r.PathValue("id") - listArgs, err := parseListParams(r.URL.Query()) - if err != nil { - return nil, err - } + id := r.PathValue("id") + listArgs, svcErr := parseListParams(r.URL.Query()) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - if svcErr := h.verifyResource(r, id); svcErr != nil { - return nil, svcErr - } + if svcErr = h.verifyResource(r, id); svcErr != nil { + handleError(r, w, svcErr) + return + } - return h.listStatuses(ctx, id, listArgs) - }, - ErrorHandler: handleError, + result, svcErr := h.listStatuses(r.Context(), id, listArgs) + if svcErr != nil { + handleError(r, w, svcErr) + return } - handleList(w, r, cfg) + writeJSONResponse(w, r, http.StatusOK, result) } // Create creates or updates an adapter status for a resource. // Verifies ownership when parent_id is present in the route. func (h *ResourceStatusHandler) Create(w http.ResponseWriter, r *http.Request) { var req openapi.AdapterStatusCreateRequest + validateFuncs := []validate{ + validateNotEmpty(&req, "Adapter", "adapter"), + validateObservedGeneration(&req), + validateConditions(&req, "Conditions"), + validateObservedTimeRange(&req.ObservedTime), + } + if svcErr := decodeAndValidate(r, &req, validateFuncs); svcErr != nil { + handleError(r, w, svcErr) + return + } - cfg := &handlerConfig{ - MarshalInto: &req, - Validate: []validate{ - validateNotEmpty(&req, "Adapter", "adapter"), - validateObservedGeneration(&req), - validateConditions(&req, "Conditions"), - validateObservedTimeRange(&req.ObservedTime), - }, - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - id := r.PathValue("id") + id := r.PathValue("id") + if svcErr := h.verifyResource(r, id); svcErr != nil { + handleError(r, w, svcErr) + return + } - if svcErr := h.verifyResource(r, id); svcErr != nil { - return nil, svcErr - } + result, svcErr := h.processStatus(r.Context(), id, &req) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - return h.processStatus(ctx, id, &req) - }, - ErrorHandler: handleError, + if result == nil { + w.WriteHeader(http.StatusNoContent) + return } - handleCreateWithNoContent(w, r, cfg) + writeJSONResponse(w, r, http.StatusCreated, result) } // verifyResource confirms the resource exists. For nested routes (parent_id diff --git a/pkg/handlers/root_resource_handler.go b/pkg/handlers/root_resource_handler.go index dea3d757..8401ea0d 100644 --- a/pkg/handlers/root_resource_handler.go +++ b/pkg/handlers/root_resource_handler.go @@ -32,259 +32,283 @@ func NewRootResourceHandler( } func (h *RootResourceHandler) List(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - listArgs, err := parseListParams(r.URL.Query()) - if err != nil { - return nil, err - } + listArgs, svcErr := parseListParams(r.URL.Query()) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - if kind := r.URL.Query().Get("kind"); kind != "" { - descriptor, ok := registry.Get(kind) - if !ok { - return nil, errors.Validation("Unknown entity kind: %s", kind) - } - kindFilter := fmt.Sprintf("kind = '%s'", descriptor.Kind) - if listArgs.Search == "" { - listArgs.Search = kindFilter - } else { - listArgs.Search = "(" + listArgs.Search + ") AND " + kindFilter - } - } + if kind := r.URL.Query().Get("kind"); kind != "" { + descriptor, ok := registry.Get(kind) + if !ok { + handleError(r, w, errors.Validation("Unknown entity kind: %s", kind)) + return + } + kindFilter := fmt.Sprintf("kind = '%s'", descriptor.Kind) + if listArgs.Search == "" { + listArgs.Search = kindFilter + } else { + listArgs.Search = "(" + listArgs.Search + ") AND " + kindFilter + } + } - resources, paging, err := h.service.ListAll(r.Context(), listArgs) - if err != nil { - return nil, err - } - result := presenters.PresentResourceList(resources, paging) - if listArgs.Fields != nil { - return presenters.SliceFilter(listArgs.Fields, result) - } - return result, nil - }, + resources, paging, svcErr := h.service.ListAll(r.Context(), listArgs) + if svcErr != nil { + handleError(r, w, svcErr) + return + } + + presented := presenters.PresentResourceList(resources, paging) + if listArgs.Fields != nil { + filtered, svcErr := presenters.SliceFilter(listArgs.Fields, presented) + if svcErr != nil { + handleError(r, w, svcErr) + return + } + writeJSONResponse(w, r, http.StatusOK, filtered) + return } - handleList(w, r, cfg) + writeJSONResponse(w, r, http.StatusOK, presented) } func (h *RootResourceHandler) Get(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - resource, err := h.service.GetByID(r.Context(), id) - if err != nil { - return nil, err - } - presented := presenters.PresentResource(resource) - return applyFieldFilter(r, presented) - }, + id := r.PathValue("id") + resource, svcErr := h.service.GetByID(r.Context(), id) + if svcErr != nil { + handleError(r, w, svcErr) + return } - handleGet(w, r, cfg) + + result, svcErr := applyFieldFilter(r, presenters.PresentResource(resource)) + if svcErr != nil { + handleError(r, w, svcErr) + return + } + writeJSONResponse(w, r, http.StatusOK, result) } func (h *RootResourceHandler) Create(w http.ResponseWriter, r *http.Request) { var req openapi.ResourceCreateRequest - cfg := &handlerConfig{ - MarshalInto: &req, - Validate: []validate{ - validateSpec(&req, "Spec", "spec"), - validateLabels(&req, "Labels"), - func() *errors.ServiceError { - descriptor, ok := registry.Get(req.Kind) - if !ok { - return errors.Validation("Unknown entity kind: %s", req.Kind) - } - if descriptor.ParentKind != "" { - return childCreateRejection(descriptor) - } - return validateName(&req, "Name", "name", descriptor.NameMinLen, descriptor.NameMaxLen)() - }, - }, - Action: func() (interface{}, *errors.ServiceError) { - descriptor, _ := registry.Get(req.Kind) - - resource, convErr := presenters.ConvertResource(&req) - if convErr != nil { - return nil, errors.GeneralError("failed to convert resource: %v", convErr) - } - refs := extractReferences(req.References) - resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource, refs) - if svcErr != nil { - return nil, svcErr - } - return presenters.PresentResource(resource), nil - }, + validateFuncs := []validate{ + validateSpec(&req, "Spec", "spec"), + validateLabels(&req, "Labels"), } - handle(w, r, cfg, http.StatusCreated) + if svcErr := decodeAndValidate(r, &req, validateFuncs); svcErr != nil { + handleError(r, w, svcErr) + return + } + + descriptor, ok := registry.Get(req.Kind) + if !ok { + handleError(r, w, errors.Validation("Unknown entity kind: %s", req.Kind)) + return + } + + if descriptor.ParentKind != "" { + handleError(r, w, childCreateRejection(descriptor)) + return + } + if svcErr := validateName(&req, "Name", "name", descriptor.NameMinLen, descriptor.NameMaxLen)(); svcErr != nil { + handleError(r, w, svcErr) + return + } + + resource, convErr := presenters.ConvertResource(&req) + if convErr != nil { + handleError(r, w, errors.GeneralError("failed to convert resource: %v", convErr)) + return + } + + refs := extractReferences(req.References) + resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource, refs) + if svcErr != nil { + handleError(r, w, svcErr) + return + } + + writeJSONResponse(w, r, http.StatusCreated, presenters.PresentResource(resource)) } func (h *RootResourceHandler) Patch(w http.ResponseWriter, r *http.Request) { var req openapi.ResourcePatchRequest - cfg := &handlerConfig{ - MarshalInto: &req, - StrictUnmarshal: true, - Validate: []validate{ - validatePatchRequest(&req), - validateLabels(&req, "Labels"), - }, - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - resource, err := h.service.GetByID(r.Context(), id) - if err != nil { - return nil, err - } + validateFuncs := []validate{ + validatePatchRequest(&req), + validateLabels(&req, "Labels"), + } + if svcErr := decodeAndValidate(r, &req, validateFuncs, "strict"); svcErr != nil { + handleError(r, w, svcErr) + return + } - if req.Spec != nil && h.validator != nil { - descriptor, ok := registry.Get(resource.Kind) - if !ok { - return nil, errors.GeneralError("Resource kind %q is no longer registered", resource.Kind) - } - if validationErr := h.validator.Validate(descriptor.Plural, *req.Spec); validationErr != nil { - if svcErr, ok := validationErr.(*errors.ServiceError); ok { - return nil, svcErr - } - return nil, errors.Validation("Spec validation failed: %v", validationErr) - } - } + id := r.PathValue("id") + ctx := r.Context() + resource, svcErr := h.service.GetByID(ctx, id) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - patch := convertResourcePatch(&req) - resource, err = h.service.Patch(r.Context(), resource.Kind, id, patch) - if err != nil { - return nil, err + if req.Spec != nil && h.validator != nil { + descriptor, ok := registry.Get(resource.Kind) + if !ok { + handleError(r, w, errors.GeneralError("Resource kind %q is no longer registered", resource.Kind)) + return + } + if validationErr := h.validator.Validate(descriptor.Plural, *req.Spec); validationErr != nil { + specErr, ok := validationErr.(*errors.ServiceError) + if !ok { + specErr = errors.Validation("Spec validation failed: %v", validationErr) } - return presenters.PresentResource(resource), nil - }, + handleError(r, w, specErr) + return + } } - handle(w, r, cfg, http.StatusOK) + + patch := convertResourcePatch(&req) + resource, svcErr = h.service.Patch(ctx, resource.Kind, id, patch) + if svcErr != nil { + handleError(r, w, svcErr) + return + } + + writeJSONResponse(w, r, http.StatusOK, presenters.PresentResource(resource)) } func (h *RootResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) { var req openapi.ForceDeleteRequest - cfg := &handlerConfig{ - MarshalInto: &req, - Validate: []validate{ - validateNotEmpty(&req, "Reason", "reason"), - validateMaxLength(&req, "Reason", "reason", maxReasonLength), - }, - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - resource, err := h.service.GetByID(r.Context(), id) - if err != nil { - return nil, err - } - if err := h.service.ForceDelete(r.Context(), resource.Kind, id, req.Reason); err != nil { - return nil, err - } - return nil, nil - }, + validateFuncs := []validate{ + validateNotEmpty(&req, "Reason", "reason"), + validateMaxLength(&req, "Reason", "reason", maxReasonLength), + } + if svcErr := decodeAndValidate(r, &req, validateFuncs); svcErr != nil { + handleError(r, w, svcErr) + return + } + + id := r.PathValue("id") + ctx := r.Context() + resource, svcErr := h.service.GetByID(ctx, id) + if svcErr != nil { + handleError(r, w, svcErr) + return } - handleForceDelete(w, r, cfg) + + if svcErr := h.service.ForceDelete(ctx, resource.Kind, id, req.Reason); svcErr != nil { + handleError(r, w, svcErr) + return + } + + w.WriteHeader(http.StatusNoContent) } func (h *RootResourceHandler) Delete(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - id := r.PathValue("id") - resource, err := h.service.GetByID(r.Context(), id) - if err != nil { - return nil, err - } - resource, svcErr := h.service.Delete(r.Context(), resource.Kind, id) - if svcErr != nil { - return nil, svcErr - } - return presenters.PresentResource(resource), nil - }, + id := r.PathValue("id") + ctx := r.Context() + resource, svcErr := h.service.GetByID(ctx, id) + if svcErr != nil { + handleError(r, w, svcErr) + return + } + + resource, svcErr = h.service.Delete(ctx, resource.Kind, id) + if svcErr != nil { + handleError(r, w, svcErr) + return } - handleSoftDelete(w, r, cfg) + + writeJSONResponse(w, r, http.StatusAccepted, presenters.PresentResource(resource)) } // ListStatuses returns adapter statuses for a resource resolved by ID. func (h *RootResourceHandler) ListStatuses(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - id := r.PathValue("id") - listArgs, err := parseListParams(r.URL.Query()) - if err != nil { - return nil, err - } + ctx := r.Context() + id := r.PathValue("id") + listArgs, svcErr := parseListParams(r.URL.Query()) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - resource, err := h.service.GetByID(ctx, id) - if err != nil { - return nil, err - } + resource, svcErr := h.service.GetByID(ctx, id) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - statuses, total, svcErr := h.adapterStatusService.FindByResourcePaginated( - ctx, resource.Kind, id, listArgs, - ) - if svcErr != nil { - return nil, svcErr - } + statuses, total, svcErr := h.adapterStatusService.FindByResourcePaginated( + ctx, resource.Kind, id, listArgs, + ) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - items := make([]openapi.AdapterStatus, 0, len(statuses)) - for _, as := range statuses { - presented, presErr := presenters.PresentAdapterStatus(as) - if presErr != nil { - logger.WithError(ctx, presErr).Error("Failed to present adapter status") - return nil, errors.GeneralError("Failed to present adapter status") - } - items = append(items, presented) - } + items := make([]openapi.AdapterStatus, 0, len(statuses)) + for _, as := range statuses { + presented, presErr := presenters.PresentAdapterStatus(as) + if presErr != nil { + logger.WithError(ctx, presErr).Error("Failed to present adapter status") + handleError(r, w, errors.GeneralError("Failed to present adapter status")) + return + } + items = append(items, presented) + } - return openapi.AdapterStatusList{ - Items: items, - Page: int32(listArgs.Page), - Size: int32(len(items)), - Total: int32(total), - }, nil - }, + result := openapi.AdapterStatusList{ + Items: items, + Page: int32(listArgs.Page), + Size: int32(len(items)), + Total: int32(total), } - handleList(w, r, cfg) + + writeJSONResponse(w, r, http.StatusOK, result) } // CreateStatus creates or updates an adapter status for a resource resolved by ID. func (h *RootResourceHandler) CreateStatus(w http.ResponseWriter, r *http.Request) { var req openapi.AdapterStatusCreateRequest + validateFuncs := []validate{ + validateNotEmpty(&req, "Adapter", "adapter"), + validateObservedGeneration(&req), + validateConditions(&req, "Conditions"), + validateObservedTimeRange(&req.ObservedTime), + } + if svcErr := decodeAndValidate(r, &req, validateFuncs); svcErr != nil { + handleError(r, w, svcErr) + return + } - cfg := &handlerConfig{ - MarshalInto: &req, - Validate: []validate{ - validateNotEmpty(&req, "Adapter", "adapter"), - validateObservedGeneration(&req), - validateConditions(&req, "Conditions"), - validateObservedTimeRange(&req.ObservedTime), - }, - Action: func() (interface{}, *errors.ServiceError) { - ctx := r.Context() - id := r.PathValue("id") - - resource, err := h.service.GetByID(ctx, id) - if err != nil { - return nil, err - } + ctx := r.Context() + id := r.PathValue("id") + resource, svcErr := h.service.GetByID(ctx, id) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - newStatus, convErr := presenters.ConvertAdapterStatus(resource.Kind, id, &req) - if convErr != nil { - logger.WithError(ctx, convErr).Error("Failed to convert adapter status") - return nil, errors.GeneralError("Failed to convert adapter status") - } + newStatus, convErr := presenters.ConvertAdapterStatus(resource.Kind, id, &req) + if convErr != nil { + logger.WithError(ctx, convErr).Error("Failed to convert adapter status") + handleError(r, w, errors.GeneralError("Failed to convert adapter status")) + return + } - adapterStatus, svcErr := h.service.ProcessAdapterStatus(ctx, resource.Kind, id, newStatus) - if svcErr != nil { - return nil, svcErr - } + adapterStatus, svcErr := h.service.ProcessAdapterStatus(ctx, resource.Kind, id, newStatus) + if svcErr != nil { + handleError(r, w, svcErr) + return + } - if adapterStatus == nil { - return nil, nil - } + if adapterStatus == nil { + w.WriteHeader(http.StatusNoContent) + return + } - status, presErr := presenters.PresentAdapterStatus(adapterStatus) - if presErr != nil { - logger.WithError(ctx, presErr).Error("Failed to present adapter status") - return nil, errors.GeneralError("Failed to present adapter status") - } - return &status, nil - }, + status, presErr := presenters.PresentAdapterStatus(adapterStatus) + if presErr != nil { + logger.WithError(ctx, presErr).Error("Failed to present adapter status") + handleError(r, w, errors.GeneralError("Failed to present adapter status")) + return } - handleCreateWithNoContent(w, r, cfg) + writeJSONResponse(w, r, http.StatusCreated, status) } diff --git a/pkg/handlers/validation.go b/pkg/handlers/validation.go index 71c5ddda..c7bfd1fd 100755 --- a/pkg/handlers/validation.go +++ b/pkg/handlers/validation.go @@ -28,6 +28,8 @@ const ( dbNameMaxLen = 100 ) +type validate func() *errors.ServiceError + // dnsLabelPattern matches a single DNS label segment in a Kubernetes label key prefix: // lowercase alphanumeric with hyphens, must start and end with alphanumeric. var dnsLabelPattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`)