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)
 			}
@@ -79,15 +79,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.checkParentExists(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)
@@ -107,17 +107,19 @@ func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) {
 		Action: func() (interface{}, *errors.ServiceError) {
 			ctx := r.Context()
 
+			parentID, err := h.checkParentExists(r)
+			if err != nil {
+				return nil, err
+			}
+
 			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 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)
@@ -148,7 +150,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
 			}
 
@@ -168,24 +170,42 @@ 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
 		},
 	}
 	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 {
+// checkParentExists 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) checkParentExists(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.checkParentExists(r)
+	if err != nil {
+		return err
+	}
+	if parentID != "" {
 		if _, err := h.service.GetByOwner(r.Context(), h.descriptor.Kind, id, parentID); err != nil {
 			return err
 		}
@@ -218,7 +238,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
 			}
 
@@ -230,13 +250,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

@@ -556,6 +556,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)
@@ -572,11 +576,23 @@ 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"))
 			},
 			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 {
@@ -610,6 +626,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)
@@ -624,11 +644,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 {
@@ -768,6 +800,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)
@@ -781,12 +817,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": ""}`,
@@ -822,3 +871,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
@@ -112,6 +113,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 {