Make django-simple-history compatible with Django 1.7 #82

Merged
merged 17 commits into from Apr 15, 2014

3 participants

@treyhunner
Owner

Exception:

RuntimeError: App registry isn't ready yet.

Failing Travis build:

https://travis-ci.org/treyhunner/django-simple-history/jobs/20010399

@treyhunner
Owner

There are still multiple lines in HistoricalRecords.create_history_model that are throwing the "App registry isn't ready yet" error. After those lines are resolved the problem should be solved.

@treyhunner treyhunner commented on an outdated diff Mar 14, 2014
simple_history/models.py
@@ -92,7 +92,7 @@ def create_history_model(self, model):
attrs['__module__'] = self.module
elif app_module != self.module:
# has meta options with app_label
- app = models.get_app(model._meta.app_label)
+ app = models.get_app(model._meta.app_label) # FIXME This is broken in 1.7
@treyhunner
Owner

This line breaks in Django 1.7.

@dnozay I don't think I understand the magic in this code section well enough to fix it. Do you have any interest in taking a stab at digging into how we might get around this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@treyhunner
Owner

My current solution to the Django 1.7 app config problem is to remove some currently supported features for 1.7. The verbose_name for a historical model won't always be correct and the app used in the table name won't always be correct.

The current regressions don't affect me, but they may affect others

I believe the possible regressions are related to #34 and #62. CC-ing @dnozay and @foobacca in case these regressions might be problematic.

@treyhunner
Owner

#88 may help identify other bugs that appear when upgrading to Django 1.7.

@treyhunner
Owner

Broken in 1.7:

  • On class_prepared, models.get_app cannot be used before the app registry is ready (we're using app.__name__ to set the __module__ of the model
  • model._meta.verbose_name doesn't always work before the app registry is ready (sometimes it raises an exception)

These situations both cause an App registry isn't ready error.

@aaugustin

I haven't looked at the code yet, but given the kind of errors you're seeing, I assume it generates dynamic models?

@bjdixon

Not sure if I'm looking at the right branch for this or not. I added DiscoverRunner for running the tests against 1.7 in this commit. This gets me a bit further at least with a couple fails and attribute errors. Let me know if I'm going down the wrong path before I continue please.

@treyhunner
Owner

@aaugustin yes a "historical" model is generated for each model that django-simple-history is applied to.

@treyhunner
Owner

@bjdixon It's Django trunk (Django 1.8) that requires DiscoverRunner along with other changes.

I just pushed some changes that add Django 1.7-specific tests and fix some bugs due to Django trunk changes. Don't worry about the bugs due to Django trunk.

Thanks to both of you for looking into this. I should note that I haven't yet stepped through the app framework code with pdb.

@aaugustin

@treyhunner I just ran the tests successfully on Django 1.7. Do you still need my help? If you do, can you tell me how to observe a failure?

@treyhunner
Owner

@aaugustin I had regressed features in order to make the app not completely fail on 1.7.

I just reverted the incorrect workarounds I implemented for 1.7 so you can see the failures.

@aaugustin

Here's a diff with two fixes:

1) abuse the app registry's internals -- nothing to worry about if you're generate dynamic models ;-)
2) avoid improper resolution of lazy translations at import time -- this is a silent bug in previous Django versions that Django 1.7 catches.

Regarding 1), a better long-term strategy might be to create the history models in AppConfig.ready().

diff --git a/simple_history/models.py b/simple_history/models.py
index e041b32..9fe069c 100644
--- a/simple_history/models.py
+++ b/simple_history/models.py
@@ -2,6 +2,10 @@ from __future__ import unicode_literals

 import copy
 from django import VERSION as django_version
+try:
+    from django.apps import apps    # Django >= 1.7
+except ImportError:
+    apps = None
 from django.db import models
 from django.db.models.fields.related import RelatedField
 from django.conf import settings
@@ -11,6 +15,7 @@ try:
     from django.utils.six import text_type
 except ImportError:
     text_type = unicode
+from django.utils.translation import string_concat
 from .manager import HistoryDescriptor

 try:
@@ -92,9 +97,14 @@ class HistoricalRecords(object):
             # registered under different app
             attrs['__module__'] = self.module
         elif app_module != self.module:
-            # has meta options with app_label
-            app = models.get_app(model._meta.app_label)  # FIXME This is broken in 1.7
-            attrs['__module__'] = app.__name__  # full dotted name
+            if apps is None:
+                # has meta options with app_label
+                app = models.get_app(model._meta.app_label)
+                attrs['__module__'] = app.__name__  # full dotted name
+            else:
+                # Abuse an internal API because the app registry is loading.
+                app = apps.app_configs[model._meta.app_label]
+                attrs['__module__'] = app.name      # full dotted name

         fields = self.copy_fields(model)
         attrs.update(fields)
@@ -173,7 +183,7 @@ class HistoricalRecords(object):
         if self.user_set_verbose_name:
             name = self.user_set_verbose_name
         else:
-            name = 'historical ' + text_type(model._meta.verbose_name)
+            name = string_concat('historical ', model._meta.verbose_name)
         meta_fields['verbose_name'] = name
         return meta_fields
@aaugustin

Tests pass at least on Python 3.3 + Django 1.7. I hope this helps. Let me know if that's insufficient or if you run into more issues.

@treyhunner
Owner

Wow thanks for the fast fixes @aaugustin!

1) That abuse is minor compared to much of the rest of our code.
2) The use ofstring_concat looks much more appropriate. I was ignorant of the various lazy string functions.

In regards to using AppConfig.ready(), that does sound most appropriate. I believe I would still need to abuse that internal API to fetch the AppConfig reference and then monkey patch ready (unless there's an app-prepared/ready signal I'm missing).

@aaugustin

My suggestion would be to implement an AppConfig subclass for django-simple-history. In its ready() method, it would create all the historical models. Since ready() runs after the app registry is fully populated, it can easily iterate over all apps and models.

@treyhunner
Owner

I hadn't realized that ready() was always called after the app registry was fully populated.

That would be a much cleaner implementation. This new AppConfig framework is pretty sweet (minor hiccups withstanding).

@aaugustin

Reading this again, I realize I should have put smart_text(model._meta.verbose_name). As it stands you may encounter a UnicodeDecodeError on Python 2 if model._meta.verbose_name is a non-ASCII bytes object, lazily-translated or not. That would be a regression. smart_text lives in django.utils.encoding.

Sorry for the oversight.

Thanks. I'll implement a test for that and get that changed.

@aaugustin I realized we actually already have a test for this that passes. Do you know of a unicode verbose_name that might fail without smart_text?

The problem I'm referring to can only happen on Python 2 if verbose_name is a bytestring that contains non-ASCII characters. This isn't common; verbose_name is usually a unicode string.

@treyhunner treyhunner merged commit 1e4e4bb into master Apr 15, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@macro1 macro1 deleted the django-1.7 branch Oct 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment