control panel throws exception if accessed before addon upgrade step that adds field (controlpanel serializer so it handles schemas with new fields by returning field default)

[djay]

To reproduce.

• create an addon with a control panel using a schema
• install it
• create a new version of the addon that adds a new field to the schema
• startup plone
• control panel is now broken (until you go run an upgrade step)

Ultimately the issue is that schemas aren't versioned and they are loaded via code not configuration so there is no way to properly manage schema migration in plone/zope. This applies to dexterity schemas too.

In the absence of that, not breaking and returning something sensible when data is missing (the default) seems like a good way forward.

What this change does is make the restapi return the default defined in the schema if it can't find the data in the registry.
This change also means that in many cases you don't need an upgrade step for your addons that add fields. But until the control panel is saved again the value in the registry will still be missing. Not sure if that creates other problems.

Note this is a problem for classic as well (as pointed out by @fredvd ) so a similar fix might be needed there too?

[netlify]

✅ Deploy Preview for plone-restapi canceled.

Name	Link
🔨 Latest commit	6789907
🔍 Latest deploy log	https://app.netlify.com/sites/plone-restapi/deploys/656691ebf99d8e000941ceac

[mister-roboto]

@djay thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[davisagli]

It can be confusing when a default is calculated every time the field is read. For example if there's a field with a defaultFactory that returns the current userid, then it will look like a different value is stored depending on which user reads the value. For that reason, I would be inclined to require an explicit upgrade step if a new field is added that uses defaultFactory.

I would also suggest putting DefaultRecordsProxy in plone.registry so that it can be used by both plone.restapi and plone.api instead of duplicating it. That does make things a bit more complicated since plone.restapi needs to potentially handle old versions of Plone that don't have it.

[djay]

It can be confusing when a default is calculated every time the field is read. For example if there's a field with a defaultFactory that returns the current userid, then it will look like a different value is stored depending on which user reads the value.

Well it does only fix the case of accessing hte control panel so the user will see their name and if they hit save then it persist.
If the registry information is used elsewhere like in another public api, then that code will have to make other decisions about how to handle missing information.
I'm also not sure this scenario is that common. do you know of an example @davisagli ?

For that reason, I would be inclined to require an explicit upgrade step if a new field is added that uses defaultFactory.

Thats the case now that this is trying to solve. Control panels break until you upgrade your plugin which is confusing for admins. There is no nice explanation why, just a stack trace. And in addition its just extra work for the developer to go put in explicit upgrade steps that rerun your profile or add that specific missing data in the registry when they can just specify a default in the schema instead (the more intuitive thing to do)

A more complete solution is to have different versions of the schema interface linked to a particular installed version and use the old one until the upgrade step is run. Or maybe fields in a schema linked to a version? But all of this also adding more work for the developer.

Or we solve this by showing a nicer error to users asking them to upgrade the plugin?

I would also suggest putting DefaultRecordsProxy in plone.registry so that it can be used by both plone.restapi and plone.api instead of duplicating it. That does make things a bit more complicated since plone.restapi needs to potentially handle old versions of Plone that don't have it.

[davisagli]

Well it does only fix the case of accessing hte control panel so the user will see their name and if they hit save then it persist.

My concern is that they are seeing a value that is not actually persisted. If it's the value they want, they'll probably think they don't need to save.

It's not really a problem for a static default value, but more of a potential problem if there's a defaultFactory that returns different defaults at different times.

An unrelated question: It looks like the existing RecordsProxy already falls back to the field's missing_value. Is there a reason it doesn't work to use that when you add a new field?

[djay]

An unrelated question: It looks like the existing RecordsProxy already falls back to the field's missing_value. Is there a reason it doesn't work to use that when you add a new field?

The check=False would still need to be part of this change otherwise it won't work. The check is what make's it blow up currently.

You are right. the semantics of using this make more sense that using the default. Not sure developers would intuitively think to use missing_value but either way documentation would be needed.

[davisagli]

@djay Hmm, no, it's not that simple. It'll use the missing_value when reading, but throw an exception when trying to write the field if it hasn't been registered yet: https://github.com/plone/plone.registry/blob/master/plone/registry/recordsproxy.py#L46

The registry design stores a persistent field definition as part of a record; I expect it will be an uphill battle to get things to work correctly without an upgrade step to add records for the new fields.

[djay]

@djay Hmm, no, it's not that simple. It'll use the missing_value when reading, but throw an exception when trying to write the field if it hasn't been registered yet: https://github.com/plone/plone.registry/blob/master/plone/registry/recordsproxy.py#L46

You are right again. my test doesn't properly test setting since I didn't actually remove the value from the registry
This check could be changed to check against the schema however, instead of the registry?

The registry design stores a persistent field definition as part of a record; I expect it will be an uphill battle to get things to work correctly without an upgrade step to add records for the new fields.

It does? I wasn't aware. if so why can't the recordsproxy use that to check against instead of the current Interface? Then it will be checking against the schema that was there at the time it was installed which would also solve the problem I think?

EDIT: ok it wouldn't... because the control panel view would also have to display widgets etc based on the schema stored in the registry for this to work.

[djay]

@davisagli my test did delete the record so does show that the value can be set again in that case. I've changed it to use the missing value and the test passes.
So is that the solution we want?