Integrate Labels and Annotations Filtering into Selector Framework#751
Integrate Labels and Annotations Filtering into Selector Framework#751
Conversation
WalkthroughThis pull request introduces significant changes to the Flight Control API, focusing on selector and query management. The modifications streamline the API by removing several query parameters from endpoints such as ResourceSyncs, Devices, and Fleets. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIServer
participant SelectorPackage
participant DatabaseStore
Client->>APIServer: List Request with Selectors
APIServer->>SelectorPackage: Parse Selectors
SelectorPackage-->>APIServer: Validated Selectors
APIServer->>DatabaseStore: Query with Parsed Selectors
DatabaseStore-->>APIServer: Filtered Results
APIServer-->>Client: Return Filtered Resources
This diagram illustrates the new flow of selector-based querying, highlighting how selectors are parsed, validated, and used to filter resources before returning results to the client. 🪧 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: 3
🧹 Nitpick comments (47)
internal/store/common.go (1)
78-85: Ensure LabelSelector usage is tested and validated.The invocation of
listParams.LabelSelector.Parsewithselector.NewHiddenSelectorName("metadata.labels")is correct, but ensure your test suite covers edge cases such as empty label selectors, invalid syntax, and complex label-based queries.internal/store/selector/types.go (4)
100-102: Verify string conversion usage.The
String()method returnsstring(sn.name). This is safe, but if future expansions introduce typed or localized strings, be mindful that the underlying representation might need translation or additional checks.
105-109: Hidden selector usage for internal queries.
hiddenSelectorNameis a neat approach for isolating internal or system-level selectors. Provide doc comments or usage examples to help developers understand how and when to use hidden selectors.
115-117: Overriding hidden selectors.
hiddenSelectorNameandselectorNameshare the same interface. If future requirements dictate special hidden selector logic, consider giving them a more specialized method set.
194-196: Potential future expansions for SelectorOpt.
SelectorOptcan store more structured data if needed. Currently, it’s a map of empty structs, which is lightweight, but you might adapt it for storing typed information.pkg/queryparser/sqljsonb/sqljsonb.go (3)
89-98: Wrap for typed arguments.
Wrapensures typed argument checking. IfAssertSliceTypefails, consider capturing why in logs for simpler debugging (e.g., "expected string, got int").
273-282: queryContains usage.Using the Postgres JSON containment operator
@>is a powerful approach. If you switch to a different DB engine in the future, keep an abstraction layer to avoid vendor lock-in.
322-326: isValidColumnName sanity check.The regex ensures the name begins with letters/underscore. This is correct for typical naming. If you foresee uppercase or special naming conventions, you might need to adapt or expand the pattern.
internal/store/resourcesync.go (1)
66-78: Index creation for annotations.Same approach as labels. Confirm annotation usage patterns for large unstructured data. GIN indexing can be expensive for frequent updates. Make sure it’s well-justified.
internal/store/repository.go (1)
241-241: Consider preserving existing annotations
Settingrepository.Annotations = nilunconditionally might override user-provided annotations. Evaluate if there's a scenario requiring merging or retaining existing annotations.internal/store/selector/annotation.go (5)
22-41: Improve clarity of panic messages
Currently, the function panics with the original error. Consider adding contextual information (e.g.,"failed to create annotation selector from map: %v").
43-72: Efficient string construction
Consider using a string builder or directly constructing the final selector string to reduce intermediate allocations if performance becomes critical.
74-91: Consistent usage of OrDie pattern
This mirrorsNewAnnotationSelectorFromMapOrDie. Keeping panic messages consistent helps with debugging.
93-116: Documentation note
You mention keys are validated later. Consider referencing that detail more explicitly in the docstring, clarifying that annotation key checks happen post-parse.
182-228: Potential performance consideration
If many requirements exist, repeatedly callingctx.Err()is fine, but you may want to short-circuit if performance or large sets become a concern.internal/store/certificatesigningrequest.go (1)
208-208: Discuss overwriting Annotations
Unconditionally settingAnnotationsto nil can remove user data. Double-check if partial merges or preserving them might be needed.internal/service/device.go (2)
67-70: Explicit field selector creation
Creating a field selector from user input is direct. Consider logging parser errors for easier debugging.
116-116: Consistent usage
Including the label selector in listParams matches the rest of the store logic.internal/store/fleet.go (1)
518-518: Concurrent updates on annotations
While settingannotationswithMakeJSONMap, consider concurrency scenarios. If multiple updates happen around the same time, consider employing optimistic locking or additional checks if partial merges are needed.internal/store/store.go (1)
133-137: Leverage new selectors cautiously.
By introducingFieldSelector,LabelSelector, andAnnotationSelectorinListParams, we gain improved filtering. Make sure to consistently switch to these new selectors across all relevant code, removing any obsolete references to the old filtering approach.test/integration/tasks/repotester_conditions_test.go (1)
6-6: Optimize string import usage if possible.
If the only usage of"strings"is forstrings.EqualFold(), this is fine. Otherwise, consider grouping string utilities if needed to maintain clarity and reduce redundant imports.internal/store/model/selectors_test.go (2)
49-49: Improve error context.When returning errors such as “does not exist in API schema,” consider appending more context to help pinpoint the problematic selector or path inside the schema. For instance, you could include the schema name or path that was expected.
53-53: Align error message with domain terminology.Similarly, consider consistency in error messaging. Re-check whether “defined types and schema types do not match” should align with domain-specific language or remain general.
internal/store/selector/selector_test.go (1)
110-113: Maintain consistent type usage for unrecognized selectors.For “bad” or unsupported fields, returning them in the list ensures negative test coverage. Just keep verifying that each new “bad” selector ties to test cases that confirm error handling.
pkg/k8s/selector/labels/selector.go (1)
29-66: New functionParseSelectorOrDieprovides a convenient parse-or-panic approach.
This is acceptable for internal usage or in contexts where a parse error indicates a developer or configuration mistake. If used on user input, handle with caution.internal/service/templateversion.go (3)
44-47: Ensure descriptive error messages.When parsing the fleet-based field selector, consider appending more contextual information to the error message to facilitate easier debugging, such as indicating the fleet name or relevant request parameters that caused the failure.
49-55: Consider adding a guard clause.Before spawning an additional field selector, you might add a brief check to ensure
*request.Params.FieldSelectoris not empty, preventing any extraneous calls. This won't drastically affect performance but can maintain clarity.
68-68: Retain consistent ordering of fields in ListParams.If you plan to add more selectors or filters in the future, consider grouping them consistently (e.g., always list
FieldSelectorbeforeLabelSelector) for better visual clarity.internal/store/templateversion.go (2)
48-60: Refactor repeated GIN index creation logic.Both label and annotation indexing logic share a similar structure. You could refactor them into a small helper function that handles the “check, then create” process, reducing duplication and potential errors.
74-74: Wrap final return for clarity.Consider adding a small debug log before returning nil to indicate successful migration completion, helping in diagnosing migration flows in logs.
api/v1alpha1/util.go (1)
292-324: Defensive handling of unknown operators.Returning an empty string for unsupported operators is fine. However, consider logging a debug or trace message in case an unexpected operator arises. This will aid in diagnosing potential configuration mistakes.
internal/store/enrollmentrequest.go (1)
43-45: Consider adding a log statement on migration error.
While the error is correctly returned, adding a log statement here can provide more context in production environments. This helps operators troubleshoot migration issues quickly.if err := s.db.AutoMigrate(&model.EnrollmentRequest{}); err != nil { + s.log.Errorf("Failed to auto-migrate EnrollmentRequest model: %v", err) return err }test/integration/store/resourcesync_test.go (1)
238-239: Refine label-based filtering test coverage.
It's good to see label-based filtering. Consider adding tests that validate behavior with multiple matching labels or partial matches to ensure that the new label selector logic works correctly in more complex scenarios.internal/service/resourcesync.go (1)
82-82: Logical separation
Adding a blank line helps readability by separating the code for label selector handling from the subsequent logic.internal/cli/get.go (1)
85-86: Clarify supported operators in help text
The updated descriptions forselectorandfield-selectorflags are more explicit. Consider also listing other supported operators (e.g.,notin,exists) if relevant.internal/store/device.go (4)
119-130: Validate creation of both labels and annotations JSONB indexes.
From a design standpoint, creating two separate JSONB GIN indexes can be beneficial for read-heavy workloads but adds write overhead. Confirm that both label-based and annotation-based queries exist and justify the overhead before finalizing.
458-458: Narrow the scope ofutil.EnsureMap.
While ensuring attachments remain valid, consider verifying whether an empty map is always the desired fallback. In certain contexts, preserving anilmap might provide clearer signals that no annotations exist.
505-505: Use descriptive logging for successful or failed updates to rendered config.
Add a debug or info log to indicate the final annotation changes and newrendered_config. This can simplify troubleshooting.
566-566: Gracefully handle missing or malformed rendered versions.
Though you already check for a missingapi.DeviceAnnotationRenderedVersion, consider applying robust error handling for malformed or non-integer versions that might appear if external scripts or migrations insert irregular data.test/integration/store/fleet_test.go (3)
236-237: Check multi-value label usage.
Ensure that theLabelSelectorcreated frommap[string]string{"key": "value-1"}meets your multi-value or partial matching needs. You might consider a match expression if selecting across multiple label values.
296-301: One-liners for verifying negative results.
The test effectively shows how no fleets are returned. Consider adding an additional negative test with partial label overlap as well, if applicable.
324-329: Consider adding partial key tests for DoesNotExist.
Though you confirm all resources are returned when the key is absent, consider partial matches or substring checks if your system might handle them.docs/developer/architecture/field-selectors.md (4)
22-31: Clarify the repeated word usage in the heading.
"Using Hidden and Private Selectors" is repeated in the heading and description on line 22. Remove any unintended duplication or unify the heading for clarity.-#### Using Hidden and Private Selectors +#### Hidden and Private Selectors🧰 Tools
🪛 LanguageTool
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private"``` *hidden`: The selector will not be exposed to end...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. *private: The selector cannot be directly used by...(UNLIKELY_OPENING_PUNCTUATION)
52-55: Resolve potential hard tabs in the code snippet.
Markdownlint flagged these lines for using hard tabs. Convert to spaces to maintain consistency across documentation.- return []selector.SelectorName{ - selector.NewSelectorName("metadata.name"), - selector.NewSelectorName("metadata.alias"), - } + return []selector.SelectorName{ + selector.NewSelectorName("metadata.name"), + selector.NewSelectorName("metadata.alias"), + }🧰 Tools
🪛 Markdownlint (0.37.0)
52-52: Column: 1
Hard tabs(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs(MD010, no-hard-tabs)
61-61: Resolve potential hard tabs in ListSelectors snippet.
Same reason as above – convert hard tabs to spaces.🧰 Tools
🪛 Markdownlint (0.37.0)
61-61: Column: 1
Hard tabs(MD010, no-hard-tabs)
68-68: Ensure subject-verb clarity.
The note bullet might be missing a subject. Consider rephrasing to clarify that replacingNewSelectorNamewithNewHiddenSelectorNameis a recommended approach.🧰 Tools
🪛 LanguageTool
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > -NewSelectorNamecan be replaced with `NewHiddenSelectorName...(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
68-68: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
docs/user/field-selectors.md (1)
49-49: Consider adding more examples for label selectors.While the examples demonstrate the transition from field selectors to label selectors (
-lflag), it would be helpful to showcase more complex label selector examples using the new operators (exists, not exists, in, notin).Add examples like:
flightctl get devices --field-selector 'metadata.owner=Fleet/pos-fleet, status.updated.status in (Unknown, OutOfDate)' -l 'region=us' + +# Example 4: Using advanced label operators +flightctl get devices -l 'environment in (staging,prod),region exists,!deprecated'Also applies to: 57-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
api/v1alpha1/openapi.yaml(0 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(0 hunks)api/v1alpha1/util.go(2 hunks)docs/developer/architecture/field-selectors.md(2 hunks)docs/user/field-selectors.md(3 hunks)go.mod(0 hunks)internal/api/client/client.gen.go(0 hunks)internal/api/server/server.gen.go(0 hunks)internal/cli/get.go(1 hunks)internal/flterrors/flterrors.go(1 hunks)internal/service/certificatesigningrequest.go(1 hunks)internal/service/device.go(3 hunks)internal/service/enrollmentrequest.go(1 hunks)internal/service/fleet.go(1 hunks)internal/service/repository.go(1 hunks)internal/service/resourcesync.go(1 hunks)internal/service/templateversion.go(2 hunks)internal/store/certificatesigningrequest.go(2 hunks)internal/store/common.go(1 hunks)internal/store/common_test.go(0 hunks)internal/store/device.go(7 hunks)internal/store/enrollmentrequest.go(2 hunks)internal/store/fleet.go(4 hunks)internal/store/model/certificatesigningrequest.go(2 hunks)internal/store/model/device.go(2 hunks)internal/store/model/enrollmentrequest.go(2 hunks)internal/store/model/fleet.go(2 hunks)internal/store/model/jsonfield.go(2 hunks)internal/store/model/repository.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/resourcesync.go(2 hunks)internal/store/model/selectors.go(2 hunks)internal/store/model/selectors_test.go(4 hunks)internal/store/model/templateversion.go(3 hunks)internal/store/repository.go(2 hunks)internal/store/resourcesync.go(2 hunks)internal/store/selector/annotation.go(1 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(18 hunks)internal/store/selector/field_test.go(4 hunks)internal/store/selector/label.go(1 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/selector.go(7 hunks)internal/store/selector/selector_test.go(4 hunks)internal/store/selector/types.go(2 hunks)internal/store/store.go(2 hunks)internal/store/templateversion.go(1 hunks)internal/tasks/fleet_rollout.go(2 hunks)internal/tasks/fleet_selector.go(3 hunks)internal/tasks/resourcesync.go(2 hunks)internal/util/util.go(1 hunks)pkg/k8s/selector/labels/selector.go(1 hunks)pkg/queryparser/sqljsonb/sqljsonb.go(1 hunks)test/integration/store/device_test.go(4 hunks)test/integration/store/enrollmentrequest_test.go(2 hunks)test/integration/store/fleet_test.go(9 hunks)test/integration/store/repository_test.go(2 hunks)test/integration/store/resourcesync_test.go(3 hunks)test/integration/tasks/repotester_conditions_test.go(2 hunks)
💤 Files with no reviewable changes (6)
- go.mod
- internal/store/common_test.go
- api/v1alpha1/types.gen.go
- api/v1alpha1/openapi.yaml
- internal/api/server/server.gen.go
- internal/api/client/client.gen.go
🧰 Additional context used
🪛 LanguageTool
docs/developer/architecture/field-selectors.md
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private" ``` *hidden`: The selector will not be exposed to end...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. * private: The selector cannot be directly used by...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > - NewSelectorName can be replaced with `NewHiddenSelectorName...
(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
docs/developer/architecture/field-selectors.md
66-66: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
67-67: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
68-68: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
52-52: Column: 1
Hard tabs
(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs
(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs
(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs
(MD010, no-hard-tabs)
56-56: Column: 1
Hard tabs
(MD010, no-hard-tabs)
57-57: Column: 1
Hard tabs
(MD010, no-hard-tabs)
61-61: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (182)
api/v1alpha1/spec.gen.go (1)
203-234: Consider verifying the correctness of the new Swagger specification.
These lines consist of Base64-encoded, gzipped JSON data representing the updated Swagger specification. Any corruption or mismatch could surface only at runtime or when generating client code. It is advisable to run a quick verification to confirm that the decoded spec is valid and complete.
Run this script to decode and validate the Swagger JSON:
internal/store/common.go (2)
70-75: Validate error handling in FieldSelector parse flow.
The code checks for errors returned by listParams.FieldSelector.Parse, but be mindful that subsequent parse errors could arise from invalid field definitions or uninitialized selectors. The current logic looks good, though consider adding logging to aid troubleshooting in production scenarios.
87-89: Confirm annotation-based filtering edge cases.
Ensure all annotation-based filtering scenarios are explicitly tested, for instance, annotations containing special characters or nested JSON. Proper coverage will reduce regression risk if additional features are added later.
internal/store/selector/types.go (4)
86-88: Interfaces for extended usage.
Exposing SelectorName as an interface is forward-thinking. It promotes flexibility when creating custom selector name implementations.
90-94: Potential future expansions.
selectorName is a straightforward struct wrapper. If you foresee more fields or behaviors (e.g., metadata, validation), consider ensuring the struct is easy to extend.
95-98: Trimming whitespace is a good practice.
Using strings.TrimSpace ensures consistent naming internally, reducing subtle bugs from accidental trailing spaces in user input.
110-113: Consistent naming for hidden selectors.
The constructor NewHiddenSelectorName is consistent with the existing naming strategy. Include it in your broader developer docs to guide usage.
pkg/queryparser/sqljsonb/sqljsonb.go (18)
1-17: Review of package definitions and high-level structure.
This file introduces a new SQL parsing subsystem. The overall layout is clear, and modular definitions of types and handlers make future extensions straightforward.
18-21: FunctionResult correctness.
FunctionResult is minimal and effective. Verify that your code always sets both Query and Args to avoid potential nil dereferences during usage.
[approve]
23-28: handler struct usage.
usedBy is an interesting approach for controlling which functions can call a given function. Make sure you have test coverage for unauthorized calls to ensure the correct error is thrown.
34-52: withPrecedingKeyQuery verification.
This function ensures the first argument is a function 'K'. The checks look robust. Unit tests should confirm the error scenario triggers properly when 'K' is absent or replaced by 'V'.
54-63: withNoValues verification.
The logic forbids values in the function. Ensure you have test coverage for scenarios where an unexpected value sneaks in or the user passes an empty function call.
65-72: Tokenizer overrides.
WithTokenizer(tokenizer) fosters custom tokenization logic. Provide an example in your docs so others can easily adopt it for specialized edge cases or custom parsing rules.
74-87: Override function approach.
WithOverrideFunction is a clean extension point. Proper error handling is included to detect absent functions. This ensures consistency if overrides are configured incorrectly.
100-150: NewSQLParser default functions.
You define a suite of default functions (AND, OR, NOT, CONTAINS, etc.). This is an excellent built-in set. If you run into performance concerns, consider caching partial parse results for frequently used components.
152-154: parser struct usage.
This struct cements the association between parser and SQLParser. The design is consistent and helps separate concerns for the actual parse logic.
156-198: Parse method detail.
The parse method is robust:
- It handles nil input gracefully.
- It constructs function sets from
sp.funcs. - The validation for empty root arguments is well done.
Ensure concurrency is safe if Parse is called in parallel.
199-240: dispatcher method checks.
Dispatcher loops through each verification, ensuring the function is valid for the given scope. The approach is solid. Keep an eye on performance if the verification chain grows too large or is invoked repeatedly.
242-250: queryAnd function.
Implementation is straightforward. The parentheses ensure correct operator precedence in compound queries. Keep it consistent with other logical operators (e.g., OR).
252-260: queryOr function.
Similar to queryAnd, no issues spotted. The approach is consistent.
262-271: queryNot function.
The function checks argument count and wraps the query in NOT (...). This is correct.
284-294: queryExists operator.
? checks for a key in a JSON object. Consider verifying the JSON structure or ensuring that empty string keys are handled. The code is correct for typical usage.
296-309: queryKey function.
Verifies column name validity. If you add multi-level JSON fields, you might want to parse them further. Works fine for single-level field references.
311-320: queryValue function.
Generates a parameter placeholder. This is straightforward. Ensure that the parameter is sanitized if user input can pass arbitrary data in multi-tenant contexts.
328-344: Argument count validation.
validateArgsCount is universal. For frequent usage, you might unify logic with the query parser’s built-in validations. This is still fine for clarity.
internal/store/resourcesync.go (3)
49-51: Immediate error return from AutoMigrate is good practice.
Failing early avoids partially migrated states. Ensure that the returned error is logged to reveal the cause if migrations fail in production.
53-64: Index creation for labels.
Using GIN indexes for JSONB columns is correct for Postgres. The fallback path for non-Postgres is consistent with GORM’s index approach. Good use of DB-specific optimization.
215-215: Annotations explicitly set to nil on createOrUpdate.
Setting resourceSync.Annotations = nil ensures a clean slate unless explicitly provided. Confirm that existing DB rows won’t lose data inadvertently if an update is intended. Possibly add a comment clarifying why they are niled out.
internal/store/selector/label.go (9)
1-15: Package structure overview.
The new selector package for label selectors is cohesive. The approach using github.com/flightctl/flightctl/pkg/k8s/selector aligns well with Kubernetes-style filtering.
16-41: LabelSelector struct design.
The struct fields field, parser, and selector are clearly partitioned. Good job ensuring each has a distinct role in label resolution. Document the creation flow thoroughly for new contributors.
43-72: LabelSelectorFromMap variants.
Offering NewLabelSelectorFromMap and its OrDie variant is convenient. The invert logic is a neat addition for flipping equality operators.
74-91: NewLabelSelector from strings.
Parsing user-provided label selectors can be error-prone. You handle invalid syntax by wrapping it in NewSelectorError. This is excellent for consistent error reporting.
93-113: Parse method with field resolution.
The resolution of label fields and the check for jsonb type is a strong safeguard. If custom DB columns store labels differently, we can add additional type checks or error messages.
115-177: Building the parser with overrides.
Attaching ls.queryField to the "K" function is a neat override, ensuring all label key references route through a known function. The logic is robust.
179-224: Tokenize method for label requirements.
Iterating over the requirements from the selector.Selector is well structured. The multi-requirement scenario is handled by injecting an AND token. Perfect for combining label constraints.
226-283: parseRequirement method logic.
Comprehensive coverage of operators (Exists, DoesNotExist, =, !=, In, NotIn). Good job capturing multiple values with an OR token set. Wrapping negative conditions in NOT clarifies the final query.
285-309: Key and pair token creation.
Storing keys and values as JSON means the final query uses @> or ?. This selection method elegantly merges label logic with your JSON-based storage approach.
internal/store/repository.go (1)
50-80: Ensure index creation logic is properly idempotent
The logic checks for existing indexes before creating them, which is a good practice. Verify that the index naming doesn't conflict with any other indexes.
internal/store/selector/annotation.go (6)
1-20: Great introduction of the AnnotationSelector
The overall structure is well organized, and the imports are consistent with the usage.
118-180: Robust error handling
Multiple error checks are well integrated. This pipeline provides clear user feedback in each error condition.
230-287: Comprehensive operator support
Covers exists, not-exists, equals, not-equals, in, and not-in thoroughly. Ensure edge cases like empty value arrays are tested.
289-295: Key token generation
Implementation is straightforward and consistent with other token creation functions.
296-302: Escape key/value
Ensure any special JSON characters are escaped before inserting them into the JSON string.
303-313: Column name verification
Defensive check on argument count is good practice. The function’s concise return ensures clarity.
internal/store/selector/selector.go (7)
80-88: Parsing selector tag
Retrieving and applying parsed options is sensible. Confirm that partial or malformed tags won't cause panics or undefined behavior.
Line range hint 95-132: Nested selector resolution
Array indexing is well handled, especially with 1-based indexing for PostgreSQL. Confirm there's no off-by-one confusion in other interactions.
137-151: Prohibit casting
Explicitly disallowing :: casting is protective. Ensure consistency of error messages across other modules.
185-187: Explicit assignment
Setting resolvedField.Name = selectorName clarifies the field-to-user selector mapping.
236-245: Visibility filtering
Excluding hidden selectors from the user-facing error message is a solid approach for security and user clarity.
264-273: Hidden vs. regular selectors
The hidden option effectively shields certain fields. Ensure the logic consistently prevents hidden selector usage in queries.
338-364: Comprehensive parsing
Splitting and trimming the selector tag is well done. Confirm that edge cases like rogue commas or extra whitespace are handled gracefully.
internal/store/certificatesigningrequest.go (1)
46-76: Migrating and indexing
Checking for the index prior to creation is robust. Error handling appears consistent.
internal/service/device.go (2)
74-77: Explicit label selector creation
Likewise, using NewLabelSelector is straightforward. Confirm that error handling and logs match those for field selectors.
93-93: Include FieldSelector
Combining both FieldSelector and LabelSelector stubs out a comprehensive filtering approach.
internal/store/fleet.go (4)
14-14: Use of the selector package
This import looks correct and aligns with the new filtering approach. No issues found.
59-89: AutoMigrate and GIN index creation look solid
The implementation properly checks if the indexes already exist and handles both PostgreSQL-specific and other dialects. This is a clean approach.
244-245: Potential nil pointer risk
*util.SetResourceOwner(api.FleetKind, name) could be nil if name is empty or if SetResourceOwner encounters an edge case. Recommend verifying that name is non-empty before dereferencing.
510-510: Switching from LabelArrayToMap to EnsureMap
Using util.EnsureMap is a more explicit way to guarantee a valid map for annotations. Good move for readability and safety.
internal/store/selector/field.go (11)
14-14: Importing selector library
The new import for github.com/flightctl/flightctl/pkg/k8s/selector is consistent with the shift to structured selectors.
21-25: Introduction of privateSelectors
Having a boolean flag for private selectors provides flexibility. This design is clean and maintains backward compatibility.
30-37: WithPrivateSelectors option
Enabling or disabling private selectors via a functional option pattern is good for clarity and extensibility.
39-56: NewFieldSelectorFromMapOrDie
Invoking a panic on error can be acceptable in specific internal or test scenarios but ensure this is not used in production-critical paths.
58-87: NewFieldSelectorFromMap
A concise utility for converting maps to selectors. Logic looks sound—handle the invert operator carefully to avoid confusion.
89-109: NewFieldSelectorOrDie
Similar note as above about panics—usable for scenarios where failure indicates a programming error.
111-135: NewFieldSelector
Simple, readable constructor for field selectors. Good synergy with the other factory functions.
143-159: Add method merges requirements
Approach to combining selectors is straightforward and well-defined.
160-188: Parse method for SQL query generation
Leverages the parser to convert the selector into a database-friendly statement. Ensure thorough testing of edge cases (nested JSON, arrays, etc.).
241-241: Error messages for parse failures
Detailed error messages on invalid selectors or values improve debuggability. The approach is consistent with robust error handling.
Also applies to: 257-257, 267-267, 276-276
354-358: Restricted private fields
Refusing to parse selectors marked with "private" unless privateSelectors is true is a coherent approach to selectively expose fields.
internal/tasks/fleet_selector.go (4)
12-12: Importing store/selector
Aligns with the updated label/field selector usage in tasks.
121-122: Replacing Labels with LabelSelector
Using selector.NewLabelSelectorFromMapOrDie(...) is a clean approach. Be mindful of potential panics on malformed input.
237-238: Introducing FieldSelector for disowning devices
Switch from Owners to FieldSelector for device ownership is consistent with the new filtering approach. Implementation looks correct.
246-249: Combining label and field selectors
Using both LabelSelector and FieldSelector ensures precise scoping. Confirm that each condition is tested, especially the invert (true) scenario.
internal/store/model/resource.go (2)
26-28: Use JSONB for labels
Shifting from arrays to JSONB expands querying flexibility and indexing options. Ensure to handle backward compatibility if older entries exist.
30-32: Use JSONB for annotations
Like labels, JSONB-based annotations also enable robust indexing. Keep an eye on potential overhead when storing large annotation objects.
internal/flterrors/flterrors.go (1)
21-24: New errors for label and annotation selectors
The new error variables (ErrLabelSelectorSyntax, ErrLabelSelectorParseFailed, etc.) improve granularity in error handling.
internal/store/model/jsonfield.go (4)
61-71: Generic JSONMap is well-structured.
Declaring JSONMap[K comparable, V any] and providing a constructor ensures type safety and easier usage with JSONB columns. Handling nil via make(JSONMap[K, V]) avoids potential runtime issues with uninitialized maps.
73-86: Validate broader data types or unsupported usage scenarios.
The switch statement only handles []byte, string, and nil. If the stored JSON is unexpectedly another type, the error path is triggered. This is correct, but consider whether you want to provide more descriptive messaging or fallback logic for potentially overlooked database column situations.
88-96: Efficient unmarshal approach.
scanBytes(src) ensures a clear separation of concerns between the different source types and the actual JSON unmarshalling. This is a clean pattern, making the code easier to maintain and test.
98-110: Sufficient coverage for driver.Valuer implementation.
Returning nil for an empty map and properly marshaling JSON data is well done. Indeed, returning a nil driver value accurately represents an empty or non-initialized map in the database.
internal/store/model/enrollmentrequest.go (2)
50-51: Robust default maps for labels and annotations.
Using lo.FromPtrOr(resource.Metadata.Labels, make(map[string]string)) ensures no nil references occur. This pattern consistently avoids panics and subsequent complications in JSON operations.
75-76: Consistent nil-safe pointer usage.
Transforming labels and annotations into pointers via lo.ToPtr(util.EnsureMap(...)) is consistent with the approach used in other model functions. This prevents discrepancies in nil handling across the API boundaries.
internal/store/model/certificatesigningrequest.go (2)
50-51: Consistent initialization pattern adopted.
Using lo.FromPtrOr(resource.Metadata.Labels, make(map[string]string)) and lo.FromPtrOr(resource.Metadata.Annotations, make(map[string]string)) aligns with the overall codebase approach, ensuring stable label/annotation storage even for empty inputs.
75-76: Well-handled pointer assignment for metadata fields.
lo.ToPtr(util.EnsureMap(csr.Resource.Labels)) and lo.ToPtr(util.EnsureMap(csr.Resource.Annotations)) ensure the resulting API object does not contain nil maps. This is especially important when serializing or further processing these fields downstream.
internal/store/model/fleet.go (2)
53-54: Graceful handling of empty label and annotation maps.
The use of lo.FromPtrOr is consistent with other models and prevents errors from nil references, making the code uniform across the project.
98-99: Ensuring robust pointer usage for labels and annotations.
Similar to other models, converting maps to pointers using lo.ToPtr and util.EnsureMap ensures the Fleet’s metadata never contains problematic nil maps in the API output.
internal/store/store.go (1)
8-8: Use consistent naming for imports and validate usage.
Importing "github.com/flightctl/flightctl/internal/store/selector" is fine here, but ensure that the references to this package are consistent throughout the project and well-documented.
internal/store/model/templateversion.go (3)
17-22: Embrace JSONB for consistent metadata.
Switching from arrays to JSONMap[string, string] for Labels and Annotations clarifies the data structure and enables more robust queries. Ensure there aren’t any leftover references expecting arrays elsewhere in the code.
67-68: Initialize labels and annotations with defaults to prevent nil issues.
Using lo.FromPtrOr(resource.Metadata.Labels, make(map[string]string)) is a good way to avoid nil maps. Confirm that test cases cover the scenario where these fields are not provided.
97-98: Confirm correct handling when labels are absent.
Using lo.ToPtr(util.EnsureMap(t.Labels)) is a clean way to ensure the map is never nil. Verify that legacy code and external integrations also expect a valid map under all circumstances.
test/integration/tasks/repotester_conditions_test.go (1)
26-26: Case-insensitive check is more robust.
The new condition verifying strings.EqualFold(v, "OK") is helpful for ignoring case variations. Ensure that other logic also supports consistent case-insensitive checks as needed.
internal/store/model/repository.go (2)
56-57: Initialize metadata fields to avoid nil map issues.
Using lo.FromPtrOr(...) ensures that Labels and Annotations default to empty maps if none are provided. This approach prevents NPEs and simplifies usage.
115-116: Validate controlled usage of hidden/secure fields.
util.EnsureMap(f.Resource.Labels) and util.EnsureMap(f.Resource.Annotations) effectively handle missing data. Confirm that sensitive data—if any—remains properly masked or restricted to avoid accidental exposure.
internal/store/model/selectors_test.go (4)
41-47: Ensure clarity on selector normalization logic.
This block correctly ensures that each selector key is consistently wrapped in selector.NewSelectorName(s.String()) before looking up its corresponding type. This helps avoid mismatches between raw strings and selector name types. The logic here appears solid and maintains a consistent approach to normalized keys.
67-67: No issues with rootName assignment.
Setting the rootName with selector.NewSelectorName(schemaName) is consistent with the new normalized naming approach. This ensures alignment with all subsequent references to the root path.
87-87: Robust field path creation.
Using selector.NewSelectorName(path.String() + "." + name) is a neat approach that keeps path building consistent and reduces string-manipulation errors for nested fields.
126-126: Safe array path extension.
Appending [] for array fields in the path (e.g., fieldPath.String()+"[]") is a clear and consistent way to differentiate collection paths from scalar paths.
internal/store/model/resourcesync.go (2)
50-51: Initialize labels and annotations safely.
Using lo.FromPtrOr(resource.Metadata.Labels, make(map[string]string)) is a good way to prevent nil maps and guarantee safe usage of labels and annotations. This promotes more robust data handling.
80-81: Confirm pointer conversion for metadata fields.
Switching to lo.ToPtr(util.EnsureMap(...)) ensures a valid map pointer is always returned. This can help avoid nil-pointer dereferences when working with metadata in other parts of the code.
internal/store/model/device.go (2)
84-85: Initialize labels and annotations safely.
Similarly, the usage of lo.FromPtrOr here guarantees that any nil labels or annotations are converted into safe, empty maps. This approach minimizes potential runtime errors.
128-129: Pointer-based map conversion aligns with usage.
Converting labels and annotations to pointers with lo.ToPtr(util.EnsureMap(...)) is consistent with the rest of the codebase, ensuring that metadata fields are always valid references.
internal/store/selector/selector_test.go (3)
30-33: Map multiple selectors to actual fields.
Mapping "mappedselector" to multiple field selectors (model.field6 and model.field17) provides additional flexibility in the testing model. Ensure that related tests accurately verify both fields.
78-85: Consistent usage of NewSelectorName in ListSelectors.
Returning selectors via NewSelectorName(...) throughout ensures consistency. This block exemplifies the new standardized approach to selector naming and helps avoid mistakes with plain strings.
153-153: Confirm coverage for unknown selectors case.
The check for "unknownselector" returning an error is essential negative testing. Make sure any future expansions to custom selectors likewise cover unknown or malformed scenarios.
internal/store/model/selectors.go (8)
18-21: Looks great converting these selector keys to typed names.
Using selector.NewSelectorName(...) for device status fields is consistent and helps maintain type-safety. No issues found here.
24-24: Good usage of typed selector names for fleet specs.
This approach unifies how selector names are handled across resources.
27-28: Check for consistent usage of Boolean selectors.
selector.NewSelectorName("status.approval.approved") maps to a selector.Bool, which is correct for a true/false field. Just ensure that any code referencing this expects a boolean.
31-31: Repository field usage is suitably typed.
No concerns here—LGTM.
34-35: Spec type and URL as string selectors.
They align well with common string-based fields.
38-38: Certificate strings are consistently typed.
If there are length constraints or format validations, consider verifying them at the application or database level.
44-47: Good approach to mapping multiple field names.
This provides an easy way to return multiple potential matches for a single selector name. No issues detected.
68-68: Appending custom selector names to the standard set.
The approach of adding "metadata.nameoralias" ensures backward compatibility. Keep it up for consistent naming.
internal/store/selector/label_test.go (3)
10-32: Comprehensive coverage of label selector operations.
These tests ensure we handle each valid operator and syntax variation properly. Nicely done including corner cases (like !key).
34-109: Robust query tokenization validation.
You’re thoroughly verifying that each label selector string translates to the correct query tokens. This is an excellent approach to prevent regressions.
111-184: Map-based label selector generation.
It’s valuable that you test both single and multiple key-value pairs, plus the empty map scenario. Great job ensuring each scenario matches the expected query tokens.
pkg/k8s/selector/labels/selector.go (1)
73-79: Documentation is clear and aligns with new syntax grammar.
Appreciate the detailed breakdown of the valid operator forms. This helps maintainers and future contributors.
internal/store/selector/annotation_test.go (3)
10-32: Annotation selector tests mirror label selector testing.
They ensure consistent syntax handling for annotation-based queries. Good job covering positive and negative scenarios.
34-114: Validation of query generation for annotation operations.
Thorough coverage of valid and invalid inputs ensures correctness of the final tokenized representation. This is an excellent approach to catch subtle parsing issues.
116-184: Testing NewAnnotationSelectorFromMap with multiple scenarios.
Well done verifying various key-value pairs, including empty input. This approach demonstrates the selector’s reliability and flexibility.
internal/service/templateversion.go (2)
58-63: Validate label selector usage.
When creating a label selector from the request parameters, ensure that the upstream code confirms that the label format is correct and not ambiguous (e.g., empty strings or invalid keys). This helps to avoid confusion during subsequent label-based searches.
110-113: Check for potential data races in batch deletion.
During the batched iteration over TemplateVersions, ensure no concurrent modifications or parallel deletions that could lead to unexpected states. If concurrency is not a concern here, this can be safely ignored.
internal/store/templateversion.go (2)
44-46: AutoMigrate approach looks good.
AutoMigrate is appropriate for ensuring your table schema remains up-to-date. Consider adding structured logging around this migration for better traceability if something goes wrong in production.
61-73: Confirm “postgres” case sensitivity.
When checking if s.db.Dialector.Name() == "postgres", ensure that the returned name is consistently "postgres" across all environments. Otherwise, consider normalizing the string or matching case-insensitively.
api/v1alpha1/util.go (2)
8-8: Import statement usage is consistent.
The strings package import is appropriate for string building in your new functions.
274-291: Enhance test coverage for MatchExpressionsToString.
This utility function is crucial for clarity in labeling logic. Ensure you have tests covering edge cases, such as zero expressions, multiple expressions, or invalid operators within an expression.
test/integration/store/enrollmentrequest_test.go (2)
13-13: Selector import recognized.
Importing selector here aligns with the overall shift in using structured selectors. Good job maintaining consistency across test and application code.
161-162: Verify the label selector logic in tests.
The new use of LabelSelector from selector.NewLabelSelectorFromMapOrDie is helpful. Check if usage covers scenarios with invalid or partial labels.
internal/store/enrollmentrequest.go (3)
47-58: Check transactional consistency for index creation.
Creating indexes outside of a transaction can leave partial changes if multiple statements fail. Consider wrapping the index creation in a transaction if your migration processes allow it. This ensures atomicity and consistent state.
60-71: Validate index existence checks for concurrency.
While HasIndex checks for the presence of an index, concurrently running migrations in multiple threads or services might race. Consider using the GORM migrator in a more controlled environment or adding safe-guards for concurrency.
207-207: Annotations set to nil on createOrUpdate.
Ensuring enrollmentrequest.Annotations is always reset to nil might be a design choice to unify the handling of annotations. Verify if this is intentional for all usage scenarios, particularly when an Annotations field has meaningful default values.
internal/tasks/resourcesync.go (2)
16-16: New selector package import.
The import of the selector package is consistent with the overall PR changes to adopt the new selector framework.
98-99: Using FieldSelector to filter by owner.
This approach replaces older ownership filters with a robust field selector. Ensure that the metadata.owner field is consistently set within all resources to avoid unexpected filtering issues.
test/integration/store/resourcesync_test.go (2)
12-12: Selector import for tests.
This aligns test coverage with the new selector functionality. Make sure to add tests that exercise other advanced field or label filtering scenarios if needed.
115-117: Verifying FieldSelector usage in deletion tests.
This test ensures that fleets with a particular metadata.owner are handled properly on a Delete operation. Confirm that private selectors introduced by WithPrivateSelectors() are thoroughly tested for correct scoping.
internal/service/repository.go (3)
69-71: Robust error handling on field selector parsing.
Early returns with HTTP 400 errors are appropriate. Just ensure you maintain consistent messaging across the service for parse failures.
76-81: Label selector parsing complements field selector coverage.
Similar to the field selector approach, this ensures invalid label queries are gracefully handled. Good maintainability step.
87-87: LabelSelector usage in ListParams.
Storing LabelSelector in ListParams centralizes the logic for label-based filtering. This is consistent with the new approach across the PR. No issues found.
internal/service/resourcesync.go (5)
69-69: Use of pointer for fieldSelector
Defining fieldSelector as a pointer allows for distinguishing between a nil selector and an empty selector, which is beneficial for conditional handling. This change looks good.
71-71: Field selector parsing
Ensuring an appropriate error is returned when parsing fails helps with debugging malformed field selectors. Good addition.
76-76: Introduction of labelSelector
Adopting LabelSelector as a pointer is consistent with the new pattern. It helps avoid unnecessary instantiation when no labels are specified.
77-81: Nil check before labelSelector instantiation
Checking for request.Params.LabelSelector before creating a new labelSelector is a solid approach to prevent unnecessary parsing or nil-pointer dereferences.
87-87: Populating LabelSelector in listParams
Propagating the parsed labelSelector to the ListParams ensures consistency throughout the store layer.
test/integration/store/repository_test.go (2)
11-11: New import for selector package
Bringing in selector aligns the test with the enhanced label selector approach in the store layer.
172-173: Switch to LabelSelector in list parameters
Using selector.NewLabelSelectorFromMapOrDie provides more robust label matching logic, improving test accuracy for label-based queries.
internal/service/fleet.go (4)
82-82: Pointer usage for fieldSelector
Defining fieldSelector as a pointer offers better clarity when no field selector is provided, avoiding unnecessary parsing.
84-84: Field selector parsing
Error handling here is appropriate. Returning a clear message helps users quickly identify invalid selectors.
89-95: Pointer usage for labelSelector
Similar to the field selector, using a pointer for labelSelector helps distinguish between absent and explicitly empty selectors. The associated parsing and error handling logic is consistent and robust.
100-100: Enhanced ListParams with LabelSelector
Populating LabelSelector in listParams ensures a unified approach to label matching across the codebase.
internal/service/enrollmentrequest.go (4)
124-124: Pointer-based fieldSelector
Using a pointer enables the code to check for nil, enhancing clarity when no field selector is present.
126-126: Field selector error messaging
Returning descriptive errors for parsing issues improves debugging and user feedback.
131-137: Pointer-based LabelSelector
Consistently adopting the new LabelSelector pointer pattern is a solid approach. The checks for nil input before parsing reduce potential runtime errors.
142-142: Preserving LabelSelector in list parameters
Assigning labelSelector to the ListParams structure aligns with the improved filtering strategy.
internal/store/selector/field_test.go (7)
10-10: Consistent renaming for clarity
Renaming the function from TestFieldTypes to TestFieldSelectorTypes helps maintain consistency with the new selector terminology and improves readability.
48-53: Robust verification of parsed selectors
Creating a field selector, then parsing it against a model, is a clear way to verify correctness for valid inputs. This chunk effectively confirms the parser handles multiple data types.
61-67: Clear negative test coverage
Ensuring the parser fails appropriately for invalid selector strings strengthens the overall reliability. This approach properly checks for errors at parsing.
74-74: Function rename aligns with new naming scheme
Renaming TestOperations to TestFieldSelectorQueries accurately reflects a more descriptive and consistent naming convention for selector-based test queries.
244-256: Thorough comparison of tokenized selectors
Tokenizing and then comparing against the expected queryparser output helps prevent regressions. This test methodology solidifies confidence in the parser’s correctness.
274-279: Comprehensive invalid operations testing
These negative tests mirror the pattern used for other invalid selectors, ensuring consistent coverage across the board.
285-337: Good use of map-based selector creation
The new TestFieldSelectorMap function thoroughly exercises multiple input combinations and expected outputs, enhancing confidence in map-based selector handling.
internal/service/certificatesigningrequest.go (3)
93-95: Graceful field selector parsing
The introduction of selector.NewFieldSelector with error handling is a clean approach. It promptly returns a 400 status if parsing fails, improving user feedback.
100-106: Adoption of structured label selectors
Switching to NewLabelSelector for label parsing centralizes validation and reduces duplication, ensuring consistent selector usage across the application.
111-111: Unified label selector integration
Populating LabelSelector in ListParams unifies the approach for queries. This aligns with the new selector framework for consistent filtering logic.
test/integration/store/device_test.go (6)
14-14: Selector package import
Importing the new selector package enables stronger, more flexible device filtering in integration tests. This incremental addition is orderly and consistent with the PR’s direction.
282-283: Added label selector for paging test
Using selector.NewLabelSelectorFromMapOrDie verifies label filtering works seamlessly with pagination, boosting test comprehensiveness.
292-293: Field selector usage for status filtering
This test exercises multi-value field selector logic (in (Unknown, Updating)), confirming the parser handles listing devices by custom statuses.
304-306: Filtering by owner field
Demonstrates that metadata.owner can now be targeted with a field selector. This ensures the new selector mechanism can handle ownership-based queries.
313-315: Repeated check for distinct owner
Continues validating per-owner queries with the new field selector. Keeps test coverage consistent with possible ownership patterns.
322-323: Testing an in-operator on the owner field
Verifying multiple owners in a single field selector strengthens confidence in advanced query operations.
internal/store/device.go (4)
287-290: Ensure forced removal of the TemplateVersion annotation is intended behavior.
Removing api.DeviceAnnotationTemplateVersion upon user changes prevents reusing a prior template. Confirm that this logic aligns with the desired workflow.
479-479: Consider concurrency handling with concurrent annotation updates.
The code updates the same annotations map in-place and relies on an optimistic lock via resource_version. Confirm that concurrent updates to annotations won't lead to repeated conflicts or lost changes.
520-520: Validate JSON structure for rendered_applications.
Here you explicitly ensure the field is an empty array if blank. Confirm any potential mismatches between JSON arrays and your deserialization logic in the rest of the code.
106-110: Consider verifying the JSONB index usage for correctness and performance.
While creating a GIN index on the labels JSONB column can significantly improve queries against labels, ensure that this approach aligns with your expected query patterns. GIN indexes might increase storage costs and slow down writes if not frequently utilized.
You could verify that this index is effectively used by running an EXPLAIN on queries that filter by labels in your codebase:
✅ Verification successful
GIN index on labels is appropriate for JSONB operations
Based on the codebase analysis, the GIN index on the labels column is justified:
- The codebase includes JSONB query operations using the
@>containment operator (as seen inpkg/queryparser/sql/sql.go), which benefits from GIN indexes - The query parser supports both
JSONB_CONTAINSandJSONB_NOTCONTAINSoperations, indicating label-based filtering is a core functionality - The Device model structure shows labels are used for metadata filtering and templating (seen in test utilities)
The GIN index will improve performance for these JSONB operations, and the storage overhead is warranted given the apparent centrality of label-based queries in the application's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check query plan on label filters
rg -A 3 'WHERE.*labels.*' | sed -n '/SELECT/p' | while read -r query; do
echo "EXPLAIN $query;" | your_db_client
done
Length of output: 62
Script:
#!/bin/bash
# Search for SQL queries involving labels column
rg -U "labels.*WHERE|WHERE.*labels" -A 5 -B 5
Length of output: 47
Script:
#!/bin/bash
# Search for GORM queries involving labels
rg -U "Where.*labels|\.labels.*Where" -A 5 -B 5
Length of output: 49
Script:
#!/bin/bash
# Look for label-related query methods in device store
ast-grep --pattern 'func ($_ *DeviceStore) $$$labels$$$'
Length of output: 58
Script:
#!/bin/bash
# Check if there are any label-specific methods in the model
ast-grep --pattern 'type Device struct {
$$$
labels $$$
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Look for the Device model definition and labels field
rg -U "type Device struct" -A 15 -B 2
Length of output: 2740
Script:
#!/bin/bash
# Search for any query methods that might use labels
rg -U "func.*Device.*labels" -A 10 -B 2
Length of output: 2733
Script:
#!/bin/bash
# Look for GORM queries with jsonb operators
rg -U "@>|\\?\\||\\?&" -A 5 -B 5
Length of output: 4476
Script:
#!/bin/bash
# Check for label-related database operations
rg -U "db.*labels.*Where|Where.*labels.*db" -A 5 -B 5
Length of output: 55
test/integration/store/fleet_test.go (6)
13-13: Import usage check.
The newly introduced selector import is used for label-based and field-based queries. Verify that all references to selector are needed and that redundant utils have been removed to reduce confusion.
247-255: Verify consistency of match expressions.
You are using api.In with values = ["value-1"]. Confirm that your test covers edge cases like multiple values or uppercase string variants, if relevant.
262-268: Check negative matching logic.
api.NotIn filtering is confirmed to remove the value-1 result. Ensure you're covering corner cases where null, empty strings, or other user-defined labels can appear.
279-284: Test exhaustive coverage for Exists operator.
The test confirms that if the key key is present, the fleet is returned. Include scenarios where multiple labels exist, ensuring no collisions with partial matches.
310-315: Check DoesNotExist corner cases.
The test verifies empty results when the label key does exist. Ensure corner cases like empty label sets or dynamic label generation are covered.
658-660: Check for deprecation or removal of the owner field.
metadata.owner is still tested here with a FieldSelector. Confirm that removing the explicit owner filters from the main code won't break existing queries that rely on it.
internal/util/util.go (1)
272-281: Confirm the fallback strategy in EnsureMap.
This function is straightforward and ensures no nil dereferences for maps. Validate that some code paths do not rely on distinguishing “no map available” (nil) vs. “empty map” (initialized).
docs/user/field-selectors.md (2)
17-17: LGTM! Clear guidance on label querying.
The note effectively communicates the recommended approach of using Label Selectors for label filtering.
70-70: LGTM! Clear error message example.
The error message example accurately reflects the updated list of supported selectors, which aligns with the removal of metadata.labels from field selectors.
internal/tasks/fleet_rollout.go (1)
13-13: LGTM! Clean import addition.
The selector package import is properly added and aligns with the new filtering approach.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements a significant architectural change to enhance Flight Control's filtering capabilities by transitioning from array-based to JSONB-based storage for labels and annotations.
Key changes:
- Transitions database schema from PostgreSQL arrays to JSONB for labels/annotations with GIN indexes for performance
- Implements unified selector framework supporting advanced operators (exists, in, notcontains) for both labels and fields
- Adds hidden/private selector support for internal filtering operations
- Removes redundant filtering mechanisms (owner, status filters) in favor of field selectors
- Introduces proper validation and error handling for JSONB field access and array operations
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
60 file(s) reviewed, 41 comment(s)
Edit PR Review Bot Settings | Greptile
asafbss
force-pushed
the
refactor-label-selector
branch
2 times, most recently
from
499eb45 to
a79fa27
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/integration/store/resourcesync_test.go (1)
236-239:⚠️ Potential issueIncorrect test name
This test case has the same name as another test "List with paging" but actually tests label filtering functionality.
Rename the test to reflect its actual purpose:
-It("List with paging", func() { +It("List with label selector filtering", func() {internal/store/selector/field_test.go (1)
315-319:⚠️ Potential issueFix early return in TestFieldSelectorMap.
The early return on error prevents testing of remaining test cases in the loop. Consider using
continueinstead ofreturn.fr.fieldResolver, err = SelectorFieldResolver(&goodTestModel{}) if err != nil { t.Errorf("error %v (%#v)\n", err, err) - return + continue }
🧹 Nitpick comments (19)
internal/cli/get.go (1)
85-86: LGTM! Consider enhancing the documentation.The updated help text accurately reflects the enhanced filtering capabilities and provides clear examples. The descriptions align well with the new selector framework.
Consider adding a reference to the documentation where users can find the complete list of supported operators and more complex query examples. This could be added as:
- fs.StringVarP(&o.LabelSelector, "selector", "l", o.LabelSelector, "Selector (label query) to filter on, supporting operators like '=', '!=', and 'in' (e.g., -l='key1=value1,key2!=value2,key3 in (value3, value4)').") + fs.StringVarP(&o.LabelSelector, "selector", "l", o.LabelSelector, "Selector (label query) to filter on, supporting operators like '=', '!=', and 'in' (e.g., -l='key1=value1,key2!=value2,key3 in (value3, value4)'). See https://docs.flightctl.com/selectors for the complete list of operators.")internal/store/common.go (2)
70-75: LGTM! Consider enhancing error messages.The field selector implementation is clean and well-structured. The error handling is present, but consider wrapping the error with additional context about the field selector parsing failure.
if listParams.FieldSelector != nil { q, p, err := listParams.FieldSelector.Parse(ctx, lq.dest) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse field selector: %w", err) } query = query.Where(q, p...)
Line range hint
87-94: Consider refactoring to reduce duplication with label selector.The annotation selector implementation correctly mirrors the label selector pattern. However, consider extracting the common logic into a helper function to reduce code duplication.
+func (lq *listQuery) applyMetadataSelector(ctx context.Context, selector *selector.Selector, field string) (*gorm.DB, error) { + q, p, err := selector.Parse(ctx, lq.dest, selector.NewHiddenSelectorName(field)) + if err != nil { + return nil, fmt.Errorf("failed to parse %s selector: %w", field, err) + } + return query.Where(q, p...), nil +} func (lq *listQuery) Build(ctx context.Context, db *gorm.DB, orgId uuid.UUID, listParams ListParams) (*gorm.DB, error) { // ... existing code ... if listParams.LabelSelector != nil { - q, p, err := listParams.LabelSelector.Parse(ctx, lq.dest, - selector.NewHiddenSelectorName("metadata.labels")) - if err != nil { - return nil, err - } - query = query.Where(q, p...) + query, err = lq.applyMetadataSelector(ctx, listParams.LabelSelector, "metadata.labels") + if err != nil { + return nil, err + } } if listParams.AnnotationSelector != nil { - q, p, err := listParams.AnnotationSelector.Parse(ctx, lq.dest, - selector.NewHiddenSelectorName("metadata.annotations")) - if err != nil { - return nil, err - } - query = query.Where(q, p...) + query, err = lq.applyMetadataSelector(ctx, listParams.AnnotationSelector, "metadata.annotations") + if err != nil { + return nil, err + } }internal/store/model/templateversion.go (1)
97-98: Consider removing redundant map validation.The
util.EnsureMapcalls appear redundant since maps are already initialized inNewTemplateVersionFromApiResource. Consider simplifying to:-Labels: lo.ToPtr(util.EnsureMap(t.Labels)), -Annotations: lo.ToPtr(util.EnsureMap(t.Annotations)), +Labels: lo.ToPtr(t.Labels), +Annotations: lo.ToPtr(t.Annotations),Let's verify the safety of this change:
#!/bin/bash # Check if Labels/Annotations can ever be nil at this point ast-grep --pattern 't.Labels = nil' ast-grep --pattern 't.Annotations = nil'internal/store/selector/label.go (1)
38-38: Panic usage
Thepanic(err)pattern inNewLabelSelectorFromMapOrDiecan abruptly terminate the application if there's an error. Consider returning an error instead for better error handling in production systems.internal/store/selector/types.go (1)
101-102: Remove unnecessary string conversions.The
string()calls in bothString()methods are redundant as thenamefield is already a string.Apply this diff:
func (sn selectorName) String() string { - return string(sn.name) + return sn.name } func (sn hiddenSelectorName) String() string { - return string(sn.name) + return sn.name }Also applies to: 116-117
🧰 Tools
🪛 GitHub Check: lint
[failure] 102-102:
unnecessary conversion (unconvert)docs/developer/architecture/field-selectors.md (3)
22-31: Improve markdown formatting and clarity.The documentation is well-structured, but there are some formatting inconsistencies:
- Use consistent list marker style (asterisk instead of dash)
- Remove extra spaces before list markers
- Consider adding examples for both hidden and private selectors separately
- * `hidden`: The selector will not be exposed to end-users during discovery. It will not appear in the list of available selectors. - - * `private`: The selector cannot be directly used by the field-selector. However, it may still be utilized internally by other selectors, such as the label selector. + * `hidden`: The selector will not be exposed to end-users during discovery. It will not appear in the list of available selectors. + * `private`: The selector cannot be directly used by the field-selector. However, it may still be utilized internally by other selectors, such as the label selector.🧰 Tools
🪛 LanguageTool
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private"``` *hidden`: The selector will not be exposed to end...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. *private: The selector cannot be directly used by...(UNLIKELY_OPENING_PUNCTUATION)
52-55: Convert hard tabs to spaces for consistent formatting.The code example uses hard tabs for indentation. Consider using spaces for consistent rendering across different editors and platforms.
- return []selector.SelectorName{ - selector.NewSelectorName("metadata.name"), - selector.NewSelectorName("metadata.alias"), - } + return []selector.SelectorName{ + selector.NewSelectorName("metadata.name"), + selector.NewSelectorName("metadata.alias"), + }🧰 Tools
🪛 Markdownlint (0.37.0)
52-52: Column: 1
Hard tabs(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs(MD010, no-hard-tabs)
68-68: Enhance clarity about selector name variants.The note about
NewHiddenSelectorNamecould be more explicit about when to use each variant and their implications.- > - `NewSelectorName` can be replaced with `NewHiddenSelectorName` to mark the selector as hidden. This ensures it won't be exposed during discovery but can still be used internally (see the explanation above). + > - Use `NewSelectorName` for standard selectors that should be discoverable by users. + > - Use `NewHiddenSelectorName` when you want to create a selector that should not be exposed during discovery but can still be used internally by the system.🧰 Tools
🪛 LanguageTool
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > -NewSelectorNamecan be replaced with `NewHiddenSelectorName...(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
68-68: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
internal/service/templateversion.go (1)
109-113: Consider adding a comment explaining the batch size choice.The code processes template versions in batches of 100 items. Consider adding a comment explaining this choice and any implications.
// Iterate through the relevant templateVersions, 100 at a time, and delete each one's config storage listParams := store.ListParams{ - Limit: 100, + Limit: 100, // Process in small batches to avoid memory pressure FieldSelector: fieldSelector, }test/integration/store/resourcesync_test.go (1)
115-117: Test name could be more descriptiveThe test name "Delete resourcesync success" could be more specific to indicate that it's testing the cascading deletion of owner references.
Consider renaming the test to "Delete resourcesync successfully removes owner references" for better clarity.
internal/service/resourcesync.go (1)
69-81: Consider extracting selector parsing logicThis selector parsing logic is duplicated across multiple service handlers (e.g., repository.go). Consider extracting it into a common helper function.
Create a helper in the common package:
// common/selector.go func ParseSelectors(fieldSelector, labelSelector *string) (*selector.FieldSelector, *selector.LabelSelector, error) { var fs *selector.FieldSelector var ls *selector.LabelSelector var err error if fieldSelector != nil { if fs, err = selector.NewFieldSelector(*fieldSelector); err != nil { return nil, nil, fmt.Errorf("failed to parse field selector: %v", err) } } if labelSelector != nil { if ls, err = selector.NewLabelSelector(*labelSelector); err != nil { return nil, nil, fmt.Errorf("failed to parse label selector: %v", err) } } return fs, ls, nil }internal/store/selector/selector.go (2)
80-88: Extract comment to a constant or remove redundancy.The comment about selector tag parsing safety is repeated multiple times. Consider either:
- Extracting it to a constant if it's important documentation
- Removing it if it's obvious from the code context
185-186: Fix typo in comment."explictly" should be "explicitly"
-// Should not changed, explictly assign it +// Should not change, explicitly assign it🧰 Tools
🪛 GitHub Check: lint
[failure] 185-185:
explictlyis a misspelling ofexplicitly(misspell)internal/store/fleet.go (1)
59-89: Excellent database optimization with GIN indexes!The implementation:
- Properly handles database migration errors
- Creates GIN indexes for efficient JSONB querying
- Handles different database dialects appropriately
The GIN indexes will significantly improve query performance for label and annotation filtering operations.
pkg/queryparser/sqljsonb/sqljsonb.go (4)
34-52: Ensure clarity in error messages and function usage.
WhilewithPrecedingKeyQuerychecks that the first argument is the functionK, the error messages (lines 38 and 42) could be clarified to guide callers toward the correct usage pattern. Consider appending a hint suggesting that the caller should passK(columnName)before using functions likeCONTAINSorEXISTS.
311-320: Consistent argument handling forqueryValue.
WhilequeryValueappends the value as a parameter placeholder, other functions (e.g.,queryContains,queryExists) currently embed arguments directly in the string. For consistency and flexibility, consider always constructing queries with placeholders and storing all dynamic values inArgs.
324-326: Allow more sophisticated column name validation.
Currently,isValidColumnNameonly checks for basic alphanumeric and underscore patterns. If your schema includes quoted identifiers or more complex names, extendcolumnRegexor handle them gracefully.
328-344: Ensure consistent min/max argument checks.
validateArgsCounthandles one or two bounds (minimum and maximum). If future logic requires an exact argument count beyond two bounds, consider refactoring for readability or using a more flexible approach (e.g. variadic checks).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
api/v1alpha1/openapi.yaml(0 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(0 hunks)api/v1alpha1/util.go(1 hunks)docs/developer/architecture/field-selectors.md(2 hunks)docs/user/field-selectors.md(3 hunks)go.mod(0 hunks)internal/api/client/client.gen.go(0 hunks)internal/api/server/server.gen.go(0 hunks)internal/cli/get.go(1 hunks)internal/flterrors/flterrors.go(1 hunks)internal/service/certificatesigningrequest.go(1 hunks)internal/service/device.go(3 hunks)internal/service/enrollmentrequest.go(1 hunks)internal/service/fleet.go(1 hunks)internal/service/repository.go(1 hunks)internal/service/resourcesync.go(1 hunks)internal/service/templateversion.go(2 hunks)internal/store/certificatesigningrequest.go(2 hunks)internal/store/common.go(1 hunks)internal/store/common_test.go(0 hunks)internal/store/device.go(7 hunks)internal/store/enrollmentrequest.go(2 hunks)internal/store/fleet.go(4 hunks)internal/store/model/certificatesigningrequest.go(2 hunks)internal/store/model/device.go(2 hunks)internal/store/model/enrollmentrequest.go(2 hunks)internal/store/model/fleet.go(2 hunks)internal/store/model/jsonfield.go(2 hunks)internal/store/model/repository.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/resourcesync.go(2 hunks)internal/store/model/selectors.go(2 hunks)internal/store/model/selectors_test.go(4 hunks)internal/store/model/templateversion.go(3 hunks)internal/store/repository.go(2 hunks)internal/store/resourcesync.go(2 hunks)internal/store/selector/annotation.go(1 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(18 hunks)internal/store/selector/field_test.go(4 hunks)internal/store/selector/label.go(1 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/selector.go(7 hunks)internal/store/selector/selector_test.go(4 hunks)internal/store/selector/types.go(2 hunks)internal/store/store.go(2 hunks)internal/store/templateversion.go(1 hunks)internal/tasks/fleet_rollout.go(2 hunks)internal/tasks/fleet_selector.go(3 hunks)internal/tasks/resourcesync.go(2 hunks)internal/util/util.go(1 hunks)pkg/k8s/selector/labels/selector.go(1 hunks)pkg/queryparser/sqljsonb/sqljsonb.go(1 hunks)test/integration/store/device_test.go(4 hunks)test/integration/store/enrollmentrequest_test.go(2 hunks)test/integration/store/fleet_test.go(9 hunks)test/integration/store/repository_test.go(2 hunks)test/integration/store/resourcesync_test.go(3 hunks)test/integration/tasks/repotester_conditions_test.go(2 hunks)
💤 Files with no reviewable changes (6)
- internal/store/common_test.go
- internal/api/server/server.gen.go
- api/v1alpha1/types.gen.go
- api/v1alpha1/openapi.yaml
- internal/api/client/client.gen.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (21)
- internal/util/util.go
- test/integration/tasks/repotester_conditions_test.go
- test/integration/store/enrollmentrequest_test.go
- internal/store/model/resource.go
- internal/store/model/enrollmentrequest.go
- internal/store/model/device.go
- internal/store/model/resourcesync.go
- internal/store/resourcesync.go
- test/integration/store/repository_test.go
- test/integration/store/device_test.go
- internal/store/templateversion.go
- internal/store/model/selectors_test.go
- internal/store/model/repository.go
- internal/store/certificatesigningrequest.go
- internal/store/repository.go
- internal/store/model/certificatesigningrequest.go
- internal/store/enrollmentrequest.go
- internal/store/selector/selector_test.go
- internal/store/selector/annotation.go
- internal/store/device.go
- internal/store/model/selectors.go
🧰 Additional context used
🪛 GitHub Check: lint
internal/store/selector/types.go
[failure] 102-102:
unnecessary conversion (unconvert)
[failure] 117-117:
unnecessary conversion (unconvert)
internal/store/selector/selector.go
[failure] 185-185:
explictly is a misspelling of explicitly (misspell)
🪛 LanguageTool
docs/developer/architecture/field-selectors.md
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private" ``` *hidden`: The selector will not be exposed to end...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. * private: The selector cannot be directly used by...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > - NewSelectorName can be replaced with `NewHiddenSelectorName...
(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
docs/developer/architecture/field-selectors.md
66-66: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
67-67: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
68-68: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
52-52: Column: 1
Hard tabs
(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs
(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs
(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs
(MD010, no-hard-tabs)
56-56: Column: 1
Hard tabs
(MD010, no-hard-tabs)
57-57: Column: 1
Hard tabs
(MD010, no-hard-tabs)
61-61: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (56)
internal/store/common.go (1)
78-85: Verify JSONB index creation for labels.
The implementation correctly uses JSONB queries for label filtering. However, ensure that appropriate JSONB indexes are created for the metadata.labels column to maintain query performance.
✅ Verification successful
JSONB indexes are properly created for labels across all relevant tables
The verification shows that GIN indexes are correctly created for the labels JSONB column across all relevant tables:
template_versions: idx_template_versions_labelsrepositories: idx_repositories_labelsresource_syncs: idx_resource_syncs_labelsdevices: idx_device_labelscertificate_signing_requests: idx_csr_labelsfleets: idx_fleet_labelsenrollment_requests: idx_enrollment_requests_labels
Each table has a GIN index specifically created for the labels column using PostgreSQL's GIN index type, which is optimal for JSONB queries. This ensures efficient label-based filtering operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if JSONB indexes exist for labels column
# Search for index creation in migration files
fd -e sql -e go . | xargs rg -A 5 'CREATE\s+INDEX.*labels'
# Search for JSONB column definition
ast-grep --pattern 'type $_ struct {
$$$
Labels JSONB `$_`
$$$
}'
Length of output: 3271
internal/store/model/templateversion.go (2)
67-68: LGTM! Safe handling of optional fields.
The conversion logic properly handles nil cases using lo.FromPtrOr with appropriate empty map defaults.
17-22: LGTM! Verify database migration for JSONB conversion.
The struct changes look good:
- Type change from array to JSONB aligns with the PR objectives
- Proper selector tags for metadata mapping
- Correct GORM tags for PostgreSQL JSONB type
Let's verify the database migration for this change:
api/v1alpha1/spec.gen.go (3)
Line range hint 1-13: LGTM: Package declaration and imports are appropriate.
The package declaration, generated code comment, and imports are correctly structured for an OpenAPI specification file.
Line range hint 237-320: LGTM: Helper functions are well-implemented.
The helper functions for decoding and loading the specification:
- Implement proper error handling
- Use efficient caching mechanism
- Handle external references correctly
21-236: Verify the OpenAPI specification changes.
The Base64 encoded Swagger specification has been updated. Since this is generated code, please ensure:
- The changes reflect the transition to JSONB format for labels and annotations
- The specification was generated using the correct version of oapi-codegen (v2.3.0)
Run the following script to decode and verify the specification:
✅ Verification successful
Base64 encoded Swagger specification is properly generated with JSONB support
The code changes in spec.gen.go are verified to be correct based on the following evidence:
-
The codebase shows proper JSONB implementation for labels and annotations:
- Labels and annotations are stored as JSONB fields in the database models (e.g.,
gorm:"type:jsonb") - Proper JSONB query support is implemented through SQL functions like
jsonb_setandjsonb_contains
- Labels and annotations are stored as JSONB fields in the database models (e.g.,
-
The OpenAPI schema properly defines labels and annotations:
ObjectMetaschema includes labels and annotations fieldsmatchLabelsis defined as a map of key-value pairs
-
The generated code is consistent with the database schema changes:
- Models use
JSONMap[string, string]type for labels/annotations - JSONB operators and functions are properly implemented for querying
- Models use
The Base64 encoded specification in spec.gen.go is correctly generated to support these JSONB-related changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Decode and verify the Swagger specification
# Test 1: Verify the specification can be decoded
echo "Verifying specification can be decoded..."
go run main.go openapi spec decode
# Test 2: Search for JSONB-related changes in the decoded spec
echo "Searching for JSONB-related changes..."
go run main.go openapi spec decode | grep -A 5 -B 5 "jsonb"
Length of output: 463
Script:
#!/bin/bash
# Let's try a different approach to verify the Base64 encoded content
# Extract the Base64 content and decode it
echo "Extracting and decoding Base64 content..."
rg -U "(?s)\`\s*(.*?)\`" api/v1alpha1/spec.gen.go -r '$1' | base64 -d | gunzip -c || true
# Search for JSONB related patterns in the file
echo -e "\nSearching for JSONB patterns in the file..."
rg -i "jsonb|json\s+type|json\s+format" api/v1alpha1/spec.gen.go
Length of output: 485
Script:
#!/bin/bash
# Let's check the repository for documentation about the spec generation process
# and any related configuration files
echo "Searching for OpenAPI/Swagger related documentation..."
rg -i "openapi|swagger|spec.gen" -g "!{*.go,*.mod,*.sum}" --type txt
echo -e "\nSearching for generation configuration files..."
fd -t f "gen|swagger|openapi" -E "*.go" -E "*.mod" -E "*.sum"
echo -e "\nChecking for generation scripts..."
fd -t f "^gen.*\.sh$|^make.*$"
echo -e "\nChecking for related changes in other files..."
rg -i "jsonb|labels|annotations" -g "!{spec.gen.go,*.mod,*.sum}" --type go -A 3 -B 3
Length of output: 66027
Script:
#!/bin/bash
# Let's check the OpenAPI configuration files and generation scripts
echo "Checking OpenAPI configuration files..."
cat api/v1alpha1/openapi.yaml
echo -e "\nChecking spec generation config..."
cat api/v1alpha1/spec.gen.cfg
echo -e "\nChecking for generation scripts in Makefile..."
rg "gen.*spec" Makefile
Length of output: 65957
Script:
#!/bin/bash
# Let's check the schema definitions in the OpenAPI spec for labels and annotations
echo "Checking schema definitions for labels and annotations..."
rg -A 10 "ObjectMeta:|Labels:|Annotations:" api/v1alpha1/openapi.yaml
echo -e "\nChecking for JSONB related database schema changes..."
rg -i "jsonb|json\s+type" -g "*.sql" -g "*.go"
Length of output: 22857
internal/store/selector/label.go (1)
57-59: Return of empty label map might cause over-broad matching
Returning a blank selector for an empty label map may lead to selecting everything by default. This behavior could be unintended in certain scenarios.
internal/store/selector/annotation_test.go (2)
60-62: Expand test coverage for invalid inputs
Currently, only "ke@y" is tested for invalid input. Consider adding additional malformed test cases (e.g., empty strings, nested parentheses, special characters) to improve coverage.
83-84: Potential nil slice access
resolvedFields[0] is used without an explicit check for len(resolvedFields) > 0. In rare corner cases, it might lead to a panic.
internal/flterrors/flterrors.go (1)
21-24: Include more descriptive error messages
Consider appending examples of valid syntax or usage guidelines to these error messages for clarity.
internal/store/model/jsonfield.go (1)
88-96: Nil map assignment post-unmarshal
scanBytes assigns the unmarshaled result directly to *m without ensuring it’s non-nil, which could produce a nil map instead of an empty one.
internal/store/model/fleet.go (1)
53-54: Add label/annotation key/value validation
Consider validating keys and values here to ensure compliance with Kubernetes naming standards for labels and annotations.
internal/store/store.go (1)
133-137: LGTM! Well-structured selector implementation.
The refactoring of ListParams to use dedicated selector types (FieldSelector, LabelSelector, AnnotationSelector) provides a more organized and type-safe approach to handling different types of selectors.
internal/store/selector/types.go (2)
86-88: Consider adding comparison methods to SelectorName interface.
The interface could benefit from additional methods for comparison operations (e.g., Equals(), Compare()). This would ensure consistent comparison behavior across different implementations and make it safer to use as map keys.
194-202: LGTM! Well-designed selector options.
The addition of SelectorOpt and its integration into SelectorField provides a flexible way to associate metadata with selectors.
internal/store/selector/label_test.go (5)
27-27: Verify goodTestModel definition.
The test relies on goodTestModel which is not defined in this file. Ensure it's properly defined and accessible.
#!/bin/bash
# Search for goodTestModel definition
rg -l "type goodTestModel struct" 60-63: Enhance test coverage for invalid operations.
The test cases for invalid operations are limited. Consider adding more edge cases:
- Empty strings
- Very long values
- Special characters (e.g., Unicode, emojis)
- SQL injection attempts
97-99: Improve error messages for set comparison failures.
The error messages for set comparison failures could be more descriptive by showing the differences between the sets.
Apply this diff:
if !set1.Matches(set2) {
- t.Errorf("%v: %v not match %v\n", k8s, set1, set2)
+ t.Errorf("%v: sets don't match\nExpected: %v\nGot: %v\nDifferences: %v\n",
+ k8s, set2, set1, set1.Difference(set2))
}Also applies to: 180-182
10-109: LGTM! Comprehensive test coverage for label selector operations.
The test cases cover a good range of valid operations and error cases. The structure is clear and maintainable.
116-184: LGTM! Well-structured map conversion tests.
The test cases for map-based selectors are comprehensive and include important edge cases like empty maps.
pkg/k8s/selector/labels/selector.go (1)
29-66: 🛠️ Refactor suggestion
Reduce documentation duplication and clarify panic behavior.
The documentation for ParseSelectorOrDie is an exact duplicate of the Parse function documentation. This creates maintenance issues as the docs may get out of sync.
- Reference the
Parsefunction's documentation instead of duplicating it - Add explicit documentation about the panic behavior
-// Parse takes a string representing a selector and returns a selector
-// object, or panic when an error occur. This parsing function differs from ParseSelector
+// ParseSelectorOrDie is similar to Parse but panics on error instead of returning it.
+// See Parse for the detailed syntax documentation.
+//
+// This function should only be used in cases where the selector string is statically
+// known to be valid, such as in tests or initialization code where a panic is
+// acceptable if the selector is invalid.Likely invalid or redundant comment.
docs/user/field-selectors.md (2)
17-17: LGTM! Clear guidance on label filtering
The note effectively communicates the recommended approach for label filtering using Label Selectors.
49-49: LGTM! Well-structured examples
The examples effectively demonstrate:
- The proper usage of the
-lflag for label filtering - How to combine field selectors with label selectors
Also applies to: 57-57
api/v1alpha1/util.go (2)
348-363: LGTM! Efficient string concatenation implementation
The implementation correctly uses strings.Builder for efficient string concatenation when joining multiple expressions.
374-395:
Add support for standard comparison operators
The switch statement only handles Exists, DoesNotExist, In, and NotIn operators. Consider adding support for standard comparison operators like Equals, NotEquals, etc., as these are commonly used in field selectors.
Add cases for standard operators:
switch e.Operator {
case Exists:
sb.WriteString(e.Key)
case DoesNotExist:
sb.WriteString("!")
sb.WriteString(e.Key)
+case Equals:
+ if e.Values != nil && len(*e.Values) > 0 {
+ sb.WriteString(e.Key)
+ sb.WriteString("=")
+ sb.WriteString((*e.Values)[0])
+ }
+case NotEquals:
+ if e.Values != nil && len(*e.Values) > 0 {
+ sb.WriteString(e.Key)
+ sb.WriteString("!=")
+ sb.WriteString((*e.Values)[0])
+ }
case In:
if e.Values != nil {Likely invalid or redundant comment.
internal/tasks/resourcesync.go (1)
97-104: LGTM! Clean transition to field selector
The implementation correctly:
- Creates a field selector for owner filtering
- Includes proper error handling
- Updates the ListParams structure to use the new field selector
internal/service/repository.go (2)
69-81: LGTM: Clean implementation of selector parsing
The implementation properly validates and handles both field and label selectors with clear error messages.
87-87: LGTM: Proper integration with ListParams
The change correctly integrates the new selector types into the ListParams structure.
internal/service/resourcesync.go (1)
87-87: LGTM: Consistent implementation
The change maintains consistency with other service handlers in using the new selector framework.
internal/store/selector/selector.go (3)
338-364: LGTM! Clean implementation of selector tag parsing.
The implementation is well-structured and handles edge cases appropriately. The use of a map for options provides efficient lookups.
Line range hint 101-123: LGTM! Robust array index handling.
The implementation includes:
- Proper validation of array access pattern
- Integer overflow protection
- Correct handling of PostgreSQL's 1-based indexing
236-245: LGTM! Clean implementation of hidden selector filtering.
The implementation correctly filters out hidden selectors from error messages, improving the API's usability by showing only relevant selectors to users.
internal/service/fleet.go (1)
82-100: LGTM! Clean transition to structured selector types.
The implementation:
- Properly handles nil cases
- Provides clear error messages
- Maintains consistency with other services
internal/service/device.go (2)
67-77: LGTM! Consistent implementation of selector handling.
The implementation maintains consistency with other services and provides proper error handling.
Line range hint 82-93: LGTM! Clean integration with summary endpoint.
The implementation properly integrates the new selector framework while maintaining existing parameter validation logic.
internal/service/enrollmentrequest.go (3)
124-129: LGTM! Field selector implementation looks good.
The implementation correctly handles field selector parsing and error cases.
131-136: LGTM! Label selector implementation looks good.
The implementation correctly handles label selector parsing and error cases.
139-142: LGTM! ListParams structure updated correctly.
The listParams structure is properly updated to use the new selector types.
internal/tasks/fleet_rollout.go (2)
81-84: LGTM! Proper error handling for field selector creation.
The implementation correctly uses NewFieldSelectorFromMap with error handling, addressing previous concerns about using the safer non-panicking version.
86-89: Document the case sensitivity assumption.
The field selector is created with false for case sensitivity, but this assumption should be documented since it affects how owner matching works.
internal/store/selector/field_test.go (2)
Line range hint 10-73: LGTM! Comprehensive test cases for field selector types.
The test cases cover a wide range of field types and scenarios, including both valid and invalid cases.
Line range hint 74-284: LGTM! Thorough testing of field selector queries.
The test cases cover all supported operators and their combinations, with proper validation of both successful and error cases.
internal/service/certificatesigningrequest.go (2)
93-105: Well-structured selector initialization with proper error handling!
The implementation correctly handles both field and label selectors with appropriate nil checks and specific error messages for parsing failures.
111-111: Clean integration of label selector into ListParams!
The change aligns well with the PR's objective of integrating label filtering into the selector framework.
internal/store/fleet.go (2)
243-249: Clean implementation of field selector for device queries!
The code properly creates a field selector for filtering devices by owner, with appropriate error handling.
514-522: Robust implementation of JSONB-based annotation handling!
The code correctly handles annotations using map operations and JSONB format, aligning with the database schema changes.
internal/store/selector/field.go (3)
21-37: Well-designed selector structure with clear separation of concerns!
The FieldSelector struct and WithPrivateSelectors option provide a clean way to handle both public and private selectors.
610-618: Excellent implementation of field resolution with fallback mechanism!
The method properly handles both visible and hidden selectors with appropriate error handling and fallback logic.
354-358: Verify proper marking of private fields across the codebase.
The implementation correctly blocks access to private fields, but we should verify that sensitive fields are properly marked as private.
✅ Verification successful
Let me fix the regex syntax and try a more comprehensive search strategy.
The first command will help us identify potentially sensitive fields across the codebase. The second command will show us how private fields are currently marked in the selector package for comparison.
Let's also check the selector field definitions:
Let me search for how private fields are marked and used in the codebase.
Let me search for the definition of SelectorOpt and how private fields are marked.
Let me check how fields are marked as private in the struct tags.
Based on the findings, I can now provide a final response:
Private fields are properly marked and protected in the codebase
The implementation correctly handles private fields through:
- Proper marking of sensitive fields like
LabelsandAnnotationswithprivatetag option in struct definitions - Access control enforcement in
FieldSelector.resolveField()that blocks access to private fields unless explicitly enabled - Fields that contain metadata are consistently marked as private across model definitions
The current implementation follows good security practices by:
- Using a consistent tagging pattern
selector:"...,private"for sensitive fields - Enforcing access control at the selector resolution level
- Requiring explicit opt-in via
WithPrivateSelectors()option to access private fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for field definitions that might need to be marked as private
rg -A 5 "type.*struct.*{" | rg -B 5 "(password|secret|key|token|credential)"
Length of output: 172
Script:
#!/bin/bash
# Search for struct fields that might need to be marked as private
rg -i "(password|secret|key|token|credential)" -A 2 -B 2
# Search for field definitions in selector package
fd -e go . internal/store/selector --exec cat {} | rg "Options.*private"
Length of output: 98244
Script:
#!/bin/bash
# Look for SelectorField struct and its usage
ast-grep --pattern 'type SelectorField struct {
$$$
}'
Length of output: 467
Script:
#!/bin/bash
# Search for private field usage
rg -A 5 "private.*:.*true" internal/store/selector/
# Search for SelectorOpt definition and usage
ast-grep --pattern 'type SelectorOpt map[string]bool'
# Look for field definitions that might need to be private
rg -B 2 -A 2 "(password|secret|key|token|credential|private)" internal/store/selector/
Length of output: 40124
Script:
#!/bin/bash
# Look for SelectorOpt definition
ast-grep --pattern 'type SelectorOpt'
# Look for how fields are marked as private in the codebase
rg "private.*=.*true" internal/store/selector/
rg "selector.*private" internal/store/selector/
Length of output: 309
Script:
#!/bin/bash
# Look for struct field tags with private option
rg "selector:.*private" -A 2
# Look for examples of how private fields are defined
rg -B 2 "Options.*private" internal/store/selector/
Length of output: 1563
internal/tasks/fleet_selector.go (3)
119-123: LGTM: Proper error handling for LabelSelector creation
The error handling for creating a new LabelSelector is implemented correctly, ensuring invalid label selectors are caught early.
241-245: LGTM: Robust FieldSelector implementation
The transition to using FieldSelector for owner filtering is well-implemented with proper error handling.
255-267: LGTM: Well-structured selector combination
The method effectively combines LabelSelector and FieldSelector with proper error handling, providing precise device filtering capabilities.
test/integration/store/fleet_test.go (2)
236-237: LGTM: Comprehensive test coverage for new selector framework
The tests have been properly updated to use the new selector framework, with good coverage of both LabelSelector and FieldSelector functionality. The use of *OrDie variants in tests is appropriate as we want tests to fail fast on invalid selectors.
Also applies to: 658-661
247-253: LGTM: Thorough testing of selector operators
Comprehensive test coverage for all selector operators (In, NotIn, Exists, DoesNotExist) with proper validation of the results.
Also applies to: 262-268, 279-284, 296-301, 310-315, 324-329
pkg/queryparser/sqljsonb/sqljsonb.go (2)
74-87: Be mindful of concurrency when overriding functions at runtime.
Invoking WithOverrideFunction after instantiating the parser can cause race conditions if Parse is called concurrently. Since the parser stores overrides in sp.funcs, consider documenting or enforcing thread-safe usage for concurrent scenarios.
100-114: Expand coverage for newly defined functions in unit tests.
NewSQLParser introduces numerous predefined functions (e.g., AND, OR, NOT, CONTAINS, EXISTS, etc.). Ensure that each of these functions is tested for correct and erroneous inputs (e.g., insufficient arguments, invalid column names) to maintain reliability.
Would you like me to generate a testing strategy or sample test cases to help ensure thorough validation of function behavior?
asafbss
force-pushed
the
refactor-label-selector
branch
2 times, most recently
from
9d87678 to
0d27f88
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
internal/store/selector/label_test.go (1)
27-27:⚠️ Potential issueUndefined test model dependency.
The
goodTestModelis not defined in this file. This was previously flagged in past reviews.Please either:
- Define the
goodTestModelin this file, or- Import it from a shared test utilities package
api/v1alpha1/util.go (1)
374-395: 🛠️ Refactor suggestionAdd support for standard comparison operators.
The switch statement only handles Exists, DoesNotExist, In, and NotIn operators. Consider adding support for other standard operators like Equals and NotEquals for completeness.
Apply this diff to add support for standard operators:
switch e.Operator { case Exists: sb.WriteString(e.Key) case DoesNotExist: sb.WriteString("!") sb.WriteString(e.Key) case In: if e.Values != nil { sb.WriteString(e.Key) sb.WriteString(" in ") sb.WriteString("(" + strings.Join(*e.Values, ", ") + ")") } case NotIn: if e.Values != nil { sb.WriteString(e.Key) sb.WriteString(" notin ") sb.WriteString("(" + strings.Join(*e.Values, ", ") + ")") } +case Equals: + if e.Values != nil && len(*e.Values) > 0 { + sb.WriteString(e.Key) + sb.WriteString(" = ") + sb.WriteString((*e.Values)[0]) + } +case NotEquals: + if e.Values != nil && len(*e.Values) > 0 { + sb.WriteString(e.Key) + sb.WriteString(" != ") + sb.WriteString((*e.Values)[0]) + } default: return "" }test/integration/store/resourcesync_test.go (1)
236-241: 🛠️ Refactor suggestionRename the test to reflect its actual purpose.
The test name "List with paging" is duplicated and misleading as this test actually validates label selector filtering functionality.
Rename the test to better reflect its purpose:
-It("List with paging", func() { +It("List with label selector filtering", func() {internal/store/selector/field_test.go (1)
315-319:⚠️ Potential issueFix early return in test loop.
The early return on error prevents testing remaining test cases in the loop. This issue was previously identified in a past review.
- fr.fieldResolver, err = SelectorFieldResolver(&goodTestModel{}) - if err != nil { - t.Errorf("error %v (%#v)\n", err, err) - return - } + fr.fieldResolver, err = SelectorFieldResolver(&goodTestModel{}) + if err != nil { + t.Errorf("error %v (%#v)\n", err, err) + continue + }
🧹 Nitpick comments (26)
internal/store/selector/label_test.go (3)
10-32: Enhance test structure and coverage for label selector operations.Consider the following improvements:
- Use table-driven subtests with
t.Run()for better test organization and failure isolation.- Add negative test cases to validate error handling.
- Make error messages more descriptive by including expected vs actual results.
Example refactor:
func TestLabelSelectorOperations(t *testing.T) { - testGoodStrings := []string{ - "key", - "!key", - "key=val", - "key!=val", - "key in (val1,val2)", - "key notin (val1,val2)", + tests := []struct { + name string + input string + wantErr bool + }{ + {name: "exists operator", input: "key", wantErr: false}, + {name: "not exists operator", input: "!key", wantErr: false}, + {name: "equals operator", input: "key=val", wantErr: false}, + {name: "not equals operator", input: "key!=val", wantErr: false}, + {name: "in operator", input: "key in (val1,val2)", wantErr: false}, + {name: "not in operator", input: "key notin (val1,val2)", wantErr: false}, + // Add negative test cases + {name: "invalid key", input: "invalid@key=val", wantErr: true}, } - for _, test := range testGoodStrings { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { - ls, err := NewLabelSelector(test) - if err != nil { - t.Errorf("%v: error %v (%#v)\n", test, err, err) + ls, err := NewLabelSelector(tt.input) + if (err != nil) != tt.wantErr { + t.Errorf("NewLabelSelector() error = %v, wantErr %v", err, tt.wantErr) continue } + if tt.wantErr { + return + } - _, _, err = ls.Parse(context.Background(), &goodTestModel{}, NewSelectorName("model.field16")) - if err != nil { - t.Errorf("%v: error %v (%#v)\n", test, err, err) + if _, _, err = ls.Parse(context.Background(), &goodTestModel{}, NewSelectorName("model.field16")); err != nil { + t.Errorf("Parse() error = %v", err) } + }) } }
46-58: Improve readability of test query strings.The long query strings are hard to read and maintain. Consider using string constants or a helper function to construct these queries.
Example:
func buildQueryString(op string, key string, values ...string) string { switch op { case "exists": return fmt.Sprintf("EXISTS(K(field16),V(%s))", key) case "equals": return fmt.Sprintf("AND(EXISTS(K(field16),V(%s)),CONTAINS(K(field16),V({\"%s\": \"%s\"})))", key, key, values[0]) // ... other cases } return "" }
119-146: Add more edge cases to map selector tests.The current test cases cover basic scenarios well, but consider adding:
- Maps with special characters in keys/values
- Maps with very long keys/values
- Maps with duplicate values across different keys
Example additional test cases:
{ Input: map[string]string{ "very-long-key-name-that-exceeds-normal-length": "value", "key-with-special-chars!@#": "value", "key1": "shared-value", "key2": "shared-value", }, Expected: "...", // Add expected query string },internal/store/templateversion.go (1)
48-72: Consider refactoring duplicate index creation logic.The index creation logic for labels and annotations follows the same pattern. Consider extracting this into a helper method to reduce code duplication.
func (s *TemplateVersionStore) createGinIndex(field, indexName string) error { if !s.db.Migrator().HasIndex(&model.TemplateVersion{}, indexName) { if s.db.Dialector.Name() == "postgres" { return s.db.Exec(fmt.Sprintf("CREATE INDEX %s ON template_versions USING GIN (%s)", indexName, field)).Error } return s.db.Migrator().CreateIndex(&model.TemplateVersion{}, indexName) } return nil }Usage:
- // Create GIN index for TemplateVersion labels - if !s.db.Migrator().HasIndex(&model.TemplateVersion{}, "idx_template_versions_labels") { - if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_template_versions_labels ON template_versions USING GIN (labels)").Error; err != nil { - return err - } - } else { - if err := s.db.Migrator().CreateIndex(&model.TemplateVersion{}, "Labels"); err != nil { - return err - } - } - } + if err := s.createGinIndex("labels", "idx_template_versions_labels"); err != nil { + return err + } - // Create GIN index for TemplateVersion annotations - if !s.db.Migrator().HasIndex(&model.TemplateVersion{}, "idx_template_versions_annotations") { - if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_template_versions_annotations ON template_versions USING GIN (annotations)").Error; err != nil { - return err - } - } else { - if err := s.db.Migrator().CreateIndex(&model.TemplateVersion{}, "Annotations"); err != nil { - return err - } - } - } + if err := s.createGinIndex("annotations", "idx_template_versions_annotations"); err != nil { + return err + }internal/store/common.go (1)
87-92: Consider consolidating repeated selector parse code.The logic for annotation, label, and field selectors is nearly identical. You could factor out the shared pattern into a helper function that applies “parse &
query.Where” for each selector, improving code clarity.pkg/queryparser/sqljsonb/sqljsonb.go (1)
34-52: Ensure correctness for required preceding key function.The
withPrecedingKeyQueryverification mandates the first argument to beK. This strict check is good. However, consider clarifying the error message if users call other functions that also require a preceding key.internal/store/selector/annotation.go (5)
22-41: Panic usage in constructor.
NewAnnotationSelectorFromMapOrDiepanics if creation fails. This is fine for “must-succeed” usage, but ensure you handle or document these panic scenarios if used in production code.
43-72: Consolidate invert flag logic.The
invertlogic is straightforward but duplicated for each key in the map. When you have more advanced transformations, consider extracting an internal helper to handle this.
74-91: Check user awareness of “OrDie” convention.Similar to
NewAnnotationSelectorFromMapOrDie, ensure consumers know thatNewAnnotationSelectorOrDiewill panic. Provide guidance or use error returns if needed.
230-287: Mitigate potential overhead of repeated tokens.
parseRequirementcan create multiple tokens for each requirement. For large sets of annotations, token creation might become a performance bottleneck. You may optimize by short-circuiting or caching repeated keys if feasible.
303-313: Enforce single-argument usage onqueryField.You enforce a single argument but don’t specify whether the function name is locked down only to
K. If it’s used in other contexts, consider explicit checks for function name.internal/store/selector/label.go (1)
36-41: Consider returning an error instead of panicking
Usingpanic(err)withinNewLabelSelectorFromMapOrDiemay be acceptable in tests or short-lived tools, but can be risky in production contexts, as it terminates execution abruptly. Consider returning an error to maintain graceful error handling.func NewLabelSelectorFromMapOrDie(labels map[string]string, invert ...bool) *LabelSelector { ls, err := NewLabelSelectorFromMap(labels, invert...) if err != nil { - panic(err) + // handle error gracefully; for instance: + // log.Fatalf("failed to create label selector: %v", err) } return ls }internal/cli/get.go (1)
85-86: Enhance documented operators
The usage notes forselectorandfield-selectorflags mention '=', '!=', and 'in' operators. Since the PR references support for other operators such as 'notin', 'exists', and 'not exists', consider updating the help text to reflect all supported operators.fs.StringVarP(&o.LabelSelector, "selector", "l", o.LabelSelector, - "Selector (label query) to filter on, supporting operators like '=', '!=', and 'in' (e.g., -l='key1=value1,key2!=value2,key3 in (value3, value4)').") + "Selector (label query) to filter on, supporting operators such as '=', '!=', 'in', 'notin', 'exists', and 'doesnotexist'. (e.g., -l='key1=value1,key2!=value2,key3 in (value3, value4)').") fs.StringVar(&o.FieldSelector, "field-selector", o.FieldSelector, "Selector (field query) to filter on, supporting operators like '=', '==', and '!=' (e.g., --field-selector='key1=value1,key2!=value2').")internal/store/store.go (2)
8-8: Clear import grouping
While it's acceptable to importselectorin its own line, consider grouping relevant imports together to improve readability and keep third-party imports organized. This is a minor style preference, not a functional issue.
133-137: Leverage JSONB for flexible filtering
The newly introducedLabelSelector,AnnotationSelector, andFieldSelectorfields utilize JSONB-based filtering. This approach is flexible but can be costly if not indexed properly; ensure that relevant indexes are in place for performance at scale.internal/store/selector/selector_test.go (1)
110-113: Document unsupported types
ThebadTestModelexplicitly tests unsupported field types. Make sure to document any relevant error messages for maintainers, so they understand why text array or unknown numeric types are unsupported.internal/store/selector/types.go (1)
194-202: LGTM: Efficient implementation of selector options.The use of
map[string]struct{}for SelectorOpt is memory efficient and follows Go best practices for implementing sets.Consider adding a comment explaining the purpose of the Options field in SelectorField:
type SelectorField struct { Name SelectorName Type SelectorType FieldName string FieldType gormschema.DataType - Options SelectorOpt + Options SelectorOpt // Set of additional options/flags for the selector }docs/developer/architecture/field-selectors.md (1)
22-31: Enhance the documentation with more context and examples.While the documentation clearly explains the purpose of hidden and private annotations, it could be more helpful with additional context.
Consider adding:
- A complete example showing both annotations in use
- Common use cases for each annotation type
- How these annotations interact with other selector features
🧰 Tools
🪛 LanguageTool
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private"``` *hidden`: The selector will not be exposed to end...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. *private: The selector cannot be directly used by...(UNLIKELY_OPENING_PUNCTUATION)
internal/store/selector/annotation_test.go (2)
11-18: Expand test cases for annotation selector operations.The test cases only cover basic operations. Consider adding:
- Empty strings
- Special characters
- Very long keys/values
- Unicode characters
- Invalid operators
testGoodStrings := []string{ "key", "!key", "key=val", "key!=val", "key in (val1,val2)", "key notin (val1,val2)", + "", // Empty string + "very-long-key-name-that-exceeds-typical-length=value", + "unicode-key-测试=value-测试", + "key with spaces=value with spaces", + "key<>value", // Invalid operator }
60-62: Enhance invalid operation test cases.The test only checks for invalid characters. Add more error scenarios:
- Malformed expressions
- Missing values
- Invalid operators
- Syntax errors
testBadOperations := []string{ "ke@y", + "key=", // Missing value + "=value", // Missing key + "key==", // Invalid operator + "key in val1,val2)", // Malformed parentheses + "key in (val1,", // Incomplete expression + "key notin", // Missing values list }internal/service/templateversion.go (1)
109-113: Consider adding pagination parameters to ListParams.The hardcoded limit of 100 items could be configurable for better control over resource usage.
listParams := store.ListParams{ - Limit: 100, + Limit: DefaultBatchSize, FieldSelector: fieldSelector, } + +const ( + DefaultBatchSize = 100 +)internal/store/enrollmentrequest.go (1)
47-58: Extract index names as constants.The index names are hardcoded in multiple places. Extract them as constants for better maintainability.
+const ( + EnrollmentRequestLabelsIndex = "idx_enrollment_requests_labels" + EnrollmentRequestAnnotationsIndex = "idx_enrollment_requests_annotations" +) + // Create GIN index for EnrollmentRequest labels -if !s.db.Migrator().HasIndex(&model.EnrollmentRequest{}, "idx_enrollment_requests_labels") { +if !s.db.Migrator().HasIndex(&model.EnrollmentRequest{}, EnrollmentRequestLabelsIndex) { if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_enrollment_requests_labels ON enrollment_requests USING GIN (labels)").Error; err != nil { + if err := s.db.Exec("CREATE INDEX " + EnrollmentRequestLabelsIndex + " ON enrollment_requests USING GIN (labels)").Error; err != nil {internal/store/selector/selector.go (2)
94-95: Consider adding explicit empty string handlingWhile the current implementation is functionally correct, consider adding explicit empty string handling to improve robustness.
-if len(selectorName) > len(sn.String()) && strings.HasPrefix(selectorName, sn.String()) { +snStr := sn.String() +if snStr == "" { + continue +} +if len(selectorName) > len(snStr) && strings.HasPrefix(selectorName, snStr) {
338-364: Add validation for malformed selector tagsThe function should validate that the field name is not empty when options are present.
func parseSelectorTag(tag string) (string, map[string]struct{}) { if strings.TrimSpace(tag) == "" { return "", nil } // Split the tag into parts parts := strings.Split(tag, ",") fieldName := strings.TrimSpace(parts[0]) + if fieldName == "" && len(parts) > 1 { + return "", nil // Invalid tag format: options without field name + } // Initialize the map for options options := make(map[string]struct{})internal/store/selector/field_test.go (1)
48-52: Consider consolidating error handling in test loops.The error handling pattern in both test loops is identical. Consider extracting this into a helper function to reduce code duplication.
+func handleTestError(t *testing.T, test string, err error) bool { + if err != nil { + t.Errorf("%v: error %v (%#v)\n", test, err, err) + return true + } + return false +} func TestFieldSelectorTypes(t *testing.T) { // ... for _, test := range testGoodStrings { f, err := NewFieldSelector(test) - if err != nil { - t.Errorf("%v: error %v (%#v)\n", test, err, err) - continue - } + if handleTestError(t, test, err) { + continue + } // ... } for _, test := range testBadStrings { f, err := NewFieldSelector(test) - if err != nil { - t.Errorf("%v: error %v (%#v)\n", test, err, err) - continue - } + if handleTestError(t, test, err) { + continue + } // ... }Also applies to: 61-65
internal/store/fleet.go (1)
59-89: Great database optimization with GIN indexes!The implementation of GIN indexes for Fleet labels and annotations will significantly improve query performance. The code also properly handles different database dialects and includes comprehensive error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
api/v1alpha1/openapi.yaml(0 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(0 hunks)api/v1alpha1/util.go(1 hunks)docs/developer/architecture/field-selectors.md(2 hunks)docs/user/field-selectors.md(3 hunks)go.mod(0 hunks)internal/api/client/client.gen.go(0 hunks)internal/api/server/server.gen.go(0 hunks)internal/cli/get.go(1 hunks)internal/flterrors/flterrors.go(1 hunks)internal/service/certificatesigningrequest.go(1 hunks)internal/service/device.go(3 hunks)internal/service/enrollmentrequest.go(1 hunks)internal/service/fleet.go(1 hunks)internal/service/repository.go(1 hunks)internal/service/resourcesync.go(1 hunks)internal/service/templateversion.go(2 hunks)internal/store/certificatesigningrequest.go(2 hunks)internal/store/common.go(1 hunks)internal/store/common_test.go(0 hunks)internal/store/device.go(7 hunks)internal/store/enrollmentrequest.go(2 hunks)internal/store/fleet.go(4 hunks)internal/store/model/certificatesigningrequest.go(2 hunks)internal/store/model/device.go(2 hunks)internal/store/model/enrollmentrequest.go(2 hunks)internal/store/model/fleet.go(2 hunks)internal/store/model/jsonfield.go(2 hunks)internal/store/model/repository.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/resourcesync.go(2 hunks)internal/store/model/selectors.go(2 hunks)internal/store/model/selectors_test.go(4 hunks)internal/store/model/templateversion.go(3 hunks)internal/store/repository.go(2 hunks)internal/store/resourcesync.go(2 hunks)internal/store/selector/annotation.go(1 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(18 hunks)internal/store/selector/field_test.go(4 hunks)internal/store/selector/label.go(1 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/selector.go(7 hunks)internal/store/selector/selector_test.go(4 hunks)internal/store/selector/types.go(2 hunks)internal/store/store.go(2 hunks)internal/store/templateversion.go(1 hunks)internal/tasks/fleet_rollout.go(2 hunks)internal/tasks/fleet_selector.go(3 hunks)internal/tasks/resourcesync.go(2 hunks)internal/util/util.go(1 hunks)pkg/k8s/selector/labels/selector.go(1 hunks)pkg/queryparser/sqljsonb/sqljsonb.go(1 hunks)test/integration/store/device_test.go(4 hunks)test/integration/store/enrollmentrequest_test.go(2 hunks)test/integration/store/fleet_test.go(9 hunks)test/integration/store/repository_test.go(2 hunks)test/integration/store/resourcesync_test.go(3 hunks)test/integration/tasks/repotester_conditions_test.go(2 hunks)
💤 Files with no reviewable changes (6)
- internal/store/common_test.go
- api/v1alpha1/openapi.yaml
- internal/api/server/server.gen.go
- internal/api/client/client.gen.go
- api/v1alpha1/types.gen.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (22)
- test/integration/store/repository_test.go
- test/integration/tasks/repotester_conditions_test.go
- api/v1alpha1/spec.gen.go
- internal/store/model/resource.go
- internal/util/util.go
- test/integration/store/enrollmentrequest_test.go
- internal/service/fleet.go
- docs/user/field-selectors.md
- internal/store/model/repository.go
- internal/store/model/templateversion.go
- internal/store/repository.go
- internal/store/resourcesync.go
- internal/store/model/selectors_test.go
- internal/store/certificatesigningrequest.go
- internal/store/model/certificatesigningrequest.go
- internal/tasks/fleet_rollout.go
- internal/store/model/jsonfield.go
- internal/store/model/enrollmentrequest.go
- internal/store/model/resourcesync.go
- internal/store/model/device.go
- internal/store/model/selectors.go
- internal/store/device.go
🧰 Additional context used
🪛 LanguageTool
docs/developer/architecture/field-selectors.md
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private" ``` *hidden`: The selector will not be exposed to end...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. * private: The selector cannot be directly used by...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > - NewSelectorName can be replaced with `NewHiddenSelectorName...
(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
docs/developer/architecture/field-selectors.md
66-66: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
67-67: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
68-68: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
52-52: Column: 1
Hard tabs
(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs
(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs
(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs
(MD010, no-hard-tabs)
56-56: Column: 1
Hard tabs
(MD010, no-hard-tabs)
57-57: Column: 1
Hard tabs
(MD010, no-hard-tabs)
61-61: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (63)
internal/store/selector/label_test.go (2)
60-63: Expand test coverage for invalid operations.
The current test cases only check basic invalid characters. Consider adding more edge cases:
- Empty strings
- Very long values
- Special characters
- Unicode characters
- SQL injection attempts
180-182: Enhance error messages for better debugging.
When sets don't match, the error message should show the differences between set1 and set2 more clearly.
Example:
- t.Errorf("%v: %v not match %v\n", op, set1, set2)
+ t.Errorf("Test case %v failed:\nExpected: %v\nGot: %v\nDifferences: %v",
+ op, set2, set1, describeDifferences(set1, set2))internal/store/templateversion.go (3)
44-74: Implementation looks good, but index naming needs consistency.
The implementation of GIN indexes for JSONB fields is correct and includes proper error handling. However, the index naming differs between PostgreSQL and other dialects, which could cause maintenance issues.
Consider using consistent index names across all dialects:
- if err := s.db.Migrator().CreateIndex(&model.TemplateVersion{}, "Labels"); err != nil {
+ if err := s.db.Migrator().CreateIndex(&model.TemplateVersion{}, "idx_template_versions_labels"); err != nil {Line range hint 77-79: LGTM! Good defensive programming practice.
The nil check for the resource parameter is a good addition that prevents potential panics and follows defensive programming practices.
44-46: Verify query compatibility after JSONB migration.
The transition from arrays to JSONB format requires verification of existing queries in the codebase.
✅ Verification successful
JSONB migration appears safe with proper index support
Based on the verification results, the migration to JSONB format appears safe as:
-
The codebase already uses JSONB for annotations with proper GIN indexes:
CREATE INDEX idx_template_versions_annotations ON template_versions USING GIN (annotations)- Similar GIN indexes exist for devices, fleets, and other resources
-
All annotation operations use type-safe methods and map structures:
- Updates via
UpdateAnnotationsmethod usingmap[string]string - Storage using
JSONMap[string, string]type with GORM JSONB mapping - No direct array operations that would be incompatible with JSONB
- Updates via
-
Query patterns are compatible with JSONB:
- Selector implementation properly handles JSONB fields
- No legacy array-specific query patterns found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential incompatible queries after JSONB migration
# Search for array operations on labels/annotations that might need updates
echo "Searching for potential array operations that need updates..."
rg -n '(labels|annotations).*\[\]' --type go
# Search for label/annotation field access patterns
echo "Searching for label/annotation field access patterns..."
ast-grep --pattern 'labels.$$$' --type go
ast-grep --pattern 'annotations.$$$' --type go
# Look for potential GORM queries that might need updates
echo "Searching for GORM queries on labels/annotations..."
rg -n '(Where|Having).*labels|annotations' --type go
Length of output: 13213
internal/store/common.go (2)
70-75: Ensure proper sanitization of parsed queries.
The FieldSelector.Parse() method returns a query string (q) and parameters (p). Please verify that q never contains raw user-supplied content to avoid potential SQL injection, and confirm the parameters are safely parameterized.
78-85: Maintain consistent error handling pattern.
The LabelSelector.Parse() call mirrors the logic for FieldSelector. The code looks good; just keep an eye on ensuring that any parse errors are properly distinguished and helpful for debugging.
pkg/queryparser/sqljsonb/sqljsonb.go (3)
14-21: Validate proper usage of FunctionResult.
Storing both query parts and arguments in FunctionResult is clear. Ensure that each function that returns a FunctionResult sets both Query and Args consistently; missing arguments could lead to subtle bugs.
54-63: Confirm that “no values” usage covers all edge cases.
withNoValues disallows literal values but doesn’t explicitly handle nested function calls. If nesting is expected, ensure that nested function calls don’t accidentally pass values.
100-150: Confirm the function map initialization is complete.
NewSQLParser sets default SQL functions (AND, OR, NOT, CONTAINS, EXISTS, etc.). Verify you’re not missing additional operators needed by the rest of the code (e.g., detailed comparisons).
internal/store/selector/annotation.go (2)
16-20: AnnotationSelector design looks good.
Storing a SelectorField plus parser and selector.Selector provides a clear separation of concerns.
118-180: Robust field resolution checks.
You correctly ensure that the field is jsonb and only one field is resolved. The error messages are helpful. This is good and consistent with the system’s approach.
internal/flterrors/flterrors.go (1)
21-24: Error constants are consistent and clear.
The newly introduced errors for label/annotation selector syntax and parsing are concise and consistent with the existing naming convention.
internal/store/model/fleet.go (2)
53-54: Fallback to default maps is a good approach.
Using lo.FromPtrOr ensures Labels and Annotations are never nil, which simplifies handling downstream.
98-99: Ensure consistent map usage on output.
lo.ToPtr(util.EnsureMap(...)) is symmetrical with the input logic and consistently ensures a non-nil map in the API resource. This looks aligned with your overall design.
internal/store/selector/label.go (3)
16-20: Use of pointer fields in the LabelSelector struct
The usage of SelectorField and queryparser.Parser as pointers ensures they can remain nil until properly resolved. This design pattern appears consistent with the rest of the code—just ensure there is robust error handling where these fields are dereferenced.
57-59: Validate that returning an empty selector matches the intended behavior
Returning an empty selector when the label map is empty can unintentionally match all records. This behavior was previously flagged in older review comments and may still be undesirable.
292-297: Ensure safety of serialized key-value pairs in JSON
createPairToken uses fmt.Sprintf("{\"%s\": \"%s\"}", key, value) to generate JSON. If key or value originate from untrusted input, consider properly sanitizing or encoding them to avoid potential JSON injection or parsing errors.
internal/store/selector/selector_test.go (3)
30-33: Chained selectors for multiple fields
Mapping "mappedselector" to multiple selector names is a neat approach. Confirm downstream code merges these requirements logically (e.g., with AND/OR as needed) to avoid unexpected results.
78-85: Ensure consistent usage of SelectorName
Using NewSelectorName consistently across the test model methods enhances clarity. It might be worth verifying that all references to these selectors in production code are similarly updated.
153-153: Meaningful error handling for unknown selectors
The test for "unknownselector" expects an error. This effectively ensures that truly unknown selectors are rejected. The logic is solid and clarifies how the system responds to invalid inputs.
internal/store/selector/types.go (3)
86-88: Consider adding comparison methods to the SelectorName interface.
The interface only requires String() method but lacks comparison/equality methods. This could lead to issues when comparing selector names or using them as map keys.
Consider adding the following methods to the interface:
type SelectorName interface {
String() string // Returns the string representation of the selector name.
+ Equals(other SelectorName) bool
+ Compare(other SelectorName) int
}90-98: LGTM: Clean implementation with proper whitespace handling.
The default implementation with whitespace trimming is clean and correct.
105-117: LGTM: Well-structured hidden selector implementation.
The hiddenSelectorName implementation properly follows the pattern established by selectorName and correctly handles whitespace.
docs/developer/architecture/field-selectors.md (1)
52-62: LGTM: Clear and informative example.
The example effectively demonstrates the proper usage of selector mapping and the NewSelectorName constructor.
🧰 Tools
🪛 Markdownlint (0.37.0)
52-52: Column: 1
Hard tabs
(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs
(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs
(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs
(MD010, no-hard-tabs)
56-56: Column: 1
Hard tabs
(MD010, no-hard-tabs)
57-57: Column: 1
Hard tabs
(MD010, no-hard-tabs)
61-61: Column: 1
Hard tabs
(MD010, no-hard-tabs)
pkg/k8s/selector/labels/selector.go (2)
29-66: Reduce documentation duplication.
The ParseSelectorOrDie function documentation is an exact duplicate of the Parse function documentation. This creates maintenance issues as the docs may get out of sync.
Consider referencing the Parse function's documentation instead:
-// Parse takes a string representing a selector and returns a selector
-// object, or panic when an error occur. This parsing function differs from ParseSelector
-// as they parse different selectors with different syntaxes.
-// The input will cause an error if it does not follow this form:
-//
-// <selector-syntax> ::= ...
+// ParseSelectorOrDie is similar to Parse but panics if an error occurs.
+// See Parse for the full documentation of the selector syntax.73-81: LGTM: Clear and well-formatted documentation.
The updated Parse function documentation is clear, consistent, and properly formatted.
internal/store/selector/annotation_test.go (2)
119-146: LGTM! Comprehensive test cases for map-based selectors.
The test cases cover various scenarios including:
- Single key-value pair
- Multiple key-value pairs
- Empty map
- Different key-value combinations
83-84:
Add bounds check before array access.
Direct access to resolvedFields[0] could panic if the array is empty.
-s.field = resolvedFields[0]
+if len(resolvedFields) == 0 {
+ t.Fatal("no fields resolved")
+}
+s.field = resolvedFields[0]Likely invalid or redundant comment.
internal/store/enrollmentrequest.go (1)
207-207: LGTM! Explicit initialization of annotations.
Setting annotations to nil ensures consistent behavior during create/update operations.
api/v1alpha1/util.go (1)
348-363: LGTM! Well-implemented string conversion function.
The function efficiently converts MatchExpressions to string representation using strings.Builder, with proper handling of empty input and comma separation.
internal/tasks/resourcesync.go (1)
97-104: LGTM! Clean implementation of field selector for owner filtering.
The change properly implements the new field selector approach for filtering by owner, replacing the previous Owners field in ListParams.
test/integration/store/resourcesync_test.go (1)
115-118: LGTM! Well-structured test for field selector functionality.
The test properly validates the new field selector implementation for owner filtering.
internal/store/selector/selector.go (2)
80-88: LGTM: Proper handling of selector tag parsing
The implementation correctly parses and stores selector options from struct tags.
236-245: LGTM: Proper handling of hidden selectors in error messages
The implementation correctly filters out hidden selectors from user-facing error messages, improving the API's usability.
internal/service/repository.go (2)
69-81: LGTM: Consistent implementation of field selector handling
The implementation properly integrates with the new selector framework and provides clear error messages.
87-87: LGTM: Proper integration of label selector
The implementation correctly integrates the new label selector system into the ListParams structure.
internal/service/resourcesync.go (1)
69-87: Implementation matches repository.go
The changes are identical to those in repository.go and follow the same pattern.
internal/service/device.go (3)
67-71: LGTM! Field selector initialization and error handling look good.
The code properly initializes the field selector and handles parsing errors with clear error messages.
74-77: LGTM! Label selector initialization and error handling look good.
The code properly initializes the label selector and handles parsing errors with clear error messages.
93-93: LGTM! List parameters updated correctly.
The code properly integrates the new field and label selectors into the list parameters.
Also applies to: 116-116
internal/service/enrollmentrequest.go (3)
127-132: LGTM! Field selector initialization and error handling look good.
The code properly initializes the field selector and handles parsing errors with clear error messages.
134-140: LGTM! Label selector initialization and error handling look good.
The code properly initializes the label selector and handles parsing errors with clear error messages.
145-145: LGTM! List parameters updated correctly.
The code properly integrates the new label selector into the list parameters.
internal/store/selector/field_test.go (1)
286-337: LGTM! TestFieldSelectorMap implementation.
The new test function provides good coverage for map-based field selectors with various test cases.
internal/service/certificatesigningrequest.go (1)
93-111: LGTM! Well-structured selector implementation.
The changes effectively transition from string-based selectors to structured types, with proper error handling and consistent implementation for both field and label selectors.
internal/store/fleet.go (1)
514-522: LGTM! Improved annotation handling.
The transition to using EnsureMap and MakeJSONMap provides a more efficient and consistent way of handling annotations.
internal/store/selector/field.go (3)
21-37: Well-designed selector structure with proper encapsulation.
The FieldSelector structure and its options are well-designed, with clear separation of concerns and good documentation for private selectors.
39-87: Excellent documentation and error handling in constructors.
The constructor methods are well-documented with clear examples and proper error handling. The "OrDie" variants are appropriately implemented for cases where selector initialization is expected to succeed.
610-618: Robust field resolution with proper fallback.
The resolveSelectorField implementation properly handles both visible and hidden selectors with appropriate error handling and fallback mechanism.
test/integration/store/device_test.go (7)
280-280: Rename test case for clarity.
The test name "List with paging" is duplicated. Rename it to "List with label selector" to accurately reflect its purpose.
282-283: LGTM: Clean transition to LabelSelector.
The implementation correctly uses the new selector framework with NewLabelSelectorFromMapOrDie for label-based filtering.
Line range hint 119-124: LGTM: Robust error handling for label selector creation.
The implementation properly handles potential errors when creating label selectors from fleet match labels.
Line range hint 241-246: LGTM: Clean field selector implementation for owner filtering.
The implementation correctly uses field selectors to filter devices by owner, with proper error handling.
Line range hint 255-267: LGTM: Well-structured combination of label and field selectors.
The implementation effectively combines both label and field selectors for orphaned device detection:
- Label selector to match fleet's criteria
- Field selector to match devices owned by the fleet
Line range hint 236-237: LGTM: Clean label selector test implementation.
The test correctly validates the new label selector functionality using NewLabelSelectorFromMapOrDie.
Line range hint 658-661: LGTM: Well-structured field selector test for owner filtering.
The test effectively validates owner-based filtering using field selectors with private selector options.
internal/tasks/fleet_selector.go (4)
12-12: LGTM: Clean import addition for selector package
The addition of the selector package import aligns with the PR's objective of integrating label and annotation filtering into the selector framework.
119-128: LGTM: Clean transition to LabelSelector
The changes properly implement the selector framework integration:
- Robust error handling for LabelSelector creation
- Appropriate placement of selector creation before device listing
- Clean transition from Labels to LabelSelector in ListParams
241-249: LGTM: Well-implemented FieldSelector for owner filtering
The changes demonstrate proper implementation of field-based filtering:
- Robust error handling for FieldSelector creation
- Correct usage of SetResourceOwner for owner reference
- Clean integration with ListParams
255-272: LGTM: Well-implemented composite selector logic
The changes demonstrate sophisticated selector usage:
- Proper implementation of label selector with negation
- Correct field selector creation for owner matching
- Clean combination of both selectors in ListParams
test/integration/store/fleet_test.go (2)
658-660: LGTM: Well-implemented owner filtering test
The test changes properly validate the new selector-based filtering:
- Correct usage of FieldSelector for owner filtering
- Proper handling of private selectors through options
Line range hint 236-329: LGTM: Comprehensive test coverage for label selectors
The test changes provide excellent coverage of the selector framework:
- All label selector operators are thoroughly tested (=, in, notin, exists, doesnotexist)
- Edge cases are properly covered (non-existent keys)
- Consistent usage of selector creation methods
asafbss
force-pushed
the
refactor-label-selector
branch
from
95b3ed8 to
2a48b30
Compare
|
|
||
| var sb strings.Builder | ||
| for i, e := range exprs { | ||
| sb.WriteString(e.String()) |
There was a problem hiding this comment.
can e.string be nil? any check for that?
There was a problem hiding this comment.
Hey,
e is of type MatchExpression.
Since it's not a pointer, it cannot be nil.
| } | ||
| case NotIn: | ||
| if e.Values != nil { | ||
| sb.WriteString(e.Key) |
There was a problem hiding this comment.
these strings appear multiple times,
so maybe put them in consts?
There was a problem hiding this comment.
Please clarify—e.Key is not a constant string.
| // - DoesNotExist: "!key" | ||
| // - In: "key in (val1, val2)" | ||
| // - NotIn: "key notin (val1, val2)" | ||
| func (e MatchExpression) String() string { |
There was a problem hiding this comment.
@ori-amizur MatchExpression will be integrated as part of the label-selector infra
| Limit int | ||
| Continue *Continue | ||
| FieldSelector *selector.FieldSelector | ||
| LabelSelector *selector.LabelSelector |
There was a problem hiding this comment.
@ori-amizur MatchExpression will be integrated as part of the label-selector infra
| LabelMatchExpressions: api.MatchExpressions{ | ||
| { | ||
| LabelSelector: selector.NewLabelSelectorOrDie( | ||
| api.MatchExpression{ |
There was a problem hiding this comment.
@ori-amizur MatchExpression will be integrated as part of the label-selector infra
asafbss
force-pushed
the
refactor-label-selector
branch
from
2a48b30 to
603163c
Compare
… indexes - Refactored labels and annotations fields to use JSONB instead of arrays. - Added conditional logic in migrations to create GIN indexes for efficient querying.
…e for Enhanced Querying
…ucture for Enhanced Querying
asafbss
force-pushed
the
refactor-label-selector
branch
from
603163c to
43cd381
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/store/selector/selector.go (1)
94-95:⚠️ Potential issueFix potential string comparison bug.
The string length comparison before HasPrefix could cause a panic if
sn.String()returns an empty string.Apply this diff to fix the issue:
-if len(selectorName) > len(sn.String()) && strings.HasPrefix(selectorName, sn.String()) { +snStr := sn.String() +if snStr != "" && len(selectorName) > len(snStr) && strings.HasPrefix(selectorName, snStr) {internal/store/certificatesigningrequest.go (1)
51-61:⚠️ Potential issueStandardize index naming convention.
The index name 'idx_csr_labels' is hardcoded but not consistent with the fallback index name 'Labels'. This could cause issues if the code needs to check for or drop these indexes later.
Consider standardizing the index naming convention across all dialects:
-if !s.db.Migrator().HasIndex(&model.CertificateSigningRequest{}, "idx_csr_labels") { +const labelIndexName = "idx_csr_labels" +if !s.db.Migrator().HasIndex(&model.CertificateSigningRequest{}, labelIndexName) { if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_csr_labels ON certificate_signing_requests USING GIN (labels)").Error; err != nil { + if err := s.db.Exec("CREATE INDEX " + labelIndexName + " ON certificate_signing_requests USING GIN (labels)").Error; err != nil { return err } } else { - if err := s.db.Migrator().CreateIndex(&model.CertificateSigningRequest{}, "Labels"); err != nil { + if err := s.db.Migrator().CreateIndex(&model.CertificateSigningRequest{}, labelIndexName); err != nil { return err } }internal/store/selector/field_test.go (1)
315-319:⚠️ Potential issueFix early return preventing subsequent test cases
The early return on error prevents testing the remaining test cases in the loop.
fr.fieldResolver, err = SelectorFieldResolver(&goodTestModel{}) if err != nil { t.Errorf("error %v (%#v)\n", err, err) - return + continue }
🧹 Nitpick comments (28)
internal/store/model/jsonfield.go (1)
62-110: Consider adding database indexes for JSONB queries.Since this implementation is part of transitioning labels and annotations to JSONB, consider adding appropriate database indexes to optimize query performance, especially for label/annotation filtering operations.
Example index types to consider:
- GIN index for exact matches on all keys and values
- BTREE index on specific commonly queried keys
- Expression indexes for specific query patterns
Would you like me to provide specific index creation statements based on your query patterns?
pkg/queryparser/sqljsonb/sqljsonb.go (3)
74-88: Enhance error feedback when overriding non-existent functions.
The error message “does not exist” inWithOverrideFunctionmay be too generic. Consider providing the function name to guide debugging.- return fmt.Errorf("does not exist") + return fmt.Errorf("function %q does not exist", name)
89-98: Type-parameterized wrapping is helpful but watch for performance impacts.
TheWrap[T any]approach is elegant for ensuring type safety; however, reflecting on each invocation can introduce overhead in performance-critical paths.
328-344: Validate argument counts consistently using descriptive errors.
ThevalidateArgsCountfunction is straightforward, but consider enriching your error messages to clarify which function triggered the validation failure. This can help debug user-defined queries.- return fmt.Errorf("function requires at least %d arguments", min) + return fmt.Errorf("function %q requires at least %d arguments", qf.Name(), min)internal/store/common.go (1)
78-85: Validate LabelSelector usage.
The new label-based filtering approach appears correct. If large datasets are expected, ensure the presence of proper indexing for performance.internal/store/selector/annotation.go (1)
243-300: parseRequirement logic is thorough.
The design for combining tokens using AND/OR/NOT is consistent. Still, ensure thorough testing of boundary cases (e.g., multiple values with NOT).internal/store/selector/selector_test.go (2)
110-113: Extending coverage for possible customfields.
The current approach is fine; adding additional test coverage for unexpected or out-of-range keys may improve robustness.
153-153: Unknown selector test is valid.
Verifies that an unrecognized selector triggers an error. You could add more negative tests for multi-segment unknown selector usage.internal/store/selector/types.go (3)
90-98: Consider validating empty selectorName.
While trimming whitespace is correct, returning an empty name might cause subtle bugs.
194-196: SelectorOpt type alias.
Using a map for options is flexible, but a named type could improve clarity if usage grows more complex.
202-202: Document the purpose of Options.
Additional inline comments describing how Options is used would help maintainers.docs/developer/architecture/field-selectors.md (2)
22-31: Improve formatting consistency in the documentation.The bullet points in the documentation use asterisks but the notes section uses dashes. Let's maintain consistency:
-* `hidden`: The selector will not be exposed to end-users during discovery. It will not appear in the list of available selectors. -* `private`: The selector cannot be directly used by the field-selector. However, it may still be utilized internally by other selectors, such as the label selector. +- `hidden`: The selector will not be exposed to end-users during discovery. It will not appear in the list of available selectors. +- `private`: The selector cannot be directly used by the field-selector. However, it may still be utilized internally by other selectors, such as the label selector.🧰 Tools
🪛 LanguageTool
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private"``` *hidden`: The selector will not be exposed to end...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. *private: The selector cannot be directly used by...(UNLIKELY_OPENING_PUNCTUATION)
68-68: Consider adding a code example for NewHiddenSelectorName.The note would be more helpful with a concrete example showing the difference between
NewSelectorNameandNewHiddenSelectorName.> - `NewSelectorName` can be replaced with `NewHiddenSelectorName` to mark the selector as hidden. This ensures it won't be exposed during discovery but can still be used internally. +> - `NewSelectorName` can be replaced with `NewHiddenSelectorName` to mark the selector as hidden: +> ```go +> // Regular selector (visible during discovery) +> selector.NewSelectorName("metadata.name") +> +> // Hidden selector (not visible during discovery) +> selector.NewHiddenSelectorName("metadata.name") +> ```🧰 Tools
🪛 LanguageTool
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > -NewSelectorNamecan be replaced with `NewHiddenSelectorName...(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
68-68: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
pkg/k8s/selector/labels/selector.go (1)
29-66: Reduce documentation duplication.The documentation for
ParseSelectorOrDieis an exact duplicate of theParsefunction documentation. This creates maintenance issues as the docs may get out of sync.Consider referencing the
Parsefunction's documentation instead:-// Parse takes a string representing a selector and returns a selector -// object, or panic when an error occur. This parsing function differs from ParseSelector -// as they parse different selectors with different syntaxes. -// The input will cause an error if it does not follow this form: -// -// <selector-syntax> ::= <requirement> | <requirement> "," <selector-syntax> -// ... +// ParseSelectorOrDie is similar to Parse but panics if an error occurs. +// See Parse for the detailed syntax documentation. func ParseSelectorOrDie(s string) selector.Selector {internal/tasks/resourcesync.go (1)
97-100: Consider wrapping the error with context.When creating the field selector fails, the error should include context about the operation being performed.
Apply this diff to improve error handling:
fs, err := selector.NewFieldSelectorFromMap(map[string]string{"metadata.owner": *owner}, false) if err != nil { - return err + return fmt.Errorf("resourcesync/%s: failed to create field selector: %w", rs.Name, err) }internal/service/repository.go (1)
70-82: Consider extracting selector parsing into a helper function.The label and field selector parsing logic is similar and could be extracted into a helper function to reduce duplication and improve maintainability.
Consider creating a helper function like this:
func parseSelector[T any]( selectorStr *string, parser func(string) (*T, error), selectorType string, ) (*T, error) { if selectorStr == nil { return nil, nil } selector, err := parser(*selectorStr) if err != nil { return nil, fmt.Errorf("failed to parse %s selector: %v", selectorType, err) } return selector, nil }Then use it like this:
- var fieldSelector *selector.FieldSelector - if request.Params.FieldSelector != nil { - if fieldSelector, err = selector.NewFieldSelector(*request.Params.FieldSelector); err != nil { - return server.ListRepositories400JSONResponse{Message: fmt.Sprintf("failed to parse field selector: %v", err)}, nil - } - } + fieldSelector, err := parseSelector(request.Params.FieldSelector, selector.NewFieldSelector, "field") + if err != nil { + return server.ListRepositories400JSONResponse{Message: err.Error()}, nil + } - var labelSelector *selector.LabelSelector - if request.Params.LabelSelector != nil { - if labelSelector, err = selector.NewLabelSelector(*request.Params.LabelSelector); err != nil { - return server.ListRepositories400JSONResponse{Message: fmt.Sprintf("failed to parse label selector: %v", err)}, nil - } - } + labelSelector, err := parseSelector(request.Params.LabelSelector, selector.NewLabelSelector, "label") + if err != nil { + return server.ListRepositories400JSONResponse{Message: err.Error()}, nil + }internal/service/enrollmentrequest.go (1)
141-146: Consider adding validation for the selector fields.While the implementation is correct, consider adding validation to ensure that only valid fields are used in the selectors to prevent runtime errors.
listParams := store.ListParams{ Limit: int(swag.Int32Value(request.Params.Limit)), Continue: cont, FieldSelector: fieldSelector, LabelSelector: labelSelector, + // Add validation for allowed fields + ValidFields: []string{"metadata.name", "metadata.creationTimestamp"}, }internal/cli/get.go (1)
85-86: Enhance selector flag descriptions with more examples.While the descriptions are clear, consider adding more examples to help users understand the full range of selector capabilities:
- fs.StringVarP(&o.LabelSelector, "selector", "l", o.LabelSelector, "Selector (label query) to filter on, supporting operators like '=', '!=', and 'in' (e.g., -l='key1=value1,key2!=value2,key3 in (value3, value4)').") - fs.StringVar(&o.FieldSelector, "field-selector", o.FieldSelector, "Selector (field query) to filter on, supporting operators like '=', '==', and '!=' (e.g., --field-selector='key1=value1,key2!=value2').") + fs.StringVarP(&o.LabelSelector, "selector", "l", o.LabelSelector, "Selector (label query) to filter on, supporting operators:\n"+ + " = : equals (e.g., -l='environment=production')\n"+ + " != : not equals (e.g., -l='environment!=dev')\n"+ + " in : value in a set (e.g., -l='environment in (staging,qa)')\n"+ + " notin : value not in a set (e.g., -l='environment notin (dev,test)')") + fs.StringVar(&o.FieldSelector, "field-selector", o.FieldSelector, "Selector (field query) to filter on, supporting operators:\n"+ + " = : equals (e.g., --field-selector='status.phase=Running')\n"+ + " == : same as = (e.g., --field-selector='metadata.name==web-1')\n"+ + " != : not equals (e.g., --field-selector='status.phase!=Failed')")internal/store/fleet.go (1)
243-249: Consider adding error handling for invalid owner format.While the field selector construction is correct, consider adding validation for the owner format to prevent potential runtime errors.
+ owner := *util.SetResourceOwner(api.FleetKind, name) + if !strings.Contains(owner, "/") { + return nil, fmt.Errorf("invalid owner format: %s", owner) + } fs, err := selector.NewFieldSelectorFromMap( - map[string]string{"metadata.owner": *util.SetResourceOwner(api.FleetKind, name)}, false) + map[string]string{"metadata.owner": owner}, false)internal/store/selector/field.go (3)
39-56: Consider replacing panic with error handling in production code.The
NewFieldSelectorFromMapOrDiefunction uses panic which could be dangerous in production environments. While it might be convenient for testing or initialization code that must not fail, consider:
- Using the non-panic version
NewFieldSelectorFromMapin production code- Adding a comment to document that this function is intended for testing/initialization only
// NewFieldSelectorFromMapOrDie creates a FieldSelector from a map of key-value pairs, // where each pair represents a field selector condition. If the operation fails, // it panics. This function is a convenience wrapper around NewFieldSelectorFromMap. // +// IMPORTANT: This function is intended for testing and initialization code only. +// For production code, use NewFieldSelectorFromMap instead. // // The `invert` parameter allows toggling between equality (`=`) and inequality (`!=`) operators // for the field selector conditions. By default, it uses equality (`=`).
Line range hint
248-257: Document JSONB array casting limitation.The code correctly prevents JSONB to array casting, but it would be helpful to document this limitation in the API documentation. Consider adding a note about this limitation in the package documentation or in relevant API documentation.
+// Note: Casting JSONB fields to arrays is not supported. This is because PostgreSQL +// does not provide a direct way to cast JSONB to array types. If you need to work +// with arrays stored in JSONB, consider using JSONB array operators instead.
354-357: Enhance error message for private field access.The error message for private field access could be more informative by including the field name.
- return nil, fmt.Errorf("field is marked as private and cannot be selected") + return nil, fmt.Errorf("field %q is marked as private and cannot be selected", selectorField.FieldName)internal/tasks/fleet_selector.go (2)
119-123: Consider adding retry logic for label selector creation.The label selector creation is a critical operation that could fail. Consider adding retry logic with exponential backoff for transient errors.
+ // Retry label selector creation with exponential backoff + var ls selector.Selector + err = util.RetryWithExponentialBackoff(ctx, func() error { + var createErr error + ls, createErr = selector.NewLabelSelectorFromMap(getMatchLabelsSafe(fleet)) + return createErr + })
241-245: Enhance error handling for field selector creation.The error handling for field selector creation could be more specific to help with debugging.
fs, err := selector.NewFieldSelectorFromMap( map[string]string{"metadata.owner": *util.SetResourceOwner(api.FleetKind, f.resourceRef.Name)}, false) if err != nil { - return err + return fmt.Errorf("failed to create field selector for fleet %s: %w", f.resourceRef.Name, err) }test/integration/store/fleet_test.go (1)
658-661: Add test cases for field selector options.The test only covers basic field selector usage. Consider adding test cases for different field selector options, especially the
WithPrivateSelectorsoption.Add test cases to verify:
- Access to private fields is properly restricted when
WithPrivateSelectorsis not used- Access to private fields is allowed when
WithPrivateSelectorsis used- Error handling when attempting to access private fields without proper authorization
internal/service/device.go (1)
69-72: Consider extracting the error message format to a constantThe error message format "failed to parse field selector: %v" could be reused across different selector parsing functions.
+const ( + errParseFieldSelector = "failed to parse field selector: %v" +) var fieldSelector *selector.FieldSelector if request.Params.FieldSelector != nil { - if fieldSelector, err = selector.NewFieldSelector(*request.Params.FieldSelector); err != nil { - return server.ListDevices400JSONResponse{Message: fmt.Sprintf("failed to parse field selector: %v", err)}, nil + if fieldSelector, err = selector.NewFieldSelector(*request.Params.FieldSelector); err != nil { + return server.ListDevices400JSONResponse{Message: fmt.Sprintf(errParseFieldSelector, err)}, nil } }internal/store/selector/field_test.go (1)
Line range hint
10-284: Consider using table-driven subtests for better test organizationThe current test structure could be improved using
t.Runwith table-driven subtests, which would:
- Provide better test isolation
- Make it easier to identify which specific test case failed
- Allow running specific test cases using
-runflagfunc TestFieldSelectorTypes(t *testing.T) { - testGoodStrings := []string{ + tests := []struct { + name string + input string + wantErr bool + }{ + { + name: "Boolean field", + input: "model.field1=true", + wantErr: false, + }, // ... other test cases } - for _, test := range testGoodStrings { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { f, err := NewFieldSelector(test) if err != nil { t.Errorf("%v: error %v (%#v)\n", test, err, err) return } // ... rest of the test + }) }internal/service/certificatesigningrequest.go (1)
93-98: Consolidate error message formatThe error message format is duplicated across different files. This reinforces the need for a shared constant.
Consider moving the error message format to a shared constants package:
// internal/service/constants/errors.go package constants const ( ErrParseFieldSelector = "failed to parse field selector: %v" ErrParseLabelSelector = "failed to parse label selector: %v" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (61)
api/v1alpha1/openapi.yaml(0 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(0 hunks)api/v1alpha1/util.go(1 hunks)docs/developer/architecture/field-selectors.md(2 hunks)docs/user/field-selectors.md(3 hunks)go.mod(0 hunks)internal/api/client/client.gen.go(0 hunks)internal/api/server/server.gen.go(0 hunks)internal/cli/get.go(1 hunks)internal/flterrors/flterrors.go(1 hunks)internal/service/certificatesigningrequest.go(1 hunks)internal/service/device.go(3 hunks)internal/service/enrollmentrequest.go(1 hunks)internal/service/fleet.go(1 hunks)internal/service/repository.go(1 hunks)internal/service/resourcesync.go(1 hunks)internal/service/templateversion.go(2 hunks)internal/store/certificatesigningrequest.go(2 hunks)internal/store/common.go(1 hunks)internal/store/common_test.go(0 hunks)internal/store/device.go(7 hunks)internal/store/enrollmentrequest.go(2 hunks)internal/store/fleet.go(4 hunks)internal/store/model/certificatesigningrequest.go(2 hunks)internal/store/model/device.go(2 hunks)internal/store/model/enrollmentrequest.go(2 hunks)internal/store/model/fleet.go(2 hunks)internal/store/model/jsonfield.go(2 hunks)internal/store/model/repository.go(2 hunks)internal/store/model/resource.go(1 hunks)internal/store/model/resourcesync.go(2 hunks)internal/store/model/selectors.go(2 hunks)internal/store/model/selectors_test.go(4 hunks)internal/store/model/templateversion.go(3 hunks)internal/store/repository.go(2 hunks)internal/store/resourcesync.go(2 hunks)internal/store/selector/annotation.go(1 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(18 hunks)internal/store/selector/field_test.go(4 hunks)internal/store/selector/label.go(1 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/selector/selector.go(7 hunks)internal/store/selector/selector_test.go(4 hunks)internal/store/selector/types.go(2 hunks)internal/store/store.go(2 hunks)internal/store/templateversion.go(1 hunks)internal/tasks/fleet_rollout.go(2 hunks)internal/tasks/fleet_selector.go(3 hunks)internal/tasks/resourcesync.go(2 hunks)internal/util/util.go(1 hunks)pkg/k8s/selector/labels/selector.go(1 hunks)pkg/queryparser/sqljsonb/sqljsonb.go(1 hunks)test/e2e/field_selectors/field_selectors_test.go(1 hunks)test/integration/store/device_test.go(4 hunks)test/integration/store/enrollmentrequest_test.go(2 hunks)test/integration/store/fleet_test.go(9 hunks)test/integration/store/repository_test.go(2 hunks)test/integration/store/resourcesync_test.go(3 hunks)test/integration/tasks/repotester_conditions_test.go(2 hunks)
💤 Files with no reviewable changes (6)
- internal/store/common_test.go
- api/v1alpha1/types.gen.go
- api/v1alpha1/openapi.yaml
- internal/api/server/server.gen.go
- internal/api/client/client.gen.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (28)
- internal/util/util.go
- test/integration/tasks/repotester_conditions_test.go
- internal/store/model/resource.go
- internal/store/model/selectors_test.go
- internal/flterrors/flterrors.go
- test/integration/store/repository_test.go
- api/v1alpha1/spec.gen.go
- internal/store/model/resourcesync.go
- internal/store/repository.go
- internal/store/model/templateversion.go
- internal/store/model/enrollmentrequest.go
- internal/store/model/device.go
- internal/store/selector/annotation_test.go
- test/integration/store/enrollmentrequest_test.go
- internal/store/selector/label_test.go
- internal/store/model/fleet.go
- internal/service/fleet.go
- test/integration/store/device_test.go
- internal/store/enrollmentrequest.go
- internal/store/model/repository.go
- docs/user/field-selectors.md
- internal/tasks/fleet_rollout.go
- internal/store/model/certificatesigningrequest.go
- internal/store/templateversion.go
- internal/store/device.go
- test/integration/store/resourcesync_test.go
- internal/store/selector/label.go
- internal/store/model/selectors.go
🧰 Additional context used
📓 Learnings (1)
pkg/queryparser/sqljsonb/sqljsonb.go (1)
Learnt from: asafbss
PR: flightctl/flightctl#751
File: pkg/queryparser/sqljsonb/sqljsonb.go:289-293
Timestamp: 2024-12-31T09:56:55.327Z
Learning: The second argument (`args[1]`) in `queryExists` within `pkg/queryparser/sqljsonb/sqljsonb.go` is always parameterized, so there is no injection vulnerability.
🪛 LanguageTool
docs/developer/architecture/field-selectors.md
[duplication] ~22-~22: Possible typo: you repeated a word
Context: ...elector. #### Using Hidden and Private Selectors Selectors can be further annotated as hidden or p...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~28-~28: Loose punctuation mark.
Context: ....labels,hidden,private" ``` *hidden`: The selector will not be exposed to end...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...ist of available selectors. * private: The selector cannot be directly used by...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~68-~68: To form a complete sentence, be sure to include a subject.
Context: ...e resource model. > - NewSelectorName can be replaced with `NewHiddenSelectorName...
(MISSING_IT_THERE)
🪛 Markdownlint (0.37.0)
docs/developer/architecture/field-selectors.md
66-66: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
67-67: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
68-68: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
52-52: Column: 1
Hard tabs
(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs
(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs
(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs
(MD010, no-hard-tabs)
56-56: Column: 1
Hard tabs
(MD010, no-hard-tabs)
57-57: Column: 1
Hard tabs
(MD010, no-hard-tabs)
61-61: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (54)
internal/store/model/jsonfield.go (4)
62-71: Well-designed type and constructor implementation!The generic type definition with appropriate constraints and nil-safe constructor implementation look good. This provides a solid foundation for the transition to JSONB storage.
73-86: Enhance error message with actual value.The error message could be more helpful by including the actual value that failed conversion, not just its type.
- return fmt.Errorf("pq: cannot convert %T to JSONMap[%T, %T]", src, new(K), new(V)) + return fmt.Errorf("pq: cannot convert %T (%+v) to JSONMap[%T, %T]", src, src, new(K), new(V))
88-96: Ensure consistent nil map handling.The direct assignment of
resultto*mcould lead to a nil map being assigned, which is inconsistent withMakeJSONMap's behavior of returning an empty map for nil input.func (m *JSONMap[K, V]) scanBytes(src []byte) error { var result map[K]V if err := json.Unmarshal(src, &result); err != nil { return fmt.Errorf("pq: could not parse JSONB: %w", err) } - *m = result + if result == nil { + *m = make(JSONMap[K, V]) + } else { + *m = result + } return nil }
98-110: Clean Value implementation!The implementation correctly handles nil maps, uses proper error wrapping, and follows database interface best practices.
internal/store/resourcesync.go (3)
49-51: LGTM: Proper error handling added for AutoMigrateThe addition of error handling for AutoMigrate operation improves reliability by surfacing potential migration failures.
53-77: Extract duplicate index creation logic into a helper functionThe index creation logic for labels and annotations is duplicated. Consider extracting this into a helper function to improve maintainability.
+func (s *ResourceSyncStore) createGinIndex(field string) error { + indexName := fmt.Sprintf("idx_resource_syncs_%s", field) + if !s.db.Migrator().HasIndex(&model.ResourceSync{}, indexName) { + if s.db.Dialector.Name() == "postgres" { + if err := s.db.Exec(fmt.Sprintf("CREATE INDEX %s ON resource_syncs USING GIN (%s)", indexName, field)).Error; err != nil { + return err + } + } else { + if err := s.db.Migrator().CreateIndex(&model.ResourceSync{}, field); err != nil { + return err + } + } + } + return nil +} func (s *ResourceSyncStore) InitialMigration() error { if err := s.db.AutoMigrate(&model.ResourceSync{}); err != nil { return err } - // Create GIN index for ResourceSync labels - if !s.db.Migrator().HasIndex(&model.ResourceSync{}, "idx_resource_syncs_labels") { - if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_resource_syncs_labels ON resource_syncs USING GIN (labels)").Error; err != nil { - return err - } - } else { - if err := s.db.Migrator().CreateIndex(&model.ResourceSync{}, "Labels"); err != nil { - return err - } - } + if err := s.createGinIndex("labels"); err != nil { + return err } - // Create GIN index for ResourceSync annotations - if !s.db.Migrator().HasIndex(&model.ResourceSync{}, "idx_resource_syncs_annotations") { - if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_resource_syncs_annotations ON resource_syncs USING GIN (annotations)").Error; err != nil { - return err - } - } else { - if err := s.db.Migrator().CreateIndex(&model.ResourceSync{}, "Annotations"); err != nil { - return err - } - } + if err := s.createGinIndex("annotations"); err != nil { + return err } return nil }
215-215: Verify annotation clearing behaviorSetting
Annotationsto nil before create/update operations could potentially clear existing annotations. Please verify this is the intended behavior and aligns with the transition from arrays to JSONB format.Run this script to check the annotation handling in other similar operations:
✅ Verification successful
Setting annotations to nil is a consistent pattern across stores
Based on the search results, setting
Annotationsto nil before create/update operations is a consistent pattern implemented across multiple store implementations:
internal/store/device.gointernal/store/repository.gointernal/store/fleet.gointernal/store/certificatesigningrequest.gointernal/store/enrollmentrequest.goThis is intentional behavior as the code shows that after setting annotations to nil:
- It retrieves the existing record
- Handles annotation merging when needed (as seen in device.go and fleet.go with
util.MergeLabels)- Properly manages existing annotations through dedicated update methods
The pattern aligns with the transition to JSONB format and ensures a clean state before processing annotations through the proper update channels.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check annotation handling patterns across the codebase # Search for annotation assignments in store operations rg -A 5 'Annotations\s*=' --type go # Search for similar patterns in other stores ast-grep --pattern 'createOrUpdate($_) { $$$ $_.Annotations = nil $$$ }'Length of output: 5416
test/e2e/field_selectors/field_selectors_test.go (1)
130-131: Validate that the test environment has devices with the labelregion=eu-west-1.This test relies on the presence of devices labeled with
region=eu-west-1. Ensure that at least one device with this label is created or managed by the test setup so the query matches expected results.You can run the following script to confirm that device definitions in your repository include the label
region=eu-west-1:✅ Verification successful
Let me check if the test sets up the required device with the label before running the test.
Let me check the device example file that's being used in the test.
The test environment correctly sets up devices with the
region=eu-west-1labelThe test uses
examples/device.yamlwhich explicitly defines a device with the labelregion=eu-west-1. The test applies this device configuration in theapply/recursivecontext before running the field selector test, ensuring that a device with the required label exists in the test environment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for device specs that define region=eu-west-1 rg -A 5 'region=eu-west-1'Length of output: 4517
Script:
#!/bin/bash # Check the test file for device setup cat test/e2e/field_selectors/field_selectors_test.goLength of output: 7235
Script:
#!/bin/bash # Check the device example file cat examples/device.yamlLength of output: 209
pkg/queryparser/sqljsonb/sqljsonb.go (4)
16-21: Overall data structure design is solid.
TheFunctionResultstruct, which contains bothArgsfor interpolation and the resulting SQLQuery, provides a clear contract and simplifies handling return values from each query function.
34-52: Verification logic is well-designed.
ThewithPrecedingKeyQuery()function carefully enforces that the first argument is theKfunction, preventing invalid usage. This approach ensures consistent query rules.
199-240: Good modular dispatcher approach.
The centralized verification logic (sqlf.Verifications) followed by function-specific handling fosters maintainability. This pattern reduces duplication and simplifies verification changes in the future.
284-294: No SQL injection concerns here.
According to your retrieved learnings,args[1]is always parameterized, so usingfmt.Sprintf("%s ? %s", args[0], args[1])does not expose an injection vector inqueryExists. This is safe given your usage constraints and the appendedgorm.Expr("?").internal/store/common.go (2)
70-75: Ensure robust error handling for FieldSelector parsing.
No immediate issue is apparent. However, ifParsefails, consider logging the underlying error to facilitate debugging.
Line range hint
87-96: Confirm AnnotationSelector coverage.
Similar to LabelSelector, ensure this path is adequately tested, particularly for edge cases like complex annotation structures or large volumes of data.internal/store/selector/annotation.go (11)
1-2: New package structure approved.
Package naming and declaration conform to standard Go practices.
3-16: Imports check.
The imported packages appear relevant, and there's no duplication or unused imports.
18-22: AnnotationSelector struct introduced.
Defines fields for extended annotation-based selectors. The design is clear for future extension.
24-43: Typographical fix in comment.
Line 25 uses "fais" instead of "fails" and has already been flagged in a past review.
45-74: Creation from map looks good.
Provides a convenient way to generate an annotation selector from a map of key-value pairs, supporting optional negation.
76-93: Typographical fix in comment.
Line 77 also contains "fais" instead of "fails," a repeated issue. Please replace "fais" with "fails."
95-134: Robust annotation parsing.
Uses Kubernetes-style label selector syntax. Validates annotation keys and aggregates errors properly.
136-198: Parse method is clear and well-structured.
Ensures the field is JSONB and handles multiple fields gracefully with robust error checking.
200-241: Tokenize method has solid error handling.
No immediate concerns. Confirm that any concurrency usage is carefully managed if introduced later.
302-314: Potential injection vector previously discussed.
At line 312, the string interpolation is used as part of parameterized SQL. This has been acknowledged in past reviews.
316-326: queryField override looks appropriate.
No issues detected. Implementation ensures only expected arguments are accepted.internal/store/store.go (2)
8-8: New selector import recognized.
Switching to the internalselectorpackage aligns with the new architecture.
133-137: ListParams structure improved.
Migrating from single-purpose fields to dedicated selectors (FieldSelector, LabelSelector, AnnotationSelector) simplifies filtering logic.internal/store/selector/selector_test.go (2)
30-33: MapSelectorName usage.
Logic looks correct. Consider adding test cases ensuring coverage when the mapping does not match known selectors.
78-85: ListSelectors returning known selectors.
Straightforward approach. Good for enumerating test coverage of recognized fields.internal/store/selector/types.go (3)
86-88: Interface definition for SelectorName.
Straightforward way to abstract various selector name types.
100-102: SelectorName.String is correct.
Implements the required interface method as expected.
105-117: Hidden selector name usage.
This specialized type for non-exposed selectors is a neat separation of concerns.docs/developer/architecture/field-selectors.md (1)
52-55: LGTM! Clear and helpful example.The example effectively demonstrates the proper usage of
NewSelectorNamefor mapping selectors.🧰 Tools
🪛 Markdownlint (0.37.0)
52-52: Column: 1
Hard tabs(MD010, no-hard-tabs)
53-53: Column: 1
Hard tabs(MD010, no-hard-tabs)
54-54: Column: 1
Hard tabs(MD010, no-hard-tabs)
55-55: Column: 1
Hard tabs(MD010, no-hard-tabs)
internal/service/templateversion.go (2)
44-63: LGTM! Well-structured selector handling.The implementation properly handles both field and label selectors with appropriate error handling. The separation of field and label selector logic improves code clarity.
104-113: LGTM! Consistent with design decisions.The field selector implementation and error handling align with the established patterns:
- 403 status code for invalid fleet values is appropriate as it indicates a permission/access issue
- Different error handling between List and Delete operations is intentional
api/v1alpha1/util.go (2)
348-363: Well-structured implementation!The function efficiently handles string concatenation using
strings.Builderand properly manages edge cases.
374-395: Consider handling additional standard operators.The switch statement only handles a subset of operators (Exists, DoesNotExist, In, NotIn). Consider handling other standard operators like Equals, NotEquals, etc., or document why only these operators are supported.
internal/service/resourcesync.go (3)
69-74: LGTM! Robust field selector parsing implementation.The implementation properly handles field selector parsing with appropriate error handling and type safety.
76-81: LGTM! Well-structured label selector parsing.The label selector parsing is implemented with proper error handling and follows the same pattern as field selector parsing, maintaining consistency.
82-87: LGTM! Clean integration with ListParams.The updated ListParams structure properly integrates both selectors while maintaining backward compatibility.
internal/store/selector/selector.go (3)
80-88: LGTM! Clean integration of selector tag parsing.The integration of selector tag parsing is well-structured and maintains proper error handling.
236-245: LGTM! Proper handling of hidden selectors.The implementation correctly filters out hidden selectors when displaying supported selectors in error messages.
338-364: LGTM! Well-documented and robust tag parsing.The parseSelectorTag function is well-documented and handles edge cases properly:
- Empty tag handling
- Proper trimming of whitespace
- Efficient option storage using map
internal/store/certificatesigningrequest.go (2)
208-208: LGTM! Proper handling of annotations.The explicit setting of annotations to nil is consistent with the PR's objectives of transitioning to JSONB format.
63-74: 🛠️ Refactor suggestionApply consistent index naming for annotations.
Similar to the labels index, the annotations index naming should be standardized.
Apply the same naming convention to the annotations index:
-if !s.db.Migrator().HasIndex(&model.CertificateSigningRequest{}, "idx_csr_annotations") { +const annotationsIndexName = "idx_csr_annotations" +if !s.db.Migrator().HasIndex(&model.CertificateSigningRequest{}, annotationsIndexName) { if s.db.Dialector.Name() == "postgres" { - if err := s.db.Exec("CREATE INDEX idx_csr_annotations ON certificate_signing_requests USING GIN (annotations)").Error; err != nil { + if err := s.db.Exec("CREATE INDEX " + annotationsIndexName + " ON certificate_signing_requests USING GIN (annotations)").Error; err != nil { return err } } else { - if err := s.db.Migrator().CreateIndex(&model.CertificateSigningRequest{}, "Annotations"); err != nil { + if err := s.db.Migrator().CreateIndex(&model.CertificateSigningRequest{}, annotationsIndexName); err != nil { return err } }Likely invalid or redundant comment.
internal/service/enrollmentrequest.go (2)
128-133: LGTM! Field selector initialization and error handling looks good.The code properly initializes the field selector and handles potential parsing errors with clear error messages.
135-140: LGTM! Label selector initialization and error handling looks good.The code properly initializes the label selector and handles potential parsing errors with clear error messages.
internal/store/fleet.go (2)
59-89: LGTM! Proper error handling and index creation.The code properly handles errors during migration and creates appropriate GIN indexes for labels and annotations, which will improve query performance.
514-522: LGTM! Improved annotations handling.The code properly handles annotations using
util.EnsureMapandmodel.MakeJSONMap, which ensures consistent behavior and proper JSON storage.test/integration/store/fleet_test.go (1)
247-252: Add test cases for complex label expressions.Based on the past review comment about MatchExpression integration, consider adding test cases for more complex label expressions to ensure the selector framework handles them correctly.
Add test cases for:
- Multiple match expressions combined with AND/OR operators
- Nested expressions
- Edge cases with special characters in label values
internal/service/device.go (2)
Line range hint
100-105: LGTM! Consistent implementation with field selectorThe label selector initialization follows the same pattern as the field selector, maintaining code consistency.
Line range hint
111-111: LGTM! Proper integration of label selectorThe label selector is correctly integrated into the ListParams structure.
internal/service/certificatesigningrequest.go (2)
100-105: LGTM! Consistent implementation across servicesThe label selector initialization follows the same pattern used in other services, maintaining consistency throughout the codebase.
111-111: LGTM! Consistent integration of label selectorThe label selector is integrated into the ListParams structure consistently with other services.
| // labels := map[string]string{"env": "prod", "tier": "backend"} | ||
| // selector := NewLabelSelectorFromMapOrDie(labels) | ||
| // // selector represents: "env=prod,tier=backend" | ||
| func NewLabelSelectorFromMapOrDie(labels map[string]string, invert ...bool) *LabelSelector { |
There was a problem hiding this comment.
- Why panic instead of returning an error?
- I get that you want "invert" to be optional, but it's not clear because it looks I can/should pass multiple values. I don't feel too strongly about it, and I saw that it's documented above as optional, but I found it confusing.
There was a problem hiding this comment.
-
ParseSelectorOrDie is a practice used by the Kubernetes package. It simplifies specific cases and is primarily utilized in tests or scenarios where the query is created by the backend, ensuring no potential mistakes from end-user input. For safer handling of errors, we also provide the NewAnnotationSelectorFromMap function, which processes errors accordingly.
-
I'll fix the 'invert'
| // annotations := map[string]string{"env": "prod", "tier": "backend"} | ||
| // selector := NewAnnotationSelectorFromMapOrDie(annotations) | ||
| // // selector represents: "env=prod,tier=backend" | ||
| func NewAnnotationSelectorFromMapOrDie(annotations map[string]string, invert ...bool) *AnnotationSelector { |
There was a problem hiding this comment.
Same comment as the labels
| @@ -0,0 +1,326 @@ | |||
| package selector | |||
There was a problem hiding this comment.
Is this file basically duplicating the fleet version? Can we avoid it?
There was a problem hiding this comment.
Hey, 'fleet version'?
Both label and annotation selectors share the same underlying logic, differing only in validation and specific error messages indicating which selector is being used. I considered creating a unified implementation or embedding functionality for both, but it became overly complex. For now, I’ve decided to maintain two similar versions that follow the same logic. The tokenizer code is straightforward and simple, making this approach manageable.
We might consider unifying them if additional fields are required or if maintaining duplicated code becomes inefficient.
asafbss
force-pushed
the
refactor-label-selector
branch
from
43cd381 to
0329db9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/store/selector/label_test.go (1)
27-27:⚠️ Potential issueDefine or import missing goodTestModel.
The test uses
goodTestModelwhich is not defined in this file.Please either:
- Define the model in this file
- Import it from another package
- Add a comment indicating where the model is defined
🧹 Nitpick comments (13)
internal/store/selector/label_test.go (6)
11-18: Enhance test coverage with additional test cases.The test cases only cover basic scenarios. Consider adding:
- Empty string
- Multiple conditions (e.g., "key1=val1,key2=val2")
- Special characters in keys/values
- Very long keys/values
- Unicode characters
22-30: Improve error handling and validation.The test only checks if errors occur but doesn't:
- Validate the actual parsed results
- Assert specific error types/messages
- Provide context in error messages about what aspect failed
Consider refactoring to:
ls, err := NewLabelSelector(test) if err != nil { - t.Errorf("%v: error %v (%#v)\n", test, err, err) + t.Fatalf("Failed to create label selector for %q: %v", test, err) continue } _, _, err = ls.Parse(context.Background(), &goodTestModel{}, NewSelectorName("model.field16")) if err != nil { - t.Errorf("%v: error %v (%#v)\n", test, err, err) + t.Errorf("Failed to parse selector %q: %v", test, err) } + + // Add validation of parsed results + // TODO: Add assertions for expected parsing outcomes
46-58: Improve readability of test expectations.The test expectations are hard to maintain and verify due to long string literals. Consider using a helper function or constants to build the expected strings.
Example:
func buildExpectedQuery(op string, key string, values ...string) string { switch op { case "exists": return fmt.Sprintf("EXISTS(K(field16),V(%s))", key) case "equals": return fmt.Sprintf("AND(EXISTS(K(field16),V(%s)),CONTAINS(K(field16),V({\"%s\": \"%s\"})))", key, key, values[0]) // ... other cases } return "" } testGoodOperations := map[string]string{ "key": buildExpectedQuery("exists", "key"), "key=val": buildExpectedQuery("equals", "key", "val"), // ... }
111-114: Enhance test structure documentation.The test struct could be more descriptive and include additional test metadata.
type testMapOperation struct { + name string // Test case description input map[string]string expected string + wantErr bool // Whether this test should error }
119-146: Add negative test cases for map operations.The test suite only includes valid cases. Consider adding test cases for:
- Maps with invalid keys/values
- Maps with special characters
- Maps with very long keys/values
- Maps with empty keys/values
1-184: Implement consistent error handling across test functions.All test functions follow similar error handling patterns that could be improved for consistency and clarity.
Consider creating test helper functions:
func assertNoError(t *testing.T, err error, format string, args ...interface{}) { t.Helper() if err != nil { t.Fatalf(format+": %v", append(args, err)...) } } func assertTokenMatch(t *testing.T, set1, set2 *queryparser.TokenSet, desc string) { t.Helper() if !set1.Matches(set2) { t.Errorf("%s: tokens do not match\nwant: %v\ngot: %v", desc, set2, set1) } }This would improve error messages and reduce code duplication across all test functions.
internal/store/selector/annotation.go (1)
59-62: Consider returning a more explicit placeholder for an empty map
When no annotations are supplied, returningNewAnnotationSelector("")silently implies "no filtering." For maintainability, consider returning a dedicated constant or a more descriptive helper function that clarifies this behavior.internal/service/resourcesync.go (1)
69-81: Unified approach to field/label selectors
Instantiating bothFieldSelectorandLabelSelectoris consistent. To reduce boilerplate, consider a centralized helper that parses both from request parameters.internal/store/selector/field.go (2)
21-25: Well-designed selector framework with privacy controls!The enhanced
FieldSelectorstructure andWithPrivateSelectorsoption provide a robust foundation for handling both public and private selectors. The implementation follows good encapsulation practices and provides clear control over selector visibility.Also applies to: 30-37
354-358: Good implementation of field resolution with privacy controls!The field resolution implementation properly handles:
- Privacy checks for restricted fields
- Fallback mechanism for hidden selectors
- Clear error messages
Consider adding debug logging to help troubleshoot field resolution issues in production.
func (fs *FieldSelector) resolveSelectorField(key string) ([]*SelectorField, error) { + log := logrus.WithField("key", key) resolvedFields, err := fs.fieldResolver.ResolveFields(NewSelectorName(key)) if err != nil { + log.Debug("Falling back to hidden selector resolution") // Fallback to resolving as a hidden selector return fs.fieldResolver.ResolveFields(NewHiddenSelectorName(key)) } + log.WithField("count", len(resolvedFields)).Debug("Resolved selector fields") return resolvedFields, nil }Also applies to: 610-618
internal/tasks/fleet_selector.go (1)
255-267: Consider adding documentation for the combined selector logic.While the implementation is solid, adding a comment explaining how the LabelSelector and FieldSelector work together in this context would improve code maintainability.
Add a comment like:
+// Use both label and field selectors to find devices that: +// 1. Don't match the fleet's label selector (inverse match with negated=true) +// 2. Are still owned by this fleet (field selector on metadata.owner) // Create a new LabelSelector from the fleet's match labels. ls, err := selector.NewLabelSelectorFromMap(getMatchLabelsSafe(fleet), true)test/integration/store/fleet_test.go (2)
236-237: Consider adding error case test for LabelSelector creation.While the happy path is well tested, consider adding a test case for invalid label selectors to ensure proper error handling.
658-661: Consider expanding field selector test coverage.While the current test covers the basic field selector functionality, consider adding test cases for:
- Multiple field selectors
- Invalid field selector syntax
- Non-existent fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
api/v1alpha1/openapi.yaml(0 hunks)api/v1alpha1/spec.gen.go(1 hunks)api/v1alpha1/types.gen.go(0 hunks)internal/api/client/client.gen.go(0 hunks)internal/api/server/server.gen.go(0 hunks)internal/cli/get.go(1 hunks)internal/service/device.go(3 hunks)internal/service/fleet.go(1 hunks)internal/service/resourcesync.go(1 hunks)internal/service/templateversion.go(2 hunks)internal/store/common.go(1 hunks)internal/store/common_test.go(0 hunks)internal/store/fleet.go(4 hunks)internal/store/selector/annotation.go(1 hunks)internal/store/selector/annotation_test.go(1 hunks)internal/store/selector/field.go(18 hunks)internal/store/selector/label.go(1 hunks)internal/store/selector/label_test.go(1 hunks)internal/store/store.go(2 hunks)internal/tasks/fleet_rollout.go(2 hunks)internal/tasks/fleet_selector.go(3 hunks)internal/tasks/resourcesync.go(2 hunks)test/integration/store/device_test.go(4 hunks)test/integration/store/enrollmentrequest_test.go(2 hunks)test/integration/store/fleet_test.go(9 hunks)test/integration/store/repository_test.go(2 hunks)test/integration/store/resourcesync_test.go(3 hunks)
💤 Files with no reviewable changes (5)
- api/v1alpha1/openapi.yaml
- api/v1alpha1/types.gen.go
- internal/store/common_test.go
- internal/api/server/server.gen.go
- internal/api/client/client.gen.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/tasks/fleet_rollout.go
- api/v1alpha1/spec.gen.go
- test/integration/store/enrollmentrequest_test.go
- test/integration/store/repository_test.go
- internal/service/fleet.go
- internal/service/device.go
- test/integration/store/resourcesync_test.go
- internal/service/templateversion.go
- test/integration/store/device_test.go
- internal/store/selector/label.go
🔇 Additional comments (22)
internal/store/selector/label_test.go (1)
60-63: Expand invalid test cases.The test only checks basic invalid characters. Consider adding:
- Empty strings
- Invalid operators
- Malformed expressions
- Missing closing parentheses
- Invalid JSON in values
internal/store/common.go (3)
70-75: Ensure robust sanitization for field-based queries
TheFieldSelector.Parseresult is inserted directly into the SQL query. Verify that any user-provided query strings are properly sanitized and validated withinParseto guard against SQL injection or malformed queries.
78-85: Confirmmetadata.labelsalignment with the database schema
Your use ofselector.NewHiddenSelectorName("metadata.labels")looks consistent with the new label selector approach. However, please ensure that the underlying column or JSONB path is indeed named"metadata.labels"in the database, and that corresponding tests cover label-based filtering thoroughly.
Line range hint
87-95: Confirmmetadata.annotationsalignment with the database schema
Similar to the label selector, verify that"metadata.annotations"correctly matches the DB or JSONB column path in which annotations are stored. Include relevant tests to ensure accuracy and prevent discrepancies between the selector logic and schema.internal/store/selector/annotation.go (4)
25-25: Typo in comment: 'fais' should be 'fails'
This mirrors a past comment. Please correct to maintain clarity.
182-185: Parser initialization might cause nil references
As noted in a previous review, if the parser is not properly initialized or is nil, subsequent Tokenize or parse operations may cause a nil pointer dereference.
212-215: Returning nil instead of an error may mask issues
Returning(nil, nil)when the selector is not selectable could hide real errors and complicate debugging.
312-313: Potential injection risk in string interpolation
Even though code discussions indicate it’s parameterized later, thisfmt.Sprintf("{\"%s\": \"%s\"}", key, value)pattern may be abused if used in an unsafe query context.internal/store/store.go (1)
133-137: New fields look good; ensure usage consistency
These additional fields (FieldSelector,LabelSelector,AnnotationSelector) help unify filtering. Confirm that all consumers ofListParamsproperly handle these fields to avoid incongruent filtering behaviors.internal/store/selector/annotation_test.go (2)
60-62: Expand negative test coverage
Similar to a previous review, consider extending invalid inputs beyond just special characters. Include tests for empty selectors, malformed operators, and boundary cases to ensure robust validation.
83-83: Check array bounds before accessing resolvedFields[0]
AccessingresolvedFields[0]may panic if the slice is empty. Add an explicit check to prevent runtime errors.internal/tasks/resourcesync.go (1)
97-104: Verify non-nil owner reference for field selector
Using*ownerinmap[string]string{"metadata.owner": *owner}can panic ifowneris nil. Ensure thatowneris always set to avoid runtime errors.internal/service/resourcesync.go (1)
87-87: Sufficient support for advanced filtering
The newLabelSelectorintegration inListParamsaligns well with the rest of the codebase, ensuring consistent, flexible filtering in listing operations.internal/cli/get.go (1)
85-86: LGTM! Clear and accurate documentation.The updated descriptions for label and field selectors accurately reflect the supported operators, providing clear guidance for users.
internal/store/fleet.go (3)
59-89: Great addition of GIN indexes and error handling!The implementation properly handles database migrations with:
- Proper error handling for AutoMigrate
- GIN indexes for both labels and annotations, improving query performance
- Dialect-specific handling for PostgreSQL
243-249: LGTM! Clean transition to the new selector framework.The code successfully integrates with the new selector framework, replacing the old owner filtering mechanism with a more flexible field selector approach.
514-522: LGTM! Improved annotation handling.The changes align with the new approach for handling annotations, using more efficient map operations with
util.EnsureMapandmodel.MakeJSONMap.internal/store/selector/field.go (2)
39-158: Excellent constructor functions with comprehensive documentation!The new constructor functions are well-documented with clear examples and proper error handling. The implementation provides both panic and non-panic versions for different use cases.
Line range hint
188-213: Robust parsing and tokenization implementation!The enhanced Parse and Tokenize methods provide:
- Improved error handling with clear messages
- Proper validation of field resolvers
- Clean separation of concerns
Also applies to: 215-299
internal/tasks/fleet_selector.go (3)
12-12: Clean import addition!The addition of the selector package import aligns well with the codebase structure and the PR's objective of integrating label and annotation filtering.
119-123: Robust error handling for label selector creation!Good practice to handle potential errors when creating the label selector. This prevents invalid selectors from propagating through the system.
241-245: Well-structured field selector implementation!The transition to using FieldSelector for owner filtering is well implemented with proper error handling. This aligns with the PR's objective of consolidating filtering logic within the selector framework.
| fs.StringVar(&o.Owner, "owner", o.Owner, "filter by owner") | ||
| fs.StringVarP(&o.LabelSelector, "selector", "l", o.LabelSelector, "Selector (label query) to filter on, as a comma-separated list of key=value.") | ||
| fs.StringVar(&o.FieldSelector, "field-selector", o.FieldSelector, "Selector (field query) to filter on, supporting operators like '=', '==', and '!=' (e.g., --field-selector='key1=value1,key2=value2').") | ||
| fs.StringSliceVar(&o.StatusFilter, "status-filter", o.StatusFilter, "Filter the results by status field path using key-value pairs. Example: --status-filter=updated.status=UpToDate") |
3 participants
This PR enhances the Flight Control filtering capabilities by:
Supports advanced operators such as exists, not exists, =, !=, in, and notin.
Consolidates filtering logic within the unified selector framework for consistency.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactoring
Chores