Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #35520 -- Used correct database for ModelAdmin transactions #18268

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RealOrangeOne
Copy link
Contributor

@RealOrangeOne RealOrangeOne commented Jun 11, 2024

Trac ticket number

ticket-35520

Branch 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).

This PR uses db_for_read for any read requests, based on the method. 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.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

with transaction.atomic(using=router.db_for_write(self.model)):
db = (
router.db_for_read(self.model)
if request.method in ("GET", "HEAD", "OPTIONS", "TRACE")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't currently a util for whether the request's method is read-only, so I've had to duplicate this set of methods a fair bit. Not sure this is the point we want to extract them into a util?

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this @RealOrangeOne 👍
I have minor comments

tests/admin_views/test_multidb.py Outdated Show resolved Hide resolved
tests/admin_views/test_multidb.py Outdated Show resolved Hide resolved
@RealOrangeOne RealOrangeOne force-pushed the 35520-modeladmin-transaction branch 2 times, most recently from c094064 to e43fa94 Compare June 19, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants