#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 )
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)
Change History (7)
by , 10 years ago
Attachment: | patch.diff added |
---|
comment:1 by , 10 years ago
Description: | modified (diff) |
---|
comment:2 by , 10 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → New feature |
comment:3 by , 5 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.
follow-up: 5 comment:4 by , 5 years ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
Resolution: | → fixed |
Status: | new → closed |
Agreed. Creating custom auto fields should be possible with the db_returning
flag.
comment:5 by , 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 , 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.
Pat