diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index 8d5f2b96..4906088d 100644 --- a/pkg/handlers/resource_handler.go +++ b/pkg/handlers/resource_handler.go @@ -13,14 +13,8 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" ) -// ResourceHandler serves both flat and owner-nested routes for a single entity -// kind. Every method branches on whether "parent_id" is present in mux.Vars(r) -// 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). +// ResourceHandler serves both flat and owner-nested routes for one entity kind. +// Each verb branches on whether parent_id is present in the path. type ResourceHandler struct { service services.ResourceService descriptor registry.EntityDescriptor @@ -49,16 +43,19 @@ func (h *ResourceHandler) Create(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() + parentID, hasParent := mux.Vars(r)["parent_id"] + if !hasParent && h.descriptor.ParentKind != "" { + return nil, childCreateRejection(h.descriptor) + } + var resource *api.Resource var err error - if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent { + if hasParent { 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 if h.descriptor.ParentKind != "" { - return nil, childCreateRejection(h.descriptor) } else { resource, err = presenters.ConvertResource(&req) } @@ -81,15 +78,15 @@ func (h *ResourceHandler) Get(w http.ResponseWriter, r *http.Request) { cfg := &handlerConfig{ Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() - vars := mux.Vars(r) - id := vars["id"] + id := mux.Vars(r)["id"] + + parentID, err := h.parentIDIfExists(r) + if err != nil { + return nil, err + } var resource *api.Resource - var err *errors.ServiceError - if parentID, hasParent := vars["parent_id"]; hasParent { - if _, err = h.service.Get(ctx, h.descriptor.ParentKind, parentID); err != nil { - return nil, err - } + if parentID != "" { resource, err = h.service.GetByOwner(ctx, h.descriptor.Kind, id, parentID) } else { resource, err = h.service.Get(ctx, h.descriptor.Kind, id) @@ -109,6 +106,11 @@ func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() + parentID, err := h.parentIDIfExists(r) + if err != nil { + return nil, err + } + listArgs, err := services.NewListArguments(r.URL.Query()) if err != nil { return nil, err @@ -116,10 +118,7 @@ func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) { var resources api.ResourceList var paging *api.PagingMeta - if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent { - if _, svcErr := h.service.Get(ctx, h.descriptor.ParentKind, parentID); svcErr != nil { - return nil, svcErr - } + 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) @@ -150,7 +149,7 @@ func (h *ResourceHandler) Patch(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { id := mux.Vars(r)["id"] - if err := h.verifyOwnership(r, id); err != nil { + if err := h.checkOwnership(r, id); err != nil { return nil, err } @@ -170,13 +169,13 @@ func (h *ResourceHandler) Delete(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { id := mux.Vars(r)["id"] - if err := h.verifyOwnership(r, id); err != nil { + if err := h.checkOwnership(r, id); err != nil { return nil, err } - resource, svcErr := h.service.Delete(r.Context(), h.descriptor.Kind, id) - if svcErr != nil { - return nil, svcErr + resource, err := h.service.Delete(r.Context(), h.descriptor.Kind, id) + if err != nil { + return nil, err } return presenters.PresentResource(resource), nil }, @@ -184,10 +183,28 @@ func (h *ResourceHandler) Delete(w http.ResponseWriter, r *http.Request) { handleSoftDelete(w, r, cfg) } -// verifyOwnership confirms id belongs to the parent named by parent_id in the -// request path. No-op for flat (non-nested) routes, where parent_id is absent. -func (h *ResourceHandler) verifyOwnership(r *http.Request, id string) *errors.ServiceError { - if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent { +// 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, hasParent := mux.Vars(r)["parent_id"] + if !hasParent { + return "", nil + } + _, err := h.service.Get(r.Context(), h.descriptor.ParentKind, parentID) + if err != nil { + return "", err + } + return parentID, nil +} + +// checkOwnership verifies id belongs to parent_id, checking the parent first so +// a missing parent reports "not found" against the parent, not the child. +func (h *ResourceHandler) checkOwnership(r *http.Request, id string) *errors.ServiceError { + parentID, err := h.parentIDIfExists(r) + if err != nil { + return err + } + if parentID != "" { if _, err := h.service.GetByOwner(r.Context(), h.descriptor.Kind, id, parentID); err != nil { return err } @@ -195,6 +212,18 @@ func (h *ResourceHandler) verifyOwnership(r *http.Request, id string) *errors.Se 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 { @@ -229,7 +258,7 @@ func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { id := mux.Vars(r)["id"] - if err := h.verifyOwnership(r, id); err != nil { + if err := h.checkOwnership(r, id); err != nil { return nil, err } @@ -241,13 +270,3 @@ func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) { } handleForceDelete(w, r, cfg) } - -func childCreateRejection(descriptor registry.EntityDescriptor) *errors.ServiceError { - parent := registry.MustGet(descriptor.ParentKind) - svcErr := errors.Validation( - "Cannot create %s here. Use POST /%s/{%s_id}/%s", - descriptor.Kind, parent.Plural, parent.Kind, descriptor.Plural, - ) - svcErr.HTTPCode = http.StatusUnprocessableEntity - return svcErr -} diff --git a/pkg/handlers/resource_handler_test.go b/pkg/handlers/resource_handler_test.go index 3ece8726..6bf9a49a 100644 --- a/pkg/handlers/resource_handler_test.go +++ b/pkg/handlers/resource_handler_test.go @@ -593,6 +593,10 @@ func TestResourceHandler_PatchByOwner(t *testing.T) { { name: "Success", setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{ + Meta: api.Meta{ID: "ch-1"}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1"). Return(&api.Resource{Meta: api.Meta{ID: "v-1"}, Kind: "Version", Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com"}, nil) @@ -609,6 +613,10 @@ func TestResourceHandler_PatchByOwner(t *testing.T) { { name: "Child not owned by parent", setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{ + Meta: api.Meta{ID: "ch-1"}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1"). Return(nil, errors.NotFound("Version not found for channel")) }, @@ -617,6 +625,10 @@ func TestResourceHandler_PatchByOwner(t *testing.T) { { name: "Conflict 409 - soft-deleted child", setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{ + Meta: api.Meta{ID: "ch-1"}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1"). Return(&api.Resource{Meta: api.Meta{ID: "v-1"}, Kind: "Version", Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com"}, nil) @@ -626,6 +638,14 @@ func TestResourceHandler_PatchByOwner(t *testing.T) { }, expectedStatusCode: http.StatusConflict, }, + { + name: "Parent not found", + setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1"). + Return(nil, errors.NotFound("Channel not found")) + }, + expectedStatusCode: http.StatusNotFound, + }, } for _, tt := range tests { @@ -659,6 +679,10 @@ func TestResourceHandler_DeleteByOwner(t *testing.T) { { name: "Success", setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{ + Meta: api.Meta{ID: "ch-1"}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1"). Return(&api.Resource{Meta: api.Meta{ID: "v-1"}, Kind: "Version", Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com"}, nil) @@ -673,11 +697,23 @@ func TestResourceHandler_DeleteByOwner(t *testing.T) { { name: "Child not owned by parent", setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{ + Meta: api.Meta{ID: "ch-1"}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1"). Return(nil, errors.NotFound("Version not found for channel")) }, expectedStatusCode: http.StatusNotFound, }, + { + name: "Parent not found", + setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1"). + Return(nil, errors.NotFound("Channel not found")) + }, + expectedStatusCode: http.StatusNotFound, + }, } for _, tt := range tests { @@ -817,6 +853,10 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) { name: "Success 204 - nested resource force-deleted", body: `{"reason": "Stuck in finalizing"}`, setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{ + Meta: api.Meta{ID: parentID}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT(). GetByOwner(gomock.Any(), "Version", versionID, parentID). Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil) @@ -830,12 +870,25 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) { name: "Error 404 - ownership mismatch", body: `{"reason": "some reason"}`, setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{ + Meta: api.Meta{ID: parentID}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT(). GetByOwner(gomock.Any(), "Version", versionID, parentID). Return(nil, errors.NotFound("Version with id='%s' not found for owner '%s'", versionID, parentID)) }, expectedStatusCode: http.StatusNotFound, }, + { + name: "Error 404 - parent not found", + body: `{"reason": "some reason"}`, + setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", parentID). + Return(nil, errors.NotFound("Channel not found")) + }, + expectedStatusCode: http.StatusNotFound, + }, { name: "Error 400 - empty reason", body: `{"reason": ""}`, @@ -847,6 +900,10 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) { name: "Error 409 - not in Finalizing state", body: `{"reason": "some reason"}`, setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{ + Meta: api.Meta{ID: parentID}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT(). GetByOwner(gomock.Any(), "Version", versionID, parentID). Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil) @@ -860,6 +917,10 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) { name: "Error 500 - service internal error", body: `{"reason": "some reason"}`, setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{ + Meta: api.Meta{ID: parentID}, Kind: "Channel", + Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com", + }, nil) mock.EXPECT(). GetByOwner(gomock.Any(), "Version", versionID, parentID). Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil) @@ -1110,3 +1171,29 @@ func TestRootResourceHandler_Create_RejectsInvalidName(t *testing.T) { }) } } + +func TestResourceHandler_Create_ChildKindWithoutParent_Returns422(t *testing.T) { + RegisterTestingT(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + t.Cleanup(registry.Reset) + registry.Reset() + registry.Register(channelDescriptor) + registry.Register(versionDescriptor) + + mockSvc := services.NewMockResourceService(ctrl) + handler := NewResourceHandler(versionDescriptor, mockSvc) + + req := httptest.NewRequest(http.MethodPost, + "/api/hyperfleet/v1/versions", + strings.NewReader(`{"kind":"Version","name":"4-17-3","spec":{"raw_version":"4.17.3"}}`)) + req.Header.Set("Content-Type", "application/json") + req = mux.SetURLVars(req, map[string]string{}) + rr := httptest.NewRecorder() + + handler.Create(rr, req) + + Expect(rr.Code).To(Equal(http.StatusUnprocessableEntity)) + Expect(rr.Body.String()).To(ContainSubstring("/channels/{id}/versions")) +} diff --git a/pkg/middleware/schema_validation.go b/pkg/middleware/schema_validation.go index f67bd9fc..965ba092 100644 --- a/pkg/middleware/schema_validation.go +++ b/pkg/middleware/schema_validation.go @@ -174,7 +174,9 @@ func shouldValidateRequest( } } - if rootResourcePattern.MatchString(path) { + // Root /resources POST carries kind in body — resolve plural from body later. + // Root /resources PATCH has no kind; handler validates spec internally. + if method == http.MethodPost && rootResourcePattern.MatchString(path) { return true, "" } diff --git a/pkg/middleware/schema_validation_match_test.go b/pkg/middleware/schema_validation_match_test.go index 8639a0aa..7dffde71 100644 --- a/pkg/middleware/schema_validation_match_test.go +++ b/pkg/middleware/schema_validation_match_test.go @@ -112,6 +112,31 @@ func TestShouldValidateRequest_PostNestedVersionPrefersVersionOverChannel(t *tes Expect(plural).To(Equal("versions")) } +func TestShouldValidateRequest_PostRootResourceMatchesForBodyResolution(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPost, + "/api/hyperfleet/v1/resources", + matchers, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(BeEmpty()) +} + +func TestShouldValidateRequest_PatchRootResourceSkipsMiddleware(t *testing.T) { + RegisterTestingT(t) + + should, _ := shouldValidateRequest( + http.MethodPatch, + "/api/hyperfleet/v1/resources/550e8400-e29b-41d4-a716-446655440000", + matchers, + ) + + Expect(should).To(BeFalse()) +} + func TestShouldValidateRequest_GetSkipsValidation(t *testing.T) { RegisterTestingT(t) diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 734bb517..b5562242 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -90,6 +90,7 @@ func ChildrenOf(parentKind string) []EntityDescriptor { // Validate checks registry integrity. Panics on: // - empty Kind or Plural on any descriptor // - any ParentKind that references an unregistered kind +// - cycles in the ParentKind ownership chain // - duplicate Plural values across descriptors // - ReferenceDescriptor with TargetKind that doesn't resolve // - duplicate RefType within a single entity's References @@ -114,6 +115,17 @@ func Validate() { d.Kind, d.ParentKind, )) } + visited := map[string]bool{d.Kind: true} + for cur := d.ParentKind; cur != ""; { + if visited[cur] { + panic(fmt.Sprintf( + "ownership cycle detected: kind %q participates in a ParentKind cycle", + d.Kind, + )) + } + visited[cur] = true + cur = descriptors[cur].ParentKind + } } if existing, ok := plurals[d.Plural]; ok { diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index a2dfd9f3..e0dd4052 100644 --- a/pkg/registry/registry_test.go +++ b/pkg/registry/registry_test.go @@ -142,6 +142,31 @@ func TestValidate_MissingParent_Panics(t *testing.T) { }).To(PanicWith(ContainSubstring("unregistered parent kind"))) } +func TestValidate_ParentKindCycle_Panics(t *testing.T) { + RegisterTestingT(t) + Reset() + + Register(EntityDescriptor{Kind: "A", Plural: "as", ParentKind: "B"}) + Register(EntityDescriptor{Kind: "B", Plural: "bs", ParentKind: "A"}) + + Expect(func() { + Validate() + }).To(PanicWith(ContainSubstring("ownership cycle detected"))) +} + +func TestValidate_ParentKindCycle_ThreeNode_Panics(t *testing.T) { + RegisterTestingT(t) + Reset() + + Register(EntityDescriptor{Kind: "A", Plural: "as", ParentKind: "B"}) + Register(EntityDescriptor{Kind: "B", Plural: "bs", ParentKind: "C"}) + Register(EntityDescriptor{Kind: "C", Plural: "cs", ParentKind: "A"}) + + Expect(func() { + Validate() + }).To(PanicWith(ContainSubstring("ownership cycle detected"))) +} + func TestValidate_DuplicatePlural_Panics(t *testing.T) { RegisterTestingT(t) Reset() diff --git a/test/integration/root_resources_test.go b/test/integration/root_resources_test.go index 36cc5bc4..55a2d7a1 100644 --- a/test/integration/root_resources_test.go +++ b/test/integration/root_resources_test.go @@ -393,3 +393,62 @@ func TestFlatChildRoutePostReturns422(t *testing.T) { Expect(json.Unmarshal(resp.Body(), &problem)).To(Succeed()) Expect(*problem.Detail).To(ContainSubstring("channels")) } + +func TestRootResourcePostInvalidSpecReturns400(t *testing.T) { + RegisterTestingT(t) + _, h := setupResourceTest(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // POST /resources with spec as a string instead of an object. + // Middleware rejects this with "spec field must be an object". + body := fmt.Sprintf( + `{"kind":"Channel","name":"invalid-spec-%s","spec":"not-an-object"}`, + uuid.NewString()[:8], + ) + + resp, err := rootResourceRequest(ctx). + SetBody(body). + Post(h.RestURL(resourcesPath)) + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var problem openapi.ProblemDetails + Expect(json.Unmarshal(resp.Body(), &problem)).To(Succeed()) + Expect(problem.Status).To(Equal(http.StatusBadRequest)) +} + +func TestRootResourcePatchInvalidSpecReturns400(t *testing.T) { + RegisterTestingT(t) + svc, h := setupResourceTest(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + channel := createChannel(t, svc, fmt.Sprintf("patch-inv-%s", uuid.NewString()[:8])) + + // PATCH /resources/{id} with spec as a string instead of an object. + // Both root and flat routes should return the same 400 problem-details shape. + patchBody := `{"spec":"not-an-object"}` + rootResp, err := rootResourceRequest(ctx). + SetBody(patchBody). + Patch(h.RestURL(fmt.Sprintf("%s/%s", resourcesPath, channel.ID))) + Expect(err).NotTo(HaveOccurred()) + Expect(rootResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var rootProblem openapi.ProblemDetails + Expect(json.Unmarshal(rootResp.Body(), &rootProblem)).To(Succeed()) + Expect(rootProblem.Status).To(Equal(http.StatusBadRequest)) + + // Verify flat route returns the same shape + flatResp, err := rootResourceRequest(ctx). + SetBody(patchBody). + Patch(h.RestURL(fmt.Sprintf("/channels/%s", channel.ID))) + Expect(err).NotTo(HaveOccurred()) + Expect(flatResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var flatProblem openapi.ProblemDetails + Expect(json.Unmarshal(flatResp.Body(), &flatProblem)).To(Succeed()) + Expect(flatProblem.Status).To(Equal(http.StatusBadRequest)) +}