[PATCH 1 of 2 compound v2] Propagate skiprtypes to subentities cw_skip_copy_for while cloning

Denis Laxalde denis.laxalde at logilab.fr
Mon Dec 18 09:01:22 CET 2017


Sylvain Thénault a écrit :
> On 14/12/2017 09:16, Denis Laxalde wrote:
>> Sylvain Thenault a écrit :
>>> # HG changeset patch
>>> # User Sylvain Thénault <sylvain.thenault at logilab.fr>
>>> # Date 1513098203 -3600
>>> #      Tue Dec 12 18:03:23 2017 +0100
>>> # Node ID 0fc9b87651095ba0773fe2b623c05d525e564676
>>> # Parent  00a664463fdbc9db154056f064e767dc54e3f2ab
>>> Propagate skiprtypes to subentities cw_skip_copy_for while cloning
>>>
>>> We have to consider it at every levels of the container tree, not only for the
>>> top-level entity.
>>>
>>> While doing so, it sounded good to move creation of the list to a cached
>>> property to avoid redoing this for each cloned entity of the tree.
>> I'm sold on this change... Long story short, I think things are getting
>> a bit too complex to solve a use case which, if I understand correctly,
>> could be solved differently with either a couple of declarations in
>> client code or a different implementation of clone operations during
>> graph traversal.
>>
>> By client code declarations, I mean (following new test schema added in
>> this changeset), if I add:
>>
>> class Biography(AnyEntity):
>>     __regid__ = 'Biography'
>>     cw_skip_copy_for = [('see_also', 'subject')]
>>
>> doesn't it just solve the problem?
> 
> It does. My point is that as a client of the compound clone's feature, I
> understand 'skiprtypes' as a centralized way to control what's to clone
> / consider or not. If skiprtypes is only considered for relations of the
> top-level container, it feels much less useful.

So I've been thinking a bit about this and (now) agree that this is a
legitimate feature request.

I'm still not enthusiastic about how this is made possible in this
patch. In particular, the live modification of "cw_skip_copy_for"
attribute is a bit messy. In fact, I now realize that the pattern was
already there for the top-level entity, so... I don't know what to
suggest as an alternative. Not a big deal anyways, just mentioning...


Finally, about the "original vs self" issue in "copy_entity", I think
that if "copy_entity" is to be kept as a method, then one improvement
could be to have an extra "adaptation" of entities to be copied:

  clones[child] = child.cw_adapt_to('IClonable').copy_entity(**kwargs)

What do you think?

> 
>>
>>
>> Below some remarks I wrote initially, but we have to agree on the idea
>> of the patch first.
>>
>>> diff --git a/cubicweb_compound/entities.py b/cubicweb_compound/entities.py
>>> --- a/cubicweb_compound/entities.py
>>> +++ b/cubicweb_compound/entities.py
>>> @@ -1,6 +1,6 @@
>>> -# copyright 2015-2016 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
>>> +# copyright 2015 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
>>>  # contact http://www.logilab.fr -- mailto:contact at logilab.fr
>>>  #
>>>  # This program is free software: you can redistribute it and/or modify it under
>>>  # the terms of the GNU Lesser General Public License as published by the Free
>>>  # Software Foundation, either version 2.1 of the License, or (at your option)
>>> @@ -27,35 +27,18 @@ Assumptions:
>>>  .. _container: https://www.cubicweb.org/project/cubicweb-container
>>>  """
>>>  
>>>  from itertools import chain
>>>  
>>> +from logilab.common.decorators import cachedproperty
>>> +
>>>  from cubicweb.view import EntityAdapter
>>>  from cubicweb.predicates import is_instance, partial_relation_possible
>>>  
>>>  from . import CompositeGraph
>>>  
>>>  
>>> -def copy_entity(original, **attributes):
>>> -    """Return a copy of an entity.
>>> -
>>> -    Only attributes and non-composite relations are copied (relying on
>>> -    `entity.copy_relations()` for the latter).
>>> -    """
>>> -    attrs = attributes.copy()
>>> -    original.complete()
>>> -    for rschema in original.e_schema.subject_relations():
>>> -        if (not rschema.final or rschema.meta
>>> -                or (rschema.type, 'subject') in original.cw_skip_copy_for):
>>> -            continue
>>> -        attr = rschema.type
>>> -        attrs.setdefault(attr, original.cw_attr_value(attr))
>>> -    clone = original._cw.create_entity(original.cw_etype, **attrs)
>>> -    clone.copy_relations(original.eid)
>>> -    return clone
>>> -
>>> -
>>>  class IContainer(EntityAdapter):
>>>      """Abstract adapter for entities which are a container root."""
>>>      __abstract__ = True
>>>      __regid__ = 'IContainer'
>>>  
>>> @@ -201,31 +184,59 @@ class IClonableAdapter(EntityAdapter):
>>>          Return a dictionary mapping original entities to their clone.
>>>          """
>>>          assert clone.cw_etype == self.entity.cw_etype, \
>>>              "clone entity type {} does not match with original's {}".format(
>>>                  clone.cw_etype, self.entity.cw_etype)
>>> -        related = self.graph.child_related(
>>> -            self.entity, follow_relations=self.follow_relations)
>>> -        # take care not modifying clone.cw_skip_copy_for class attribute to avoid undesired side
>>> -        # effects (e.g. clone called with different skiprtypes value), set an instance attribute.
>>> -        clone.cw_skip_copy_for = list(chain(
>>> -            clone.cw_skip_copy_for,
>>> -            ((rtype, 'subject') for rtype in self.skiprtypes),
>>> -            ((rtype, 'object') for rtype in self.skiprtypes)))
>>> +        related = list(self.graph.child_related(
>>> +            self.entity, follow_relations=self.follow_relations))
>>> +        self._extend_skip_copy_for(clone)
>>>          with self._cw.deny_all_hooks_but(*self.enabled_hook_categories):
>>>              clone.copy_relations(self.entity.eid)
>>>              clones = {self.entity: clone}
>>>              for parent, (rtype, parent_role), child in related:
>>>                  rel = rtype if parent_role == 'object' else 'reverse_' + rtype
>>>                  kwargs = {rel: clones[parent]}
>>>                  clone = clones.get(child)
>>>                  if clone is not None:
>>>                      clone.cw_set(**kwargs)
>>>                  else:
>>> -                    clones[child] = copy_entity(child, **kwargs)
>>> +                    clones[child] = self.copy_entity(child, **kwargs)
>>>              return clones
>>>  
>>> +    def copy_entity(self, original, **attributes):
>>> +        """Return a copy of an entity.
>>> +
>>> +        Only attributes and non-composite relations are copied (relying on
>>> +        `entity.copy_relations()` for the latter).
>>> +        """
>>> +        attrs = attributes.copy()
>>> +        original.complete()
>>> +        for rschema in original.e_schema.subject_relations():
>>> +            if (not rschema.final or rschema.meta
>>> +                    or (rschema.type, 'subject') in original.cw_skip_copy_for):
>>> +                continue
>>> +            attr = rschema.type
>>> +            attrs.setdefault(attr, original.cw_attr_value(attr))
>>> +        clone = original._cw.create_entity(original.cw_etype, **attrs)
>>> +        self._extend_skip_copy_for(clone)
>>> +        clone.copy_relations(original.eid)
>>> +        return clone
>>> +
>>> +    def _extend_skip_copy_for(self, clone):
>>> +        # take care not modifying clone.cw_skip_copy_for **class attribute** to
>>> +        # avoid undesired side effects (e.g. clone called with different
>>> +        # skiprtypes value), so set an instance attribute.
>>> +        clone.cw_skip_copy_for = set(clone.cw_skip_copy_for) | self.skip_swallow_copy_for
>>> +
>>> +    @cachedproperty
>>> +    def skip_swallow_copy_for(self):
>>> +        return set(chain(
>>> +            # turn skiprtypes into a list suitable for Entity.cw_skip_copy_for
>>> +            ((rtype, 'subject') for rtype in self.skiprtypes),
>>> +            ((rtype, 'object') for rtype in self.skiprtypes),
>>> +        ))
>>> +
>> `set(chain(a, b))` looks convoluted, why not `set(a) | set(b)`?
> 
> no strong opinion on this. For the record I add a third list in a later
> cset. I don't care to change.
> 
>>
>>>  
>>>  def registration_callback(vreg):
>>>      vreg.register_all(globals().values(), __name__)
>>>      # Necessary during db-init or test mode.
>>>      IClonableAdapter.clone_relations.clear()
>>> diff --git a/cubicweb_compound/views.py b/cubicweb_compound/views.py
>>> --- a/cubicweb_compound/views.py
>>> +++ b/cubicweb_compound/views.py
>>> @@ -21,12 +21,10 @@ from cubicweb import _, neg_role
>>>  from cubicweb.web import Redirect
>>>  from cubicweb.predicates import adaptable, has_permission, match_form_params, one_line_rset
>>>  from cubicweb.web.controller import Controller
>>>  from cubicweb.web.views import actions, editforms, ibreadcrumbs
>>>  
>>> -from .entities import copy_entity
>>> -
>>>  
>>>  def linkto_clone_url_params(entity):
>>>      iclone = entity.cw_adapt_to('IClonable')
>>>      linkto = '%s:%s:%s' % (iclone.rtype, entity.eid, neg_role(iclone.role))
>>>      return {'__linkto': linkto}
>>> @@ -86,11 +84,11 @@ class CloneController(Controller):
>>>          original = self._cw.entity_from_eid(eid)
>>>          iclone = original.cw_adapt_to('IClonable')
>>>          rtype = (iclone.rtype if iclone.role == 'object'
>>>                   else 'reverse_' + iclone.rtype)
>>>          kwargs = {rtype: eid}
>>> -        clone = copy_entity(original, **kwargs)
>>> +        clone = iclone.copy_entity(original, **kwargs)
>> So here, iclone is an adapter on "original". It looks strange that the
>> call to 'copy_entity()' requires "original" to be passed again as
>> "original == iclone.entity" (or original == self.entity within
>> 'copy_entity' method).
> 
> I agree this is a bit strange. copy_entity is turned as a method to ease
> access to skiprtypes/follow_relation attributes of the adapter. If we
> agree to continue that way it could be left optional with default to
> self.entity.
> 
>>>          msg = self._cw._('clone of entity #%d created' % eid)
>>>          raise Redirect(clone.absolute_url(__message=msg))
>>>  
>>>  
>>>  class IContainedBreadcrumbsAdapter(ibreadcrumbs.IBreadCrumbsAdapter):
>>> diff --git a/test/data/schema.py b/test/data/schema.py
>>> --- a/test/data/schema.py
>>> +++ b/test/data/schema.py
>>> @@ -105,5 +105,10 @@ class Group(EntityType):
>>>  
>>>  class member(RelationDefinition):
>>>      """Indicates a member of a Group."""
>>>      subject = 'Group'
>>>      object = 'Agent'
>>> +
>>> +
>>> +class see_also(RelationDefinition):
>>> +    subject = 'Biography'
>>> +    object = 'Event'



More information about the saem-devel mailing list