Opened 3 weeks ago

Last modified 3 weeks ago

#36487 assigned Bug

Database on commit error logging fails for partials

Reported by: Krishnaprasad MG Owned by: Krishnaprasad MG
Component: Database layer (models, ORM) Version: 5.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The on commit handler here: https://github.com/django/django/blob/main/django/db/backends/base/base.py#L763 accepts both callback functions and partials but the error logging has a bug which expects property __qualname__ on the callback method but this doesn't work with partials.

A fix is implemented here: https://github.com/django/django/pull/19609

Change History (3)

comment:1 by Simon Charette, 3 weeks ago

Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

There is effectively a bug here as not all callable will have a __qualname__. Maybe we could simply name = getattr(func, "__qualname__", func) instead?

comment:2 by Krishnaprasad MG, 3 weeks ago

This also can be done, the current fix effectively prints the wrapped function name in case of partial, but yes may be this is fine. Updated the PR.

Last edited 3 weeks ago by Krishnaprasad MG (previous) (diff)

comment:3 by Krishnaprasad MG, 3 weeks ago

One more possible improvement would be to replace the f-string with %s placeholder in log messages if that makes sense

logger.exception(f"Error calling {name} in on_commit() (%s).", e) to logger.exception("Error calling %s in on_commit() (%s).", name, e)

because this helps log collecting systems like Sentry to effectively aggregate log messages without creating separate log messages. Also mentioned in google style guide: https://google.github.io/styleguide/pyguide.html#3101-logging

Note: See TracTickets for help on using tickets.
Back to Top