zope.app.principalannotation optimization broken
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Zope 3 |
Won't Fix
|
Undecided
|
Unassigned | ||
zope.principalannotation |
Confirmed
|
Undecided
|
Unassigned |
Bug Description
When creating a new set of annotations for a principal, zope.app.
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-
Changed in zope3: | |
status: | New → Fix Committed |
Changed in zope.principalannotation: | |
status: | New → Confirmed |
Changed in zope3: | |
status: | Confirmed → Won't Fix |
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.