From 2b06092a84b853aa0b6f8ec5b344d962f5ae4e27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Degrave?= Date: Sun, 20 Jun 2021 13:01:40 +0200 Subject: [PATCH 1/3] [FIX] base_rest_datamodel: remove incorrect validation of responses 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 trying to `load` data you just `dumped` can result in errors, 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 declared some fields to be `dump_only`, this validation necessarily crash. 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 used to serialize the data. [FIX] base_rest_datamodel: change response validation to use a schema with `unknown=EXCLUDE` This modification prevents fields that are declared as `dump_only` to mess with the validation (`dump_only` fields are seen as "unknown" during validation, since marshmallow validation is designed to be used during loading only). [FIX] base_rest_datamodel: change response validation to use a schema with `unknown=EXCLUDE` Nested model required extra care because the `unknown=EXCLUDE` is not passed down to the nested schemas when validating data, and this behavior could not easily be overridden. I ended up removing `dump_only` fields from value during `NestedModel` deserialization. [ADD] base_rest_demo: add test for new api [ADD] base_rest_demo: test [FIX] datamodel: allow to instantiate datamodel with dump_only fields as well [IMP] datamodel: improved 'validate' method, propagating 'unknown' value to nested schemas [IMP] base_rest_demo: rename test class --- base_rest_datamodel/restapi.py | 4 +- base_rest_demo/datamodels/partner_info.py | 2 +- base_rest_demo/tests/__init__.py | 1 + base_rest_demo/tests/test_new_api.py | 45 +++++++++++++++++++++++ datamodel/core.py | 40 +++++++++++++++++--- 5 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 base_rest_demo/tests/test_new_api.py diff --git a/base_rest_datamodel/restapi.py b/base_rest_datamodel/restapi.py index dc11150f4..449949075 100644 --- a/base_rest_datamodel/restapi.py +++ b/base_rest_datamodel/restapi.py @@ -37,7 +37,9 @@ def to_response(self, service, result): json = [i.dump() for i in result] else: json = result.dump() - errors = ModelClass.validate(json, many=self._is_list) + errors = ModelClass.validate( + json, many=self._is_list, unknown=marshmallow.EXCLUDE + ) if errors: raise SystemError(_("Invalid Response %s") % errors) return json diff --git a/base_rest_demo/datamodels/partner_info.py b/base_rest_demo/datamodels/partner_info.py index f8f727851..991596298 100644 --- a/base_rest_demo/datamodels/partner_info.py +++ b/base_rest_demo/datamodels/partner_info.py @@ -18,4 +18,4 @@ class PartnerInfo(Datamodel): phone = fields.String(required=False, allow_none=True) state = NestedModel("state.info") country = NestedModel("country.info") - is_componay = fields.Boolean(required=False, allow_none=False) + is_company = fields.Boolean(required=False, allow_none=False) diff --git a/base_rest_demo/tests/__init__.py b/base_rest_demo/tests/__init__.py index 12a4d7283..7fe57fd42 100644 --- a/base_rest_demo/tests/__init__.py +++ b/base_rest_demo/tests/__init__.py @@ -1,3 +1,4 @@ from . import test_controller from . import test_openapi from . import test_exception +from . import test_new_api diff --git a/base_rest_demo/tests/test_new_api.py b/base_rest_demo/tests/test_new_api.py new file mode 100644 index 000000000..4cf8cbe31 --- /dev/null +++ b/base_rest_demo/tests/test_new_api.py @@ -0,0 +1,45 @@ +# Copyright 2021 Wakari SRL +# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). + +import odoo.tests.common +from odoo.tests import new_test_user +from odoo.tests.common import HttpCase + +HOST = "127.0.0.1" +PORT = odoo.tools.config["http_port"] + + +@odoo.tests.common.at_install(False) +@odoo.tests.common.post_install(True) +class TestPartnerNewAPI(HttpCase): + def setUp(self): + super().setUp() + self.partner_data = { + "name": "Test Partner", + "country_id": self.env["res.country"].search([], limit=1).id, + "state_id": self.env["res.country.state"].search([], limit=1).id, + } + self.test_partner = self.env["res.partner"].create(self.partner_data) + self.password = "test_password" + self.user = new_test_user( + self.env, + login="test_user", + password=self.password, + groups="base.group_user", + ) + + @odoo.tools.mute_logger("odoo.addons.base_rest.http") + def test_01(self): + """Test GET""" + self.authenticate("admin", "admin") + # self.authenticate(self.user.login, self.password) + url = "http://{}:{}/base_rest_demo_api/new_api/partner/{}".format( + HOST, PORT, self.test_partner.id + ) + response = self.opener.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Content-Type"], "application/json") + data = response.json() + self.assertEqual(data["name"], self.partner_data["name"]) + self.assertEqual(data["country"]["id"], self.partner_data["country_id"]) + self.assertEqual(data["state"]["id"], self.partner_data["state_id"]) diff --git a/datamodel/core.py b/datamodel/core.py index b4e7cc5dc..576543898 100644 --- a/datamodel/core.py +++ b/datamodel/core.py @@ -5,6 +5,9 @@ import functools import logging from collections import OrderedDict, defaultdict +from contextlib import ExitStack + +from marshmallow import INCLUDE from odoo.api import Environment from odoo.tools import LastOrderedSet, OrderedSet @@ -41,12 +44,20 @@ def _get_addon_name(full_name): return addon_name +def _get_nested_schemas(schema): + res = [schema] + for field in schema.fields.values(): + if getattr(field, "schema", None): + res += _get_nested_schemas(field.schema) + return res + + class DatamodelDatabases(dict): """ Holds a registry of datamodels for each database """ class DatamodelRegistry(object): - """ Store all the datamodel and allow to retrieve them by name + """Store all the datamodel and allow to retrieve them by name The key is the ``_name`` of the datamodels. @@ -98,7 +109,7 @@ def __make_object__(self, data, **kwargs): class MetaDatamodel(ModelMeta): - """ Metaclass for Datamodel + """Metaclass for Datamodel Every new :class:`Datamodel` will be added to ``_modules_datamodels``, that will be used by the datamodel builder. @@ -131,9 +142,16 @@ def __init__(self, name, bases, attrs): self._modules_datamodels[self._module].append(self) + def __call__(self, *args, **kwargs): + """Allow to set any field (including 'dump_only') at instantiation + This is not an issue thanks to cleanup during (de)serialization + """ + kwargs["unknown"] = kwargs.get("unknown", INCLUDE) + return super().__call__(*args, **kwargs) + class Datamodel(MarshmallowModel, metaclass=MetaDatamodel): - """ Main Datamodel Model + """Main Datamodel Model All datamodels have a Python inheritance either on :class:`Datamodel`. @@ -207,9 +225,21 @@ def get_schema(cls, **kwargs): """ return cls.__get_schema_class__(**kwargs) + @classmethod + def validate(cls, data, context=None, many=None, partial=None, unknown=None): + schema = cls.__get_schema_class__( + context=context, partial=partial, unknown=unknown + ) + all_schemas = _get_nested_schemas(schema) + with ExitStack() as stack: + # propagate 'unknown' to each nested schema during validate + for nested_schema in all_schemas: + stack.enter_context(cls.propagate_unknwown(nested_schema, unknown)) + return schema.validate(data, many=many, partial=partial) + @classmethod def _build_datamodel(cls, registry): - """ Instantiate a given Datamodel in the datamodels registry. + """Instantiate a given Datamodel in the datamodels registry. This method is called at the end of the Odoo's registry build. The caller is :meth:`datamodel.builder.DatamodelBuilder.load_datamodels`. @@ -350,7 +380,7 @@ def _build_datamodel(cls, registry): @classmethod def _complete_datamodel_build(cls): - """ Complete build of the new datamodel class + """Complete build of the new datamodel class After the datamodel has been built from its bases, this method is called, and can be used to customize the class before it can be used. From 0a508ca27ccc5128c2ee4e29accb087ab78aab16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Degrave?= Date: Wed, 28 Jul 2021 16:38:34 +0200 Subject: [PATCH 2/3] [IMP][FIX] base_rest_datamodel: transform "HttpCase" tests into "DatamodelRegistryCase" and move them from base_rest_demo to base_rest_datamodel --- base_rest_datamodel/tests/__init__.py | 1 + base_rest_datamodel/tests/test_response.py | 80 ++++++++++++++++++++++ base_rest_demo/tests/__init__.py | 1 - base_rest_demo/tests/test_new_api.py | 45 ------------ 4 files changed, 81 insertions(+), 46 deletions(-) create mode 100644 base_rest_datamodel/tests/__init__.py create mode 100644 base_rest_datamodel/tests/test_response.py delete mode 100644 base_rest_demo/tests/test_new_api.py diff --git a/base_rest_datamodel/tests/__init__.py b/base_rest_datamodel/tests/__init__.py new file mode 100644 index 000000000..d5006168c --- /dev/null +++ b/base_rest_datamodel/tests/__init__.py @@ -0,0 +1 @@ +from . import test_response diff --git a/base_rest_datamodel/tests/test_response.py b/base_rest_datamodel/tests/test_response.py new file mode 100644 index 000000000..9a01f1c9a --- /dev/null +++ b/base_rest_datamodel/tests/test_response.py @@ -0,0 +1,80 @@ +# Copyright 2021 Wakari SRL +# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). +import marshmallow +import mock + +import odoo + +from odoo.addons.base_rest_datamodel import restapi +from odoo.addons.datamodel import fields +from odoo.addons.datamodel.core import Datamodel +from odoo.addons.datamodel.tests import common + + +@odoo.tests.common.at_install(False) +@odoo.tests.common.post_install(True) +class TestDataModel(common.DatamodelRegistryCase): + def _to_response(self, instance): + restapi_datamodel = restapi.Datamodel(instance._name) + mock_service = mock.Mock() + mock_service.env = self.env + return restapi_datamodel.to_response(mock_service, instance) + + def test_to_response(self): + class Datamodel1(Datamodel): + _name = "datamodel1" + + name = fields.String(required=True, allow_none=False) + + Datamodel1._build_datamodel(self.datamodel_registry) + instance = self.env.datamodels["datamodel1"](name="Instance 1") + res = self._to_response(instance) + self.assertEqual(res["name"], instance.name) + + def test_to_response_dump_only(self): + class Datamodel2(Datamodel): + _name = "datamodel2" + + name = fields.String(required=True, allow_none=False, dump_only=True) + + Datamodel2._build_datamodel(self.datamodel_registry) + schema = self.env.datamodels["datamodel2"].get_schema() + self.assertEqual(schema.unknown, "raise") + msg = r"{'name': \['Unknown field.'\]}" + with self.assertRaisesRegex(marshmallow.exceptions.ValidationError, msg): + # confirmation that "name" cannot be loaded + self.env.datamodels["datamodel2"].load({"name": "Failure"}) + instance = self.env.datamodels["datamodel2"](name="Instance 2") + res = self._to_response(instance) + self.assertEqual(res["name"], instance.name) + # schema 'unknown' is back to "raise" + self.assertEqual(schema.unknown, "raise") + + def test_to_response_dump_only_nested(self): + class Datamodel3(Datamodel): + _name = "datamodel3" + + child = fields.NestedModel("nested_datamodel") + + class NestedDatamodel(Datamodel): + _name = "nested_datamodel" + + name = fields.String(required=True, allow_none=False, dump_only=True) + + NestedDatamodel._build_datamodel(self.datamodel_registry) + Datamodel3._build_datamodel(self.datamodel_registry) + for datamodel_name in ("datamodel3", "nested_datamodel"): + schema = self.env.datamodels[datamodel_name].get_schema() + self.assertEqual(schema.unknown, "raise") + msg = r"{'name': \['Unknown field.'\]}" + with self.assertRaisesRegex(marshmallow.exceptions.ValidationError, msg): + # confirmation that child "name" cannot be loaded + self.env.datamodels["datamodel3"].load({"child": {"name": "Failure"}}) + child_instance = NestedDatamodel(name="Child Instance") + instance = self.env.datamodels["datamodel3"](child=child_instance) + res = self._to_response(instance) + self.assertEqual(res["child"]["name"], child_instance.name) + for datamodel_name in ("datamodel3", "nested_datamodel"): + schema = self.env.datamodels[datamodel_name].get_schema() + # schema 'unknown' is back to "raise" + self.assertEqual(schema.unknown, "raise") diff --git a/base_rest_demo/tests/__init__.py b/base_rest_demo/tests/__init__.py index 7fe57fd42..12a4d7283 100644 --- a/base_rest_demo/tests/__init__.py +++ b/base_rest_demo/tests/__init__.py @@ -1,4 +1,3 @@ from . import test_controller from . import test_openapi from . import test_exception -from . import test_new_api diff --git a/base_rest_demo/tests/test_new_api.py b/base_rest_demo/tests/test_new_api.py deleted file mode 100644 index 4cf8cbe31..000000000 --- a/base_rest_demo/tests/test_new_api.py +++ /dev/null @@ -1,45 +0,0 @@ -# Copyright 2021 Wakari SRL -# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). - -import odoo.tests.common -from odoo.tests import new_test_user -from odoo.tests.common import HttpCase - -HOST = "127.0.0.1" -PORT = odoo.tools.config["http_port"] - - -@odoo.tests.common.at_install(False) -@odoo.tests.common.post_install(True) -class TestPartnerNewAPI(HttpCase): - def setUp(self): - super().setUp() - self.partner_data = { - "name": "Test Partner", - "country_id": self.env["res.country"].search([], limit=1).id, - "state_id": self.env["res.country.state"].search([], limit=1).id, - } - self.test_partner = self.env["res.partner"].create(self.partner_data) - self.password = "test_password" - self.user = new_test_user( - self.env, - login="test_user", - password=self.password, - groups="base.group_user", - ) - - @odoo.tools.mute_logger("odoo.addons.base_rest.http") - def test_01(self): - """Test GET""" - self.authenticate("admin", "admin") - # self.authenticate(self.user.login, self.password) - url = "http://{}:{}/base_rest_demo_api/new_api/partner/{}".format( - HOST, PORT, self.test_partner.id - ) - response = self.opener.get(url) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.headers["Content-Type"], "application/json") - data = response.json() - self.assertEqual(data["name"], self.partner_data["name"]) - self.assertEqual(data["country"]["id"], self.partner_data["country_id"]) - self.assertEqual(data["state"]["id"], self.partner_data["state_id"]) From 026d354ad8ab47ae0c71926c4730b889ceeaaac7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Degrave?= Date: Wed, 28 Jul 2021 17:48:15 +0200 Subject: [PATCH 3/3] [IMP] base_rest_datamodel: run tests at install --- base_rest_datamodel/tests/test_response.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/base_rest_datamodel/tests/test_response.py b/base_rest_datamodel/tests/test_response.py index 9a01f1c9a..0424324f5 100644 --- a/base_rest_datamodel/tests/test_response.py +++ b/base_rest_datamodel/tests/test_response.py @@ -3,16 +3,12 @@ import marshmallow import mock -import odoo - from odoo.addons.base_rest_datamodel import restapi from odoo.addons.datamodel import fields from odoo.addons.datamodel.core import Datamodel from odoo.addons.datamodel.tests import common -@odoo.tests.common.at_install(False) -@odoo.tests.common.post_install(True) class TestDataModel(common.DatamodelRegistryCase): def _to_response(self, instance): restapi_datamodel = restapi.Datamodel(instance._name)