From cac00f5b268900357918ab9027442f4a0e32085c Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen <paulus@home-assistant.io> Date: Tue, 9 Apr 2019 09:30:32 -0700 Subject: [PATCH] Test for circular dependencies using manifests (#22908) * Integration dependencies * Lint * Lint * Fix one test * Lint * Fix load custom component integration Fix async issue Add circular dependency detection in manifest validation * Fix test * Address review comment * Apply suggestions from code review Co-Authored-By: balloob <paulus@home-assistant.io> --- homeassistant/bootstrap.py | 50 ++--- homeassistant/loader.py | 204 +++++++++++++++--- homeassistant/setup.py | 4 +- script/manifest/validate.py | 30 ++- tests/common.py | 18 +- tests/components/mobile_app/__init__.py | 4 +- tests/test_bootstrap.py | 4 +- tests/test_loader.py | 66 ++++-- tests/test_setup.py | 14 +- .../test_embedded/__init__.py | 1 + .../test_package/manifest.json | 8 + 11 files changed, 314 insertions(+), 89 deletions(-) create mode 100644 tests/testing_config/custom_components/test_package/manifest.json diff --git a/homeassistant/bootstrap.py b/homeassistant/bootstrap.py index 435ec317985..c27dde9f940 100644 --- a/homeassistant/bootstrap.py +++ b/homeassistant/bootstrap.py @@ -128,16 +128,17 @@ async def async_from_config_dict(config: Dict[str, Any], hass.config_entries = config_entries.ConfigEntries(hass, config) await hass.config_entries.async_initialize() - components = _get_components(hass, config) + domains = _get_domains(hass, config) # Resolve all dependencies of all components. - for component in list(components): - try: - components.update(loader.component_dependencies(hass, component)) - except loader.LoaderError: - # Ignore it, or we'll break startup - # It will be properly handled during setup. - pass + for dep_domains in await asyncio.gather(*[ + loader.async_component_dependencies(hass, domain) + for domain in domains + ], return_exceptions=True): + # Result is either a set or an exception. We ignore exceptions + # It will be properly handled during setup of the domain. + if isinstance(dep_domains, set): + domains.update(dep_domains) # setup components res = await core_component.async_setup(hass, config) @@ -151,10 +152,10 @@ async def async_from_config_dict(config: Dict[str, Any], _LOGGER.info("Home Assistant core initialized") # stage 0, load logging components - for component in components: - if component in LOGGING_COMPONENT: + for domain in domains: + if domain in LOGGING_COMPONENT: hass.async_create_task( - async_setup_component(hass, component, config)) + async_setup_component(hass, domain, config)) await hass.async_block_till_done() @@ -165,18 +166,18 @@ async def async_from_config_dict(config: Dict[str, Any], hass.helpers.area_registry.async_get_registry()) # stage 1 - for component in components: - if component in FIRST_INIT_COMPONENT: + for domain in domains: + if domain in FIRST_INIT_COMPONENT: hass.async_create_task( - async_setup_component(hass, component, config)) + async_setup_component(hass, domain, config)) await hass.async_block_till_done() # stage 2 - for component in components: - if component in FIRST_INIT_COMPONENT or component in LOGGING_COMPONENT: + for domain in domains: + if domain in FIRST_INIT_COMPONENT or domain in LOGGING_COMPONENT: continue - hass.async_create_task(async_setup_component(hass, component, config)) + hass.async_create_task(async_setup_component(hass, domain, config)) await hass.async_block_till_done() @@ -398,18 +399,17 @@ async def async_mount_local_lib_path(config_dir: str) -> str: @core.callback -def _get_components(hass: core.HomeAssistant, - config: Dict[str, Any]) -> Set[str]: - """Get components to set up.""" +def _get_domains(hass: core.HomeAssistant, config: Dict[str, Any]) -> Set[str]: + """Get domains of components to set up.""" # Filter out the repeating and common config section [homeassistant] - components = set(key.split(' ')[0] for key in config.keys() - if key != core.DOMAIN) + domains = set(key.split(' ')[0] for key in config.keys() + if key != core.DOMAIN) # Add config entry domains - components.update(hass.config_entries.async_domains()) # type: ignore + domains.update(hass.config_entries.async_domains()) # type: ignore # Make sure the Hass.io component is loaded if 'HASSIO' in os.environ: - components.add('hassio') + domains.add('hassio') - return components + return domains diff --git a/homeassistant/loader.py b/homeassistant/loader.py index 4ca19935206..a1dbad3439e 100644 --- a/homeassistant/loader.py +++ b/homeassistant/loader.py @@ -12,17 +12,28 @@ available it will check the built-in components and platforms. """ import functools as ft import importlib +import json import logging +import pathlib import sys from types import ModuleType -from typing import Optional, Set, TYPE_CHECKING, Callable, Any, TypeVar, List # noqa pylint: disable=unused-import +from typing import ( + Optional, + Set, + TYPE_CHECKING, + Callable, + Any, + TypeVar, + List, + Dict +) from homeassistant.const import PLATFORM_FORMAT # Typing imports that create a circular dependency # pylint: disable=using-constant-test,unused-import if TYPE_CHECKING: - from homeassistant.core import HomeAssistant # NOQA + from homeassistant.core import HomeAssistant # noqa CALLABLE_T = TypeVar('CALLABLE_T', bound=Callable) # noqa pylint: disable=invalid-name @@ -34,17 +45,146 @@ _LOGGER = logging.getLogger(__name__) DATA_KEY = 'components' +DATA_INTEGRATIONS = 'integrations' PACKAGE_CUSTOM_COMPONENTS = 'custom_components' PACKAGE_BUILTIN = 'homeassistant.components' LOOKUP_PATHS = [PACKAGE_CUSTOM_COMPONENTS, PACKAGE_BUILTIN] COMPONENTS_WITH_BAD_PLATFORMS = ['automation', 'mqtt', 'telegram_bot'] +_UNDEF = object() + + +def manifest_from_legacy_module(module: Any) -> Dict: + """Generate a manifest from a legacy module.""" + return { + 'domain': module.DOMAIN, + 'name': module.DOMAIN, + 'documentation': None, + 'requirements': getattr(module, 'REQUIREMENTS', []), + 'dependencies': getattr(module, 'DEPENDENCIES', []), + 'codeowners': [], + } + + +class Integration: + """An integration in Home Assistant.""" + + @classmethod + def resolve_from_root(cls, hass: 'HomeAssistant', root_module: Any, + domain: str) -> 'Optional[Integration]': + """Resolve an integration from a root module.""" + for base in root_module.__path__: + manifest_path = ( + pathlib.Path(base) / domain / 'manifest.json' + ) + + if not manifest_path.is_file(): + continue + + try: + manifest = json.loads(manifest_path.read_text()) + except ValueError as err: + _LOGGER.error("Error parsing manifest.json file at %s: %s", + manifest_path, err) + continue + + return cls( + hass, "{}.{}".format(root_module.__name__, domain), manifest + ) + + return None + + @classmethod + def resolve_legacy(cls, hass: 'HomeAssistant', domain: str) \ + -> 'Optional[Integration]': + """Resolve legacy component. + + Will create a stub manifest. + """ + comp = get_component(hass, domain) + + if comp is None: + return None + + return cls( + hass, comp.__name__, manifest_from_legacy_module(comp) + ) + + def __init__(self, hass: 'HomeAssistant', pkg_path: str, manifest: Dict): + """Initialize an integration.""" + self.hass = hass + self.pkg_path = pkg_path + self.name = manifest['name'] # type: str + self.domain = manifest['domain'] # type: str + self.dependencies = manifest['dependencies'] # type: List[str] + self.requirements = manifest['requirements'] # type: List[str] + + def get_component(self) -> Any: + """Return the component.""" + return importlib.import_module(self.pkg_path) + + def get_platform(self, platform_name: str) -> Any: + """Return a platform for an integration.""" + return importlib.import_module( + "{}.{}".format(self.pkg_path, platform_name) + ) + + +async def async_get_integration(hass: 'HomeAssistant', domain: str)\ + -> Integration: + """Get an integration.""" + cache = hass.data.get(DATA_INTEGRATIONS) + if cache is None: + if not _async_mount_config_dir(hass): + raise IntegrationNotFound(domain) + cache = hass.data[DATA_INTEGRATIONS] = {} + + integration = cache.get(domain, _UNDEF) # type: Optional[Integration] + + if integration is _UNDEF: + pass + elif integration is None: + raise IntegrationNotFound(domain) + else: + return integration + + try: + import custom_components + integration = await hass.async_add_executor_job( + Integration.resolve_from_root, hass, custom_components, domain + ) + if integration is not None: + cache[domain] = integration + return integration + + except ImportError: + pass + + from homeassistant import components + + integration = await hass.async_add_executor_job( + Integration.resolve_from_root, hass, components, domain + ) + + if integration is not None: + cache[domain] = integration + return integration + + integration = await hass.async_add_executor_job( + Integration.resolve_legacy, hass, domain + ) + cache[domain] = integration + + if not integration: + raise IntegrationNotFound(domain) + + return integration class LoaderError(Exception): """Loader base error.""" -class ComponentNotFound(LoaderError): +class IntegrationNotFound(LoaderError): """Raised when a component is not found.""" def __init__(self, domain: str) -> None: @@ -169,12 +309,8 @@ def _load_file(hass, # type: HomeAssistant cache = hass.data.get(DATA_KEY) if cache is None: - if hass.config.config_dir is None: - _LOGGER.error("Can't load components - config dir is not set") + if not _async_mount_config_dir(hass): return None - # Only insert if it's not there (happens during tests) - if sys.path[0] != hass.config.config_dir: - sys.path.insert(0, hass.config.config_dir) cache = hass.data[DATA_KEY] = {} for path in ('{}.{}'.format(base, comp_or_platform) @@ -294,46 +430,58 @@ def bind_hass(func: CALLABLE_T) -> CALLABLE_T: return func -def component_dependencies(hass, # type: HomeAssistant - comp_name: str) -> Set[str]: +async def async_component_dependencies(hass, # type: HomeAssistant + domain: str) -> Set[str]: """Return all dependencies and subdependencies of components. Raises CircularDependency if a circular dependency is found. - - Async friendly. """ - return _component_dependencies(hass, comp_name, set(), set()) + return await _async_component_dependencies(hass, domain, set(), set()) -def _component_dependencies(hass, # type: HomeAssistant - comp_name: str, loaded: Set[str], - loading: Set) -> Set[str]: +async def _async_component_dependencies(hass, # type: HomeAssistant + domain: str, loaded: Set[str], + loading: Set) -> Set[str]: """Recursive function to get component dependencies. Async friendly. """ - component = get_component(hass, comp_name) + integration = await async_get_integration(hass, domain) - if component is None: - raise ComponentNotFound(comp_name) + if integration is None: + raise IntegrationNotFound(domain) - loading.add(comp_name) + loading.add(domain) - for dependency in getattr(component, 'DEPENDENCIES', []): + for dependency_domain in integration.dependencies: # Check not already loaded - if dependency in loaded: + if dependency_domain in loaded: continue # If we are already loading it, we have a circular dependency. - if dependency in loading: - raise CircularDependency(comp_name, dependency) + if dependency_domain in loading: + raise CircularDependency(domain, dependency_domain) - dep_loaded = _component_dependencies( - hass, dependency, loaded, loading) + dep_loaded = await _async_component_dependencies( + hass, dependency_domain, loaded, loading) loaded.update(dep_loaded) - loaded.add(comp_name) - loading.remove(comp_name) + loaded.add(domain) + loading.remove(domain) return loaded + + +def _async_mount_config_dir(hass, # type: HomeAssistant + ) -> bool: + """Mount config dir in order to load custom_component. + + Async friendly but not a coroutine. + """ + if hass.config.config_dir is None: + _LOGGER.error("Can't load components - config dir is not set") + return False + if hass.config.config_dir not in sys.path: + sys.path.insert(0, hass.config.config_dir) + return True diff --git a/homeassistant/setup.py b/homeassistant/setup.py index 29c8e22d45d..0e294200b5f 100644 --- a/homeassistant/setup.py +++ b/homeassistant/setup.py @@ -108,8 +108,8 @@ async def _async_setup_component(hass: core.HomeAssistant, # Validate all dependencies exist and there are no circular dependencies try: - loader.component_dependencies(hass, domain) - except loader.ComponentNotFound as err: + await loader.async_component_dependencies(hass, domain) + except loader.IntegrationNotFound as err: _LOGGER.error( "Not setting up %s because we are unable to resolve " "(sub)dependency %s", domain, err.domain) diff --git a/script/manifest/validate.py b/script/manifest/validate.py index d0d59529d20..e5db1c9368c 100755 --- a/script/manifest/validate.py +++ b/script/manifest/validate.py @@ -18,10 +18,16 @@ MANIFEST_SCHEMA = vol.Schema({ }) -components_path = pathlib.Path('homeassistant/components') +COMPONENTS_PATH = pathlib.Path('homeassistant/components') -def validate_integration(path): +def validate_dependency(path, dependency, loaded, loading): + """Validate dependency is exist and no circular dependency.""" + dep_path = path.parent / dependency + return validate_integration(dep_path, loaded, loading) + + +def validate_integration(path, loaded, loading): """Validate that an integrations has a valid manifest.""" errors = [] path = pathlib.Path(path) @@ -29,7 +35,7 @@ def validate_integration(path): manifest_path = path / 'manifest.json' if not manifest_path.is_file(): - errors.append('File manifest.json not found') + errors.append('Manifest file {} not found'.format(manifest_path)) return errors # Fatal error try: @@ -47,10 +53,18 @@ def validate_integration(path): errors.append('Domain does not match dir name') for dep in manifest['dependencies']: - dep_manifest = path.parent / dep / 'manifest.json' - if not dep_manifest.is_file(): - errors.append("Unable to find dependency {}".format(dep)) + if dep in loaded: + continue + if dep in loading: + errors.append("Found circular dependency {} in {}".format( + dep, path + )) + continue + loading.add(dep) + + errors.extend(validate_dependency(path, dep, loaded, loading)) + loaded.add(path.name) return errors @@ -58,11 +72,11 @@ def validate_all(): """Validate all integrations.""" invalid = [] - for fil in components_path.iterdir(): + for fil in COMPONENTS_PATH.iterdir(): if fil.is_file() or fil.name == '__pycache__': continue - errors = validate_integration(fil) + errors = validate_integration(fil, set(), set()) if errors: invalid.append((fil, errors)) diff --git a/tests/common.py b/tests/common.py index e04c8347c09..2f89d91e4d3 100644 --- a/tests/common.py +++ b/tests/common.py @@ -16,7 +16,7 @@ from unittest.mock import MagicMock, Mock, patch import homeassistant.util.dt as date_util import homeassistant.util.yaml as yaml -from homeassistant import auth, config_entries, core as ha +from homeassistant import auth, config_entries, core as ha, loader from homeassistant.auth import ( models as auth_models, auth_store, providers as auth_providers, permissions as auth_permissions) @@ -35,6 +35,7 @@ from homeassistant.util.unit_system import METRIC_SYSTEM from homeassistant.util.async_ import ( run_callback_threadsafe, run_coroutine_threadsafe) + _TEST_INSTANCE_PORT = SERVER_PORT _LOGGER = logging.getLogger(__name__) INSTANCES = [] @@ -894,3 +895,18 @@ async def flush_store(store): async def get_system_health_info(hass, domain): """Get system health info.""" return await hass.data['system_health']['info'][domain](hass) + + +def mock_integration(hass, module): + """Mock an integration.""" + integration = loader.Integration( + hass, 'homeassisant.components.{}'.format(module.DOMAIN), + loader.manifest_from_legacy_module(module)) + integration.get_component = lambda: module + + # Backwards compat + loader.set_component(hass, module.DOMAIN, module) + + hass.data.setdefault( + loader.DATA_INTEGRATIONS, {} + )[module.DOMAIN] = integration diff --git a/tests/components/mobile_app/__init__.py b/tests/components/mobile_app/__init__.py index cf617ff0528..98c7a20b059 100644 --- a/tests/components/mobile_app/__init__.py +++ b/tests/components/mobile_app/__init__.py @@ -55,7 +55,7 @@ async def webhook_client(hass, aiohttp_client, hass_storage, hass_admin_user): } await async_setup_component(hass, DOMAIN, {DOMAIN: {}}) - + await hass.async_block_till_done() return await aiohttp_client(hass.http.app) @@ -63,6 +63,7 @@ async def webhook_client(hass, aiohttp_client, hass_storage, hass_admin_user): async def authed_api_client(hass, hass_client): """Provide an authenticated client for mobile_app to use.""" await async_setup_component(hass, DOMAIN, {DOMAIN: {}}) + await hass.async_block_till_done() return await hass_client() @@ -70,3 +71,4 @@ async def authed_api_client(hass, hass_client): async def setup_ws(hass): """Configure the websocket_api component.""" assert await async_setup_component(hass, 'websocket_api', {}) + await hass.async_block_till_done() diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 1b62c5244e4..eade9d5fc63 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -108,7 +108,7 @@ async def test_async_from_config_file_not_mount_deps_folder(loop): async def test_load_hassio(hass): """Test that we load Hass.io component.""" with patch.dict(os.environ, {}, clear=True): - assert bootstrap._get_components(hass, {}) == set() + assert bootstrap._get_domains(hass, {}) == set() with patch.dict(os.environ, {'HASSIO': '1'}): - assert bootstrap._get_components(hass, {}) == {'hassio'} + assert bootstrap._get_domains(hass, {}) == {'hassio'} diff --git a/tests/test_loader.py b/tests/test_loader.py index 09f830a8eab..e3888412f0c 100644 --- a/tests/test_loader.py +++ b/tests/test_loader.py @@ -4,9 +4,10 @@ import asyncio import pytest import homeassistant.loader as loader -import homeassistant.components.http as http +from homeassistant.components import http, hue +from homeassistant.components.hue import light as hue_light -from tests.common import MockModule, async_mock_service +from tests.common import MockModule, async_mock_service, mock_integration def test_set_component(hass): @@ -22,31 +23,30 @@ def test_get_component(hass): assert http == loader.get_component(hass, 'http') -def test_component_dependencies(hass): +async def test_component_dependencies(hass): """Test if we can get the proper load order of components.""" - loader.set_component(hass, 'mod1', MockModule('mod1')) - loader.set_component(hass, 'mod2', MockModule('mod2', ['mod1'])) - loader.set_component(hass, 'mod3', MockModule('mod3', ['mod2'])) + mock_integration(hass, MockModule('mod1')) + mock_integration(hass, MockModule('mod2', ['mod1'])) + mock_integration(hass, MockModule('mod3', ['mod2'])) assert {'mod1', 'mod2', 'mod3'} == \ - loader.component_dependencies(hass, 'mod3') + await loader.async_component_dependencies(hass, 'mod3') # Create circular dependency - loader.set_component(hass, 'mod1', MockModule('mod1', ['mod3'])) + mock_integration(hass, MockModule('mod1', ['mod3'])) with pytest.raises(loader.CircularDependency): - print(loader.component_dependencies(hass, 'mod3')) + print(await loader.async_component_dependencies(hass, 'mod3')) # Depend on non-existing component - loader.set_component(hass, 'mod1', - MockModule('mod1', ['nonexisting'])) + mock_integration(hass, MockModule('mod1', ['nonexisting'])) - with pytest.raises(loader.ComponentNotFound): - print(loader.component_dependencies(hass, 'mod1')) + with pytest.raises(loader.IntegrationNotFound): + print(await loader.async_component_dependencies(hass, 'mod1')) # Try to get dependencies for non-existing component - with pytest.raises(loader.ComponentNotFound): - print(loader.component_dependencies(hass, 'nonexisting')) + with pytest.raises(loader.IntegrationNotFound): + print(await loader.async_component_dependencies(hass, 'nonexisting')) def test_component_loader(hass): @@ -142,3 +142,39 @@ async def test_get_platform_enforces_component_path(hass, caplog): assert loader.get_platform(hass, 'comp_path_test', 'hue') is None assert ('Search path was limited to path of component: ' 'homeassistant.components') in caplog.text + + +async def test_get_integration(hass): + """Test resolving integration.""" + integration = await loader.async_get_integration(hass, 'hue') + assert hue == integration.get_component() + assert hue_light == integration.get_platform('light') + + +async def test_get_integration_legacy(hass): + """Test resolving integration.""" + integration = await loader.async_get_integration(hass, 'test_embedded') + assert integration.get_component().DOMAIN == 'test_embedded' + assert integration.get_platform('switch') is not None + + +async def test_get_integration_custom_component(hass): + """Test resolving integration.""" + integration = await loader.async_get_integration(hass, 'test_package') + print(integration) + assert integration.get_component().DOMAIN == 'test_package' + assert integration.name == 'Test Package' + + +def test_integration_properties(hass): + """Test integration properties.""" + integration = loader.Integration(hass, 'homeassistant.components.hue', { + 'name': 'Philips Hue', + 'domain': 'hue', + 'dependencies': ['test-dep'], + 'requirements': ['test-req==1.0.0'], + }) + assert integration.name == "Philips Hue" + assert integration.domain == 'hue' + assert integration.dependencies == ['test-dep'] + assert integration.requirements == ['test-req==1.0.0'] diff --git a/tests/test_setup.py b/tests/test_setup.py index c6126bc4a3b..00518776b52 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -20,7 +20,7 @@ from homeassistant.helpers import discovery from tests.common import \ get_test_home_assistant, MockModule, MockPlatform, \ - assert_setup_component, get_test_config_dir + assert_setup_component, get_test_config_dir, mock_integration ORIG_TIMEZONE = dt_util.DEFAULT_TIME_ZONE VERSION_PATH = os.path.join(get_test_config_dir(), config_util.VERSION_FILE) @@ -378,18 +378,18 @@ class TestSetup: def test_component_not_setup_missing_dependencies(self): """Test we do not set up a component if not all dependencies loaded.""" - deps = ['non_existing'] - loader.set_component( - self.hass, 'comp', MockModule('comp', dependencies=deps)) + deps = ['maybe_existing'] + mock_integration(self.hass, MockModule('comp', dependencies=deps)) assert not setup.setup_component(self.hass, 'comp', {}) assert 'comp' not in self.hass.config.components self.hass.data.pop(setup.DATA_SETUP) - loader.set_component( - self.hass, 'non_existing', MockModule('non_existing')) - assert setup.setup_component(self.hass, 'comp', {}) + mock_integration(self.hass, MockModule('comp2', dependencies=deps)) + mock_integration(self.hass, MockModule('maybe_existing')) + + assert setup.setup_component(self.hass, 'comp2', {}) def test_component_failing_setup(self): """Test component that fails setup.""" diff --git a/tests/testing_config/custom_components/test_embedded/__init__.py b/tests/testing_config/custom_components/test_embedded/__init__.py index 2206054356e..21843fc927a 100644 --- a/tests/testing_config/custom_components/test_embedded/__init__.py +++ b/tests/testing_config/custom_components/test_embedded/__init__.py @@ -1,4 +1,5 @@ """Component with embedded platforms.""" +DOMAIN = 'test_embedded' async def async_setup(hass, config): diff --git a/tests/testing_config/custom_components/test_package/manifest.json b/tests/testing_config/custom_components/test_package/manifest.json new file mode 100644 index 00000000000..320d2768d27 --- /dev/null +++ b/tests/testing_config/custom_components/test_package/manifest.json @@ -0,0 +1,8 @@ +{ + "domain": "test_package", + "name": "Test Package", + "documentation": "http://test-package.io", + "requirements": [], + "dependencies": [], + "codeowners": [] +} -- GitLab