[sneridagh]

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]

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

Proof that we are not handling defaults ^^^

[jensens]

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]

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.

[ericof]

@sneridagh Should we keep this one open?

[read-the-docs-community]

Documentation build overview

📚 plone.restapi | 🛠️ Build #35814195 | 📁 Comparing 98cdf8a against latest (497756e)

  🔍 Preview build  

1 file changed

± usage/content.html

[sneridagh]

@ericof TBH, if in 5 years we did not care about it... I think it popped while trying to work different combinations...
However, taking a look at the tests response on the PR files... the language is no longer empty string, but the correct lang, which is an improvement, so you don't have to guess again.
We should give Claudinho a try on this :) and see the outcome. Also, it might be a breaking change too, of course...

[ericof]

Thanks for this, @sneridagh. I merged main into the branch to see where it stands today. The PR's own tests pass on 6.0, 6.1 and 6.2; the red jobs are unrelated (no Python 3.8/3.9 on the new Ubuntu runner, an apt DNS failure, and drift in querystring_get.resp).

The problem is real, and the language improvement you pointed out is worth having. Still, I'm closing this PR rather than reviving it, because the 2021 patch would need a rewrite anyway:

1. It always calls defaultFactory(self.context). A plain, non-context-aware factory (e.g. defaultFactory=lambda: ...) on an optional field missing from the POST raises TypeError. The code right below it on main already distinguishes IContextAwareDefaultFactory.
2. It writes defaults and missing_value without the canWrite() and write-permission checks the regular branch applies, so it can set fields the user is not allowed to edit.
3. It checks field.default is not None but then elif field.default:, so falsy defaults (False, 0) pass the check and are never written.
4. It writes values and marks fields as modified inside the validate_all validation branch, which mixes two concerns in code that was just refactored in Share schema and value conversion helpers #2071.
5. It changes the shape of documented responses (language goes from "" to {"token": "en", ...}). That is a breaking change for API clients, not a bugfix.

If we pick this up again, I'd do it as a fresh PR on current main: on creation only, fill in missing writable fields from their defaults before deserializing, using the same permission and factory handling as submitted fields, with tests for language, a plain-lambda factory, falsy defaults and a field the user cannot write, plus a breaking-change news entry.