[Django] #29655: Cannot enter Django admin interface when model instances are validated on save
Django
save
-------------------------------------------+------------------------
Reporter: Evgeny Arshinov | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
Hello,
In #29549 I was advised to enable model validation on save to properly
validate choice fields, like this:
{{{
@receiver(pre_save)
def pre_save_handler(sender, instance, *args, **kwargs):
instance.full_clean()
}}}
Turned out this approach breaks Django session's middleware and, as a
result, no one can enter Django admin interface (which uses the session
middleware).
Scenario:
1. Use a project that includes Django admin interface.
2. Enable model validation:
{{{
from django.db.models.signals import pre_save
from django.dispatch import receiver
@receiver(pre_save)
def pre_save_handler(sender, instance, *args, **kwargs):
instance.full_clean()
}}}
3. Visit Django admin interface start page (`/admin`).
4. Enter valid credentials.
The result is error 500 due to a `ValidationError`:
{{{
Internal Server Error: /admin/login/
Traceback (most recent call last):
File "C:\django_sample\venv\lib\site-
packages\django\core\handlers\exception.py", line 34, in inner
response = get_response(request)
File "C:\django_sample\venv\lib\site-
packages\django\utils\deprecation.py", line 93, in __call__
response = self.process_response(request, response)
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\middleware.py", line 58, in
process_response
request.session.save()
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\backends\db.py", line 87, in save
obj.save(force_insert=must_create, force_update=not must_create,
using=using)
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 717, in save
force_update=force_update, update_fields=update_fields)
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 742, in save_base
update_fields=update_fields,
File "C:\django_sample\venv\lib\site-
packages\django\dispatch\dispatcher.py", line 175, in send
for receiver in self._live_receivers(sender)
File "C:\django_sample\venv\lib\site-
packages\django\dispatch\dispatcher.py", line 175, in <listcomp>
for receiver in self._live_receivers(sender)
File "C:\django_sample\django_sample\models.py", line 41, in
pre_save_handler
instance.full_clean()
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 1151, in full_clean
raise ValidationError(errors)
django.core.exceptions.ValidationError: {'session_key': ['Session with
this Session key already exists.']}
}}}
We dug this problem a bit, and found out that during the login procedure
saving a `django.contrib.sessions.Session` instance is performed twice
with the same session key:
- In the first case, the object is saved with `force_insert=True,
force_update=False`
- In the second case, the object is saved with `force_insert=False,
force_update=True`
- Hovewer, in both cases the object's `_state.adding` flag is `True`,
which causes `django.db.Model._perform_unique_checks` to include the value
of the unique field of the object being saved (in this case, a session
key) in unique validation. Since there is already an object with the given
session key in the database after the first save, `validate_unique`
reports an error.
Traceback for the first save operation (some bottom frames omitted for
brevity):
{{{
...
File "C:\django_sample\venv\lib\site-
packages\django\contrib\auth\views.py", line 90, in form_valid
auth_login(self.request, form.get_user())
File "C:\django_sample\venv\lib\site-
packages\django\contrib\auth\__init__.py", line 108, in login
request.session.cycle_key()
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\backends\base.py", line 298, in cycle_key
self.create()
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\backends\db.py", line 55, in create
self.save(must_create=True)
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\backends\db.py", line 87, in save
obj.save(force_insert=must_create, force_update=not must_create,
using=using)
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 717, in save
force_update=force_update, update_fields=update_fields)
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 742, in save_base
update_fields=update_fields,
File "C:\django_sample\venv\lib\site-
packages\django\dispatch\dispatcher.py", line 175, in send
for receiver in self._live_receivers(sender)
File "C:\django_sample\venv\lib\site-
packages\django\dispatch\dispatcher.py", line 175, in <listcomp>
for receiver in self._live_receivers(sender)
File "C:\django_sample\django_sample\models.py", line 41, in
pre_save_handler
instance.full_clean()
}}}
Traceback for the second save operation:
{{{
...
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\middleware.py", line 58, in
process_response
request.session.save()
File "C:\django_sample\venv\lib\site-
packages\django\contrib\sessions\backends\db.py", line 87, in save
obj.save(force_insert=must_create, force_update=not must_create,
using=using)
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 717, in save
force_update=force_update, update_fields=update_fields)
File "C:\django_sample\venv\lib\site-packages\django\db\models\base.py",
line 742, in save_base
update_fields=update_fields,
File "C:\django_sample\venv\lib\site-
packages\django\dispatch\dispatcher.py", line 175, in send
for receiver in self._live_receivers(sender)
File "C:\django_sample\venv\lib\site-
packages\django\dispatch\dispatcher.py", line 175, in <listcomp>
for receiver in self._live_receivers(sender)
File "C:\django_sample\django_sample\models.py", line 41, in
pre_save_handler
instance.full_clean()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29655>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
save
Reporter: Evgeny Arshinov | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Evgeny Arshinov):
I have committed a demo project:
https://github.com/earshinov/django_sample/tree/issue-29655
For reproducing:
{{{
git clone https://github.com/earshinov/django_sample.git
cd django_sample
git checkout issue-29655
python3 -m venv venv
. ./venv/bin/activate
pip install -r requirements.txt
python manage.py migrate
python manage.py createsuperuser
# enter credentials to your liking
python manage.py runserver 127.0.0.1:9726
}}}
Next, visit http://localhost:9726/admin/, enter the credentials and click
the [Log in] button.
--
Ticket URL: <https://code.djangoproject.com/ticket/29655#comment:1>
Django
save
Reporter: Evgeny Arshinov | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Evgeny Arshinov):
As a temporary solution in our project we excluded instances of
`django.contrib.sessions.Session` objects from validation:
{{{
@receiver(pre_save)
def pre_save_handler(sender, instance, *args, **kwargs):
if not isinstance(instance, Session):
instance.full_clean()
pass
}}}
but it certainly is an ugly workaround.
--
Ticket URL: <https://code.djangoproject.com/ticket/29655#comment:2>
Django
save
Reporter: Evgeny Arshinov | Owner: nobody
Component: Uncategorized | Version: 2.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* status: new => closed
* resolution: => invalid
Comment:
Enabling model validation on save is likely to break a few things and
degrade performance significantly. I don't think that's a pattern we
should encourage or support at this point.
Django and third-party applications simply don't expect `ValidationError`
to be thrown on `save()`, this effectively change the signature of the
function.
--
Ticket URL: <https://code.djangoproject.com/ticket/29655#comment:3>
Django
save
Reporter: Evgeny Arshinov | Owner: nobody
Component: Uncategorized | Version: 2.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Evgeny Arshinov):
[comment:7:ticket:29549 Reopened] #29549.
--
Ticket URL: <https://code.djangoproject.com/ticket/29655#comment:4>