Skip to content

Commit

Permalink
Fixed #13711 -- Model check added to ensure that auto-generated colum…
Browse files Browse the repository at this point in the history
…n name is within limits of the database.

Thanks russellm for report and Tim Graham for review.
  • Loading branch information
coder9042 authored and timgraham committed Jun 17, 2014
1 parent e470838 commit 91f1b6d
Show file tree
Hide file tree
Showing 8 changed files with 254 additions and 24 deletions.
3 changes: 0 additions & 3 deletions django/db/backends/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion django/db/backends/mysql/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
73 changes: 72 additions & 1 deletion django/db/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
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
Expand Down Expand Up @@ -1068,6 +1068,7 @@ def check(cls, **kwargs):
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
Expand Down Expand Up @@ -1451,6 +1452,76 @@ def _check_ordering(cls):
)
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) #
Expand Down
5 changes: 5 additions & 0 deletions docs/ref/checks.txt
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ Models
* **models.E016**: ``index_together/unique_together`` refers to field
``<field_name>`` which is not local to model ``<model>``.
* **models.E017**: Proxy model ``<model>`` contains model fields.
* **models.E018**: Autogenerated column name too long for field ``<field>``.
Maximum length is ``<maximum length>`` for database ``<alias>``.
* **models.E019**: Autogenerated column name too long for M2M field
``<M2M field>``. Maximum length is ``<maximum length>`` for database
``<alias>``.

Fields
~~~~~~
Expand Down
22 changes: 22 additions & 0 deletions docs/releases/1.8.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
~~~~~~~~~~~~~

Expand Down
22 changes: 10 additions & 12 deletions tests/backends/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
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


Expand All @@ -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):
Expand Down
9 changes: 3 additions & 6 deletions tests/backends/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
143 changes: 142 additions & 1 deletion tests/invalid_models_tests/test_models.py
Original file line number Diff line number Diff line change
@@ -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):
Expand Down Expand Up @@ -252,6 +275,124 @@ class Model(models.Model):
]
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()
Expand Down

0 comments on commit 91f1b6d

Please sign in to comment.