Opened 3 months ago

Closed 2 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 Simon Charette, 3 months ago

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted

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

    diff --git a/django/db/models/query_utils.py b/django/db/models/query_utils.py
    index 1bf396723e..1b41314bc7 100644
    a b  
    1010import inspect
    1111import logging
    1212from collections import namedtuple
     13from contextlib import nullcontext
    1314
    1415from django.core.exceptions import FieldError
    15 from django.db import DEFAULT_DB_ALIAS, DatabaseError, connections
     16from django.db import DEFAULT_DB_ALIAS, DatabaseError, connections, transaction
    1617from django.db.models.constants import LOOKUP_SEP
    1718from django.utils import tree
    1819from django.utils.functional import cached_property
    def check(self, against, using=DEFAULT_DB_ALIAS):  
    131132            query.add_annotation(value, name, select=False)
    132133        query.add_annotation(Value(1), "_check")
    133134        # This will raise a FieldError if a field is missing in "against".
    134         if connections[using].features.supports_comparing_boolean_expr:
     135        connection = connections[using]
     136        if connection.features.supports_comparing_boolean_expr:
    135137            query.add_q(Q(Coalesce(self, True, output_field=BooleanField())))
    136138        else:
    137139            query.add_q(self)
    138140        compiler = query.get_compiler(using=using)
     141        context_manager = (
     142            transaction.atomic(using=using)
     143            if connection.in_atomic_block
     144            else nullcontext()
     145        )
    139146        try:
    140             return compiler.execute_sql(SINGLE) is not None
     147            with context_manager:
     148                return compiler.execute_sql(SINGLE) is not None
    141149        except DatabaseError as e:
    142150            logger.warning("Got a database error calling check() on %r: %s", self, e)
    143151            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.

Last edited 3 months ago by Simon Charette (previous) (diff)

comment:2 by a-p-f, 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 Simon Charette, 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 a-p-f, 3 months ago

Has patch: set

comment:5 by Simon Charette, 3 months ago

Owner: set to a-p-f
Status: newassigned

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 Sarah Boyce, 2 months ago

Triage Stage: AcceptedReady for checkin

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 2 months ago

Resolution: fixed
Status: assignedclosed

In c6a4f853:

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

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

Note: See TracTickets for help on using tickets.
Back to Top