plone / Products.CMFEditions

Provides versioning in Plone
5 stars 12 forks source link

Exception when saving on folderish type, hard to track #26

Open thet opened 9 years ago

thet commented 9 years ago

i got this exception on a folderish type document (folderish type, default page of a parent folder, has 4 images within):

Traceback (innermost last):

    Module ZPublisher.Publish, line 138, in publish
    Module ZPublisher.mapply, line 77, in mapply
    Module ZPublisher.Publish, line 48, in call_object
    Module plone.z3cform.layout, line 66, in __call__
    Module plone.z3cform.layout, line 50, in update
    Module plone.dexterity.browser.edit, line 62, in update
    Module plone.z3cform.fieldsets.extensible, line 59, in update
    Module plone.z3cform.patch, line 30, in GroupForm_update
    Module z3c.form.group, line 145, in update
    Module plone.app.z3cform.csrf, line 21, in execute
    Module z3c.form.action, line 98, in execute
    Module z3c.form.button, line 315, in __call__
    Module z3c.form.button, line 170, in __call__
    Module plone.dexterity.browser.edit, line 26, in handleApply
    Module z3c.form.group, line 126, in applyChanges
    Module zope.event, line 31, in notify
    Module zope.component.event, line 24, in dispatch
    Module zope.component._api, line 136, in subscribers
    Module zope.component.registry, line 321, in subscribers
    Module zope.interface.adapter, line 585, in subscribers
    Module zope.component.event, line 32, in objectEventNotify
    Module zope.component._api, line 136, in subscribers
    Module zope.component.registry, line 321, in subscribers
    Module zope.interface.adapter, line 585, in subscribers
    Module plone.app.versioningbehavior.subscribers, line 62, in create_version_on_save
    Module Products.CMFEditions.CopyModifyMergeRepositoryTool, line 305, in save
    Module Products.CMFEditions.CopyModifyMergeRepositoryTool, line 451, in _recursiveSave
    Module Products.CMFEditions.ArchivistTool, line 267, in prepare
    Module Products.CMFEditions.ArchivistTool, line 221, in _cloneByPickle

TypeError: Can't pickle objects in acquisition wrappers.

I placed a pdb in a try/except block here: Module Products.CMFEditions.ArchivistTool, line 221, in _cloneByPickle. It fails at this line p.dump(aq_base(obj)). So, here is a aq_base statement, which should return the object with all acquisition wrappers removed. When I run into this problem with pdb, I tried the same statement and it worked. Weired. I suspect it has something to do with the folderish structure, but on the other hand this is the first time I encounter this problem, even with many collective.folderishtypes deployments since years. However, I'm going to deactivate versioningbehavior for a quick fix.

vincentfretin commented 9 years ago

@cedricmessiant, I think we had this exact traceback, the reason was a field that contains RelationValue. How did we fix it, do you remember? @thet Do you have a RelationChoice on your content type by any chance?

Gagaro commented 9 years ago

I have the same issue with a content type with a RelationChoice.

Do you remember how you fixed this ?

cedricmessiant commented 9 years ago

When I look at the commit that seems related to the fix in our product, it seems that I just :

Gagaro commented 9 years ago

Thanks a lot, it works for me as well :+1: .

cedricmessiant commented 9 years ago

Cool !

ghost commented 8 years ago

I'm having the same issue and updating repositorytool.xml and diff_too.xml profiles (and running steps) does not help. In fact, I already had those steps installed for my dexterity content type. It doesn't make sense that this would matter here anyway. Do you know what version of dexterity and Plone you were using?

Gagaro commented 8 years ago

I was on either Plone 5.0b2 or 5.0. It was the default dexterity version.

2015-12-23 18:10 GMT+01:00 esoth notifications@github.com:

I'm having the same issue and updating repositorytool.xml and diff_too.xml profiles (and running steps) does not help. In fact, I already had those steps installed for my dexterity content type. It doesn't make sense that this would matter here anyway. Do you know what version of dexterity and Plone you were using?

— Reply to this email directly or view it on GitHub https://github.com/plone/Products.CMFEditions/issues/26#issuecomment-166946826 .

Gagaro

darryldixon commented 2 years ago

I have tracked this issue down to the fact that for some reason, CMFEditions is trying to pickle the OFS children of the folderish item. The error thrown "TypeError: Can't pickle objects in acquisition wrappers." is being emitted from C code in _Acquisition.c. I instrumented this code so that it would include a repr of the object it was complaining about. This showed that it was the sub-objects (OFS children) of the item being versioned that the error was being emitted for.

In other words, for some reason the OMOutsideChildrensModifier is not preventing child items from being included in the pickler stream. This is a CMFEditions/Dexterity bug, one way or the other - and furthermore, that those children are still wrapped in Acquisition wrappers when they enter the pickler.

darryldixon commented 2 years ago

For those that want to see this for themselves, you will need to manually patch the Acquisition egg and then insert in to your buildout, eg:

--- Acquisition-4.7/setup.py    2020-10-07 22:44:28.000000000 +1300
+++ Acquisition-4.7a/setup.py   2022-04-06 18:29:55.235552924 +1200
@@ -36,7 +36,7 @@
                   include_dirs=['include', 'src']),
     ]

-version = '4.7'
+version = '4.7a'

 setup(
     name='Acquisition',
diff -ru Acquisition-4.7/src/Acquisition/_Acquisition.c Acquisition-4.7a/src/Acquisition/_Acquisition.c
--- Acquisition-4.7/src/Acquisition/_Acquisition.c  2020-10-07 22:44:28.000000000 +1300
+++ Acquisition-4.7a/src/Acquisition/_Acquisition.c 2022-04-07 00:14:47.942013701 +1200
@@ -1477,8 +1477,16 @@
 PyObject *
 Wrappers_are_not_picklable(PyObject *wrapper, PyObject *args)
 {
+    PyObject *obj;
+    PyObject *repr;
+    /* C progammers disease... */
+    char msg[1024];
+    /* Unwrap wrapper completely -> obj. */
+    obj = get_base(wrapper);
+    repr = PyObject_Repr((PyObject *)obj);
+    snprintf(msg, 1024, "Can't pickle objects in acquisition wrappers - %s", (char *)PyBytes_AS_STRING(repr));
     PyErr_SetString(PyExc_TypeError,
-                    "Can't pickle objects in acquisition wrappers.");
+                    msg);
     return NULL;
 }

(A quick warning that this code probably isn't safe for a Production environment as I'm not certain what potential outputs repr might generate in all situations)

darryldixon commented 2 years ago

I have solved what is causing this issue but do not yet have a good solution:

Dexterity items that use z3c.relationfield.schema RelationList or RelationChoice, (eg, they store a RelationValue) will trigger this error. This is because RelationValue items use an attribute of __parent__ as well as from_object which points back to the original Dexterity item. When CMFEditions tries to clone the Dexterity item with the Python Pickler, the Pickler follows the __parent__ & from_object attribute back around to the original item, except this time wrapped in an Acquisition wrapper, which then fails with the error recorded above. Presumably the Pickler normally has mechanisms to avoid circular references; but because following the __parent__ & from_object results in the Acquisition wrapper, the Pickler attempts to pickle the wrapper, which throws TypeError.

darryldixon commented 2 years ago

Below is a CMFEditions modifier that solves the problem when applied to Dexterity items. It's mostly good but may some some corner-case gaps with Dexterity Behaviours that implement relation fields:

from z3c.relationfield.relation import RelationValue
from z3c.relationfield import RelationList, RelationChoice

@implementer(ICloneModifier, ISaveRetrieveModifier)
class DeconstructZ3CRelationValues(object):
    """Standard modifier to avoid trying to clone z3c.relationfield.relation.RelationValue items
    and instead to recreate them from first principals on load
    """

    def __init__(self, id='DeconstructZ3CRelationValues', title=''):
        self.id = id
        self.title = title

    def getOnCloneModifiers(self, obj):
        """Remove RelationValue items and store a marker
        """
        def persistent_id(obj):
            # Another possibility here maybe would be to store the _p_oid of the RelationValue and then refetch
            # later to fix up properly based on that?
            if isinstance(obj, RelationValue):
                rv_string = 'RelationValue-to_id:{}'.format(obj.to_id)
                if obj.from_attribute is not None:
                    rv_string += ',from_attribute:{}'.format(obj.from_attribute)
                return rv_string
            return None

        def persistent_load(relationvalue_string):
            if relationvalue_string.startswith('RelationValue-'):
                attribute_values = relationvalue_string.split('-', 1)[-1]
                to_id_part = attribute_values.split(',', 1)[0]
                to_id = int(to_id_part.split(':')[-1])
                y = RelationValue(to_id)
                if len(attribute_values.split(',', 1)) > 1:
                    # String also specifies the from_attribute
                    from_attribute_part = attribute_values.split(',', 1)[-1]
                    from_attribute = from_attribute_part.split(',', 1)[-1]
                    y.from_attribute = from_attribute
                return y
            return None

        return persistent_id, persistent_load, [], []

    def beforeSaveModifier(self, obj, clone):
        """Does nothing, the pickler does the work"""
        return {}, [], []

    def afterRetrieveModifier(self, obj, repo_clone, preserve=()):
        """Recreate the RelationValue from the original copy"""
        ptt = getToolByName(self, 'portal_types')
        ti = ptt.getTypeInfo(obj.portal_type)
        t_schema = ti.lookupSchema()
        t_behaviors = ti.behaviors
        related_ids = set()
        # TODO: Include t_behaviors in the below. This will involve manually parsing
        # the dotted names and importing and a bunch of other stuff to get instances.
        for fieldname, fieldvalue in schema.getFieldsInOrder(t_schema):
            if isinstance(fieldvalue, ListType):
                for v in fieldvalue:
                    if isinstance(v, RelationValue):
                        v.from_object = aq_base(aq_inner(repo_clone))
            elif isinstance(fieldvalue, RelationList):
                rl = fieldvalue.get(repo_clone)
                # Sometimes rl is None. Yes, really:
                if rl is not None:
                    for v in rl:
                        if isinstance(v, RelationValue):
                            v.from_object = aq_base(aq_inner(repo_clone))
            elif isinstance(fieldvalue, RelationValue):
                fieldvalue.from_object = aq_base(aq_inner(repo_clone))
            elif isinstance(fieldvalue, RelationChoice):
                rv = fieldvalue.get(repo_clone)
                if rv is not None:
                    rv.from_objet = aq_base(aq_inner(repo_clone))
        return [], [], {}

InitializeClass(DeconstructZ3CRelationValues)