[Django] #34466: Django 4.2 overwrites user-specified psycopg cursor_factory
Django
-------------------------------------+-------------------------------------
Reporter: Anders | Owner: nobody
Kaseorg |
Type: Bug | Status: new
Component: Database | Version: 4.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 |
-------------------------------------+-------------------------------------
Zulip
[https://github.com/zulip/zulip/blob/7c023042cfbbd8817d5ba3adfca365734500a0e2/zproject/computed_settings.py#L285
configures] a custom `cursor_factory` that
[https://github.com/zulip/zulip/blob/7c023042cfbbd8817d5ba3adfca365734500a0e2/zerver/lib/db.py#L31
wraps] `psycopg2.extensions.cursor` to collect timing statistics for
logging. But this no longer works in Django 4.2 due to
09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca (#33308) and
0e2649fdf40cedc5be7e2c0e5f7711f315e36b84 (#34255) because
`DatabaseWrapper.get_new_connection` now unconditionally overwrites
`connection.cursor_factory` (even for psycopg2).
The configured `cursor_factory` is being passed to `get_new_connection` as
`conn_params["cursor_factory"]`. `get_new_connection` should leave that
alone if it’s set. (And if it’s not, it might also be cleaner for to pass
the default `cursor_factory` via a keyword argument to
`psycopg[2].connect` too, rather than mutating it later.)
--
Ticket URL: <https://code.djangoproject.com/ticket/34466>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Kaseorg
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Anders Kaseorg
* status: new => assigned
* has_patch: 0 => 1
Comment:
Submitted a patch at https://github.com/django/django/pull/16736.
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Sounds like a legit request, we didn't limit this functionality on purpose
but rather by accident.
Assuming we fix this we should also make it more pythonic (and I guess I
am at fault for it being like it is now) and not check for `is True` but
only truthiness. Either way, a test ensuring that we do not regress here
would be good.
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Type: Bug | Status: assigned
Component: Database layer | Version: 4.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
-------------------------------------+-------------------------------------
Comment (by Anders Kaseorg):
Removed `is True` and added a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* severity: Normal => Release blocker
* needs_docs: 0 => 1
Comment:
Replying to [comment:2 Florian Apolloner]:
> Assuming we fix this we should also make it more pythonic (and I guess I
am at fault for it being like it is now) and not check for `is True` but
only truthiness.
We use `is True` intentionally, to avoid errors when passing wrong truthy
values, e.g.`"server_side_binding": os.environ.get("USE_SSB")` (where
`os.environ.get("USE_SSB")` returns `"False"`).
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Component: Database layer | Version: 4.2
(models, ORM) |
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
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"73cbb372baa45d1fdafd571e2f430a980831f722" 73cbb372]:
{{{
#!CommitTicketReference repository=""
revision="73cbb372baa45d1fdafd571e2f430a980831f722"
Fixed #34466 -- Reallowed setting cursor_factory in DATABASES["options"]
on PostgreSQL.
Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Anders Kaseorg | Owner: Anders
| Kaseorg
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | 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
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0bc2bbf041df40a6ecb6262b2e5f3bb659dd8da8" 0bc2bbf]:
{{{
#!CommitTicketReference repository=""
revision="0bc2bbf041df40a6ecb6262b2e5f3bb659dd8da8"
[4.2.x] Fixed #34466 -- Reallowed setting cursor_factory in
DATABASES["options"] on PostgreSQL.
Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.
Backport of 73cbb372baa45d1fdafd571e2f430a980831f722 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34466#comment:8>