Commit 38d6e1e2 authored by Antoine Catton's avatar Antoine Catton Committed by Tim Graham
Browse files

[1.9.x] Fixed #25535 -- Made ForeignObject checks less strict.

Check that the foreign object `from_fields` are a subset of any unique
constraints on the foreign model.

Backport of 80dac8c3 and
c7aff313 from master
parent aa0a3b68
Loading
Loading
Loading
Loading
+19 −6
Original line number Diff line number Diff line
@@ -473,22 +473,35 @@ class ForeignObject(RelatedField):
        if not self.foreign_related_fields:
            return []

        has_unique_field = any(rel_field.unique
            for rel_field in self.foreign_related_fields)
        if not has_unique_field and len(self.foreign_related_fields) > 1:
        unique_foreign_fields = {
            frozenset([f.name])
            for f in self.remote_field.model._meta.get_fields()
            if getattr(f, 'unique', False)
        }
        unique_foreign_fields.update({
            frozenset(ut)
            for ut in self.remote_field.model._meta.unique_together
        })
        foreign_fields = {f.name for f in self.foreign_related_fields}
        has_unique_constraint = any(u <= foreign_fields for u in unique_foreign_fields)

        if not has_unique_constraint and len(self.foreign_related_fields) > 1:
            field_combination = ', '.join("'%s'" % rel_field.name
                for rel_field in self.foreign_related_fields)
            model_name = self.remote_field.model.__name__
            return [
                checks.Error(
                    "None of the fields %s on model '%s' have a unique=True constraint."
                    "No subset of the fields %s on model '%s' is unique."
                    % (field_combination, model_name),
                    hint=None,
                    hint=(
                        "Add unique=True on any of those fields or add at "
                        "least a subset of them to a unique_together constraint."
                    ),
                    obj=self,
                    id='fields.E310',
                )
            ]
        elif not has_unique_field:
        elif not has_unique_constraint:
            field_name = self.foreign_related_fields[0].name
            model_name = self.remote_field.model.__name__
            return [
+3 −2
Original line number Diff line number Diff line
@@ -189,8 +189,9 @@ Related Fields
  for ``<field name>``.
* **fields.E306**: Related name must be a valid Python identifier or end with
  a ``'+'``.
* **fields.E310**: None of the fields ``<field1>``, ``<field2>``, ... on model
  ``<model>`` have a ``unique=True`` constraint.
* **fields.E310**: No subset of the fields ``<field1>``, ``<field2>``, ... on
  model ``<model>`` is unique. Add ``unique=True`` on any of those fields or
  add at least a subset of them to a unique_together constraint.
* **fields.E311**: ``<model>`` must set ``unique=True`` because it is
  referenced by a ``ForeignKey``.
* **fields.E320**: Field specifies ``on_delete=SET_NULL``, but cannot be null.
+52 −1
Original line number Diff line number Diff line
@@ -2,7 +2,9 @@ import datetime
from operator import attrgetter

from django.core.exceptions import FieldError
from django.test import TestCase, skipUnlessDBFeature
from django.db import models
from django.db.models.fields.related import ForeignObject
from django.test import SimpleTestCase, TestCase, skipUnlessDBFeature
from django.utils import translation

from .models import (
@@ -391,3 +393,52 @@ class MultiColumnFKTests(TestCase):
        """ See: https://code.djangoproject.com/ticket/21566 """
        objs = [Person(name="abcd_%s" % i, person_country=self.usa) for i in range(0, 5)]
        Person.objects.bulk_create(objs, 10)


class TestModelCheckTests(SimpleTestCase):

    def test_check_composite_foreign_object(self):
        class Parent(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()

            class Meta:
                unique_together = (('a', 'b'),)

        class Child(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            value = models.CharField(max_length=255)
            parent = ForeignObject(
                Parent,
                on_delete=models.SET_NULL,
                from_fields=('a', 'b'),
                to_fields=('a', 'b'),
                related_name='children',
            )

        self.assertEqual(Child._meta.get_field('parent').check(from_model=Child), [])

    def test_check_subset_composite_foreign_object(self):
        class Parent(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            c = models.PositiveIntegerField()

            class Meta:
                unique_together = (('a', 'b'),)

        class Child(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            c = models.PositiveIntegerField()
            d = models.CharField(max_length=255)
            parent = ForeignObject(
                Parent,
                on_delete=models.SET_NULL,
                from_fields=('a', 'b', 'c'),
                to_fields=('a', 'b', 'c'),
                related_name='children',
            )

        self.assertEqual(Child._meta.get_field('parent').check(from_model=Child), [])
+80 −3
Original line number Diff line number Diff line
@@ -5,6 +5,7 @@ import warnings

from django.core.checks import Error, Warning as DjangoWarning
from django.db import models
from django.db.models.fields.related import ForeignObject
from django.test import ignore_warnings
from django.test.testcases import skipIfDBFeature
from django.test.utils import override_settings
@@ -507,9 +508,11 @@ class RelativeFieldTests(IsolatedModelsTestCase):
        errors = field.check()
        expected = [
            Error(
                "None of the fields 'country_id', 'city_id' on model 'Person' "
                "have a unique=True constraint.",
                hint=None,
                "No subset of the fields 'country_id', 'city_id' on model 'Person' is unique.",
                hint=(
                    "Add unique=True on any of those fields or add at least "
                    "a subset of them to a unique_together constraint."
                ),
                obj=field,
                id='fields.E310',
            )
@@ -1395,3 +1398,77 @@ class M2mThroughFieldsTests(IsolatedModelsTestCase):
                obj=field,
                id='fields.E337')]
        self.assertEqual(expected, errors)

    def test_superset_foreign_object(self):
        class Parent(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            c = models.PositiveIntegerField()

            class Meta:
                unique_together = (('a', 'b', 'c'),)

        class Child(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            value = models.CharField(max_length=255)
            parent = ForeignObject(
                Parent,
                on_delete=models.SET_NULL,
                from_fields=('a', 'b'),
                to_fields=('a', 'b'),
                related_name='children',
            )

        field = Child._meta.get_field('parent')
        errors = field.check(from_model=Child)
        expected = [
            Error(
                "No subset of the fields 'a', 'b' on model 'Parent' is unique.",
                hint=(
                    "Add unique=True on any of those fields or add at least "
                    "a subset of them to a unique_together constraint."
                ),
                obj=field,
                id='fields.E310',
            ),
        ]
        self.assertEqual(expected, errors)

    def test_insersection_foreign_object(self):
        class Parent(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            c = models.PositiveIntegerField()
            d = models.PositiveIntegerField()

            class Meta:
                unique_together = (('a', 'b', 'c'),)

        class Child(models.Model):
            a = models.PositiveIntegerField()
            b = models.PositiveIntegerField()
            d = models.PositiveIntegerField()
            value = models.CharField(max_length=255)
            parent = ForeignObject(
                Parent,
                on_delete=models.SET_NULL,
                from_fields=('a', 'b', 'd'),
                to_fields=('a', 'b', 'd'),
                related_name='children',
            )

        field = Child._meta.get_field('parent')
        errors = field.check(from_model=Child)
        expected = [
            Error(
                "No subset of the fields 'a', 'b', 'd' on model 'Parent' is unique.",
                hint=(
                    "Add unique=True on any of those fields or add at least "
                    "a subset of them to a unique_together constraint."
                ),
                obj=field,
                id='fields.E310',
            ),
        ]
        self.assertEqual(expected, errors)