From 744881da59f6d586cf7512350b68206f7d5a24ff Mon Sep 17 00:00:00 2001 From: Ivan Kravets Date: Mon, 30 Sep 2019 19:44:03 +0300 Subject: [PATCH] Refactor DataModel with a strict type declaration --- platformio/commands/lib.py | 5 +- platformio/datamodel.py | 125 ++++++++++++-------------- platformio/package/manifest/model.py | 35 +++++--- platformio/package/manifest/parser.py | 18 ++-- tests/test_pkgmanifest.py | 27 +++--- 5 files changed, 100 insertions(+), 110 deletions(-) diff --git a/platformio/commands/lib.py b/platformio/commands/lib.py index 8f61769f..897a8a39 100644 --- a/platformio/commands/lib.py +++ b/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"]: diff --git a/platformio/datamodel.py b/platformio/datamodel.py index 39547d59..2a754e6d 100644 --- a/platformio/datamodel.py +++ b/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} diff --git a/platformio/package/manifest/model.py b/platformio/package/manifest/model.py index 63bad52e..3e3fd4ea 100644 --- a/platformio/package/manifest/model.py +++ b/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) diff --git a/platformio/package/manifest/parser.py b/platformio/package/manifest/parser.py index faf576c1..580a9465 100644 --- a/platformio/package/manifest/parser.py +++ b/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) diff --git a/tests/test_pkgmanifest.py b/tests/test_pkgmanifest.py index 8ca36add..1a09852a 100644 --- a/tests/test_pkgmanifest.py +++ b/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": ["*"],