refactor(store): wave E.4 — devices.labels JSONB → device_labels child table (#297)#298
Merged
Merged
Conversation
…d table (#297) Final piece of Wave E (tracker #242). After this lands, no JSONB operator (->>, ->, @>, jsonb_*) remains in any sqlc query. Schema (migration 047): - New device_labels(device_id FK CASCADE, key, value, PK (device_id, key)) + idx_device_labels_key_value for the label-filter path. - Backfill from devices_projection.labels via jsonb_each_text. - Re-create the dynamic-group queue trigger against device_labels (INSERT/UPDATE/DELETE) — preserves the "re-evaluate on label change" behaviour the PL/pgSQL trigger had on the JSONB column. - ALTER TABLE devices_projection DROP COLUMN labels. Queries: - UpsertDeviceProjection no longer writes labels — initial-labels go to the child table via SetDeviceLabel in the listener. - Replace SetDeviceLabelKey / RemoveDeviceLabelKey / UpdateDeviceLabelsProjection with SetDeviceLabel (ON CONFLICT DO UPDATE) / RemoveDeviceLabel / ClearDeviceLabels. - AdvanceDeviceProjectionVersion provides the per-event stale-replay guard the listener uses around child-table writes (same pattern as Wave E.3's TouchUserUpdatedAt). - GetDevicesWithLabel rewritten as a JOIN against device_labels. - ListDevicesForDynamicEvaluation strips the labels column; ListDeviceLabels / ListDeviceLabelsBatch / ListAllDeviceLabels are the new label-load entry points. Domain types: - store.Device.Labels is map[string]string. Postgres repo populates it via ListDeviceLabelsBatch in List/ListOnline/ListOffline and per- device ListDeviceLabels in Get. - DeviceRegisteredPayload / DeviceLabelsUpdatedPayload carry typed map[string]string. New decodeLabelsMap helper coerces the JSON payload bytes into the parallel map (matching PL/pgSQL ::TEXT coercion for non-string values). - DeviceLabelsUpdatedPayload grows HasLabels so the listener can distinguish "labels key absent on the wire (preserve)" from "labels key present and empty (clear all)" — replaces the SQL COALESCE(payload, labels) shape. Consumers: - internal/dyngroupeval batch-loads labels via ListAllDeviceLabels and threads the map[string]string per device through DeviceContext. - internal/search.FlattenLabels takes the typed map directly; warm paths in api/search_listener.go + search/index.go updated. - api/device_handler.userToProto drops the json.Unmarshal block and copies the map directly. Tests: - internal/projectors/device_test.go assertions converted to ListDeviceLabels rows + a labelRowsAsMap helper. Refs #242.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors device labels from JSONB column storage to a normalized child table, introducing separate label read/write queries, updating the domain model to use typed maps, redecorating event payloads, batch-loading labels in the store repository, pre-loading labels in the evaluator, and updating all API consumers. ChangesDevice Labels Normalization
Sequence DiagramsequenceDiagram
participant Event as Device Event
participant Projector as Event Projector
participant AdvanceVersion as Version Guard
participant Queries as SQL Queries
participant Repo as Store Repository
participant Consumer as API Consumer
Event->>Projector: DeviceRegistered/LabelsUpdated (seqNum, labels?)
Projector->>Projector: decodeLabelsMap(rawJSON)
alt DeviceLabelsUpdated with labels
Projector->>AdvanceVersion: advanceDeviceVersion(seqNum)
AdvanceVersion-->>Projector: success/stale
alt version bump succeeds
Projector->>Queries: ClearDeviceLabels(device_id)
Projector->>Queries: SetDeviceLabel(device_id, key, value) [per key]
else stale event
Projector-->>Event: skip
end
else DeviceRegistered
Projector->>Queries: UpsertDeviceProjection
Projector->>Queries: SetDeviceLabel (per key)
end
Repo->>Queries: ListDeviceLabelsBatch(device_ids)
Queries-->>Repo: [(device_id, key, value), ...]
Repo->>Repo: attachLabels (map per device)
Repo-->>Consumer: Device{Labels: map[string]string}
Consumer->>Consumer: FlattenLabels(labels map)
Consumer-->>Consumer: "key=value key2=value2"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/dyngroupeval/evaluator.go (1)
540-553:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
parseLabelsJSONfunction.The staticcheck pipeline reports this function as unused (U1000). With the refactoring to
loadAllDeviceLabelsand typedmap[string]stringlabels, this JSON parsing helper is no longer needed.🗑️ Proposed fix: remove dead code
-func parseLabelsJSON(raw []byte) map[string]string { - if len(raw) == 0 { - return nil - } - var m map[string]any - if err := json.Unmarshal(raw, &m); err != nil { - return nil - } - out := make(map[string]string, len(m)) - for k, v := range m { - out[k] = anyToString(v) - } - return out -}🤖 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 `@internal/dyngroupeval/evaluator.go` around lines 540 - 553, Remove the dead helper function parseLabelsJSON from internal/dyngroupeval/evaluator.go since staticcheck flags it as unused (U1000) after refactoring to loadAllDeviceLabels and typed map[string]string labels; delete the entire parseLabelsJSON declaration (including its json.Unmarshal logic and any anyToString conversion usage within it) to eliminate the unused symbol from the codebase.
🧹 Nitpick comments (1)
internal/projectors/device_test.go (1)
221-229: ⚡ Quick winAdd a decoder test for explicit empty
labelsobject ({}) to pin clear-all semantics.Current coverage checks “missing key” but not “present and empty,” which is the critical branch for
HasLabels.Suggested test addition
func TestDeviceLabelsUpdatedFromEvent_Pure(t *testing.T) { @@ t.Run("missing labels => HasLabels false (preserve existing rows)", func(t *testing.T) { @@ assert.False(t, got.HasLabels) assert.Nil(t, got.Labels) }) + + t.Run("explicit empty labels object => HasLabels true (clear all)", func(t *testing.T) { + got, err := projectors.DeviceLabelsUpdatedFromEvent(store.PersistedEvent{ + StreamType: "device", StreamID: "dev-1", EventType: "DeviceLabelsUpdated", + Data: jsonOrFail(t, map[string]any{"labels": map[string]any{}}), + }) + require.NoError(t, err) + assert.True(t, got.HasLabels) + assert.Empty(t, got.Labels) + })🤖 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 `@internal/projectors/device_test.go` around lines 221 - 229, Add a new table test calling projectors.DeviceLabelsUpdatedFromEvent where the event Data includes an explicit "labels" key set to an empty object (e.g. jsonOrFail(t, map[string]any{"labels": map[string]any{}})); assert that the returned struct sets HasLabels to true and that Labels is an empty map (not nil) to pin the clear-all semantics for DeviceLabelsUpdated events. Use the same test pattern as the existing "missing labels" case and name it something like "empty labels object => HasLabels true (clear all)".
🤖 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 `@internal/projectors/device.go`:
- Around line 24-26: The code currently treats non-object JSON label payloads as
present because it only checks len(any) after json.Unmarshal, causing HasLabels
to be set for non-object values and triggering clear-all writes; update the
decode logic in internal/projectors/device.go (the block that does
json.Unmarshal(raw, &any) and the logic that sets HasLabels from raw.Labels) to
only consider labels present when the unmarshaled value is a JSON object:
unmarshal into an interface{}, assert it to map[string]interface{}, and set
HasLabels = true only if that assertion succeeds and the map has entries; apply
the same change to the second occurrence at the lines referenced (the block
around where raw.Labels is inspected).
---
Outside diff comments:
In `@internal/dyngroupeval/evaluator.go`:
- Around line 540-553: Remove the dead helper function parseLabelsJSON from
internal/dyngroupeval/evaluator.go since staticcheck flags it as unused (U1000)
after refactoring to loadAllDeviceLabels and typed map[string]string labels;
delete the entire parseLabelsJSON declaration (including its json.Unmarshal
logic and any anyToString conversion usage within it) to eliminate the unused
symbol from the codebase.
---
Nitpick comments:
In `@internal/projectors/device_test.go`:
- Around line 221-229: Add a new table test calling
projectors.DeviceLabelsUpdatedFromEvent where the event Data includes an
explicit "labels" key set to an empty object (e.g. jsonOrFail(t,
map[string]any{"labels": map[string]any{}})); assert that the returned struct
sets HasLabels to true and that Labels is an empty map (not nil) to pin the
clear-all semantics for DeviceLabelsUpdated events. Use the same test pattern as
the existing "missing labels" case and name it something like "empty labels
object => HasLabels true (clear all)".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49fed5a2-d6f6-4724-aaf4-7583edb23814
⛔ Files ignored due to path filters (3)
internal/store/generated/device_groups.sql.gois excluded by!**/generated/**internal/store/generated/devices.sql.gois excluded by!**/generated/**internal/store/generated/models.gois excluded by!**/generated/**
📒 Files selected for processing (12)
internal/api/device_handler.gointernal/api/search_listener.gointernal/dyngroupeval/evaluator.gointernal/projectors/device.gointernal/projectors/device_listener.gointernal/projectors/device_test.gointernal/search/index.gointernal/store/device.gointernal/store/migrations/047_device_labels_child_table.sqlinternal/store/postgres/device.gointernal/store/queries/device_groups.sqlinternal/store/queries/devices.sql
Comment on lines
+24
to
+26
| if err := json.Unmarshal(raw, &any); err != nil || len(any) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
HasLabels is set for non-object payloads, causing unintended clear-all writes.
At Line 24 and Line 256, non-object labels payloads decode to nil but still set HasLabels=true (via len(raw.Labels) > 0), so the listener clears existing labels instead of skipping writes.
Proposed fix
func decodeLabelsMap(raw []byte) map[string]string {
if len(raw) == 0 {
return nil
}
var any map[string]any
- if err := json.Unmarshal(raw, &any); err != nil || len(any) == 0 {
+ if err := json.Unmarshal(raw, &any); err != nil {
+ return nil
+ }
+ if any == nil {
return nil
}
out := make(map[string]string, len(any))
for k, v := range any {
switch x := v.(type) {
@@
var raw payloads.DeviceLabelsUpdated
if err := json.Unmarshal(e.Data, &raw); err != nil {
return DeviceLabelsUpdatedPayload{}, fmt.Errorf("projector: invalid DeviceLabelsUpdated payload: %w", err)
}
- if len(raw.Labels) > 0 {
- out.Labels = decodeLabelsMap(raw.Labels)
+ if labels := decodeLabelsMap(raw.Labels); labels != nil {
+ out.Labels = labels
out.HasLabels = true
}
return out, nil
}Also applies to: 256-259
🤖 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 `@internal/projectors/device.go` around lines 24 - 26, The code currently
treats non-object JSON label payloads as present because it only checks len(any)
after json.Unmarshal, causing HasLabels to be set for non-object values and
triggering clear-all writes; update the decode logic in
internal/projectors/device.go (the block that does json.Unmarshal(raw, &any) and
the logic that sets HasLabels from raw.Labels) to only consider labels present
when the unmarshaled value is a JSON object: unmarshal into an interface{},
assert it to map[string]interface{}, and set HasLabels = true only if that
assertion succeeds and the map has entries; apply the same change to the second
occurrence at the lines referenced (the block around where raw.Labels is
inspected).
CI caught two follow-ups on E.4: - TestProjection_DeviceLabelSet still asserted against the dropped devices_projection.labels column; ported to query device_labels. - internal/dyngroupeval.parseLabelsJSON became unreachable when buildDeviceContext started taking the typed map directly; staticcheck U1000 flagged it. Removed. Refs #297.
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the storage-abstraction tracker (#242), Wave E (JSONB normalization). Closes Wave E — after this lands, no JSONB operator (->>, ->, @>, jsonb_*) remains in any sqlc query.
Summary
DoD checks
After this lands
Wave E DoD met. Remaining storage-abstraction roadmap items: Wave F (reactive triggers → Go listeners) and Wave G (EventStore interface). Wave H is the validation backend (worked example: Turso/libSQL). The whole stack is now JSONB-operator-free and ready for those.
Summary by CodeRabbit
Refactor
Chores
Tests