Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#17671 closed Bug (fixed)

CursorWrapper in Python 2.7 cannot be used as a contextmanager

Reported by: Michael Manfre Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: regression
Cc: denisenkom Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The following code pattern for executing raw SQL works fine in Python 2.6.x, but fails in Python 2.7.x.

from django.db import connection
with connection.cursor() as c:
    c.execute('select 1')

Python 2.7.x results in

Traceback (most recent call last):
  File "C:\projects\web-site\test\withcursor\myapp\", line 21, in test_with_cursor
    with connection.cursor() as c:
AttributeError: __exit__

I was able to reproduce this with a new project using sqlite3 and a new app without any models.

Attachments (1)

django-ticket17671.diff (3.7 KB) - added by Michael Manfre 4 years ago.
context manager shortcut without pass-thru

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by Claude Paroz

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

AFAIK Django only wraps the cursor obtained from the underlying database library. It might be that your 2.7 installation does not use the same library as the 2.6 installation, hence the error.

I was personally able to reproduce the same error with my Python 2.6 installation, which enforces my opinion about an underlying library issue:

In [1]: from django.db import connection

In [2]: with connection.cursor() as c:
   ...:     c.execute('select 1')
AttributeError                            Traceback (most recent call last)

/home/claude/checkouts/django-git/mysite/<ipython console> in <module>()

/home/claude/virtualenvs/djangogit/lib/python2.6/site-packages/django/db/backends/util.pyc in __getattr__(self, attr)
     26             return self.__dict__[attr]
     27         else:
---> 28             return getattr(self.cursor, attr)
     30     def __iter__(self):

AttributeError: 'SQLiteCursorWrapper' object has no attribute '__exit__'

comment:2 Changed 5 years ago by denisenkom@…

Has patch: set
Resolution: invalid
Status: closedreopened

To fix this add following methods to django.db.backends.CursorWrapper

def __enter__(self):
    return self

def __exit__(self, type, value, traceback):
    return self.cursor.__exit__(type, value, traceback)
Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:3 Changed 5 years ago by denisenkom

Cc: denisenkom added

comment:4 Changed 5 years ago by denisenkom

Here is more detailed description of underlying problem with python 2.7

comment:5 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: reopenednew

comment:6 Changed 4 years ago by Aymeric Augustin

Triage Stage: UnreviewedDesign decision needed

This code should never have worked; see (and its duplicate

I'm having a hard time determining if we should change something in Django:

  • our documentation states that the cursors implement the standard Python DB-API described in PEP 249
  • but PEP 249 doesn't say whether cursors are supposed to work as context managers
  • since Python 2.6 sqlite's cursors act as context managers for transaction control,
  • but Django implements its own transaction control

__enter__ and __exit__ methods that just forward to __enter__ and __exit__ wouldn't hurt, but I can't find a real reason to implement them.

comment:7 Changed 4 years ago by Michael Manfre

The underlying Python "regression" and whether or not the Django code should have ever worked is not really relevant. The important question is, was it the intention that cursors in Django should be usable as a contextmanager? If so, then this is clearly a regression. If not, then is there a reason to explicitly block the functionality provided by the underlying cursors?

The expectation for django-mssql has always been that cursors, which must be used more frequently than with the core backends, should be usable as a contextmanager for the sake of code brevity and readability. I have applied a (hopefully temporary monkey) patch to django-mssql v1.1 to maintain this expected behavior.

comment:8 Changed 4 years ago by Anssi Kääriäinen

Hmm, IMHO there is reason to block the forwarding. The reason is that what the forwarding does is backend specific, and thus could act as transaction control mechanism for example, but depending on the backend.

A shortcut of using "with connection.cursor() as c: ..." in place of "c = connection.cursor(); try: ... finally: c.close()" could be nice.

comment:9 Changed 4 years ago by Michael Manfre

Having a context manager with consistent behavior for the backends makes sense. Adding a patch with documentation to point out the changing behaviors.

Changed 4 years ago by Michael Manfre

Attachment: django-ticket17671.diff added

context manager shortcut without pass-thru

comment:10 Changed 4 years ago by Aymeric Augustin

Owner: changed from Aymeric Augustin to nobody

comment:11 Changed 4 years ago by Anssi Kääriäinen

Triage Stage: Design decision neededAccepted

I am marking this as accepted, the semantics should be that:

with connection.cursor() as c:

is equivalent to

c = connection.cursor()

That is, we avoid database specific implementations like SQLite's transaction management semantics.

comment:12 Changed 3 years ago by Michael Manfre

Didn't realize the previous patch didn't make it in to 1.5. Here's a pull request against master.

comment:13 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

In 99c87f1410106ce543a1a0332428afc472beef7f:

Fixed #17671 - Cursors are now context managers.

comment:14 Changed 3 years ago by Tim Graham <timograham@…>

In 0d02c5429970804068520f0dc6f0ae9fb3e08b9c:

Fixed #21207 -- Fixed test failure on Oracle: test_cursor_contextmanager

refs #17671

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