Opened 3 months ago

Closed 3 months ago

#36433 closed Bug (fixed)

Model validation of constraints fails if condition's Q object references foreign key by _id

Reported by: Jacob Walls Owned by: colleenDunlap
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords:
Cc: Mariusz Felisiak 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

Bisected to e44e8327d3d88d86895735c0e427102063ff5b55 (refs #36222).

With these models:

class Language(models.Model):
    pass


class LanguageValue(models.Model):
    language = models.ForeignKey(
        Language,
        on_delete=models.PROTECT,
        null=True,
        blank=True,
    )
    value = models.CharField(max_length=1024, null=False, blank=True)

    class Meta:
        constraints = [
            models.CheckConstraint(
                condition=Q(language_id__isnull=False),
                name="require_language",
            ),
        ]

This works on 5.2 and fails on main. Notice wrong "Choices are: _check". (Curiously it works if the Q() object is adjusted to reference language rather than language_id.)

In [1]: lang = Language.objects.create()

In [2]: value = LanguageValue.objects.create(language=lang)

In [3]: value.full_clean()
--------------------------------------------
FieldError Traceback (most recent call last)
Cell In[3], line 1
----> 1 value.full_clean()

File ~/django/django/db/models/base.py:1633, in Model.full_clean(self, exclude, validate_unique, validate_constraints)
   1631         exclude.add(name)
   1632 try:
-> 1633     self.validate_constraints(exclude=exclude)
   1634 except ValidationError as e:
   1635     errors = e.update_error_dict(errors)

File ~/django/django/db/models/base.py:1581, in Model.validate_constraints(self, exclude)
   1579 for constraint in model_constraints:
   1580     try:
-> 1581         constraint.validate(model_class, self, exclude=exclude, using=using)
   1582     except ValidationError as e:
   1583         if (
   1584             getattr(e, "code", None) == "unique"
   1585             and len(constraint.fields) == 1
   1586         ):

File ~/django/django/db/models/constraints.py:212, in CheckConstraint.validate(self, model, instance, exclude, using)
    210 if exclude and self._expression_refs_exclude(model, self.condition, exclude):
    211     return
--> 212 if not Q(self.condition).check(against, using=using):
    213     raise ValidationError(
    214         self.get_violation_error_message(), code=self.violation_error_code
    215     )

File ~/django/django/db/models/query_utils.py:137, in Q.check(self, against, using)
    135 # This will raise a FieldError if a field is missing in "against".
    136 if connection.features.supports_comparing_boolean_expr:
--> 137     query.add_q(Q(Coalesce(self, True, output_field=BooleanField())))
    138 else:
    139     query.add_q(self)

File ~/django/django/db/models/sql/query.py:1646, in Query.add_q(self, q_object, reuse_all)
   1644 else:
   1645     can_reuse = self.used_aliases
-> 1646 clause, _ = self._add_q(q_object, can_reuse)
   1647 if clause:
   1648     self.where.add(clause, AND)

File ~/django/django/db/models/sql/query.py:1678, in Query._add_q(self, q_object, used_aliases, branch_negated, current_negated, allow_joins, split_subq, check_filterable, summarize, update_join_types)
   1674 joinpromoter = JoinPromoter(
   1675     q_object.connector, len(q_object.children), current_negated
   1676 )
   1677 for child in q_object.children:
-> 1678     child_clause, needed_inner = self.build_filter(
   1679         child,
   1680         can_reuse=used_aliases,
   1681         branch_negated=branch_negated,
   1682         current_negated=current_negated,
   1683         allow_joins=allow_joins,
   1684         split_subq=split_subq,
   1685         check_filterable=check_filterable,
   1686         summarize=summarize,
   1687         update_join_types=update_join_types,
   1688     )
   1689     joinpromoter.add_votes(needed_inner)
   1690     if child_clause:

File ~/django/django/db/models/sql/query.py:1517, in Query.build_filter(self, filter_expr, branch_negated, current_negated, can_reuse, allow_joins, split_subq, check_filterable, summarize, update_join_types)
   1515 if not getattr(filter_expr, "conditional", False):
   1516     raise TypeError("Cannot filter against a non-conditional expression.")
-> 1517 condition = filter_expr.resolve_expression(
   1518     self, allow_joins=allow_joins, reuse=can_reuse, summarize=summarize
   1519 )
   1520 if not isinstance(condition, Lookup):
   1521     condition = self.build_lookup(["exact"], condition, True)

File ~/django/django/db/models/expressions.py:300, in BaseExpression.resolve_expression(self, query, allow_joins, reuse, summarize, for_save)
    296 c = self.copy()
    297 c.is_summary = summarize
    298 source_expressions = [
    299     (
--> 300         expr.resolve_expression(query, allow_joins, reuse, summarize)
    301         if expr is not None
    302         else None
    303     )
    304     for expr in c.get_source_expressions()
    305 ]
    306 if not self.allows_composite_expressions and any(
    307     isinstance(expr, ColPairs) for expr in source_expressions
    308 ):
    309     raise ValueError(
    310         f"{self.__class__.__name__} expression does not support "
    311         "composite primary keys."
    312     )

File ~/django/django/db/models/query_utils.py:91, in Q.resolve_expression(self, query, allow_joins, reuse, summarize, for_save)
     86 def resolve_expression(
     87     self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False
     88 ):
     89     # We must promote any new joins to left outer joins so that when Q is
     90     # used as an expression, rows aren't filtered due to joins.
---> 91     clause, joins = query._add_q(
     92         self,
     93         reuse,
     94         allow_joins=allow_joins,
     95         split_subq=False,
     96         check_filterable=False,
     97         summarize=summarize,
     98     )
     99     query.promote_joins(joins)
    100     return clause

File ~/django/django/db/models/sql/query.py:1678, in Query._add_q(self, q_object, used_aliases, branch_negated, current_negated, allow_joins, split_subq, check_filterable, summarize, update_join_types)
   1674 joinpromoter = JoinPromoter(
   1675     q_object.connector, len(q_object.children), current_negated
   1676 )
   1677 for child in q_object.children:
-> 1678     child_clause, needed_inner = self.build_filter(
   1679         child,
   1680         can_reuse=used_aliases,
   1681         branch_negated=branch_negated,
   1682         current_negated=current_negated,
   1683         allow_joins=allow_joins,
   1684         split_subq=split_subq,
   1685         check_filterable=check_filterable,
   1686         summarize=summarize,
   1687         update_join_types=update_join_types,
   1688     )
   1689     joinpromoter.add_votes(needed_inner)
   1690     if child_clause:

File ~/django/django/db/models/sql/query.py:1503, in Query.build_filter(self, filter_expr, branch_negated, current_negated, can_reuse, allow_joins, split_subq, check_filterable, summarize, update_join_types)
   1501     raise FieldError("Cannot parse keyword query as dict")
   1502 if isinstance(filter_expr, Q):
-> 1503     return self._add_q(
   1504         filter_expr,
   1505         branch_negated=branch_negated,
   1506         current_negated=current_negated,
   1507         used_aliases=can_reuse,
   1508         allow_joins=allow_joins,
   1509         split_subq=split_subq,
   1510         check_filterable=check_filterable,
   1511         summarize=summarize,
   1512         update_join_types=update_join_types,
   1513     )
   1514 if hasattr(filter_expr, "resolve_expression"):
   1515     if not getattr(filter_expr, "conditional", False):

File ~/django/django/db/models/sql/query.py:1678, in Query._add_q(self, q_object, used_aliases, branch_negated, current_negated, allow_joins, split_subq, check_filterable, summarize, update_join_types)
   1674 joinpromoter = JoinPromoter(
   1675     q_object.connector, len(q_object.children), current_negated
   1676 )
   1677 for child in q_object.children:
-> 1678     child_clause, needed_inner = self.build_filter(
   1679         child,
   1680         can_reuse=used_aliases,
   1681         branch_negated=branch_negated,
   1682         current_negated=current_negated,
   1683         allow_joins=allow_joins,
   1684         split_subq=split_subq,
   1685         check_filterable=check_filterable,
   1686         summarize=summarize,
   1687         update_join_types=update_join_types,
   1688     )
   1689     joinpromoter.add_votes(needed_inner)
   1690     if child_clause:

File ~/django/django/db/models/sql/query.py:1526, in Query.build_filter(self, filter_expr, branch_negated, current_negated, can_reuse, allow_joins, split_subq, check_filterable, summarize, update_join_types)
   1524 if not arg:
   1525     raise FieldError("Cannot parse keyword query %r" % arg)
-> 1526 lookups, parts, reffed_expression = self.solve_lookup_type(arg, summarize)
   1528 if check_filterable:
   1529     self.check_filterable(reffed_expression)

File ~/django/django/db/models/sql/query.py:1333, in Query.solve_lookup_type(self, lookup, summarize)
   1331             expression = Ref(annotation, expression)
   1332         return expression_lookups, (), expression
-> 1333 _, field, _, lookup_parts = self.names_to_path(lookup_splitted, self.get_meta())
   1334 field_parts = lookup_splitted[0 : len(lookup_splitted) - len(lookup_parts)]
   1335 if len(lookup_parts) > 1 and not field_parts:

File ~/django/django/db/models/sql/query.py:1805, in Query.names_to_path(self, names, opts, allow_many, fail_on_missing)
   1797     if pos == -1 or fail_on_missing:
   1798         available = sorted(
   1799             [
   1800                 *get_field_names_from_opts(opts),
   (...)
   1803             ]
   1804         )
-> 1805         raise FieldError(
   1806             "Cannot resolve keyword '%s' into field. "
   1807             "Choices are: %s" % (name, ", ".join(available))
   1808         )
   1809     break
   1810 # Check if we need any joins for concrete inheritance cases (the
   1811 # field lives in parent, but we are currently in one of its
   1812 # children)

FieldError: Cannot resolve keyword 'language_id' into field. Choices are: _check

Change History (8)

comment:1 by Simon Charette, 3 months ago

Triage Stage: UnreviewedAccepted

Thanks for the report Jacob!

I think this is a bug in _get_field_expression_map, it should augment the map with Field.attname as well

  • django/db/models/base.py

    diff --git a/django/db/models/base.py b/django/db/models/base.py
    index d4559e0693..901743147d 100644
    a b def _get_field_expression_map(self, meta, exclude=None):  
    13221322            if not value or not hasattr(value, "resolve_expression"):
    13231323                value = Value(value, field)
    13241324            field_map[field.name] = value
     1325            field_map[field.attname] = value
    13251326        if "pk" not in exclude:
    13261327            field_map["pk"] = Value(self.pk, meta.pk)
    13271328        if generated_fields:
  • tests/constraints/tests.py

    diff --git a/tests/constraints/tests.py b/tests/constraints/tests.py
    index 20a5357cc5..2816f01150 100644
    a b def test_validate_pk_field(self):  
    361361            constraint_with_pk.validate(ChildModel, ChildModel(id=1, age=1))
    362362        constraint_with_pk.validate(ChildModel, ChildModel(pk=1, age=1), exclude={"pk"})
    363363
     364    def test_validate_fk_attname(self):
     365        constraint_with_pk = models.CheckConstraint(
     366            condition=models.Q(uniqueconstraintproduct_ptr_id__isnull=False),
     367            name="parent_ptr_present",
     368        )
     369        with self.assertRaises(ValidationError):
     370            constraint_with_pk.validate(
     371                ChildUniqueConstraintProduct, ChildUniqueConstraintProduct()
     372            )
     373
    364374    @skipUnlessDBFeature("supports_json_field")
    365375    def test_validate_jsonfield_exact(self):
    366376        data = {"release": "5.0.2", "version": "stable"}

As for the unhelpful error message I wonder if we should make Q.check catch FieldError and re-reraise with against keys instead. Maybe not worth it as users should hit this code path normally.

comment:2 by colleenDunlap, 3 months ago

Owner: set to colleenDunlap
Status: newassigned

comment:3 by Mariusz Felisiak, 3 months ago

Cc: Mariusz Felisiak added

comment:4 by Colleen Dunlap, 3 months ago

Has patch: set
Last edited 3 months ago by Colleen Dunlap (previous) (diff)

comment:7 by Sarah Boyce, 3 months ago

Triage Stage: AcceptedReady for checkin

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

Resolution: fixed
Status: assignedclosed

In 830e69a:

Fixed #36433 -- Fixed constraint validation crash when condition uses a ForeignKey attname.

Regression in e44e8327d3d88d86895735c0e427102063ff5b55.

Thank you to Jacob Walls for the report.

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

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