Fixed #23338 -- Added warning when unique=True on ForeigKey

Thanks Jonathan Lindén for the initial patch, and Tim Graham
and Gabe Jackson for the suggestions.

8 files changed, 54 insertions, 4 deletions

diff --git a/django/contrib/auth/tests/test_management.py b/django/contrib/auth/tests/test_management.py
index 20f6c58f05..72d61a0919 100644
--- a/django/contrib/auth/tests/test_management.py
+++ b/django/contrib/auth/tests/test_management.py
@@ -153,6 +153,7 @@ class ChangepasswordManagementCommandTestCase(TestCase):
 
 
 @skipIfCustomUser
+@override_settings(SILENCED_SYSTEM_CHECKS=['fields.W342'])  # ForeignKey(unique=True)
 class CreatesuperuserManagementCommandTestCase(TestCase):
 
     def test_basic_usage(self):
diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py
index ea88d19a17..04189ad097 100644
--- a/django/db/models/fields/related.py
+++ b/django/db/models/fields/related.py
@@ -1710,6 +1710,7 @@ class ForeignKey(ForeignObject):
     def check(self, **kwargs):
         errors = super(ForeignKey, self).check(**kwargs)
         errors.extend(self._check_on_delete())
+        errors.extend(self._check_unique())
         return errors
 
     def _check_on_delete(self):
@@ -1735,6 +1736,16 @@ class ForeignKey(ForeignObject):
         else:
             return []
 
+    def _check_unique(self, **kwargs):
+        return [
+            checks.Warning(
+                'Setting unique=True on a ForeignKey has the same effect as using a OneToOneField.',
+                hint='ForeignKey(unique=True) is usually better served by a OneToOneField.',
+                obj=self,
+                id='fields.W342',
+            )
+        ] if self.unique else []
+
     def deconstruct(self):
         name, path, args, kwargs = super(ForeignKey, self).deconstruct()
         del kwargs['to_fields']
@@ -1891,6 +1902,10 @@ class OneToOneField(ForeignKey):
         else:
             setattr(instance, self.attname, data)
 
+    def _check_unique(self, **kwargs):
+        # override ForeignKey since check isn't applicable here
+        return []
+
 
 def create_many_to_many_intermediary_model(field, klass):
     from django.db import models
diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt
index 86b287c0f2..03e74ecd1a 100644
--- a/docs/ref/checks.txt
+++ b/docs/ref/checks.txt
@@ -154,6 +154,8 @@ Related Fields
 * **fields.E339**: ``<model>.<field name>`` is not a foreign key to ``<model>``.
 * **fields.W340**: ``null`` has no effect on ``ManyToManyField``.
 * **fields.W341**: ``ManyToManyField`` does not support ``validators``.
+* **fields.W342**: Setting ``unique=True`` on a ``ForeignKey`` has the same
+  effect as using a ``OneToOneField``.
 
 Signals
 ~~~~~~~
diff --git a/tests/admin_checks/tests.py b/tests/admin_checks/tests.py
index 799fc2899a..1e9b6f4646 100644
--- a/tests/admin_checks/tests.py
+++ b/tests/admin_checks/tests.py
@@ -8,6 +8,7 @@ from django.contrib.contenttypes.admin import GenericStackedInline
 from django.core import checks
 from django.core.exceptions import ImproperlyConfigured
 from django.test import TestCase
+from django.test.utils import override_settings
 
 from .models import Song, Book, Album, TwoAlbumFKAndAnE, City, State, Influence
 
@@ -34,6 +35,10 @@ class ValidFormFieldsets(admin.ModelAdmin):
     )
 
 
+@override_settings(
+    SILENCED_SYSTEM_CHECKS=['fields.W342'],  # ForeignKey(unique=True)
+    INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes', 'admin_checks']
+)
 class SystemChecksTestCase(TestCase):
 
     def test_checks_are_performed(self):
diff --git a/tests/check_framework/tests.py b/tests/check_framework/tests.py
index db06c138da..a6f086aa86 100644
--- a/tests/check_framework/tests.py
+++ b/tests/check_framework/tests.py
@@ -8,7 +8,6 @@ from django.apps import apps
 from django.conf import settings
 from django.core import checks
 from django.core.checks import Error, Warning
-from django.core.checks.model_checks import check_all_models
 from django.core.checks.registry import CheckRegistry
 from django.core.checks.compatibility.django_1_7_0 import check_1_7_compatibility
 from django.core.management.base import CommandError
@@ -303,7 +302,10 @@ class CheckFrameworkReservedNamesTests(TestCase):
             del self.current_models[model]
         apps.clear_cache()
 
-    @override_settings(SILENCED_SYSTEM_CHECKS=['models.E020'])
+    @override_settings(
+        SILENCED_SYSTEM_CHECKS=['models.E20', 'fields.W342'],  # ForeignKey(unique=True)
+        INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes', 'check_framework']
+    )
     def test_model_check_method_not_shadowed(self):
         class ModelWithAttributeCalledCheck(models.Model):
             check = 42
@@ -318,6 +320,7 @@ class CheckFrameworkReservedNamesTests(TestCase):
             check = models.ForeignKey(ModelWithRelatedManagerCalledCheck)
             article = models.ForeignKey(ModelWithRelatedManagerCalledCheck, related_name='check')
 
+        errors = checks.run_checks()
         expected = [
             Error(
                 "The 'ModelWithAttributeCalledCheck.check()' class method is "
@@ -341,5 +344,4 @@ class CheckFrameworkReservedNamesTests(TestCase):
                 id='models.E020'
             ),
         ]
-
-        self.assertEqual(check_all_models(), expected)
+        self.assertEqual(errors, expected)
diff --git a/tests/contenttypes_tests/tests.py b/tests/contenttypes_tests/tests.py
index 20efbe4145..64414a8732 100644
--- a/tests/contenttypes_tests/tests.py
+++ b/tests/contenttypes_tests/tests.py
@@ -105,6 +105,7 @@ class IsolatedModelsTestCase(TestCase):
         apps.clear_cache()
 
 
+@override_settings(SILENCED_SYSTEM_CHECKS=['fields.W342'])  # ForeignKey(unique=True)
 class GenericForeignKeyTests(IsolatedModelsTestCase):
 
     def test_str(self):
@@ -202,6 +203,7 @@ class GenericForeignKeyTests(IsolatedModelsTestCase):
         ]
         self.assertEqual(errors, expected)
 
+    @override_settings(INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes', 'contenttypes_tests'])
     def test_generic_foreign_key_checks_are_performed(self):
         class MyGenericForeignKey(GenericForeignKey):
             def check(self, **kwargs):
diff --git a/tests/model_fields/tests.py b/tests/model_fields/tests.py
index 650e6dc0f9..db04b7fcad 100644
--- a/tests/model_fields/tests.py
+++ b/tests/model_fields/tests.py
@@ -8,6 +8,7 @@ import warnings
 from django import test
 from django import forms
 from django.core import validators
+from django.core import checks
 from django.core.exceptions import ValidationError
 from django.db import connection, transaction, models, IntegrityError
 from django.db.models.fields import (
@@ -20,6 +21,7 @@ from django.db.models.fields import (
 from django.db.models.fields.files import FileField, ImageField
 from django.utils import six
 from django.utils.functional import lazy
+from django.test.utils import override_settings
 
 from .models import (
     Foo, Bar, Whiz, BigD, BigS, BigIntegerModel, Post, NullBooleanModel,
@@ -181,6 +183,22 @@ class ForeignKeyTests(test.TestCase):
         fk_model_empty = FkToChar.objects.select_related('out').get(id=fk_model_empty.pk)
         self.assertEqual(fk_model_empty.out, char_model_empty)
 
+    @override_settings(INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes', 'model_fields'])
+    def test_warning_when_unique_true_on_fk(self):
+        class FKUniqueTrue(models.Model):
+            fk_field = models.ForeignKey(Foo, unique=True)
+
+        expected_warnings = [
+            checks.Warning(
+                'Setting unique=True on a ForeignKey has the same effect as using a OneToOneField.',
+                hint='ForeignKey(unique=True) is usually better served by a OneToOneField.',
+                obj=FKUniqueTrue.fk_field.field,
+                id='fields.W342',
+            )
+        ]
+        warnings = checks.run_checks()
+        self.assertEqual(warnings, expected_warnings)
+
 
 class DateTimeFieldTests(unittest.TestCase):
     def test_datetimefield_to_python_usecs(self):
diff --git a/tests/model_validation/tests.py b/tests/model_validation/tests.py
index a9fb30cbcd..50ccfd2de6 100644
--- a/tests/model_validation/tests.py
+++ b/tests/model_validation/tests.py
@@ -3,6 +3,7 @@ from django.core.checks import run_checks, Error
 from django.db.models.signals import post_init
 from django.test import TestCase
 from django.utils import six
+from django.test.utils import override_settings
 
 
 class OnPostInit(object):
@@ -14,6 +15,10 @@ def on_post_init(**kwargs):
     pass
 
 
+@override_settings(
+    INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes'],
+    SILENCED_SYSTEM_CHECKS=['fields.W342'],  # ForeignKey(unique=True)
+)
 class ModelValidationTest(TestCase):
     def test_models_validate(self):
         # All our models should validate properly