[Django] #35712: CheckConstraint with RawSQL breaks ModelForm when inside transaction.atomic()

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-----------------------+-----------------------------------------
Reporter: a-p-f | Type: Bug
Status: new | Component: Uncategorized
Version: 5.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
-----------------------+-----------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35712>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

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

-------------------------------------+-------------------------------------
Reporter: a-p-f | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
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 Simon Charette):

* component: Uncategorized => Database layer (models, ORM)
* stage: Unreviewed => Accepted

Comment:

Accepting on the basis that performing the validation, or not, should not
leave the database in a unusable state. A potential patch could be

{{{#!diff
diff --git a/django/db/models/query_utils.py
b/django/db/models/query_utils.py
index 1bf396723e..1b41314bc7 100644
--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -10,9 +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,13 +132,20 @@ def check(self, against, using=DEFAULT_DB_ALIAS):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:1>

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-------------------------------------+-------------------------------------
Reporter: a-p-f | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:2>

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-------------------------------------+-------------------------------------
Reporter: a-p-f | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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
[https://docs.djangoproject.com/en/5.1/internals/contributing/writing-code
/unit-tests/ this part of the documentation] which covers the basics. In
this ticket's case you'll want to adjust
[https://github.com/django/django/blob/2b71b2c8dcd40f2604310bb3914077320035b399/tests/queries/test_q.py#L317-L329
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
[https://docs.djangoproject.com/en/5.1/internals/contributing/writing-code
/unit-tests/#running-tests-using-django-docker-box 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:3>

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-------------------------------------+-------------------------------------
Reporter: a-p-f | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by a-p-f):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:4>

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-------------------------------------+-------------------------------------

Reporter: a-p-f | Owner: a-p-f
Type: Bug | Status: assigned

Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* owner: (none) => a-p-f
* status: new => assigned

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:5>

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-------------------------------------+-------------------------------------
Reporter: a-p-f | Owner: a-p-f
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:6>

[Django]

#35712: CheckConstraint with RawSQL breaks ModelForm when inside
transaction.atomic()
-------------------------------------+-------------------------------------
Reporter: a-p-f | Owner: a-p-f

Type: Bug | Status: closed

Component: Database layer | Version: 5.1
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"c6a4f853c7167c1001761dcff30d7a64690e8236" c6a4f853]:
{{{#!CommitTicketReference repository=""
revision="c6a4f853c7167c1001761dcff30d7a64690e8236"
Fixed #35712 -- Prevented Q.check() from leaving the connection in an
unusable state.

Co-authored-by: Simon Charette <chare...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35712#comment:7>