PLIP: (WIP) Feature flag to disable implicit enabled behavior attribute access on Dexterity objects

[datakurre]

PLIP: (WIP) Remove implicit attribute default value access on Dexterity objects

Responsible Persons

Proposer: Asko Soukka

Seconder:

Abstract

Dexterity objects have magical __getattr__ implemented in their base class, which provides implicit default value lookup for all main and behavior schema fields (even for those fields, which are not really stored directly as an object attribute).

This PLIP is about removing that __getattr__ and minimizing the consequences.

Motivation

Implicit default values directly on the objects have the following major issues:

• z3c.forms will save and create a real attribute on the edited object only if the new value is different from the implicitly available default value; when default value is changed afterwards, it will look like all the existing content without changed value have magically updated

• path expressions are surprisingly expensive, because e.g. path expression nocall:context/portal_catalog will go through all schemas and fields of each level before finally reaching the site root and finding the catalog object

Assumptions

Proposal & Implementation

• Disabling __getattr__ with an environmental variable
• Fixing missing attributes on Dexterity DefaultView with default value adapter
• Migration tool for setting missing attributes when schema changes
• ...

https://github.com/plone/plone.dexterity/compare/datakurre-no-getattr

Deliverables

Risks

When new fields are introduced or new behaviors enabled for existing content, views may break, because they expect those attributes to exist automatically. A good example is enabling lead image behavior for a content type with existing content. A few viewlets will break. Many custom add-ons (private and public) may break.

Participants

[datakurre]

@jensens I'm not yet sure, if I'll complete this into a real PLIP, but it's good to have a place to discuss this. My current implementation is at: https://github.com/plone/plone.dexterity/compare/datakurre-no-getattr

A most obvious side-effect seen so far is that, if you enable lead image behavior for existing content, a few places will break, because they expect image attribute on the object. I've included a helper for setting defaults, but I doubt it can be made automatic because of unexpected cost of such operation. I'll update PLIP once, I have more experience on this change.

[jensens]

@datakurre I think if this works out as a better and more performant solution this should be pliped. Overall I would propose to have one explicit way it's done, the switch is fine for now, i.e in order to check the new way in an existing environment, but this should not rely on a global setting - too many side effects possible. If there are problems with i.e. lead image, we should solve it there.

It also solves a currently existing issue with indexed calculated default values: If the calculated value is dynamic (i.e. a date), the index contains the value at index time (am I right here)?

One thing we should think about is, that it may blow up the database by size. If you do measurements please have a look here too (also what this means by in pickle size, over-network load time, unpickle time and memory consumption for DB-caches. These are factors increasing access time as well. At the end, we may want to have a solution, where we can define on a per-field basis if we want to calculate a default value every time on access or to store it in the DB on save (which is probably tricky to do efficiently)?

Overall a difficult to address problem with need of dexterity (in its original meaning)

[rnixx]

Related:

http://plone.293351.n2.nabble.com/RFC-Proposal-Dexterity-API-two-variants-td7572793.html

https://github.com/plone/plone.dexterity/tree/dx_api

[datakurre]

@jensens Thanks. I renamed this PLIP to be about removing implicit attribute completely. My current implementation will keep having the flag until all issues are resolved (or PLIP abandoned).

About indexing. If object does not have the indexed attribute, it could still be found through acquisition (which is a real issue!) or attribute error is raised and nothing is indexed. But this does fix the issue, that when add or edit form is saved without changing defaults, those default values are saved as object attributes – previously they were not saved, but getattr continued to get defaults from schemas for those attributes.

I don't fully understand the issue about db size. Relying on schema defaults and not save all attributes on object on save only to save in pickle size sounds like an anti-pattern for me. I've always thought that it was a bug, not a feature. Any tips, how could I measure the difference? It's hard to believe that Python code to potentially loop through all schemas to find out the default could be faster than C-optimized pickle.

Bigger related issue is that if there is no attribute on object, implicit acquisition may get unexpected results.

FYI: There's still a TTW way to get the defaults, but it may not be obvious. The Dexterity default view is technically a z3c.form display form, so I could fix it to still show default values by registering a default value adapter for it. Those values should be available TTW via widgets traverser of the view.

[thet]

/cc @lukasgraf referring to our discussion in bern.

partly related to:
4teamwork/opengever.core#1778
4teamwork/opengever.core#2118

[lukasgraf]

@thet thanks for the ping!

One thing that immediately comes to mind is that plone.behavior.annotation.AnnotationsFactoryImpl has a very similar __getattr__ fallback in place. That one only falls back to missing_value though, not default values.

[datakurre]

But isn’t that correct behavior? AFAIK default value is a feature for add forms. We mixed things by making it implicit value for attributes.

…

On 12 Jul 2018, 12.28 +0300, Lukas Graf ***@***.***>, wrote: @thet thanks for the ping! One thing that immediately comes to mind is that plone.behavior.annotation.AnnotationsFactoryImpl has a very similar __getattr__ fallback in place. That one only falls back to missing_value though, not default values. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[lukasgraf]

But isn’t that correct behavior?

Maybe. But at the very least, it's currently not constistent with what DexterityContent.__getattr__ does. In my opinion falling back (on __getattr__) to missing_value (but not default values) can be viewed as the correct behavior. I was just pointing it out as something to consider, so that, which ever way it might be changed with this PLIP, we end up with consistent behavior across storage implementations.

Basically your proposed fix (no magical default value fallback for __getattr__) will solve the problem that this check in z3c.form doesn't get fooled anymore, because the new value (default) and the existing value (not set -> fallback -> default) look the same. For most cases.

But if there is still a fallback in place for missing value, this check could still be fooled in the case where default == missing_value, and you'd still end up with non-persisted values.

So, I would argue, it's really that check in z3c.form that's misguided. What is that even? A performance optimization? Shouldn't something like that be handled at the transaction / DataManager level?

AFAIK default value is a feature for add forms.

That I disagree with though ;) Default values IMHO should be a concern of the data model. So that's zope.schema in our case, with its default and defaultFactory keyword arguments for fields. Unfortunately, those don't get respected by any content creation method other than forms - both createContentInContainer() and invokeFactory() simply ignore them.

[jensens]

cc @davisagli IIRC you also heavily worked into this topic of default values, may you comment on the above?

[datakurre]

David has fixed the issue related to default values preventing z3c.form to save equal value a long time ago. We’ve been running Plone 5 without this implicit default value lookup for 1,5 years for now. We do have the issue that when we enable new behaviors in fly, we see errors, be because code for those (core) behaviors expect the new attributes to be there.

…

On 12 Jul 2018, 18.55 +0300, Jens W. Klein ***@***.***>, wrote: cc @davisagli IIRC you also heavily worked into this topic of default values, may you comment on the above? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[lukasgraf]

Don't get me wrong, I don't oppose you proposal - quite the contrary actually ;). I just wasn't aware of that particular fix by David (plone/plone.dexterity#67), which indeed takes care of the z3c.form related issue.

Increase in DB size isn't much of a worry to me personally. A more noticeable consequence for us is that without the implicit default value lookup, introducing a new field with a default means having to run an upgrade step (*) that updates possibly hundreds of thousands of objects, and possibly indexes that field. That can take a while to execute. But that's a price I'm happy to pay for persistence of these values.

(*) That is, if you need your existing objects to also have the value of whatever the default is when the new field is introduced. For some people it might be fine to just apply the default for newly created objects.

[datakurre]

Yes. This has significant risks. This is why we should still keep thinking how to overcome the risks: https://community.plone.org/t/why-is-plone-4-3-faster-than-plone-5-1/3808/8