Code

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#2749 closed defect (fixed)

[patch] Deleting object with generic relation does not work with one to one

Reported by: clong Owned by: adrian
Component: Core (Other) Version: master
Severity: normal Keywords:
Cc: indirecthit@…, favo@…, tom@…, peter.kese@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Jay Parlar came across this bug while testing the POP branch. It appears that this code, when using generic relations:

      for related in cls._meta.get_all_related_many_to_many_objects():
            for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE):
                cursor.execute("DELETE FROM %s WHERE %s IN (%s)" % \
                    (qn(related.field.m2m_db_table()),
                        qn(related.field.m2m_reverse_name()), 

For the m2m_reverse_name returns the one-to-one id name (in the example Jay Parlar gives it's user_id) instead of the id used in the generic relation (in POP branch it's model_id or owner_id). The below patch seems to have fixed this problem.

Patch:

--- django/db/models/fields/generic.py  (revision 3668)
+++ django/db/models/fields/generic.py  (working copy)
@@ -117,7 +117,7 @@
         return self.object_id_field_name

     def m2m_reverse_name(self):
-        return self.model._meta.pk.attname
+        return self.object_id_field_name

     def contribute_to_class(self, cls, name):
         super(GenericRelation, self).contribute_to_class(cls, name)

Attachments (0)

Change History (7)

comment:1 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [3787]) Fixed #2749 -- Get the correct m2m_reverse_name() in generic relations. Thanks
to Jay Parlar and Chris Long for some good debugging here.

comment:2 Changed 8 years ago by Favo <favo@…>

  • Cc favo@… added
  • priority changed from normal to high
  • Resolution fixed deleted
  • Status changed from closed to reopened

the old action just remove only one record, the patch version will remove all
records who's object_id = xxx

Class Tag:
   object_id = ...
   content_type = ...
   content_object = GenericFK()

Class Photo:
   tags = GenericRelation(Tag)
   ...

photo1 = Photo()
tag1 = Tag(GFK=photo1); tag1.save()
tag2 = Tag(GFK=photo1); tag2.save()

tag1.delete() will cause:

"DELETE FROM tag WHERE object_id in (1);"

then both tag1 and tag2 have been deleted.

in fact the correct delete sql for generic FK should be:

   DELETE FROM tag WHERE object_id =1 and content_type = xxx;

but it's not easy to implement for current query backend, so I suggest
we just revert the changeset.

    # revert it.
    def m2m_reverse_name(self):
-        return self.model._meta.pk.attname
+        return self.object_id_field_name

comment:3 Changed 8 years ago by Thomas Steinacher <tom@…>

  • Cc tom@… added

comment:4 Changed 7 years ago by peter.kese@…

  • Cc peter.kese@… added

I am also encountering the same problem and have LOST heaps of data due to it and had to ask site administrators to avoid any deletion of any data for the while.

I have noticed, that by trying to remove models.GenericRelation(...) from models in which objects are often deleted improves situation, but data stil gets lost on models, where GenericRelation fields can't be removed.

Also in the Favo's suggeston, he says that correct deletion should be like this:

DELETE FROM tag WHERE object_id =1 and content_type = xxx;

but I think, if you wish to delete a specific tag, then it would be best to just delete the tag by tag's
tag1.id like this:

DELETE FROM tag WHERE id =''tag1.id'';

I believe Favo's example would still delete all tags that relate to photo1 instead of just the selected tag1.


This ticket is related to #3215, it is clearly a bug, it causes unexpected data loss and is critical.

comment:5 Changed 7 years ago by Favo <favo@…>

The implement for GenericRelation is totally wrong. it should not inherit from M anytoManyRelation -- that cause serious delete bug. because I have not time to implement a new relation class for generic, and the current support generic look up search with our patch. we just hide the delete function for short term.

For long term we need help django implement a new GenericRelation which support generic lookup and sync delete(with test).

I paste the patch here because it's not a real solution

Index: django/db/models/query.py
===================================================================
--- django/db/models/query.py   (revision 5321)
+++ django/db/models/query.py   (revision 5322)
@@ -926,6 +926,8 @@

         pk_list = [pk for pk,instance in seen_objs[cls]]
         for related in cls._meta.get_all_related_many_to_many_objects():
+            if related.field.__class__.__name__ == 'GenericRelation':
+                continue
             for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE):
                 cursor.execute("DELETE FROM %s WHERE %s IN (%s)" % \
                     (qn(related.field.m2m_db_table()),
@@ -933,6 +935,8 @@
                         ','.join(['%s' for pk in pk_list[offset:offset+GET_ITERATOR_CHUNK_SIZE]])),                     pk_list[offset:offset+GET_ITERATOR_CHUNK_SIZE])
         for f in cls._meta.many_to_many:
+            if f.__class__.__name__ == 'GenericRelation':
+                continue
             for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE):
                 cursor.execute("DELETE FROM %s WHERE %s IN (%s)" % \
                     (qn(f.m2m_db_table()), qn(f.m2m_column_name()),

comment:6 Changed 7 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [4428]) Fixed #3215, #3081, #2749 -- Fixed problem with mistaken deletion of objects when a GenericRelation? is involved. Thanks to Thomas Steinacher for helping to narrow down the problem (#3215), and Alex Dedul (#3081) for the starting point of a working patch.

comment:7 Changed 7 years ago by mtredinnick

(In [6900]) Fixed #3906 -- Fixed the reverse_m2m_name for a generic relation. Refs #2749.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.