Opened 5 weeks ago

Closed 4 weeks ago

#36086 closed Bug (fixed)

Creating an instance of a model with a non-auto primary key and a generated field crashes on backends not supporting returning columns from inserts

Reported by: Simon Charette Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 5.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Discovered while working on #36075.

---

The SQLInsertCompiler.execute_sql changes (7254f1138d9c51fa558229c39c9559b369c4278a) made to accommodate returning_fields support (#29444) failed to account for the possibility that backends not supporting returning columns from inserts (MySQL, SQLite < 3.35, and Oracle when the use_returning_into option is disabled) could still request returning_fields and not have an auto-primary key when GeneratedField was introduced.

Given the following model

class TestModel(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4)
    a = models.IntegerField()
    b = models.GeneratedField(
        expression=F("a"),
        output_field=models.IntegerField(),
        db_persist=True,
    )

Attempting to perform TestModel.objects.create(a=1) crashes because even if there is no usage of AutoField we still attempt to call last_insert_id and for the UUIDField primary key (which will either return 0 or crash on Oracle since it attempts to retrieve a non-existing sequence) and then try to convert 0 to a uuid.UUID which crashes with

Traceback (most recent call last):
  File "/django/source/tests/model_fields/test_generatedfield.py", line 361, in test_create_with_non_auto_pk
    obj = GeneratedModelNonAutoPk.objects.create(a=1, b=2)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/query.py", line 663, in create
    obj.save(force_insert=True, using=self.db)
  File "/django/source/django/db/models/base.py", line 901, in save
    self.save_base(
  File "/django/source/django/db/models/base.py", line 1007, in save_base
    updated = self._save_table(
              ^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/base.py", line 1168, in _save_table
    results = self._do_insert(
              ^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/base.py", line 1209, in _do_insert
    return manager._insert(
           ^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/query.py", line 1845, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/models/sql/compiler.py", line 1927, in execute_sql
    return list(rows)
           ^^^^^^^^^^
  File "/django/source/django/db/models/sql/compiler.py", line 1541, in apply_converters
    value = converter(value, expression, connection)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/backends/mysql/operations.py", line 327, in convert_uuidfield_value
    value = uuid.UUID(value)
            ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/uuid.py", line 175, in __init__
    hex = hex.replace('urn:', '').replace('uuid:', '')
          ^^^^^^^^^^^
AttributeError: 'int' object has no attribute 'replace'

Note that in a case of a non-integer primary, like described above we get a crash, but in a case of an integer one the issue is even more insidious as the bogus value (which will be either be 0 or whatever value the last inserted table with an auto primary key was) will happily pass the conversion layer and then be wrongly assigned to GeneratedField value. In other words, if the model is defined like the following instead

class TestModel(models.Model):
    id = models.IntegerField(primary_key=True)
    a = models.IntegerField()
    b = models.GeneratedField(
        expression=F("a"),
        output_field=models.IntegerField(),
        db_persist=True,
    )

Then we'll encounter the following problem

>>> obj = TestModel.object.create(id=1, a=42)
>>> obj.b
1  # This should be 42!!

Change History (5)

comment:1 by Simon Charette, 5 weeks ago

Has patch: set

comment:2 by Mariusz Felisiak, 4 weeks ago

Triage Stage: UnreviewedAccepted

comment:3 by Mariusz Felisiak, 4 weeks ago

Patch needs improvement: set
Triage Stage: AcceptedReady for checkin

comment:4 by Mariusz Felisiak, 4 weeks ago

Patch needs improvement: unset

comment:5 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

Resolution: fixed
Status: assignedclosed

In 9e55201:

Fixed #36086 -- Fixed crash when using GeneratedField with non-AutoField pk.

The previous logic was systematically attempting to retrieve last_insert_id
even for models without an AutoField primary key when they had a GeneratedField
on backends that can't return columns from INSERT.

The issue affected MySQL, SQLite < 3.35, and Oracle when the use_returning_into
option was disabled and could result in either crashes when the non-auto
primary key wasn't an IntegerField subclass or silent misassignment of bogus
insert ids (0 or the previous auto primary key insert value) to the first
defined generated field value.

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