Code

Opened 3 years ago

Closed 17 months ago

#16491 closed Uncategorized (fixed)

transaction.is_dirty() is very (and surprisingly) conservative

Reported by: mwhudson Owned by: nobody
Component: Uncategorized Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

As near as I can tell transaction.is_dirty() returns True if any queries have been executed at all (I think this is what lead to #15317 being reported). This is sort of fair enough (it's better to have a false positive for this sort of thing) but it certainly confused me for a good few minutes (commit_manually was complaining at me on exit). If it can't be made more accurate, at least the docstring should be improved. I'll try to come up with a patch tomorrow.

Attachments (0)

Change History (3)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

We're discussing https://docs.djangoproject.com/en/dev/topics/db/transactions/. I'm accepting this ticket as a documentation patch, to fix the following inconsistency.

The docs for commit_manually() say:

If your view changes data and doesn't commit() or rollback(), Django will raise a TransactionManagementError exception.

This is incompatible with the next paragraph, "Requirements for transaction handling":

This applies to all database operations, not just write operations. Even if your transaction only reads from the database, the transaction must be committed or rolled back before you complete a request.


Also, I think this is related to #6623: that ticket discusses the wording of the error message, and your confusion certainly stemmed from there too.


In terms of process, please assign the ticket to yourself if you're working on a patch.

Version 0, edited 3 years ago by aaugustin (next)

comment:2 Changed 17 months ago by EmilStenstrom

I find no occurrences of the string "Django will raise a TransactionManagementError exception" in the Django code base. Is the commit_manually docstring still misleading?

def commit_manually(using=None):
    """
    Decorator that activates manual transaction control. It just disables
    automatic transaction control and doesn't do any commit/rollback of its
    own -- it's up to the user to call the commit and rollback functions
    themselves.
    """
    ...

I suggest this bug is marked as resolved.

comment:3 Changed 17 months ago by andreas_pelme

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.