rename the annotations attribute

[d-maurer]

Fixes #15.

attribute.AttributeAnnotations currently stores the annotations of an object in the attribute __annotations__. This may conflict with Python's special attribute __annotations__, e.g. used for type hints.

The PR renames __annotations__ to _zope_annotations, provides migration code (at least for the simple cases not involving slots) and introduces an optional callback notify_object_changed to inform an interested observer (e.g. plone.protect) about changes to the object.

[jamadden]

I am not a fan of the implicit migration, especially at creation time. I don't like read-only transactions that suddenly become read-write without my knowledge. And in my case, they won't work anyway: an HTTP GET request isn't allowed to modify the database and any transaction it starts is rolled back. So this winds up doing pointless work over and over.

I also don't want to rename the attribute. I have millions of objects with annotations. I don't want to rewrite my database for the hypothetical situation that __annotations__ on an instance becomes important. One could make the same argument about __name__ (it has a meaning on classes, but we use it for a different purpose on instances); we're not going to change that, I don't see why we should change __annotations__.

I don't think that a notification hook at this level is appropriate. It seems very far out of scope for this issue in particular, and zope.annotation in general.

I support the minimum change to make sure we're using the instance __annotations__ and not the type's annotations. (Which, to be clear, should be spelled type(obj).__dict__.get('__annotations__') and not type(obj).__annotations__ to work correctly on Python < 3.10, and to avoid bloating memory on Python >= 3.10.)

[d-maurer]

The check to detect the need for migration is incomplete.

Currently, I think the best approach looks as follows:
Migrate only in the persistent case. In the persistent case, the __annotations__ type is almost surely an OOBTree. In this case, it was not created by Python proper.

[jamadden]

I disagree. All that a non-BTree annotation means is that the BTrees package wasn't available when the annotation was created. There's no requirement for it to be a BTree, and there are uses of persistence outside of ZODB.

[d-maurer]

Jason Madden wrote at 2024-12-10 06:02 -0800:

I am not a fan of the implicit migration, especially at creation time.

I see an easy possibility to delay the migration until the annotations are changed. This would avoid new write transactions.

[d-maurer]

The PR now tries an alternative approach to distinguish the cases "annotations from class" and "__annotations from instance": it directly checks the instance.
If the instance dict contains the right __annotations__ or if there is an __annotations__ slot, it assumes "annotations from instance", otherwise "annotations from class". Weird descriptors may still render this approach unreliable but it is unlikely that they are used in the real world.

Potentially, the check is sufficiently fast to be used also without renaming the attribute.

[icemac]

I am also not a big fan of renaming the attribute. If we have a way without this I'd prefer it.

[d-maurer]

Michael Howitz wrote at 2024-12-13 09:25 -0800:

I am also not a big fan of renaming the attribute. If we have a way without this I'd prefer it.

The PR is based on a test "can we trust an existing obj.__annotations__`. If it gives true, it migrates. We can use a similar test "annotations invalid" instead in the access methods and avoid the migration.

[ale-rt]

Can we have this as a class attribute?

That would make it easier to come up with a customized adapter that uses another value.

I am BTW not sure this is a use case for anybody :)

[d-maurer]

Alessandro Pisa wrote at 2024-12-14 00:44 -0800:

@ale-rt commented on this pull request. > @@ -30,16 +31,22 @@ +ATTR = "_zope_annotations" Can we have this as a class attribute? That would make it easier to come up with a customized adapter that uses another value.

Sure. But why would one want to use a specific name? Apparently, there is a strong wish to keep the current attribute name `__annoatations__` (to avoid a migration). Therefore, this PR will likely not get merged.