Code

Opened 3 years ago

Closed 2 years ago

#16340 closed Cleanup/optimization (fixed)

get_or_create should preserve the original traceback.

Reported by: d0ugal Owned by: ojii
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 d0ugal 3 years ago.
16340_get_or_create_traceback.2.patch (2.1 KB) - added by d0ugal 3 years ago.
16340_get_or_create_traceback.3.patch (2.1 KB) - added by d0ugal 3 years ago.
16340.patch (2.3 KB) - added by ojii 2 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 3 years ago by d0ugal

Changed 3 years ago by d0ugal

comment:1 Changed 3 years ago by d0ugal

  • 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 3 years ago by d0ugal (previous) (diff)

comment:2 Changed 3 years ago by d0ugal

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 3 years ago by ramiro

  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by d0ugal

comment:4 Changed 2 years ago by ojii

  • Owner changed from nobody to ojii

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

comment:5 Changed 2 years ago by julien

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

In [17333]:

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

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.