Revert "Folder: call activate on the object instead of...

Revert "Folder: call activate on the object instead of portal_activities.activateObject in _recurseCallMethod()."

This reverts commits 3d3ec3cb ((+ fixup), which
broke recursion to objects that don't inherit ActiveObject, as explained in the
docstring.

I also think it was wrong in that default activate parameters disappear at the
end of the transaction whereas _recurseCallMethod may activate itself to avoid
creating to many activities at once. IOW, it's important that created
activities are the same no matter where _recurseCallMethod splits the browsing.

This change needed more review, as well as a unit test.

product/ERP5Type/Core/Folder.py

@@ -449,6 +449,7 @@ class FolderMixIn(ExtensionClass.Base):
     activate_kw.update(kw.get('activate_kw', ()))
     activate_kw.setdefault('active_process', None)
     activate_kw.setdefault('activity', 'SQLQueue')
+    activate = self.getPortalObject().portal_activities.activateObject
     validate = restricted and getSecurityManager().validate
     cost = activate_kw.setdefault('group_method_cost', .034) # 30 objects
     if cost != 1:
@@ -512,7 +513,7 @@ class FolderMixIn(ExtensionClass.Base):
           del next_id[0]
       if min_depth <= depth:
         check_limit()
-        getattr(container.activate(**getActivateKw(container, recurse_activate_kw)),
+        getattr(activate(container, **getActivateKw(container, recurse_activate_kw)),
                 method_id)(*method_args, **method_kw)
       del recurse_stack[depth:]
     try:
@@ -522,8 +523,8 @@ class FolderMixIn(ExtensionClass.Base):
         raise
       activate_kw['group_method_id'] = kw['group_id'] = '' # no grouping
       activate_kw['activity'] = 'SQLQueue'
-      self.getPortalObject().portal_activities.activateObject(self, **activate_kw
-        )._recurseCallMethod(method_id, method_args, method_kw, restricted=restricted, **kw)
+      activate(self, **activate_kw)._recurseCallMethod(
+        method_id, method_args, method_kw, restricted=restricted, **kw)
 
   security.declarePublic('recurseCallMethod')
   def recurseCallMethod(self, method_id, *args, **kw):

[Kazuhiko Shiozaki]

@jm @vpelletier I admit that my commit was 'incomplete workaround'. But reverting it means that 'newContent(activate_kw=...)' on Folder-ish object does not work at all as before. Shouldn't we also revert 'Folder: Make recursiveReindexObject scalable by calling _recurseCallMethod' (and related commits) as well, to avoid the regression ?

Or such usage was wrong (but was working by chance) and we should use newContent(reindex_kw=...) instead ?

[Vincent Pelletier]

Shouldn't we also revert 'Folder: Make recursiveReindexObject scalable by calling _recurseCallMethod' (and related commits) as well, to avoid the regression ?

That is quite a lot of things to revert, and to reapply when we know how we want to fix newContent(activate_kw=...), so I'm not sure it would be efficient to revert that too.

What about fixing this issue at constructInstance or _reindexOnCreation level instead ?

Or such usage was wrong (but was working by chance) and we should use newContent(reindex_kw=...) instead ?

I'm annoyed by the API supporting both arguments. That's 2 arguments to carefully keep track of, one which could contain the other (reindex_kw['activate_kw'] can be set and has to be pulled and applied in a specific order)... While initially checking this issue with Kazuhiko I realised that activate_kw was being ignored in constructInstance when requesting immediate indexation. And to add an unfortunate illustration of this, I just pushed yet another broken attempt at supporting activate_kw in the case of immediate reindexation inside constructInstance. (fix in the works, I believe nothing should rely on this argument anyway as it was never functional)

[Vincent Pelletier]

Extract from a traceback happening on content creation (as I was not sure about the whole code path):

  File "parts/erp5/product/ERP5Type/ERP5Type.py", line 415, in constructInstance
    container._setObject(id, base_ob)
  File "parts/erp5/product/ERP5Type/Core/Folder.py", line 945, in _setObject
    return folder._setObject(self, *args, **kw)
  File "eggs/Products.BTreeFolder2-2.13.5-py2.7.egg/Products/BTreeFolder2/BTreeFolder2.py", line 461, in _setObject
    notify(ObjectAddedEvent(ob, self, id))
  File "eggs/zope.event-3.5.2-py2.7.egg/zope/event/__init__.py", line 31, in notify
    subscriber(event)
  File "eggs/zope.component-3.9.5-py2.7.egg/zope/component/event.py", line 24, in dispatch
    zope.component.subscribers(event, None)
  File "eggs/zope.component-3.9.5-py2.7.egg/zope/component/_api.py", line 136, in subscribers
    return sitemanager.subscribers(objects, interface)
  File "eggs/zope.component-3.9.5-py2.7.egg/zope/component/registry.py", line 321, in subscribers
    return self.adapters.subscribers(objects, provided)
  File "eggs/zope.interface-4.3.3-py2.7-linux-x86_64.egg/zope/interface/adapter.py", line 598, in subscribers
    subscription(*objects)
  File "eggs/zope.component-3.9.5-py2.7.egg/zope/component/event.py", line 32, in objectEventNotify
    zope.component.subscribers((event.object, event), None)
  File "eggs/zope.component-3.9.5-py2.7.egg/zope/component/_api.py", line 136, in subscribers
    return sitemanager.subscribers(objects, interface)
  File "eggs/zope.component-3.9.5-py2.7.egg/zope/component/registry.py", line 321, in subscribers
    return self.adapters.subscribers(objects, provided)
  File "eggs/zope.interface-4.3.3-py2.7-linux-x86_64.egg/zope/interface/adapter.py", line 598, in subscribers
    subscription(*objects)
  File "eggs/Zope2-2.13.26-py2.7.egg/OFS/subscribers.py", line 110, in dispatchObjectMovedEvent
    callManageAfterAdd(ob, event.object, event.newParent)
  File "eggs/Zope2-2.13.26-py2.7.egg/OFS/subscribers.py", line 143, in callManageAfterAdd
    ob.manage_afterAdd(item, container)
  File "parts/erp5/product/ERP5Type/CopySupport.py", line 329, in manage_afterAdd
    self.reindexObject()

That's a crazy heap of intermediate methods. I understand better that we stash indexation arguments rather than pass them somehow as arguments.

What about pulling the indexation argumets in ERP5Type/CopySupport.py:manage_afterAdd and passing these as arguments to reindexObject ?

[Vincent Pelletier]

What about pulling the indexation argumets in ERP5Type/CopySupport.py:manage_afterAdd and passing these as arguments to reindexObject ?

Actually, scratch that: this is exactly what happens in _reindexObject as it calls _getReindexAndActivateParameterDict. And I do not see it calling into anything recursive either (which would end up calling recurseCallMethod)... @kazuhiko Would you have the call stack of a problematic case, which would show how recurseCallMethod is called ?

[Kazuhiko Shiozaki]

Here is a pseudo code :

invoice = accounting_module.newContent(
...
)
invoice.setDefaultActivateParameterDict({'tag': tag})
invoice.start()
invoice.setDefaultActivateParameterDict({})
invoice.activate(after_tag=tag).stop()

And here is the backtrace when invoice.start() is called, just before calling _recurseMethod :

  Script (Python)(59)XXX_xxx()
     58 
---> 59 invoice.start()
     60 invoice.setDefaultActivateParameterDict({})

  erp5/product/ERP5Type/Base.py(238)__call__()
    237       try:
--> 238         wf[wf_id].notifyWorkflowMethod(instance, transition_list, args=args, kw=kw)
    239       except ObjectDeleted:

  erp5/product/ERP5Type/patches/WorkflowTool.py(76)DCWorkflowDefinition_notifyWorkflowMethod()
     75             activate_kw = {}
---> 76         ob.reindexObject(activate_kw=activate_kw)
     77 

  erp5/product/ERP5/Document/Delivery.py(455)reindexObject()
    454       """
--> 455       self.recursiveReindexObject(*k, **kw)
    456       # do not reexpand simulation: this is a task for DSolver / TSolver

> erp5/product/ERP5Type/Core/Folder.py(1236)recursiveReindexObject()
   1235         import ipdb; ipdb.set_trace()
-> 1236       kw, activate_kw = self._getReindexAndActivateParameterDict(
   1237         kw,

ipdb> args
self = <Sale Invoice Transaction at 1>
activate_kw = {}
kw = {}

ipdb> l
   1231   security.declarePublic('recursiveReindexObject')
   1232   def recursiveReindexObject(self, activate_kw=None, **kw):
   1233     if self.isAncestryIndexable():
   1234       if '/erp5/accounting_module/1' == self.getPath():
   1235         import ipdb; ipdb.set_trace()
-> 1236       kw, activate_kw = self._getReindexAndActivateParameterDict(
   1237         kw,
   1238         activate_kw,
   1239       )
   1240       activate_kw['group_method_cost'] = 0.01
   1241       self._recurseCallMethod(

[Vincent Pelletier]

erp5/product/ERP5/Document/Delivery.py(455)reindexObject()

Ah, of course, these types override single-shot indexation to become recursive. How can I have forgotten this ?

-> 1236       kw, activate_kw = self._getReindexAndActivateParameterDict(

And I think the cullprit is this method: it should pull from default activate parameters when activate_kw is empty.

Could you try with this (the 2 first lines of _getReindexAndActivateParameterDict change):

  def _getReindexAndActivateParameterDict(self, kw, activate_kw):
    if not activate_kw:
      activate_kw = context.getDefaultActivateParameterDict()

BTW:

this is exactly what happens in _reindexObject as it calls _getReindexAndActivateParameterDict.

I was again not attentive enough to the reindex_kw vs. activate_kw duality: _getReindexAndActivateParameterDict does not check getDefaultActivateParameterDict, but does check getDefaultReindexParameterDict.

[Vincent Pelletier]

Actually, context.activate does more than the above... What about:

  def _getReindexAndActivateParameterDict(self, kw, activate_kw):
    full_activate_kw = context.getDefaultActivateParameterDict()
    full_activate_kw.update(activate_kw or ())
    activate_kw = full_activate_kw

[Kazuhiko Shiozaki]

Your patch above (with s/context/self/) solves the issue with my pseudo code above.

You can get why I call setDefaultActivateParameterDict()twice by comparing with the following code :

invoice = self.portal.accounting_module.newContent(portal_type='Sale Invoice Transaction', activate_kw={'tag': 'xxx'})
invoice.activate(after_tag='xxx').stop()

If we do like this in the same transaction, the following activities are generated :

2018-09-17 12:25:02.980 INFO Tracking queuing message: activity=SQLDict, object_path=/erp5/accounting_module/1, method_id=immediateReindexObject, args=(), kw={}, activity_kw={'alternate_method_id': 'alternateReindexObject', 'group_method_cost': 0.01, 'tag': 'xxx', 'serialization_tag': '/erp5/accounting_module/1', 'group_method_id': 'portal_catalog/catalogObjectList', 'group_id': ''}, user_name=ERP5TypeTestCase
...
2018-09-17 12:25:31.460 INFO Tracking queuing message: activity=SQLDict, object_path=/erp5/accounting_module/1, method_id=stop, args=(), kw={}, activity_kw={'tag': 'xxx', 'after_tag': 'xxx'}, user_name=ERP5TypeTestCase

where the last line has looping dependency. Is it expected behaviour ?

[Julien Muchembled]

invoice.setDefaultActivateParameterDict({'tag': tag})
invoice.start()
invoice.setDefaultActivateParameterDict({})

First of all, such pattern is unsafe. Assuming that using default activate parameters is correct, use:

with invoice.defaultActivateParameterDict({'tag': tag}):
  invoice.start()

edit: I mixed up _getReindexAndActivateParameterDict and what's passed as get_activate_kw_method_id. The former is called outside _recurseCallMethod so that's fine.

[Vincent Pelletier]

If we do like this in the same transaction, the following activities are generated :

Yeah, this is an old bug: newContent leaks the default activity parameters, leading to any tag being also applied to all activities spawned without an explicit tag for the rest of the transaction.

I think the pattern @jm proposes should be applied to constructInstance so that gets popped. This will require a few more shenanigans as we do not want to step on any existing default parameters if activate_kw is not provided (so we need a no-op context manager for that case). Also, reindex_kw must be made into a similar context manager, and with similar fallback to no-op. And then maybe we will be breaking accidentally-working code by not applying tags anymore...