Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#24042 closed New feature (fixed)

Custom AutoField fields do not work correctly on postgres

Reported by: Lucas Wiman Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Lucas Wiman)

Patch also available at https://github.com/django/django/pull/3776

The documentation (https://docs.djangoproject.com/en/dev/howto/custom-model-fields/#emulating-built-in-field-types) on Field.get_internal_type implies that it should define custom behavior and ensure that database-correct behavior is followed. For Postgres (though not for sqlite), specifying get_internal_type() to be "AutoField" fails to correctly set the field, leading to errors like this:

test_auto_field (field_subclassing.tests.CustomField) ... ERROR

======================================================================
ERROR: test_auto_field (field_subclassing.tests.CustomField)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/lucaswiman/opensource/django/tests/field_subclassing/tests.py", line 128, in test_auto_field
    o.save()
  File "/Users/lucaswiman/opensource/django/django/db/models/base.py", line 694, in save
    force_update=force_update, update_fields=update_fields)
  File "/Users/lucaswiman/opensource/django/django/db/models/base.py", line 722, in save_base
    updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
  File "/Users/lucaswiman/opensource/django/django/db/models/base.py", line 803, in _save_table
    result = self._do_insert(cls._base_manager, using, fields, update_pk, raw)
  File "/Users/lucaswiman/opensource/django/django/db/models/base.py", line 842, in _do_insert
    using=using, raw=raw)
  File "/Users/lucaswiman/opensource/django/django/db/models/manager.py", line 131, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/Users/lucaswiman/opensource/django/django/db/models/query.py", line 954, in _insert
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/Users/lucaswiman/opensource/django/django/db/models/sql/compiler.py", line 931, in execute_sql
    cursor.execute(sql, params)
  File "/Users/lucaswiman/opensource/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/Users/lucaswiman/opensource/django/django/db/utils.py", line 95, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/Users/lucaswiman/opensource/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
IntegrityError: null value in column "id" violates not-null constraint
DETAIL:  Failing row contains (null).

The attached diff adds a regression test which fails on postgres databases with the error above. I also fixed all other uses of isinstance(..., AutoField) I could find in the code.

The immediate motivation for this is writing integration tests which use django-salesforce on a CI system that uses a postgres database. I've filed a separate issue there, though I think fixing the behavior in Django to match the documentation would be a preferable solution. The package maintainer for django-salesforce agrees (https://github.com/django-salesforce/django-salesforce/issues/90#issuecomment-67932443).

How to Reproduce

Check out the master branch and apply my diff from the tests/ directory. Run the tests with a settings file pointing to a postgres database:

PYTHONPATH=..:$PYTHONPATH ./runtests.py --verbosity=2 --failfast field_subclassing --settings=test_postgres

It will fail with the above stack trace.

Testing

I verified that the regression test fails without the remainder of the diff. I also verified that the tests still pass on the default test_sqlite file.

Attachments (1)

patch.diff (11.9 KB ) - added by Lucas Wiman 9 years ago.

Download all attachments as: .zip

Change History (7)

by Lucas Wiman, 9 years ago

Attachment: patch.diff added

comment:1 by Lucas Wiman, 9 years ago

Description: modified (diff)

Pat

comment:2 by Tim Graham, 9 years ago

Component: UncategorizedDatabase layer (models, ORM)
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

comment:3 by Simon Charette, 4 years ago

I think this ticket should probably be closed as fixed in the light of #27452 and support for the generic db_returning flag.

comment:4 by Mariusz Felisiak, 4 years ago

Has patch: unset
Patch needs improvement: unset
Resolution: fixed
Status: newclosed

Agreed. Creating custom auto fields should be possible with the db_returning flag.

in reply to:  4 comment:5 by Nick Pope, 4 years ago

Replying to felixxm:

Agreed. Creating custom auto fields should be possible with the db_returning flag.

I'm not entirely convinced that everything has been resolved here. Is db_returning enough?

After digging, the issue was that a custom text-based field was being used and AutoField wasn't being subclassed.

AutoField is essentially IntegerField with some additional bits on top. Hence the refactoring in #29979 to segregate out the bits that make it an auto-field and to correct the inheritance from the appropriate field for the desired data type. The next stage was to work toward making class TextAutoField(AutoFieldMixin, TextField): or class UUIDAutoField(AutoFieldMixin, UUIDField): workable. This ticket was about replacing all of the isinstance(..., AutoField) checks to use some property such as db_generated and/or is_auto. See the unfinished PR.

Eventually we could decide to deprecate inheritance from AutoField itself which has always been more tightly coupled with integer-based auto-fields.

comment:6 by Mariusz Felisiak, 4 years ago

Tests from PR works for me when I will declare db_returning = True in CustomDbGeneratedField, e.g.

class Default(Func):
    template = 'DEFAULT'


class CustomDbGeneratedField(models.Field):
    db_returning = True
    empty_strings_allowed = False

    def __init__(self, *args, **kwargs):
        kwargs.setdefault('default', Default)
        super().__init__(*args, **kwargs)

    def db_type(self, connection):
        if connection.vendor == 'mysql':
            return 'integer AUTO_INCREMENT'
        elif connection.vendor == 'postgresql':
            return 'serial'
        else:
            return 'integer'

    def get_internal_type(self):
        return "NotAutoField"

db_returning is still a private API but it's feasible to create a custom generated field.

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