[Django] #36487: Database on commit error logging fails for partials

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: krishna- | Owner: krishna-holvi
holvi |
Type: Bug | Status: assigned
Component: Database | Version: 5.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/36487>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------

Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* has_patch: 0 => 1
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted

Comment:

There is effectively a bug here as not all callable will have a
`__qualname__`. Maybe we could simply `name = getattr(func,
"__qualname__", func)` instead?
--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:1>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Krishnaprasad MG):

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 (copied from
the tests)

`def robust_callback():`
` raise ForcedError("robust callback")`

`robust_callback_partial = partial(robust_callback)`

`>>> getattr(robust_callback_partial, "__qualname__",
robust_callback_partial)`
`functools.partial(<function robust_callback at 0x101287600>)`
`>>> getattr(robust_callback, "__qualname__", robust_callback)`
`'robust_callback'`

I could update the PR
--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:2>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Krishnaprasad MG):

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
--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:3>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:4>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Krishnaprasad MG):

I have updated the PR. The logging improvements (switching to %s
placeholders) are now addressed. Tests are passing on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:5>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_tests: 0 => 1

Comment:

Very close -- just asking to evaluate the feasibility of a third test.
--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:6>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:7>

[Django]

#36487: Database on commit error logging fails for partials
-------------------------------------+-------------------------------------
Reporter: Krishnaprasad MG | Owner:
| Krishnaprasad MG

Type: Bug | Status: closed

Component: Database layer | Version: 5.2
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls <jacobtylerwalls@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"7a2f35b1b755b20a1e4d290a88bf059e6897f798" 7a2f35b]:
{{{#!CommitTicketReference repository=""
revision="7a2f35b1b755b20a1e4d290a88bf059e6897f798"
Fixed #36487 -- Fixed logger error message with partial callbacks.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36487#comment:8>