From fb3b5da775a980b73176f4196a0c068796410a41 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 2 Jul 2026 19:22:53 +0100 Subject: [PATCH 1/3] fix(Segment Membership): Percent sign in regex condition crashes segment count refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- api/pyproject.toml | 2 +- api/segment_membership/services.py | 26 +++++++++-- .../test_unit_segment_membership_services.py | 44 ++++++++++++++++++- api/uv.lock | 8 ++-- 4 files changed, 71 insertions(+), 9 deletions(-) diff --git a/api/pyproject.toml b/api/pyproject.toml index d722b9a5060d..3eeab6874148 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -44,7 +44,7 @@ dependencies = [ "drf-writable-nested>=0.6.2,<0.7.0", "django-filter>=2.4.0,<2.5.0", "flagsmith-flag-engine>=10.1.0,<11.0.0", - "flagsmith-sql-flag-engine>=0.1.3,<0.2.0", + "flagsmith-sql-flag-engine>=0.2.0,<0.3.0", "django-clickhouse-backend>=1.4,<2.0", "clickhouse-driver", "boto3>=1.35.95,<1.36.0", diff --git a/api/segment_membership/services.py b/api/segment_membership/services.py index 9df56cffd7aa..b1e353ce23f3 100644 --- a/api/segment_membership/services.py +++ b/api/segment_membership/services.py @@ -7,7 +7,12 @@ from django.db.backends.utils import CursorWrapper from django.db.models import Q from flag_engine.context.types import EvaluationContext -from flagsmith_sql_flag_engine import TranslateContext, translate_segment +from flagsmith_sql_flag_engine import ( + Binder, + PyformatParamStyle, + TranslateContext, + translate_segment, +) from flagsmith_sql_flag_engine.dialects import ClickHouseDialect from task_processor.models import Task @@ -122,6 +127,12 @@ def compute_segment_counts_for_project( return [] dialect = ClickHouseDialect() + # One binder across the whole UNION ALL: it mints unique placeholder + # names (p0, p1, …) per bound literal, so segment values — including + # regex patterns whose `%` would otherwise break clickhouse-driver's + # `query % params` substitution — travel as query parameters, never + # inlined into the SQL text. + binder = Binder(PyformatParamStyle()) select_clauses: list[str] = [] for seg in segments: translate_ctx = TranslateContext( @@ -129,6 +140,7 @@ def compute_segment_counts_for_project( environment={"key": "_count", "name": project.name} ), dialect=dialect, + binder=binder, ) predicate = translate_segment( map_segment_to_segment_context(map_segment_to_engine(seg)), @@ -155,7 +167,7 @@ def compute_segment_counts_for_project( return [] sql = "\nUNION ALL\n".join(select_clauses) - cursor.execute(sql, {"env_keys": tuple(env_id_by_key)}) + cursor.execute(sql, {"env_keys": tuple(env_id_by_key), **binder.params}) rows: list[tuple[Any, ...]] = cursor.fetchall() membership_counts: list[SegmentMembershipCount] = [] for row in rows: @@ -186,11 +198,15 @@ def get_segment_members_page( Provide identifier as `cursor` to get a page after that identifier. Provide `q` to filter to identifiers containing it (case-insensitive). """ + # See `compute_segment_counts_for_project`: the binder keeps segment + # values (regex patterns especially) out of the SQL text as bound params. + binder = Binder(PyformatParamStyle()) translate_ctx = TranslateContext( evaluation_context=EvaluationContext( environment={"key": "_members", "name": segment.project.name} ), dialect=ClickHouseDialect(), + binder=binder, ) predicate = translate_segment( map_segment_to_segment_context(map_segment_to_engine(segment)), @@ -208,7 +224,11 @@ def get_segment_members_page( "i.environment_id = %(env_key)s", "i.is_deleted = false", ] - params: dict[str, Any] = {"env_key": environment.api_key, "limit": limit} + params: dict[str, Any] = { + "env_key": environment.api_key, + "limit": limit, + **binder.params, + } if cursor: conditions.append("i.identifier > %(cursor)s") params["cursor"] = cursor diff --git a/api/tests/unit/segment_membership/test_unit_segment_membership_services.py b/api/tests/unit/segment_membership/test_unit_segment_membership_services.py index 767b17b42a07..e0c445e0afc9 100644 --- a/api/tests/unit/segment_membership/test_unit_segment_membership_services.py +++ b/api/tests/unit/segment_membership/test_unit_segment_membership_services.py @@ -5,7 +5,7 @@ from common.test_tools import RunTasksFixture from django.db import connections from django.utils import timezone -from flag_engine.segments.constants import EQUAL +from flag_engine.segments.constants import EQUAL, REGEX from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from task_processor.models import Task @@ -191,6 +191,17 @@ def matching_segment(segment: Segment) -> Segment: return segment +@pytest.fixture +def percent_regex_segment(segment: Segment) -> Segment: + # A REGEX condition whose pattern carries a literal `%`. The `[b%]` + # class still matches the seeded "bar" trait, while the `%` is what + # used to crash clickhouse-driver's `query % params` substitution + # when the pattern was inlined into the SQL text. + rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) + Condition.objects.create(rule=rule, property="foo", operator=REGEX, value="[b%]ar") + return segment + + @pytest.mark.clickhouse def test_get_segment_members_page__deleted_identity__excluded( segment_membership_identities: None, @@ -281,6 +292,37 @@ def test_compute_segment_counts_for_project__deleted_identity__excluded_from_cou assert counts[0].count == 2 +@pytest.mark.clickhouse +def test_get_segment_members_page__regex_with_percent__returns_matches( + segment_membership_identities: None, + percent_regex_segment: Segment, + environment: Environment, +) -> None: + # Given / When + members = get_segment_members_page( + percent_regex_segment, environment, cursor=None, limit=100 + ) + + # Then + assert [member["identifier"] for member in members] == ["alice", "bob"] + + +@pytest.mark.clickhouse +def test_compute_segment_counts_for_project__regex_with_percent__counts_matches( + segment_membership_identities: None, + percent_regex_segment: Segment, + project: Project, +) -> None: + # Given / When + with connections["clickhouse"].cursor() as cursor: + counts = compute_segment_counts_for_project(project, cursor) + + # Then + assert len(counts) == 1 + assert counts[0].segment_id == percent_regex_segment.id + assert counts[0].count == 2 + + @pytest.mark.clickhouse def test_get_segment_members_page__matching_identities__returns_members_ordered_by_identifier( segment_membership_identities: None, diff --git a/api/uv.lock b/api/uv.lock index a57bf8efeabb..aa586f711b7a 100644 --- a/api/uv.lock +++ b/api/uv.lock @@ -1556,7 +1556,7 @@ requires-dist = [ { name = "flagsmith-common", extras = ["test-tools"], marker = "extra == 'dev'" }, { name = "flagsmith-flag-engine", specifier = ">=10.1.0,<11.0.0" }, { name = "flagsmith-private", marker = "extra == 'private'", specifier = ">=0.10.1,<1", index = "https://flagsmith-production-084060095745.d.codeartifact.eu-west-2.amazonaws.com/pypi/flagsmith-pypi-production/simple/" }, - { name = "flagsmith-sql-flag-engine", specifier = ">=0.1.3,<0.2.0" }, + { name = "flagsmith-sql-flag-engine", specifier = ">=0.2.0,<0.3.0" }, { name = "google-api-python-client", specifier = ">=1.12.5,<1.13.0" }, { name = "google-re2", specifier = ">=1.0,<2.0.0" }, { name = "gunicorn", specifier = ">=23.0.0,<23.1.0" }, @@ -1706,15 +1706,15 @@ wheels = [ [[package]] name = "flagsmith-sql-flag-engine" -version = "0.1.3" +version = "0.2.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "flagsmith-flag-engine" }, { name = "jsonpath-rfc9535" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/f4/bd/6647e7d6598b57b7edd4685fd28b27fb477a7bdf93525a1351cdd4fcdd86/flagsmith_sql_flag_engine-0.1.3.tar.gz", hash = "sha256:33b7203d25ded9f00c213910294030b6e102d34230c7fa51b43ccc9ec913f5c6", size = 17992, upload-time = "2026-07-01T19:08:46.985Z" } +sdist = { url = "https://files.pythonhosted.org/packages/72/34/961758e0946ffd430cf90480dce67fd7b548269749b174961d6380f2fb57/flagsmith_sql_flag_engine-0.2.0.tar.gz", hash = "sha256:38af827ba846795b04d7504e47498875bbfb6951c26177427b0e6378eaebed52", size = 19396, upload-time = "2026-07-02T17:40:02.645Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/13/1f/19219272e2b2ee3e44aca4c881c290b0399086c60c6733091d25f6faaa75/flagsmith_sql_flag_engine-0.1.3-py3-none-any.whl", hash = "sha256:6da33dc77636924a523df8c265a7e196fec886b5d0a87e5b37a3cb20e71dd77f", size = 21736, upload-time = "2026-07-01T19:08:45.767Z" }, + { url = "https://files.pythonhosted.org/packages/22/59/6ea43aba79109cbd4feaad3623a7554d4364c1b825d1989ba6410764aa0f/flagsmith_sql_flag_engine-0.2.0-py3-none-any.whl", hash = "sha256:a30658d740a3c7f8111c3475005f8ec75a9f9ad7ce4ab58e4fd300364dca139c", size = 23742, upload-time = "2026-07-02T17:40:01.263Z" }, ] [[package]] From 68b701a1a99c416d94c305136bfe96311cbb6377 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 2 Jul 2026 19:32:59 +0100 Subject: [PATCH 2/3] docs(Segment Membership): Regenerate events catalogue for shifted line 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 --- .../observability/_events-catalogue.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 73eb191ab05f..75a148993429 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -369,7 +369,7 @@ Attributes: ### `segment_membership.compute.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:138` + - `api/segment_membership/services.py:150` Attributes: - `project.id` @@ -379,7 +379,7 @@ Attributes: ### `segment_membership.members.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:200` + - `api/segment_membership/services.py:216` Attributes: - `reason` From 826f3b33bf16261a37591f6c3507b2e91f9c3af4 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 2 Jul 2026 19:37:06 +0100 Subject: [PATCH 3/3] refactor(Segment Membership): Drop multi-line comments from the fix Remove the explanatory block comments and regenerate the events catalogue for the resulting line-number shift. beep boop --- api/segment_membership/services.py | 7 ------- .../test_unit_segment_membership_services.py | 4 ---- .../observability/_events-catalogue.md | 4 ++-- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/api/segment_membership/services.py b/api/segment_membership/services.py index b1e353ce23f3..532c3e1b630c 100644 --- a/api/segment_membership/services.py +++ b/api/segment_membership/services.py @@ -127,11 +127,6 @@ def compute_segment_counts_for_project( return [] dialect = ClickHouseDialect() - # One binder across the whole UNION ALL: it mints unique placeholder - # names (p0, p1, …) per bound literal, so segment values — including - # regex patterns whose `%` would otherwise break clickhouse-driver's - # `query % params` substitution — travel as query parameters, never - # inlined into the SQL text. binder = Binder(PyformatParamStyle()) select_clauses: list[str] = [] for seg in segments: @@ -198,8 +193,6 @@ def get_segment_members_page( Provide identifier as `cursor` to get a page after that identifier. Provide `q` to filter to identifiers containing it (case-insensitive). """ - # See `compute_segment_counts_for_project`: the binder keeps segment - # values (regex patterns especially) out of the SQL text as bound params. binder = Binder(PyformatParamStyle()) translate_ctx = TranslateContext( evaluation_context=EvaluationContext( diff --git a/api/tests/unit/segment_membership/test_unit_segment_membership_services.py b/api/tests/unit/segment_membership/test_unit_segment_membership_services.py index e0c445e0afc9..b119643060b6 100644 --- a/api/tests/unit/segment_membership/test_unit_segment_membership_services.py +++ b/api/tests/unit/segment_membership/test_unit_segment_membership_services.py @@ -193,10 +193,6 @@ def matching_segment(segment: Segment) -> Segment: @pytest.fixture def percent_regex_segment(segment: Segment) -> Segment: - # A REGEX condition whose pattern carries a literal `%`. The `[b%]` - # class still matches the seeded "bar" trait, while the `%` is what - # used to crash clickhouse-driver's `query % params` substitution - # when the pattern was inlined into the SQL text. rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) Condition.objects.create(rule=rule, property="foo", operator=REGEX, value="[b%]ar") return segment diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 75a148993429..fe0d6052fcd1 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -369,7 +369,7 @@ Attributes: ### `segment_membership.compute.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:150` + - `api/segment_membership/services.py:145` Attributes: - `project.id` @@ -379,7 +379,7 @@ Attributes: ### `segment_membership.members.segment.skipped` Logged at `error` from: - - `api/segment_membership/services.py:216` + - `api/segment_membership/services.py:209` Attributes: - `reason`