Usiel commented on PR #35048:
URL: https://github.com/apache/airflow/pull/35048#issuecomment-1774471133
> This feels a bit too fragile to me; it is much too easy for new code to
introduce a logging call that hits the errornous code path again. Is it
possible to fix this in the log rendering logic instead of relying on the call
site to hold this correctly?
I get your concern. I would still leave in the fix the `log.warning(...)`
call, as that makes sure Airflow doesn't die when the next unhandled case
occurs. I like @utkarsharma2's idea on just printing out some debug info here
(instead of the `str(...)` call).
Regarding actually fixing this particular case, where `item` passes
`isinstance(item, str)`, but then cannot be handled by re2's substitute code
(e.g. `sqlalchemy.sql.elements.quoted_name`): I wonder if we can simply call
`str(...)` on the `item` before passing it to the `replacer`, since that should
result in a sensible output;`item` is either `str` or one of the compatibility
types like `six.text_type` or SQLAlchemy's `quoted_name`.
If we fix it here, then we can remove the change on `db_cleanup` as you
suggested @Taragolis.
```diff
@@ -259,11 +259,11 @@ class SecretsMasker(logging.Filter):
elif isinstance(item, str):
if self.replacer:
...
- return self.replacer.sub("***", item)
+ return self.replacer.sub("***", str(item))
return item
elif isinstance(item, (tuple, set)):
...
```
--
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]
