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

Sylvain Thénault sylvain.thenault at logilab.fr
Thu Dec 14 11:12:35 CET 2017



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.

>
>
> 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'
>> diff --git a/test/test_compound.py b/test/test_compound.py
>> --- a/test/test_compound.py
>> +++ b/test/test_compound.py
>> @@ -16,11 +16,11 @@
>>  """cubicweb-compound tests"""
>>  
>>  from cubicweb.devtools.testlib import CubicWebTC
>>  
>>  from cubicweb_compound import CompositeGraph
>> -from cubicweb_compound.entities import IClonableAdapter, copy_entity
>> +from cubicweb_compound.entities import IClonableAdapter
>>  from cubicweb_compound.views import CloneAction
>>  
>>  
>>  def sort_keys(dic):
>>      return dict((k, sorted(v)) for k, v in dic.items())
>> @@ -287,21 +287,23 @@ class CloneTC(CubicWebTC):
>>          with self.admin_access.repo_cnx() as cnx:
>>              cnx.create_entity('Agent', name=u'bob', skip=u'skipped')
>>              cnx.commit()
>>          with self.new_access(u'georges').repo_cnx() as cnx:
>>              bob = cnx.find('Agent').one()
>> -            bob2 = copy_entity(bob)
>> -            alice = copy_entity(bob, name=u'alice', knows=bob)
>> +            iclone = bob.cw_adapt_to('IClonable')
>> +            bob2 = iclone.copy_entity(bob)
>> +            alice = iclone.copy_entity(bob, name=u'alice', knows=bob)
>>              cnx.commit()
>>              self.assertEqual(bob2.name, u'bob')
>>              self.assertIsNone(bob2.skip)
>>              self.assertEqual(alice.name, u'alice')
>>              self.assertEqual([x.eid for x in alice.knows], [bob.eid])
>>              self.assertEqual(alice.created_by[0].login, u'georges')
>>              self.assertEqual(bob2.created_by[0].login, u'georges')
>>              self.assertGreater(alice.creation_date, bob.creation_date)
>> -            alice2 = copy_entity(alice)
>> +            iclone = alice.cw_adapt_to('IClonable')
>> +            alice2 = iclone.copy_entity(alice)
>>              cnx.commit()
>>              alice2.cw_clear_all_caches()
>>              self.assertEqual(alice2.name, u'alice')
>>              self.assertEqual([x.eid for x in alice2.knows], [bob.eid])
>>  
>> @@ -359,10 +361,20 @@ class CloneTC(CubicWebTC):
>>              orig_cw_skip_copy_for = bob.cw_skip_copy_for
>>              clone = clone_agent(cnx, u'bob', skiprtypes=('member',))
>>              self.assertFalse(clone.reverse_member)
>>              self.assertIs(bob.cw_skip_copy_for, orig_cw_skip_copy_for)
>>  
>> +    def test_clone_skiprtypes_sublevel(self):
>> +        with self.admin_access.repo_cnx() as cnx:
>> +            event = cnx.create_entity('Event')
>> +            bio = cnx.create_entity('Biography', event=event, see_also=event)
>> +            cnx.create_entity('Agent', name=u'bob', biography=bio)
>> +            cnx.commit()
>> +
>> +            clone = clone_agent(cnx, u'bob', skiprtypes=('see_also',))
>> +            self.assertEqual(clone.biography[0].see_also, ())
>> +
>>      def test_clone_full(self):
>>          with self.admin_access.repo_cnx() as cnx:
>>              agent = cnx.create_entity('Agent', name=u'bob')
>>              cnx.create_entity('OnlineAccount', reverse_account=agent)
>>              bio = cnx.create_entity('Biography', reverse_biography=agent)
>>

-- 
Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42)
Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations
Développement logiciel sur mesure:       http://www.logilab.fr/services
CubicWeb, the semantic web framework:    http://www.cubicweb.org



More information about the saem-devel mailing list