Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sneridagh
Copy link
Member

@sneridagh sneridagh commented Dec 4, 2021

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.

@mister-roboto
Copy link

@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!

@sneridagh sneridagh requested review from jensens, ericof and tisto December 4, 2021 11:28
@sneridagh
Copy link
Member Author

Proof that we are not handling defaults ^^^

@sneridagh sneridagh changed the title Revisit improve support for missing value if present Revisit improve support for zope.schema default and missing_value if present Dec 4, 2021
@jensens
Copy link
Member

jensens commented Dec 6, 2021

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.

@tiberiuichim
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants