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.

Sign up for GitHub

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

Jump to bottom

Model instances without primary key value are unhashable in admin panel #678

Closed
kplatis opened this issue Nov 15, 2017 · 18 comments
Closed

Model instances without primary key value are unhashable in admin panel #678

kplatis opened this issue Nov 15, 2017 · 18 comments

Comments

@kplatis
Copy link

kplatis commented Nov 15, 2017

I have implemented a "backup" functionality when a user wants to keep a revision of his object along with all the related ones. I have implemented the admin integration and registered all the "follows" correctly. When the user presses the "create new backup" button, then the following code is executed:

    with reversion.create_revision():
        main_object.save()

        reversion.set_user(request.user)
        reversion.set_comment(comment)

and a revision is created for the object and all the related ones.

The problem appears when I create a new object that has a relationship to the main_object. When the new object is created, if the user presses "Create new Backup", then a revision is successfully created for the main_object and all the related ones BUT the previous revisions show this message on the admin panel: "Model instances without primary key value are unhashable".

The traceback that appears is:

File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  132.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  110.                     response = view_func(request, *args, **kwargs)
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  57.         response = view_func(request, *args, **kwargs)
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/django/contrib/admin/sites.py" in inner
  233.             return view(request, *args, **kwargs)
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/reversion/admin.py" in revision_view
  238.             context,
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/reversion/admin.py" in _reversion_revisionform_view
  184.                 version.revision.revert(delete=True)
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/reversion/models.py" in revert
  89.                         if item not in old_revision:
File "/Users/platico/PycharmProjects/news-service/src/lib/python2.7/site-packages/django/db/models/base.py" in __hash__
  521.             raise TypeError("Model instances without primary key value are unhashable")

with an Exception Type of TypeError and an Exception Value of Model instances without primary key value are unhashable
@etianen
Copy link
Owner

etianen commented Nov 16, 2017 via email

@kplatis
Copy link
Author

kplatis commented Nov 16, 2017 •
edited

Hmm.. can you please give me some directions on how I can check that?

@etianen
Copy link
Owner

etianen commented Nov 16, 2017 via email

@kplatis
Copy link
Author

kplatis commented Nov 16, 2017 •
edited

No I don't have any "follow" relations to properties, only to foreign keys.

Also I checked the database entries, none of the registered fields have null pk. Something that I noticed in the traceback is that the error is thrown when the 'revert' function is called. Isn't this suspicious since the only thing I do is to visit the revision page of an object in the admin? Why is the revert function called?

@kplatis
Copy link
Author

kplatis commented Nov 17, 2017 •
edited

Something else I noticed: the error is produced when I create objects of 2 specific classes, that are inherited by a 3rd one, hence multi-table inheritance.

When I create an object of one of those 2 classes, in the database I can find 2 revision entries of the same object (one for the child object and one for the parent object). They share the same object_id though. Is it supposed to be done this way? If not, I might have messed up the "follow" registration of those classes...

@etianen
Copy link
Owner

etianen commented Nov 17, 2017 via email

@kplatis
Copy link
Author

kplatis commented Nov 17, 2017 •
edited

So the general structure goes like this:

class Newspaper:
   title = models.CharField()

class Section:
  newspaper = models.foreignKey('app.Newspaper', related_name='sections')
  # other fields also

class LinkSection(Section):
  # some fields

class FeedSection(Section):
  # some fields

Then I register using admin integration so in admin.py the structure is:

@admin.register(Newspaper)
class NewspaperAdmin(VersionAdmin):

    def reversion_register(self, model, **options):
        options["follow"] = ("sections",)
        super(NewspaperAdmin, self).reversion_register(model, **options)
 

@admin.register(Section)
class SectionAdmin(VersionAdmin):

    def reversion_register(self, model, **options):
        options["follow"] = ("newspaper", "linksection", "feedsection", )
        # linksection and feedsection are the backward pointers as found using dir()
        super(SectionAdmin, self).reversion_register(model, **options)
 
@admin.register(LinkSection)
class LinkSectionAdmin(VersionAdmin):

    def reversion_register(self, model, **options):
        options["follow"] = ("section_ptr", )
        super(LinkSectionAdmin, self).reversion_register(model, **options)

@admin.register(FeedSection)
class FeedSectionAdmin(VersionAdmin):

    def reversion_register(self, model, **options):
        options["follow"] = ("section_ptr",)
        super(FeedSectionAdmin, self).reversion_register(model, **options)

@roman-karpovich
Copy link

roman-karpovich commented Nov 17, 2017 •
edited

Same problem for me. Multi-table inheritance.

import reversion
from django.db import models


@reversion.register(follow=('diaryevent_set', ))
class Diary(models.Model):
    pass


@reversion.register(follow=('specialdiaryevent', ))
class DiaryEvent(models.Model):
    diary = models.ForeignKey(Diary)


@reversion.register(follow=('diaryevent_ptr', ))
class SpecialDiaryEvent(DiaryEvent):
    pass

diary = Diary.objects.create()

with reversion.create_revision():
    reversion.add_to_revision(diary)

SpecialDiaryEvent.objects.create(diary=diary)

with reversion.create_revision():
    reversion.add_to_revision(diary)

reversion.models.Version.objects.get_for_object(diary)[1].revision.revert(delete=True)

@etianen
Copy link
Owner

etianen commented Nov 20, 2017

@platico , @roman-karpovich - thanks for the extremely useful debugging information.

Can you add a debugging line to your django-reversion installation? In models.py, line 88:

print(item)

I want to see what relation is causing the breakage. I have my suspicions, but it would be good to confirm. Once I have that, issuing a fix should be easy.

@roman-karpovich
Copy link

@etianen

In [2]: diary = Diary.objects.create()

In [3]: with reversion.create_revision():
   ...:         reversion.add_to_revision(diary)
   ...:     

In [4]: SpecialDiaryEvent.objects.create(diary=diary)
Out[4]: <SpecialDiaryEvent: SpecialDiaryEvent object>

In [5]: with reversion.create_revision():
   ...:         reversion.add_to_revision(diary)

In [6]: reversion.models.Version.objects.get_for_object(diary)[1].revision.revert(delete=True)
(9, <Diary: Diary object>)
(12, <DiaryEvent: DiaryEvent object>)
(12, <SpecialDiaryEvent: SpecialDiaryEvent object>)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-6-78965f73df34> in <module>()
----> 1 reversion.models.Version.objects.get_for_object(diary)[1].revision.revert(delete=True)

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/reversion/models.py in revert(self, delete)
     89                         print(item.pk, item)
     90                         if item not in old_revision:
---> 91                             item.delete(using=version.db)
     92                 # Attempt to revert all revisions.
     93                 _safe_revert(versions)

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/base.pyc in delete(self, using, keep_parents)
    867 
    868         collector = Collector(using=using)
--> 869         collector.collect([self], keep_parents=keep_parents)
    870         return collector.delete()
    871 

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/deletion.pyc in collect(self, objs, source, nullable, collect_related, source_attr, reverse_dependency, keep_parents)
    217                                  source_attr=ptr.remote_field.related_name,
    218                                  collect_related=False,
--> 219                                  reverse_dependency=True)
    220         if collect_related:
    221             for related in get_candidate_relations_to_delete(model._meta):

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/deletion.pyc in collect(self, objs, source, nullable, collect_related, source_attr, reverse_dependency, keep_parents)
    197             return
    198         new_objs = self.add(objs, source, nullable,
--> 199                             reverse_dependency=reverse_dependency)
    200         if not new_objs:
    201             return

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/deletion.pyc in add(self, objs, source, nullable, reverse_dependency)
    101         instances = self.data.setdefault(model, set())
    102         for obj in objs:
--> 103             if obj not in instances:
    104                 new_objs.append(obj)
    105         instances.update(new_objs)

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/base.pyc in __hash__(self)
    490     def __hash__(self):
    491         if self._get_pk_val() is None:
--> 492             raise TypeError("Model instances without primary key value are unhashable")
    493         return hash(self._get_pk_val())
    494 

TypeError: Model instances without primary key value are unhashable

Django is trying to delete inherited child while parent is already deleted.

@etianen
Copy link
Owner

etianen commented Nov 20, 2017

Thanks for all the debugging help! I've uploaded a fix to master. Please let me know if it's fixed.

@roman-karpovich
Copy link

Problem still persists. Objects drops their pks during .delete() call, so i think we should sync object from db, for example using item.refresh_from_db() with except on DoesNotExist. On the other hand, this will produce huge amount of database queries...

@etianen
Copy link
Owner

etianen commented Nov 20, 2017 via email

@roman-karpovich
Copy link 

In [8]: from tv_survey.test_reversion.models import *

In [9]: diary = Diary.objects.create()

In [10]: with reversion.create_revision():
    ...:         reversion.add_to_revision(diary)
    ...:     

In [11]: with reversion.create_revision():
    ...:         reversion.add_to_revision(diary)
    ...:     

In [12]: SpecialDiaryEvent.objects.create(diary=diary)
Out[12]: <SpecialDiaryEvent: SpecialDiaryEvent object>

In [13]: with reversion.create_revision():
    ...:         reversion.add_to_revision(diary)
    ...:     

In [14]: reversion.models.Version.objects.get_for_object(diary)[1].revision.revert(delete=True)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-14-78965f73df34> in <module>()
----> 1 reversion.models.Version.objects.get_for_object(diary)[1].revision.revert(delete=True)

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/reversion/models.pyc in revert(self, delete)
     91                         # https://github.com/etianen/django-reversion/issues/678
     92                         if item.pk is not None and item not in old_revision:
---> 93                             item.delete(using=version.db)
     94                 # Attempt to revert all revisions.
     95                 _safe_revert(versions)

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/base.pyc in delete(self, using, keep_parents)
    867 
    868         collector = Collector(using=using)
--> 869         collector.collect([self], keep_parents=keep_parents)
    870         return collector.delete()
    871 

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/deletion.pyc in collect(self, objs, source, nullable, collect_related, source_attr, reverse_dependency, keep_parents)
    217                                  source_attr=ptr.remote_field.related_name,
    218                                  collect_related=False,
--> 219                                  reverse_dependency=True)
    220         if collect_related:
    221             for related in get_candidate_relations_to_delete(model._meta):

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/deletion.pyc in collect(self, objs, source, nullable, collect_related, source_attr, reverse_dependency, keep_parents)
    197             return
    198         new_objs = self.add(objs, source, nullable,
--> 199                             reverse_dependency=reverse_dependency)
    200         if not new_objs:
    201             return

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/deletion.pyc in add(self, objs, source, nullable, reverse_dependency)
    101         instances = self.data.setdefault(model, set())
    102         for obj in objs:
--> 103             if obj not in instances:
    104                 new_objs.append(obj)
    105         instances.update(new_objs)

/home/th13f/.virtualenvs/tv_survey/local/lib/python2.7/site-packages/django/db/models/base.pyc in __hash__(self)
    490     def __hash__(self):
    491         if self._get_pk_val() is None:
--> 492             raise TypeError("Model instances without primary key value are unhashable")
    493         return hash(self._get_pk_val())
    494 

TypeError: Model instances without primary key value are unhashable

@etianen etianen reopened this Nov 20, 2017
etianen added a commit that referenced this issue Nov 20, 2017
@etianen
Using django collector API to safely delete stale revision data in re…
d81ae0b 
…vert(). Refs #678
@etianen
Copy link
Owner

etianen commented Nov 20, 2017

How annoying!

I've pushed another potential fix, using the Django Collector() API which should hopefully trace all the relations correctly before deleting them.

Unfortunately, this is hard for me to debug locally, as it depends on the ordering of the version models!

@roman-karpovich
Copy link

@etianen seems that fix working. thanks alot.

@etianen
Copy link
Owner

etianen commented Nov 20, 2017 via email

@kplatis
Copy link
Author

kplatis commented Nov 20, 2017

I confirm it is fixed for me too. Thank you for the help!

@etianen etianen closed this as completed Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@etianen @kplatis @roman-karpovich