Opened 4 years ago

Closed 2 years ago

#15694 closed New feature (fixed)

Overriding Django admin views and nested transactions

Reported by: etianen Owned by: aaugustin
Component: contrib.admin Version: 1.2
Severity: Normal Keywords:
Cc: etianen, mmitar@… Triage Stage: Design decision needed
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This problem is related to #2227, but has a potentially easier solution.

Because the Django admin site directly decorates it's methods with @commit_on_success, and @commit_on_success is unaware on nested outer transactions, it's currently impossible to override an admin view and maintain data integrity. This is a problem for django-reversion, which augments many of the django admin methods by wrapping them in a revision. Ideally, I would be able to save the primary model and it's associated revision data in a single transaction. Currently, however, the superclass view commits the transaction and thus forces the associated revision data to be saved in it's own transaction.

Obviously, a satisfactory fix for #2227 would solve this. However, this ticket seems to be a bit dead, due to conflicting ways of solving it.

Fixing the problem of overriding admin views is easier, however. My proposed solution is to move the existing @commit_on_success and @csrf_protect decorators off the methods, and apply them in the admin site under AdminSite.admin_view instead. This would mean they are applied after all subclassing has taken place.

I've implemented a tentative patch here:

https://github.com/etianen/django/commit/6682fc31c810f763f08dcf14505c5762ab1d18a1

Change History (9)

comment:1 Changed 4 years ago by mk

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

I think that's a good idea.

However, if this was accepted it still needs tests, documentation and probably a note in the BackwardsIncompatibleChanges page.

comment:2 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:3 Changed 4 years ago by etianen

  • Cc etianen added
  • Type set to Uncategorized

I'm very happy to write the tests, documentation and the like. Once (if) this gets accepted, I'll get right on it.

comment:4 Changed 4 years ago by julien

  • Type changed from Uncategorized to New feature

comment:5 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:7 Changed 2 years ago by mitar

  • Cc mmitar@… added

comment:8 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:9 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 14cddf51c5f001bb426ce7f7a83fdc52c8d8aee9:

Merged branch 'database-level-autocommit'.

Fixed #2227: atomic supports nesting.
Fixed #6623: commit_manually is deprecated and atomic doesn't suffer from this defect.
Fixed #8320: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10744: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10813: since autocommit is enabled, it isn't necessary to rollback after errors any more.
Fixed #13742: savepoints are now implemented for SQLite.
Fixed #13870: transaction management in long running processes isn't a problem any more, and it's documented.
Fixed #14970: while it digresses on transaction management, this ticket essentially asks for autocommit on PostgreSQL.
Fixed #15694: atomic supports nesting.
Fixed #17887: autocommit makes it impossible for a connection to stay "idle of transaction".

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