fix(Segment Membership): Percent sign in regex condition crashes segment count refresh#7944
Conversation
…ent count refresh A regex condition containing a `%` inlined a literal `%` into the ClickHouse query text, which clickhouse-driver then misread as a `query % params` placeholder, raising `TypeError: not enough arguments for format string` before the query ran. Bump flagsmith-sql-flag-engine to 0.2.0 and thread its `Binder` through both segment-membership SQL builders so segment values — regex patterns included — bind as query parameters instead of being inlined. beep boop
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change bumps the flagsmith-sql-flag-engine dependency version, introduces Binder/PyformatParamStyle usage into TranslateContext construction within segment_membership services for building and executing parameterised ClickHouse queries, adds test coverage for regex patterns containing percent characters, and updates two log line-number references in the events catalogue documentation. Estimated code review effort: 3 (Moderate) | ~20 minutes Changes
Sequence Diagram(s)sequenceDiagram
participant Service as compute_segment_counts_for_project/get_segment_members_page
participant Binder as Binder(PyformatParamStyle)
participant Context as TranslateContext
participant Cursor as cursor
Service->>Binder: create Binder(PyformatParamStyle())
Service->>Context: TranslateContext(binder=Binder)
Context-->>Service: translated predicate
Service->>Service: merge binder.params with env_key/limit
Service->>Cursor: execute(sql, **params, **binder.params)
Related issues: None provided. Related PRs: None provided. Suggested labels: dependencies, backend, tests Suggested reviewers: None specified. Poem
Comment |
…e numbers The services.py edits shifted the source lines of two existing structlog events, so the generated events catalogue no longer matched. Regenerate it. beep boop
github-actions
Bot
added
docs
Remove the explanatory block comments and regenerate the events catalogue for the resulting line-number shift. beep boop
github-actions
Bot
added
docs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7944 +/- ##
=======================================
Coverage 98.63% 98.63%
=======================================
Files 1497 1497
Lines 59151 59169 +18
=======================================
+ Hits 58345 58363 +18
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7939.
In this PR, we bump
flagsmith-sql-flag-engineto 0.2.0, and use its newly introduced query param capability to avoid clickhouse-driver crash with unescaped%characters in segment condition values.How did you test this code?
Added ClickHouse-backed regression tests for both
get_segment_members_pageandcompute_segment_counts_for_projectusing a segment whose regex pattern ([b%]ar) carries a literal%and still matches seeded identities.