HYPERFLEET-536 - feat: add condition subfield queries for selective Sentinel polling#71
Open
rafabene wants to merge 7 commits intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-536 - feat: add condition subfield queries for selective Sentinel polling#71rafabene wants to merge 7 commits intoopenshift-hyperfleet:mainfrom
rafabene wants to merge 7 commits intoopenshift-hyperfleet:mainfrom
Conversation
…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.
|
[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 |
WalkthroughThis PR adds support for querying Kubernetes condition subfields (e.g., Sequence DiagramsequenceDiagram
participant Client
participant Service as pkg/services/generic.go
participant Preprocessor as pkg/db/sql_helpers.go<br/>(Preprocess)
participant TSLParser as TSL Parser
participant Converter as pkg/db/sql_helpers.go<br/>(Converter)
participant Database as Database
Client->>Service: Search request with query
Service->>Service: Validate query length (≤4096 chars)
Service->>Preprocessor: PreprocessConditionSubfields(query)
Preprocessor-->>Service: Encoded query<br/>(Type__Subfield format)
Service->>TSLParser: ParseTSL(preprocessedSearch)
TSLParser-->>Service: Parsed AST
Service->>Converter: Process condition nodes
Converter->>Converter: Detect subfield pattern
alt Subfield Query
Converter->>Converter: Extract Type & Subfield
Converter->>Converter: Validate operator support
Converter->>Converter: Apply type casting<br/>(time→timestamptz, int→INTEGER)
Converter-->>Service: SQL expression
else Status Query
Converter->>Converter: Validate status values<br/>(True/False/Unknown)
Converter-->>Service: JSON path SQL
end
Service->>Database: Execute SQL with indexes
Database-->>Service: Results
Service-->>Client: Filtered resources
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
- 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
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
Comment on lines
+131
to
+151
| ### Supported Operators | ||
|
|
||
| Condition subfields support comparison operators: `=`, `!=`, `<`, `<=`, `>`, `>=`. | ||
|
|
||
| > **Note**: `status.conditions.<Type>` (without subfield) only supports the `=` operator. | ||
|
|
||
| ```bash | ||
| # Find clusters where Ready condition hasn't been updated in the last hour | ||
| curl -G "http://localhost:8000/api/hyperfleet/v1/clusters" \ | ||
| --data-urlencode "search=status.conditions.Ready.last_updated_time < '2026-03-06T14:00:00Z'" | ||
|
|
||
| # Find stale-ready resources (Sentinel selective polling use case) | ||
| curl -G "http://localhost:8000/api/hyperfleet/v1/clusters" \ | ||
| --data-urlencode "search=status.conditions.Ready='True' AND status.conditions.Ready.last_updated_time < '2026-03-06T14:00:00Z'" | ||
|
|
||
| # Find clusters with observed_generation below a threshold (uses unquoted integer) | ||
| curl -G "http://localhost:8000/api/hyperfleet/v1/clusters" \ | ||
| --data-urlencode "search=status.conditions.Ready.observed_generation < 5" | ||
| ``` | ||
|
|
||
| Time subfields require RFC3339 format (e.g., `2026-01-01T00:00:00Z`). Integer subfields use unquoted numeric values. |
There was a problem hiding this comment.
Call out that NOT is unsupported for condition subfield queries.
This section lists the comparison operators but doesn't mention that condition queries under NOT are rejected. Adding that caveat here will save users a confusing 400 the first time they try NOT status.conditions.Ready.last_updated_time < ....
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/search.md` around lines 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.
Comment on lines
+32
to
+52
| // Expression index on clusters for Ready condition last_updated_time | ||
| if err := tx.Exec(` | ||
| CREATE INDEX IF NOT EXISTS idx_clusters_ready_last_updated_time | ||
| ON clusters USING BTREE (( | ||
| immutable_timestamptz( | ||
| jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') ->> 'last_updated_time' | ||
| ) | ||
| )); | ||
| `).Error; err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Expression index on node_pools for Ready condition last_updated_time | ||
| if err := tx.Exec(` | ||
| CREATE INDEX IF NOT EXISTS idx_node_pools_ready_last_updated_time | ||
| ON node_pools USING BTREE (( | ||
| immutable_timestamptz( | ||
| jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') ->> 'last_updated_time' | ||
| ) | ||
| )); | ||
| `).Error; err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can PostgreSQL use an expression index when constants embedded in the indexed expression are bind parameters in the query predicate? Example index expression: immutable_timestamptz(jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') ->> 'last_updated_time') Example predicate: immutable_timestamptz(jsonb_path_query_first(status_conditions, $1::jsonpath) ->> $2) < $3::timestamptz
💡 Result:
No—not in the general case.
PostgreSQL can use an expression index only when the query contains the same indexed expression (i.e., the planner must be able to match the expression tree it sees in the predicate to the expression tree stored in the index definition). If your index hard-codes constants (the JSONPath string and the ->> key), but the query uses bind parameters in those positions, the expressions are not identical, so that index won’t match and won’t be used. [1]
Concretely, this index:
(immutable_timestamptz(jsonb_path_query_first(status_conditions,
'$[*] ? (@.type == "Ready")') ->> 'last_updated_time'))does not match this predicate (different expression due to $1, $2):
immutable_timestamptz(jsonb_path_query_first(status_conditions, $1::jsonpath) ->> $2) < $3Even if, at execution time, $1 and $2 happen to equal those same literals, the planner usually can’t assume that at plan time—especially for generic plans used with prepared statements—so it can’t select the index based on that equality. This is the same underlying limitation that causes indexes (notably partial/expression cases) to be skipped when the decisive value is only known via a parameter at execution time. [2][3]
Practical options
- Use the same literals in the SQL text (so the expression matches exactly) → index can be used. [1]
- If you must pass values dynamically, use dynamic SQL that embeds the literals (carefully quoted) so PostgreSQL plans with constants (a “custom plan”), rather than relying on a generic parameterized plan. Settings like
plan_cache_modeinfluence this tradeoff. [3] - If you need this to be truly parameterized across many JSONPaths/keys, you typically need a different indexing strategy (e.g., store/extract the relevant value into a generated/maintained column and index that, or create separate expression indexes per fixed path/key combination).
Sources: PostgreSQL docs on indexes on expressions (matching behavior) [1]; discussion of parameters preventing index use because values aren’t known at planning time [2]; prepared statements/generic vs custom plans and plan_cache_mode behavior [3].
[1] (postgresql.org)
[2] (stackoverflow.com)
[3] (vladmihalcea.com)
Citations:
- 1: https://www.postgresql.org/docs/current/static/indexes-expressional.html?utm_source=openai
- 2: https://stackoverflow.com/questions/67997619/postgresql-is-not-using-index-when-passed-by-param?utm_source=openai
- 3: https://vladmihalcea.com/postgresql-plan-cache-mode/?utm_source=openai
PostgreSQL expression indexes cannot match parameterized predicates—these indexes will not be used by queries from the query builder.
PostgreSQL only uses an expression index when the query's WHERE clause contains the exact same expression tree. Since this index hardcodes constants ('Ready' and 'last_updated_time'), but the generated query predicates use bind parameters in those positions, the planner will not recognize a match and will ignore the index. This defeats the polling performance win intended by this migration.
Fix options:
- Embed the JSONPath and field name as literals in the query SQL (match the index expression exactly)
- Use dynamic SQL that constructs the parameterized expression at query time with
plan_cache_mode = 'custom'to force per-query planning with visible constants - Create separate expression indexes for each fixed condition type/field combination
- Store the extracted value in a generated column and index that instead
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/migrations/202603100001_add_conditions_subfield_indexes.go` around
lines 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.
Comment on lines
+436
to
+444
| // Reject NOT applied to condition queries — the condition is extracted before | ||
| // NOT is applied, so the negation would be silently lost, producing wrong results. | ||
| if n.Func == tsl.NotOp { | ||
| if child, ok := n.Left.(tsl.Node); ok && hasCondition(child) { | ||
| return n, errors.BadRequest( | ||
| "NOT operator is not supported with condition queries; use the inverse condition instead (e.g., Ready='False')", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject condition queries anywhere under NOT, not just direct children.
This only catches NOT status.conditions.... NOT (status.conditions.Ready='True' AND name='x') still recurses into the subtree, extracts the condition without its negation, and changes the query semantics instead of returning 400.
🛠️ Tighten the guard to scan the whole `NOT` subtree
+func containsConditionNode(n tsl.Node) bool {
+ if hasCondition(n) {
+ return true
+ }
+ if child, ok := n.Left.(tsl.Node); ok && containsConditionNode(child) {
+ return true
+ }
+ switch v := n.Right.(type) {
+ case tsl.Node:
+ return containsConditionNode(v)
+ case []tsl.Node:
+ for _, child := range v {
+ if containsConditionNode(child) {
+ return true
+ }
+ }
+ }
+ return false
+}
+
func extractConditionsWalk(n tsl.Node, conditions *[]sq.Sqlizer) (tsl.Node, *errors.ServiceError) {
// Reject NOT applied to condition queries — the condition is extracted before
// NOT is applied, so the negation would be silently lost, producing wrong results.
if n.Func == tsl.NotOp {
- if child, ok := n.Left.(tsl.Node); ok && hasCondition(child) {
+ if child, ok := n.Left.(tsl.Node); ok && containsConditionNode(child) {
return n, errors.BadRequest(
"NOT operator is not supported with condition queries; use the inverse condition instead (e.g., Ready='False')",
)
}
}🤖 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 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.
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
3 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
Release Notes
New Features
Documentation