HYPERFLEET-1156 - feat: add resource references#288
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 |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (24)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds persisted non-ownership resource references with validation, create/patch/delete/force-delete handling, reference-aware listing, API presentation, database storage, and integration coverage for required, optional, duplicate, and soft-deleted target cases. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1396 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/api pkg/dao pkg/dao/mocks pkg/db/migrations pkg/registry | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
test/integration/resource_references_test.go (1)
1-427: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing coverage for duplicate refs and soft-deleted targets.
PR objectives explicitly list validation for "duplicates, and soft-deleted targets" as part of this feature, but no test here exercises either path — only missing-required-ref, non-existent-target, and too-many-refs are covered. These are exactly the kind of critical-logic error paths this suite should assert on.
Suggest adding:
- A case creating
RefSourcewith two identical{id, kind}entries under the sameRefType(duplicate target within one ref array) and expecting 400.- A case referencing a
RefTargetthat has been soft-deleted and expecting 400 (or the applicable rejection code), similar toTestResourceReferences_CreateRefToNonExistentTarget.As per path instructions, "Error paths SHOULD be tested, not just happy paths."
🤖 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 `@test/integration/resource_references_test.go` around lines 1 - 427, Add test coverage for the missing error paths in the resource reference suite: extend resource_references_test.go with a create scenario in TestResourceReferences_... that uses RefSource and makeRefs to submit two identical ObjectReference entries under the same ref type and assert the service rejects it with 400, and add another create scenario that points RefSource at a RefTarget after it has been soft-deleted (or otherwise marked deleted) and assert rejection with the appropriate error code. Use the existing setupRefTest, newRefTestResource, makeRefs, svc.Create, and svc.Delete/soft-delete helpers so the new cases match the current integration patterns.Source: Path instructions
🧹 Nitpick comments (8)
pkg/api/presenters/resource.go (1)
116-131: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Hrefbuilt from unescapedTargetIDviafmt.Sprintf.
ref.TargetIDis interpolated directly into the URL path withouturl.PathEscape. Target IDs are presumably server-generated UUIDs validated to exist by the FK constraint, so exploitability is low today (CWE-116 output-encoding hygiene, not a live injection). Still, nothing enforces UUID shape at this layer — escape defensively so a malformed/legacy ID can't produce a broken or spoofedhref.🛡️ Proposed fix
+ "net/url" ... if targetDesc, ok := registry.Get(ref.TargetKind); ok { - href := fmt.Sprintf("/api/hyperfleet/v1/%s/%s", targetDesc.Plural, ref.TargetID) + href := fmt.Sprintf("/api/hyperfleet/v1/%s/%s", targetDesc.Plural, url.PathEscape(ref.TargetID)) objRef.Href = &href }🤖 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/presenters/resource.go` around lines 116 - 131, The presentResourceReferences helper builds Href values from ref.TargetID without escaping, so update the URL construction to path-escape the target ID before interpolating it. Use the existing presentResourceReferences function and registry.Get lookup to locate the href creation, and ensure the generated openapi.ObjectReference.Href always contains a safely encoded path segment even for malformed or legacy IDs.pkg/services/types.go (1)
67-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
NewListArgumentsnow exceeds the 50-line/branching-path guidance.The function body (lines 67-147) has grown past the QUAL-03 threshold with the added ref-pairing block. Extract page/size/order/ref validation into helper functions for testability and cognitive-load reduction.
As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
🤖 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/services/types.go` around lines 67 - 147, `NewListArguments` in `types.go` is now too large and branchy, so split the parsing/validation into smaller helpers. Extract the page/size validation, order parsing, and ref_type/ref_target_id pairing check into dedicated functions (or methods) that `NewListArguments` can call. Keep the main constructor focused on assembling `ListArguments`, and make the new helpers reusable and easier to test.Source: Path instructions
pkg/handlers/root_resource_handler.go (1)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reference-extraction logic across handlers.
Per the AI summary,
ResourceHandler.Createextractsreq.Referencesinto arefsmap using the identical pattern. Extract a small shared helper (e.g.extractReferences(refs *map[string][]openapi.ObjectReference) map[string][]openapi.ObjectReference) to avoid drift between the two call sites as the reference contract evolves.♻️ Proposed helper extraction
+func extractReferences(refs *map[string][]openapi.ObjectReference) map[string][]openapi.ObjectReference { + if refs == nil { + return nil + } + return *refs +} + func (h *RootResourceHandler) Create(w http.ResponseWriter, r *http.Request) { ... - var refs map[string][]openapi.ObjectReference - if req.References != nil { - refs = *req.References - } - resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource, refs) + resource, svcErr := h.service.Create(r.Context(), descriptor.Kind, resource, extractReferences(req.References))🤖 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/handlers/root_resource_handler.go` around lines 100 - 104, The reference extraction in ResourceHandler.Create is duplicated and should be centralized to prevent the two call sites from drifting. Add a small shared helper such as extractReferences that takes the req.References pointer and returns the map, then use it in ResourceHandler.Create and the other handler that currently repeats the same nil-check-and-deref pattern. Keep the helper near the handler code so the reference contract stays consistent and easy to update.pkg/services/resource_test.go (2)
187-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock can't exercise
ClearTargetReferences/FindSourceIDsByReferror branches.Lines 200-206 hardcode success for both methods with no injectable error field (unlike
replaceRefsErrat Line 52). This makeserrors.GeneralError("failed to clear references...")(ForceDelete) anderrors.GeneralError("failed to query references...")(applyRefFilter) untestable here.As per path instructions, "Error paths SHOULD be tested, not just happy paths."
🤖 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/services/resource_test.go` around lines 187 - 207, The mockResourceDao currently forces ClearTargetReferences and FindSourceIDsByRef to always succeed, so their error branches in ForceDelete and applyRefFilter cannot be tested. Add injectable error fields to mockResourceDao similar to replaceRefsErr, then update ClearTargetReferences and FindSourceIDsByRef to return those errors when set; use these symbols to wire the tests so the GeneralError paths for failed reference clearing and reference lookup can be exercised.Source: Path instructions
1645-1733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing error-path coverage: duplicate target ID and deleted-target rejection.
validateReferences(pkg/services/resource.go Lines 661-680) has two untested branches: duplicate target ID within a ref type (Line 662) and target marked for deletion (Line 676). Only missing-required, exceeds-max, valid-persist, and target-not-found are covered here.As per path instructions, "Error paths SHOULD be tested, not just happy paths."
🤖 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/services/resource_test.go` around lines 1645 - 1733, The resource service reference validation has two untested error paths in validateReferences: duplicate target IDs within the same ref type and rejecting targets marked for deletion. Add focused tests alongside the existing Create tests in TestResourceService_Create_* using the same setupRefDescriptors, newTestResourceService, and mockDao helpers; one test should pass duplicate ObjectReference entries for the same "dep" target ID and assert the 400 with a duplicate-related reason, and another should create a target resource flagged as deleted/marked for deletion and assert Create returns 400 with a deleted/not-allowed reason. Keep the assertions aligned with validateReferences behavior so these branches are covered.Source: Path instructions
pkg/services/resource.go (2)
191-206: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant referencer check duplicated between
DeleteanddeleteResourceTree.Lines 194-205 check
FindReferencersonresource.IDwhenDeletedTime == nil, thenDeleteimmediately callsdeleteResourceTree(ctx, resource, ...)which unconditionally re-runs the identicalFindReferencerscheck (Lines 260-269) on the sameresource.ID, with the sameConflictStatemessage duplicated verbatim. On every first-time delete this is one extra indexed-scan query and duplicated error text to maintain in two places.Proposed fix
- // Reject first-time deletion if other resources still reference this one. - // Skip on re-evaluation (already soft-deleted) so the re-delete path can - // re-assess whether to hard-delete after children are finalized. - if resource.DeletedTime == nil { - referencers, refErr := s.resourceDao.FindReferencers(ctx, resource.ID) - if refErr != nil { - return nil, errors.GeneralError("failed to check references: %s", refErr) - } - if len(referencers) > 0 { - return nil, errors.ConflictState( - "cannot delete %s %q: referenced by %s %q — remove the reference before deleting", - kind, id, referencers[0].Kind, referencers[0].Name, - ) - } - } - deletedBy := actorFromContext(ctx)
deleteResourceTreealready performs this exact check unconditionally for the root resource before making the soft/hard delete decision — the block above is dead weight.🤖 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/services/resource.go` around lines 191 - 206, The referencer validation is duplicated between Delete and deleteResourceTree, causing an unnecessary extra FindReferencers call and duplicated ConflictState text. Remove the pre-check in Delete around the resource.DeletedTime nil branch and let deleteResourceTree handle the root referencer check and error reporting for resource.ID. Keep the existing soft-delete/re-evaluation flow intact, but ensure only one path owns this validation and message formatting.
609-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ClearTargetReferencessilently strips references without logging count or impact.Force-delete removes all inbound
resource_referencesrows pointing at this target (FK RESTRICT workaround), but referencing resources withMin>0on that ref type are never re-validated or flagged — they silently end up violating their own required-reference invariant with no audit trail (CWE-778, insufficient logging) of what was severed or how many rows were affected.Proposed fix
- // Clear inbound references before hard-deleting (FK uses ON DELETE RESTRICT). - if err := s.resourceDao.ClearTargetReferences(ctx, resource.ID); err != nil { - return errors.GeneralError("failed to clear references: %s", err) - } + // Clear inbound references before hard-deleting (FK uses ON DELETE RESTRICT). + // NOTE: referencing resources with Min>0 on this ref type are left invariant-violating. + if err := s.resourceDao.ClearTargetReferences(ctx, resource.ID); err != nil { + return errors.GeneralError("failed to clear references: %s", err) + } + logger.With(ctx, "resource_kind", resource.Kind, "resource_id", resource.ID). + Info("Cleared inbound resource_references before force-delete")🤖 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/services/resource.go` around lines 609 - 619, The force-delete flow in the delete path clears inbound references without any audit or validation, so update the logic around ClearTargetReferences in the resource delete routine to log what was removed and how many references were affected. Capture the cleared reference count and the affected target/resource context before calling resourceDao.Delete, and if any cleared references belong to required ref types, re-validate or explicitly flag that impact so the delete path does not silently break invariants. Keep the change localized to the delete flow and the ClearTargetReferences call site so the behavior is easy to trace.pkg/db/migrations/202607021925_add_resource_references.go (1)
8-33: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd
Rollbackand make index creation idempotent.No
Rollbackis defined on thisgormigrate.Migration, so this migration can't be reverted — straightforward here since it's a single new table. Also,CREATE INDEXlacksIF NOT EXISTSwhile the table creation guards against re-run; if the migration partially fails after table creation (and isn't wrapped in one transaction), a retry will error on the index statement.As per path instructions, "Includes rollback migration when possible" for
**/migrations/**.🔧 Proposed fix
func addResourceReferences() *gormigrate.Migration { return &gormigrate.Migration{ ID: "202607021925", Migrate: func(tx *gorm.DB) error { if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_references ( source_id VARCHAR(255) NOT NULL, ref_type VARCHAR(255) NOT NULL, target_id VARCHAR(255) NOT NULL, target_kind VARCHAR(100) NOT NULL, PRIMARY KEY (source_id, ref_type, target_id), FOREIGN KEY (source_id) REFERENCES resources(id) ON DELETE CASCADE, FOREIGN KEY (target_id) REFERENCES resources(id) ON DELETE RESTRICT );`).Error; err != nil { return err } if err := tx.Exec( - `CREATE INDEX idx_resource_references_target ON resource_references (target_id);`, + `CREATE INDEX IF NOT EXISTS idx_resource_references_target ON resource_references (target_id);`, ).Error; err != nil { return err } return nil }, + Rollback: func(tx *gorm.DB) error { + return tx.Exec(`DROP TABLE IF EXISTS resource_references;`).Error + }, } }🤖 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/db/migrations/202607021925_add_resource_references.go` around lines 8 - 33, The addResourceReferences() migration is missing a Rollback and the index creation is not safe to re-run. Update the gormigrate.Migration returned by addResourceReferences() to include a Rollback that drops resource_references, and make the CREATE INDEX statement idempotent by using an if-not-exists form so retries after partial failures won’t error. Use the existing addResourceReferences and gormigrate.Migration definitions to place both changes in the same migration.Source: Path instructions
🤖 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 `@test/integration/resource_references_test.go`:
- Around line 1-427: Add test coverage for the missing error paths in the
resource reference suite: extend resource_references_test.go with a create
scenario in TestResourceReferences_... that uses RefSource and makeRefs to
submit two identical ObjectReference entries under the same ref type and assert
the service rejects it with 400, and add another create scenario that points
RefSource at a RefTarget after it has been soft-deleted (or otherwise marked
deleted) and assert rejection with the appropriate error code. Use the existing
setupRefTest, newRefTestResource, makeRefs, svc.Create, and
svc.Delete/soft-delete helpers so the new cases match the current integration
patterns.
---
Nitpick comments:
In `@pkg/api/presenters/resource.go`:
- Around line 116-131: The presentResourceReferences helper builds Href values
from ref.TargetID without escaping, so update the URL construction to
path-escape the target ID before interpolating it. Use the existing
presentResourceReferences function and registry.Get lookup to locate the href
creation, and ensure the generated openapi.ObjectReference.Href always contains
a safely encoded path segment even for malformed or legacy IDs.
In `@pkg/db/migrations/202607021925_add_resource_references.go`:
- Around line 8-33: The addResourceReferences() migration is missing a Rollback
and the index creation is not safe to re-run. Update the gormigrate.Migration
returned by addResourceReferences() to include a Rollback that drops
resource_references, and make the CREATE INDEX statement idempotent by using an
if-not-exists form so retries after partial failures won’t error. Use the
existing addResourceReferences and gormigrate.Migration definitions to place
both changes in the same migration.
In `@pkg/handlers/root_resource_handler.go`:
- Around line 100-104: The reference extraction in ResourceHandler.Create is
duplicated and should be centralized to prevent the two call sites from
drifting. Add a small shared helper such as extractReferences that takes the
req.References pointer and returns the map, then use it in
ResourceHandler.Create and the other handler that currently repeats the same
nil-check-and-deref pattern. Keep the helper near the handler code so the
reference contract stays consistent and easy to update.
In `@pkg/services/resource_test.go`:
- Around line 187-207: The mockResourceDao currently forces
ClearTargetReferences and FindSourceIDsByRef to always succeed, so their error
branches in ForceDelete and applyRefFilter cannot be tested. Add injectable
error fields to mockResourceDao similar to replaceRefsErr, then update
ClearTargetReferences and FindSourceIDsByRef to return those errors when set;
use these symbols to wire the tests so the GeneralError paths for failed
reference clearing and reference lookup can be exercised.
- Around line 1645-1733: The resource service reference validation has two
untested error paths in validateReferences: duplicate target IDs within the same
ref type and rejecting targets marked for deletion. Add focused tests alongside
the existing Create tests in TestResourceService_Create_* using the same
setupRefDescriptors, newTestResourceService, and mockDao helpers; one test
should pass duplicate ObjectReference entries for the same "dep" target ID and
assert the 400 with a duplicate-related reason, and another should create a
target resource flagged as deleted/marked for deletion and assert Create returns
400 with a deleted/not-allowed reason. Keep the assertions aligned with
validateReferences behavior so these branches are covered.
In `@pkg/services/resource.go`:
- Around line 191-206: The referencer validation is duplicated between Delete
and deleteResourceTree, causing an unnecessary extra FindReferencers call and
duplicated ConflictState text. Remove the pre-check in Delete around the
resource.DeletedTime nil branch and let deleteResourceTree handle the root
referencer check and error reporting for resource.ID. Keep the existing
soft-delete/re-evaluation flow intact, but ensure only one path owns this
validation and message formatting.
- Around line 609-619: The force-delete flow in the delete path clears inbound
references without any audit or validation, so update the logic around
ClearTargetReferences in the resource delete routine to log what was removed and
how many references were affected. Capture the cleared reference count and the
affected target/resource context before calling resourceDao.Delete, and if any
cleared references belong to required ref types, re-validate or explicitly flag
that impact so the delete path does not silently break invariants. Keep the
change localized to the delete flow and the ClearTargetReferences call site so
the behavior is easy to trace.
In `@pkg/services/types.go`:
- Around line 67-147: `NewListArguments` in `types.go` is now too large and
branchy, so split the parsing/validation into smaller helpers. Extract the
page/size validation, order parsing, and ref_type/ref_target_id pairing check
into dedicated functions (or methods) that `NewListArguments` can call. Keep the
main constructor focused on assembling `ListArguments`, and make the new helpers
reusable and easier to test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f3d9ba1e-43c0-43d1-984e-9fcca6ce3e9c
📒 Files selected for processing (23)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_reference.gopkg/dao/mocks/resource.gopkg/dao/resource.gopkg/db/migrations/202607021925_add_resource_references.gopkg/db/migrations/migration_structs.gopkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/registry/registry.gopkg/services/resource.gopkg/services/resource_test.gopkg/services/types.gopkg/services/types_test.gotest/integration/channels_test.gotest/integration/resource_conditions_test.gotest/integration/resource_delete_test.gotest/integration/resource_force_delete_test.gotest/integration/resource_references_test.gotest/integration/root_resources_test.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
kuudori
force-pushed
the
HYPERFLEET-1156-feat-resource-references
branch
from
a7f3a0d to
aa4d661
Compare
…ations) Add a resource_references table with composite PK (source_id, ref_type, target_id) and FK constraints (CASCADE on source, RESTRICT on target). Supports CRUD: - Create/Patch persist references with validation (min/max, target existence, duplicates) - Delete blocks when target is referenced (409), force-delete clears inbound refs - List supports ref_type + ref_target_id query params for filtering by reference - Presenter renders references as grouped ObjectReference map with href links
- Reuse convertRefs result in Patch (eliminate duplicate call) - Add Preload Conditions+References to GetForUpdate (fixes Patch response dropping existing refs/conditions) - Add Conditions+References preloads to ListAll for parity - CREATE INDEX IF NOT EXISTS in migration - Add Rollback to migration - Fix doc comment 422 -> 400 - Extract shared extractReferences helper from handlers - Add logging to ClearTargetReferences in forceDeleteResourceTree - Replace raw SQL with GORM query builder in FindReferencers - Add unit tests for duplicate ref target ID and soft-deleted target rejection - Add integration tests for duplicate refs and soft-deleted targets
kuudori
force-pushed
the
HYPERFLEET-1156-feat-resource-references
branch
from
aa4d661 to
2c4b172
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/handlers/root_resource_handler.go (1)
107-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that references reach the service layer.
The current handler tests match the new argument with
gomock.Any(), so they would still pass if this change regressed to passingnil. Add a create case with non-emptyreferencesand assert the exact map passed toResourceService.Create.As per path instructions, critical logic paths should have tests and error paths should be covered.
🤖 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/handlers/root_resource_handler.go` around lines 107 - 108, Update the root resource handler tests for the Create call to include a case with non-empty references, and configure the mock expectation to assert the exact references map rather than gomock.Any(). Ensure the test verifies extractReferences output reaches ResourceService.Create and add coverage for the relevant error path.Source: Path instructions
🤖 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 `@test/integration/resource_status_test.go`:
- Around line 107-109: The test currently only verifies that conditions exist,
without validating the synthesized aggregation result. In the resource status
test around svc.Get, assert the expected condition types, statuses, and
generations produced from the submitted adapter report, using precise
expectations for the aggregated conditions instead of only BeEmpty().
- Line 96: Check the error returned by json.Marshal in all three occurrences
within the affected tests, including the calls near lines 96, 152, and 197.
Handle marshal failures consistently with the existing pattern near line 53,
failing the test immediately instead of continuing with an invalid request body.
- Around line 187-205: The future-generation PUT test only checks for a 204
response and must also verify persistence behavior. After the request in the
future-generation case, issue a GET for the channel statuses and assert that no
status entry or derived condition was added, using the existing status retrieval
helpers and authentication setup in the test.
- Around line 120-125: The non-existent resource test only checks the HTTP
status and must validate the RFC 9457 response body. In the GET request
assertion block for `channelsPath + "/nonexistent-id/statuses"`, unmarshal the
response into the project’s problem-details type and assert the required
`status`, `code`, `detail`, and `traceId` fields.
---
Nitpick comments:
In `@pkg/handlers/root_resource_handler.go`:
- Around line 107-108: Update the root resource handler tests for the Create
call to include a case with non-empty references, and configure the mock
expectation to assert the exact references map rather than gomock.Any(). Ensure
the test verifies extractReferences output reaches ResourceService.Create and
add coverage for the relevant error path.
🪄 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: CHILL
Plan: Enterprise
Run ID: 8ed4405e-e520-4413-85ec-f0dff225e653
📒 Files selected for processing (24)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_reference.gopkg/dao/mocks/resource.gopkg/dao/resource.gopkg/db/migrations/202607021925_add_resource_references.gopkg/db/migrations/migration_structs.gopkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/registry/registry.gopkg/services/resource.gopkg/services/resource_test.gopkg/services/types.gopkg/services/types_test.gotest/integration/channels_test.gotest/integration/resource_conditions_test.gotest/integration/resource_delete_test.gotest/integration/resource_force_delete_test.gotest/integration/resource_references_test.gotest/integration/resource_status_test.gotest/integration/root_resources_test.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (3)
- pkg/handlers/resource_handler_test.go
- pkg/registry/registry.go
- pkg/services/types_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/api/resource.go
- test/integration/resource_force_delete_test.go
- pkg/api/resource_reference.go
- pkg/db/migrations/202607021925_add_resource_references.go
- pkg/dao/mocks/resource.go
- pkg/services/types.go
- pkg/handlers/resource_handler.go
- pkg/api/presenters/resource_test.go
- test/integration/resource_conditions_test.go
- test/integration/root_resources_test.go
- pkg/db/migrations/migration_structs.go
- test/integration/resource_delete_test.go
- pkg/api/presenters/resource.go
- test/integration/channels_test.go
- test/integration/versions_test.go
- pkg/dao/resource.go
- test/integration/resource_references_test.go
- pkg/services/resource_test.go
- pkg/services/resource.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/handlers/root_resource_handler.go (1)
107-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that references reach the service layer.
The current handler tests match the new argument with
gomock.Any(), so they would still pass if this change regressed to passingnil. Add a create case with non-emptyreferencesand assert the exact map passed toResourceService.Create.As per path instructions, critical logic paths should have tests and error paths should be covered.
🤖 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/handlers/root_resource_handler.go` around lines 107 - 108, Update the root resource handler tests for the Create call to include a case with non-empty references, and configure the mock expectation to assert the exact references map rather than gomock.Any(). Ensure the test verifies extractReferences output reaches ResourceService.Create and add coverage for the relevant error path.Source: Path instructions
🤖 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 `@test/integration/resource_status_test.go`:
- Around line 107-109: The test currently only verifies that conditions exist,
without validating the synthesized aggregation result. In the resource status
test around svc.Get, assert the expected condition types, statuses, and
generations produced from the submitted adapter report, using precise
expectations for the aggregated conditions instead of only BeEmpty().
- Line 96: Check the error returned by json.Marshal in all three occurrences
within the affected tests, including the calls near lines 96, 152, and 197.
Handle marshal failures consistently with the existing pattern near line 53,
failing the test immediately instead of continuing with an invalid request body.
- Around line 187-205: The future-generation PUT test only checks for a 204
response and must also verify persistence behavior. After the request in the
future-generation case, issue a GET for the channel statuses and assert that no
status entry or derived condition was added, using the existing status retrieval
helpers and authentication setup in the test.
- Around line 120-125: The non-existent resource test only checks the HTTP
status and must validate the RFC 9457 response body. In the GET request
assertion block for `channelsPath + "/nonexistent-id/statuses"`, unmarshal the
response into the project’s problem-details type and assert the required
`status`, `code`, `detail`, and `traceId` fields.
---
Nitpick comments:
In `@pkg/handlers/root_resource_handler.go`:
- Around line 107-108: Update the root resource handler tests for the Create
call to include a case with non-empty references, and configure the mock
expectation to assert the exact references map rather than gomock.Any(). Ensure
the test verifies extractReferences output reaches ResourceService.Create and
add coverage for the relevant error path.
🪄 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: CHILL
Plan: Enterprise
Run ID: 8ed4405e-e520-4413-85ec-f0dff225e653
📒 Files selected for processing (24)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_reference.gopkg/dao/mocks/resource.gopkg/dao/resource.gopkg/db/migrations/202607021925_add_resource_references.gopkg/db/migrations/migration_structs.gopkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/registry/registry.gopkg/services/resource.gopkg/services/resource_test.gopkg/services/types.gopkg/services/types_test.gotest/integration/channels_test.gotest/integration/resource_conditions_test.gotest/integration/resource_delete_test.gotest/integration/resource_force_delete_test.gotest/integration/resource_references_test.gotest/integration/resource_status_test.gotest/integration/root_resources_test.gotest/integration/versions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (3)
- pkg/handlers/resource_handler_test.go
- pkg/registry/registry.go
- pkg/services/types_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/api/resource.go
- test/integration/resource_force_delete_test.go
- pkg/api/resource_reference.go
- pkg/db/migrations/202607021925_add_resource_references.go
- pkg/dao/mocks/resource.go
- pkg/services/types.go
- pkg/handlers/resource_handler.go
- pkg/api/presenters/resource_test.go
- test/integration/resource_conditions_test.go
- test/integration/root_resources_test.go
- pkg/db/migrations/migration_structs.go
- test/integration/resource_delete_test.go
- pkg/api/presenters/resource.go
- test/integration/channels_test.go
- test/integration/versions_test.go
- pkg/dao/resource.go
- test/integration/resource_references_test.go
- pkg/services/resource_test.go
- pkg/services/resource.go
🛑 Comments failed to post (4)
test/integration/resource_status_test.go (4)
96-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not discard
json.Marshalerrors.These calls violate CWE-252/CWE-390: a serialization failure is ignored, allowing the test to continue with an invalid request body and produce a misleading HTTP failure. Handle the error as done at Line 53.
As per path instructions, every error return MUST be checked.
Proposed fix
- body, _ := json.Marshal(statusReq) + body, err := json.Marshal(statusReq) + Expect(err).NotTo(HaveOccurred())Apply this at all three occurrences.
Also applies to: 152-152, 197-197
🤖 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 `@test/integration/resource_status_test.go` at line 96, Check the error returned by json.Marshal in all three occurrences within the affected tests, including the calls near lines 96, 152, and 197. Handle marshal failures consistently with the existing pattern near line 53, failing the test immediately instead of continuing with an invalid request body.Source: Path instructions
107-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the actual aggregated conditions.
NotTo(BeEmpty())passes for any condition, so it will not detect missing or incorrect synthesized condition types, statuses, or generations. Assert the expected aggregation output for the submitted adapter report.As per path instructions, critical logic paths should have meaningful assertions.
🤖 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 `@test/integration/resource_status_test.go` around lines 107 - 109, The test currently only verifies that conditions exist, without validating the synthesized aggregation result. In the resource status test around svc.Get, assert the expected condition types, statuses, and generations produced from the submitted adapter report, using precise expectations for the aggregated conditions instead of only BeEmpty().Source: Path instructions
120-125: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the RFC 9457 error response, not only its status.
A malformed or nonconforming 404 body will still pass this test. Unmarshal the response and assert the required Problem Details fields, including
status,code,detail, andtraceId.As per path instructions, error paths should be tested, not only their HTTP status codes.
🤖 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 `@test/integration/resource_status_test.go` around lines 120 - 125, The non-existent resource test only checks the HTTP status and must validate the RFC 9457 response body. In the GET request assertion block for `channelsPath + "/nonexistent-id/statuses"`, unmarshal the response into the project’s problem-details type and assert the required `status`, `code`, `detail`, and `traceId` fields.Source: Path instructions
187-205: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Verify that discarded updates are not persisted.
A 204 response alone does not prove the future-generation report was discarded; a buggy handler could persist it and still return 204. Follow the request with GET and assert that no status or derived condition was added.
As per path instructions, critical logic paths should be tested beyond status-only assertions.
🤖 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 `@test/integration/resource_status_test.go` around lines 187 - 205, The future-generation PUT test only checks for a 204 response and must also verify persistence behavior. After the request in the future-generation case, issue a GET for the channel statuses and assert that no status entry or derived condition was added, using the existing status retrieval helpers and authentication setup in the test.Source: Path instructions
1 participant
Summary
resource_referencestable with composite PK(source_id, ref_type, target_id)and FK constraints (CASCADE on source delete, RESTRICT on target delete)ref_type+ref_target_idquery parameters for filtering List results by reference associationObjectReferencemap withhreflinks resolved from the entity registryTest plan
make verify-allpasses (vet + fmt + lint + unit tests)make test-integrationpasses with testcontainers PostgreSQL