potiuk commented on a change in pull request #20039:
URL: https://github.com/apache/airflow/pull/20039#discussion_r762573088
##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -213,11 +213,13 @@ def _redact(self, item: "RedactableItem", name:
Optional[str], depth: int) -> "R
else:
return item
# I think this should never happen, but it does not hurt to leave it
just in case
+ # Well. It happened (see
https://github.com/apache/airflow/issues/19816#issuecomment-983311373)
+ # but it caused infinite recursion, so we need to cast it to str first.
except Exception as e:
log.warning(
- "Unable to redact %r, please report this via
<https://github.com/apache/airflow/issues>. "
+ "Unable to redact %s, please report this via
<https://github.com/apache/airflow/issues>. "
"Error was: %s: %s",
- item,
+ str(item),
type(e).__name__,
str(e),
)
Review comment:
Not really. Neither of those proposal is good. But I think mine was not
perfect either.
1) If we use '%s' and pass the object, we might have the same recursion
problem. The problem was that calling `repr()` of the object threw an
exception which caused the recursion (inside the log.warning() message). But
`str()` migh have exactly the same problem. So the only good solution to avoid
the recursion is to run a conversion to string before the `warning` message is
entered - i.e. before the counter is reset. Just in case so if it throws an
exception, it won't reset the counter.
2) using '%r' when you pass string makes little sense. because it will call
`repr()` method on a string. Which is unnecessary (though result is the same).
By convention (and I think it is used everywhere in our code including two
lines below when we do str(e) - we always use '%s' when we pass string, because
it will simply check that the class is string and will print it without running
extra conversion. I've never seen someone uses '%r` to print a string :).
However your comment made me think and slightly better solution would be to
actually call _repr() manually and then use '%s' to log it. This would be a bit
better equivalent of the previous version. Let me fix it quickly.
--
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]
