zope.app.principalannotation optimization broken

[Gary Poster]

When creating a new set of annotations for a principal, zope.app.principalannotation tries to be clever by creating a mapping that does not save itself in the utility until a __setitem__ is performed on itself. This causes "mysterious" errors in the following situation:

1. code gets principal annotations for a new principal
2. code checks for key in annotations
3. key is missing
4. code calls another function to populate key
5. other function gets principal annotations
6. other function stores value in annotations and returns
7. original code stores value in annotations

In this scenario, the principal annotation mapping in step 1 is a different object than in step 5. Step 6 saves the second one. Step 7 saves the first one, overwriting and losing the data from step 6.

All of the solutions to this that keep the optimization that I see are too complex. I suggest ripping out the optimization. For applications in which avoiding this potentially unnecessary write is essential, I suggest using the ``hasAnnotations`` method of the principal annotation utility directly before getting the annotations.

-----------------

In the hope that listing these saves time down the road, here are the two solutions I see that keep the optimization. Again, I reject both of them as too complex.

- Keep a separate data structure on the principal annotation utility of created annotations that have not yet been saved, and check that when handing out annotations. I see one primary downside. You presumably don't want to store these in a persistent structure, because the point of the optimization is to only write to the DB if necessary. Storing the mapping on an _v_ attribute of an object--say, the principal annotation utility--that has not been modified or created since the beginning of the current transaction is dangerous, because the value may be lost if it is evicted from the cache. Further approaches such as using precommit hooks and other tricks to clean out temporary stashes seem even more questionable.

- Have the __setitem__ honor data already in the annotation utility--effectively, step 7 would notice that other code had stored values, and do an update rather than an override. Maybe it would even then replace its own ``data`` with the one actually stored in the utility, so further changes would be transparent. Downsides include the following. The synchronization would only happen on __setitem__ not __getitem__. Object identity would be different. It feels too tricky.

[Dan Korostelev (nadako)]

I uploaded a fix in rev 93756. It's based on removing cache object from the state on utilitiy's __getstate__ method. Are there any problems with that? Please, review.

[Dan Korostelev (nadako)]

However, after some research, looks like I just implemented the same as _v_ attribute in a non-standard way and that won't fix a problem. :-) I'll look at it some more time and revert my changes :) Sorry for bothering.