Change `items` implementation (PersistentMapping or annotations)

[jean]

Currently items is stored as a simple dictionary. This will cause multiple problems. One partial workaround is making a deepcopy in getItem, but others remain. See e.g.

https://mail.zope.org/pipermail/zodb-dev/2004-January/006519.html https://mail.zope.org/pipermail/zodb-dev/2004-January/006524.html

For other approaches, see http://www.zodb.org/documentation/guide/prog-zodb.html#rules-for-writing-persistent-classes

I think annotations would also work, but I don't know enough about them to be sure: http://pypi.python.org/pypi/zope.annotation

[ebrehault]

items are now PersistentDict: https://github.com/plomino/Plomino/blob/github-main/Products/CMFPlomino/PlominoDocument.py#L86 and getItem returns a deepcopy: https://github.com/plomino/Plomino/blob/github-main/Products/CMFPlomino/PlominoDocument.py#L134 the associated test is here: https://github.com/plomino/Plomino/blob/github-main/Products/CMFPlomino/tests/plomino.txt#L89

Looks like it does not break anything for now, I guess it will be part of the next release.

And once again: thank you for identifying that :)

[jean]

:-)

The test reminds me that: "You can access item values as attributes:" That doesn't deepcopy, but I don't think it should be fixed. I think it should go away. (Be deprecated, losslessly migrated in Plomino apps, and then go away.)

It's leaky, and it confuses the expectations of normal attribute access. If possible, Plomino should have one obvious right way to do things, certainly for something basic like item access.

[ebrehault]

oh no, you are right, getattr does not use getItem :)

ok, anyway the deprecation sounds like a good idea but the migration of existing code might be tricky I need to think about it

[jean]

True ... in principle migration is easy (just change all doc.item into doc.getItem('item') if 'item' is found in getItems), but in practice there are too many possibilities, it can't be done by simple text replacement. Logging deprecation messages from __getattr__ is probably the best we can do.

[ebrehault]

yes moreover, the plomino index uses getattr to index items: https://github.com/plomino/Plomino/blob/github-main/Products/CMFPlomino/index/PlominoCatalog.py#L47

I guess we can replace it with getItem, but it must be done carefully, because in some contexts, the object is not a PlominoDocument but a CatalogBrain, so getattr will work, but getItem will not.

[jean]

I think it's worthwhile handling brains and documents differently. Or conversely, I think it's not so important to have all code work the same for both brains and documents.

[ebrehault]

oh no it is not, I agree, I just say that when removing getattr on PlominoDocument we must be sure we are not damaging the current PlominoIndex implementation (the catalog metadata is kind of tricky, and that's something I coded quite a long time ago now)

but I am not worried, the tests would break immediately if we touch any painful part