[Django] #24042: Custom AutoField fields do not work correctly on postgres
Django
-------------------------------+--------------------
Reporter: lucaswiman | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24042>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: lucaswiman | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by lucaswiman):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> 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.
New description:
Patch also available at https://github.com/django/django/pull/3776
{{{
test_auto_field (field_subclassing.tests.CustomField) ... ERROR
'''How to Reproduce'''
'''Testing'''
--
Comment:
Pat
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:1>
Django
Reporter: lucaswiman | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Uncategorized => Database layer (models, ORM)
* needs_better_patch: 0 => 1
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:2>
Django
Reporter: Lucas Wiman | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I think this ticket should probably be closed as ''fixed'' in the light of
#27452 and support for the generic `db_returning` flag.
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:3>
Django
Reporter: Lucas Wiman | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* resolution: => fixed
Comment:
Agreed. Creating custom auto fields should be possible with the
`db_returning` flag.
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:4>
Django
Reporter: Lucas Wiman | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Nick Pope):
Replying to [comment:4 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
[https://github.com/django/django/pull/3776 PR].
Eventually we could decide to deprecate inheritance from `AutoField`
itself which has always been more tightly coupled with integer-based auto-
fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:5>
Django
Reporter: Lucas Wiman | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by felixxm):
Tests from [https://github.com/django/django/pull/3776 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:6>
Django
Reporter: Lucas Wiman | Owner: nobody
Type: New feature | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Natalia Bidart):
* cc: Simon Charette (added)
Comment:
Replying to [comment:3 Simon Charette]:
of #27452 and support for the generic `db_returning` flag.
start the review iteration process for getting this into 6.0. Nick
references this ticket in ticket:32577#comment:11, and I'm not sure his
concerns in comment:5 (in this ticket) were ever fully addressed. Would
you have any availability to take another look at his questions around the
ticket closure?
--
Ticket URL: <https://code.djangoproject.com/ticket/24042#comment:7>