Opened 4 years ago

Closed 4 years ago

Last modified 4 years 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


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)
    145     def get_or_create(self, **kwargs):
--> 146         return self.get_query_set().get_or_create(**kwargs)
    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 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 4 years ago by Claude Paroz

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted

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 4 years 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.

Edit: Probably shouldn't look at the ORM code at 4AM. The self.model(**params) part can/should be moved outside of the try/except construct, only the save logic needs to be there.

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

comment:4 Changed 4 years ago by loic84

Has patch: set
Owner: changed from nobody to loic84
Status: newassigned
Version: 1.5master

comment:5 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e716518ad29898fb25c820023aaf2fdd1c9eb4af:

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

comment:6 Changed 4 years ago by Tim Graham <timograham@…>

comment:7 Changed 4 years ago by Claude Paroz <claude@…>

In 311c1d2848bde59bf03627e5c3d72b319285201b:

Fixed #20761 -- Reworded ForeignKey default error message

comment:8 Changed 4 years ago by Claude Paroz

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