From 91f1b6dcdc5da47d7794a55e3114820407a5bd62 Mon Sep 17 00:00:00 2001 From: Anubhav Joshi Date: Tue, 10 Jun 2014 17:34:19 +0530 Subject: [PATCH] Fixed #13711 -- Model check added to ensure that auto-generated column name is within limits of the database. Thanks russellm for report and Tim Graham for review. --- django/db/backends/__init__.py | 3 - django/db/backends/mysql/base.py | 1 - django/db/models/base.py | 73 ++++++++++- docs/ref/checks.txt | 5 + docs/releases/1.8.txt | 22 ++++ tests/backends/models.py | 22 ++-- tests/backends/tests.py | 9 +- tests/invalid_models_tests/test_models.py | 143 +++++++++++++++++++++- 8 files changed, 254 insertions(+), 24 deletions(-) diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py index 869644bfacc..8acfde014c3 100644 --- a/django/db/backends/__init__.py +++ b/django/db/backends/__init__.py @@ -520,9 +520,6 @@ class BaseDatabaseFeatures(object): # at the end of each save operation? supports_forward_references = True - # Does the backend allow very long model names without error? - supports_long_model_names = True - # Is there a REAL datatype in addition to floats/doubles? has_real_datatype = False supports_subqueries_in_group_by = True diff --git a/django/db/backends/mysql/base.py b/django/db/backends/mysql/base.py index 697a35a1ddb..77369dcd8ba 100644 --- a/django/db/backends/mysql/base.py +++ b/django/db/backends/mysql/base.py @@ -171,7 +171,6 @@ class DatabaseFeatures(BaseDatabaseFeatures): has_select_for_update = True has_select_for_update_nowait = False supports_forward_references = False - supports_long_model_names = False # XXX MySQL DB-API drivers currently fail on binary data on Python 3. supports_binary_field = six.PY2 supports_microsecond_precision = False diff --git a/django/db/models/base.py b/django/db/models/base.py index 5142f934622..74bece1f5cd 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -12,7 +12,7 @@ from django.conf import settings from django.core import checks from django.core.exceptions import (ObjectDoesNotExist, MultipleObjectsReturned, FieldError, ValidationError, NON_FIELD_ERRORS) -from django.db import (router, transaction, DatabaseError, +from django.db import (router, connections, transaction, DatabaseError, DEFAULT_DB_ALIAS, DJANGO_VERSION_PICKLE_KEY) from django.db.models.deletion import Collector from django.db.models.fields import AutoField, FieldDoesNotExist @@ -1068,6 +1068,7 @@ class Model(six.with_metaclass(ModelBase)): if not cls._meta.swapped: errors.extend(cls._check_fields(**kwargs)) errors.extend(cls._check_m2m_through_same_relationship()) + errors.extend(cls._check_long_column_names()) clash_errors = cls._check_id_field() + cls._check_field_name_clashes() errors.extend(clash_errors) # If there are field name clashes, hide consequent column name @@ -1451,6 +1452,76 @@ class Model(six.with_metaclass(ModelBase)): ) return errors + @classmethod + def _check_long_column_names(cls): + """ + Check that any auto-generated column names are shorter than the limits + for each database in which the model will be created. + """ + errors = [] + allowed_len = None + db_alias = None + + # Find the minimum max allowed length among all specified db_aliases. + for db in settings.DATABASES.keys(): + # skip databases where the model won't be created + if not router.allow_migrate(db, cls): + continue + connection = connections[db] + max_name_length = connection.ops.max_name_length() + if max_name_length is None: + continue + else: + if allowed_len is None: + allowed_len = max_name_length + db_alias = db + elif max_name_length < allowed_len: + allowed_len = max_name_length + db_alias = db + + if allowed_len is None: + return errors + + for f in cls._meta.local_fields: + _, column_name = f.get_attname_column() + + # Check if auto-generated name for the field is too long + # for the database. + if (f.db_column is None and column_name is not None + and len(column_name) > allowed_len): + errors.append( + checks.Error( + 'Autogenerated column name too long for field "%s". ' + 'Maximum length is "%s" for database "%s".' + % (column_name, allowed_len, db_alias), + hint="Set the column name manually using 'db_column'.", + obj=cls, + id='models.E018', + ) + ) + + for f in cls._meta.local_many_to_many: + # Check if auto-generated name for the M2M field is too long + # for the database. + for m2m in f.rel.through._meta.local_fields: + _, rel_name = m2m.get_attname_column() + if (m2m.db_column is None and rel_name is not None + and len(rel_name) > allowed_len): + errors.append( + checks.Error( + 'Autogenerated column name too long for M2M field ' + '"%s". Maximum length is "%s" for database "%s".' + % (rel_name, allowed_len, db_alias), + hint=("Use 'through' to create a separate model " + "for M2M and then set column_name using " + "'db_column'."), + obj=cls, + id='models.E019', + ) + ) + + return errors + ############################################ # HELPER FUNCTIONS (CURRIED MODEL METHODS) # diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index aedaa921dbc..85cd6162534 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -58,6 +58,11 @@ Models * **models.E016**: ``index_together/unique_together`` refers to field ```` which is not local to model ````. * **models.E017**: Proxy model ```` contains model fields. +* **models.E018**: Autogenerated column name too long for field ````. + Maximum length is ```` for database ````. +* **models.E019**: Autogenerated column name too long for M2M field + ````. Maximum length is ```` for database + ````. Fields ~~~~~~ diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index c54e1d5ab0d..7f3b4aa6c7b 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -305,6 +305,28 @@ Now to implement the same behavior, you have to create an ``parser.add_argument`` to add any custom arguments, as parser is now an :py:class:`argparse.ArgumentParser` instance. +Model check ensures auto-generated column names are within limits specified by database +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +A field name that's longer than the column name length supported by a database +can create problems. For example, with MySQL you'll get an exception trying to +create the column, and with PostgreSQL the column name is truncated by the +database (you may see a warning in the PostgreSQL logs). + +A model check has been introduced to better alert users to this scenario before +the actual creation of database tables. + +If you have an existing model where this check seems to be a false positive, +for example on PostgreSQL where the name was already being truncated, simply +use :attr:`~django.db.models.Field.db_column` to specify the name that's being +used. + +The check also applies to the columns generated in an implicit +``ManyToManyField.through`` model. If you run into an issue there, use +:attr:`~django.db.models.ManyToManyField.through` to create an explicit model +and then specify :attr:`~django.db.models.Field.db_column` on its column(s) +as needed. + Miscellaneous ~~~~~~~~~~~~~ diff --git a/tests/backends/models.py b/tests/backends/models.py index 2a94c249cad..b550212d0b6 100644 --- a/tests/backends/models.py +++ b/tests/backends/models.py @@ -4,7 +4,7 @@ from django.contrib.contenttypes.fields import ( GenericForeignKey, GenericRelation ) from django.contrib.contenttypes.models import ContentType -from django.db import models, connection +from django.db import models from django.utils.encoding import python_2_unicode_compatible @@ -31,17 +31,15 @@ class SchoolClass(models.Model): day = models.CharField(max_length=9, blank=True) last_updated = models.DateTimeField() -# Unfortunately, the following model breaks MySQL hard. -# Until #13711 is fixed, this test can't be run under MySQL. -if connection.features.supports_long_model_names: - class VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ(models.Model): - class Meta: - # We need to use a short actual table name or - # we hit issue #8548 which we're not testing! - verbose_name = 'model_with_long_table_name' - primary_key_is_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = models.AutoField(primary_key=True) - charfield_is_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = models.CharField(max_length=100) - m2m_also_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = models.ManyToManyField(Person, blank=True) + +class VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ(models.Model): + class Meta: + # We need to use a short actual table name or + # we hit issue #8548 which we're not testing! + verbose_name = 'model_with_long_table_name' + primary_key_is_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = models.AutoField(primary_key=True) + charfield_is_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = models.CharField(max_length=100) + m2m_also_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz = models.ManyToManyField(Person, blank=True) class Tag(models.Model): diff --git a/tests/backends/tests.py b/tests/backends/tests.py index 9742358a59f..40b0b550825 100644 --- a/tests/backends/tests.py +++ b/tests/backends/tests.py @@ -379,26 +379,23 @@ class LongNameTest(TestCase): check it is. Refs #8901. """ - @skipUnlessDBFeature('supports_long_model_names') def test_sequence_name_length_limits_create(self): """Test creation of model with long name and long pk name doesn't error. Ref #8901""" - models.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ.objects.create() + models.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ.objects.create() - @skipUnlessDBFeature('supports_long_model_names') def test_sequence_name_length_limits_m2m(self): """Test an m2m save of a model with a long name and a long m2m field name doesn't error as on Django >=1.2 this now uses object saves. Ref #8901""" - obj = models.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ.objects.create() + obj = models.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ.objects.create() rel_obj = models.Person.objects.create(first_name='Django', last_name='Reinhardt') obj.m2m_also_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz.add(rel_obj) - @skipUnlessDBFeature('supports_long_model_names') def test_sequence_name_length_limits_flush(self): """Test that sequence resetting as part of a flush with model with long name and long pk name doesn't error. Ref #8901""" # A full flush is expensive to the full test, so we dig into the # internals to generate the likely offending SQL and run it manually # Some convenience aliases - VLM = models.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ + VLM = models.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ VLM_m2m = VLM.m2m_also_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz.through tables = [ VLM._meta.db_table, diff --git a/tests/invalid_models_tests/test_models.py b/tests/invalid_models_tests/test_models.py index 9cbaa8449c2..8550dc30652 100644 --- a/tests/invalid_models_tests/test_models.py +++ b/tests/invalid_models_tests/test_models.py @@ -1,13 +1,36 @@ # -*- encoding: utf-8 -*- from __future__ import unicode_literals +import unittest + +from django.conf import settings from django.core.checks import Error -from django.db import models +from django.db import models, connections from django.test.utils import override_settings from .base import IsolatedModelsTestCase +def get_max_column_name_length(): + allowed_len = None + db_alias = None + + for db in settings.DATABASES.keys(): + connection = connections[db] + max_name_length = connection.ops.max_name_length() + if max_name_length is None: + continue + else: + if allowed_len is None: + allowed_len = max_name_length + db_alias = db + elif max_name_length < allowed_len: + allowed_len = max_name_length + db_alias = db + + return (allowed_len, db_alias) + + class IndexTogetherTests(IsolatedModelsTestCase): def test_non_iterable(self): @@ -252,6 +275,124 @@ class FieldNamesTests(IsolatedModelsTestCase): ] self.assertEqual(errors, expected) + max_column_name_length, column_limit_db_alias = get_max_column_name_length() + + @unittest.skipIf(max_column_name_length is None, + "The database doesn't have a column name length limit.") + def test_M2M_long_column_name(self): + """ + #13711 -- Model check for long M2M column names when database has + column name length limits. + """ + allowed_len, db_alias = get_max_column_name_length() + + # A model with very long name which will be used to set relations to. + class VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz(models.Model): + title = models.CharField(max_length=11) + + # Main model for which checks will be performed. + class ModelWithLongField(models.Model): + m2m_field = models.ManyToManyField( + VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, + related_name="rn1" + ) + m2m_field2 = models.ManyToManyField( + VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, + related_name="rn2", through='m2msimple' + ) + m2m_field3 = models.ManyToManyField( + VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, + related_name="rn3", + through='m2mcomplex' + ) + fk = models.ForeignKey(VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, related_name="rn4") + + # Models used for setting `through` in M2M field. + class m2msimple(models.Model): + id2 = models.ForeignKey(ModelWithLongField) + + class m2mcomplex(models.Model): + id2 = models.ForeignKey(ModelWithLongField) + + long_field_name = 'a' * (self.max_column_name_length + 1) + models.ForeignKey( + VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz + ).contribute_to_class(m2msimple, long_field_name) + + models.ForeignKey( + VeryLongModelNamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, + db_column=long_field_name + ).contribute_to_class(m2mcomplex, long_field_name) + + errors = ModelWithLongField.check() + + # First error because of M2M field set on the model with long name. + m2m_long_name = "verylongmodelnamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz_id" + expected = [ + Error( + ('Autogenerated column name too long for M2M field "%s". ' + 'Maximum length is "%s" for database "%s".' + % (m2m_long_name, self.max_column_name_length, self.column_limit_db_alias)), + hint=("Use 'through' to create a separate model for " + "M2M and then set column_name using 'db_column'."), + obj=ModelWithLongField, + id='models.E019', + ) + ] + + # Second error because the FK specified in the `through` model + # `m2msimple` has auto-genererated name longer than allowed. + # There will be no check errors in the other M2M because it + # specifies db_column for the FK in `through` model even if the actual + # name is longer than the limits of the database. + expected.append( + Error( + ('Autogenerated column name too long for M2M field "%s_id". ' + 'Maximum length is "%s" for database "%s".' + % (long_field_name, self.max_column_name_length, self.column_limit_db_alias)), + hint=("Use 'through' to create a separate model for " + "M2M and then set column_name using 'db_column'."), + obj=ModelWithLongField, + id='models.E019', + ) + ) + + self.assertEqual(errors, expected) + + @unittest.skipIf(max_column_name_length is None, + "The database doesn't have a column name length limit.") + def test_local_field_long_column_name(self): + """ + #13711 -- Model check for long column names + when database does not support long names. + """ + allowed_len, db_alias = get_max_column_name_length() + + class ModelWithLongField(models.Model): + title = models.CharField(max_length=11) + + long_field_name = 'a' * (self.max_column_name_length + 1) + long_field_name2 = 'b' * (self.max_column_name_length + 1) + models.CharField(max_length=11).contribute_to_class(ModelWithLongField, long_field_name) + models.CharField(max_length=11, db_column='vlmn').contribute_to_class(ModelWithLongField, long_field_name2) + + errors = ModelWithLongField.check() + + # Error because of the field with long name added to the model + # without specifying db_column + expected = [ + Error( + ('Autogenerated column name too long for field "%s". ' + 'Maximum length is "%s" for database "%s".' + % (long_field_name, self.max_column_name_length, self.column_limit_db_alias)), + hint="Set the column name manually using 'db_column'.", + obj=ModelWithLongField, + id='models.E018', + ) + ] + + self.assertEqual(errors, expected) + def test_including_separator(self): class Model(models.Model): some__field = models.IntegerField()