CheckConstraint with RawSQL breaks ModelForm when inside transaction.atomic()

[a-p-f]

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.

[Simon Charette]

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

@@ -10 +10 @@
 import inspect
 import logging
 from collections import namedtuple
+from contextlib import nullcontext
 
 from django.core.exceptions import FieldError
-from django.db import DEFAULT_DB_ALIAS, DatabaseError, connections
+from django.db import DEFAULT_DB_ALIAS, DatabaseError, connections, transaction
 from django.db.models.constants import LOOKUP_SEP
 from django.utils import tree
 from django.utils.functional import cached_property
@@ -131 +132 @@
             query.add_annotation(value, name, select=False)
         query.add_annotation(Value(1), "_check")
         # This will raise a FieldError if a field is missing in "against".
-        if connections[using].features.supports_comparing_boolean_expr:
+        connection = connections[using]
+        if connection.features.supports_comparing_boolean_expr:
             query.add_q(Q(Coalesce(self, True, output_field=BooleanField())))
         else:
             query.add_q(self)
         compiler = query.get_compiler(using=using)
+        context_manager = (
+            transaction.atomic(using=using)
+            if connection.in_atomic_block
+            else nullcontext()
+        )
         try:
-            return compiler.execute_sql(SINGLE) is not None
+            with context_manager:
+                return compiler.execute_sql(SINGLE) is not None
         except DatabaseError as e:
             logger.warning("Got a database error calling check() on %r: %s", self, e)
             return True

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.

[a-p-f]

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

[Simon Charette]

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.

[Simon Charette]

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.

[Sarah Boyce <42296566+sarahboyce@…>]

In c6a4f853:

Fixed #35712 -- Prevented Q.check() from leaving the connection in an unusable state.

Co-authored-by: Simon Charette <charette.s@…>