Copilot commented on code in PR #64610:
URL: https://github.com/apache/airflow/pull/64610#discussion_r3070054583
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -1380,6 +1380,8 @@ def _handle_request(self, msg: ToSupervisor, log:
FilteringBoundLogger, req_id:
before=msg.before,
ascending=msg.ascending,
limit=msg.limit,
+ partition_key=msg.partition_key,
+ partition_key_pattern=msg.partition_key_pattern,
)
Review Comment:
The supervisor now forwards `partition_key_pattern` to
`client.asset_events.get()`, but
`task-sdk/tests/task_sdk/execution_time/test_supervisor.py` only asserts the
`partition_key_pattern` kwarg is `None`. Add a request/response test case that
sets `partition_key_pattern` (and verifies it’s passed through) to prevent
regressions in this new behavior.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py:
##########
@@ -73,6 +74,31 @@ def _get_asset_events_through_sql_clauses(
)
+def _validate_partition_key_params(partition_key: str | None,
partition_key_pattern: str | None) -> None:
+ """Validate partition_key and partition_key_pattern are not both set, and
that the pattern is valid regex."""
+ if partition_key is not None and partition_key_pattern is not None:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail={
+ "reason": "Mutually exclusive parameters",
+ "message": "partition_key and partition_key_pattern cannot
both be provided. "
+ "Use partition_key for exact match or partition_key_pattern
for regex filtering.",
+ },
+ )
+ if partition_key_pattern is None:
+ return
Review Comment:
`partition_key_pattern` can be provided as an empty string via
`?partition_key_pattern=`; FastAPI will pass this as `""` (not `None`).
`_validate_partition_key_params()` currently accepts it and the query will end
up doing `regexp_match("")`, which can unintentionally match most/all non-null
partition keys and exclude NULLs. Consider normalizing empty strings to `None`
or returning a 400 when `partition_key_pattern == ""`.
```suggestion
return
if partition_key_pattern == "":
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail={
"reason": "Invalid regex",
"message": "The partition_key_pattern must be a non-empty
regular expression when provided.",
},
)
```
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -675,16 +675,24 @@ def get(
before: datetime | None = None,
ascending: bool = True,
limit: int | None = None,
+ partition_key: str | None = None,
+ partition_key_pattern: str | None = None,
) -> AssetEventsResponse:
"""Get Asset event from the API server."""
+ if partition_key is not None and partition_key_pattern is not None:
+ raise ValueError("partition_key and partition_key_pattern are
mutually exclusive")
common_params: dict[str, Any] = {}
Review Comment:
`AssetEventOperations.get()` now raises a `ValueError` when both
`partition_key` and `partition_key_pattern` are provided, but the test suite
doesn’t currently cover this new validation path. Add a unit test asserting the
exception is raised (and ideally that neither request is attempted).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
