potiuk commented on code in PR #67244:
URL: https://github.com/apache/airflow/pull/67244#discussion_r3678107532
##########
airflow-core/tests/unit/jobs/test_triggerer_job.py:
##########
@@ -2589,3 +2589,87 @@ async def _drive():
trigger_id, _event, seq = events[0]
assert trigger_id == 1
assert seq is None
+
+
[email protected]
+async def test_trigger_event_payload_not_logged_at_info(cap_structlog):
+ """Ensure the full event payload is not logged at INFO level."""
+ runner = TriggerRunner()
+ runner.triggers = {
+ 1: {
+ "task": MagicMock(spec=asyncio.Task),
+ "is_watcher": False,
+ "name": "test_dag/run_id/test_task/0/1",
+ "events": 0,
+ }
+ }
+
+ mock_trigger = MagicMock(spec=BaseTrigger)
+ mock_trigger.task_instance = MagicMock()
+ mock_trigger.task_instance.map_index = -1
+
+ payload = {"api_response": {"token": "s3cr3t-api-k3y", "user_id": 42}}
+
+ async def fake_run():
+ yield TriggerEvent(payload)
+
+ mock_trigger.run = fake_run
+
+ mock_trigger.cleanup = AsyncMock()
+
+ task = asyncio.create_task(runner.run_trigger(1, mock_trigger))
+ await task
+
+ assert any(log["event"] == "Trigger fired event" for log in
cap_structlog), (
+ "Expected a 'Trigger fired event' log entry"
+ )
+ info_logs = [log for log in cap_structlog if log.get("log_level") ==
"info"]
+
+ for _key, value in payload.items():
+ assert not any(str(value) in str(log) for log in info_logs), (
+ "payload value must not appear in INFO-level logs"
+ )
+
+
[email protected]
+async def test_trigger_event_payload_available_at_debug(cap_structlog):
+ runner = TriggerRunner()
+ runner.triggers = {
+ 1: {
+ "task": MagicMock(spec=asyncio.Task),
+ "is_watcher": False,
+ "name": "test_dag/run_id/test_task/0/1",
+ "events": 0,
+ }
+ }
+
+ mock_trigger = MagicMock(spec=BaseTrigger)
+ mock_trigger.task_instance = MagicMock()
+ mock_trigger.task_instance.map_index = -1
+
+ payload = {"api_response": {"token": "s3cr3t-api-k3y", "user_id": 42}}
+
+ async def fake_run():
+ yield TriggerEvent(payload)
+
+ mock_trigger.run = fake_run
+
+ mock_log = AsyncMock()
+ mock_log.isEnabledFor = MagicMock(return_value=True)
Review Comment:
This sets `isEnabledFor`, but the production code calls `is_enabled_for`
(the structlog spelling — `isEnabledFor` exists only on
`structlog.stdlib.BoundLogger`, not on the `FilteringBoundLogger` protocol used
here).
Since `runner.log` is an `AsyncMock`,
`self.log.is_enabled_for(logging.DEBUG)` returns an auto-created child mock
rather than your `True` — truthy, so the branch is taken regardless. The test
therefore passes for the wrong reason and would keep passing if the
`is_enabled_for` guard were removed entirely. It also leaves an un-awaited
coroutine behind, since `AsyncMock` children are async by default.
Setting `is_enabled_for` as a plain `MagicMock(return_value=True)` fixes
both, and a companion case with it returning `False` would pin the guard down
properly.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/tests/unit/jobs/test_triggerer_job.py:
##########
@@ -2589,3 +2589,87 @@ async def _drive():
trigger_id, _event, seq = events[0]
assert trigger_id == 1
assert seq is None
+
+
[email protected]
+async def test_trigger_event_payload_not_logged_at_info(cap_structlog):
+ """Ensure the full event payload is not logged at INFO level."""
+ runner = TriggerRunner()
+ runner.triggers = {
+ 1: {
+ "task": MagicMock(spec=asyncio.Task),
+ "is_watcher": False,
+ "name": "test_dag/run_id/test_task/0/1",
+ "events": 0,
+ }
+ }
+
+ mock_trigger = MagicMock(spec=BaseTrigger)
+ mock_trigger.task_instance = MagicMock()
+ mock_trigger.task_instance.map_index = -1
+
+ payload = {"api_response": {"token": "s3cr3t-api-k3y", "user_id": 42}}
+
+ async def fake_run():
+ yield TriggerEvent(payload)
+
+ mock_trigger.run = fake_run
+
+ mock_trigger.cleanup = AsyncMock()
+
+ task = asyncio.create_task(runner.run_trigger(1, mock_trigger))
+ await task
+
+ assert any(log["event"] == "Trigger fired event" for log in
cap_structlog), (
+ "Expected a 'Trigger fired event' log entry"
+ )
+ info_logs = [log for log in cap_structlog if log.get("log_level") ==
"info"]
+
+ for _key, value in payload.items():
+ assert not any(str(value) in str(log) for log in info_logs), (
+ "payload value must not appear in INFO-level logs"
+ )
+
+
[email protected]
+async def test_trigger_event_payload_available_at_debug(cap_structlog):
+ runner = TriggerRunner()
+ runner.triggers = {
+ 1: {
+ "task": MagicMock(spec=asyncio.Task),
+ "is_watcher": False,
+ "name": "test_dag/run_id/test_task/0/1",
+ "events": 0,
+ }
+ }
+
+ mock_trigger = MagicMock(spec=BaseTrigger)
+ mock_trigger.task_instance = MagicMock()
+ mock_trigger.task_instance.map_index = -1
+
+ payload = {"api_response": {"token": "s3cr3t-api-k3y", "user_id": 42}}
+
+ async def fake_run():
+ yield TriggerEvent(payload)
+
+ mock_trigger.run = fake_run
+
+ mock_log = AsyncMock()
+ mock_log.isEnabledFor = MagicMock(return_value=True)
+ runner.log = mock_log
+
+ mock_trigger.cleanup = AsyncMock()
+
+ task = asyncio.create_task(runner.run_trigger(1, mock_trigger))
+ await task
+
+ # ainfo must not be called with result
+ for call in mock_log.ainfo.call_args_list:
+ assert "result" not in call.kwargs, "Payload must not appear in ainfo
call"
+
+ # adebug must be called with full payload as 'result'
+ print(mock_log.adebug.call_args.kwargs.get("result"))
Review Comment:
Leftover debug `print` — please drop.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/jobs/triggerer_job_runner.py:
##########
@@ -1554,9 +1554,15 @@ async def run_trigger(
event_stream = trigger.run()
async for event in event_stream:
- await self.log.ainfo(
- "Trigger fired event",
name=self.triggers[trigger_id]["name"], result=event
- )
+ # Avoid logging the full payload at INFO — it may contain
sensitive data and
+ # inflate log storage on every execution. DEBUG is used
instead so developers
+ # can still inspect the payload when needed without.
Review Comment:
The comment ends mid-sentence: "…can still inspect the payload when needed
without."
While you're here: `adebug` already no-ops when DEBUG isn't enabled, so the
`is_enabled_for` guard only saves building the kwargs. If that isn't a measured
win, calling `adebug` unconditionally would be simpler and would remove the
branch the test above is struggling to cover.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
