follow

#31 Incorrect ManyToMany field handling

Reported by Sardar Yumatov 2010-07-30 (last edited unknown)

If the node has many-to-many field, then this can not be assigned until the node is saved first. Django forms always defer relational fields until post save()

Treebeard creates nodes with * *kwargs, assigning many-to-many values directly in the constructor. Standard django model fails here. Yes, it is not the problem of treebeard, but rather how django handles relational fields (consistency).

Current solution is to override init and save_base and defer all many-to-many fields until object is saved. This requires extra code.

Better fix: instead of node(* *kwargs) iterate over Model._meta.many_to_many, pop all these fields from kwargs. Then instance.save() and then assign all popped field values.

Attachments

Comments and changes

1. #1 Anonymous

   written 2010-10-10

   Encountered the same issue. Could you elaborate on the workaround in the init and save_base?

2. #2 Gustavo Picon

   written 2010-11-28

   • Changed status from new to open.

   Can somebody write a test case for this? Currently there isn't a M2M test case in treebeard's suite.

3. #3 Alexei Vlasov

   written 2011-03-28

   • Added attachment django-treebeard_forms_m2m.patch.

   I wrote a simple && dirty patch for forms.py

4. #4 Alejandro Peralta

   written 2011-04-28

   @Alexei Vlasov I think the self.save() in line 35 of your patch:

   +                self.instance = self.Meta.model.add_root(** cl_data)
   +            self.save()
   

   makes the node be saved in the root position. That is it creates the node ok, but in level 1

   also what is the point of the m2m_fl in the following bit:

   +            m2m_fl = False
   +            for field in self.cleaned_data:
   +                if type(self.cleaned_data[field]) == list:
   +                    m2m_fl = True
   +                else:
   +                    cl_data[field] = self.cleaned_data[field]
   

   same as:

   +            for field in self.cleaned_data:
   +                if type(self.cleaned_data[field]) != list:
   +                    cl_data[field] = self.cleaned_data[field]
   

   Just a friendly advice,

   Cheers!

5. #5 Alejandro Peralta

   written 2011-04-28

   I tried this as save method, it seems to work:

       def save(self, commit=True):
           reference_node_id = 0
           if '_ref_node_id' in self.cleaned_data:
               reference_node_id = self.cleaned_data['_ref_node_id']
               # delete auxilary fields not belonging to node model
               del self.cleaned_data['_ref_node_id']
   
           if '_position' in self.cleaned_data:
               position_type = self.cleaned_data['_position']
               # delete auxilary fields not belonging to node model
               del self.cleaned_data['_position']          
   
           if self.instance.pk is None:
               cl_data = {}
               for field in self.cleaned_data:
                   if not isinstance(self.cleaned_data[field], (list, QuerySet)):
                       cl_data[field] = self.cleaned_data[field]
               if reference_node_id:
                   reference_node = self.Meta.model.objects.get(
                       pk=reference_node_id)
                   self.instance = reference_node.add_child(** cl_data)
                   self.instance.move(reference_node, pos=position_type)
               else:
                   self.instance = self.Meta.model.add_root(** cl_data)
   
           else:
               # this is needed in django >= 1.2
   ...
   

6. #6 Anonymous

   written 2011-06-16

   Ran into this problem moments ago, wrote a quick fix:

   (in mp_tree.MP_Node)

       @classmethod
       def add_root(cls, **kwargs):
           """
           Adds a root node to the tree.
   
           :raise PathOverflow: when no more root objects can be added
           """
   
           # do we have a root node already?
           last_root = cls.get_last_root_node()
   
           if last_root and last_root.node_order_by:
               # there are root nodes and node_order_by has been set
               # delegate sorted insertion to add_sibling
               return last_root.add_sibling('sorted-sibling', **kwargs)
   
           if last_root:
               # adding the new root node as the last one
               newpath = cls._inc_path(last_root.path)
           else:
               # adding the first root node
               newpath = cls._get_path(None, 1, 1)
           # Pop ManyToManyFields to add them later
           m2m = {}
           for field in cls._meta.many_to_many:
               if field.name not in kwargs:
                   continue
               m2m[field.name] = kwargs.pop(field.name)
           
           # creating the new object
           newobj = cls(**kwargs)
           newobj.depth = 1
           newobj.path = newpath
           # saving the instance before returning it
           newobj.save()
           # save m2m
           for field, value in m2m.items():
               setattr(newobj, field, value)
           newobj.save()
           transaction.commit_unless_managed()
           return newobj
   

7. #7 Anonymous

   written 2011-08-11

   Bump. Why is this still an open issue after over a year with a provided patch? First time out of the gate using django-treebeard, and I ran into this bug. Essentially, treebeard only works on models that have no m2m relations, which is useless, unless you add complicated overrides to each model or manually patch the source.

   I'd heard good things about treebeard, but I'm rethinking my options at this point. Something this core shouldn't just be sitting in the pipeline, so it doesn't instill much faith in the rest of the codebase for me.

Verification: Please write the text from the image in the box (letters only)

Is that you, Humanoid? Is this me?

• Cancel