Opened 12 years ago

Closed 10 years ago

Last modified 10 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

Description

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\tests.py", 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 12 years ago.
context manager shortcut without pass-thru

Download all attachments as: .zip

Change History (15)

comment:1 by Claude Paroz, 12 years ago

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)
     29 
     30     def __iter__(self):

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

comment:2 by denisenkom@…, 12 years ago

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 12 years ago by Aymeric Augustin (previous) (diff)

comment:3 by denisenkom, 12 years ago

Cc: denisenkom added

comment:4 by denisenkom, 12 years ago

Here is more detailed description of underlying problem with python 2.7
http://stackoverflow.com/questions/4146095/getattr-doesnt-work-with-enter-attributeerror

comment:5 by Aymeric Augustin, 12 years ago

Owner: changed from nobody to Aymeric Augustin
Status: reopenednew

comment:6 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedDesign decision needed

This code should never have worked; see http://bugs.python.org/issue9220 (and its duplicate http://bugs.python.org/issue9259).

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 by Michael Manfre, 12 years ago

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 by Anssi Kääriäinen, 12 years ago

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 by Michael Manfre, 12 years ago

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

by Michael Manfre, 12 years ago

Attachment: django-ticket17671.diff added

context manager shortcut without pass-thru

comment:10 by Aymeric Augustin, 12 years ago

Owner: changed from Aymeric Augustin to nobody

comment:11 by Anssi Kääriäinen, 11 years ago

Triage Stage: Design decision neededAccepted

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

with connection.cursor() as c:
    do_something

is equivalent to

c = connection.cursor()
try:
    do_something
finally:
    c.close()

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

comment:12 by Michael Manfre, 10 years ago

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

https://github.com/django/django/pull/1673

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

Resolution: fixed
Status: newclosed

In 99c87f1410106ce543a1a0332428afc472beef7f:

Fixed #17671 - Cursors are now context managers.

comment:14 by Tim Graham <timograham@…>, 10 years ago

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