Opened 23 months ago

Closed 22 months ago

Last modified 22 months ago

#20761 closed Bug (fixed)

UnboundLocalError: local variable 'sid' referenced before assignment when using get_or_create with a badly formed __init__

Reported by: alastair.porter@… Owned by: loic84
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The cause of this bug seems to be a little convoluted. I don't expect it to happen in normal use, but it still seems to be unexpected behaviour.

The smallest testcase I can create it with is:

class Test(models.Model):
    def __init__(self, **kwargs):
        import django.db
        raise django.db.IntegrityError("oops")

The cause of the Integrity error isn't important. I was trying some stuff in a custom init that didn't end up working. The error I get is:

In [2]: models.Test.objects.get_or_create(id=1)

/env/local/lib/python2.7/site-packages/django/db/models/manager.pyc in get_or_create(self, **kwargs)
    144 
    145     def get_or_create(self, **kwargs):
--> 146         return self.get_query_set().get_or_create(**kwargs)
    147 
    148     def create(self, **kwargs):

/env/local/lib/python2.7/site-packages/django/db/models/query.pyc in get_or_create(self, **kwargs)
    479                 return obj, True
    480             except IntegrityError as e:
--> 481                 transaction.savepoint_rollback(sid, using=self.db)
    482                 exc_info = sys.exc_info()
    483                 try:

UnboundLocalError: local variable 'sid' referenced before assignment

The reason for this seems to be that the try: block in query.py:get_or_create expects the IntegrityError to be thrown when the new object is saved, not when it's created (line 476).

Change History (8)

comment:1 Changed 23 months ago by claudep

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I guess that the obj = self.model(**params) line (now in _create_object_from_params in master) should be outside the try/except construct.

comment:2 Changed 23 months ago by loic84

@claudep the try/except construct is there to catch a race condition.

[0e51d8eb66121b2558a38c9f4e366df781046873] changed the method to handle transaction better, however I don't like how any error would lead to a self.get().

I'd suggest the following snippet:

except DatabaseError as e:
    transaction.savepoint_rollback(sid, using=self.db)
    if isinstance(e, IntegrityError):
        return self.get(**lookup), False

Refs. #20463 #20429 #12579 #13906

Version 0, edited 23 months ago by loic84 (next)

comment:4 Changed 22 months ago by loic84

  • Has patch set
  • Owner changed from nobody to loic84
  • Status changed from new to assigned
  • Version changed from 1.5 to master

comment:5 Changed 22 months ago by Tim Graham <timograham@…>

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

In e716518ad29898fb25c820023aaf2fdd1c9eb4af:

Fixed #20761 -- Fixed DatabaseError handling in get_or_create and update_or_create.

comment:6 Changed 22 months ago by Tim Graham <timograham@…>

comment:7 Changed 22 months ago by Claude Paroz <claude@…>

In 311c1d2848bde59bf03627e5c3d72b319285201b:

Fixed #20761 -- Reworded ForeignKey default error message

comment:8 Changed 22 months ago by claudep

Oups, sorry for the wrong ticket number, the latest commit above refers to #20791

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