Refactor DataModel with a strict type declaration

platformio/commands/lib.py

@@ -25,6 +25,7 @@ from platformio import exception, util
 from platformio.commands import PlatformioCLI
 from platformio.compat import dump_json_to_unicode
 from platformio.managers.lib import LibraryManager, get_builtin_libs, is_builtin_lib
+from platformio.package.manifest.model import ManifestModel
 from platformio.package.manifest.parser import ManifestFactory
 from platformio.proc import is_ci
 from platformio.project.config import ProjectConfig
@@ -493,8 +494,8 @@ def lib_register(config_url):
     if not config_url.startswith("http://") and not config_url.startswith("https://"):
         raise exception.InvalidLibConfURL(config_url)
 
-    manifest = ManifestFactory.new_from_url(config_url)
-    assert set(["name", "version"]) & set(list(manifest.as_dict()))
+    model = ManifestModel(**ManifestFactory.new_from_url(config_url).as_dict())
+    assert set(["name", "version"]) & set(list(model.as_dict()))
 
     result = util.get_api_result("/lib/register", data=dict(config_url=config_url))
     if "message" in result and result["message"]:

platformio/datamodel.py

@@ -26,6 +26,11 @@ class DataModelException(PlatformioException):
     pass
 
 
+class ListOfType(object):
+    def __init__(self, type):
+        self.type = type
+
+
 class DataField(object):
     def __init__(
         self,
@@ -47,6 +52,8 @@ class DataField(object):
         self.validate_factory = validate_factory
         self.title = title
 
+        self._parent = None
+        self._name = None
         self._value = None
 
     def __repr__(self):
@@ -55,36 +62,53 @@ class DataField(object):
             self.default if self._value is None else self._value,
         )
 
-    def validate(self, value, parent, attr):
-        if self.title is None:
-            self.title = attr.title()
+    def validate(self, parent, name, value):
+        self._parent = parent
+        self._name = name
+        self.title = self.title or name.title()
+
         try:
             if self.required and value is None:
-                raise ValueError("Required field, value is None")
+                raise ValueError("Required field `%s` is None" % name)
             if self.validate_factory is not None:
-                value = self.validate_factory(value)
+                return self.validate_factory(self, value) or self.default
             if value is None:
                 return self.default
-            if issubclass(self.type, (str, list, bool)):
+            if inspect.isclass(self.type) and issubclass(self.type, DataModel):
+                return self.type(**value).as_dict()
+            if isinstance(self.type, ListOfType):
+                return self._validate_list_of_type(self.type.type, value)
+            if issubclass(self.type, (str, bool)):
                 return getattr(self, "_validate_%s_value" % self.type.__name__)(value)
-        except (AssertionError, ValueError) as e:
+        except ValueError as e:
             raise DataModelException(
-                "%s for %s.%s" % (str(e), parent.__class__.__name__, attr)
+                "%s for %s.%s" % (str(e), parent.__class__.__name__, name)
             )
         return value
 
+    def _validate_list_of_type(self, list_of_type, value):
+        if not isinstance(value, list):
+            raise ValueError("Value should be a list")
+        if isinstance(list_of_type, DataField):
+            return [list_of_type.validate(self._parent, self._name, v) for v in value]
+        assert issubclass(list_of_type, DataModel)
+        return [list_of_type(**v).as_dict() for v in value]
+
     def _validate_str_value(self, value):
         if not isinstance(value, string_types):
             value = str(value)
-        assert self.min_length is None or len(value) >= self.min_length, (
-            "Minimum allowed length is %d characters" % self.min_length
-        )
-        assert self.max_length is None or len(value) <= self.max_length, (
-            "Maximum allowed length is %d characters" % self.max_length
-        )
-        assert self.regex is None or re.match(
-            self.regex, value
-        ), "Value `%s` does not match RegExp `%s` pattern" % (value, self.regex)
+        if self.min_length and len(value) < self.min_length:
+            raise ValueError(
+                "Minimum allowed length is %d characters" % self.min_length
+            )
+        if self.max_length and len(value) > self.max_length:
+            raise ValueError(
+                "Maximum allowed length is %d characters" % self.max_length
+            )
+        if self.regex and not re.match(self.regex, value):
+            raise ValueError(
+                "Value `%s` does not match RegExp `%s` pattern" % (value, self.regex)
+            )
         return value
 
     @staticmethod
@@ -95,60 +119,21 @@ class DataField(object):
 
 
 class DataModel(object):
-    __PRIVATE_ATTRIBUTES__ = ("__PRIVATE_ATTRIBUTES__", "_init_type", "as_dict")
-
-    def __init__(self, data=None):
-        data = data or {}
-        assert isinstance(data, dict)
-
-        for attr, scheme_or_model in get_class_attributes(self).items():
-            if attr in self.__PRIVATE_ATTRIBUTES__:
+    def __init__(self, **kwargs):
+        self._known_attributes = []
+        for name, field in get_class_attributes(self).items():
+            if not isinstance(field, DataField):
                 continue
-            if isinstance(scheme_or_model, list):
-                assert len(scheme_or_model) == 1
-                if data.get(attr) is None:
-                    setattr(self, attr, None)
-                    continue
+            self._known_attributes.append(name)
+            setattr(self, name, field.validate(self, name, kwargs.get(name)))
 
-                if not isinstance(data.get(attr), list):
-                    raise DataModelException("Value should be a list for %s" % (attr))
-                setattr(
-                    self,
-                    attr,
-                    [
-                        self._init_type(scheme_or_model[0], v, attr)
-                        for v in data.get(attr)
-                    ],
-                )
-            else:
-                setattr(
-                    self, attr, self._init_type(scheme_or_model, data.get(attr), attr)
-                )
-
-    def __repr__(self):
-        attrs = []
-        for name, value in get_class_attributes(self).items():
-            if name in self.__PRIVATE_ATTRIBUTES__:
-                continue
-            attrs.append('%s="%s"' % (name, value))
-        return "<%s %s>" % (self.__class__.__name__, " ".join(attrs))
-
-    def _init_type(self, type_, value, attr):
-        if inspect.isclass(type_) and issubclass(type_, DataModel):
-            return type_(value)
-        if isinstance(type_, DataField):
-            return type_.validate(value, parent=self, attr=attr)
-        raise DataModelException("Undeclared or unknown data type for %s" % attr)
+    # def __repr__(self):
+    #     attrs = []
+    #     for name, value in get_class_attributes(self).items():
+    #         if name in self.__PRIVATE_ATTRIBUTES__:
+    #             continue
+    #         attrs.append('%s="%s"' % (name, value))
+    #     return "<%s %s>" % (self.__class__.__name__, " ".join(attrs))
 
     def as_dict(self):
-        result = {}
-        for name, value in get_class_attributes(self).items():
-            if name in self.__PRIVATE_ATTRIBUTES__:
-                continue
-            if isinstance(value, DataModel):
-                result[name] = value.as_dict()
-            elif value and isinstance(value, list) and isinstance(value[0], DataModel):
-                result[name] = value[0].as_dict()
-            else:
-                result[name] = value
-        return result
+        return {name: getattr(self, name) for name in self._known_attributes}

platformio/package/manifest/model.py

@@ -14,7 +14,7 @@
 
 import semantic_version
 
-from platformio.datamodel import DataField, DataModel
+from platformio.datamodel import DataField, DataModel, ListOfType
 
 
 class AuthorModel(DataModel):
@@ -31,27 +31,36 @@ class RepositoryModel(DataModel):
 
 
 class ExportModel(DataModel):
-    include = [DataField()]
-    exclude = [DataField()]
+    include = DataField(type=ListOfType(DataField()))
+    exclude = DataField(type=ListOfType(DataField()))
 
 
 class ManifestModel(DataModel):
 
+    # Required fields
     name = DataField(max_length=100, required=True)
     version = DataField(
-        required=True,
         max_length=50,
-        validate_factory=lambda v: v if semantic_version.Version.coerce(v) else None,
+        validate_factory=lambda field, value: value
+        if semantic_version.Version.coerce(value)
+        else None,
+        required=True,
     )
-
-    description = DataField(max_length=1000)
-    keywords = [DataField(max_length=255, regex=r"^[a-z][a-z\d\- ]*[a-z]$")]
-    authors = [AuthorModel]
+    description = DataField(max_length=1000, required=True)
+    keywords = DataField(
+        type=ListOfType(DataField(max_length=255, regex=r"^[a-z][a-z\d\- ]*[a-z]$")),
+        required=True,
+    )
+    authors = DataField(type=ListOfType(AuthorModel), required=True)
 
     homepage = DataField(max_length=255)
     license = DataField(max_length=255)
-    platforms = [DataField(max_length=50, regex=r"^[a-z\d\-_\*]+$")]
-    frameworks = [DataField(max_length=50, regex=r"^[a-z\d\-_\*]+$")]
+    platforms = DataField(
+        type=ListOfType(DataField(max_length=50, regex=r"^([a-z\d\-_]+|\*)$"))
+    )
+    frameworks = DataField(
+        type=ListOfType(DataField(max_length=50, regex=r"^([a-z\d\-_\*]+|\*)$"))
+    )
 
-    repository = RepositoryModel
-    export = ExportModel
+    repository = DataField(type=RepositoryModel)
+    export = DataField(type=ExportModel)

platformio/package/manifest/parser.py

@@ -21,7 +21,6 @@ import requests
 from platformio.compat import get_class_attributes, string_types
 from platformio.exception import PlatformioException
 from platformio.fs import get_file_contents
-from platformio.package.manifest.model import ManifestModel
 
 try:
     from urllib.parse import urlparse
@@ -86,8 +85,7 @@ class ManifestFactory(object):
         clsname = ManifestFactory.type_to_clsname(type_)
         if clsname not in globals():
             raise ManifestException("Unknown manifest file type %s" % clsname)
-        mp = globals()[clsname](contents, remote_url)
-        return ManifestModel(mp.as_dict())
+        return globals()[clsname](contents, remote_url)
 
 
 class BaseManifestParser(object):
@@ -234,9 +232,9 @@ class LibraryPropertiesManifestParser(BaseManifestParser):
         if repository and repository["url"] == homepage:
             homepage = None
         return dict(
-            name=properties["name"],
-            version=properties["version"],
-            description=properties["sentence"],
+            name=properties.get("name"),
+            version=properties.get("version"),
+            description=properties.get("sentence"),
             frameworks=["arduino"],
             platforms=self._process_platforms(properties) or ["*"],
             keywords=self._parse_keywords(properties),
@@ -258,12 +256,6 @@ class LibraryPropertiesManifestParser(BaseManifestParser):
                 continue
             key, value = line.split("=", 1)
             data[key.strip()] = value.strip()
-
-        required_fields = set(["name", "version", "author", "sentence"])
-        if not set(data.keys()) >= required_fields:
-            raise ManifestParserException(
-                "Missing fields: " + ",".join(required_fields - set(data.keys()))
-            )
         return data
 
     @staticmethod
@@ -301,6 +293,8 @@ class LibraryPropertiesManifestParser(BaseManifestParser):
         return result
 
     def _parse_authors(self, properties):
+        if "author" not in properties:
+            return None
         authors = []
         for author in properties["author"].split(","):
             name, email = self.parse_author_name_and_email(author)

tests/test_pkgmanifest.py

@@ -14,7 +14,9 @@
 
 import pytest
 
+from platformio.datamodel import DataModelException
 from platformio.package.manifest import parser
+from platformio.package.manifest.model import ManifestModel
 
 
 def test_library_json_parser():
@@ -88,10 +90,6 @@ def test_module_json_parser():
 
 
 def test_library_properties_parser():
-    # test missed fields
-    with pytest.raises(parser.ManifestParserException):
-        parser.LibraryPropertiesManifestParser("name=TestPackage")
-
     # Base
     contents = """
 name=TestPackage
@@ -151,7 +149,7 @@ sentence=This is Arduino library
     }
 
 
-def test_library_json_model():
+def test_library_json_valid_model():
     contents = """
 {
   "name": "ArduinoJson",
@@ -178,7 +176,8 @@ def test_library_json_model():
   "license": "MIT"
 }
 """
-    model = parser.ManifestFactory.new(contents, parser.ManifestFileType.LIBRARY_JSON)
+    data = parser.ManifestFactory.new(contents, parser.ManifestFileType.LIBRARY_JSON)
+    model = ManifestModel(**data.as_dict())
     assert sorted(model.as_dict().items()) == sorted(
         {
             "name": "ArduinoJson",
@@ -191,15 +190,17 @@ def test_library_json_model():
                 "branch": None,
             },
             "version": "6.12.0",
-            "authors": {
-                "url": "https://blog.benoitblanchon.fr",
-                "maintainer": False,
-                "email": None,
-                "name": "Benoit Blanchon",
-            },
+            "authors": [
+                {
+                    "url": "https://blog.benoitblanchon.fr",
+                    "maintainer": False,
+                    "email": None,
+                    "name": "Benoit Blanchon",
+                }
+            ],
             "export": {
-                "include": None,
                 "exclude": ["fuzzing", "scripts", "test", "third-party"],
+                "include": None,
             },
             "frameworks": ["arduino"],
             "platforms": ["*"],