From 4c8a0199ec1781805db5768db4c6627d753457b9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 07:49:40 -0500 Subject: [PATCH 1/7] wip --- homeassistant/helpers/service.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index 715a960de5dc03..8a04671bd647ed 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -574,17 +574,15 @@ async def async_get_all_descriptions( missing = set() all_services = [] for domain in services: - for service in services[domain]: - cache_key = (domain, service) + for service_name in services[domain]: + cache_key = (domain, service_name) all_services.append(cache_key) if cache_key not in descriptions_cache: missing.add(domain) # If we have a complete cache, check if it is still valid - if ALL_SERVICE_DESCRIPTIONS_CACHE in hass.data: - previous_all_services, previous_descriptions_cache = hass.data[ - ALL_SERVICE_DESCRIPTIONS_CACHE - ] + if all_cache := hass.data.get(ALL_SERVICE_DESCRIPTIONS_CACHE): + previous_all_services, previous_descriptions_cache = all_cache # If the services are the same, we can return the cache if previous_all_services == all_services: return cast(dict[str, dict[str, Any]], previous_descriptions_cache) @@ -609,11 +607,12 @@ async def async_get_all_descriptions( # Build response descriptions: dict[str, dict[str, Any]] = {} - for domain in services: + for domain, services_map in services.items(): descriptions[domain] = {} + domain_descriptions = descriptions[domain] - for service in services[domain]: - cache_key = (domain, service) + for service_name in services_map: + cache_key = (domain, service_name) description = descriptions_cache.get(cache_key) # Cache missing descriptions @@ -621,7 +620,7 @@ async def async_get_all_descriptions( domain_yaml = loaded[domain] yaml_description = domain_yaml.get( # type: ignore[union-attr] - service, {} + service_name, {} ) # Don't warn for missing services, because it triggers false @@ -637,7 +636,7 @@ async def async_get_all_descriptions( description["target"] = yaml_description["target"] if ( - response := hass.services.supports_response(domain, service) + response := hass.services.supports_response(domain, service_name) ) != SupportsResponse.NONE: description["response"] = { "optional": response == SupportsResponse.OPTIONAL, @@ -645,7 +644,7 @@ async def async_get_all_descriptions( descriptions_cache[cache_key] = description - descriptions[domain][service] = description + domain_descriptions[service_name] = description hass.data[ALL_SERVICE_DESCRIPTIONS_CACHE] = (all_services, descriptions) return descriptions From ffcaedc626ea967614bade1cfd5c41b5ee6113e4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 09:51:54 -0500 Subject: [PATCH 2/7] tweaks --- homeassistant/helpers/service.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index 8a04671bd647ed..739e69ce3b6f44 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -566,7 +566,9 @@ async def async_get_all_descriptions( hass: HomeAssistant, ) -> dict[str, dict[str, Any]]: """Return descriptions (i.e. user documentation) for all service calls.""" - descriptions_cache = hass.data.setdefault(SERVICE_DESCRIPTION_CACHE, {}) + descriptions_cache: dict[ + tuple[str, str], dict[str, Any] | None + ] = hass.data.setdefault(SERVICE_DESCRIPTION_CACHE, {}) services = hass.services.async_services() # See if there are new services not seen before. @@ -597,13 +599,10 @@ async def async_get_all_descriptions( for int_or_exc in ints_or_excs.values() if isinstance(int_or_exc, Integration) ] - contents = await hass.async_add_executor_job( _load_services_files, hass, integrations ) - - for domain, content in zip(missing, contents): - loaded[domain] = content + loaded = dict(zip(missing, contents)) # Build response descriptions: dict[str, dict[str, Any]] = {} @@ -614,11 +613,8 @@ async def async_get_all_descriptions( for service_name in services_map: cache_key = (domain, service_name) description = descriptions_cache.get(cache_key) - # Cache missing descriptions - if description is None: - domain_yaml = loaded[domain] - + if description is None and (domain_yaml := loaded.get(domain)): yaml_description = domain_yaml.get( # type: ignore[union-attr] service_name, {} ) @@ -666,7 +662,9 @@ def async_set_service_schema( hass: HomeAssistant, domain: str, service: str, schema: dict[str, Any] ) -> None: """Register a description for a service.""" - hass.data.setdefault(SERVICE_DESCRIPTION_CACHE, {}) + descriptions_cache: dict[ + tuple[str, str], dict[str, Any] | None + ] = hass.data.setdefault(SERVICE_DESCRIPTION_CACHE, {}) description = { "name": schema.get("name", ""), @@ -678,7 +676,7 @@ def async_set_service_schema( description["target"] = schema["target"] hass.data.pop(ALL_SERVICE_DESCRIPTIONS_CACHE, None) - hass.data[SERVICE_DESCRIPTION_CACHE][(domain, service)] = description + descriptions_cache[(domain, service)] = description @bind_hass From 3a93f0583eba8484f6bcb5717b4a82d14d34b566 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 09:52:33 -0500 Subject: [PATCH 3/7] tweaks --- homeassistant/helpers/service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index 739e69ce3b6f44..eadc7d9a71f986 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -590,7 +590,7 @@ async def async_get_all_descriptions( return cast(dict[str, dict[str, Any]], previous_descriptions_cache) # Files we loaded for missing descriptions - loaded = {} + loaded: dict[str, JSON_TYPE] = {} if missing: ints_or_excs = await async_get_integrations(hass, missing) From 8be2a2f27aa8e77dc190bf6245615b0f8e374ebc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 09:57:34 -0500 Subject: [PATCH 4/7] add coverage --- homeassistant/helpers/service.py | 1 - tests/helpers/test_service.py | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index eadc7d9a71f986..90d3668b9f4fa5 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -621,7 +621,6 @@ async def async_get_all_descriptions( # Don't warn for missing services, because it triggers false # positives for things like scripts, that register as a service - description = { "name": yaml_description.get("name", ""), "description": yaml_description.get("description", ""), diff --git a/tests/helpers/test_service.py b/tests/helpers/test_service.py index 291a1744d20976..652f2f8ae78c07 100644 --- a/tests/helpers/test_service.py +++ b/tests/helpers/test_service.py @@ -605,6 +605,76 @@ async def test_async_get_all_descriptions(hass: HomeAssistant) -> None: assert await service.async_get_all_descriptions(hass) is descriptions +async def test_async_get_all_descriptions_failing_integration( + hass: HomeAssistant, +) -> None: + """Test async_get_all_descriptions when async_get_integrations returns an exception.""" + group = hass.components.group + group_config = {group.DOMAIN: {}} + await async_setup_component(hass, group.DOMAIN, group_config) + descriptions = await service.async_get_all_descriptions(hass) + + assert len(descriptions) == 1 + + assert "description" in descriptions["group"]["reload"] + assert "fields" in descriptions["group"]["reload"] + + logger = hass.components.logger + logger_config = {logger.DOMAIN: {}} + await async_setup_component(hass, logger.DOMAIN, logger_config) + with patch( + "homeassistant.helpers.service.async_get_integrations", + return_value={"logger": ImportError}, + ): + descriptions = await service.async_get_all_descriptions(hass) + + assert len(descriptions) == 2 + + # Services are None if the load fails but should + # not raise + assert descriptions[logger.DOMAIN]["set_level"] is None + + hass.services.async_register(logger.DOMAIN, "new_service", lambda x: None, None) + service.async_set_service_schema( + hass, logger.DOMAIN, "new_service", {"description": "new service"} + ) + descriptions = await service.async_get_all_descriptions(hass) + assert "description" in descriptions[logger.DOMAIN]["new_service"] + assert descriptions[logger.DOMAIN]["new_service"]["description"] == "new service" + + hass.services.async_register( + logger.DOMAIN, "another_new_service", lambda x: None, None + ) + hass.services.async_register( + logger.DOMAIN, + "service_with_optional_response", + lambda x: None, + None, + SupportsResponse.OPTIONAL, + ) + hass.services.async_register( + logger.DOMAIN, + "service_with_only_response", + lambda x: None, + None, + SupportsResponse.ONLY, + ) + + descriptions = await service.async_get_all_descriptions(hass) + assert "another_new_service" in descriptions[logger.DOMAIN] + assert "service_with_optional_response" in descriptions[logger.DOMAIN] + assert descriptions[logger.DOMAIN]["service_with_optional_response"][ + "response" + ] == {"optional": True} + assert "service_with_only_response" in descriptions[logger.DOMAIN] + assert descriptions[logger.DOMAIN]["service_with_only_response"]["response"] == { + "optional": False + } + + # Verify the cache returns the same object + assert await service.async_get_all_descriptions(hass) is descriptions + + async def test_call_with_required_features(hass: HomeAssistant, mock_entities) -> None: """Test service calls invoked only if entity has required features.""" test_service_mock = AsyncMock(return_value=None) From a807d5161c6a8b6c86bc07e957600f02a095a43b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 10:36:24 -0500 Subject: [PATCH 5/7] complain loudly as we never execpt this to happen --- homeassistant/helpers/service.py | 13 ++++++++----- tests/helpers/test_service.py | 3 ++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index 90d3668b9f4fa5..91c50365f09101 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -594,11 +594,14 @@ async def async_get_all_descriptions( if missing: ints_or_excs = await async_get_integrations(hass, missing) - integrations = [ - int_or_exc - for int_or_exc in ints_or_excs.values() - if isinstance(int_or_exc, Integration) - ] + integrations: list[Integration] = [] + for domain, int_or_exc in ints_or_excs.items(): + if type(int_or_exc) is Integration: # pylint: disable=unidiomatic-typecheck + integrations.append(int_or_exc) + continue + if TYPE_CHECKING: + assert isinstance(int_or_exc, Exception) + _LOGGER.error("Failed to load integration: %s", domain, exc_info=int_or_exc) contents = await hass.async_add_executor_job( _load_services_files, hass, integrations ) diff --git a/tests/helpers/test_service.py b/tests/helpers/test_service.py index 652f2f8ae78c07..f1fd520c88c5d4 100644 --- a/tests/helpers/test_service.py +++ b/tests/helpers/test_service.py @@ -606,7 +606,7 @@ async def test_async_get_all_descriptions(hass: HomeAssistant) -> None: async def test_async_get_all_descriptions_failing_integration( - hass: HomeAssistant, + hass: HomeAssistant, caplog: pytest.LogCaptureFixture ) -> None: """Test async_get_all_descriptions when async_get_integrations returns an exception.""" group = hass.components.group @@ -629,6 +629,7 @@ async def test_async_get_all_descriptions_failing_integration( descriptions = await service.async_get_all_descriptions(hass) assert len(descriptions) == 2 + assert "Failed to load integration: logger" in caplog.text # Services are None if the load fails but should # not raise From 346af33703df9dfdcfe8ff53145bfa889d86b53b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 11:15:30 -0500 Subject: [PATCH 6/7] ensure not None --- homeassistant/helpers/service.py | 3 ++- tests/helpers/test_service.py | 35 ++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index 91c50365f09101..c8f409a30f91cd 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -617,7 +617,8 @@ async def async_get_all_descriptions( cache_key = (domain, service_name) description = descriptions_cache.get(cache_key) # Cache missing descriptions - if description is None and (domain_yaml := loaded.get(domain)): + if description is None: + domain_yaml = loaded.get(domain) or {} yaml_description = domain_yaml.get( # type: ignore[union-attr] service_name, {} ) diff --git a/tests/helpers/test_service.py b/tests/helpers/test_service.py index f1fd520c88c5d4..e8d39ab01b98ee 100644 --- a/tests/helpers/test_service.py +++ b/tests/helpers/test_service.py @@ -631,9 +631,13 @@ async def test_async_get_all_descriptions_failing_integration( assert len(descriptions) == 2 assert "Failed to load integration: logger" in caplog.text - # Services are None if the load fails but should + # Services are empty defaults if the load fails but should # not raise - assert descriptions[logger.DOMAIN]["set_level"] is None + assert descriptions[logger.DOMAIN]["set_level"] == { + "description": "", + "fields": {}, + "name": "", + } hass.services.async_register(logger.DOMAIN, "new_service", lambda x: None, None) service.async_set_service_schema( @@ -676,6 +680,33 @@ async def test_async_get_all_descriptions_failing_integration( assert await service.async_get_all_descriptions(hass) is descriptions +async def test_async_get_all_descriptions_dynamically_created_services( + hass: HomeAssistant, caplog: pytest.LogCaptureFixture +) -> None: + """Test async_get_all_descriptions when async_get_integrations when services are dynamic.""" + group = hass.components.group + group_config = {group.DOMAIN: {}} + await async_setup_component(hass, group.DOMAIN, group_config) + descriptions = await service.async_get_all_descriptions(hass) + + assert len(descriptions) == 1 + + assert "description" in descriptions["group"]["reload"] + assert "fields" in descriptions["group"]["reload"] + + shell_command = hass.components.shell_command + shell_command_config = {shell_command.DOMAIN: {"test_service": "ls /bin"}} + await async_setup_component(hass, shell_command.DOMAIN, shell_command_config) + descriptions = await service.async_get_all_descriptions(hass) + + assert len(descriptions) == 2 + assert descriptions[shell_command.DOMAIN]["test_service"] == { + "description": "", + "fields": {}, + "name": "", + } + + async def test_call_with_required_features(hass: HomeAssistant, mock_entities) -> None: """Test service calls invoked only if entity has required features.""" test_service_mock = AsyncMock(return_value=None) From abde1df7e30565e16983204e2b297761c78fa69a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 5 Jul 2023 11:18:07 -0500 Subject: [PATCH 7/7] comment it --- homeassistant/helpers/service.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index c8f409a30f91cd..b1e2dd9a723a96 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -619,6 +619,11 @@ async def async_get_all_descriptions( # Cache missing descriptions if description is None: domain_yaml = loaded.get(domain) or {} + # The YAML may be empty for dynamically defined + # services (ie shell_command) that never call + # service.async_set_service_schema for the dynamic + # service + yaml_description = domain_yaml.get( # type: ignore[union-attr] service_name, {} )