diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index e4b90f32..2d3ddd34 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 @@ -48,16 +42,22 @@ 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 != "" { + parent := registry.MustGet(h.descriptor.ParentKind) + return nil, errors.Validation( + "kind %q is a child kind; create it via /%s/{id}/%s", h.descriptor.Kind, parent.Plural, h.descriptor.Plural, + ) + } + 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,13 +81,11 @@ func (h *ResourceHandler) Get(w http.ResponseWriter, r *http.Request) { ctx := r.Context() vars := mux.Vars(r) id := vars["id"] + parentID, hasParent := vars["parent_id"] 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 hasParent { resource, err = h.service.GetByOwner(ctx, h.descriptor.Kind, id, parentID) } else { resource, err = h.service.Get(ctx, h.descriptor.Kind, id) @@ -107,6 +105,8 @@ func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() + parentID, hasParent := mux.Vars(r)["parent_id"] + listArgs, err := services.NewListArguments(r.URL.Query()) if err != nil { return nil, err @@ -114,10 +114,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 hasParent { resources, paging, err = h.service.ListByOwner(ctx, h.descriptor.Kind, parentID, listArgs) } else { resources, paging, err = h.service.List(ctx, h.descriptor.Kind, listArgs) @@ -185,7 +182,8 @@ func (h *ResourceHandler) Delete(w http.ResponseWriter, r *http.Request) { // 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 { + parentID, hasParent := mux.Vars(r)["parent_id"] + if hasParent { if _, err := h.service.GetByOwner(r.Context(), h.descriptor.Kind, id, parentID); err != nil { return err } @@ -230,13 +228,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 ef6b3117..7115e81c 100644 --- a/pkg/handlers/resource_handler_test.go +++ b/pkg/handlers/resource_handler_test.go @@ -431,10 +431,6 @@ func TestResourceHandler_GetByOwner(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", CreatedTime: now, UpdatedTime: now}, Kind: "Version", Name: "4-17-3", @@ -443,21 +439,9 @@ func TestResourceHandler_GetByOwner(t *testing.T) { }, expectedStatusCode: http.StatusOK, }, - { - 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, - }, { name: "Child not found", 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")) }, @@ -497,10 +481,6 @@ func TestResourceHandler_ListByOwner(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().ListByOwner(gomock.Any(), "Version", "ch-1", gomock.AssignableToTypeOf(&services.ListArguments{})). Return(api.ResourceList{ @@ -514,15 +494,6 @@ func TestResourceHandler_ListByOwner(t *testing.T) { expectedStatusCode: http.StatusOK, expectedItems: 1, }, - { - 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, - expectedItems: 0, - }, } for _, tt := range tests { @@ -822,3 +793,27 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) { }) } } + +func TestResourceHandler_Create_ChildKindWithoutParent_Returns400(t *testing.T) { + RegisterTestingT(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + 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.StatusBadRequest)) +} diff --git a/pkg/handlers/root_resource_handler.go b/pkg/handlers/root_resource_handler.go index 8a13226f..6d2ee7db 100644 --- a/pkg/handlers/root_resource_handler.go +++ b/pkg/handlers/root_resource_handler.go @@ -90,7 +90,10 @@ func (h *RootResourceHandler) Create(w http.ResponseWriter, r *http.Request) { return nil, errors.Validation("Unknown entity kind: %s", req.Kind) } if descriptor.ParentKind != "" { - return nil, childCreateRejection(descriptor) + parent := registry.MustGet(descriptor.ParentKind) + return nil, errors.Validation( + "kind %q is a child kind; create it via /%s/{id}/%s", descriptor.Kind, parent.Plural, descriptor.Plural, + ) } resource, convErr := presenters.ConvertResource(&req) diff --git a/pkg/middleware/schema_validation.go b/pkg/middleware/schema_validation.go index ac905e16..ed159336 100644 --- a/pkg/middleware/schema_validation.go +++ b/pkg/middleware/schema_validation.go @@ -191,7 +191,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 2d84a3d4..4e68acd3 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,20 @@ func Validate() { } } + if 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 { panic(fmt.Sprintf( "duplicate plural %q: registered by both %q and %q", diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index c97606a0..4f5ed029 100644 --- a/pkg/registry/registry_test.go +++ b/pkg/registry/registry_test.go @@ -142,6 +142,18 @@ 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_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 a0298df4..b2bb7930 100644 --- a/test/integration/root_resources_test.go +++ b/test/integration/root_resources_test.go @@ -141,7 +141,7 @@ func TestRootResourceCreate(t *testing.T) { }) } -func TestRootResourceCreateChildKindReturns422(t *testing.T) { +func TestRootResourceCreateChildKindReturns400(t *testing.T) { RegisterTestingT(t) _, h := setupResourceTest(t) @@ -165,7 +165,7 @@ func TestRootResourceCreateChildKindReturns422(t *testing.T) { SetBody(body). Post(h.RestURL(resourcesPath)) Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusUnprocessableEntity)) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) var problem openapi.ProblemDetails Expect(json.Unmarshal(resp.Body(), &problem)).To(Succeed()) @@ -363,7 +363,7 @@ func TestFlatChildRouteDelete(t *testing.T) { Expect(resp.StatusCode()).To(Equal(http.StatusAccepted)) } -func TestFlatChildRoutePostReturns422(t *testing.T) { +func TestFlatChildRoutePostReturns400(t *testing.T) { RegisterTestingT(t) _, h := setupResourceTest(t) @@ -387,7 +387,7 @@ func TestFlatChildRoutePostReturns422(t *testing.T) { SetBody(body). Post(h.RestURL("/versions")) Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusUnprocessableEntity)) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) var problem openapi.ProblemDetails Expect(json.Unmarshal(resp.Body(), &problem)).To(Succeed())