#37446 new Bug

DatabaseError ignored in Q.check() during validate_constraints()

Reported by: Jacob Walls Owned by:
Component: Database layer (models, ORM) Version: 5.2
Severity: Normal Keywords: constraint, CheckConstraint
Cc: Simon Charette Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A pragmatic compromise was done in 5d91dc8ee3ff7e2230cfacf41005f61b8efbf057 (#35081) to log and swallow a DatabaseError that arises during constraint validation, to support the case where a RawSQL expression in a constraint cannot be translated to SQL.

Still, there are cases like this reproducer without RawSQL (​dry ORM fiddle) where I would expect an error:

from django.db import models
from django.db.models.functions import Cast
from django.db.models import CheckConstraint
from django.db.models.lookups import GreaterThan
from django.db.models.fields.json import KT


class Order(models.Model):
    data = models.JSONField()

    class Meta:
        constraints = [
            models.CheckConstraint(
                condition=GreaterThan(
                    Cast(KT("data__quantity"), models.IntegerField()),
                    0
                ),
                name="positive_quantity",
            )
        ]


def run():
    instance = Order(data={"quantity": "not an int"})
    instance.validate_constraints()
    assert False, "see logged content above"

The above snippet should have failed validation, but it succeeds, and then fails on assert False where you can see this logged output in the fiddle:

Got a database error calling check() on <Q: (AND: Coalesce(GreaterThan(Cast(KeyTextTransform(F(data))), 0), Value(True)))>: invalid input syntax for type integer: "not an int"

Swallowing the error was questioned later during triage of #34840, and in ticket:34840#comment:19, Simon asked that we "not forget" about coming back to this.

I think we should avoid swallowing the error, while still being lenient for RawSQL. Maybe something like this, although I'm sure there are interesting nested cases to check:

  • django/db/models/query_utils.py

    diff --git a/django/db/models/query_utils.py b/django/db/models/query_utils.py
    index 279e153ac1..931f15a182 100644
    a b class Q(tree.Node): (this hunk was shorter than expected)  
    175175        """
    176176        # Avoid circular imports.
    177177        from django.db.models import Value
     178        from django.db.models.expressions import RawSQL
    178179        from django.db.models.sql import Query
    179180        from django.db.models.sql.constants import SINGLE
    180181
    181182        query = Query(None)
    182183        for name, value in against.items():
    … … class Q(tree.Node):  
    198200                return compiler.execute_sql(SINGLE) is not None
    199201        except DatabaseError as e:
    200202            logger.warning("Got a database error calling check() on %r: %s", self, e)
    201             return True
     203            # Be lenient: if any WHERE clause contained RawSQL, return True.
     204            if any(
     205                isinstance(ref, RawSQL)
     206                for leaf in query.where.leaves()
     207                for ref in leaf.flatten()
     208            ):
     209                return True
     210            return False
    202211
    203212    def deconstruct(self):
    204213        path = "%s.%s" % (self.__class__.__module__, self.__class__.__name__)

Change History (0)

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