[Django] #35520: ModelAdmin uses incorrect database in change and delete views

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-----------------------------------------+-----------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+-----------------------------
In `ModelAdmin.delete_view` and `ModelAdmin.changelist_view`, the
transaction is hardcoded to use `db_for_write`, even if the request is
read-only (eg `GET`). This potentially results in the wrong database being
selected (or errors if `db_for_write` is used to prevent writing to
models).

I propose modifying these calls to use `db_for_read` if the request is
read-only (`request.method in ("GET", "HEAD", "OPTIONS", "TRACE")`). Most
users won't notice the difference, as the same database will be selected.
But those who are using a custom router will get the database they expect.
--
Ticket URL: <https://code.djangoproject.com/ticket/35520>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views

-------------------------------+---------------------------------------

Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------+---------------------------------------
Comment (by Simon Charette):

I wonder if this is intentional to avoid any form of misrepresentation of
data on reads due to factors like replication lag. Given the action is
meant to be performed against the write database I could see it being the
case.
--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:1>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

9459ec82aa12cad9b859c54c2f33f50bec057f2e was added to resolve #26170
through [https://github.com/django/django/pull/7143 !7143] where the point
of data inconsistencies was not brought up. I think it's fair that using
`db_for_read` for non-mutable HTTP requests was missed rather than
intentional.
--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:2>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------

Comment (by Jake Howard):

I suspect we don't want to create a transaction in the first place if
we're using `db_for_read`?

Perhaps not, but keeping the transaction is less likely to break any
existing behaviour, not to mention making more sure the view is internally
consistent. Changing the database is going to be less impactful than
removing a transaction.

I worry about admin extensions that might create access records
against the write database and might start failing here.

Django's built-in `LogEntry` only logs writes ("Addition", "Change",
"Deletion"), so simply viewing a model shouldn't trigger any writes. I
could add a test for this, but not sure whether that's overkill.
--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:3>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
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 Jake Howard):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18268 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:4>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
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 Clinton Christian):

Hi, I was working on this, and was preparing a PR, but it seems the Author
has changed.
--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:5>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
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 Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:6>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
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 Jake Howard):

* needs_better_patch: 1 => 0

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

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------+---------------------------------------
Reporter: Jake Howard | Owner: Jake Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
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 Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:8>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views

-------------------------------------+-------------------------------------

Reporter: Jake Howard | Owner: Jake
| Howard
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
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 Sarah Boyce):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:9>

[Django]

#35520: ModelAdmin uses incorrect database in change and delete views
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Jake
| Howard

Type: Bug | Status: closed
Component: contrib.admin | Version: dev
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 Sarah Boyce <42296566+sarahboyce@…>):

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

Comment:

In [changeset:"53e674d5744faad61e52d8459c9198b2aa6f63dd" 53e674d]:
{{{#!CommitTicketReference repository=""
revision="53e674d5744faad61e52d8459c9198b2aa6f63dd"
Fixed #35520 -- Avoided opening transaction for read-only ModelAdmin
requests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35520#comment:10>