HYPERFLEET-1084 - feat: add generic resource data layer with entity registry#180
Open
tirthct wants to merge 1 commit into
Open
HYPERFLEET-1084 - feat: add generic resource data layer with entity registry#180tirthct wants to merge 1 commit into
tirthct wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR adds a generic GORM-backed Resource model with lifecycle hooks, indexing helpers, and mutation helpers. It adds a ResourceDao interface with a SQL implementation and an in-memory mock, a gormigrate migration to create the resources table and indexes, an EntityDescriptor/registry for entity metadata and href composition, and SQL helper support plus tests for querying JSONB spec fields via spec.. sequenceDiagram
participant Client
participant API as API.Server
participant BeforeCreate as Resource.BeforeCreate
participant Registry as registry.MustGet
participant DAO as sqlResourceDao
participant DB
Client->>API: POST /{plural} create payload
API->>BeforeCreate: construct Resource, call BeforeCreate
BeforeCreate->>Registry: MustGet(kind)
Registry-->>BeforeCreate: EntityDescriptor (plural/parent)
BeforeCreate->>DAO: Create(resource)
DAO->>DB: INSERT INTO resources ...
DB-->>DAO: success
DAO-->>API: created resource
API-->>Client: 201 Created (with Href)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/registry/registry_test.go (1)
9-167: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRefactor repeated unit cases into table-driven scenario tests.
This file has several same-shape tests (
Register/Get/MustGet/Validate) that should be consolidated into table-driven cases with scenario names to reduce duplication and improve extension safety.As per coding guidelines,
**/*_test.go: "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/registry/registry_test.go` around lines 9 - 167, Multiple tests in this file repeat the same setup/assertion patterns (e.g., TestRegister_Success, TestRegister_DuplicateType_Panics, TestGet_NotFound, TestMustGet_Success, TestMustGet_NotFound_Panics, TestValidate_*); refactor them into table-driven subtests using a slice of scenarios with a name and inputs/expected behavior, then iterate with t.Run for each case, calling Reset(), Register(EntityDescriptor{...}), and the appropriate function under test (Register, Get, MustGet, Validate, ChildrenOf, All) inside each subtest while preserving existing Gomega assertions (RegisterTestingT(t) per top-level test or per subtest as preferred); retain unique behavior tests like TestDescriptorFields if they don't fit the table shape.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/api/resource.go`:
- Around line 70-75: The BeforeCreate hook currently calls registry.MustGet for
r.Kind and for *r.OwnerType which panics on unknown kinds; replace those MustGet
calls with the non-panicking lookup (e.g., registry.Get or equivalent) and
return a descriptive error when the kind or owner type is not found instead of
letting it panic; update the checks around r.Kind, r.OwnerID, r.OwnerType and
the parentDesc lookup in the BeforeCreate method so that missing registry
entries produce fmt.Errorf errors that cause the insert to fail gracefully.
- Around line 76-81: The child Href construction currently rebuilds the parent
path using parentDesc.Plural and *r.OwnerID which drops ancestor segments;
change the logic in the block that sets r.Href and r.OwnerHref so that if
r.OwnerHref is non-nil you use the existing *r.OwnerHref as the base path when
forming r.Href (append desc.Plural and r.ID to it), and only synthesize
ownerHref via fmt.Sprintf("/api/hyperfleet/v1/%s/%s", parentDesc.Plural,
*r.OwnerID) and assign r.OwnerHref when r.OwnerHref is nil; update references to
r.Href, r.OwnerHref, parentDesc.Plural, *r.OwnerID and desc.Plural accordingly.
- Around line 69-85: The ownership/parentage validation must run regardless of
whether r.Href is pre-populated: move or duplicate the checks that inspect
r.OwnerID, r.OwnerType and registry.MustGet into a validation path executed
before or outside the if r.Href == "" block so callers cannot bypass them;
ensure that when the Kind has a declared ParentType (use
registry.MustGet(r.Kind) and or registry.MustGet(*r.OwnerType)) you enforce
OwnerID presence/OwnerType correctness and set/validate OwnerHref consistency
even if r.Href was already set, and only then compute or verify r.Href (using
the existing fmt.Sprintf logic) so Href always matches the registry contract.
In `@pkg/dao/mocks/resource.go`:
- Around line 18-22: The mock currently returns and stores live pointers to its
internal resources, letting callers mutate internal state; update
resourceDaoMock methods (notably Get, List, Create/Save/Update and any methods
around the indicated ranges) to always clone/deep-copy api.Resource instances
when returning to callers and when inserting/updating the internal d.resources
slice so the mock emulates SQL DAO semantics (no in-place persistence). Locate
functions named Get, List, Create/Save/Update in resourceDaoMock and replace
direct pointer returns/assignments with newly allocated copies (copying all
fields and nested structs/slices) so mutations on the returned object do not
affect the mock's internal state.
- Around line 40-43: The mock Create implementation in resourceDaoMock currently
appends any owner combination; change it to mirror the SQL DAO's owner
validation/normalization before appending: in resourceDaoMock.Create, perform
the same checks used by the SQL DAO (reject when OwnerID=="" but OwnerType is
set, and when OwnerID is non-empty but OwnerType is missing, normalize OwnerType
to the same default the SQL DAO uses), or better yet call the shared helper the
SQL DAO uses (e.g., validate/normalize owner function) and return the same error
when validation fails; only append to d.resources and return the resource when
validation/normalization succeeds.
In `@pkg/dao/resource.go`:
- Line 23: FindByIDs currently returns resources across all kinds; update its
contract and implementation so queries are always scoped to the DAO's kind (or
an explicit kind parameter). Change the FindByIDs implementation (and any query
construction around the existing block at lines ~130-137) to include a kind
filter (use the DAO's kind field or add a kind argument to FindByIDs) so the
returned api.ResourceList contains only records matching that kind; ensure
callers are adjusted to pass/expect the kind-scoped behavior.
In `@pkg/registry/registry.go`:
- Around line 21-22: The registry currently stores and returns descriptors and
their SearchDisallowedFields slices by reference, allowing external mutation;
update Register (where descriptors[d.Type] = d) to store a defensive copy of the
incoming Descriptor including a copied SearchDisallowedFields slice, and update
accessors Get, All, and ChildrenOf to return copies (deep copy or at least copy
the SearchDisallowedFields slice and the Descriptor struct) rather than
references into the internal map so callers cannot mutate internal state without
locking; ensure any creation of descriptor lists/maps in All/ChildrenOf clones
each Descriptor and its slice before returning.
- Around line 77-94: The Validate() loop must reject descriptors with empty Type
or Plural: add checks in the same loop that if d.Type == "" or d.Plural == ""
you fail fast (match the existing behavior—panic or return an error consistent
with Validate()) with a clear message naming the missing field and the offending
descriptor (use d and its map key for context). Keep these checks before using
d.Type/d.Plural (i.e., before the ParentType and plurals duplicate checks) so
invalid descriptors cannot produce ambiguous Kind values or malformed href
segments.
---
Outside diff comments:
In `@pkg/registry/registry_test.go`:
- Around line 9-167: Multiple tests in this file repeat the same setup/assertion
patterns (e.g., TestRegister_Success, TestRegister_DuplicateType_Panics,
TestGet_NotFound, TestMustGet_Success, TestMustGet_NotFound_Panics,
TestValidate_*); refactor them into table-driven subtests using a slice of
scenarios with a name and inputs/expected behavior, then iterate with t.Run for
each case, calling Reset(), Register(EntityDescriptor{...}), and the appropriate
function under test (Register, Get, MustGet, Validate, ChildrenOf, All) inside
each subtest while preserving existing Gomega assertions (RegisterTestingT(t)
per top-level test or per subtest as preferred); retain unique behavior tests
like TestDescriptorFields if they don't fit the table shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3171c7b5-edfa-467a-967a-f8614361902d
📒 Files selected for processing (11)
pkg/api/resource.gopkg/api/resource_test.gopkg/dao/mocks/resource.gopkg/dao/resource.gopkg/db/migrations/202605202128_add_resources.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/registry/registry_test.go
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/api/resource_test.go (1)
60-182: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRefactor
BeforeCreatetests to table-driven form.These tests cover one behavior family (
BeforeCreate) across many input variants, but each case is split into a standalone test with duplicated setup/assert scaffolding. A table-driven structure will reduce repetition and make adding/reading cases significantly easier.As per coding guidelines "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 - 182, The current many TestResource_BeforeCreate_* functions duplicate setup/assert scaffolding; refactor them into a single table-driven test (e.g., TestResource_BeforeCreate_Table) that iterates cases describing input Resource fields and expected outcomes for ID, Generation, CreatedTime/UpdatedTime, Href, OwnerHref and error conditions; inside the loop call r.BeforeCreate(nil) and run the appropriate assertions per-case (preserve existing expectations like ID generation vs preservation, Generation default vs preserved, timestamps present or preserved, top-level vs child Href and OwnerKind-missing error, and preserved custom Href), reuse setupTestRegistry() and RegisterTestingT(t) once at start, and reference the Resource type and its BeforeCreate method and existing case names (TestResource_BeforeCreate_IDGeneration, _IDPreservation, _GenerationDefault, _GenerationPreserved, _Timestamps, _CreatedTimePreserved, _HrefTopLevel, _HrefChild, _OwnerKindMissing, _HrefPreserved) to map each case into the table rows.pkg/registry/registry_test.go (1)
102-166: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse a table-driven test for the
Validatecase matrix.The
Validatescenarios are split into many near-identical tests with repeated setup, which makes future coverage expansion harder and noisier to maintain. Consolidate these into one table-driven test with{name, descriptors, expectPanic, panicContains}cases.As per coding guidelines "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/registry/registry_test.go` around lines 102 - 166, Replace the many near-identical TestValidate_* functions with a single table-driven test that iterates cases defined as {name, descriptors []EntityDescriptor, expectPanic bool, panicContains string}; inside the loop call t.Run(name, func(t *testing.T){ RegisterTestingT(t); Reset(); register each descriptor via Register(...); then assert Validate() either panics with the provided substring (use PanicWith(ContainSubstring(panicContains))) when expectPanic is true or does not panic otherwise }). Keep using the existing symbols EntityDescriptor, Register, Reset, Validate and the same Gomega assertions to preserve behavior.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@pkg/api/resource_test.go`:
- Around line 60-182: The current many TestResource_BeforeCreate_* functions
duplicate setup/assert scaffolding; refactor them into a single table-driven
test (e.g., TestResource_BeforeCreate_Table) that iterates cases describing
input Resource fields and expected outcomes for ID, Generation,
CreatedTime/UpdatedTime, Href, OwnerHref and error conditions; inside the loop
call r.BeforeCreate(nil) and run the appropriate assertions per-case (preserve
existing expectations like ID generation vs preservation, Generation default vs
preserved, timestamps present or preserved, top-level vs child Href and
OwnerKind-missing error, and preserved custom Href), reuse setupTestRegistry()
and RegisterTestingT(t) once at start, and reference the Resource type and its
BeforeCreate method and existing case names
(TestResource_BeforeCreate_IDGeneration, _IDPreservation, _GenerationDefault,
_GenerationPreserved, _Timestamps, _CreatedTimePreserved, _HrefTopLevel,
_HrefChild, _OwnerKindMissing, _HrefPreserved) to map each case into the table
rows.
In `@pkg/registry/registry_test.go`:
- Around line 102-166: Replace the many near-identical TestValidate_* functions
with a single table-driven test that iterates cases defined as {name,
descriptors []EntityDescriptor, expectPanic bool, panicContains string}; inside
the loop call t.Run(name, func(t *testing.T){ RegisterTestingT(t); Reset();
register each descriptor via Register(...); then assert Validate() either panics
with the provided substring (use PanicWith(ContainSubstring(panicContains)))
when expectPanic is true or does not panic otherwise }). Keep using the existing
symbols EntityDescriptor, Register, Reset, Validate and the same Gomega
assertions to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 575b1d53-3a91-45ad-bc45-f8b5db6e9192
📒 Files selected for processing (8)
pkg/api/resource.gopkg/api/resource_test.gopkg/dao/mocks/resource.gopkg/dao/resource.gopkg/db/migrations/202605202128_add_resources.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/registry/registry_test.go
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1107 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/dao pkg/dao/mocks pkg/db/migrations | +1 |
Computed by hyperfleet-risk-scorer
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/api/resource_test.go`:
- Around line 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.
- Around line 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.
In `@pkg/api/resource.go`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9c87074e-b0df-4735-b381-123933fd1aae
📒 Files selected for processing (11)
pkg/api/resource.gopkg/api/resource_test.gopkg/dao/mocks/resource.gopkg/dao/resource.gopkg/db/migrations/202605202128_add_resources.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/registry/registry_test.go
Comment on lines
+60
to
+214
| 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")) | ||
| } | ||
|
|
||
| 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")) | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+189
to
+204
| 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")) | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+72
to
+84
| 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) |
There was a problem hiding this comment.
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.
rafabene
reviewed
May 21, 2026
Comment on lines
+14
to
+25
| func Register(d EntityDescriptor) { | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
|
|
||
| if d.Kind == "" { | ||
| panic("entity kind cannot be empty") | ||
| } | ||
| if _, exists := descriptors[d.Kind]; exists { | ||
| panic(fmt.Sprintf("entity kind %q already registered", d.Kind)) | ||
| } | ||
| descriptors[d.Kind] = d | ||
| } |
Contributor
There was a problem hiding this comment.
[Pattern · nit · High confidence] `Register()` silently accepts descriptors with empty `Plural`
`Register()` validates `Kind != ""` (line 18) but does not validate `Plural != ""`. A descriptor with `Kind: "Foo", Plural: ""` is accepted and stored. While `Validate()` catches empty `Plural` later, between `Register()` and `Validate()` the registry contains an invalid descriptor. If any code calls `MustGet` during that window, it gets back a descriptor with empty `Plural`, producing malformed hrefs like `/api/hyperfleet/v1//resource-id`. Adding `Plural` validation to `Register` would fail-fast and be consistent with the `Kind` check already present.
Suggested change
| func Register(d EntityDescriptor) { | |
| mu.Lock() | |
| defer mu.Unlock() | |
| if d.Kind == "" { | |
| panic("entity kind cannot be empty") | |
| } | |
| if _, exists := descriptors[d.Kind]; exists { | |
| panic(fmt.Sprintf("entity kind %q already registered", d.Kind)) | |
| } | |
| descriptors[d.Kind] = d | |
| } | |
| func Register(d EntityDescriptor) { | |
| mu.Lock() | |
| defer mu.Unlock() | |
| if d.Kind == "" { | |
| panic("entity kind cannot be empty") | |
| } | |
| if d.Plural == "" { | |
| panic(fmt.Sprintf("entity kind %q has empty plural", d.Kind)) | |
| } | |
| if _, exists := descriptors[d.Kind]; exists { | |
| panic(fmt.Sprintf("entity kind %q already registered", d.Kind)) | |
| } | |
| descriptors[d.Kind] = d | |
| } |
Contributor
|
[JIRA · nit · High confidence] AC field names deviate from implementation naming The JIRA ticket (HYPERFLEET-1084) AC specifies Suggestion: Update the JIRA ticket AC to replace |
kuudori
reviewed
May 21, 2026
| ResourceIndex map[string]*Resource | ||
| ) | ||
|
|
||
| func (l ResourceList) Index() ResourceIndex { |
Contributor
There was a problem hiding this comment.
Dead code? Appears only in the test itself 🙂
| Save(ctx context.Context, resource *api.Resource) error | ||
| Delete(ctx context.Context, kind, id string) error | ||
| CountByOwner(ctx context.Context, kind, ownerID string) (int64, error) | ||
| FindByType(ctx context.Context, kind string) (api.ResourceList, error) |
| CountByOwner(ctx context.Context, kind, ownerID string) (int64, error) | ||
| FindByType(ctx context.Context, kind string) (api.ResourceList, error) | ||
| FindByTypeAndOwner(ctx context.Context, kind, ownerID string) (api.ResourceList, error) | ||
| FindByIDs(ctx context.Context, kind string, ids []string) (api.ResourceList, error) |
Contributor
There was a problem hiding this comment.
I wonder if a single FindByFilter(ctx, filter) would fit better here. Something like:
type ResourceFilter struct {
Kind *string
OwnerID *string
IDs []string // nil or empty = no ID filter
}
FindByFilter(ctx, filters.ResourceFilter{Kind: ptr.To("cluster")})
FindByFilter(ctx, filters.ResourceFilter{Kind: ptr.To("cluster"), OwnerID: ptr.To(owner)})
FindByFilter(ctx, filters.ResourceFilter{IDs: ids})
This way we wouldn't need a new method every time a new query shape comes up, and callers that need to combine criteria (e.g. owner + IDs) get it for free.
| return &sqlResourceDao{sessionFactory: sessionFactory} | ||
| } | ||
|
|
||
| func (d *sqlResourceDao) Get(ctx context.Context, kind, id string) (*api.Resource, error) { |
Contributor
There was a problem hiding this comment.
I'm wondering if kind is required here, since ID should be unique. @rh-amarin WDYT?
| deleted_by VARCHAR(255) NULL, | ||
| owner_id VARCHAR(255) NULL, | ||
| owner_kind VARCHAR(100) NULL, | ||
| owner_href VARCHAR(500) NULL, |
Contributor
There was a problem hiding this comment.
Are these constraints required?
Not complaining, just asking, since I mostly used TEXT.
| } | ||
|
|
||
| if err := tx.Exec( | ||
| "CREATE INDEX IF NOT EXISTS idx_resources_kind ON resources (kind);", |
Contributor
There was a problem hiding this comment.
I'm wondering if this index will be selective enough for postgres. Right now, we have... fewer than 5 kinds?
| 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"` |
Contributor
There was a problem hiding this comment.
kuudori
reviewed
May 21, 2026
| owner_href VARCHAR(500) NULL, | ||
| spec JSONB NOT NULL, | ||
| labels JSONB NULL, | ||
| generation INTEGER NOT NULL DEFAULT 1 |
Contributor
There was a problem hiding this comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ResourceGORM model,ResourceDAO, database migration, andEntityDescriptorregistrythat all new entity types (Channel, Version, WIF Config) will share
spec.xxxJSONB field mapping to the TSL search infrastructure for querying spec fieldsresourcestable lives alongside existingclustersandnode_poolstables — no migrationof existing entities
Context
HYPERFLEET-1084 is the foundational story in
the Version Management API epic (HYPERFLEET-1077).
It establishes a generic resource pattern so that adding a new entity type requires only a descriptor
registration and a thin handler — eliminating ~832 lines of duplicated DAO/service/handler code per entity.
Design context: architecture PR #122
(Generic Resource Registry DDR).
What's new
Entity Registry (
pkg/registry/)EntityDescriptorstruct with: Kind, Plural, NameMinLen, NameMaxLen, ParentKind, OnParentDelete,SpecSchemaName, SearchDisallowedFields
init()calls — no config YAML (deferred to v2)Resource model (
pkg/api/resource.go)KindOwnerID/OwnerKind/OwnerHreffor parent-child relationships (nullable for top-level entities)BeforeCreatehook generates UUIDv7, sets timestamps, initializes generation, and computes hreffrom the entity registry descriptor + parent chain (supports deep nesting via OwnerHref base path)
StatusConditions— these resources are not adapter-managedResourceDAO (
pkg/dao/resource.go)FindByTypeAndOwner, FindByIDs
kind = ?to prevent cross-kind leakageDatabase migration
resourcestable with 5 indexes:idx_resources_kind— drives all list queriesidx_resources_kind_name— unique partial (top-level, non-deleted)idx_resources_kind_owner_name— unique partial (child, non-deleted)idx_resources_owner_id— child lookupsidx_resources_deleted_time— soft-delete queriesSpec field filtering (
pkg/db/sql_helpers.go)spec.xxxmaps tospec->>'xxx'in TSL search queriesSearchDisallowedFields— entity types that disallow spec searches (Cluster, NodePool)are not bypassed
Design decisions
resource_labelstable) — consistent with the existing Cluster/NodePool pattern. The DDR proposed separate tables, but the ticket chose JSONB for consistency.Kindnaming — EntityDescriptor usesKind/ParentKind, Resource usesOwnerKind. Avoids having bothTypeandKindas variable names across the codebase.Out of scope
Test plan
make lint— 0 issuesmake test— 957 unit tests pass (39 new)make test-integration— 102 integration tests pass, migration applies cleanlyOwnerKind guard), BeforeUpdate, MarkDeleted, IncrementGeneration, TableName (17 tests)