Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 103 additions & 0 deletions pkg/api/resource.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package api

import (
"fmt"
"time"

"gorm.io/datatypes"
"gorm.io/gorm"

"github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry"
)

// Resource is the generic GORM model for entity types managed by the entity
// registry (Channel, Version, WIF Config, etc.). Entity kinds are
// differentiated by the Kind field. Existing Cluster and NodePool types
// are NOT migrated to this model.
type Resource struct {
Meta
Kind string `json:"kind" gorm:"size:100;not null"`
Name string `json:"name" gorm:"size:100;not null"`
Href string `json:"href,omitempty" gorm:"size:500"`
CreatedBy string `json:"created_by" gorm:"size:255;not null"`
UpdatedBy string `json:"updated_by" gorm:"size:255;not null"`
DeletedBy *string `json:"deleted_by,omitempty" gorm:"size:255"`
DeletedTime *time.Time `json:"deleted_time,omitempty"`
OwnerID *string `json:"owner_id,omitempty" gorm:"size:255"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OwnerKind *string `json:"owner_kind,omitempty" gorm:"size:100"`
OwnerHref *string `json:"owner_href,omitempty" gorm:"size:500"`
Spec datatypes.JSON `json:"spec" gorm:"type:jsonb;not null"`
Labels datatypes.JSON `json:"labels,omitempty" gorm:"type:jsonb"`
Generation int32 `json:"generation" gorm:"default:1;not null"`
}

type (
ResourceList []*Resource
ResourceIndex map[string]*Resource
)

func (l ResourceList) Index() ResourceIndex {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code? Appears only in the test itself 🙂

index := ResourceIndex{}
for _, o := range l {
index[o.ID] = o
}
return index
}

func (r Resource) TableName() string {
return "resources"
}

// BeforeCreate TODO: Validate the necessity for this as part of https://redhat.atlassian.net/browse/HYPERFLEET-1085
func (r *Resource) BeforeCreate(tx *gorm.DB) error {
if r.ID == "" {
id, err := NewID()
if err != nil {
return fmt.Errorf("failed to generate resource ID: %w", err)
}
r.ID = id
}

now := time.Now()
if r.CreatedTime.IsZero() {
r.CreatedTime = now
}
r.UpdatedTime = now
if r.Generation == 0 {
r.Generation = 1
}

if r.Href == "" {
desc := registry.MustGet(r.Kind)
if r.OwnerID != nil && *r.OwnerID != "" {
if r.OwnerKind == nil || *r.OwnerKind == "" {
return fmt.Errorf("owner_kind is required when owner_id is set")
}
if r.OwnerHref == nil {
parentDesc := registry.MustGet(*r.OwnerKind)
ownerHref := fmt.Sprintf("/api/hyperfleet/v1/%s/%s",
parentDesc.Plural, *r.OwnerID)
r.OwnerHref = &ownerHref
}
r.Href = fmt.Sprintf("%s/%s/%s", *r.OwnerHref, desc.Plural, r.ID)
} else {
r.Href = fmt.Sprintf("/api/hyperfleet/v1/%s/%s", desc.Plural, r.ID)
Comment on lines +72 to +84
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize owner fields and treat empty owner_href as missing.

On Line 76 and Line 82, an explicitly empty owner_href ("") is accepted and produces malformed child hrefs (e.g. /<plural>/<id>). Also, when owner_id is absent (Line 83), stale owner_kind/owner_href are not cleared, allowing inconsistent records.

Suggested fix
 import (
 	"fmt"
+	"strings"
 	"time"
@@
-		if r.OwnerID != nil && *r.OwnerID != "" {
+		if r.OwnerID != nil && strings.TrimSpace(*r.OwnerID) != "" {
 			if r.OwnerKind == nil || *r.OwnerKind == "" {
 				return fmt.Errorf("owner_kind is required when owner_id is set")
 			}
-			if r.OwnerHref == nil {
+			if r.OwnerHref == nil || strings.TrimSpace(*r.OwnerHref) == "" {
 				parentDesc := registry.MustGet(*r.OwnerKind)
 				ownerHref := fmt.Sprintf("/api/hyperfleet/v1/%s/%s",
 					parentDesc.Plural, *r.OwnerID)
 				r.OwnerHref = &ownerHref
 			}
 			r.Href = fmt.Sprintf("%s/%s/%s", *r.OwnerHref, desc.Plural, r.ID)
 		} else {
+			r.OwnerID = nil
+			r.OwnerKind = nil
+			r.OwnerHref = nil
 			r.Href = fmt.Sprintf("/api/hyperfleet/v1/%s/%s", desc.Plural, r.ID)
 		}

As per coding guidelines "Validate changes against HyperFleet architecture standards from the linked architecture repository."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/api/resource.go` around lines 72 - 84, When constructing child Href in
Validate/normalization, treat empty owner fields as missing and clear stale
owner metadata: if r.OwnerID is nil or points to an empty string then set
r.OwnerKind and r.OwnerHref to nil and build r.Href from desc.Plural and r.ID;
if r.OwnerID is non-empty ensure r.OwnerKind is present, and treat
r.OwnerHref=="" as missing (nil) so you call registry.MustGet(*r.OwnerKind) and
set r.OwnerHref before composing r.Href; update logic around r.OwnerID,
r.OwnerKind, r.OwnerHref and r.Href (references: r.OwnerID, r.OwnerKind,
r.OwnerHref, r.Href, desc.Plural, registry.MustGet) to normalize empty strings
to nil and clear inconsistent state.

}
Comment thread
tirthct marked this conversation as resolved.
}

return nil
}

func (r *Resource) BeforeUpdate(tx *gorm.DB) error {
r.UpdatedTime = time.Now()
return nil
}

func (r *Resource) MarkDeleted(by string, t time.Time) {
r.DeletedTime = &t
r.DeletedBy = &by
}

func (r *Resource) IncrementGeneration() {
r.Generation++
}
258 changes: 258 additions & 0 deletions pkg/api/resource_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,258 @@
package api

import (
"testing"
"time"

. "github.com/onsi/gomega"

"github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry"
)

func setupTestRegistry() {
registry.Reset()
registry.Register(registry.EntityDescriptor{
Kind: "Channel",
Plural: "channels",
})
registry.Register(registry.EntityDescriptor{
Kind: "Version",
Plural: "versions",
ParentKind: "Channel",
})
}

func strPtr(s string) *string {
return &s
}

func TestResourceList_Index(t *testing.T) {
RegisterTestingT(t)

emptyList := ResourceList{}
emptyIndex := emptyList.Index()
Expect(len(emptyIndex)).To(Equal(0))

r1 := &Resource{}
r1.ID = "res-1"
r1.Name = "test-resource-1"

r2 := &Resource{}
r2.ID = "res-2"
r2.Name = "test-resource-2"

multiList := ResourceList{r1, r2}
multiIndex := multiList.Index()
Expect(len(multiIndex)).To(Equal(2))
Expect(multiIndex["res-1"]).To(Equal(r1))
Expect(multiIndex["res-2"]).To(Equal(r2))

r1Dup := &Resource{}
r1Dup.ID = "res-1"
r1Dup.Name = "duplicate"

dupList := ResourceList{r1, r1Dup}
dupIndex := dupList.Index()
Expect(len(dupIndex)).To(Equal(1))
Expect(dupIndex["res-1"].Name).To(Equal("duplicate"))
}

func TestResource_BeforeCreate_IDGeneration(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{Name: "test", Kind: "Channel"}

err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.ID).ToNot(BeEmpty())
}

func TestResource_BeforeCreate_IDPreservation(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{Name: "test", Kind: "Channel"}
r.ID = "pre-set-id"

err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.ID).To(Equal("pre-set-id"))
}

func TestResource_BeforeCreate_GenerationDefault(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{Name: "test", Kind: "Channel"}

err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.Generation).To(Equal(int32(1)))
}

func TestResource_BeforeCreate_GenerationPreserved(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{Name: "test", Kind: "Channel", Generation: 5}

err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.Generation).To(Equal(int32(5)))
}

func TestResource_BeforeCreate_Timestamps(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

before := time.Now()
r := &Resource{Name: "test", Kind: "Channel"}

err := r.BeforeCreate(nil)
Expect(err).To(BeNil())

Expect(r.CreatedTime).ToNot(BeZero())
Expect(r.UpdatedTime).ToNot(BeZero())
Expect(r.CreatedTime.After(before) || r.CreatedTime.Equal(before)).To(BeTrue())
Expect(r.UpdatedTime.After(before) || r.UpdatedTime.Equal(before)).To(BeTrue())
}

func TestResource_BeforeCreate_CreatedTimePreserved(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

fixedTime := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
r := &Resource{Name: "test", Kind: "Channel"}
r.CreatedTime = fixedTime

err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.CreatedTime).To(Equal(fixedTime))
}

func TestResource_BeforeCreate_HrefTopLevel(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{Name: "stable", Kind: "Channel"}
err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.Href).To(Equal("/api/hyperfleet/v1/channels/" + r.ID))
}

func TestResource_BeforeCreate_HrefChild(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{
Name: "4-17-12",
Kind: "Version",
OwnerID: strPtr("ch-1"),
OwnerKind: strPtr("Channel"),
}
err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.Href).To(Equal("/api/hyperfleet/v1/channels/ch-1/versions/" + r.ID))
Expect(*r.OwnerHref).To(Equal("/api/hyperfleet/v1/channels/ch-1"))
}

func TestResource_BeforeCreate_OwnerKindMissing(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{
Name: "4-17-12",
Kind: "Version",
OwnerID: strPtr("ch-1"),
}
err := r.BeforeCreate(nil)
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("owner_kind is required"))
}

func TestResource_BeforeCreate_OwnerKindEmpty(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{
Name: "4-17-12",
Kind: "Version",
OwnerID: strPtr("ch-1"),
OwnerKind: strPtr(""),
}
err := r.BeforeCreate(nil)
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("owner_kind is required"))
}

func TestResource_BeforeCreate_HrefChildWithPresetOwnerHref(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{
Name: "some-label",
Kind: "Version",
OwnerID: strPtr("v-1"),
OwnerKind: strPtr("Version"),
OwnerHref: strPtr("/api/hyperfleet/v1/channels/ch-1/versions/v-1"),
}
err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.Href).To(Equal("/api/hyperfleet/v1/channels/ch-1/versions/v-1/versions/" + r.ID))
Expect(*r.OwnerHref).To(Equal("/api/hyperfleet/v1/channels/ch-1/versions/v-1"))
}
Comment on lines +189 to +204
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Version-owned-by-Version fixture conflicts with the registered parent contract.

Line 197 sets OwnerKind to Version, but in this test registry Version is defined as child of Channel. Keeping this as a success case risks codifying invalid owner/href chains that downstream consumers rely on being consistent.

As per coding guidelines "Validate changes against HyperFleet architecture standards from the linked architecture repository."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/api/resource_test.go` around lines 189 - 204, The test fixture in
TestResource_BeforeCreate_HrefChildWithPresetOwnerHref uses OwnerKind "Version"
which conflicts with the registered parent contract; update the Resource fixture
used in this test (fields OwnerKind, OwnerID, OwnerHref) to reflect the correct
parent type (e.g., set OwnerKind to "Channel" with the corresponding OwnerID and
OwnerHref for the channel) and adjust the expected Href/assertion accordingly,
or alternatively change the test to assert an error from Resource.BeforeCreate
when OwnerKind is invalid; locate the Resource struct instance in
TestResource_BeforeCreate_HrefChildWithPresetOwnerHref and modify its
OwnerKind/OwnerID/OwnerHref and expected Href to match the registry's
parent-child contract enforced by BeforeCreate.


func TestResource_BeforeCreate_HrefPreserved(t *testing.T) {
RegisterTestingT(t)
setupTestRegistry()

r := &Resource{Name: "test", Kind: "Channel", Href: "/custom/href"}
err := r.BeforeCreate(nil)
Expect(err).To(BeNil())
Expect(r.Href).To(Equal("/custom/href"))
}
Comment on lines +60 to +214
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Refactor BeforeCreate coverage into a table-driven test with scenario-based case names.

These cases are currently split into many implementation-named tests, which makes maintenance harder and drifts from test conventions for multi-scenario behavior.

As per coding guidelines "Test names describe the scenario, not the implementation" and "Table-driven tests used for multiple cases".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/api/resource_test.go` around lines 60 - 214, The tests for
Resource.BeforeCreate are split into many implementation-named functions;
refactor them into a single table-driven test that iterates scenarios with
descriptive case names (e.g., "generates ID when missing", "preserves pre-set
ID", "sets initial generation", "preserves generation", "sets timestamps",
"preserves CreatedTime", "builds top-level Href", "builds child Href and
OwnerHref", "errors when owner_kind missing/empty", "uses preset Href"). Create
a slice of test cases each containing input Resource, optional pre-test setup
(like fixed CreatedTime), expected outputs or expected error substrings, and a
name; then loop over cases and call r.BeforeCreate(nil) asserting results
(checking r.ID, r.Generation, r.CreatedTime/UpdatedTime, r.Href, r.OwnerHref,
and error strings) using the same helpers currently used in tests. Keep the
setupTestRegistry() call outside the loop and reuse the Resource type and
BeforeCreate method names to locate the code.


func TestResource_BeforeUpdate_UpdatesTimestamp(t *testing.T) {
RegisterTestingT(t)

r := &Resource{Name: "test", Kind: "Channel"}
r.UpdatedTime = time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)

before := time.Now()
err := r.BeforeUpdate(nil)
Expect(err).To(BeNil())
Expect(r.UpdatedTime.After(before) || r.UpdatedTime.Equal(before)).To(BeTrue())
}

func TestResource_MarkDeleted(t *testing.T) {
RegisterTestingT(t)

r := &Resource{Name: "test", Kind: "Channel"}
now := time.Now()

r.MarkDeleted("admin", now)

Expect(r.DeletedTime).ToNot(BeNil())
Expect(*r.DeletedTime).To(Equal(now))
Expect(r.DeletedBy).ToNot(BeNil())
Expect(*r.DeletedBy).To(Equal("admin"))
}

func TestResource_IncrementGeneration(t *testing.T) {
RegisterTestingT(t)

r := &Resource{Name: "test", Kind: "Channel", Generation: 1}
r.IncrementGeneration()
Expect(r.Generation).To(Equal(int32(2)))

r.IncrementGeneration()
Expect(r.Generation).To(Equal(int32(3)))
}

func TestResource_TableName(t *testing.T) {
RegisterTestingT(t)

r := Resource{}
Expect(r.TableName()).To(Equal("resources"))
}
Loading