HYPERFLEET-1160 - feat: separate resource_labels table for generic resource labels#270
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResource labels move from a JSONB field to a 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 | 646 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
from
c832e6b to
6ffe8d7
Compare
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
6ffe8d7 to
665e255
Compare
|
@kuudori: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
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