Add partition_key and partition_key_pattern filters for asset events#64610
Add partition_key and partition_key_pattern filters for asset events#64610hussein-awala wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a regex-based partition key filter for asset events end-to-end (public REST API, execution API, Task SDK/supervisor), plus a supporting DB index/migration and documentation updates.
Changes:
- Introduces
partition_key_pattern(REST) /partition_key(execution API + SDK) regex filtering and wires it through API ↔ supervisor ↔ Task SDK accessor. - Adds
(asset_id, partition_key)composite index onasset_eventwith Alembic migration + updates migration head mapping/docs. - Regenerates OpenAPI artifacts (YAML + UI TS clients) and adds tests/docs for the new filter.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| task-sdk/tests/task_sdk/execution_time/test_supervisor.py | Supervisor request forwarding test updates + new partition_key cases |
| task-sdk/tests/task_sdk/api/test_client.py | SDK client query param forwarding tests for partition_key |
| task-sdk/src/airflow/sdk/execution_time/supervisor.py | Forwards partition_key from supervisor messages to API client |
| task-sdk/src/airflow/sdk/execution_time/context.py | Adds .partition_key(pattern) chaining to InletEventsAccessor |
| task-sdk/src/airflow/sdk/execution_time/comms.py | Adds partition_key to supervisor request message models |
| task-sdk/src/airflow/sdk/api/client.py | Adds partition_key query param forwarding in asset_events.get() |
| airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_asset_events.py | Adds execution API regex filter tests (postgres-only) |
| airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_assets.py | Adds REST API partition_key_pattern filtering tests (postgres-only) |
| airflow-core/src/airflow/utils/db.py | Updates migration head mapping for 3.3.0 |
| airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts | Regenerates UI request types incl. partitionKeyPattern |
| airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts | Regenerates request service to pass partition_key_pattern |
| airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts | Regenerates request schemas (incl. ValidationError schema changes) |
| airflow-core/src/airflow/ui/openapi-gen/queries/suspense.ts | Regenerates suspense query hook with partitionKeyPattern |
| airflow-core/src/airflow/ui/openapi-gen/queries/queries.ts | Regenerates query hook with partitionKeyPattern |
| airflow-core/src/airflow/ui/openapi-gen/queries/prefetch.ts | Regenerates prefetch helper with partitionKeyPattern |
| airflow-core/src/airflow/ui/openapi-gen/queries/ensureQueryData.ts | Regenerates ensureQueryData helper with partitionKeyPattern |
| airflow-core/src/airflow/ui/openapi-gen/queries/common.ts | Regenerates query key fn to include partitionKeyPattern |
| airflow-core/src/airflow/models/asset.py | Adds ORM index idx_asset_event_asset_id_partition_key |
| airflow-core/src/airflow/migrations/versions/0111_3_3_0_add_idx_asset_event_partition_key.py | New migration to create/drop the composite index |
| airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py | Adds execution API regex filter param + query clause |
| airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py | Adds REST partition_key_pattern filter dependency to /assets/events |
| airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml | Regenerates REST OpenAPI (adds param + changes ValidationError schema) |
| airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml | Regenerates private UI OpenAPI (ValidationError schema changes) |
| airflow-core/src/airflow/api_fastapi/common/parameters.py | Adds reusable _RegexParam + regex_param_factory + query alias type |
| airflow-core/src/airflow/api_fastapi/auth/managers/simple/ui/openapi-gen/requests/types.gen.ts | Regenerates simple auth manager UI request types |
| airflow-core/src/airflow/api_fastapi/auth/managers/simple/ui/openapi-gen/requests/schemas.gen.ts | Regenerates simple auth manager UI request schemas |
| airflow-core/src/airflow/api_fastapi/auth/managers/simple/openapi/v2-simple-auth-manager-generated.yaml | Regenerates simple auth manager OpenAPI (ValidationError schema changes) |
| airflow-core/docs/migrations-ref.rst | Updates auto-generated migration reference table (new head) |
| airflow-core/docs/authoring-and-scheduling/assets.rst | Documents Task SDK accessor + REST query usage for partition key regex |
Comments suppressed due to low confidence (1)
airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py:341
- If
partition_key_patterncontains an invalid regex, the underlying DB will raise during query execution (e.g. PostgreSQL error), which currently propagates as a 500. Consider catching the relevant SQLAlchemy/DBAPI exception around the query execution and returning a 400 with a clear message that the regex pattern is invalid for the current backend.
assets_event_select, total_entries = paginated_select(
statement=base_statement,
filters=[
asset_id,
source_dag_id,
source_task_id,
source_run_id,
source_map_index,
partition_key_pattern,
name_pattern,
timestamp_range,
],
order_by=order_by,
offset=offset,
limit=limit,
session=session,
)
6304aa9 to
0626b0b
Compare
7ddc996 to
6cf39d4
Compare
371aac1 to
5dbfe96
Compare
f509f70 to
5afedc4
Compare
165ec20 to
7ec7341
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
Is regexp required here? We have a lot of pattern and prefix_pattern filters doing both substring match and prefix match. (supporting logical OR, some special character too "~" etc....)
That introduce an inconsistency for that specific pattern param as well as introducing complexity handling regexp.
Would it be possible to implement that search similarly to what we already have on other APIs and discuss on a separate issue/PR specifically the introduction of Regexp matching.
|
Sorry for the late reply @pierrejeambrun, and thanks for the review. You raise a fair point about consistency, and I agree the regex introduction deserves a dedicated discussion. Let me explain the concrete gap so we can decide together. The existing 1. The Both filters treat
2. Suffix / "ends with" matching Find all partitions for a given date regardless of the region prefix:
3. Character-class / format constraints Match only well-formed date partitions for a region:
4. Structured mid-string alternation Match either region, but only for a specific month:
|
Add two new query parameters to the asset events API endpoints: - partition_key: exact-match filter leveraging the new composite B-tree index on (asset_id, partition_key) - partition_key_pattern: regex-based filter using database-native regexp_match for flexible pattern matching Includes Core API, Execution API, Task SDK (InletEventsAccessor), client, documentation, migration for the composite index, and tests. Made-with: Cursor
da1abd9 to
02b4b83
Compare
…-regex-filter # Conflicts: # airflow-core/docs/authoring-and-scheduling/assets.rst # airflow-core/docs/migrations-ref.rst # airflow-core/src/airflow/ui/openapi-gen/queries/ensureQueryData.ts # airflow-core/src/airflow/ui/openapi-gen/queries/prefetch.ts # airflow-core/src/airflow/ui/openapi-gen/queries/queries.ts # airflow-core/src/airflow/ui/openapi-gen/queries/suspense.ts # airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts # airflow-core/src/airflow/utils/db.py
The pattern is evaluated by the database's regex engine, so a pathological pattern could consume DB CPU (ReDoS). Mitigations: - Cap the pattern length (MAX_REGEX_PATTERN_LENGTH) in both the Core and Execution API validators, returning 400 for over-long patterns. - Bound the regex query with a transaction-local statement_timeout on PostgreSQL (apply_regex_query_timeout); no-op elsewhere. MySQL bounds regex evaluation via its built-in regexp_time_limit; SQLite has no server-side regex and MariaDB is unsupported. - Document the safeguards in assets.rst.
Add two [api] configs (Airflow 3.4.0): - enable_regexp_query_filters (bool, default False): the partition_key_pattern filter passes a user-supplied regex to the database engine, which is a ReDoS vector, so it is disabled by default and must be explicitly enabled. Exact-match partition_key filtering is unaffected. - regexp_query_timeout (int seconds, default 30): the primary runtime mitigation, enforced as a PostgreSQL transaction-local statement_timeout; 0 disables it. When the flag is off, partition_key_pattern requests return HTTP 400 on both the Core and Execution APIs. Config descriptions document the security rationale. Adds tests for the disabled path and the timeout helper.
Drop MAX_REGEX_PATTERN_LENGTH; the opt-in flag and the PostgreSQL statement_timeout are the meaningful ReDoS safeguards, and a length cap does not stop short catastrophic patterns while risking false rejections of legitimate ones.
|
Hey @pierrejeambrun, I did the security deep-dive on the regex filtering. Summary and what I've done: The risk is real, and it's DB-side
Per-engine picture (only the backends we support)
Important subtlety about re2re2 fixes ReDoS when we do the matching (like CVE-2023-36543's Python-side fix). Here the database matches, so a Python-side re2 can't protect the DB engine. The SQL-world equivalent of "an engine that stops backtracking" is a statement timeout. What I implemented
(I initially also added a pattern-length cap but dropped it, it doesn't stop short catastrophic patterns like Net: the feature is safe-by-default (off), and when enabled it's bounded by a configurable timeout on the one backend that needs it. Happy to adjust the default timeout or add anything else you'd like. |
- Add the enable_regexp_query_filters conf_vars fixture to TestGetAssetEventByAssetAliasPartitionKey (was missed), fixing the 4 by-alias regex tests that now hit the default-off flag; add a disabled->400 test for the alias endpoint too. - Regenerate ts-sdk/src/generated/supervisor.ts for the partition_key / partition_key_pattern comms fields (check-ts-sdk-supervisor-schema).
| partition_key: Annotated[ | ||
| str | None, | ||
| Query(description="Exact partition key filter."), | ||
| ] = None, | ||
| partition_key_pattern: Annotated[ | ||
| str | None, | ||
| Query( | ||
| description="Partition key regex filter. Uses database-native regex " | ||
| "(PostgreSQL ~ operator, MySQL REGEXP, SQLite re.match). " | ||
| "Note: on SQLite, matching is anchored at the start of the string." | ||
| ), | ||
| ] = None, |
There was a problem hiding this comment.
Why not use QueryAssetEventPartitionKeyRegex and the other one?
This create duplicated validation code in the view
There was a problem hiding this comment.
Good catch, they were different when I started the implementation but it is not the case now.
The execution routes now depend on the shared QueryAssetEventPartitionKeyFilter / QueryAssetEventPartitionKeyRegex, applied via a filters argument on the SQL helper. The inline Query(...) params and the _validate_partition_key_params helper are gone, so validation (gate + regex compile) lives in one place.
…-regex-filter # Conflicts: # airflow-core/docs/authoring-and-scheduling/assets.rst # airflow-core/docs/migrations-ref.rst # airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py # airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py # airflow-core/src/airflow/ui/openapi-gen/queries/common.ts # airflow-core/src/airflow/ui/openapi-gen/queries/ensureQueryData.ts # airflow-core/src/airflow/ui/openapi-gen/queries/prefetch.ts # airflow-core/src/airflow/ui/openapi-gen/queries/queries.ts # airflow-core/src/airflow/ui/openapi-gen/queries/suspense.ts # airflow-core/src/airflow/utils/db.py # airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_assets.py # airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_asset_events.py # task-sdk/src/airflow/sdk/api/client.py # task-sdk/src/airflow/sdk/execution_time/comms.py # task-sdk/src/airflow/sdk/execution_time/context.py # task-sdk/src/airflow/sdk/execution_time/schema/schema.json # task-sdk/src/airflow/sdk/execution_time/supervisor.py # task-sdk/tests/task_sdk/api/test_client.py # task-sdk/tests/task_sdk/execution_time/test_supervisor.py
- Rename the public/SDK parameter partition_key_pattern -> partition_key_regexp_pattern to distinguish regexp filtering from the substring "*_pattern" filters on other APIs. - Move the enable_regexp_query_filters gate into _RegexParam construction and the regex compile check into depends_regex, so a regexp filter can never be instantiated without the setting being enabled. - Drop the mutually-exclusive 400 between partition_key and partition_key_regexp_pattern (both now combine with AND) on the Core and Execution APIs and in the Task SDK client/accessor. - Reuse the shared QueryAssetEventPartitionKeyFilter/Regex params in the Execution API asset-event routes instead of duplicating the query params and validation. - Keep backend implementation details out of the public parameter description; simplify the config docs; revert an unrelated limit check. - Regenerate OpenAPI specs, UI TS clients, and the supervisor schema snapshots.
Address further review feedback: - Replace the enable_regexp_query_filters + regexp_query_timeout pair with a single [api] regexp_query_timeout: 0 (default) disables regexp filtering entirely, any positive value enables it and bounds the query runtime. This removes the "enabled but unbounded" combination. - Make apply_regex_query_timeout a context manager that sets a transaction-local statement_timeout on enter and resets it on exit, so the bound is scoped to the regexp query and does not leak to other statements in the request's transaction. Both asset-event routes apply it automatically around query execution instead of relying on a manual call in the view.
|
Thanks @pierrejeambrun! I've applied all of these in the last two commits and I updated the PR after #64611 is merged. I will try to test all the new changes before merging |
The hand-applied rename missed the type-block declarations and JSDoc in the generated query hooks (they were not on lines matching the AssetEvents hook name). Regenerate with `pnpm codegen` so the UI client matches the OpenAPI spec, fixing the "Compile / format / lint UI" static check.
…-regex-filter # Conflicts: # ts-sdk/src/generated/supervisor.ts
The check-no-new-provide-session-positional hook flagged the partition-key regexp tests. The nine test methods never used the injected session, so drop their @provide_session decorator and unused session parameter; make session keyword-only in the _create_partition_key_test_data helper that does use it.
pierrejeambrun
left a comment
There was a problem hiding this comment.
LGTM, we might want a second pair of eyes as this is a significant change.
| finally: | ||
| # Reset to the default for the remainder of the transaction so the timeout only bounds the | ||
| # regexp query and not unrelated statements in the same request. | ||
| session.execute(text("SELECT set_config('statement_timeout', '0', true)")) |
There was a problem hiding this comment.
Nit: This restores 0, but if there was a global timeout on the db side, this will override it. We should restore to the db side default.
There was a problem hiding this comment.
Maybe capture the value before and restore it after?
| QueryAssetEventPartitionKeyFilter = Annotated[ | ||
| FilterParam[str | None], | ||
| Depends(filter_param_factory(AssetEvent.partition_key, str | None, filter_name="partition_key")), | ||
| ] | ||
| QueryAssetEventPartitionKeyRegex = Annotated[ | ||
| _RegexParam, Depends(regex_param_factory(AssetEvent.partition_key, "partition_key_regexp_pattern")) | ||
| ] |
There was a problem hiding this comment.
The thing I wanted to prevent by moving the timeout inside the regex_param_factory or the filter class directly, is because if someone add a filter in here using the factory, and forget to call the timeout in the view -> CVE.
So I was trying to be defensive for a future bug.
There was a problem hiding this comment.
Maybe in paginated_select, add a check. If one of the filters is an instance of _RegexpParam and there is no timeout set for the request -> big server failure, refusing to process.
That doesn't cover 'all cases' someone could still use a bare _RegexpParam without using the paginated_select helper but that will cover 'most cases' of using filters.
And maybe something in the _RegexParam docstring, explaining that the caller query using this should time bound calling apply_regex_query_timeout
ashb
left a comment
There was a problem hiding this comment.
Mostly looks okay - not having a db query timeout on MySQL feels like something we need to fix
| for event in us_events: | ||
| print(event.extra, event.partition_key) | ||
|
|
||
| For an exact partition key match, use ``.partition_key(value)`` instead. Regexp filtering is opt-in: it is enabled only by setting ``[api] regexp_query_timeout`` to a positive number of seconds, which also bounds the query runtime; see the config for the security trade-off. |
There was a problem hiding this comment.
I think we can do a direct link to the config via
:conf:`api__regexp_query__timeout`
Something like this exists
| on/off switch with the timeout intentionally makes it impossible to enable regexp | ||
| filtering without a runtime bound in place. | ||
| version_added: 3.4.0 | ||
| type: integer |
| configured timeout is ``0`` (which also means regexp filtering is disabled). | ||
| """ | ||
| timeout_seconds = conf.getint("api", "regexp_query_timeout") | ||
| if timeout_seconds <= 0 or get_dialect_name(session) != "postgresql": |
There was a problem hiding this comment.
So this timeout config doesn't apply to mysql? That feels risky?
| def test_partition_key_regexp_pattern_filtering( | ||
| self, test_client, partition_key_regexp_pattern, expected_count | ||
| ): | ||
| self._create_partition_key_test_data() |
There was a problem hiding this comment.
Nit: it seems wasteful to create the same rows for each parameter set? Class scoped fixture perhaps? That could also delete/clean on teardown that way
| def test_partition_key_exact_match_via_regex(self, test_client): | ||
| self._create_partition_key_test_data() | ||
| response = test_client.get("/assets/events", params={"partition_key_regexp_pattern": "^2024-01-01$"}) | ||
| assert response.status_code == 200 | ||
| assert response.json()["total_entries"] == 1 | ||
|
|
||
| def test_partition_key_exact_match(self, test_client): | ||
| self._create_partition_key_test_data() | ||
| response = test_client.get("/assets/events", params={"partition_key": "2024-01-01"}) | ||
| assert response.status_code == 200 | ||
| assert response.json()["total_entries"] == 1 | ||
|
|
||
| def test_partition_key_exact_match_composite(self, test_client): | ||
| self._create_partition_key_test_data() | ||
| response = test_client.get("/assets/events", params={"partition_key": "us|2024-01-01"}) | ||
| assert response.status_code == 200 | ||
| assert response.json()["total_entries"] == 1 | ||
|
|
||
| def test_partition_key_exact_match_no_match(self, test_client): | ||
| self._create_partition_key_test_data() | ||
| response = test_client.get("/assets/events", params={"partition_key": "nonexistent"}) | ||
| assert response.status_code == 200 | ||
| assert response.json()["total_entries"] == 0 |
There was a problem hiding this comment.
These could be 4 cases in a single parameteized test i think?
| "/assets/events", | ||
| params={"partition_key": "2024-01-01", "partition_key_regexp_pattern": "^2025-"}, | ||
| ) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
I would have expected providing both to return a 400?
Summary
Add two query parameters for filtering asset events by partition key across the full stack:
partition_key— exact-match filter using==equality, leverages the B-treecomposite index
(asset_id, partition_key)for fast lookups.partition_key_pattern— regex filter using SQLAlchemy'sregexp_match, which mapsto database-native regex on all backends (PostgreSQL
~, MySQLREGEXP, SQLitere.match).The two parameters are mutually exclusive — providing both returns a 400 error.
Also includes:
(asset_id, partition_key)onasset_eventtable with Alembic migration.assets.rstwith usage examples for both parameters.InletEventsAccessorexposes.partition_key(value)for exact match and.partition_key_pattern(pattern)for regex within tasks.Details
Exact match (
partition_key)Uses a simple
WHERE partition_key = ?equality check. This is the preferred optionwhen you know the full partition key — it leverages the B-tree index directly with
zero regex overhead.
Regex filter (
partition_key_pattern)Uses
regexp_matchwhich maps to database-native regex on all supported backends:PostgreSQL (
~operator), MySQL (REGEXP), and SQLite (via Pythonre.match).Note that SQLite's
re.matchis anchored at the start of the string, so patternsshould use
^-anchored regex for consistent cross-backend behavior.Invalid regex patterns are validated with
re.compile()before hitting the database,returning a 400 error with a descriptive message.
A
_RegexParamclass andregex_param_factorywere added to the common FastAPIparameters module for reuse (the factory accepts a customizable
description).Performance note
Currently,
partition_key_patternperforms a full table scan (filtered by any otherWHERE clauses) since standard B-tree indexes cannot accelerate regex matching. For
exact matches, the composite B-tree index
(asset_id, partition_key)is used directly.For high-volume deployments where regex filtering on
partition_keybecomes a bottleneck,a trigram (n-gram) index can significantly improve performance on PostgreSQL. This
requires the
pg_trgmextension and can be created manually by a database administratorwithout any application-level changes:
PostgreSQL's query planner will automatically use the trigram index for
~(regex)queries emitted by
regexp_match— no Airflow configuration changes needed.When it helps:
.*sales$,.*\|2024-.*,us.*westWhen it doesn't help (and what to use instead):
^us\|2024-— for these, usepartition_key(exact match)which leverages the B-tree index directly with zero overhead
update than B-tree, adding write-time cost on every
INSERTintoasset_eventShipping this as an Alembic migration would require
CREATE EXTENSIONprivileges(typically superuser) and a cross-backend strategy, since MySQL and SQLite have no
equivalent. This can be discussed separately if there is demand.
Files changed
Core API layer:
parameters.py— new_RegexParam,regex_param_factory,QueryAssetEventPartitionKeyFilter(exact match),
QueryAssetEventPartitionKeyRegex(regex)assets.py— accept bothpartition_keyandpartition_key_patternfilters inGET /api/v2/assets/eventswith mutual-exclusion validationExecution API layer:
asset_events.py— accept both params in/by-assetand/by-asset-aliasendpoints,with mutual-exclusion and regex validation
Task SDK:
client.py— sendpartition_keyandpartition_key_patternas separate query params,with client-side
ValueErrorif both are providedcomms.py— add both fields to request modelssupervisor.py— forward both fields to API clientcontext.py—InletEventsAccessorexposes.partition_key()and.partition_key_pattern()(mutually exclusive — setting one clears the other)
Database:
asset.py— composite indexidx_asset_event_asset_id_partition_key0112_3_3_0_add_idx_asset_event_partition_key.pyAuto-generated:
openapi-gen/queries/*,openapi-gen/requests/*)Tests:
Docs:
assets.rst— usage examples for both parameters via REST API andInletEventsAccessorTest plan
ValueErrorwhen both params are providedInletEventsAccessorclears the other field when setting one