[Django] #37446: DatabaseError ignored in Q.check() during validate_constraints()

[Django]

#37446: DatabaseError ignored in Q.check() during validate_constraints()
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Normal
Keywords: constraint, | Triage Stage:
CheckConstraint | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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`
([https://dryorm.xterm.info/database-error-logged-and-ignored dry ORM
fiddle]) where I would expect an error:
{{{#!py
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:

{{{#!diff
diff --git a/django/db/models/query_utils.py
b/django/db/models/query_utils.py
index 279e153ac1..931f15a182 100644
--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -175,8 +175,10 @@ class Q(tree.Node):
"""
# Avoid circular imports.
from django.db.models import Value
+ from django.db.models.expressions import RawSQL
from django.db.models.sql import Query
from django.db.models.sql.constants import SINGLE

query = Query(None)
for name, value in against.items():
@@ -198,7 +200,14 @@ class Q(tree.Node):
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
+ # Be lenient: if any WHERE clause contained RawSQL, return
True.
+ if any(
+ isinstance(ref, RawSQL)
+ for leaf in query.where.leaves()
+ for ref in leaf.flatten()
+ ):
+ return True
+ return False

def deconstruct(self):
path = "%s.%s" % (self.__class__.__module__,
self.__class__.__name__)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37446>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.