Code

Opened 2 years ago

Closed 10 months ago

Last modified 9 months ago

#17671 closed Bug (fixed)

CursorWrapper in Python 2.7 cannot be used as a contextmanager

Reported by: 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 manfre 2 years ago.
context manager shortcut without pass-thru

Download all attachments as: .zip

Change History (15)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 Changed 2 years ago by denisenkom@…

  • Has patch set
  • Resolution invalid deleted
  • Status changed from closed to reopened

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 2 years ago by aaugustin (previous) (diff)

comment:3 Changed 2 years ago by denisenkom

  • Cc denisenkom added

comment:4 Changed 2 years ago by denisenkom

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 Changed 2 years ago by aaugustin

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

comment:6 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Design 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 Changed 2 years ago by 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 2 years ago by akaariai

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 2 years ago by 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 2 years ago by manfre

context manager shortcut without pass-thru

comment:10 Changed 22 months ago by aaugustin

  • Owner changed from aaugustin to nobody

comment:11 Changed 18 months ago by akaariai

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 10 months ago by manfre

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 Changed 10 months ago by Anssi Kääriäinen <akaariai@…>

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

In 99c87f1410106ce543a1a0332428afc472beef7f:

Fixed #17671 - Cursors are now context managers.

comment:14 Changed 9 months ago by Tim Graham <timograham@…>

In 0d02c5429970804068520f0dc6f0ae9fb3e08b9c:

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

refs #17671

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.