HYPERFLEET-1160 - feat: separate resource_labels table for generic resource labels#270
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughResource labels move from a JSONB field to normalized Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceService
participant ResourceDAO
participant ResourceLabelDAO
participant Database
Client->>ResourceService: create or patch resource
ResourceService->>ResourceDAO: persist resource row
ResourceService->>ResourceLabelDAO: ReplaceLabels
ResourceLabelDAO->>Database: delete old labels
ResourceLabelDAO->>Database: insert label rows
ResourceService-->>Client: resource with Labels slice
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 643 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/api pkg/dao pkg/db pkg/db/migrations plugins/resources | +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.
Actionable comments posted: 2
🤖 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/presenters/resource.go`:
- Around line 123-132: `convertLabelsToModel` currently forwards user-supplied
label keys and values without enforcing the `ResourceLabel` size limits, so
oversized input falls through as a DB constraint error instead of a validation
error. Add boundary validation in `convertLabelsToModel` for key and value
length (matching the `pkg/api/resource_label.go` limits) and return a typed
validation error before persistence. Update `ConvertResource` to propagate the
new error return, and apply the same validation flow in `applyResourcePatch` so
both create and patch paths reject invalid labels consistently.
In `@pkg/db/migrations/202607010001_add_resource_labels.go`:
- Around line 11-31: The migration in the resource labels `Migrate` function
drops `resources.labels` without preserving existing data, and it has no
rollback path. Before the `ALTER TABLE resources DROP COLUMN IF EXISTS labels`
step, backfill `resource_labels` from the existing `resources.labels` JSONB data
in the same migration, then remove the column only after the copy succeeds. Also
add a `Rollback` for this migration that recreates `resources.labels` and
restores data from `resource_labels` using the migration’s existing symbols
(`Migrate`, `Rollback`, `resource_labels`, `resources.labels`) so rollbacks and
rolling deploys remain safe.
🪄 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: 123fe713-ae4e-4454-b5cf-0d71a78eeefd
📒 Files selected for processing (15)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_helpers.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-1160-feat-resource-labels
branch
2 times, most recently
from
32471e8 to
9f97971
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/db/migrations/202607010001_add_resource_labels.go`:
- Around line 12-24: The migration in Migrate creates resource_labels and drops
resources.labels, but it never adds the planned (key, value) index. Update the
migration to create that composite index as part of the same transaction, using
the resource_labels table definition and keeping the existing Exec error
handling consistent. Make sure the new index creation is placed near the table
setup so the schema change fully matches the PR objective.
🪄 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: 956f56ad-d3c4-497f-a25b-608e88b90765
📒 Files selected for processing (15)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_helpers.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/db/migrations/migration_structs.go
- pkg/api/resource_label.go
- pkg/db/sql_helpers.go
🚧 Files skipped from review as they are similar to previous changes (11)
- plugins/resources/plugin.go
- pkg/api/presenters/resource_test.go
- pkg/api/resource.go
- pkg/services/resource_test.go
- pkg/dao/resource.go
- test/integration/versions_test.go
- test/integration/channels_test.go
- test/integration/resource_helpers.go
- pkg/api/presenters/resource.go
- pkg/services/resource.go
- pkg/dao/resource_label.go
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
2 times, most recently
from
6640b02 to
c832e6b
Compare
|
[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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dao/resource.go (1)
21-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd integration coverage for the new raw-SQL query.
ExistsSoftDeletedByOwneruses hand-written raw SQL with slice binding (kind IN (?)). Per graph context, it's only exercised via a mockedResourceDaoinresource_test.go. A schema rename or GORM dialect quirk here would silently break parent soft-delete conflict detection without any test catching it.As per path instructions, "New exported functions and critical logic paths SHOULD have tests" (
**/*_test.gotesting standard).Also applies to: 116-128
🤖 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/dao/resource.go` at line 21, Add integration test coverage for ResourceDao.ExistsSoftDeletedByOwner, since it uses raw SQL with slice binding and is currently only covered by mocks. Create a real database-backed test that exercises the actual query path with soft-deleted and non-deleted resources across multiple kinds, and verify the result changes correctly when ownerID/kinds match. Place the test alongside the existing ResourceDao tests so the new exported method and its raw-SQL conflict-check logic are protected from schema or dialect regressions.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.
Nitpick comments:
In `@pkg/dao/resource.go`:
- Line 21: Add integration test coverage for
ResourceDao.ExistsSoftDeletedByOwner, since it uses raw SQL with slice binding
and is currently only covered by mocks. Create a real database-backed test that
exercises the actual query path with soft-deleted and non-deleted resources
across multiple kinds, and verify the result changes correctly when
ownerID/kinds match. Place the test alongside the existing ResourceDao tests so
the new exported method and its raw-SQL conflict-check logic are protected from
schema or dialect regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f9b2307e-25bc-4d99-8047-20cd8c94ff2f
📒 Files selected for processing (16)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/generic.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_helpers.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 (1)
- pkg/db/migrations/migration_structs.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/api/resource.go
- pkg/services/resource_test.go
- pkg/db/migrations/202607010001_add_resource_labels.go
- pkg/api/resource_label.go
- test/integration/channels_test.go
- test/integration/resource_helpers.go
- pkg/api/presenters/resource_test.go
- pkg/dao/resource_label.go
- pkg/services/resource.go
- pkg/api/presenters/resource.go
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
2 times, most recently
from
6ffe8d7 to
665e255
Compare
|
/test lint |
Replace JSONB labels column on resources with a dedicated resource_labels table (resource_id, key, value; composite PK, FK with ON DELETE CASCADE). - Add ResourceLabelDao with delete-all + bulk-insert ReplaceLabels - Wire label writes through ResourceService Create/Patch - Add Labels/Conditions preloads to all ResourceDao read methods - Add ExtractLabelQueries for EXISTS-subquery label filtering - Generalize extractMatchingQueries from condition-only to reusable - Restrict label filter operators to = and != (no lexicographic traps) - Remove validateLabelKey from new table path (key is parameterized) - Mark legacy JSONB label paths with TODO(HYPERFLEET-1159) - Enable ListByLabel integration test
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
from
4b41590 to
525257c
Compare
…validateJSONBKey
- GetByID DAO was missing Preload("Labels"), causing GET/PATCH/DELETE
on /resources/{id} to silently drop labels from responses
- ListAll service was not injecting Labels+Conditions preloads like
List and ListByOwner already do
- Rename validateJsonbKey -> validateJSONBKey per Go acronym convention
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
from
525257c to
cd68edd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/resource_delete_test.go (1)
194-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise
ExistsSoftDeletedByOwnerthrough the production path.These assertions reimplement the query with
dbSession.Raw(...), so they can pass even ifResourceDao.ExistsSoftDeletedByOwnerhas incorrect SQL, slice binding, or filtering. Drive the actual DAO/service method for the single-kind, multi-kind, and negative cases; retain raw SQL only as an independent database-state check.🤖 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_delete_test.go` around lines 194 - 218, Replace the direct dbSession.Raw assertions in the test with calls through the production ResourceDao.ExistsSoftDeletedByOwner (or its service wrapper) for single-kind, multi-kind, and nonexistent-owner cases, asserting each returned result and error. Retain a separate raw SQL query only as an independent verification of the seeded soft-deleted database state.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_delete_test.go`:
- Around line 35-37: Replace prerequisite Expect assertions in the resource
deletion test with fatal checks that immediately stop execution when setup or
service calls fail. Update the checks surrounding createdChannel,
createdVersion, retrievedVersion, deletedVersion, deletedChannel, and
svcErr.HTTPCode to use t.Fatalf or add explicit nil guards before dereferencing
these values.
---
Nitpick comments:
In `@test/integration/resource_delete_test.go`:
- Around line 194-218: Replace the direct dbSession.Raw assertions in the test
with calls through the production ResourceDao.ExistsSoftDeletedByOwner (or its
service wrapper) for single-kind, multi-kind, and nonexistent-owner cases,
asserting each returned result and error. Retain a separate raw SQL query only
as an independent verification of the seeded soft-deleted database 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: CHILL
Plan: Enterprise
Run ID: 5221a435-3630-4305-acaf-0997d14515c3
📒 Files selected for processing (17)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/generic.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.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 (1)
- pkg/db/migrations/migration_structs.go
🚧 Files skipped from review as they are similar to previous changes (15)
- test/integration/resource_helpers.go
- pkg/api/resource.go
- pkg/api/presenters/resource_test.go
- pkg/db/migrations/202607010001_add_resource_labels.go
- pkg/dao/resource_label.go
- pkg/api/resource_label.go
- pkg/dao/resource.go
- plugins/resources/plugin.go
- test/integration/versions_test.go
- pkg/services/resource_test.go
- pkg/services/generic.go
- test/integration/channels_test.go
- pkg/api/presenters/resource.go
- pkg/db/sql_helpers.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: 1
🧹 Nitpick comments (1)
test/integration/resource_delete_test.go (1)
194-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise
ExistsSoftDeletedByOwnerthrough the production path.These assertions reimplement the query with
dbSession.Raw(...), so they can pass even ifResourceDao.ExistsSoftDeletedByOwnerhas incorrect SQL, slice binding, or filtering. Drive the actual DAO/service method for the single-kind, multi-kind, and negative cases; retain raw SQL only as an independent database-state check.🤖 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_delete_test.go` around lines 194 - 218, Replace the direct dbSession.Raw assertions in the test with calls through the production ResourceDao.ExistsSoftDeletedByOwner (or its service wrapper) for single-kind, multi-kind, and nonexistent-owner cases, asserting each returned result and error. Retain a separate raw SQL query only as an independent verification of the seeded soft-deleted database state.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_delete_test.go`:
- Around line 35-37: Replace prerequisite Expect assertions in the resource
deletion test with fatal checks that immediately stop execution when setup or
service calls fail. Update the checks surrounding createdChannel,
createdVersion, retrievedVersion, deletedVersion, deletedChannel, and
svcErr.HTTPCode to use t.Fatalf or add explicit nil guards before dereferencing
these values.
---
Nitpick comments:
In `@test/integration/resource_delete_test.go`:
- Around line 194-218: Replace the direct dbSession.Raw assertions in the test
with calls through the production ResourceDao.ExistsSoftDeletedByOwner (or its
service wrapper) for single-kind, multi-kind, and nonexistent-owner cases,
asserting each returned result and error. Retain a separate raw SQL query only
as an independent verification of the seeded soft-deleted database 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: CHILL
Plan: Enterprise
Run ID: 5221a435-3630-4305-acaf-0997d14515c3
📒 Files selected for processing (17)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/generic.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.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 (1)
- pkg/db/migrations/migration_structs.go
🚧 Files skipped from review as they are similar to previous changes (15)
- test/integration/resource_helpers.go
- pkg/api/resource.go
- pkg/api/presenters/resource_test.go
- pkg/db/migrations/202607010001_add_resource_labels.go
- pkg/dao/resource_label.go
- pkg/api/resource_label.go
- pkg/dao/resource.go
- plugins/resources/plugin.go
- test/integration/versions_test.go
- pkg/services/resource_test.go
- pkg/services/generic.go
- test/integration/channels_test.go
- pkg/api/presenters/resource.go
- pkg/db/sql_helpers.go
- pkg/services/resource.go
🛑 Comments failed to post (1)
test/integration/resource_delete_test.go (1)
35-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file="test/integration/resource_delete_test.go" echo "== outline ==" ast-grep outline "$file" --view expanded || true echo echo "== relevant ranges ==" nl -ba "$file" | sed -n '1,120p' echo nl -ba "$file" | sed -n '220,290p'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 493
🏁 Script executed:
#!/bin/bash set -euo pipefail file="test/integration/resource_delete_test.go" echo "== lines 1-120 ==" sed -n '1,120p' "$file" | cat -n echo echo "== lines 120-280 ==" sed -n '120,280p' "$file" | cat -nRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 14125
Stop after failed prerequisite assertions (CWE-476).
Expect(...)only records a failure here, so these subtests keep running into nil dereferences oncreatedChannel,createdVersion,retrievedVersion,deletedVersion,deletedChannel, andsvcErr.HTTPCode. Switch the prerequisite checks tot.Fatalfor add explicit nil guards before each dereference intest/integration/resource_delete_test.go.🤖 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_delete_test.go` around lines 35 - 37, Replace prerequisite Expect assertions in the resource deletion test with fatal checks that immediately stop execution when setup or service calls fail. Update the checks surrounding createdChannel, createdVersion, retrievedVersion, deletedVersion, deletedChannel, and svcErr.HTTPCode to use t.Fatalf or add explicit nil guards before dereferencing these values.Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/resource_delete_test.go (1)
23-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCleanup hard-codes
nilinstead of restoring the prior descriptor value.Each
t.Cleanuphere resetsRequiredAdapterstonilrather than capturing and restoring whatever value the descriptor had before the test mutated it.registry.UpdateDescriptormutates process-global state shared with other integration tests in this cohort (channels_test.go,versions_test.go). If a future change givesVersion/Channela non-nil defaultRequiredAdapters, these tests will silently wipe it out for any test running afterward in the same binary, causing hard-to-diagnose cross-test pollution.♻️ Capture and restore original value
- registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) { - d.RequiredAdapters = []string{"test-adapter"} - }) - t.Cleanup(func() { - registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) { - d.RequiredAdapters = nil - }) - }) + original := registry.MustGet("Version").RequiredAdapters + registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) { + d.RequiredAdapters = []string{"test-adapter"} + }) + t.Cleanup(func() { + registry.UpdateDescriptor("Version", func(d *registry.EntityDescriptor) { + d.RequiredAdapters = original + }) + })Also applies to: 116-123, 178-185, 229-236
🤖 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_delete_test.go` around lines 23 - 30, Capture each descriptor’s existing RequiredAdapters value before mutating it, making a copy to avoid aliasing, and have the corresponding t.Cleanup restore that captured value instead of assigning nil. Apply this consistently to all affected registry.UpdateDescriptor blocks, including the Version and Channel descriptors in the relevant test sections.
🤖 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_delete_test.go`:
- Around line 173-220: Replace the duplicated raw SQL in
TestExistsSoftDeletedByOwner with calls to resourceDao.ExistsSoftDeletedByOwner.
Construct the resource DAO using h.DBFactory, then use it for the single-kind,
multi-kind, and nonexistent-owner assertions so the test exercises the
production DAO path.
---
Nitpick comments:
In `@test/integration/resource_delete_test.go`:
- Around line 23-30: Capture each descriptor’s existing RequiredAdapters value
before mutating it, making a copy to avoid aliasing, and have the corresponding
t.Cleanup restore that captured value instead of assigning nil. Apply this
consistently to all affected registry.UpdateDescriptor blocks, including the
Version and Channel descriptors in the relevant test sections.
🪄 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: ff53225d-4839-4ab2-b642-3fa29c01694a
📒 Files selected for processing (17)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/generic.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_delete_test.gotest/integration/resource_helpers.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 as they are similar to previous changes (16)
- plugins/resources/plugin.go
- pkg/db/migrations/migration_structs.go
- pkg/dao/resource_label.go
- pkg/api/presenters/resource_test.go
- pkg/db/migrations/202607010001_add_resource_labels.go
- test/integration/resource_helpers.go
- pkg/services/generic.go
- pkg/dao/resource.go
- pkg/api/resource.go
- test/integration/channels_test.go
- pkg/api/presenters/resource.go
- pkg/services/resource_test.go
- pkg/api/resource_label.go
- test/integration/versions_test.go
- pkg/db/sql_helpers.go
- pkg/services/resource.go
| return "resource_labels" | ||
| } | ||
|
|
||
| func ValidateLabel(key, value string) error { |
There was a problem hiding this comment.
Should we add a limit to how many labels can be added per resource? Consider a case of a huge bulk insert. Should that be a security concern?
| if n.Operator != tsl.OpEQ && n.Operator != tsl.OpNE { | ||
| return nil, errors.BadRequest( | ||
| "operator '%s' is not supported for label queries; use = or !=", n.Operator, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
labelsNodeConverter only allows =/!= for labels.xxx on Resource-kind entities:
if n.Operator != tsl.OpEQ && n.Operator != tsl.OpNE {
return nil, errors.BadRequest(
"operator '%s' is not supported for label queries; use = or !=", n.Operator,
)
}The legacy JSONB labels.xxx path (still used by Cluster/NodePool) and spec.xxx/properties.xxx never restrict operators, so they inherit the full TSL set (LIKE, IN, BETWEEN, comparisons,
regex). That means labels.region IN (...) works for a Cluster today but 400s for a Channel/Version.
| Expect(foundIDs[version3.ID]).To(BeTrue(), "should find version3") | ||
| }) | ||
|
|
||
| t.Run("ListByLabel", func(t *testing.T) { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
The PR description says label search is blocked for Resource entities pending JOIN-based query support, with TestVersionList/ListByLabel listed as skipped — but this test isn't skipped, and it
exercises exactly the EXISTS-based label search this PR adds (ExtractLabelQueries/labelsNodeConverter), asserting it works for Version. Since Channel/Version/WifConfig all resolve to resourceType == "Resource", label search is live for them today, not deferred.
Mind updating the PR description/test-plan checklist before merge so it reflects what actually shipped? Might be worth double-checking whether HYPERFLEET-1324 (linked as "Closed" — restore
labels.xxx TSL search) is meant to track this, since it looks like it landed here instead.
| } | ||
|
|
||
| func TestExistsSoftDeletedByOwner(t *testing.T) { | ||
| t.Run("DetectsSoftDeletedChild", func(t *testing.T) { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
This test hand-duplicates the DAO's SQL string and runs it via dbSession.Raw(...) rather than calling the actual method it's named after (dao.ResourceDao.ExistsSoftDeletedByOwner,
pkg/dao/resource.go:117). As written, a bug in the real method (wrong column, wrong bind order, the len(kinds)==0 early-return) would sail through this test undetected.
resourceDao := dao.NewResourceDao(h.DBFactory)
exists, err := resourceDao.ExistsSoftDeletedByOwner(t.Context(), []string{"Version"}, channel.ID)
Expect(err).To(BeNil())
Expect(exists).To(BeTrue(), "should detect soft-deleted child via the DAO method")Also worth splitting the three scenarios (single-kind, multi-kind IN, negative case) into separate t.Run subtests so a failure in the first doesn't hide whether the other two pass.
| return &sqlResourceLabelDao{sessionFactory: sessionFactory} | ||
| } | ||
|
|
||
| func (d *sqlResourceLabelDao) ReplaceLabels( |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
All the label-patch tests here start from a resource with no pre-existing labels and only add new ones — none of them re-patch an already-labeled resource with a different key set. Since
ReplaceLabels is delete-then-insert, a bug that skipped the delete (or targeted the wrong resource_id) would pass every test in this PR because there are never any stale rows to leave behind.
Worth adding one integration test like:
// patch to {a:1, b:2}, then patch again to {a:1, c:3}
// assert retrieved labels are exactly {a:1, c:3} — b is gone, a's value retained, c addedThat would give real coverage for the "replace" half of the feature, not just the "add" half.
3 participants
Summary
labels JSONBcolumn onresourcestable with dedicatedresource_labelstable (composite PK(resource_id, key), FK with CASCADE delete)ResourceLabelmodel,ResourceLabelDaowith delete+insertReplaceLabels, preload labels on all DAO read methodsReplaceLabelsexplicitly (DAO usesOmit(clause.Associations))labels.xxxTSL search for Resource entities until JOIN-based query support addedWhy
JSONB cannot enforce label key uniqueness at DB level, doesn't support efficient label-filter queries without GIN indexes, and conflates user-writable fields with system fields. Separate table enforces uniqueness via composite PK and enables indexed lookups. Per design doc §4.1-4.2.
Test plan
make test— 1314 tests)make test-integration— 237 tests, 3 skipped)golangci-lint— 0 issues)TestVersionList/ListByLabelskipped — TSL parser needs changes for JOIN-based label queries on separate tableTicket
https://redhat-internal.atlassian.net/browse/HYPERFLEET-1160