EDM-683: Normalize Device Labels into a Separate Table for Optimized Queries and Fast Distinct Label Retrieval#845
Conversation
WalkthroughThis pull request enhances device and resource management by updating the database migration and query building logic. Changes include introducing a new device label model and migrating it alongside devices, creating a GIN index and triggers for synchronizing device labels, and expanding selector functionality. The updates extend to list query configuration, error handling, and UUID support across selectors, while also revising the API specification to include device label lists. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DB as Database
participant Trigger as Sync_Trigger
App->>DB: Insert/Update device (with labels JSONB)
DB->>Trigger: Execute trigger function (sync_device_labels)
Trigger->>DB: Update device_labels table based on labels change
DB-->>App: Return acknowledgement
sequenceDiagram
participant Client
participant Store as DeviceStore
participant Resolver as SelectorResolver
participant DB as Database
Client->>Store: Call Labels(ctx, orgId, listParams)
Store->>Resolver: Resolve selectors via provided options
Resolver-->>Store: Return resolved field mappings
Store->>DB: Execute query using resolved selectors
DB-->>Store: Return query results
Store-->>Client: Return device labels
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/store/device.go(2 hunks)internal/store/model/device.go(1 hunks)internal/store/model/resource.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: e2e
- GitHub Check: smoke
- GitHub Check: integration-tests
🔇 Additional comments (3)
internal/store/device.go (3)
76-76: IncludeDeviceLabelin auto-migration.
Auto-migratingDeviceLabelalongsideDeviceensures the new table is created in sync with the existing schema. This looks good.
154-167: Validate partial matching index usage.
Creating GIN indexes withpg_trgmforlabel_keyandlabel_valueis a good approach for partial matching. Ensure your queries (e.g.,ILIKE) actually leverage these indexes.
168-211: Assess performance of JSONB-to-table trigger approach.
The trigger logic cleanly synchronizesdevice_labelswith thelabelsJSONB field. However, large or frequent JSON updates could impact performance. Consider monitoring or batching updates if the label sets grow significantly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/store/model/device.go (1)
43-51: LGTM! Consider adding a composite index for label searches.The
DeviceLabelstruct is well-designed with appropriate primary keys and indexes. The foreign key constraint with CASCADE DELETE ensures data consistency.Consider adding a composite index on
(org_id, label_key, label_value)to optimize queries that search for devices by label key-value pairs within an organization:type DeviceLabel struct { OrgID uuid.UUID `gorm:"primaryKey;type:uuid;index:,composite:device_label_org_device"` DeviceName string `gorm:"primaryKey;index:,composite:device_label_org_device"` LabelKey string `gorm:"primaryKey;index:,composite:device_label_key"` - LabelValue string `gorm:"index"` + LabelValue string `gorm:"index;index:,composite:device_label_search"` // Foreign Key Constraint with CASCADE DELETE Device Device `gorm:"foreignKey:OrgID,DeviceName;references:OrgID,Name;constraint:OnDelete:CASCADE"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/store/device.go(2 hunks)internal/store/model/device.go(2 hunks)internal/store/model/resource.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/store/model/resource.go
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: unit-tests
- GitHub Check: smoke
- GitHub Check: integration-tests
- GitHub Check: lint
- GitHub Check: e2e
🔇 Additional comments (3)
internal/store/device.go (3)
76-76: LGTM! Migration includes both models.The migration correctly includes both
DeviceandDeviceLabelmodels.
154-166: LGTM! Efficient indexing for partial matching.The implementation correctly:
- Enables the
pg_trgmextension for partial matching support- Creates a GIN index using
gin_trgm_opsfor bothlabel_keyandlabel_value
168-211: LGTM! Robust trigger-based synchronization.The trigger function implementation is thorough and efficient:
- Handles both INSERT and UPDATE operations
- Properly cleans up deleted labels during updates
- Uses UPSERT for atomic updates
- Includes condition to avoid unnecessary trigger execution
Let's verify the trigger behavior:
✅ Verification successful
Trigger implementation verified as robust and safe
The implementation is well-designed with multiple safety layers:
- Input validation ensures K8s-compliant labels
- PostgreSQL's jsonb_each_text safely handles and flattens any complex JSONB structures
- Proper cleanup and atomic updates maintain data consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify trigger function behavior for edge cases # Test: Check if trigger handles JSON arrays or nested objects rg -A 5 'labels.*jsonb.*array|labels.*jsonb.*object' internal/store/ # Test: Check if there are any existing label synchronization functions rg -A 5 'sync.*label|label.*sync' internal/store/Length of output: 1646
Script:
#!/bin/bash # Check devices table schema and constraints ast-grep --pattern 'type Device struct { $$$ Labels $_ `$$$` $$$ }' # Look for JSONB validation rg -B2 -A2 'validate.*labels|labels.*validate' internal/Length of output: 943
asafbss
changed the title
asafbss
requested review from
amir-yogev-gh,
avishayt,
hexfusion,
mangelajo and
ori-amizur
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/store/common.go (2)
71-87: Consider handling the resolver creation error explicitly.
Whenselector.SelectorFieldResolverreturns an error,q.resolversilently defaults toselector.EmptyResolver{}. This might mask underlying schema issues.You could log or track the error to help diagnose unexpected schema resolution failures.
if q.resolver == nil { resolver, err := selector.SelectorFieldResolver(q.dest) if err != nil { - q.resolver = selector.EmptyResolver{} + log.Errorf("failed to create resolver for %T: %v", q.dest, err) + q.resolver = selector.EmptyResolver{} } else { q.resolver = resolver } }
134-144: Optional logging for resolution failure.
WhenResolveFieldsreturns an error, you return the default. This is fine if the default is the intended fallback; otherwise, you may consider logging or distinguishing empty resolution vs. error.pkg/k8s/selector/selector.go (1)
232-234: LGTM! UUID validation is properly implemented.The addition of UUID validation as a fallback is well-placed, maintaining the existing validation flow while adding support for UUID fields. The error message could be more specific about UUID support.
Consider updating the error message to explicitly mention UUID support:
- allErrs = append(allErrs, field.Invalid(valuePath.Index(i), vals[i][j], "for 'Gt', 'Lt', 'Gte', and 'Lte' operators, the value must be a number or a valid time in RFC3339 format")) + allErrs = append(allErrs, field.Invalid(valuePath.Index(i), vals[i][j], "for 'Gt', 'Lt', 'Gte', and 'Lte' operators, the value must be a number, a valid time in RFC3339 format, or a valid UUID"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
internal/store/common.go(2 hunks)internal/store/device.go(5 hunks)internal/store/model/device.go(2 hunks)internal/store/model/selectors.go(1 hunks)internal/store/selector/annotation.go(2 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(10 hunks)internal/store/selector/field_test.go(9 hunks)internal/store/selector/label.go(3 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/resolvers.go(1 hunks)internal/store/selector/selector.go(5 hunks)internal/store/selector/selector_test.go(7 hunks)internal/store/selector/types.go(4 hunks)pkg/k8s/selector/selector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/store/model/device.go
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: smoke
- GitHub Check: e2e
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: integration-tests
🔇 Additional comments (61)
internal/store/common.go (6)
58-65: Use of functional options is clear and flexible.
This pattern makes it easier to customize the query behavior and injector logic. The approach is well-structured.
67-68: New field “resolver” in listQuery struct.
Adding theresolverfield aligns with the new functional options approach. Just ensure any concurrent usage oflistQueryis carefully managed since it holds mutable state.
93-95: Confirm that “metadata.orgid” is consistently spelled.
Make sure you standardize the naming, e.g., “metadata.orgid” vs. “metadata.orgId,” to avoid confusion in the codebase.Would you like to run a codebase search to confirm consistent usage of "metadata.orgid"?
98-99: Field selector resolution is cleanly integrated.
InvokingParsewith the new resolver is consistent with the updated design. No issues noted.
106-107: Label selector usage aligns with the new parsing flow.
This is consistent with the approach for handling hidden selector names. Looks good.
115-116: Annotation selector resolution mirrors label selector pattern.
It’s symmetrical with the label selector logic. No immediate concerns.internal/store/selector/field.go (19)
19-19: Importing uuid is appropriate for new UUID logic.
No concerns here.
29-33: New selectorParserSession struct consolidates selector + resolver.
This design unifies the contextual parsing data, making the code more explicit and testable.
167-169: Updated documentation clarifies usage of resolver.
This helps maintain clarity on how the parse method works with the newresolver.
173-174: Parameter doc lines are helpful.
Good to see thorough documentation regardingctxandresolver.
180-180: Return signature adds clarity on error conditions.
Looks good.
192-195: Check resolver non-nil requirement.
This early check prevents downstream nil reference errors.
197-197: Parser initialization updated to respect new flow.
Approach appears solid.
206-206: Use of selectorParserSession.
Makes sense to pass both theselector&resolvertogether for parsing.
208-208: Error type check ensures correct handling of known selector errors.
No issues noted.
223-226: Ensuring input is a selectorParserSession.
This safety check helps avoid unexpected type assertion panics.
228-230: Resolver presence check.
This guards against nil usage if something is misconfigured.
232-232: Retrieving requirements from session.selector.
Straightforward extraction.
243-243: Custom logic for JSONB array casting.
Correctly flags unsupported cases.
249-253: Improved error message for unknown selector name.
Clarifying that the user can see supported selectors is very helpful.
301-301: Wrap multiple requirements in an AND token.
This is consistent with multi-clause handling.
422-422: Refactoring if private field is used.
The error message is clear that private fields can’t be accessed.
429-435: UUID parsing introduced in resolveValue.
Error handling is correct, returning parse failure immediately.
510-511: Checking for “UUID” type in resolveQuery.
Extends the parser for new type coverage.
529-541: applyUUIDOperator design covers standard operators.
Supports equality, inequality, range comparisons, and membership checks. Good approach.internal/store/selector/resolvers.go (6)
1-27: EmptyResolver for no-op handling.
These methods returning empty slices ensure safe fallback without unexpected panics.
28-31: CompositeSelectorResolver struct design.
Holds a map of table-based resolvers, facilitating multi-model usage. Looks flexible.
33-56: NewCompositeSelectorResolver processes multiple models.
Fetching GORM schema for each, then building a field resolver is a neat approach. Check potential concurrency if multiple goroutines might share resolvers.
58-73: ResolveNames aggregates across all resolvers.
Prefixed table name is correct to avoid collisions in multi-model queries.
75-91: ResolveFields merges field metadata with table name prefix.
Gracefully short-circuits if it finds a matching set in the first resolver.
93-107: List gathers all known selector names.
Sorting them is helpful for stable output. Good use of a set to avoid duplicates.internal/store/selector/annotation_test.go (2)
25-29: LGTM! Proper error handling for resolver initialization.The initialization of the resolver with appropriate error handling is well implemented.
38-38: LGTM! Updated Parse call to use resolver.The Parse method call has been correctly updated to use the new resolver parameter.
internal/store/selector/selector_test.go (3)
28-28: LGTM! Added UUID field with proper type and selector tag.The UUID field is correctly added with appropriate GORM and selector tags.
92-92: LGTM! Added GoodSelector field with proper selector tag.The GoodSelector field is correctly added with the appropriate selector tag.
174-176: LGTM! Updated unknown selector test logic.The test logic for unknown selectors has been updated to check the length of resolved fields instead of expecting an error, which is more precise.
internal/store/model/selectors.go (2)
71-79: LGTM! Well-implemented MapSelectorName for DeviceLabel.The implementation correctly maps the metadata.label.keyorvalue selector to both key and value fields, following the established pattern in the codebase.
81-83: LGTM! Consistent implementation of ListSelectors for DeviceLabel.The ListSelectors method follows the same pattern as other types and correctly exposes the metadata.label.keyorvalue selector.
internal/store/selector/types.go (3)
14-14: LGTM! Added UUID selector type with proper schema resolution.The UUID selector type is correctly added and properly mapped in the schemaTypeResolution map.
Also applies to: 33-33
67-79: LGTM! Well-documented Resolver interface.The Resolver interface is clearly documented with comprehensive method descriptions that explain the purpose and functionality of each method.
175-176: LGTM! Added UUID case to String method.The String method has been properly updated to handle the new UUID type.
internal/store/selector/label_test.go (2)
33-37: LGTM! Good error handling for resolver initialization.The error handling for the resolver initialization is well-implemented, with clear error messages and early return on failure.
46-46: LGTM! Updated Parse call to use the resolver.The Parse method call has been correctly updated to use the resolver, aligning with the changes in the selector interface.
internal/store/selector/label.go (4)
100-118: LGTM! Well-documented method signature.The updated documentation clearly explains the method's parameters, return values, and provides a good example usage.
122-126: LGTM! Good defensive programming with resolver validation.The nil check for the resolver is a good practice that prevents potential runtime panics.
155-156: LGTM! Clear field resolution logic.The comment and implementation for resolving selector fields using the provided resolver is clear and maintainable.
182-185: LGTM! Improved error handling.The error handling has been enhanced to properly handle selector-specific errors, improving error reporting.
internal/store/selector/annotation.go (3)
127-134: LGTM! Consistent documentation style.The documentation follows the same style as label.go, maintaining consistency across the codebase.
150-153: LGTM! Consistent resolver validation.The resolver validation matches the implementation in label.go, ensuring consistent behavior.
182-186: LGTM! Consistent error handling.The error handling pattern matches label.go, maintaining a consistent approach across selectors.
internal/store/selector/selector.go (3)
31-31: LGTM! Interface-based return type.Returning the Resolver interface instead of the concrete type improves abstraction and flexibility.
201-203: LGTM! Added field length validation.The validation ensures consistency between the number of fields and selector names, preventing partial resolutions.
227-230: LGTM! Efficient hidden selector filtering.Using slices.DeleteFunc for filtering hidden selectors is an efficient and idiomatic approach.
internal/store/selector/field_test.go (4)
33-33: LGTM! Good test coverage for valid UUID.The test case properly validates UUID string format support.
46-46: LGTM! Invalid UUID test case is included.The test case ensures proper error handling for invalid UUID formats.
101-107: LGTM! Comprehensive test coverage for UUID operations.The test cases cover all essential UUID operations including existence checks, equality comparisons, and set operations.
200-201: LGTM! Invalid operations are properly tested.The test cases correctly identify unsupported operations for UUID fields.
internal/store/device.go (4)
78-78: LGTM! Device label model is included in migration.The migration properly includes both Device and DeviceLabel models.
156-168: LGTM! Efficient indexing for partial matching.The implementation correctly:
- Ensures pg_trgm extension is available
- Creates GIN index for both key and value fields
- Only applies to PostgreSQL dialect
237-267: LGTM! Well-implemented label retrieval method.The Labels method implementation:
- Uses composite selector resolver
- Properly joins device_labels table
- Handles pagination through limit parameter
- Returns a map of key-value pairs
Let's verify the query performance:
170-213: LGTM! Robust trigger implementation for label synchronization.The trigger implementation:
- Handles both INSERT and UPDATE operations
- Properly manages label deletions
- Uses DISTINCT FROM for efficient change detection
- Implements UPSERT pattern for label updates
Let's verify the trigger behavior:
✅ Verification successful
Verified trigger implementation in internal/store/device.go
The search confirms that the trigger function
sync_device_labelsalong with bothdevice_labels_insertanddevice_labels_updatetriggers are correctly defined in the codebase.
- Trigger function is defined at line 176.
- Insert trigger is defined at line 199.
- Update trigger is defined at line 204.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify trigger function and its usage # Test: Check if the trigger function exists and is properly attached psql -c "\df sync_device_labels" && \ psql -c "\d+ devices" | grep -A 2 "Triggers:"Length of output: 129
Script:
#!/bin/bash # Description: Re-verify that the trigger function and its attached triggers are defined in the codebase. # We search for the trigger function and the triggers in the internal/store/device.go file. rg -n "CREATE OR REPLACE FUNCTION sync_device_labels" internal/store/device.go && \ rg -n "CREATE TRIGGER device_labels_insert" internal/store/device.go && \ rg -n "CREATE TRIGGER device_labels_update" internal/store/device.goLength of output: 360
asafbss
force-pushed
the
EDM-683
branch
2 times, most recently
from
874b38f to
9c0706b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
pkg/k8s/selector/selector.go (1)
232-234: Update error message to include UUID format.The error message should be updated to mention UUID as a valid format since it's now supported.
Apply this diff to improve the error message:
- allErrs = append(allErrs, field.Invalid(valuePath.Index(i), vals[i][j], "for 'Gt', 'Lt', 'Gte', and 'Lte' operators, the value must be a number or a valid time in RFC3339 format")) + allErrs = append(allErrs, field.Invalid(valuePath.Index(i), vals[i][j], "for 'Gt', 'Lt', 'Gte', and 'Lte' operators, the value must be a number, a valid time in RFC3339 format, or a valid UUID"))internal/store/common.go (1)
93-95: Consider using a constant for the metadata.orgid selector name.The hardcoded selector name could be moved to a constant to improve maintainability and reusability.
+const metadataOrgIDSelector = "metadata.orgid" -fmt.Sprintf("%s = ?", lq.resolveOrDefault( - selector.NewHiddenSelectorName("metadata.orgid"), "org_id")), orgId) +fmt.Sprintf("%s = ?", lq.resolveOrDefault( + selector.NewHiddenSelectorName(metadataOrgIDSelector), "org_id")), orgId)internal/store/selector/annotation.go (1)
182-186: Consider extracting error handling logic.The error handling pattern is repeated in multiple places. Consider extracting it to a helper function.
+func wrapSelectorError(err error) error { + if IsSelectorError(err) { + return err + } + return NewSelectorError(flterrors.ErrAnnotationSelectorParseFailed, err) +} -if IsSelectorError(err) { - return "", nil, err -} -return "", nil, NewSelectorError(flterrors.ErrAnnotationSelectorParseFailed, err) +return "", nil, wrapSelectorError(err)internal/store/selector/selector.go (1)
108-110: Consider adding a comment explaining the reset logic.The logic to reset fields when lengths don't match would benefit from a comment explaining the rationale.
+// Reset fields if not all selectors were resolved to maintain consistency if len(fields) > 0 && len(fields) != len(selectorNames) { fields = make([]*SelectorField, 0) }internal/store/selector/field_test.go (1)
49-53: Consider extracting test setup to a helper function.The resolver setup code is repeated in multiple test functions. Consider extracting it to a helper function.
+func setupTestResolver(t *testing.T) Resolver { + resolver, err := SelectorFieldResolver(&goodTestModel{}) + if err != nil { + t.Fatalf("error setting up test resolver: %v", err) + } + return resolver +} -resolver, err := SelectorFieldResolver(&goodTestModel{}) -if err != nil { - t.Errorf("error %v (%#v)\n", err, err) - return -} +resolver := setupTestResolver(t)Also applies to: 264-269
internal/store/device.go (2)
170-213: Consider adding error handling for trigger deletion.While the trigger implementation is solid, consider handling potential errors when dropping existing triggers.
Apply this diff to improve error handling:
- DROP TRIGGER IF EXISTS device_labels_insert ON devices; - DROP TRIGGER IF EXISTS device_labels_update ON devices; + DO $$ + BEGIN + DROP TRIGGER IF EXISTS device_labels_insert ON devices; + DROP TRIGGER IF EXISTS device_labels_update ON devices; + EXCEPTION WHEN OTHERS THEN + -- Log error and continue, as the trigger might not exist + RAISE NOTICE 'Error dropping triggers: %', SQLERRM; + END $$;
237-267: Consider adding pagination support.The
Labelsmethod implementation looks good, but consider adding pagination support for better performance with large datasets.Apply this diff to add pagination:
func (s *DeviceStore) Labels(ctx context.Context, orgId uuid.UUID, listParams ListParams) (map[string]string, error) { var labels []model.DeviceLabel + var offset int + if listParams.Page > 0 { + offset = (listParams.Page - 1) * listParams.Limit + } resolver, err := selector.NewCompositeSelectorResolver(&model.Device{}, &model.DeviceLabel{}) if err != nil { return nil, fmt.Errorf("failed to create selector resolver: %w", err) } query, err := ListQuery(model.Device{}, WithSelectorResolver(resolver)).BuildNoOrder(ctx, s.db, orgId, listParams) if err != nil { return nil, err } query = query.Select("DISTINCT device_labels.label_key, device_labels.label_value"). Joins("JOIN device_labels ON devices.org_id = device_labels.org_id AND devices.name = device_labels.device_name") if listParams.Limit > 0 { query = query.Limit(listParams.Limit) + if offset > 0 { + query = query.Offset(offset) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
internal/store/common.go(2 hunks)internal/store/device.go(5 hunks)internal/store/model/device.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/selectors.go(1 hunks)internal/store/selector/annotation.go(2 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(10 hunks)internal/store/selector/field_test.go(9 hunks)internal/store/selector/label.go(3 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/resolvers.go(1 hunks)internal/store/selector/selector.go(5 hunks)internal/store/selector/selector_test.go(7 hunks)internal/store/selector/types.go(4 hunks)pkg/k8s/selector/selector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/store/selector/label_test.go
- internal/store/model/device.go
- internal/store/selector/label.go
- internal/store/selector/resolvers.go
- internal/store/selector/types.go
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: smoke
- GitHub Check: e2e
🔇 Additional comments (23)
pkg/k8s/selector/selector.go (1)
33-33: LGTM!The addition of the
github.com/google/uuidimport is correctly placed and necessary for UUID parsing support.internal/store/model/resource.go (2)
12-13: LGTM! Clear documentation of the composite primary key.The comment clearly documents the composite primary key structure, which is essential for understanding the model's uniqueness constraints.
16-16: LGTM! Well-structured composite key and index definitions.The GORM tags correctly define:
- A composite primary key using
OrgIDandName- A composite index
org_namewith proper field priorities- A simple index
owner_idxfor theOwnerfieldThis structure aligns well with the PR's goal of optimizing queries while maintaining proper multi-tenant data isolation.
Also applies to: 21-21, 27-27
internal/store/selector/annotation_test.go (2)
25-29: LGTM! Proper error handling for resolver initialization.The code correctly initializes the resolver and handles potential errors, preventing null pointer dereferences in subsequent operations.
38-38: LGTM! Updated Parse method call with resolver.The code correctly uses the resolver pattern for parsing annotations, which aligns with the PR's goal of improving query capabilities.
internal/store/selector/selector_test.go (3)
28-28: LGTM! Added UUID field with proper GORM and selector tags.The Field18 definition correctly includes both GORM and selector tags, enhancing test coverage for UUID handling.
92-93: LGTM! Added GoodSelector field with proper selector tag.The GoodSelector field is correctly defined with a selector tag, improving test coverage for selector resolution.
156-171: LGTM! Enhanced field resolution validation.The code now properly validates both the presence of resolved fields and names, improving error detection and reporting.
internal/store/model/selectors.go (2)
72-80: LGTM! Well-implemented MapSelectorName method for DeviceLabel.The method correctly maps the composite selector to individual key and value selectors, enabling flexible label querying.
82-84: LGTM! Consistent ListSelectors implementation for DeviceLabel.The method follows the established pattern for selector registration, maintaining consistency across the codebase.
internal/store/common.go (3)
58-64: LGTM! Well-designed functional options pattern.The introduction of
ListQueryOptionandWithSelectorResolverfollows the functional options pattern, providing a clean and extensible way to configurelistQueryinstances.
71-88: LGTM! Good error handling and default resolver setup.The
ListQueryfunction properly handles options and provides a sensible default resolver when none is provided. The error handling for resolver creation is appropriate.
134-143: LGTM! Robust field resolution with fallback.The
resolveOrDefaultmethod provides a clean way to resolve fields with a fallback value, properly handling errors and nil cases.internal/store/selector/annotation.go (2)
127-148: LGTM! Well-documented API changes.The documentation updates are thorough and include clear examples of the new resolver-based API.
149-153: LGTM! Good error handling for resolver validation.The early validation of the resolver parameter prevents null pointer issues later in the code.
internal/store/selector/selector.go (2)
31-45: LGTM! Good interface-based design.The change to return a
Resolverinterface instead of a concrete type improves abstraction and flexibility.
227-231: LGTM! Good use of slices.DeleteFunc for filtering.The use of
slices.DeleteFuncfor filtering hidden selectors is clean and efficient.internal/store/selector/field_test.go (1)
33-34: LGTM! Good test coverage for UUID handling.The addition of UUID test cases improves the test coverage for both valid and invalid scenarios.
Also applies to: 46-47
internal/store/selector/field.go (3)
29-32: LGTM! Good encapsulation of selector and resolver.The
selectorParserSessionstruct provides a clean way to bundle the selector and resolver together, improving code organization.
191-195: Good defensive programming with resolver validation.The early validation of the resolver parameter prevents potential nil pointer dereferences later in the code.
429-435: LGTM! Robust UUID parsing and validation.The implementation properly handles UUID parsing and validation using the
google/uuidpackage.internal/store/device.go (2)
78-78: LGTM! Proper model migration order.The migration correctly includes both
DeviceandDeviceLabelmodels in the correct order.
156-168: Good use of GIN index for partial matching.The implementation correctly sets up GIN indexes with trigram operations for efficient partial matching on label keys and values.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
api/v1alpha1/openapi.yaml (1)
16-30: Warning: Undefined Operation Tagdevice-labelsin Global TagsThe new
/api/v1/devicelabelsendpoint uses the tagdevice-labels, but this tag is not defined in the global tags section. To satisfy the API guidelines and suppress the pipeline warning, please add thedevice-labelstag to the global tags list. For example, adjust the tags section as follows:tags: - name: authentication description: Operations for authentication. - name: certificatesigningrequest description: Operations on CertificateSigningRequest resources. - name: device description: Operations on Device resources. - name: enrollmentrequest description: Operations on EnrollmentRequest resources. - name: fleet description: Operations on Fleet resources. - name: repository description: Operations on Repository resources. - name: resourcesync description: Operations on ResourceSync resources. + - name: device-labels + description: Operations on device label mappings.This ensures consistency across your API specification by defining all operation tags globally.
🧹 Nitpick comments (2)
internal/service/device.go (1)
150-204: Consider adding pagination or continue paramWhile the method provides a
limitoption, it does not parse or handle acontinuetoken (like inListDevices), which may become problematic for large label sets. Having a consistent approach to pagination across listing methods helps maintain uniformity and efficiency.internal/store/device.go (1)
156-169: Assess partial matching indexesCreating a single GIN index on both
label_keyandlabel_valuewithpg_trgmis a strong step for substring searches combining both fields. If your queries often involve filtering on just one column, consider separate indexes for potential performance gains.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/v1alpha1/openapi.yaml(2 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(2 hunks)internal/api/client/client.gen.go(7 hunks)internal/api/server/server.gen.go(7 hunks)internal/service/device.go(1 hunks)internal/store/device.go(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: API Quality
api/v1alpha1/openapi.yaml
[error] 925-925: Verb /api/v1/devicelabels.post is required.
[error] 925-925: Verb /api/v1/devicelabels.delete is required.
[warning] 928-928: Operation tags must be defined in global tags.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: smoke
- GitHub Check: integration-tests
- GitHub Check: lint
- GitHub Check: e2e
🔇 Additional comments (14)
api/v1alpha1/spec.gen.go (1)
21-243: Validate the newly embedded Swagger specificationYou're replacing the entire Base64-encoded content with new data. Please verify that the embedded Swagger specification aligns with your new endpoints, especially
/api/v1/devicelabels. Consider running an OpenAPI validator or integration test to confirm that the generated documentation and stubs are valid and up to date.internal/store/device.go (4)
13-13: New import usage looks fineThe import
github.com/flightctl/flightctl/internal/store/selectormatches your usage below; no issues found.
29-29: Labeled interface additionAdding
Labels(...)to the interface provides a convenient way to fetch label data. The signature is consistent with the existing interface patterns.
170-214: Trigger-based device_labels synchronizationThe trigger function and new triggers keep the
device_labelstable in sync with changes to thedevices.labelsJSONB. This approach looks correct for both insert and update operations. If concurrent updates on the same row are likely, ensure transactional consistency is acceptable.
237-271: Verify potential label value overwritingWhen multiple devices share the same label key with different values, this logic overwrites previous values in your final
labelsMap. If that is unintentional, consider returning an array of values per key to avoid data loss.Below is a possible approach if you need to accumulate multiple values for each key:
- func (s *DeviceStore) Labels(ctx context.Context, orgId uuid.UUID, listParams ListParams) (map[string]string, error) { + func (s *DeviceStore) Labels(ctx context.Context, orgId uuid.UUID, listParams ListParams) (map[string][]string, error) { // ... labelsMap := make(map[string][]string, len(labels)) for _, label := range labels { - labelsMap[label.LabelKey] = label.LabelValue + labelsMap[label.LabelKey] = append(labelsMap[label.LabelKey], label.LabelValue) } return labelsMap, nil }api/v1alpha1/types.gen.go (2)
418-419: LGTM! Well-defined type for device labels.The
DeviceLabelMaptype is appropriately defined as a map with string keys and values, which aligns well with the PR's objective of normalizing device labels.
1454-1464: LGTM! Parameters struct follows established patterns.The
ListDeviceLabelsParamsstruct is well-structured with:
- Standard filtering capabilities via label and field selectors
- Pagination support through the limit parameter
- Clear and consistent documentation
internal/api/server/server.gen.go (3)
1-4: Note: This is an auto-generated file.This file is generated by oapi-codegen v2.3.0 and should not be manually modified. Any changes should be made to the OpenAPI specification and the file should be regenerated.
51-52: LGTM: New endpoint implementation is consistent.The new
ListDeviceLabelsendpoint is properly integrated with:
- Interface definition
- Default implementation
- Request/Response types
- Middleware handler
The implementation follows the same patterns as other endpoints in the file.
Also applies to: 268-271, 3213-3264, 6914-6938
2493-2495: LGTM: Router configuration is properly updated.The new endpoint is correctly registered in the router configuration with the appropriate path and handler.
internal/api/client/client.gen.go (4)
131-132: LGTM! Interface method addition is well-defined.The new
ListDeviceLabelsmethod is properly added to theClientInterfacewith appropriate parameters and return types.
1874-1953: LGTM! Request generator implementation follows established patterns.The
NewListDeviceLabelsRequestfunction correctly:
- Handles server URL parsing
- Constructs the API endpoint path
- Processes query parameters (labelSelector, fieldSelector, limit)
- Creates and returns the HTTP request
5025-5049: LGTM! Response type definition is complete.The
ListDeviceLabelsResponsestruct properly includes:
- Raw response body
- HTTP response object
- Typed response fields for success (200) and error cases (400, 401, 403, 503)
7788-7840: LGTM! Response parser implementation is thorough.The
ParseListDeviceLabelsResponsefunction correctly:
- Reads and closes the response body
- Handles content type checking
- Unmarshals JSON responses into appropriate types
- Handles all defined response status codes
|
Hey @celdrake , I want to test the solution to confirm that it meets the requirements. For testing, I’ve created a temporary endpoint ( For listing distinct labels only (not for listing devices), three new selectors are introduced:
My suggestion: Example query: |
|
Hey @asafbss I don't think I'll have time for testing this PR in a few days, as I'm currently working in finishing the Decommissioning and Rollout policies in the UI. |
Hey @celdrake , I completely understand, but I can't proceed with the design review without a basic acknowledgment that the solution meets the requirement. Keep in mind that the TP closes on February 12, including QE, and without approval of the entire solution lifecycle, it may not make it in. Adding the solution after the TP could require a migration for the device labels data, which would add extreme complexity. 🙏 Please share your thoughts—I just need confirmation that it meets the requirement. |
Hi, @asafbss Eg. The following requests does not return all labels that include "test" because there are multiple labels with a key equal to "test". (This happens either with metadata.label.key=test AND metadata.label.keyorvalue=test) I'd expect the response to be a List of unique labels matching the query (either as key=value strings or as `{key: 'just-a-test', value: 'yest'} objects devices-labels-endpoint-test.mp4 |
| additionalProperties: | ||
| type: string | ||
| description: | | ||
| A map of distinct labels, where the keys are label names and the values are label values. |
There was a problem hiding this comment.
| A map of distinct labels, where the keys are label names and the values are label values. | |
| A map of distinct labels, where the keys are label keys and the values are label values. |
which is quite redundant
As this likely will change, let's see what the best description for the new response.
Good catch, I'll return an Array instead of Map. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
internal/store/selector/resolvers.go (1)
75-91: Consider aggregating fields from all resolvers.The
ResolveFieldsmethod returns early when fields are found in one resolver, potentially missing fields from other resolvers. Consider aggregating fields from all resolvers before returning.Apply this diff to aggregate fields from all resolvers:
func (r *CompositeSelectorResolver) ResolveFields(name SelectorName) ([]*SelectorField, error) { + var allFields []*SelectorField for table, resolver := range r.resolvers { fields, err := resolver.ResolveFields(name) if err != nil { return nil, err } if len(fields) > 0 { for _, field := range fields { field.FieldName = fmt.Sprintf("%s.%s", table, field.FieldName) } - return fields, nil + allFields = append(allFields, fields...) } } - return []*SelectorField{}, nil + return allFields, nil }internal/store/model/selectors.go (1)
82-84: Consider adding individual selectors to the list.The method could also include "metadata.label.key" and "metadata.label.value" selectors for direct access.
Apply this diff to include individual selectors:
func (m *DeviceLabel) ListSelectors() selector.SelectorNameSet { - return selector.NewSelectorFieldNameSet().Add(selector.NewSelectorName("metadata.label.keyorvalue")) + return selector.NewSelectorFieldNameSet().Add( + selector.NewSelectorName("metadata.label.keyorvalue"), + selector.NewSelectorName("metadata.label.key"), + selector.NewSelectorName("metadata.label.value"), + ) }internal/store/common.go (1)
71-88: Consider improving error handling for resolver initialization.The error from
SelectorFieldResolveris silently handled by falling back toEmptyResolver. Consider propagating the error or logging it.Apply this diff to improve error handling:
func ListQuery(dest any, opts ...ListQueryOption) *listQuery { q := &listQuery{dest: dest} for _, opt := range opts { opt(q) } // Set resolver if not provided if q.resolver == nil { resolver, err := selector.SelectorFieldResolver(q.dest) if err != nil { + // Log the error before falling back to EmptyResolver + log.Printf("Failed to initialize selector field resolver: %v", err) q.resolver = selector.EmptyResolver{} } else { q.resolver = resolver } } return q }internal/store/device.go (1)
170-213: Consider adding a DELETE trigger for complete synchronization.While the current triggers handle INSERT and UPDATE operations effectively, consider adding a DELETE trigger to automatically clean up the device_labels table when a device is deleted. Although the foreign key with ON DELETE CASCADE would handle this, having an explicit trigger provides better visibility and control over the cleanup process.
CREATE TRIGGER device_labels_update AFTER UPDATE OF labels ON devices FOR EACH ROW WHEN (OLD.labels IS DISTINCT FROM NEW.labels) EXECUTE FUNCTION sync_device_labels(); + + CREATE TRIGGER device_labels_delete + BEFORE DELETE ON devices + FOR EACH ROW + EXECUTE FUNCTION ( + DELETE FROM device_labels + WHERE org_id = OLD.org_id AND device_name = OLD.name; + RETURN OLD; + );internal/api/client/client.gen.go (1)
1-11059: The client code changes align well with the device labels normalization effort.The generated client code provides a clean interface for the new device labels endpoint, maintaining consistency with the existing patterns. The implementation properly handles all standard response cases and error scenarios.
api/v1alpha1/openapi.yaml (2)
16-32: Define Global Tag for device-labels
The GET operation for/api/v1/devicelabelsis tagged withdevice-labels. However, this tag is not currently defined in the global tags section at the top of the specification. Please add a global tag definition (e.g., with a description such as "Operations for managing device labels") to ensure consistency and clarity in the API documentation.
4213-4221: Review Response Schema for Device Labels
TheDeviceLabelMapschema returns an object (map) of distinct labels where each key is a label name and its corresponding value is a label value. Please verify that this format meets the intended use case. If there is a possibility that multiple labels might share the same key (or if clients require access to more than one value per label key), consider revising the response schema to return an array of key–value pairs instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
api/v1alpha1/openapi.yaml(2 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(2 hunks)internal/api/client/client.gen.go(7 hunks)internal/api/server/server.gen.go(7 hunks)internal/service/device.go(1 hunks)internal/store/common.go(2 hunks)internal/store/device.go(5 hunks)internal/store/model/device.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/selectors.go(1 hunks)internal/store/selector/annotation.go(2 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(10 hunks)internal/store/selector/field_test.go(9 hunks)internal/store/selector/label.go(3 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/resolvers.go(1 hunks)internal/store/selector/selector.go(5 hunks)internal/store/selector/selector_test.go(7 hunks)internal/store/selector/types.go(4 hunks)pkg/k8s/selector/selector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/store/selector/label_test.go
- internal/store/model/device.go
- internal/store/selector/annotation_test.go
- internal/store/selector/label.go
- internal/store/selector/field_test.go
- internal/store/selector/selector_test.go
- internal/store/selector/types.go
- internal/store/model/resource.go
- pkg/k8s/selector/selector.go
🧰 Additional context used
📓 Learnings (1)
api/v1alpha1/openapi.yaml (1)
Learnt from: asafbss
PR: flightctl/flightctl#845
File: api/v1alpha1/openapi.yaml:925-981
Timestamp: 2025-02-04T07:53:53.903Z
Learning: The /api/v1/devicelabels endpoint in PR #845 is temporarily exposed for solution validation purposes only, with its final API design still to be determined.
🪛 GitHub Actions: API Quality
api/v1alpha1/openapi.yaml
[error] 952-952: Verb /api/v1/devicelabels.post is required.
[error] 952-952: Verb /api/v1/devicelabels.delete is required.
[warning] 955-955: Operation tags must be defined in global tags.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: smoke
- GitHub Check: e2e
- GitHub Check: integration-tests
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (25)
internal/store/selector/resolvers.go (2)
10-26: LGTM! Clean implementation of a no-op resolver.The
EmptyResolverprovides a clean and correct implementation of theResolverinterface, returning empty slices for all methods.
28-56: LGTM! Clean initialization of composite resolver.The
NewCompositeSelectorResolverfunction correctly:
- Initializes the resolver map
- Parses schema to retrieve table names
- Handles errors from schema parsing and field resolution
internal/store/model/selectors.go (1)
72-80: LGTM! Clean implementation of selector name mapping.The method correctly maps the composite selector to its constituent parts using case-insensitive comparison.
internal/store/common.go (2)
58-64: LGTM! Clean implementation of options pattern.The implementation follows Go best practices for functional options pattern.
134-143: LGTM! Clean implementation of field resolution with fallback.The method correctly handles field resolution and provides appropriate fallback behavior.
internal/store/selector/annotation.go (1)
126-188: LGTM! Clean implementation of resolver-based parsing.The changes correctly:
- Validate the resolver parameter
- Handle errors comprehensively
- Provide clear documentation with examples
internal/store/selector/selector.go (1)
1-352: LGTM!The changes to the
selectorFieldResolverfunctionality improve error handling and the public interface. The code is well-structured and follows best practices.api/v1alpha1/spec.gen.go (1)
1-319: No review needed for auto-generated code.This file contains auto-generated code from the OpenAPI specification.
internal/store/selector/field.go (3)
29-32: LGTM!The new
selectorParserSessiontype improves type safety and maintainability by encapsulating the selector and resolver.
429-435: LGTM!The UUID handling implementation is robust with proper validation and parsing.
191-195: LGTM!The updated
Parsemethod improves type safety and error handling by requiring a non-nil resolver.internal/store/device.go (2)
78-78: LGTM: Device labels table migration added correctly.The migration for the new
DeviceLabelmodel is properly added alongside the existingDevicemodel migration.
156-168: LGTM: Efficient indexing strategy for device labels.The implementation:
- Properly enables pg_trgm extension for partial matching
- Creates a GIN index supporting both label_key and label_value fields
- Includes appropriate existence checks to prevent duplicate indexes
api/v1alpha1/types.gen.go (1)
1464-1474: LGTM: Well-structured parameters for listing device labels.The
ListDeviceLabelsParamsstruct is properly defined with:
- Optional label and field selectors for filtering
- Limit parameter for pagination
- Appropriate JSON and form field tags
internal/api/server/server.gen.go (6)
51-52: LGTM: Interface definition for device labels endpoint.The new
ListDeviceLabelsmethod is correctly added to theServerInterfacewith appropriate parameters.
271-274: LGTM: Unimplemented stub handler.The unimplemented handler correctly returns HTTP 501 Not Implemented status, following the pattern used for other endpoints.
3239-3290: LGTM: Request/Response type definitions.The request and response types are properly defined with:
- Request object containing query parameters
- Response object interface with visitor pattern
- Response implementations for success (200) and error cases (400, 401, 403, 503)
823-865: LGTM: Handler implementation.The middleware handler implementation follows the established pattern with:
- Proper context handling
- Parameter binding from query string
- Error handling for invalid parameters
- Middleware chain execution
- Response type validation
2516-2518: LGTM: Router configuration.The new endpoint is correctly registered with the router using the chi framework.
1-8550: Verify generated code integrity.Since this is generated code, let's verify it matches the OpenAPI spec.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the generated code matches the OpenAPI spec # Check if the OpenAPI spec defines the devicelabels endpoint rg -A 5 '/api/v1/devicelabels' api/v1alpha1/openapi.yaml # Check if the spec defines the DeviceLabelMap type rg 'DeviceLabelMap' api/v1alpha1/openapi.yamlLength of output: 324
Generated Code Verified Against OpenAPI Spec Integrity
The search results confirm that the OpenAPI spec defines the "/api/v1/devicelabels" endpoint (with a GET operation) and references the DeviceLabelMap type as expected. The generated code matches these definitions.
internal/api/client/client.gen.go (4)
131-132: LGTM: Interface addition for device labels listing.The new
ListDeviceLabelsmethod is correctly added to theClientInterfacewith appropriate parameters.
511-521: LGTM: Client implementation for device labels listing.The implementation follows the standard pattern used throughout the codebase:
- Proper error handling
- Context propagation
- Request editor application
5070-5094: LGTM: Response type definition for device labels.The
ListDeviceLabelsResponsestruct correctly handles:
- Raw response body
- HTTP response object
- JSON response for success (200) and error cases (400, 401, 403, 503)
7867-7918: LGTM: Response parsing implementation.The
ParseListDeviceLabelsResponsefunction properly:
- Reads and closes response body
- Handles content type checking
- Unmarshals JSON responses for all status codes
api/v1alpha1/openapi.yaml (1)
952-1008:⚠️ Potential issueMissing Required HTTP Methods in /api/v1/devicelabels Endpoint
The newly introduced/api/v1/devicelabelspath currently only defines a GET operation. However, API quality checks (and previous design discussions) require that POST and DELETE operations be defined as well for complete CRUD support. Even if this endpoint is temporary and used solely for validation purposes, please add stub implementations (or at least placeholders with appropriate documentation/TODO markers) for the POST and DELETE verbs.
For example, you might add something like:‾‾‾diff
- post:
- tags:
- device-labels- description: Create a new device label mapping entry (temporary stub – final design TBD).
- operationId: createDeviceLabel
- requestBody:
description: [Temporary] Device label mapping entry.content:application/json:schema:$ref: '#/components/schemas/DeviceLabelMap'required: true- responses:
"201":description: Createdcontent:application/json:schema:$ref: '#/components/schemas/DeviceLabelMap'"400":description: Bad Requestcontent:application/json:schema:$ref: '#/components/schemas/Error'- delete:
- tags:
- device-labels- description: Delete device label mapping entries (temporary stub – final design TBD).
- operationId: deleteDeviceLabels
- parameters:
- name: labelSelectorin: querydescription: Selector to restrict which device label mappings to delete.schema:type: string- responses:
"200":description: OKcontent:application/json:schema:$ref: '#/components/schemas/Status'"400":description: Bad Requestcontent:application/json:schema:$ref: '#/components/schemas/Error'‾‾‾
🧰 Tools
🪛 GitHub Actions: API Quality
[error] 952-952: Verb /api/v1/devicelabels.post is required.
[error] 952-952: Verb /api/v1/devicelabels.delete is required.
[warning] 955-955: Operation tags must be defined in global tags.
Go head, try again :) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/v1alpha1/types.gen.go (1)
419-420: 🛠️ Refactor suggestionConsider using a structured type for device labels.
The current implementation using a string slice with "key=value" format has limitations:
- No validation of key/value format
- Requires parsing strings for key-value operations
- Cannot represent multiple distinct label combinations with the same key
Consider using a structured type instead:
-// DeviceLabelList A list of distinct labels, where each item is formatted as "key=value". -type DeviceLabelList = []string +// DeviceLabel represents a single key-value label pair +type DeviceLabel struct { + Key string `json:"key"` + Value string `json:"value"` +} + +// DeviceLabelList A list of distinct labels +type DeviceLabelList struct { + Items []DeviceLabel `json:"items"` +}
🧹 Nitpick comments (3)
internal/store/selector/resolvers.go (1)
93-107: Optimize slice allocation in List method.Pre-allocate the slice with the expected capacity to avoid potential reallocations.
func (r *CompositeSelectorResolver) List() []SelectorName { - set := NewSelectorFieldNameSet() + // Pre-allocate with capacity based on number of resolvers + set := NewSelectorFieldNameSet() + list := make([]SelectorName, 0, len(r.resolvers)) for _, resolver := range r.resolvers { set.Add(resolver.List()...) } - list := set.List() + list = set.List() sort.Slice(list, func(i, j int) bool { return list[i].String() < list[j].String() })internal/store/common.go (1)
134-143: Consider adding error logging in resolveOrDefault.The method silently falls back to the default value when resolution fails. Consider logging the error for debugging purposes.
func (lq *listQuery) resolveOrDefault(sn selector.SelectorName, d string) string { r, err := lq.resolver.ResolveFields(sn) if err != nil { + // Log error for debugging purposes + log.Printf("Failed to resolve field %s: %v, using default value %s", sn, err, d) return d } if len(r) > 0 && r[0] != nil { return r[0].FieldName } + log.Printf("No fields resolved for %s, using default value %s", sn, d) return d }internal/store/device.go (1)
237-271: Consider returning structured label data.The current implementation returns labels as string slices in "key=value" format. Consider returning a structured type to make it easier for clients to parse and use the data.
Apply this diff to return structured data:
-func (s *DeviceStore) Labels(ctx context.Context, orgId uuid.UUID, listParams ListParams) ([]string, error) { +func (s *DeviceStore) Labels(ctx context.Context, orgId uuid.UUID, listParams ListParams) ([]struct{ Key, Value string }, error) { var labels []model.DeviceLabel if listParams.Limit < 0 { return nil, flterrors.ErrLimitParamOutOfBounds } resolver, err := selector.NewCompositeSelectorResolver(&model.Device{}, &model.DeviceLabel{}) if err != nil { return nil, fmt.Errorf("failed to create selector resolver: %w", err) } query, err := ListQuery(model.Device{}, WithSelectorResolver(resolver)).BuildNoOrder(ctx, s.db, orgId, listParams) if err != nil { return nil, err } query = query.Select("DISTINCT device_labels.label_key, device_labels.label_value"). Joins("JOIN device_labels ON devices.org_id = device_labels.org_id AND devices.name = device_labels.device_name") if listParams.Limit > 0 { query = query.Limit(listParams.Limit) } if err := query.Find(&labels).Error; err != nil { return nil, ErrorFromGormError(err) } - labelStrings := make([]string, len(labels)) - for i, label := range labels { - labelStrings[i] = fmt.Sprintf("%s=%s", label.LabelKey, label.LabelValue) - } - - return labelStrings, nil + result := make([]struct{ Key, Value string }, len(labels)) + for i, label := range labels { + result[i] = struct{ Key, Value string }{ + Key: label.LabelKey, + Value: label.LabelValue, + } + } + + return result, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
api/v1alpha1/openapi.yaml(2 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(2 hunks)internal/api/client/client.gen.go(7 hunks)internal/api/server/server.gen.go(7 hunks)internal/service/device.go(1 hunks)internal/store/common.go(2 hunks)internal/store/device.go(5 hunks)internal/store/model/device.go(2 hunks)internal/store/model/selectors.go(1 hunks)internal/store/selector/annotation.go(2 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(10 hunks)internal/store/selector/field_test.go(9 hunks)internal/store/selector/label.go(3 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/resolvers.go(1 hunks)internal/store/selector/selector.go(5 hunks)internal/store/selector/selector_test.go(7 hunks)internal/store/selector/types.go(4 hunks)pkg/k8s/selector/selector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/store/selector/label_test.go
- internal/store/model/device.go
- internal/store/selector/label.go
- internal/store/selector/selector_test.go
- internal/store/selector/types.go
- pkg/k8s/selector/selector.go
🧰 Additional context used
📓 Learnings (1)
api/v1alpha1/openapi.yaml (1)
Learnt from: asafbss
PR: flightctl/flightctl#845
File: api/v1alpha1/openapi.yaml:925-981
Timestamp: 2025-02-04T07:53:53.903Z
Learning: The /api/v1/devicelabels endpoint in PR #845 is temporarily exposed for solution validation purposes only, with its final API design still to be determined.
🪛 GitHub Actions: API Quality
api/v1alpha1/openapi.yaml
[error] 952-952: Verb /api/v1/devicelabels.post is required.
[error] 952-952: Verb /api/v1/devicelabels.delete is required.
[warning] 955-955: Operation tags must be defined in global tags.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: e2e
- GitHub Check: integration-tests
- GitHub Check: smoke
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (35)
internal/store/selector/resolvers.go (2)
10-26: LGTM! Clean implementation of the null object pattern.The EmptyResolver provides a safe no-op implementation that can be used as a default resolver.
28-56: LGTM! Well-structured composite pattern implementation.The CompositeSelectorResolver effectively manages multiple resolvers with proper error handling and clear separation of concerns.
internal/store/selector/annotation_test.go (1)
25-29: LGTM! Comprehensive test coverage.The test cases thoroughly validate the resolver functionality, error handling, and various selector operations.
Also applies to: 83-87, 161-165
internal/store/model/selectors.go (1)
72-84: LGTM! Well-implemented selector functionality for DeviceLabel.The implementation follows established patterns and adds useful search flexibility with the keyorvalue selector.
internal/store/common.go (1)
58-88: LGTM! Clean implementation of the functional options pattern.The ListQueryOption and WithSelectorResolver provide a flexible and extensible way to configure queries.
internal/store/selector/annotation.go (3)
127-134: LGTM! Documentation improvements.The updated documentation clearly describes the method's parameters and their purpose.
149-153: LGTM! Proper error handling.Good addition of nil check for the resolver parameter.
155-156: LGTM! Clean resolver usage.The code now properly uses the resolver interface for field resolution.
internal/store/selector/selector.go (3)
108-110: LGTM! Proper handling of partial field resolution.Good addition to reset fields when not all selectors are resolved, ensuring data consistency.
115-206: LGTM! Well-structured resolver implementation.The new
resolveSelectormethod provides comprehensive field resolution with proper error handling for:
- Schema type validation
- Array validation
- JSONB field handling
- Nested selector resolution
227-231: LGTM! Improved hidden selector filtering.Good use of
slices.DeleteFuncto filter out hidden selectors from the list.internal/service/device.go (3)
134-183: Consider returning an array instead of a map for device labels.Based on user feedback in the PR comments, returning a map prevents users from retrieving all labels that match a query if they share the same key. Consider modifying the response to return a list of unique labels matching the query instead.
156-166: LGTM! Proper limit handling.Good implementation of limit validation and default value assignment.
175-182: LGTM! Comprehensive error handling.Good error handling for:
- Limit parameter bounds
- Selector errors
internal/store/selector/field_test.go (3)
33-34: LGTM! Comprehensive UUID test cases.Good addition of both valid and invalid UUID test cases.
Also applies to: 46-47
101-107: LGTM! Complete UUID operation test coverage.Good coverage of UUID operations including:
- Existence checks
- Equality operations
- Array operations
200-202: LGTM! Proper UUID validation.Good addition of test cases to verify that contains/notcontains operations are not supported for UUIDs.
api/v1alpha1/spec.gen.go (1)
21-246: LGTM!The changes to the swagger specification are auto-generated and align with the API changes for device labels management.
internal/store/selector/field.go (4)
29-32: LGTM! Well-structured session type.The new
selectorParserSessiontype effectively encapsulates the selector and resolver, improving code organization and type safety.
191-195: Improved error handling for resolver validation.Early validation of the resolver parameter prevents potential nil pointer dereferences and provides clear error messages.
429-435: LGTM! Robust UUID value parsing.The implementation correctly handles UUID parsing and validation using the
google/uuidpackage.
529-541: LGTM! Comprehensive UUID operator support.The
applyUUIDOperatorfunction supports all necessary comparison operators for UUID fields while properly rejecting unsupported operations.internal/store/device.go (2)
156-168: LGTM! Efficient indexing for partial matching.The implementation correctly creates GIN indexes with trigram operators for partial matching on label keys and values, which will improve search performance.
170-213: Verify the trigger implementation.The trigger implementation for synchronizing device labels looks correct, but let's verify a few aspects:
- The trigger handles both INSERT and UPDATE operations
- The trigger properly cleans up old labels
- The trigger maintains data consistency with UPSERT operations
Run this script to verify the trigger implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the trigger implementation for device labels synchronization # Test 1: Check if triggers are created echo "Checking triggers..." rg -A 5 "CREATE TRIGGER device_labels" internal/store/device.go # Test 2: Check if the trigger function handles all cases echo "Checking trigger function..." rg -A 20 "sync_device_labels" internal/store/device.go # Test 3: Check if proper cleanup is implemented echo "Checking cleanup logic..." rg "DELETE FROM device_labels" internal/store/device.goLength of output: 2558
Verification of Trigger Implementation
- Both INSERT and UPDATE triggers are correctly created and reference the "sync_device_labels" function.
- The trigger function properly performs cleanup by deleting outdated labels during UPDATE events.
- The UPSERT operation using "ON CONFLICT" ensures data consistency.
api/v1alpha1/types.gen.go (1)
1464-1474: LGTM! Well-structured parameters for listing device labels.The parameters follow Kubernetes conventions for list operations and include:
- Label selector for filtering by labels
- Field selector for filtering by fields
- Limit for pagination
internal/api/server/server.gen.go (5)
51-52: LGTM: New interface method for device labels endpoint.The interface definition for
ListDeviceLabelsis correctly added to theServerInterfacewith appropriate parameters.
271-274: LGTM: Unimplemented stub handler added.The unimplemented stub handler for
ListDeviceLabelsfollows the same pattern as other endpoints, returningStatusNotImplementedby default.
823-865: LGTM: Handler implementation follows established patterns.The
ListDeviceLabelshandler implementation:
- Correctly handles context and error management
- Properly binds query parameters (labelSelector, fieldSelector, limit)
- Follows the same middleware pattern as other endpoints
- Has appropriate error handling for parameter binding
2516-2518: LGTM: Router configuration properly added.The new endpoint is correctly registered in the router configuration with appropriate path and handler.
1-8550: 🏁 Script executed:#!/bin/bash # Description: Verify the generated code matches the OpenAPI spec # Find the OpenAPI spec file fd -t f 'openapi.yaml' -x echo "Found OpenAPI spec at: {}" # Check when the spec was last modified fd -t f 'openapi.yaml' -x stat -f "%m - %N" {} # Check if there are any uncommitted changes to the spec git status --porcelain | grep 'openapi.yaml'Length of output: 332
internal/api/client/client.gen.go (4)
131-132: LGTM! Interface method addition is consistent.The
ListDeviceLabelsmethod follows the established pattern for list operations in the interface.
511-521: LGTM! Client method implementation is correct.The implementation follows the established pattern for list operations, with proper context handling and error management.
5070-5094: LGTM! Response type definition is complete.The
ListDeviceLabelsResponsestruct includes all necessary fields and methods for handling the response, with proper error types.
7867-7918: LGTM! Response parser implementation is thorough.The parser correctly handles all response types and follows proper response body handling practices.
api/v1alpha1/openapi.yaml (1)
952-1007: undefined🧰 Tools
🪛 GitHub Actions: API Quality
[error] 952-952: Verb /api/v1/devicelabels.post is required.
[error] 952-952: Verb /api/v1/devicelabels.delete is required.
[warning] 955-955: Operation tags must be defined in global tags.
|
@asafbss Tested with the latest changes and on a first glance, I can see it's working correctly 🎉 . I won't be able to spend much more time on it though. EDIT: I just noticed that unlike other "List" responses, there is no |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/store/common.go (1)
134-143: Consider optimizing the field resolution logic.The current implementation could be more efficient by avoiding unnecessary array access.
Consider this optimization:
-func (lq *listQuery) resolveOrDefault(sn selector.SelectorName, d string) string { - r, err := lq.resolver.ResolveFields(sn) - if err != nil { - return d - } - if len(r) > 0 && r[0] != nil { - return r[0].FieldName - } - return d -} +func (lq *listQuery) resolveOrDefault(sn selector.SelectorName, d string) string { + names, err := lq.resolver.ResolveNames(sn) + if err != nil || len(names) == 0 { + return d + } + return names[0] +}This version:
- Uses
ResolveNameswhich is more appropriate for this use case- Avoids unnecessary array access checks
internal/store/device.go (1)
170-213: Add a comment about PostgreSQL dependency.The trigger implementation for synchronizing device labels is well-designed. Consider adding a comment to document that this synchronization mechanism is PostgreSQL-specific.
// Ensure trigger is created for INSERT & UPDATE (labels JSONB changes) +// Note: This synchronization mechanism is PostgreSQL-specific and relies on PL/pgSQL triggers if s.db.Dialector.Name() == "postgres" {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/v1alpha1/openapi.yaml(1 hunks)api/v1alpha1/spec.gen.go(2 hunks)api/v1alpha1/types.gen.go(1 hunks)internal/store/common.go(2 hunks)internal/store/device.go(5 hunks)internal/store/model/device.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/selectors.go(1 hunks)internal/store/selector/annotation.go(2 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(10 hunks)internal/store/selector/field_test.go(9 hunks)internal/store/selector/label.go(3 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/resolvers.go(1 hunks)internal/store/selector/selector.go(5 hunks)internal/store/selector/selector_test.go(7 hunks)internal/store/selector/types.go(4 hunks)pkg/k8s/selector/selector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- api/v1alpha1/types.gen.go
- internal/store/selector/label_test.go
- internal/store/selector/field_test.go
- internal/store/selector/annotation_test.go
- internal/store/model/device.go
- internal/store/selector/selector_test.go
- internal/store/selector/label.go
- internal/store/model/resource.go
- internal/store/selector/resolvers.go
- pkg/k8s/selector/selector.go
- api/v1alpha1/openapi.yaml
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: lint
- GitHub Check: smoke
- GitHub Check: e2e
- GitHub Check: integration-tests
- GitHub Check: unit-tests
🔇 Additional comments (22)
internal/store/model/selectors.go (2)
72-80: LGTM! Clean implementation of selector name mapping.The function correctly implements case-insensitive mapping of the composite selector to its constituent parts.
82-84: LGTM! Clean implementation of selector listing.The function correctly returns a selector set containing the composite selector.
internal/store/common.go (3)
58-64: LGTM! Clean implementation of the functional options pattern.The implementation provides a flexible and type-safe way to configure listQuery instances.
71-88: LGTM! Robust initialization with fallback handling.The function correctly handles option application and provides a sensible default resolver.
90-124: LGTM! Clean integration of resolver-based field resolution.The method correctly uses the resolver for field resolution while maintaining existing functionality.
internal/store/selector/types.go (3)
14-14: LGTM! Clean addition of UUID support.The UUID constant is correctly integrated into the type system.
67-79: LGTM! Well-designed and documented interface.The Resolver interface is well-structured with clear method responsibilities and comprehensive documentation.
175-176: LGTM! Consistent type string representation.The UUID string representation follows the established pattern and matches the database type.
internal/store/selector/annotation.go (2)
126-149: LGTM! Clear and comprehensive documentation update.The documentation accurately reflects the new resolver-based approach with clear examples.
149-188: LGTM! Robust implementation with comprehensive error handling.The implementation correctly uses the resolver with appropriate error handling at each step.
internal/store/selector/selector.go (4)
29-32: LGTM!The
selectorParserSessionstruct is well-designed with clear and descriptive field names.
31-45: LGTM!The function signature change to return
Resolverinterface improves the abstraction and aligns with the PR objectives.
108-110: LGTM!The added condition is a good defensive programming practice that ensures data consistency by resetting fields when lengths don't match.
115-206: LGTM!The
resolveSelectorfunction is well-implemented with:
- Clear error handling
- Proper array indexing and JSONB casting
- Descriptive error messages
internal/store/selector/field.go (2)
429-435: LGTM!The UUID value resolution is correctly implemented with proper parsing and validation.
529-541: LGTM!The UUID operator handling is comprehensive with support for all standard comparison operators and proper error handling. Unit tests for this functionality exist in
field_test.go.internal/store/device.go (3)
78-78: LGTM!The addition of
model.DeviceLabelto AutoMigrate aligns with the PR objectives to normalize device labels into a separate table.
156-168: LGTM!The GIN index creation with pg_trgm extension will optimize partial match searches on label keys and values.
237-271: LGTM!The Labels method implementation correctly returns a slice of strings to preserve all distinct label combinations, avoiding potential data loss that could occur with a map-based approach.
api/v1alpha1/spec.gen.go (3)
1-16: LGTM! Note that this is generated code.The imports are appropriate for handling the encoded Swagger specification. As this is generated code, any changes should be made to the source OpenAPI specification rather than this file.
246-316: LGTM! The utility functions are well-implemented.The functions for decoding and loading the specification handle errors appropriately, implement efficient caching, and securely resolve external references.
19-244: Verify the encoded Swagger specification content.The Base64 encoded specification has been updated to include the new device label endpoints and schemas. Let's verify the decoded content matches the expected changes.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Decode and verify the Swagger specification content # Test: Decode the Base64 content and check for device label related endpoints echo "$(echo "$(echo "${swaggerSpec[*]}" | base64 -d)" | gunzip)" | grep -A 10 "devicelabels"Length of output: 216
Additional verification required for the encoded spec.
The initial command failed because the environment didn’t have the base64 utility. Please run the following Python‐based script to decode the Swagger specification from
api/v1alpha1/spec.gen.goand verify whether it contains the expected "devicelabel" endpoints. Once confirmed, we can conclude the review items.Once this script is executed, please verify that the output confirms the presence of the new device label endpoints and schema changes as expected by the PR objectives.
🏁 Script executed:
#!/bin/bash # This script uses Python to decode and decompress the swaggerSpec variable from the Go file python3 << 'EOF' import re, base64, gzip try: with open("api/v1alpha1/spec.gen.go", "r") as f: content = f.read() except Exception as e: print("Error reading file:", e) exit(1) # Extract the swaggerSpec block containing the Base64 encoded, gzipped content m = re.search(r'var\s+swaggerSpec\s*=\s*\[]string\s*{([^}]+)}', content, re.DOTALL) if not m: print("swaggerSpec variable block not found.") exit(1) # Find all base64 encoded string literals in the block strings = re.findall(r'"([^"]+)"', m.group(1)) if not strings: print("No base64 encoded strings found.") exit(1) # Concatenate all parts to form the full Base64 string data = "".join(strings) try: decoded = base64.b64decode(data) except Exception as e: print("Error decoding base64:", e) exit(1) try: decompressed = gzip.decompress(decoded).decode("utf-8") except Exception as e: print("Error decompressing:", e) exit(1) # Look for device label related endpoints (case insensitive match) matches = [line for line in decompressed.splitlines() if "devicelabel" in line.lower()] if matches: print("Device label endpoints found:") for line in matches: print(line) else: print("No device label endpoints found in the decoded spec.") EOFLength of output: 65546
The decoded Swagger spec confirms the updated device label endpoints and schema changes.
The Python script output shows that the spec now contains device label related definitions (e.g., "DeviceLabelList"), aligning with the PR objectives.
Correct, pagination for this list is not supported and will have a hard limit (e.g., 1000) once the API is introduced. |
…g JSONB for fast listing - Introduce `device_labels` table to store labels as structured key-value pairs for optimized querying - Retain `devices.labels` JSONB field to ensure efficient device listing while enabling structured lookups - Ensure `device_labels` references `devices (org_id, name)` as a foreign key with `ON DELETE CASCADE` - Improve query performance by indexing `label_key` and `label_value` using `pg_trgm` for partial matching - Add trigger-based synchronization to keep `device_labels` updated whenever `devices.labels` JSONB changes - Lay the foundation for efficient, indexed label-based queries without requiring full JSONB scans
- Implement `Labels` function in `DeviceStore` to retrieve distinct labels efficiently. - Utilize `CompositeSelectorResolver` to dynamically resolve field names across `devices` and `device_labels`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/store/selector/resolvers.go (1)
58-91: Potential improvement to logging errors.
When iteration encounters an error inResolveNamesorResolveFields, it returns immediately. Consider adding logging or wrapping the error with additional context about which table triggered the error to improve debuggability.internal/store/common.go (1)
71-88: Provide clearer error feedback when resolving fields fails.
By defaulting toEmptyResolveron error, potential misconfigurations silently degrade to no-op resolutions. You may want to log or bubble up such errors if they are likely to indicate a developer mistake.internal/store/selector/types.go (1)
108-110: Consider adding a comment explaining the reset logic.The reset of the
fieldsslice when the length doesn't matchselectorNamesis a critical operation that deserves explanation.+ // Reset fields if not all selectors were resolved to maintain consistency if len(fields) > 0 && len(fields) != len(selectorNames) { fields = make([]*SelectorField, 0) }internal/store/selector/annotation.go (1)
149-153: Consider using a constant for the error message.The error message for invalid array access could be moved to a constant to maintain consistency and make updates easier.
+ const errInvalidArrayAccess = "array access must specify a valid index (e.g., 'conditions[0]'); invalid selector: %s" if !arrayPattern.MatchString(selectorName) { - return nil, fmt.Errorf( - "array access must specify a valid index (e.g., 'conditions[0]'); invalid selector: %s", selectorName) + return nil, fmt.Errorf(errInvalidArrayAccess, selectorName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/v1alpha1/openapi.yaml(1 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(1 hunks)internal/store/common.go(2 hunks)internal/store/device.go(5 hunks)internal/store/model/device.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/selectors.go(1 hunks)internal/store/selector/annotation.go(2 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(10 hunks)internal/store/selector/field_test.go(9 hunks)internal/store/selector/label.go(3 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/resolvers.go(1 hunks)internal/store/selector/selector.go(5 hunks)internal/store/selector/selector_test.go(7 hunks)internal/store/selector/types.go(4 hunks)pkg/k8s/selector/selector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- api/v1alpha1/types.gen.go
- pkg/k8s/selector/selector.go
- internal/store/selector/label_test.go
- internal/store/selector/label.go
- internal/store/model/device.go
- internal/store/selector/annotation_test.go
- internal/store/selector/field_test.go
- internal/store/model/resource.go
- internal/store/selector/selector_test.go
🧰 Additional context used
🧠 Learnings (1)
api/v1alpha1/openapi.yaml (1)
Learnt from: asafbss
PR: flightctl/flightctl#845
File: api/v1alpha1/openapi.yaml:925-981
Timestamp: 2025-02-04T07:53:53.903Z
Learning: The /api/v1/devicelabels endpoint in PR #845 is temporarily exposed for solution validation purposes only, with its final API design still to be determined.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-eln-aarch64
- GitHub Check: lint
- GitHub Check: e2e
- GitHub Check: unit-tests
- GitHub Check: integration-tests
- GitHub Check: smoke
🔇 Additional comments (17)
internal/store/selector/resolvers.go (2)
28-31: Consider handling table name collisions.
Currently, if two models share the same table name, the last resolver assigned will overwrite any previous resolver in theresolversmap. If this is unintended, you might want to detect duplicates and raise an error or implement a merging strategy.
58-91: Return behavior inResolveFieldsmight cause partial resolution.
ResolveFieldsimmediately returns once it finds non-empty fields in a single resolver. If a selector could validly match multiple resolvers, only the first match will be returned, ignoring others. Verify that this is the intended design.internal/store/model/selectors.go (1)
72-85: Clarify combined selectors with “keyorvalue”.
The aggregator approach that returns both “metadata.label.key” and “metadata.label.value” is helpful for flexible matching. However, ensure that all query paths and the rest of your code can handle these two separate selectors consistently. For instance, confirm that yourCompositeSelectorResolveror other resolvers do not mistakenly ignore “metadata.label.key” or “metadata.label.value” when referenced separately.internal/store/common.go (2)
60-64: Injecting a custom resolver is a great addition.
TheWithSelectorResolverfunction neatly encapsulates custom resolver injection. This will make the code more modular and testable.
134-143: Silent fallback inresolveOrDefault.
WhenResolveFieldsfails, the function silently reverts to a default string. This can obscure potential incorrect selectors. Consider logging or clearly surfacing the error to guide queries toward valid selectors.internal/store/selector/types.go (2)
14-14: LGTM! Addition of UUID type.The addition of the
UUIDtype to theSelectorTypeenum is well-placed and properly integrated with the existing type system.
67-79: LGTM! Well-designed Resolver interface.The new
Resolverinterface is well-documented and provides a clean abstraction for dynamic selector resolution. The three methods cover all necessary operations:
ResolveNames: Maps selectors to field namesResolveFields: Provides detailed selector metadataList: Enumerates available selectorsinternal/store/selector/selector.go (2)
31-31: LGTM! Interface change aligns with new architecture.The return type change from
*selectorFieldResolvertoResolverimproves abstraction and flexibility.
227-231: LGTM! Improved hidden selector filtering.The use of
slices.DeleteFuncfor filtering hidden selectors is a clean and efficient implementation.api/v1alpha1/spec.gen.go (1)
21-243: LGTM! Updated API specification.The swagger specification has been updated to include device label support, aligning with the PR objectives.
internal/store/selector/field.go (3)
29-32: LGTM! Good encapsulation of selector and resolver.The introduction of
selectorParserSessiontype and the resolver validation inParsemethod improves type safety and error handling.Also applies to: 191-195
429-435: LGTM! Comprehensive UUID support added.The implementation includes proper UUID parsing, validation, and a complete set of supported operators (=, !=, >, >=, <, <=, in, notin, exists, !exists).
Also applies to: 529-541
643-650: LGTM! Improved error handling in resolveSelectorField.The error message now includes the list of supported selectors, making it more helpful for debugging.
internal/store/device.go (3)
78-78: LGTM! Proper setup of device_labels table and indexes.The migration correctly sets up the device_labels table and creates a GIN index for optimized partial match searches on label_key and label_value fields.
Also applies to: 156-168
170-213: LGTM! Well-implemented label synchronization trigger.The trigger function correctly:
- Synchronizes JSONB labels with the device_labels table
- Handles both INSERT and UPDATE operations
- Cleans up old labels on UPDATE
- Uses ON CONFLICT for upsert behavior
237-271: LGTM! Efficient implementation of Labels method.The method correctly:
- Returns distinct label key-value pairs
- Uses JOIN with device_labels table for efficient querying
- Formats labels as "key=value" strings as required by the API
api/v1alpha1/openapi.yaml (1)
4157-4166: Review of the new DeviceLabelList schema definitionThe new schema defines
DeviceLabelListas an array of strings, where each entry is expected to be formatted as"key=value". This design meets the goal of returning an array (instead of a map) for distinct labels, which aligns with the feedback provided (i.e. ensuring that multiple labels sharing the same key can be returned correctly).A few suggestions for further consideration:
- Validation of Format: Consider if additional constraints (e.g., a regular expression) should be applied to ensure that each string adheres to the "key=value" format, especially to prevent issues if keys or values themselves contain an equals sign.
- Documentation Clarity: The description and example are clear. However, you may want to note any limitations or expectations regarding special characters in keys/values.
Overall, the implementation looks consistent with the PR objectives and the API improvements discussed.
…Queries and Fast Distinct Label Retrieval (flightctl#845) * EDM-683: Normalize device labels into a separate table while retaining JSONB for fast listing - Introduce `device_labels` table to store labels as structured key-value pairs for optimized querying - Retain `devices.labels` JSONB field to ensure efficient device listing while enabling structured lookups - Ensure `device_labels` references `devices (org_id, name)` as a foreign key with `ON DELETE CASCADE` - Improve query performance by indexing `label_key` and `label_value` using `pg_trgm` for partial matching - Add trigger-based synchronization to keep `device_labels` updated whenever `devices.labels` JSONB changes - Lay the foundation for efficient, indexed label-based queries without requiring full JSONB scans * EDM-683: Add device Labels listing function and Composite Selector - Implement `Labels` function in `DeviceStore` to retrieve distinct labels efficiently. - Utilize `CompositeSelectorResolver` to dynamically resolve field names across `devices` and `device_labels` * EDM-683: Add api.devicelist as preparation for future API Signed-off-by: kubealex <al.rossi87@gmail.com>
3 participants
Summary
This PR introduces a structured approach to managing device labels by normalizing them into a separate
device_labelstable while retaining the existing JSONB-based storage indevices. The new structure significantly improves query performance, enables efficient lookups, and allows direct retrieval of distinct labels without iterating over all devices.Key Changes
device_labelstable to store labels as structured key-value pairs, optimizing queries.devices.labelsJSONB field to ensure efficient device listing while enabling structured lookups.device_labelsreferencesdevices (org_id, name)as a foreign key withON DELETE CASCADE.label_keyandlabel_valueusingpg_trgmfor partial matching.device_labelsupdated wheneverdevices.labelsJSONB changes.Optimized Querying of Distinct Labels
By leveraging the structured
device_labelstable, distinct label retrieval is now handled efficiently at the database level. Instead of iterating through JSONB fields, queries can directly fetch unique labels using indexed lookups:Summary by CodeRabbit
New Features
Refactor
Documentation