From 8d3519071ec001f763b70a3a1f98ae2e980bd552 Mon Sep 17 00:00:00 2001 From: Adnan Umer Date: Sat, 3 Aug 2019 16:22:27 +0500 Subject: [PATCH] Fixed #30673 -- Relaxed system check for db_table collision when database routers are installed by turning the error into a warning. --- AUTHORS | 1 + django/core/checks/model_checks.py | 20 ++++++-- docs/ref/checks.txt | 2 + docs/releases/2.2.5.txt | 4 +- tests/check_framework/test_model_checks.py | 60 +++++++++++++++++++++- 5 files changed, 80 insertions(+), 7 deletions(-) diff --git a/AUTHORS b/AUTHORS index 640ef009204..8df700a3e95 100644 --- a/AUTHORS +++ b/AUTHORS @@ -18,6 +18,7 @@ answer newbie questions, and generally made Django that much better: Adam Malinowski Adam Vandenberg Adiyat Mubarak + Adnan Umer Adrian Holovaty Adrien Lemaire Afonso Fernández Nogueira diff --git a/django/core/checks/model_checks.py b/django/core/checks/model_checks.py index 5c2266ca1d9..7b156fceee6 100644 --- a/django/core/checks/model_checks.py +++ b/django/core/checks/model_checks.py @@ -4,7 +4,8 @@ from collections import defaultdict from itertools import chain from django.apps import apps -from django.core.checks import Error, Tags, register +from django.conf import settings +from django.core.checks import Error, Tags, Warning, register @register(Tags.models) @@ -35,14 +36,25 @@ def check_all_models(app_configs=None, **kwargs): indexes[model_index.name].append(model._meta.label) for model_constraint in model._meta.constraints: constraints[model_constraint.name].append(model._meta.label) + if settings.DATABASE_ROUTERS: + error_class, error_id = Warning, 'models.W035' + error_hint = ( + 'You have configured settings.DATABASE_ROUTERS. Verify that %s ' + 'are correctly routed to separate databases.' + ) + else: + error_class, error_id = Error, 'models.E028' + error_hint = None for db_table, model_labels in db_table_models.items(): if len(model_labels) != 1: + model_labels_str = ', '.join(model_labels) errors.append( - Error( + error_class( "db_table '%s' is used by multiple models: %s." - % (db_table, ', '.join(db_table_models[db_table])), + % (db_table, model_labels_str), obj=db_table, - id='models.E028', + hint=(error_hint % model_labels_str) if error_hint else None, + id=error_id, ) ) for index_name, model_labels in indexes.items(): diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index 99f4e1d316c..515d75d91aa 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -317,6 +317,8 @@ Models or a number. * **models.E034**: The index name ```` cannot be longer than ```` characters. +* **models.W035**: ``db_table`` ```` is used by multiple models: + ````. Security -------- diff --git a/docs/releases/2.2.5.txt b/docs/releases/2.2.5.txt index 178588327ee..718ec8d8888 100644 --- a/docs/releases/2.2.5.txt +++ b/docs/releases/2.2.5.txt @@ -9,4 +9,6 @@ Django 2.2.5 fixes several bugs in 2.2.4. Bugfixes ======== -* ... +* Relaxed the system check added in Django 2.2 for models to reallow use of the + same ``db_table`` by multiple models when database routers are installed + (:ticket:`30673`). diff --git a/tests/check_framework/test_model_checks.py b/tests/check_framework/test_model_checks.py index 79177e38f76..02c36dc6109 100644 --- a/tests/check_framework/test_model_checks.py +++ b/tests/check_framework/test_model_checks.py @@ -1,12 +1,16 @@ from django.core import checks -from django.core.checks import Error +from django.core.checks import Error, Warning from django.db import models from django.test import SimpleTestCase, TestCase, skipUnlessDBFeature from django.test.utils import ( - isolate_apps, modify_settings, override_system_checks, + isolate_apps, modify_settings, override_settings, override_system_checks, ) +class EmptyRouter: + pass + + @isolate_apps('check_framework', attr_name='apps') @override_system_checks([checks.model_checks.check_all_models]) class DuplicateDBTableTests(SimpleTestCase): @@ -28,6 +32,30 @@ class DuplicateDBTableTests(SimpleTestCase): ) ]) + @override_settings(DATABASE_ROUTERS=['check_framework.test_model_checks.EmptyRouter']) + def test_collision_in_same_app_database_routers_installed(self): + class Model1(models.Model): + class Meta: + db_table = 'test_table' + + class Model2(models.Model): + class Meta: + db_table = 'test_table' + + self.assertEqual(checks.run_checks(app_configs=self.apps.get_app_configs()), [ + Warning( + "db_table 'test_table' is used by multiple models: " + "check_framework.Model1, check_framework.Model2.", + hint=( + 'You have configured settings.DATABASE_ROUTERS. Verify ' + 'that check_framework.Model1, check_framework.Model2 are ' + 'correctly routed to separate databases.' + ), + obj='test_table', + id='models.W035', + ) + ]) + @modify_settings(INSTALLED_APPS={'append': 'basic'}) @isolate_apps('basic', 'check_framework', kwarg_name='apps') def test_collision_across_apps(self, apps): @@ -50,6 +78,34 @@ class DuplicateDBTableTests(SimpleTestCase): ) ]) + @modify_settings(INSTALLED_APPS={'append': 'basic'}) + @override_settings(DATABASE_ROUTERS=['check_framework.test_model_checks.EmptyRouter']) + @isolate_apps('basic', 'check_framework', kwarg_name='apps') + def test_collision_across_apps_database_routers_installed(self, apps): + class Model1(models.Model): + class Meta: + app_label = 'basic' + db_table = 'test_table' + + class Model2(models.Model): + class Meta: + app_label = 'check_framework' + db_table = 'test_table' + + self.assertEqual(checks.run_checks(app_configs=apps.get_app_configs()), [ + Warning( + "db_table 'test_table' is used by multiple models: " + "basic.Model1, check_framework.Model2.", + hint=( + 'You have configured settings.DATABASE_ROUTERS. Verify ' + 'that basic.Model1, check_framework.Model2 are correctly ' + 'routed to separate databases.' + ), + obj='test_table', + id='models.W035', + ) + ]) + def test_no_collision_for_unmanaged_models(self): class Unmanaged(models.Model): class Meta: