agateblue / django-dynamic-preferences

Dynamic global and instance settings for your django project
https://django-dynamic-preferences.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
348 stars 86 forks source link

When a preference is removed from the registry, it crashes when loading from database #41

Closed czlee closed 7 years ago

czlee commented 8 years ago

When you remove a preference from the registry, it currently crashes because it can't find the preference it's trying to restore from the database (because the deleted preference is still in the database).

Here's the traceback when you load a form: http://dpaste.com/2JHWJCN Here's the traceback when you load the admin page: http://dpaste.com/02CZKTB

I'm not sure what the intended behavior is here, or even what it should be. I was inclined to say it should just fail silently, ignoring the preference and leaving it in the database, but that approach has problems of its own. Perhaps it should log a warning or something, or maybe it should delete the preference from the database. It'd probably be nice for it to do something other than crash completely.

This might seem like something that never changes, but projects might like to deprecate preferences from time to time (this was why it happened to me). The onus is of course on the project developer to do something sensible with deprecated options. Any thoughts?

agateblue commented 8 years ago

I remembered writing a management command to handle this case. However, it's not very handy to run it manually. You can probably use it as a temporary fix for this issue.

Just like you, I'm not totally sure what would be the right approach here. Available options that come to my mind (you mentioned some):

  1. Crash (what currently happens)
  2. Fail silently
  3. Fail with a warning log to explain that the issue need to be fixed
  4. Provide a management command to explicitely deprecate preferences, that would handle database deletion, something such as python manage.py delete_obsolete_preferences section1.preference_name section2.preference_name
  5. Fail silently and delete the obsolete preference from database : but that mean if you temporary disable a preference, all related data would potentially be destroyed.

I'd rather go for a combination of 3 and 4, but maybe some other options are available ?

czlee commented 8 years ago

Ah, I hadn't noticed the management command—for what I had it would actually work fine. (I've been just going into psql and deleting the offending entry even more manually.) But I agree it wouldn't help with deprecating preferences. For what it's worth, I'd agree with a combination of 3 and 4, and don't have any better ideas. At worst, if the user never runs the command, the entry just lurks around in the database and keeps raising warnings, which in the grand scheme of things doesn't seem too harmful.

I suppose if you wanted to be safer, apps (in new versions) could specify explicitly which preferences are made obsolete somewhere (similar to migrations but with manual and with less complexity), and the management command would only delete those. I'm not sure that there's much point in that though.

agateblue commented 8 years ago

I suppose if you wanted to be safer, apps (in new versions) could specify explicitly which preferences are made obsolete somewhere (similar to migrations but with manual and with less complexity), and the management command would only delete those. I'm not sure that there's much point in that though.

I'm also concerned about reproducibility : it's easy to run some management commands locally on your development machine, but if you want to automate things, management commands are not really attractive.

Using django's built-in migrations would be a good idea : The problem here is that the database keeps obsolete rows, so removing them via a datamigration looks like the cleanest way.

But, automatically generating migration files from obsolete preferences will probably be really difficult. In fact, it scares me.

Let's go for 3 and 4, 4 being a management command that delete given preferences from database. Later, if we have some time and other ideas to handle the migration thing, we'll work on it.

pip182 commented 7 years ago

Thank you for your hard work on this project! It does everything I need so far but this issue has bitten me many times and was especially frustrating before I figured out how to fix it. I use DBeaver to delete the rows from the database manually but this is a pain and the site will down until I am able to remove the row (which is not ideal by any means). A management command that would just remove the unused rows would be great!

czlee commented 7 years ago

Ah, for what it's worth, here's what @philipbelesky and I are currently doing, which is working fine for our purposes:

  1. We put python manage.py checkpreferences (which removes obsolete preferences) in our post-compile script, which runs after python manage.py migrate. For practical purposes, this is basically automated. Important caveat: I think this script/hook is a Heroku-specific thing, and also I've just tried looking for it and I think it might be undocumented.
  2. When we change a preference (not just remove), we're writing (Django) data migrations to handle them; here's an example. I suppose one could also do this for deleted preferences, but as discussed above, this is a little cumbersome to do either manually or automatically!

Hope that might be helpful for others wondering what to do with this, though of course the above might not work for everyone.

agateblue commented 7 years ago

Thank you for bumping this, I'll try to work on this today or tomorrow. Stay tuned ;)

agateblue commented 7 years ago

Can anyone of you @czlee or @pip182 post a sample stacktrace of the erros (hte pastebin ones have been deleted) directly in the issue ? I'dl like to know precisely where the code is crashing because of obsolete preferences in database.

I think the fix for that fill involve multiple things:

Do you think this would adress your issue ?

pip182 commented 7 years ago

I think the plan you have to fix this is great! I think that maybe even just adding a section to the docs that explains how to fix this error would help new users for now.

Here is one I was dealing with yesterday.

Traceback (most recent call last):
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 132, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/contrib/auth/decorators.py", line 22, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/contrib/auth/decorators.py", line 22, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/home/django/sites/production/hallmark-homestead/bidding/views.py", line 93, in wrapped
    return function(request, *args, **kwargs)
  File "/home/django/sites/production/hallmark-homestead/bidding/views.py", line 773, in roomdetail
    'deleted_cabinets': cabinets_in_room.filter(deleted__isnull=False),
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/shortcuts.py", line 67, in render
    template_name, context, request=request, using=using)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/template/loader.py", line 99, in render_to_string
    return template.render(context, request)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/template/backends/django.py", line 74, in render
    return self.template.render(context)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/template/base.py", line 209, in render
    with context.bind_template(self):
  File "/usr/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/template/context.py", line 241, in bind_template
    updates.update(processor(self.request))
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/dynamic_preferences/processors.py", line 9, in global_preferences
    return {'global_preferences': manager.all()}
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/dynamic_preferences/managers.py", line 150, in all
    return self.load_from_db()
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/dynamic_preferences/managers.py", line 157, in load_from_db
    db_prefs = {p.preference.identifier(): p for p in self.queryset}
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/dynamic_preferences/managers.py", line 157, in <dictcomp>
    db_prefs = {p.preference.identifier(): p for p in self.queryset}
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/django/utils/functional.py", line 59, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/dynamic_preferences/models.py", line 36, in preference
    return self.registry.get(section=self.section, name=self.name)
  File "/home/django/virtualenvs/hallmark_django/local/lib/python2.7/site-packages/dynamic_preferences/registries.py", line 123, in get
    self.__class__.__name__, section, name))
NotFoundInRegistry: No such preference in GlobalPreferenceRegistry with section=Misc and name=job_number_note

Request repr():
<WSGIRequest
path:/bid/3953/room/detail/10913/,
GET:<QueryDict: {}>,
POST:<QueryDict: {}>,
COOKIES:{'csrftoken': 'HRFsLmZxQY8XIfWLQzCe1fYOOk7gGdFL',
 'ie-warned': '1',
 'sessionid': 'da7ulipk1skobt4sj6cip0r2aqzbxua2'},
META:{u'CSRF_COOKIE': u'HRFsLmZxQY8XIfWLQzCe1fYOOk7gGdFL',
 'HTTP_ACCEPT': 'text/html, application/xhtml+xml, */*',
 'HTTP_ACCEPT_ENCODING': 'gzip, deflate',
 'HTTP_ACCEPT_LANGUAGE': 'en-US',
 'HTTP_CONNECTION': 'close',
 'HTTP_COOKIE': 'ie-warned=1; csrftoken=HRFsLmZxQY8XIfWLQzCe1fYOOk7gGdFL; sessionid=da7ulipk1skobt4sj6cip0r2aqzbxua2',
 'HTTP_DNT': '1',
 'HTTP_HOST': 'hallmarkcabinetpricing.com',
 'HTTP_REFERER': 'https://hallmarkcabinetpricing.com/bid/3953/room/1/',
 'HTTP_USER_AGENT': 'Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; rv:11.0) like Gecko',
 'HTTP_X_REAL_IP': '13.91.1.166',
 'HTTP_X_SCHEME': 'https',
 'PATH_INFO': u'/bid/3953/room/detail/10913/',
 'QUERY_STRING': '',
 'RAW_URI': '/bid/3953/room/detail/10913/',
 'REMOTE_ADDR': '127.0.0.1',
 'REMOTE_PORT': '53916',
 'REQUEST_METHOD': 'GET',
 'SCRIPT_NAME': u'',
 'SERVER_NAME': '127.0.0.1',
 'SERVER_PORT': '9000',
 'SERVER_PROTOCOL': 'HTTP/1.0',
 'SERVER_SOFTWARE': 'gunicorn/19.6.0',
 'gunicorn.socket': <socket._socketobject object at 0x7fc6f910a440>,
 'wsgi.errors': <gunicorn.http.wsgi.WSGIErrorsWrapper object at 0x7fc6f924c290>,
 'wsgi.file_wrapper': <class 'gunicorn.http.wsgi.FileWrapper'>,
 'wsgi.input': <gunicorn.http.body.Body object at 0x7fc6f924c350>,
 'wsgi.multiprocess': True,
 'wsgi.multithread': False,
 'wsgi.run_once': False,
 'wsgi.url_scheme': 'http',
 'wsgi.version': (1, 0)}>
czlee commented 7 years ago

I'm not currently hitting it, but the error's easy enough to reproduce by adding a fake line to the database. Just be aware that this one's not actually caused by the removal of a preference, per se, just artificially constructed to look like that's what happened: http://dpaste.com/04TBDED

I think I'd recommend getting it to post a warning to the logs but otherwise just ignore the obsolete preference. All things considered, having an old unused preference hanging around the database isn't really harmful. (Of course you can make it an option if you want some more work. =) ) To be honest, I wouldn't really use a management command specifically targeted for named preferences—more likely, I'd want to run checkpreferences as it already stands, maybe with a dry run if I'm nervous about accidentally deleting useful data.

As for the migration script… I guess the biggest difficulty with automatically generating migrations is that, to do this properly, you'd need to compare the preferences currently in code, to the existing migrations, not just the existing database (as checkpreferences does)? I don't know how Django (or formerly South) analyzes the code to pick up stuff like that. On the other hand, I'd imagine generating the actual migration code wouldn't be too hard, though whether it's worth the investment is another question. As things currently stand, I only bother writing migrations when I need to "translate" an old preference to a new preference, e.g. changing a boolean to a choice (with three choices).

I mean, maybe the advantage of migrations is that we can just roll preference updates into manage.py migrate, rather than having to run a separate command for preferences? But if you can find a way to automatically run the migrations, you can presumably also find a way to automatically run checkpreferences (as we have, noted above).

agateblue commented 7 years ago

I've done some work in #68 regarding this issue, can you review that when you have the time ?