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

feat(StateLog): Migrated state log from integerfield to charfield #97

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
30 changes: 13 additions & 17 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,33 @@ language: python

matrix:
include:
- env: TOXENV=py27-dj-1.8
python: 2.7
- env: TOXENV=py27-dj-1.11
python: 2.7
- env: TOXENV=py34-dj-1.8
python: 3.4
- env: TOXENV=py34-dj-1.11
python: 3.4
- env: TOXENV=py35-dj-1.8
python: 3.5
- env: TOXENV=py35-dj-1.11
python: 3.5
- env: TOXENV=py36-dj-1.8
python: 3.6
- env: TOXENV=py36-dj-1.11
python: 3.6
- env: TOXENV=py34-dj-2.0
python: 3.4
- env: TOXENV=py37-dj-1.11
python: 3.7
- env: TOXENV=py35-dj-2.0
python: 3.5
- env: TOXENV=py36-dj-2.0
python: 3.6
- env: TOXENV=py36-dj-2.1
python: 3.6
- env: TOXENV=py36-dj-master
- env: TOXENV=py36-dj-2.2
python: 3.6
- env: TOXENV=py37-dj-3.0
python: 3.7
- env: TOXENV=py38-dj-3.0
python: 3.8
- env: TOXENV=py37-dj-master
python: 3.7
- env: TOXENV=py38-dj-master
python: 3.8
- env: TOXENV=flake8
python: 3.6
python: 3.8
allow_failures:
- env: TOXENV=py36-dj-master
python: 3.6
python: 3.8

install:
- pip install tox codecov
Expand Down
1 change: 1 addition & 0 deletions django_fsm_log/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
default_app_config = 'django_fsm_log.apps.DjangoFSMLogAppConfig'
from .checks import * # noqa
72 changes: 72 additions & 0 deletions django_fsm_log/checks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import ast
import inspect

import django
from django.core.checks import (
Warning,
register,
Tags,
)
from django.core.exceptions import FieldDoesNotExist


@register(Tags.compatibility)
def integer_object_id_check(app_configs, **kwargs):
errors = []
for app in django.apps.apps.get_app_configs():

# Skip third party apps.
if app.path.find('site-packages') > -1:
continue

for model in app.get_models():
for check_message in check_model_for_integer_object_id(model):
errors.append(check_message)
return errors


def check_model_for_integer_object_id(model):
"""Check a single model.

Yields (django.checks.CheckMessage)
"""

from django_fsm_log.models import StateLog

model_source = inspect.getsource(model)
model_node = ast.parse(model_source)

for node in model_node.body[0].body:

# Check if node is a model field.
if not isinstance(node, ast.Assign):
continue

if len(node.targets) != 1:
continue

if not isinstance(node.targets[0], ast.Name):
continue

field_name = node.targets[0].id
try:
field = model._meta.get_field(field_name)
except FieldDoesNotExist:
continue

# Node is a model field here

# Check if field has foreign key to StateLog defined
for kw in node.value.keywords:
if kw.arg == 'to':
to = kw
if to == 'django_fsm_log.StateLog' or isinstance(to, StateLog):
yield Warning(
'StateLog has changed its object_id from PositiveIntegerField to TextField to better support '
'the primary key types in Django.',
hint='Migration PositiveIntegerField -> TextField is potentially needed on field {}.'.format(
field.name
),
obj=field,
id='django_fsm_log.W001',
)

Choose a reason for hiding this comment

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

Is this warning absolutely necessary? I'm assuming it relates to https://code.djangoproject.com/ticket/25012 or similar?

Copy link
Author

Choose a reason for hiding this comment

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

Basically yeah, one of the only solid bits of contention in the issue where this was discussed was surrounding the use of the field by user based code which implicitly expected an integer.
I am also aware that I've been tripped up by types of foreign key references changing in the past in my own code and it's within the scope of the check framework to automate looking for these kinds of errors and warning users about them.
We can also nerf it again at some point in the futurewhen we feel we've had enough of a deprecation period. I just wish there was a way I could also check to see if any python code references these fields.

2 changes: 1 addition & 1 deletion django_fsm_log/conf.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.conf import settings # noqa:F811
from django.conf import settings # noqa
from appconf import AppConf


Expand Down
18 changes: 18 additions & 0 deletions django_fsm_log/migrations/0006_allow_non_numeric_ids.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 2.1.3 on 2018-11-16 12:57

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('django_fsm_log', '0005_description_null'),
]

operations = [
migrations.AlterField(
model_name='statelog',
name='object_id',
field=models.TextField(db_index=True),
),
]
8 changes: 2 additions & 6 deletions django_fsm_log/models.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
# -*- coding:utf-8 -*-
from __future__ import unicode_literals

from django.contrib.contenttypes.fields import GenericForeignKey
from django.contrib.contenttypes.models import ContentType
from django.db import models
from django.utils.encoding import force_text, python_2_unicode_compatible
from django.utils.encoding import force_text
from django.utils.timezone import now
from django_fsm import FSMFieldMixin, FSMIntegerField

from .conf import settings
from .managers import StateLogManager


@python_2_unicode_compatible
class StateLog(models.Model):
timestamp = models.DateTimeField(default=now)
by = models.ForeignKey(getattr(settings, 'AUTH_USER_MODEL', 'auth.User'), blank=True,
Expand All @@ -23,7 +19,7 @@ class StateLog(models.Model):
transition = models.CharField(max_length=255)

content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE)
object_id = models.PositiveIntegerField(db_index=True)
object_id = models.TextField(db_index=True)
content_object = GenericForeignKey('content_type', 'object_id')

description = models.TextField(blank=True, null=True)
Expand Down
12 changes: 7 additions & 5 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,23 +1,25 @@
[tox]
envlist =
py{27,34,35,36}-dj-{1.8,1.11}
py{34,35,36}-dj-2.0
py{35,36}-dj-master
py{35,36,37}-dj-{1.11}
py{35,36,37}-dj-{2.0,2.1,2.2}
py{37,38}-dj-{3.0}
py{37,38}-dj-{master}
flake8

[testenv]
usedevelop = true
commands = pytest --cov=django_fsm_log --cov=tests {posargs}
extras = testing
deps =
dj-1.8: Django>=1.8,<1.9
dj-1.11: Django>=1.11,<2.0
dj-2.0: Django>=2.0,<2.1
dj-2.1: Django>=2.1,<2.2
dj-2.2: Django>=2.2,<2.3
dj-3.0: Django>=3.0,<4.0
dj-master: https://github.com/django/django/archive/master.tar.gz

[testenv:flake8]
basepython = python3.6
basepython = python3.8
commands = flake8 django_fsm_log tests
extras =
deps =
Expand Down