HYPERFLEET-536 - feat: add condition subfield queries for selective Sentinel polling#71
Open
rafabene wants to merge 8 commits intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-536 - feat: add condition subfield queries for selective Sentinel polling#71rafabene wants to merge 8 commits intoopenshift-hyperfleet:mainfrom
rafabene wants to merge 8 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds support for querying condition subfields (status.conditions..) by preprocessing 4-part paths into a 3-part form, mapping TSL operators to SQL operators, and converting subfield queries to SQL with appropriate casting for timestamptz and integers. Introduces detection helpers to reject NOT on condition queries, adds expression indexes and an immutable timestamptz wrapper via a migration, and flips Ready->False on spec changes by updating status_conditions. Tests, factories, and integration tests were extended to cover subfield queries and time-based behaviors. Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as pkg/services/generic.go
participant Preprocessor as pkg/db/sql_helpers.go (PreprocessConditionSubfields)
participant Parser as TSL Parser
participant Converter as pkg/db/sql_helpers.go (converters)
participant DB as Database
Client->>Service: Search request with query
Service->>Service: Validate length (<=4096)
Service->>Preprocessor: PreprocessConditionSubfields(query)
Preprocessor-->>Service: Preprocessed query
Service->>Parser: ParseTSL(preprocessed query)
Parser-->>Service: AST
Service->>Converter: Convert condition nodes
alt condition subfield (4-part)
Converter->>Converter: Validate operator & subfield
Converter->>DB: SQL expression with casting (timestamptz/int)
else condition status (3-part)
Converter->>DB: JSON path SQL expression
end
DB-->>Service: Results
Service-->>Client: Response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api-resources.md`:
- Around line 453-456: The documentation currently lists operators as
universally supported but the field status.conditions.<Type> only supports
equality in code (see conditionStatusConverter in pkg/db/sql_helpers.go); update
the docs to enumerate operator support by field category (e.g., equality-only
for status.conditions.<Type>, full comparison for numeric/time fields, in-list
for array/list fields, etc.), explicitly call out status.conditions.<Type> as
equality-only and map each operator to the corresponding field types so the API
contract matches the implementation.
In `@pkg/db/sql_helpers_test.go`:
- Around line 320-325: Tighten the negative-case assertions in
pkg/db/sql_helpers_test.go so that when tt.expectError is true you not only
assert err != nil but also verify the error message contains tt.errorContains;
update the block in the test (the branch checking tt.expectError and using the
err variable) to assert the error string contains tt.errorContains (e.g., via
strings.Contains(err.Error(), tt.errorContains) or your test helper like
require.ErrorContains) before returning so mismatched errors fail the test.
In `@pkg/db/sql_helpers.go`:
- Around line 172-173: PreprocessConditionSubfields currently applies
conditionSubfieldPattern.ReplaceAllString to the whole query and thus mutates
text inside quoted string literals; change it to only run the replacement on
non-quoted identifier tokens by scanning the input and skipping content inside
single and double quotes (handling escaped quotes), e.g. split the string into
segments or use a simple state machine that toggles in-quote/out-of-quote and
apply conditionSubfieldPattern.ReplaceAllString only when out-of-quote; update
the PreprocessConditionSubfields function and keep reference to
conditionSubfieldPattern so only unquoted segments are transformed.
- Around line 323-331: The code currently takes r.Left as a string (rightStr)
and builds a ::timestamptz predicate that defers timestamp parsing to the DB;
instead validate rightStr before building the query: attempt to parse rightStr
with the expected timezone-aware layouts (e.g. RFC3339 / acceptable formats)
using time.Parse/ParseInLocation, and if parsing fails return
errors.BadRequest("invalid timestamp for condition subfield '%s': %v", subfield,
parseErr); only when parsing succeeds proceed to construct the query string
(using sqlOp, jsonPath, subfield) and return sq.Expr(..., rightStr), nil so
invalid literals are rejected with a 400.
- Around line 336-345: The code currently casts r.Left (rightVal float64)
directly to int before building the SQL expression (sq.Expr), which silently
truncates fractions and can overflow PostgreSQL INTEGER; update the validation
around r.Left (rightVal) to reject non-integer values (e.g., require rightVal ==
math.Trunc(rightVal) or use math.Modf) and reject values outside the 32-bit
signed int range (<= -2147483648 or >= 2147483648), returning errors.BadRequest
with a clear message mentioning subfield (e.g., "expected integer within 32-bit
range for condition subfield '%s'"); only after those checks convert to an int
(or int32) and pass that to sq.Expr(query, jsonPath, subfield, intSafe) so
observed_generation and similar fields are validated and not truncated or
overflowed.
In `@test/factories/clusters.go`:
- Around line 165-166: The DB session is created using context.Background()
(dbFactory.New) which bypasses request-scoped context; update the helper to
accept a context parameter and use that when creating the session (pass the
existing ctx into the helper instead of context.Background()), and replace the
dbFactory.New(...) call with the context-aware session creation (use
db.NewContext(ctx) or dbFactory.NewContext/dbFactory.New(ctx) per your DB
factory API) so the call that sets err = dbSession.Model(cluster).Update(...)
uses a request-scoped context; locate the dbFactory.New invocation in
clusters.go and the helper signature to add a ctx parameter and thread it
through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f24f421-2616-4edd-8e19-5b68594f5372
📒 Files selected for processing (6)
docs/api-resources.mdpkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/services/generic.gotest/factories/clusters.gotest/integration/search_field_mapping_test.go
Contributor
Author
|
Related architecture docs PR: openshift-hyperfleet/architecture#99 |
2 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/db/sql_helpers.go (1)
182-202: Escaped quotes inside string literals are not handled.The quote-aware state machine doesn't account for escaped quotes (e.g.,
'it\'s'or"say \"hello\""). If a user provides a query with escaped quotes, the state machine will prematurely exit the quoted segment, potentially transforming text that should remain untouched.This is a minor edge case since condition paths are unlikely to appear in user-provided string values with escaped quotes, but worth noting for completeness.
💡 Suggested fix to handle escaped quotes
for i := 0; i < len(search); i++ { ch := search[i] if inQuote { + // Skip escaped characters + if ch == '\\' && i+1 < len(search) { + i++ // Skip the next character + continue + } if ch == quoteChar { // Flush quoted segment as-is (no replacement) result.WriteString(search[segStart : i+1]) segStart = i + 1 inQuote = false } continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/sql_helpers.go` around lines 182 - 202, The loop that scans the SQL fragment in pkg/db/sql_helpers.go (using variables search, result, segStart, inQuote, quoteChar and conditionSubfieldPattern) doesn't treat escaped quotes as part of a quoted literal; update the state machine so that when encountering a quote character (ch == quoteChar) you first check whether it is escaped (preceded by an odd number of backslashes) and only close the quoted segment if it is not escaped, otherwise treat it as part of the quote; ensure flushing logic for quoted and unquoted segments (the result.WriteString and pattern.ReplaceAllString calls around segStart and i) remains correct when skipping escaped quotes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/db/sql_helpers.go`:
- Around line 182-202: The loop that scans the SQL fragment in
pkg/db/sql_helpers.go (using variables search, result, segStart, inQuote,
quoteChar and conditionSubfieldPattern) doesn't treat escaped quotes as part of
a quoted literal; update the state machine so that when encountering a quote
character (ch == quoteChar) you first check whether it is escaped (preceded by
an odd number of backslashes) and only close the quoted segment if it is not
escaped, otherwise treat it as part of the quote; ensure flushing logic for
quoted and unquoted segments (the result.WriteString and
pattern.ReplaceAllString calls around segStart and i) remains correct when
skipping escaped quotes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d567d38e-d25f-4a40-bf2b-7ee79e522713
📒 Files selected for processing (3)
docs/api-resources.mdpkg/db/sql_helpers.gopkg/db/sql_helpers_test.go
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
ciaranRoche
reviewed
Mar 10, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/dao/conditions_test.go (1)
11-113: Use the repo's Gomega test style in these new DAO tests.These cases introduce raw
t.Fatalf/t.Errorfassertions instead of the usualRegisterTestingT(t)+ Gomega matchers, so they don't follow the test pattern used elsewhere in the repo.As per coding guidelines:
**/*_test.go: Test patterns: use Gomega assertions withRegisterTestingT(t).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dao/conditions_test.go` around lines 11 - 113, The tests TestResetReadyConditionOnSpecChange, TestResetReadyConditionOnSpecChange_EmptyConditions and TestResetReadyConditionOnSpecChange_InvalidJSON use raw t.Fatalf/t.Errorf; update them to the repo's Gomega style by calling gomega.RegisterTestingT(t) at the start of each subtest or test and replace t.Fatalf/t.Errorf checks with appropriate gomega.Expect(...) matchers (e.g. Expect(err).ToNot(HaveOccurred()), Expect(result).To(BeNil()) or Expect(string(result)).To(Equal(string(input))), Expect(cond.Status).To(Equal(api.ConditionFalse)) / To(Equal(api.ConditionTrue)), Expect(cond.Reason).ToNot(BeNil()) and Expect(*cond.Reason).To(Equal("SpecChanged")), and Expect(cond.LastTransitionTime.Equal(now)).To(BeTrue()) when asserting behavior of resetReadyConditionOnSpecChange).pkg/db/sql_helpers_test.go (1)
10-146: Use the repo’s Gomega test pattern in these new cases.These additions keep extending the legacy
t.Errorf/t.Fatalfstyle instead of the repository-standard assertion flow. Please switch the new tests to Gomega and callRegisterTestingT(t)in each test entry point so this file stays aligned with the rest of the suite.As per coding guidelines, "Test patterns: use Gomega assertions with RegisterTestingT(t)".
Also applies to: 148-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/sql_helpers_test.go` around lines 10 - 146, Replace the legacy t.Errorf/t.Fatalf style in TestConditionsNodeConverterStatus with the repo-standard Gomega pattern: call RegisterTestingT(t) at the start of each t.Run subtest, import and use Gomega matchers (Expect/To/ContainSubstring/BeNil/Equal/HaveLen etc.) for all assertions (e.g., SQL, args, errors) instead of t.Errorf checks; update assertions around the call to conditionsNodeConverter and the sqlizer.ToSql() results to use Expect, and apply the same Gomega conversion to the other tests in this file (lines 148-577) that exercise conditionsNodeConverter and related tsl.Node-based cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/search.md`:
- Around line 131-151: The docs fail to warn that using NOT with condition
subfield expressions (e.g., NOT status.conditions.<Type>.last_updated_time <
'...') is rejected; update the "Supported Operators" /
`status.conditions.<Type>` note to explicitly state that the NOT operator is not
supported for condition subfield queries and will result in a 400 error, and
suggest using positive expressions combined with other boolean operators
(AND/OR) or restructuring the query instead; mention the specific symbol forms
`status.conditions.<Type>` and examples like
`status.conditions.Ready.last_updated_time` to make the limitation discoverable.
In `@pkg/db/migrations/202603100001_add_conditions_subfield_indexes.go`:
- Around line 32-52: Expression indexes idx_clusters_ready_last_updated_time and
idx_node_pools_ready_last_updated_time embed literal JSONPath/field names that
won't match query builder bind-parameter predicates; instead add a
STORED/generated column on clusters and node_pools (e.g.,
ready_last_updated_time) computed with
immutable_timestamptz(jsonb_path_query_first(status_conditions, '$[*] ? (@.type
== "Ready")') ->> 'last_updated_time') cast to timestamptz, create a BTREE index
on that generated column, and update any queries to filter/order by the new
ready_last_updated_time column so the planner can use the index; ensure you
reference the generated column name and the original
status_conditions/jsonb_path_query_first expression in the migration change.
In `@pkg/db/sql_helpers.go`:
- Around line 436-444: The NOT-operator guard only inspects the direct child
node and misses conditions deeper in the NOT subtree; update the check in the
block where you test n.Func == tsl.NotOp to recursively scan the entire NOT
subtree using hasCondition instead of only testing the immediate child (i.e.,
call hasCondition on the full left subtree node rather than relying on a
single-level type assertion). Keep the error return unchanged; refer to symbols
n.Func, tsl.NotOp, n.Left and hasCondition to locate and modify the logic so any
condition anywhere under a NOT triggers the BadRequest.
---
Nitpick comments:
In `@pkg/dao/conditions_test.go`:
- Around line 11-113: The tests TestResetReadyConditionOnSpecChange,
TestResetReadyConditionOnSpecChange_EmptyConditions and
TestResetReadyConditionOnSpecChange_InvalidJSON use raw t.Fatalf/t.Errorf;
update them to the repo's Gomega style by calling gomega.RegisterTestingT(t) at
the start of each subtest or test and replace t.Fatalf/t.Errorf checks with
appropriate gomega.Expect(...) matchers (e.g. Expect(err).ToNot(HaveOccurred()),
Expect(result).To(BeNil()) or Expect(string(result)).To(Equal(string(input))),
Expect(cond.Status).To(Equal(api.ConditionFalse)) /
To(Equal(api.ConditionTrue)), Expect(cond.Reason).ToNot(BeNil()) and
Expect(*cond.Reason).To(Equal("SpecChanged")), and
Expect(cond.LastTransitionTime.Equal(now)).To(BeTrue()) when asserting behavior
of resetReadyConditionOnSpecChange).
In `@pkg/db/sql_helpers_test.go`:
- Around line 10-146: Replace the legacy t.Errorf/t.Fatalf style in
TestConditionsNodeConverterStatus with the repo-standard Gomega pattern: call
RegisterTestingT(t) at the start of each t.Run subtest, import and use Gomega
matchers (Expect/To/ContainSubstring/BeNil/Equal/HaveLen etc.) for all
assertions (e.g., SQL, args, errors) instead of t.Errorf checks; update
assertions around the call to conditionsNodeConverter and the sqlizer.ToSql()
results to use Expect, and apply the same Gomega conversion to the other tests
in this file (lines 148-577) that exercise conditionsNodeConverter and related
tsl.Node-based cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c7fdb8b-7e01-42d7-9000-155854d94403
📒 Files selected for processing (13)
docs/search.mdpkg/dao/cluster.gopkg/dao/conditions.gopkg/dao/conditions_test.gopkg/dao/node_pool.gopkg/db/migrations/202603100001_add_conditions_subfield_indexes.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/db/sql_helpers_test.gopkg/services/generic.gotest/factories/clusters.gotest/factories/node_pools.gotest/integration/search_field_mapping_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/search_field_mapping_test.go
Contributor
Author
rafabene
force-pushed
the
HYPERFLEET-536
branch
from
c2d0653 to
f25b258
Compare
…entinel polling Support querying condition subfields (last_updated_time, last_transition_time, observed_generation) with comparison operators, enabling Sentinel to selectively fetch only not-ready and stale-ready resources instead of all resources.
- Fix operator docs to show per-field-type support table - Fix weak error assertions in condition converter tests - Make PreprocessConditionSubfields quote-aware to avoid mutating string literals inside quoted segments - Add RFC3339 timestamp validation for time subfield queries - Add integer truncation and int32 overflow validation - Add test cases for new validations and quote-aware preprocessing
rafabene
force-pushed
the
HYPERFLEET-536
branch
from
d770ed3 to
8346325
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
last_updated_time,last_transition_time,observed_generation) with comparison operators (=,!=,<,<=,>,>=)status.conditions.Ready.last_updated_time) into encoded 3-part paths (status.conditions.Ready__last_updated_time)New API query examples
Related
Test plan
TestConditionsNodeConverterSubfields(12 cases),TestPreprocessConditionSubfields(6 cases),TestHasConditionWithSubfields(3 cases),TestExtractConditionQueriesWithSubfields(3 cases)TestSearchConditionSubfieldLastUpdatedTime,TestSearchConditionSubfieldCombinedWithStatus,TestSearchConditionSubfieldInvalidSubfieldSummary by CodeRabbit
New Features
Documentation
Migrations
Tests