HYPERFLEET-1160 - feat: separate resource_labels table for generic resource labels#270
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (5)
📝 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 | 896 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/dao 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?
There was a problem hiding this comment.
My opinion is no...
We don't have a use case for it yet and I don't want to restrict users preemptively. Request body size is already bounded by the API layer. If it becomes a real problem we can add a cap in one line 🙂
| 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.
There was a problem hiding this comment.
that's a really good catch, added IN support. =, !=, and IN all work now.
other TSL (like, between, ...) I believe it's less common.
| } | ||
|
|
||
| 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.
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/db/sql_helpers_test.go`:
- Around line 1095-1102: Update the remaining-tree assertions in the relevant
sql helper test to verify that subtreeHasMatch(remaining, isLabelIdentifier) is
false, rather than only checking remaining.Left is non-nil; also add a separate
assertion confirming the expected non-label predicate remains.
- Around line 1088-1093: Update the table-driven tests around labels[0].ToSql()
to define expected bound arguments for each case, including all values and IN
members, then retain the returned arguments and assert they equal the case
expectations alongside the SQL assertion. Use the existing test case structure
and ToSql() call to verify both SQL shape and argument ordering.
🪄 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: a254d857-f974-478b-a292-2e7224e89cca
📒 Files selected for processing (5)
pkg/api/resource_label_test.gopkg/db/sql_helpers.gopkg/db/sql_helpers_test.gotest/integration/channels_test.gotest/integration/resource_delete_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 (1)
- pkg/db/sql_helpers.go
- Add unit tests for ValidateLabel boundary conditions - Add unit tests for ExtractLabelQueries (EQ, NE, IN, combined, errors) - Add IN operator support to labelsNodeConverter - Document operator scope in labelsNodeConverter - Fix TestExistsSoftDeletedByOwner to call production DAO - Add LabelReplace integration test for re-patch coverage
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
from
b9f113d to
1e01f9f
Compare
| @@ -215,7 +234,9 @@ | |||
|
|
|||
| func newTestResourceService(mockDao *mockResourceDao) (ResourceService, *mockResourceDao, *resourceGenericMock) { | |||
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Standards
newTestResourceService/newTestResourceServiceWithAdapterStatus build a mockResourceLabelDao but never return it, so nothing in this file can assert against it. The mock already has a
replaceErr injection field that's never set — worth wiring it up:
func newTestResourceService(mockDao *mockResourceDao) (ResourceService, *mockResourceDao, *resourceGenericMock, *mockResourceLabelDao) {
// ... existing setup ...
labelDao := newMockResourceLabelDao()
svc := NewResourceService(mockDao, adapterStatusDao, conditionDao, labelDao, generic)
return svc, mockDao, generic, labelDao
}Then add cases like TestResourceService_Create_LabelDaoError (set replaceErr, assert it surfaces) and TestResourceService_Patch_SpecOnlyChange_SkipsReplaceLabels (assert labelDao.labels
untouched when only spec changed).
| }, | ||
| { | ||
| name: "NOT wrapping label query returns error", | ||
| searchQuery: "NOT (labels.env='prod')", |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Standards
The comment above the isLabelIdentifier post-check explains that not labels.env='x' (unparenthesized) parses as (NOT labels.env) = 'x' due to TSL precedence, so hasLabel never catches it —
that's exactly the case this table doesn't test. The only NOT case here ("NOT (labels.env='prod')") is parenthesized and hits a different branch entirely.
{
name: "unparenthesized NOT before label survives precedence and is rejected",
searchQuery: "not labels.env='prod'",
expectError: true,
},| }) | ||
| } | ||
| } | ||
| func TestExtractLabelQueries(t *testing.T) { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Standards
The condition-query tests above cover OR-rejection ("OR with condition on right side is rejected", etc.), but TestExtractLabelQueries has no equivalent case for the label-specific OR message.
Worth adding one so a regression that silently let OR through with a label filter would actually get caught:
{
name: "OR with label query returns error",
searchQuery: "labels.env='prod' OR name='foo'",
expectError: true,
},| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" | ||
| ) | ||
|
|
||
| func TestConvertResource(t *testing.T) { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Standards
ConvertResource now returns "invalid labels: %w" when a label fails ValidateLabel, but nothing here exercises that path. Worth adding:
func TestConvertResource_InvalidLabel(t *testing.T) {
req := &openapi.ResourceCreateRequest{
Kind: "Channel",
Name: "test",
Labels: &map[string]string{
"": "value", // empty key fails ValidateLabel
},
}
_, err := ConvertResource(req)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("invalid labels"))
}| Expect(result.Generation).To(Equal(int32(2))) | ||
| } | ||
|
|
||
| func TestResourceService_Patch_LabelsChanged_IncrementsGeneration(t *testing.T) { |
There was a problem hiding this comment.
Should we add a test case that patches with an overlong label key/value? Wdyt?
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 via EXISTS subqueries againstresource_labelstable (=, !=, and IN operators)Why
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/ListByLabel— label search via EXISTS subquery verified in integration testsTicket
https://redhat-internal.atlassian.net/browse/HYPERFLEET-1160