moomindani commented on PR #70385: URL: https://github.com/apache/airflow/pull/70385#issuecomment-5552028392
The production case is useful — an independent report with a real `AnalysisException` payload is better evidence for this fix than my synthetic one. The second hole is not there, though, and I checked rather than reasoned. I replayed both payload shapes into a real Postgres 16.13 `jsonb` column the way SQLAlchemy writes them (`json.dumps` text cast to `jsonb`), with the same dirty string — a NUL plus a lone surrogate — in each: | payload | result | |---|---| | `errors[].error`, unsanitized | rejected: `UntranslatableCharacter: unsupported Unicode escape sequence` | | `run_state = RunState.to_json()` carrying that `state_message` | **stored** | | `run_state` as a nested dict instead of a `to_json()` string | rejected | The double encoding is what saves it. `json.dumps` turns a NUL into the six ASCII characters `\u0000`; serializing the event payload for the column escapes that backslash in turn, so Postgres sees literal text rather than an escape it must interpret. Rejection needs a *live* `\u0000` in the wire JSON, and only the raw `errors[]` path produces one — the third row is what that would take. I suspect the repro is what led there: in Python `"boom \^@ more"` is a literal backslash-caret-at rather than a NUL, since `\^` is not an escape sequence — `\^@` is how psql renders a NUL, not how Python writes one. A real `\x00` comes out as `\u0000`. Every state in these triggers goes through `to_json()` (`triggers/databricks.py:155`, `:181`, `:208`, `:320`, `:331`, `:424`, and `operators/databricks.py:280`), so `ClusterState` and `SQLStatementState` are covered by the same stringification. One thing does remain, and it is not this: `from_json()` revives the real NUL, so if a revived `state_message` is ever put into a payload unstringified, that is the hole. Today it only reaches exception text and logs. Sanitizing inside `to_json()` would also change the string that `databricks_repair_reason_new_settings` matching reads (`operators/databricks.py:210`, `:226`) — an argument for keeping this at the persistence boundary, where the PR already has it. So this looks right to me as it stands, and my approval holds. --- Drafted-by: Claude Code (Opus 5); reviewed by @moomindani 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]
