etianen / django-reversion

django-reversion is an extension to the Django web framework that provides version control for model instances.
https://django-reversion.readthedocs.io
BSD 3-Clause "New" or "Revised" License
3.03k stars 488 forks source link

Circular Reference Support in Reversion #51

Closed eskhool closed 13 years ago

eskhool commented 13 years ago

Setup:

Steps:

  1. Delete Model A (deleting the inlines of Model B) using django admin
  2. Revert Model A using reversion

Fails, as I'm guessing circular references are not supported...before I go chasing this down (since I'm still firefighting this problem in production right now!), can somebody please confirm that this is not supported yet and any advice/help on recovering...

I haven't lost data since the reversion has the model stored but is just unable to restore it without some help.

Thanks for that help :)

etianen commented 13 years ago

That kinda depends on your database backend. Because MySQL InnoDB doesn't support deferred foreign keys, circular relationships plain won't work. Postgres will manage, so long as you do it all in a transaction. SQL doesn't have foreign key support at all!

Of course, this is assuming that you're being borked by a database integrity error. If it's something else, I'm gonna need a stack trace.

Sorry about the grief! :(

eskhool commented 13 years ago

I'm on Postgres though I'm using django admin without any transaction middleware enabled. I'm guessing that is required for this to happen in a transaction involving the admin?

I'm getting a

DoesNotExist: [Model B] matching query does not exist. error

which I'm assuming is because Model B does not exist yet when Model A is being saved?

etianen commented 13 years ago

Indeed. I'm still going to need that stack trace, though, to work out what's up.

eskhool commented 13 years ago

Here you go, and thanks for the great support!

Exception: <class 'common.models.bus.DoesNotExist'> Traceback (most recent call last): File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/core/handlers/base.py", line 100, in get_response response = callback(request, _callback_args, _callback_kwargs) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/utils/decorators.py", line 76, in _wrapped_view response = view_func(request, _args, _kwargs) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/views/decorators/cache.py", line 78, in _wrapped_view_func response = view_func(request, _args, _kwargs) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/contrib/admin/sites.py", line 190, in inner return view(request, _args, _kwargs) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/db/transaction.py", line 299, in _commit_on_success res = func(_args, *_kw) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/reversion/revisions.py", line 346, in _create_on_success self.end() File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/reversion/revisions.py", line 280, in end serialized_data = serializers.serialize(registration_info.format, [obj], fields=registration_info.fields) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/core/serializers/init.py", line 91, in serialize s.serialize(queryset, options) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/core/serializers/base.py", line 48, in serialize self.handle_fk_field(obj, field) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/core/serializers/python.py", line 48, in handle_fk_field related = getattr(obj, field.name) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/db/models/fields/related.py", line 313, in get rel_obj = QuerySet(self.field.rel.to).using(db).get(params) File "/mnt/code/vhosts/skoolpal/ps/.virtualenv/lib/python2.6/site-packages/django/db/models/query.py", line 347, in get % self.model._meta.object_name) DoesNotExist: MBusStopBusRouteVariation matching query does not exist.

etianen commented 13 years ago

Ooo, interesting. It's not going wrong during the revert procedure, but during the revision that gets saved immediately after the revert.

Can I see your model definitions and reversion.register() call? By the look of things, it's attempted an incomplete revert, possibly due to an error in the initial model registration.

eskhool commented 13 years ago

Why bore everyone with a long and complex set of models :)...

I've been able to narrow this defect down by using a vanilla django POC environment where I can successfully reproduce the failure of recovery of a deleted object in the event that the FK field in Model A is excluded (look at setup above in the issue)...i.e. it is not supposed to show up in the django admin and instead gets auto calculated

etianen commented 13 years ago

Still, unless I see some source code, I can't really help. There are intricacies with regard to setting up the follow relationships with reversion. If you don't register models correctly, or are relying on the automagical admin integration, it can sometimes lead to unexpected results.

eskhool commented 13 years ago

Understood, here you go:

models.py

from django.db import models

class Poll(models.Model): question = models.CharField(max_length=200) pub_date = models.DateTimeField('date published')

def __unicode__(self):
    return '%s' % self.question

class People(models.Model): name = models.CharField(max_length=128)

def __unicode__(self):
    return '%s' % self.name

class Choice(models.Model): poll = models.ForeignKey(Poll) choice = models.CharField(max_length=200) votes = models.IntegerField() person_choice = models.ForeignKey('PeopleChoice', blank=True, null=True, default=None, related_name='linked_people_choice')

def save(self, *args, **kwargs):
    super(Choice, self).save(*args, **kwargs)

def __unicode__(self):
    return '%s' % self.choice

class PeopleChoice(models.Model): people = models.ForeignKey(People) choice = models.ForeignKey(Choice)

def __unicode__(self):
    return '%s - %s' % (self.people, self.choice,)

admin.py

import reversion from django.contrib import admin from reversion.admin import VersionAdmin

from poc_app.models import Poll, Choice, People, PeopleChoice

class PeopleChoiceInline(admin.TabularInline): model = PeopleChoice

class PollAdmin(VersionAdmin): fields = ['pub_date', 'question'] list_display = ['pub_date', 'question']

class ChoiceAdmin(VersionAdmin): inlines = [PeopleChoiceInline] exclude = ('person_choice',)

class PeopleAdmin(VersionAdmin): pass

class PeopleChoiceAdmin(VersionAdmin): pass

admin.site.register(Poll, PollAdmin) admin.site.register(Choice, ChoiceAdmin) admin.site.register(People, PeopleAdmin) admin.site.register(PeopleChoice, PeopleChoiceAdmin)

I had to unregister and register because reversion was now following the M2M automatically in Choice

reversion.unregister(Choice) reversion.register(Choice, follow=['person_choice', 'peoplechoice_set', 'poll'])

etianen commented 13 years ago

I'm afraid I'm not really following your schema here. The ForeignKey from Choice to PeopleChoice is pretty unfathomable to me.

I don't think your problem has anything to do with recursive relationships. This is more to do with incomplete revisions being saved. If you can give me a simple schema where this is failing, and a series of steps to reproduce the error, I'll take a look. There's just too many variables at the moment.

I'll need the example as a complete django project, running on sqlite, and the steps need to be a complete set of actions from the initial syncdb to the error occuring.

etianen commented 13 years ago

I'm closing this for now. If you come up with a more concrete example that fails, I'll be happy to investigate.