Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kuudori wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 61 additions & 42 deletions pkg/handlers/resource_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Comment thread
kuudori marked this conversation as resolved.
} else {
resource, err = presenters.ConvertResource(&req)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand All @@ -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)
Expand All @@ -109,17 +106,19 @@ 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
}

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)
Expand Down Expand Up @@ -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
}

Expand All @@ -170,31 +169,61 @@ 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 {
// 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
}
}
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 {
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}
87 changes: 87 additions & 0 deletions pkg/handlers/resource_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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"))
},
Expand All @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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": ""}`,
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Comment thread
kuudori marked this conversation as resolved.

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"))
}
4 changes: 3 additions & 1 deletion pkg/middleware/schema_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment thread
kuudori marked this conversation as resolved.
Comment thread
kuudori marked this conversation as resolved.
return true, ""
}

Expand Down
Loading