Opened 5 months ago
Closed 5 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 , 5 months ago
comment:2 by , 5 months ago
Triage Stage: | Unreviewed → Accepted |
---|
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.
comment:3 by , 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:5 by , 5 months ago
Hi, I was working on this, and was preparing a PR, but it seems the Author has changed.
comment:6 by , 5 months ago
Patch needs improvement: | set |
---|
comment:7 by , 5 months ago
Patch needs improvement: | unset |
---|
comment:8 by , 5 months ago
Patch needs improvement: | set |
---|
comment:9 by , 5 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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.