Opened 4 years ago

Closed 3 years 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.

Change History (3)

comment:1 Changed 4 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() are misleading:

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

The behavior is correctly described in 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.

Last edited 4 years ago by aaugustin (previous) (diff)

comment:2 Changed 3 years 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 3 years ago by andreas_pelme

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top