Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix max_length issue for mysql #202

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,17 @@ pip command::

$ pip install https://github.com/celery/django-celery-beat/zipball/master#egg=django-celery-beat

Issues with mysql
-----------------
If you want to run ``django-celery-beat`` with MySQL, you might run into some issues.

One such issue is when you try to run ``python manage.py migrate django_celery_beat``, you might get the following error::
django.db.utils.OperationalError: (1071, 'Specified key was too long; max key length is 767 bytes')
To get around this issue, you can set::
DJANGO_CELERY_BEAT_NAME_MAX_LENGTH=191
(or any other value if any other db other than MySQL is causing similar issues.)
max_length of **191** seems to work for MySQL.


TZ Awareness:
-------------
Expand Down
12 changes: 10 additions & 2 deletions django_celery_beat/migrations/0001_initial.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from django.db import migrations, models
import django.db.models.deletion
from django.conf import settings


class Migration(migrations.Migration):
Expand Down Expand Up @@ -71,8 +72,15 @@ class Migration(migrations.Migration):
auto_created=True, primary_key=True,
serialize=False, verbose_name='ID')),
('name', models.CharField(
help_text='Useful description', max_length=200,
unique=True, verbose_name='name')),
help_text='Useful description',
max_length=getattr(
settings,
'DJANGO_CELERY_BEAT_NAME_MAX_LENGTH',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa... you can't base migrations off of a settings option that people can change, that makes things completely unpredictable. Just choose a more correct value, one that won't ever have the issue again.

Copy link

@tuky tuky May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa...

Can you elaborate on your surprise?

  1. The code is not "unpredictable", the defaults are backwards compatible
  2. Projects with even more stars follow this pattern (https://github.com/python-social-auth/social-app-django/blob/master/social_django/migrations/0001_initial.py)
  3. Simply changing this to 191 would solve it for me, but everyone using this with e.g. postgresql would be facing a DB schema which differs from their migrations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liquidpele can't really choose "a more correct value" in this case since some values work for some databases and the same values cause errors in other databases.

As @tuky points out driving the max_length value from the settings is not a new pattern and its pretty well-established.

If there is another approach you have in mind, please share.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa... you can't base migrations off of a settings option that people can change, that makes things completely unpredictable. Just choose a more correct value, one that won't ever have the issue again.

IMHO this is allowed in some case... but we need to handle this with care. let us not rush on merging this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that some values work and some don't, it's simply a limitation in mysql, one that is already fixed in newer versions where they increased the prefix limit for innodb tables.

One question I have, and @auvipy may know this... is the task_id ever NOT a uuid? I thought it was always a uuid, but a length of 255 there makes me think it could be other things too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do somehow agree with you. Need some analysis to answer your question properly

200
),
unique=True,
verbose_name='name'
)),
('task', models.CharField(
max_length=200, verbose_name='task name')),
('args', models.TextField(
Expand Down
7 changes: 6 additions & 1 deletion django_celery_beat/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,12 @@ class PeriodicTask(models.Model):
"""Model representing a periodic task."""

name = models.CharField(
max_length=200, unique=True,
max_length=getattr(
settings,
'DJANGO_CELERY_BEAT_NAME_MAX_LENGTH',
200
),
unique=True,
verbose_name=_('Name'),
help_text=_('Short Description For This Task'),
)
Expand Down
44 changes: 41 additions & 3 deletions t/unit/test_models.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,52 @@
from __future__ import absolute_import, unicode_literals
import importlib
import os

from django.test import TestCase
from django.apps import apps
from django.db.migrations.state import ProjectState
from django.db.migrations.autodetector import MigrationAutodetector
from django.db.migrations.loader import MigrationLoader
from django.db.migrations.questioner import NonInteractiveMigrationQuestioner
from django.db.migrations.state import ProjectState
from django.test import TestCase, override_settings
from django.utils import six

from django_celery_beat import migrations as beat_migrations, models


class ModelMigrationTests(TestCase):
def test_periodic_task_name_max_length_defaults_to_200_in_model(self):
six.moves.reload_module(models)
self.assertEqual(
200, models.PeriodicTask._meta.get_field('name').max_length)

@override_settings(DJANGO_CELERY_BEAT_NAME_MAX_LENGTH=191)
def test_periodic_task_name_max_length_changed_to_191_in_model(self):
six.moves.reload_module(models)
self.assertEqual(
191, models.PeriodicTask._meta.get_field('name').max_length)

def test_periodic_task_name_max_length_defaults_to_200_in_migration(self):
initial_migration_module = importlib.import_module(
'django_celery_beat.migrations.0001_initial')
six.moves.reload_module(initial_migration_module)
initial_migration = initial_migration_module.Migration
periodic_task_creation = initial_migration.operations[2]
fields = dict(periodic_task_creation.fields)

self.assertEqual('PeriodicTask', periodic_task_creation.name)
self.assertEqual(200, fields['name'].max_length)

@override_settings(DJANGO_CELERY_BEAT_NAME_MAX_LENGTH=191)
def test_periodic_task_name_max_length_changed_to_191_in_migration(self):
initial_migration_module = importlib.import_module(
'django_celery_beat.migrations.0001_initial')
six.moves.reload_module(initial_migration_module)
initial_migration = initial_migration_module.Migration
periodic_task_creation = initial_migration.operations[2]
fields = dict(periodic_task_creation.fields)

from django_celery_beat import migrations as beat_migrations
self.assertEqual('PeriodicTask', periodic_task_creation.name)
self.assertEqual(191, fields['name'].max_length)


class MigrationTests(TestCase):
Expand Down
5 changes: 3 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ sitepackages = False
recreate = False
commands =
pip list
py.test -xv
py.test -xv --ignore=t/unit/test_models.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you ignoring the test you added?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new tests are run with the line below: py.test -xv -k 'ModelMigrationTests'. It is necessary, to run the new tests separately, because they override settings used in models and it is therefore necessary, to do some six.moves.reload_module calls in the tests, which breaks subsequent test runs, that rely on the same models.

py.test -xv -k 'ModelMigrationTests'

[testenv:upgradebeat111]
basepython = python2.7
Expand Down Expand Up @@ -170,4 +171,4 @@ commands =
pip install -U https://github.com/celery/celery/zipball/master#egg=celery
pip install -U https://github.com/celery/kombu/zipball/master#egg=kombu
pip install Django==2.0
py.test -x --cov=django_celery_beat --cov-report=xml --no-cov-on-fail
py.test -x --cov=django_celery_beat --cov-report=xml --no-cov-on-fail --ignore=t/unit/test_models.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to exclude things like this.