Add workflow label list filters - #1224
Conversation
📝 WalkthroughWalkthroughChangesWorkflow label filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant list_workflow
participant get_workflows
participant PostgreSQL
participant ListEntry
Client->>list_workflow: label and no_label query parameters
list_workflow->>get_workflows: validated filter inputs
get_workflows->>PostgreSQL: JSONB label predicates
PostgreSQL-->>get_workflows: matching workflow rows
get_workflows->>ListEntry: construct entries with labels
ListEntry-->>Client: workflow list response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5ddbeac to
998c815
Compare
029d92d to
988b43c
Compare
988b43c to
100bd6b
Compare
100bd6b to
0b07961
Compare
0b07961 to
8c1ed9c
Compare
8c1ed9c to
905db9b
Compare
905db9b to
0cfa33a
Compare
bb90913 to
7053641
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
==========================================
+ Coverage 66.42% 66.46% +0.04%
==========================================
Files 201 201
Lines 25992 26023 +31
Branches 3924 3930 +6
==========================================
+ Hits 17264 17295 +31
Misses 8021 8021
Partials 707 707
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
7053641 to
4e3be9b
Compare
Compile repeatable label=key=value selectors into fixed SQL predicates: exact values use GIN-indexable JSONB containment, glob patterns use LIKE with escaped literals, alternation branches OR together, and a bare * collapses to key existence. no_label=key is NULL-inclusive so pre-feature rows count as missing. List responses now carry the labels map. Selector parsing happens before any SQL, so malformed filters reject as usage errors and user input never reaches the query text unbound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bind label keys with %s like every other get_workflows filter instead of hand-rendering SQL literals (psycopg2 interpolates client-side, so the planner still sees literal keys and the GIN index stays eligible), collapse the glob-to-LIKE translation into chained replaces, drop the off-topic warning-column test, and trim the planner test to the shipped GIN index asserting on plan shape rather than index names, which the isolation fixture regenerates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Name the parsed selector and key lists for what they hold, document the LIKE escape contract and the no_label parameter semantics, point the fixture docstring at its mirror, and regenerate API artifacts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4e3be9b to
0ead5e3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/service/core/workflow/workflow_service.py (1)
610-623: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuery() defaults mirror existing convention; B008 is a known FastAPI false positive.
The Ruff B008 warnings on these new params match the same
fastapi.Query(default=...)pattern already used for every other parameter in this signature (e.g.users,statuses,pools). This is the documented FastAPI idiom, and Ruff'sflake8-bugbearextend-immutable-callssetting is meant to suppress it forfastapi.Query. Not something to change here since it's consistent with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/service/core/workflow/workflow_service.py` around lines 610 - 623, Preserve the fastapi.Query defaults and parameter definitions for label_filters and missing_label_filters; no code change is needed. Treat the Ruff B008 warnings as covered by the existing fastapi.Query convention and configured immutable-call exception used elsewhere in the same signature.Source: Linters/SAST tools
src/service/core/workflow/helpers.py (1)
169-172: 🚀 Performance & Scalability | 🔵 TrivialMissing-label negation can't use the GIN index the way positive filters do.
(workflows.labels IS NULL OR NOT (workflows.labels ? %s))is a negation, so it likely falls back to a sequential scan even with the GIN index onworkflows.labelspresent — the shippedtest_shipped_gin_index_is_planner_eligibletest only verifies planner-eligibility for the positive?and@>predicates, not this negated path. Worth documenting as a known tradeoff, or considering a partial index ifno_labelfiltering becomes a hot path on large tables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/service/core/workflow/helpers.py` around lines 169 - 172, Document in the missing_label_keys filtering path that the negated workflows.labels predicate is not expected to use the GIN index, unlike the positive label predicates, and may require a sequential scan. Keep the current filtering behavior unchanged; only consider a partial-index optimization if no_label filtering becomes a measured large-table hot path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/service/core/workflow/helpers.py`:
- Around line 169-172: Document in the missing_label_keys filtering path that
the negated workflows.labels predicate is not expected to use the GIN index,
unlike the positive label predicates, and may require a sequential scan. Keep
the current filtering behavior unchanged; only consider a partial-index
optimization if no_label filtering becomes a measured large-table hot path.
In `@src/service/core/workflow/workflow_service.py`:
- Around line 610-623: Preserve the fastapi.Query defaults and parameter
definitions for label_filters and missing_label_filters; no code change is
needed. Treat the Ruff B008 warnings as covered by the existing fastapi.Query
convention and configured immutable-call exception used elsewhere in the same
signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7d02106c-6a8f-4d99-8586-0a72eeaeb3bd
📒 Files selected for processing (7)
src/service/core/workflow/helpers.pysrc/service/core/workflow/objects.pysrc/service/core/workflow/tests/BUILDsrc/service/core/workflow/tests/test_helpers.pysrc/service/core/workflow/tests/test_workflow_label_filters_db.pysrc/service/core/workflow/tests/test_workflow_service_units.pysrc/service/core/workflow/workflow_service.py
Summary
Issue - None
[OSMO-6501] Track B6 of the workflow-labels PR split.
label=key=valueandno_label=keyquery filters (AND semantics across repeats).labelvalues support*wildcards and non-nested(a|b)alternation; selectors compile server-side into fixed SQL predicate shapes with bound parameters: exact branches as GIN-indexable@>containment, wildcard branches asLIKEwith_/%/escape-char escaping, OR-joined; a bare*collapses to a?key-existence check. Both keys and values are bound (psycopg2 interpolates client-side, so the planner still sees literals and the GIN index stays eligible). No user-supplied regex — no ReDoS surface.no_label=keyis NULL-inclusive (labels IS NULL OR NOT labels ? key) so pre-feature and unlabeled rows are counted; the warn-phase soak query depends on this.labelsmap.Depends on #1223 (admission). Extracted from prototype #1194.
Testing
bazel test //src/service/core/workflow/tests:test_helpers(predicate shapes: containment/LIKE/existence, escaping, AND semantics, injection-syntax rejection before query, bound keys)bazel test //src/service/core/workflow/tests:test_workflow_label_filters_db(testcontainers Postgres: filters execute against real JSONB; the shippedworkflow_labels_gin_idxis planner-eligible, asserted on plan shape since the isolation fixture regenerates index names; NULL-inclusive missing filter)bazel test //src/service/core/workflow/tests:test_workflow_service_unitsChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit