HYPERFLEET-1328 - refactor: simplify resource handlers and add registry cycle detection

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,17 +105,16 @@ 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
 			}
 
 			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
-}

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))
+}

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)

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, ""
 	}
 

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)
 

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",

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()

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())