From f39b0421b406b411c3bcb58e8aa415d885fea505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20Guimar=C3=A3es?= Date: Mon, 8 Sep 2014 19:38:07 +0200 Subject: [PATCH] Fixed #23338 -- Added warning when unique=True on ForeigKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks Jonathan Lindén for the initial patch, and Tim Graham and Gabe Jackson for the suggestions. --- django/contrib/auth/tests/test_management.py | 1 + django/db/models/fields/related.py | 15 +++++++++++++++ docs/ref/checks.txt | 2 ++ tests/admin_checks/tests.py | 5 +++++ tests/check_framework/tests.py | 10 ++++++---- tests/contenttypes_tests/tests.py | 2 ++ tests/model_fields/tests.py | 18 ++++++++++++++++++ tests/model_validation/tests.py | 5 +++++ 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**: ``.`` is not a foreign key to ````. * **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