Opened 5 months ago

Closed 4 months ago

#35520 closed Bug (fixed)

ModelAdmin uses incorrect database in change and delete views

Reported by: Jake Howard Owned by: Jake Howard
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (10)

comment:1 by Simon Charette, 5 months ago

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.

comment:2 by Simon Charette, 5 months ago

Triage Stage: UnreviewedAccepted

9459ec82aa12cad9b859c54c2f33f50bec057f2e was added to resolve #26170 through !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.

I suspect we don't want to create a transaction in the first place if we're using db_for_read? I worry about admin extensions that might create access records against the write database and might start failing here.

Last edited 5 months ago by Simon Charette (previous) (diff)

comment:3 by Jake Howard, 5 months ago

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.

comment:4 by Jake Howard, 5 months ago

Has patch: set

comment:5 by Clinton Christian, 5 months ago

-

Last edited 5 months ago by Clinton Christian (previous) (diff)

comment:6 by Sarah Boyce, 5 months ago

Patch needs improvement: set

comment:7 by Jake Howard, 5 months ago

Patch needs improvement: unset

comment:8 by Sarah Boyce, 5 months ago

Patch needs improvement: set

comment:9 by Sarah Boyce, 4 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

Resolution: fixed
Status: assignedclosed

In 53e674d:

Fixed #35520 -- Avoided opening transaction for read-only ModelAdmin requests.

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