Opened 3 months ago
Closed 3 months ago
#35712 closed Bug (fixed)
CheckConstraint with RawSQL breaks ModelForm when inside transaction.atomic()
Reported by: | a-p-f | Owned by: | a-p-f |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 5.1 |
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
When a model uses a CheckConstraint
with RawSQL
, trying to save a ModelForm
for that model inside a transaction.atomic()
block fails.
Project Setup
settings.py:
INSTALLED_APPS = [ "test_app", ... ] DATABASES = { "default": { "ENGINE": "django.db.backends.postgresql", ... } }
test_app/models.py (the Bar model has a CheckConstraint using RawSQL):
from django.db import models from django.db.models.expressions import ExpressionWrapper, RawSQL class Bar(models.Model): a = models.IntegerField(null=True) class Meta: constraints = [ models.CheckConstraint( name="a_is_1", check=ExpressionWrapper( RawSQL( "a = 1", []), output_field=models.BooleanField(), ), ), ]
test_app/management_commands/form_test.py (a ModelForm is used to create a Bar instance):
from django import forms from django.core.management import BaseCommand from django.db import transaction from test_app.models import Bar class BarForm(forms.ModelForm): class Meta: model = Bar fields = ["a"] class Command(BaseCommand): def handle(self, **options): with transaction.atomic(): form = BarForm({"a": 1}) form.save()
Test Procedure
>>> python manage.py form_test
Expected Outcome
The code runs without any exceptions, and a new Bar instance is created.
Actual Outcome
form.save()
raises django.db.utils.InternalError
Traceback (most recent call last): File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 105, in _execute return self.cursor.execute(sql, params) psycopg2.errors.InFailedSqlTransaction: current transaction is aborted, commands ignored until end of transaction block The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/Users/alex/projects/django_constraint_issue/manage.py", line 22, in <module> main() File "/Users/alex/projects/django_constraint_issue/manage.py", line 18, in main execute_from_command_line(sys.argv) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line utility.execute() File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/core/management/__init__.py", line 436, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/core/management/base.py", line 413, in run_from_argv self.execute(*args, **cmd_options) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/core/management/base.py", line 459, in execute output = self.handle(*args, **options) File "/Users/alex/projects/django_constraint_issue/test_app/management/commands/form_test.py", line 18, in handle form.save() File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/forms/models.py", line 554, in save self.instance.save() File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/base.py", line 891, in save self.save_base( File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/base.py", line 997, in save_base updated = self._save_table( File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/base.py", line 1160, in _save_table results = self._do_insert( File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/base.py", line 1201, in _do_insert return manager._insert( File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/manager.py", line 87, in manager_method return getattr(self.get_queryset(), name)(*args, **kwargs) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/query.py", line 1847, in _insert return query.get_compiler(using=using).execute_sql(returning_fields) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 1836, in execute_sql cursor.execute(sql, params) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 122, in execute return super().execute(sql, params) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 79, in execute return self._execute_with_wrappers( File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 92, in _execute_with_wrappers return executor(sql, params, many, context) File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 100, in _execute with self.db.wrap_database_errors: File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/utils.py", line 91, in __exit__ raise dj_exc_value.with_traceback(traceback) from exc_value File "/Users/alex/projects/django_constraint_issue/venv/lib/python3.10/site-packages/django/db/backends/utils.py", line 105, in _execute return self.cursor.execute(sql, params) django.db.utils.InternalError: current transaction is aborted, commands ignored until end of transaction block
Notes
Whether you use an atomic transaction or not, Django _tries_ to validate the CheckConstraint before inserting the new Bar instance. This logs the following warning:
Got a database error calling check() on <Q: (AND: ExpressionWrapper(RawSQL(a = 1, [])))>: column "a" does not exist LINE 1: SELECT 1 AS "_check" WHERE COALESCE(((a = 1)), true)
Django appears to log this warning and then carry on. If the connection is in auto-commit mode, this is not a problem. The next statement will be executed in a new transaction. In an atomic block, this causes the InternalError
.
When you migrate, Django does warn about the CheckConstraint with RawSQL:
>>> python manage.py migrate System check identified some issues: WARNINGS: test_app.Bar: (models.W045) Check constraint 'a_is_1' contains RawSQL() expression and won't be validated during the model full_clean(). HINT: Silence this warning if you don't care about it.
It seems that rather than skipping the validation of this constraint, Django is still validating it and then ignoring/logging the error. And it doesn't handle transaction state properly.
Change History (7)
comment:1 by , 3 months ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 3 months ago
Thanks for the quick input Simon.
I gave a quick stab at writing a PR, but I'm not familiar enough with Django's test suite. This issue does not occur when using sqlite, and I'm not sure how to run any of the tests from Django's test suite using postgres as the database engine.
I'm happy to keep working on this if someone wants to point me in the right direction, but it's such a small patch/test that it's probably easier for someone else to handle it.
Alex
comment:3 by , 3 months ago
I gave a quick stab at writing a PR, but I'm not familiar enough with Django's test suite.
I suggest your refer to this part of the documentation which covers the basics. In this ticket's case you'll want to adjust this test to ensure that the connection is still usable after q.check
is called.
This issue does not occur when using sqlite, and I'm not sure how to run any of the tests from Django's test suite using postgres as the database engine.
In order to test against Postgres, which is where the issues manifests itself, I suggest you have a look at django-docker-box if you have Docker installed locally. Once checked out you should simply have to point it at your local django/django checkout to run your adjusted tests against Postgres.
I'm happy to keep working on this if someone wants to point me in the right direction, but it's such a small patch/test that it's probably easier for someone else to handle it.
I think it's the perfect opportunity to contribute back a code change given the scope of the ticket and we're happy to support you through it. If you're willing to do so you should start by assigning this ticket to yourself in the Mofify Ticket section of this tracker.
comment:4 by , 3 months ago
Has patch: | set |
---|
comment:5 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
I'm not entirely sure the explicit DEFAULT_DB_ALIAS
usage is necessary in the test but otherwise the patch is on point. Thanks for submitting it a-p-f.
comment:6 by , 3 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Accepting on the basis that performing the validation, or not, should not leave the connection in a unusable state. A potential patch could be
django/db/models/query_utils.py
If you're interested in submitting a PR it looks like
test_q.QCheckTests.test_rawsql
could be adjusted to confirm that the connection is still usable after database error.