Opened 5 years ago

Closed 5 years ago

#16340 closed Cleanup/optimization (fixed)

get_or_create should preserve the original traceback.

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

Description

(more context in #15117)

get_or_create in django.db.models.query re-raises the last exception as below. This modifies the traceback and the exception should preserve this.

            except IntegrityError, e:
                transaction.savepoint_rollback(sid, using=self.db)
                try:
                    return self.get(**lookup), False
                except self.model.DoesNotExist:
                    raise e

this can be fixed with

                exc_info = sys.exc_info()
                try:
                    return self.get(**lookup), False
                except self.model.DoesNotExist:
                    raise exc_info[1], None, exc_info[2]

Attachments (4)

16340_get_or_create_traceback.patch (2.1 KB) - added by Dougal Matthews 5 years ago.
16340_get_or_create_traceback.2.patch (2.1 KB) - added by Dougal Matthews 5 years ago.
16340_get_or_create_traceback.3.patch (2.1 KB) - added by Dougal Matthews 5 years ago.
16340.patch (2.3 KB) - added by Jonas Obrist 5 years ago.
new patch (same as https://github.com/ojii/django/compare/django:master...ojii:16340)

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by Dougal Matthews

Changed 5 years ago by Dougal Matthews

comment:1 Changed 5 years ago by Dougal Matthews

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Added a patch that preserves the original traceback with a test. The test isn't ideal since we need to check the contents of a traceback, but it looks for the save call since this is where the IntegrityError comes from (in other words its the safest thing we can bet on being there.)

(Trac added the upload twice for some reason?)

Last edited 5 years ago by Dougal Matthews (previous) (diff)

comment:2 Changed 5 years ago by Dougal Matthews

For reference. This patch changes a traceback like this:

Traceback (most recent call last):
  File "/vagrant/tests/modeltests/get_or_create/tests.py", line 60, in test_get_or_create
    ManualPrimaryKeyTest.objects.get_or_create(id=1, data="Different")
  File "/vagrant/django/db/models/manager.py", line 135, in get_or_create
    return self.get_query_set().get_or_create(**kwargs)
  File "/vagrant/django/db/models/query.py", line 396, in get_or_create
    raise e
IntegrityError: PRIMARY KEY must be unique

to show the original full exception traceback:

Traceback (most recent call last):
  File "/vagrant/tests/modeltests/get_or_create/tests.py", line 60, in test_get_or_create
    ManualPrimaryKeyTest.objects.get_or_create(id=1, data="Different")
  File "/vagrant/django/db/models/manager.py", line 135, in get_or_create
    return self.get_query_set().get_or_create(**kwargs)
  File "/vagrant/django/db/models/query.py", line 386, in get_or_create
    obj.save(force_insert=True, using=self.db)
  File "/vagrant/django/db/models/base.py", line 463, in save
    self.save_base(using=using, force_insert=force_insert, force_update=force_update)
  File "/vagrant/django/db/models/base.py", line 556, in save_base
    result = manager._insert(values, return_id=update_pk, using=using)
  File "/vagrant/django/db/models/manager.py", line 198, in _insert
    return insert_query(self.model, values, **kwargs)
  File "/vagrant/django/db/models/query.py", line 1459, in insert_query
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/vagrant/django/db/models/sql/compiler.py", line 810, in execute_sql
    cursor = super(SQLInsertCompiler, self).execute_sql(None)
  File "/vagrant/django/db/models/sql/compiler.py", line 754, in execute_sql
    cursor.execute(sql, params)
  File "/vagrant/django/db/backends/sqlite3/base.py", line 226, in execute
    return Database.Cursor.execute(self, query, params)
IntegrityError: PRIMARY KEY must be unique

comment:3 Changed 5 years ago by Ramiro Morales

Triage Stage: UnreviewedAccepted

Changed 5 years ago by Dougal Matthews

comment:4 Changed 5 years ago by Jonas Obrist

Owner: changed from nobody to Jonas Obrist

Updated the patch to apply to trunk.

Also made use of the new unittest2 features (namely assertIn).

https://github.com/ojii/django/compare/django:master...ojii:16340

Changed 5 years ago by Jonas Obrist

Attachment: 16340.patch added

comment:5 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: newclosed

In [17333]:

Fixed #16340 -- Made get_or_create() re-raise any IntegrityError with its original traceback. Thanks to d0ugal and Jonas Obrist.

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