Skip to content

Commit c309ee6

Browse files
kuudorikuudori
committed
HYPERFLEET-1328 - refactor: simplify resource handlers and add registry cycle detection
Consolidate parent-kind checks in resource handlers with actionable error messages, add ParentKind cycle detection to registry startup validation, and harden schema validation path matching.
1 parent 5aefe87 commit c309ee6

7 files changed

Lines changed: 272 additions & 43 deletions

File tree

‎pkg/handlers/resource_handler.go‎

Lines changed: 61 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,8 @@ import (
1313
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/services"
1414
)
1515

16-
// ResourceHandler serves both flat and owner-nested routes for a single entity
17-
// kind. Every method branches on whether "parent_id" is present in mux.Vars(r)
18-
// rather than dispatching statically per route. This is only correct because
19-
// plugins/entities/plugin.go guarantees the invariant: a nested (ParentKind != "")
20-
// descriptor is registered exclusively under a {parent_id} subrouter, and a flat
21-
// descriptor never is. If that registration is ever bypassed — e.g. a nested kind
22-
// wired to a flat route — these branches take the wrong path silently (Create
23-
// would skip setting owner references instead of erroring).
16+
// ResourceHandler serves both flat and owner-nested routes for one entity kind.
17+
// Each verb branches on whether parent_id is present in the path.
2418
type ResourceHandler struct {
2519
service services.ResourceService
2620
descriptor registry.EntityDescriptor
@@ -49,16 +43,19 @@ func (h *ResourceHandler) Create(w http.ResponseWriter, r *http.Request) {
4943
Action: func() (interface{}, *errors.ServiceError) {
5044
ctx := r.Context()
5145

46+
parentID, hasParent := mux.Vars(r)["parent_id"]
47+
if !hasParent && h.descriptor.ParentKind != "" {
48+
return nil, childCreateRejection(h.descriptor)
49+
}
50+
5251
var resource *api.Resource
5352
var err error
54-
if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent {
53+
if hasParent {
5554
parent, svcErr := h.service.Get(ctx, h.descriptor.ParentKind, parentID)
5655
if svcErr != nil {
5756
return nil, svcErr
5857
}
5958
resource, err = presenters.ConvertResourceWithOwner(&req, parent.ID, parent.Kind, parent.Href)
60-
} else if h.descriptor.ParentKind != "" {
61-
return nil, childCreateRejection(h.descriptor)
6259
} else {
6360
resource, err = presenters.ConvertResource(&req)
6461
}
@@ -81,15 +78,15 @@ func (h *ResourceHandler) Get(w http.ResponseWriter, r *http.Request) {
8178
cfg := &handlerConfig{
8279
Action: func() (interface{}, *errors.ServiceError) {
8380
ctx := r.Context()
84-
vars := mux.Vars(r)
85-
id := vars["id"]
81+
id := mux.Vars(r)["id"]
82+
83+
parentID, err := h.parentIDIfExists(r)
84+
if err != nil {
85+
return nil, err
86+
}
8687

8788
var resource *api.Resource
88-
var err *errors.ServiceError
89-
if parentID, hasParent := vars["parent_id"]; hasParent {
90-
if _, err = h.service.Get(ctx, h.descriptor.ParentKind, parentID); err != nil {
91-
return nil, err
92-
}
89+
if parentID != "" {
9390
resource, err = h.service.GetByOwner(ctx, h.descriptor.Kind, id, parentID)
9491
} else {
9592
resource, err = h.service.Get(ctx, h.descriptor.Kind, id)
@@ -109,17 +106,19 @@ func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) {
109106
Action: func() (interface{}, *errors.ServiceError) {
110107
ctx := r.Context()
111108

109+
parentID, err := h.parentIDIfExists(r)
110+
if err != nil {
111+
return nil, err
112+
}
113+
112114
listArgs, err := services.NewListArguments(r.URL.Query())
113115
if err != nil {
114116
return nil, err
115117
}
116118

117119
var resources api.ResourceList
118120
var paging *api.PagingMeta
119-
if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent {
120-
if _, svcErr := h.service.Get(ctx, h.descriptor.ParentKind, parentID); svcErr != nil {
121-
return nil, svcErr
122-
}
121+
if parentID != "" {
123122
resources, paging, err = h.service.ListByOwner(ctx, h.descriptor.Kind, parentID, listArgs)
124123
} else {
125124
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) {
150149
Action: func() (interface{}, *errors.ServiceError) {
151150
id := mux.Vars(r)["id"]
152151

153-
if err := h.verifyOwnership(r, id); err != nil {
152+
if err := h.checkOwnership(r, id); err != nil {
154153
return nil, err
155154
}
156155

@@ -170,31 +169,61 @@ func (h *ResourceHandler) Delete(w http.ResponseWriter, r *http.Request) {
170169
Action: func() (interface{}, *errors.ServiceError) {
171170
id := mux.Vars(r)["id"]
172171

173-
if err := h.verifyOwnership(r, id); err != nil {
172+
if err := h.checkOwnership(r, id); err != nil {
174173
return nil, err
175174
}
176175

177-
resource, svcErr := h.service.Delete(r.Context(), h.descriptor.Kind, id)
178-
if svcErr != nil {
179-
return nil, svcErr
176+
resource, err := h.service.Delete(r.Context(), h.descriptor.Kind, id)
177+
if err != nil {
178+
return nil, err
180179
}
181180
return presenters.PresentResource(resource), nil
182181
},
183182
}
184183
handleSoftDelete(w, r, cfg)
185184
}
186185

187-
// verifyOwnership confirms id belongs to the parent named by parent_id in the
188-
// request path. No-op for flat (non-nested) routes, where parent_id is absent.
189-
func (h *ResourceHandler) verifyOwnership(r *http.Request, id string) *errors.ServiceError {
190-
if parentID, hasParent := mux.Vars(r)["parent_id"]; hasParent {
186+
// parentIDIfExists returns the parent_id if the parent exists, "" for flat
187+
// routes, or a 404 if parent_id is present but the parent is missing.
188+
func (h *ResourceHandler) parentIDIfExists(r *http.Request) (string, *errors.ServiceError) {
189+
parentID, hasParent := mux.Vars(r)["parent_id"]
190+
if !hasParent {
191+
return "", nil
192+
}
193+
_, err := h.service.Get(r.Context(), h.descriptor.ParentKind, parentID)
194+
if err != nil {
195+
return "", err
196+
}
197+
return parentID, nil
198+
}
199+
200+
// checkOwnership verifies id belongs to parent_id, checking the parent first so
201+
// a missing parent reports "not found" against the parent, not the child.
202+
func (h *ResourceHandler) checkOwnership(r *http.Request, id string) *errors.ServiceError {
203+
parentID, err := h.parentIDIfExists(r)
204+
if err != nil {
205+
return err
206+
}
207+
if parentID != "" {
191208
if _, err := h.service.GetByOwner(r.Context(), h.descriptor.Kind, id, parentID); err != nil {
192209
return err
193210
}
194211
}
195212
return nil
196213
}
197214

215+
// childCreateRejection returns a validation error indicating the resource must be
216+
// created via its parent's nested route.
217+
func childCreateRejection(descriptor registry.EntityDescriptor) *errors.ServiceError {
218+
parent := registry.MustGet(descriptor.ParentKind)
219+
svcErr := errors.Validation(
220+
"kind %q is a child kind; create it via /%s/{id}/%s",
221+
descriptor.Kind, parent.Plural, descriptor.Plural,
222+
)
223+
svcErr.HTTPCode = http.StatusUnprocessableEntity
224+
return svcErr
225+
}
226+
198227
func convertResourcePatch(req *openapi.ResourcePatchRequest) *api.ResourcePatch {
199228
patch := &api.ResourcePatch{}
200229
if req.Spec != nil {
@@ -229,7 +258,7 @@ func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) {
229258
Action: func() (interface{}, *errors.ServiceError) {
230259
id := mux.Vars(r)["id"]
231260

232-
if err := h.verifyOwnership(r, id); err != nil {
261+
if err := h.checkOwnership(r, id); err != nil {
233262
return nil, err
234263
}
235264

@@ -241,13 +270,3 @@ func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) {
241270
}
242271
handleForceDelete(w, r, cfg)
243272
}
244-
245-
func childCreateRejection(descriptor registry.EntityDescriptor) *errors.ServiceError {
246-
parent := registry.MustGet(descriptor.ParentKind)
247-
svcErr := errors.Validation(
248-
"Cannot create %s here. Use POST /%s/{%s_id}/%s",
249-
descriptor.Kind, parent.Plural, parent.Kind, descriptor.Plural,
250-
)
251-
svcErr.HTTPCode = http.StatusUnprocessableEntity
252-
return svcErr
253-
}

‎pkg/handlers/resource_handler_test.go‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,10 @@ func TestResourceHandler_PatchByOwner(t *testing.T) {
593593
{
594594
name: "Success",
595595
setupMock: func(mock *services.MockResourceService) {
596+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{
597+
Meta: api.Meta{ID: "ch-1"}, Kind: "Channel",
598+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
599+
}, nil)
596600
mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1").
597601
Return(&api.Resource{Meta: api.Meta{ID: "v-1"}, Kind: "Version",
598602
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com"}, nil)
@@ -609,6 +613,10 @@ func TestResourceHandler_PatchByOwner(t *testing.T) {
609613
{
610614
name: "Child not owned by parent",
611615
setupMock: func(mock *services.MockResourceService) {
616+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{
617+
Meta: api.Meta{ID: "ch-1"}, Kind: "Channel",
618+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
619+
}, nil)
612620
mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1").
613621
Return(nil, errors.NotFound("Version not found for channel"))
614622
},
@@ -617,6 +625,10 @@ func TestResourceHandler_PatchByOwner(t *testing.T) {
617625
{
618626
name: "Conflict 409 - soft-deleted child",
619627
setupMock: func(mock *services.MockResourceService) {
628+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{
629+
Meta: api.Meta{ID: "ch-1"}, Kind: "Channel",
630+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
631+
}, nil)
620632
mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1").
621633
Return(&api.Resource{Meta: api.Meta{ID: "v-1"}, Kind: "Version",
622634
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com"}, nil)
@@ -626,6 +638,14 @@ func TestResourceHandler_PatchByOwner(t *testing.T) {
626638
},
627639
expectedStatusCode: http.StatusConflict,
628640
},
641+
{
642+
name: "Parent not found",
643+
setupMock: func(mock *services.MockResourceService) {
644+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").
645+
Return(nil, errors.NotFound("Channel not found"))
646+
},
647+
expectedStatusCode: http.StatusNotFound,
648+
},
629649
}
630650

631651
for _, tt := range tests {
@@ -659,6 +679,10 @@ func TestResourceHandler_DeleteByOwner(t *testing.T) {
659679
{
660680
name: "Success",
661681
setupMock: func(mock *services.MockResourceService) {
682+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{
683+
Meta: api.Meta{ID: "ch-1"}, Kind: "Channel",
684+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
685+
}, nil)
662686
mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1").
663687
Return(&api.Resource{Meta: api.Meta{ID: "v-1"}, Kind: "Version",
664688
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com"}, nil)
@@ -673,11 +697,23 @@ func TestResourceHandler_DeleteByOwner(t *testing.T) {
673697
{
674698
name: "Child not owned by parent",
675699
setupMock: func(mock *services.MockResourceService) {
700+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").Return(&api.Resource{
701+
Meta: api.Meta{ID: "ch-1"}, Kind: "Channel",
702+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
703+
}, nil)
676704
mock.EXPECT().GetByOwner(gomock.Any(), "Version", "v-1", "ch-1").
677705
Return(nil, errors.NotFound("Version not found for channel"))
678706
},
679707
expectedStatusCode: http.StatusNotFound,
680708
},
709+
{
710+
name: "Parent not found",
711+
setupMock: func(mock *services.MockResourceService) {
712+
mock.EXPECT().Get(gomock.Any(), "Channel", "ch-1").
713+
Return(nil, errors.NotFound("Channel not found"))
714+
},
715+
expectedStatusCode: http.StatusNotFound,
716+
},
681717
}
682718

683719
for _, tt := range tests {
@@ -817,6 +853,10 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) {
817853
name: "Success 204 - nested resource force-deleted",
818854
body: `{"reason": "Stuck in finalizing"}`,
819855
setupMock: func(mock *services.MockResourceService) {
856+
mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{
857+
Meta: api.Meta{ID: parentID}, Kind: "Channel",
858+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
859+
}, nil)
820860
mock.EXPECT().
821861
GetByOwner(gomock.Any(), "Version", versionID, parentID).
822862
Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil)
@@ -830,12 +870,25 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) {
830870
name: "Error 404 - ownership mismatch",
831871
body: `{"reason": "some reason"}`,
832872
setupMock: func(mock *services.MockResourceService) {
873+
mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{
874+
Meta: api.Meta{ID: parentID}, Kind: "Channel",
875+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
876+
}, nil)
833877
mock.EXPECT().
834878
GetByOwner(gomock.Any(), "Version", versionID, parentID).
835879
Return(nil, errors.NotFound("Version with id='%s' not found for owner '%s'", versionID, parentID))
836880
},
837881
expectedStatusCode: http.StatusNotFound,
838882
},
883+
{
884+
name: "Error 404 - parent not found",
885+
body: `{"reason": "some reason"}`,
886+
setupMock: func(mock *services.MockResourceService) {
887+
mock.EXPECT().Get(gomock.Any(), "Channel", parentID).
888+
Return(nil, errors.NotFound("Channel not found"))
889+
},
890+
expectedStatusCode: http.StatusNotFound,
891+
},
839892
{
840893
name: "Error 400 - empty reason",
841894
body: `{"reason": ""}`,
@@ -847,6 +900,10 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) {
847900
name: "Error 409 - not in Finalizing state",
848901
body: `{"reason": "some reason"}`,
849902
setupMock: func(mock *services.MockResourceService) {
903+
mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{
904+
Meta: api.Meta{ID: parentID}, Kind: "Channel",
905+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
906+
}, nil)
850907
mock.EXPECT().
851908
GetByOwner(gomock.Any(), "Version", versionID, parentID).
852909
Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil)
@@ -860,6 +917,10 @@ func TestResourceHandler_ForceDeleteByOwner(t *testing.T) {
860917
name: "Error 500 - service internal error",
861918
body: `{"reason": "some reason"}`,
862919
setupMock: func(mock *services.MockResourceService) {
920+
mock.EXPECT().Get(gomock.Any(), "Channel", parentID).Return(&api.Resource{
921+
Meta: api.Meta{ID: parentID}, Kind: "Channel",
922+
Spec: datatypes.JSON(`{}`), CreatedBy: "u@t.com", UpdatedBy: "u@t.com",
923+
}, nil)
863924
mock.EXPECT().
864925
GetByOwner(gomock.Any(), "Version", versionID, parentID).
865926
Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil)
@@ -1110,3 +1171,29 @@ func TestRootResourceHandler_Create_RejectsInvalidName(t *testing.T) {
11101171
})
11111172
}
11121173
}
1174+
1175+
func TestResourceHandler_Create_ChildKindWithoutParent_Returns422(t *testing.T) {
1176+
RegisterTestingT(t)
1177+
ctrl := gomock.NewController(t)
1178+
defer ctrl.Finish()
1179+
1180+
t.Cleanup(registry.Reset)
1181+
registry.Reset()
1182+
registry.Register(channelDescriptor)
1183+
registry.Register(versionDescriptor)
1184+
1185+
mockSvc := services.NewMockResourceService(ctrl)
1186+
handler := NewResourceHandler(versionDescriptor, mockSvc)
1187+
1188+
req := httptest.NewRequest(http.MethodPost,
1189+
"/api/hyperfleet/v1/versions",
1190+
strings.NewReader(`{"kind":"Version","name":"4-17-3","spec":{"raw_version":"4.17.3"}}`))
1191+
req.Header.Set("Content-Type", "application/json")
1192+
req = mux.SetURLVars(req, map[string]string{})
1193+
rr := httptest.NewRecorder()
1194+
1195+
handler.Create(rr, req)
1196+
1197+
Expect(rr.Code).To(Equal(http.StatusUnprocessableEntity))
1198+
Expect(rr.Body.String()).To(ContainSubstring("/channels/{id}/versions"))
1199+
}

‎pkg/middleware/schema_validation.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,9 @@ func shouldValidateRequest(
174174
}
175175
}
176176

177-
if rootResourcePattern.MatchString(path) {
177+
// Root /resources POST carries kind in body — resolve plural from body later.
178+
// Root /resources PATCH has no kind; handler validates spec internally.
179+
if method == http.MethodPost && rootResourcePattern.MatchString(path) {
178180
return true, ""
179181
}
180182

0 commit comments

Comments
 (0)