[13.0][FIX] base_rest_datamodel: modify validation of responses to allow dump_only fields

[fdegrave]

In marshmallow, dump (serialization) and load (deserialization) operations using the same schema are not symmetrical in general. That is because fields can be declared as dump_only or load_only. That means that if you try to load data you just dumped it could fail, and vice-versa.

And that is exactly what is (was) wrong with this validation: the validate method on the ModelClass uses load to validate the data. That means that if you declare some fields to be dump_only, this validation necessarily crashes.

Moreover, it seems that this validation is not only incorrect but also useless (if it worked as intended), since it bases its verification on the very same schema as the one that was just used to serialize the data.

[lmignon]

@fdegrave Validation is IMO a requirement since I want to be sure that the data returned by the service are conform to the schema. I understand that the current one is not right but how can we provide such a validation.?

[fdegrave]

The simple fact that the schema is used to dump the data is already a validation. Here is a simple example:

from marshmallow import Schema, fields


class MySchema(Schema):
    my_string = fields.Str()
    my_integer = fields.Integer()
    

class DataClass:
    def __init__(self, my_string, my_integer):
        self.my_string = my_string
        self.my_integer = my_integer

Now let's dump a valid instance:

>>> instance_ok = DataClass(my_string="Yes", my_integer=5)
>>> MySchema().dump(instance_ok)
{'my_string': 'Yes', 'my_integer': 5}

As expected. Now let's do it with an instance that does not conform to the schema:

>>> instance_bad = DataClass(my_string=1, my_integer="No")
>>> MySchema().dump(instance_bad)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/fd/.virtualenvs/odoo/lib/python3.8/site-packages/marshmallow/schema.py", line 547, in dump
    result = self._serialize(processed_obj, many=many)
  File "/home/fd/.virtualenvs/odoo/lib/python3.8/site-packages/marshmallow/schema.py", line 515, in _serialize
    value = field_obj.serialize(attr_name, obj, accessor=self.get_attribute)
  File "/home/fd/.virtualenvs/odoo/lib/python3.8/site-packages/marshmallow/fields.py", line 318, in serialize
    return self._serialize(value, attr, obj, **kwargs)
  File "/home/fd/.virtualenvs/odoo/lib/python3.8/site-packages/marshmallow/fields.py", line 898, in _serialize
    ret = self._format_num(value)  # type: _T
  File "/home/fd/.virtualenvs/odoo/lib/python3.8/site-packages/marshmallow/fields.py", line 873, in _format_num
    return self.num_type(value)
ValueError: invalid literal for int() with base 10: 'No'

As you can see, validation is included in the dump method, since the schema is used during the serialization. No need to try to re-validate it afterwards.

[fdegrave]

However you are right that not everything is validated (see there for example). In the same thread, people have the exact same issue we are discussing here (link) but they don't come up with a solution (unfortunately)...

One thing that we could do is list the names of dump_only fields, and pass them to the ModelClass.validate method under the partial parameter (which can be a list of field names). Not ideal (because dump_only fields are not validated at all) but better than what we currently have (because at least it would not fail).

I'll try that tomorrow.

[fdegrave]

What they do there is instantiating the schema with a unknown=EXCLUDE parameter.

Therefore a second solution would be to add a new unknown parameter to validate (define in marshmallow_objects) and override it to modify the way the schema is instantiated. Here is the current method:

    @classmethod
    def validate(cls, data, context=None, many=None, partial=None):
        schema = cls.__get_schema_class__(context=context, partial=partial)
        return schema.validate(data, many=many, partial=partial)

And we could simply do:

    @classmethod
    def validate(cls, data, context=None, many=None, partial=None, unknown=None):
        schema = cls.__get_schema_class__(context=context, partial=partial, unknown=unknown)
        return schema.validate(data, many=many, partial=partial)

And modify the current validation with:

from marshmallow import EXCLUDE

...

errors = ModelClass.validate(json, many=self._is_list, unknown=EXCLUDE)

Since after some thoughts I doubt the first solution would work, I'll try this one tomorrow.

[fdegrave]

I implemented my latest proposition, but it was trickier than I though because of nested models (no control on the schema for those, no way to easily override them during validation -- had to be creative, I think it's the cleanest I can do).

[fdegrave]

I rebased my branch after the fix of the tests have been merged. Hopefully everything should work OK.

[fdegrave]

Nearly there :-D

[fdegrave]

Victory!

[lmignon]

@fdegrave Thank you for this fix and all your experiments to provide the right one. First of all, feel free to add your name into the list of contributors!
Before merging I propose to add some tests and change the existing one (see my comments)

[fdegrave]

Is there anything still preventing the merge?

[lmignon]

/ocabot merge patch

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-169-by-lmignon-bump-patch, awaiting test results.

[lmignon]

times to merge a pending merge for a long time

[OCA-git-bot]

Congratulations, your PR was merged at 5362630. Thanks a lot for contributing to OCA. ❤️