plone.restapi icon indicating copy to clipboard operation
plone.restapi copied to clipboard

Revisit improve support for zope.schema `default` and `missing_value` if present

Open sneridagh opened this issue 4 years ago • 4 comments

Story:

Volto only sends the "Changed" values to p.restapi, so it doesn't has to send (and set) over and over again all the content type data. Then, zope.schema default and missing_value are not observed correctly.

Since it was a "hidden" problem, people tended to use default whenever necessary, when in fact, they really wanted to mean missing_value. In some fields like List-ish fields, that lead to a "Constrain not satisfied" error, forcing the developer to misuse default.

This PR tries to fix the problem. #1283 tried to fix it, improving and adding proper support in p.restapi for them. However, in the wild, there are combinations of default and missing_value that are not consistent or non-sense that broke the overall deserialisation for these kind of fields.

Language (Dublin Core)

This is how the language field is defined in plone.app.dexterity:

    language = schema.Choice(
        title=_(u'label_language', default=u'Language'),
        vocabulary='plone.app.vocabularies.SupportedContentLanguages',
        required=False,
        missing_value='',
        defaultFactory=default_language,
    )

It has both a defaultFactory AND a missing_value. Also, missing_value is a null-ish/false-ish value, which are always complex to handle.

Defaults must have precedence over missing_value and take into account null-ish/false-ish values.

The code gets hairy, and complex, but having defaults and missing_values out of the equation is not good for RESTAPI, so we need to find the correct implementation and a good set of tests/QA and a dozen of eyes over it.

Leaving this here, so we can handle it whenever we have time. But for sure it's a sprint level endeavour.

sneridagh avatar Dec 04 '21 10:12 sneridagh

@sneridagh thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

mister-roboto avatar Dec 04 '21 10:12 mister-roboto

Proof that we are not handling defaults ^^^

sneridagh avatar Dec 04 '21 11:12 sneridagh

Ok, I drop here the the definition of both first - I need to wrap my head around the whole... https://zopeschema.readthedocs.io/en/latest/narr.html#data-modeling-concepts

Whether or not the field is required
If a field is designated as required, assigned field values must always be non-missing. See the next section for a description of missing values.

A value designated as missing Missing values, when assigned to an object, indicate that there is ‘no data’ for that field. Missing values are analogous to null values in relational databases. For example, a boolean value can be True, False, or missing, in which case its value is unknown.

While Python’s None is the most likely value to signify ‘missing’, some fields may use different values. For example, it is common for text fields to use the empty string (‘’) to signify that a value is missing. Numeric fields may use 0 or -1 instead of None as their missing value.

A field that is ‘required’ signifies that missing values are invalid and should not be assigned

A default value Default field values are assigned to objects when they are first created. A default factory can be specified to dynamically compute default values.

jensens avatar Dec 06 '21 21:12 jensens

missing = "I'm empty but here's my value".

I can understand the case where you're creating an object, it comes with a default value, but then the user decides that the default value is wrong and instead "empty" should be the real value. But what is actually the use case for missing? It seems to me that "missing = default" would seem like the sane value all the time.

tiberiuichim avatar Dec 07 '21 06:12 tiberiuichim