Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ServiceValidationError and translation support #102592

Merged
merged 16 commits into from
Nov 6, 2023
Merged
5 changes: 5 additions & 0 deletions homeassistant/components/homeassistant/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -136,5 +136,10 @@
"name": "Reload all",
"description": "Reload all YAML configuration that can be reloaded without restarting Home Assistant."
}
},
"exceptions": {
"service_not_found": {
"message": "Service {domain}.{service} not found."
}
}
}
16 changes: 14 additions & 2 deletions homeassistant/components/mqtt/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@
SERVICE_RELOAD,
)
from homeassistant.core import HassJob, HomeAssistant, ServiceCall, callback
from homeassistant.exceptions import HomeAssistantError, TemplateError, Unauthorized
from homeassistant.exceptions import (
HomeAssistantError,
ServiceValidationError,
TemplateError,
Unauthorized,
)
from homeassistant.helpers import config_validation as cv, event as ev, template
from homeassistant.helpers.device_registry import DeviceEntry
from homeassistant.helpers.dispatcher import async_dispatcher_connect
Expand Down Expand Up @@ -246,7 +251,14 @@ async def async_check_config_schema(
message, _ = conf_util._format_config_error(
ex, domain, config, integration.documentation
)
raise HomeAssistantError(message) from ex
raise ServiceValidationError(
message,
translation_domain=DOMAIN,
translation_key="invalid_platform_config",
translation_placeholders={
"domain": domain,
},
) from ex


async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
Expand Down
5 changes: 5 additions & 0 deletions homeassistant/components/mqtt/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -207,5 +207,10 @@
"name": "[%key:common::action::reload%]",
"description": "Reloads MQTT entities from the YAML-configuration."
}
},
"exceptions": {
"invalid_platform_config": {
"message": "Reloading YAML config for manually configured MQTT `{domain}` failed. See logs for more details."
}
}
}
46 changes: 43 additions & 3 deletions homeassistant/components/websocket_api/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from homeassistant.exceptions import (
HomeAssistantError,
ServiceNotFound,
ServiceValidationError,
TemplateError,
Unauthorized,
)
Expand Down Expand Up @@ -238,14 +239,53 @@ async def handle_call_service(
connection.send_result(msg["id"], {"context": context})
except ServiceNotFound as err:
if err.domain == msg["domain"] and err.service == msg["service"]:
connection.send_error(msg["id"], const.ERR_NOT_FOUND, "Service not found.")
connection.send_error(
msg["id"],
const.ERR_NOT_FOUND,
f"Service {err.domain}.{err.service} not found.",
translation_domain=err.translation_domain,
translation_key=err.translation_key,
translation_placeholders=err.translation_placeholders,
)
else:
connection.send_error(msg["id"], const.ERR_HOME_ASSISTANT_ERROR, str(err))
# The called service called another service which does not exist
connection.send_error(
msg["id"],
const.ERR_HOME_ASSISTANT_ERROR,
f"Service {err.domain}.{err.service} called service "
f"{msg['domain']}.{msg['service']} which was not found.",
translation_domain=const.DOMAIN,
translation_key="child_service_not_found",
translation_placeholders={
"domain": err.domain,
"service": err.service,
"child_domain": msg["domain"],
"child_service": msg["service"],
},
)
except vol.Invalid as err:
connection.send_error(msg["id"], const.ERR_INVALID_FORMAT, str(err))
except ServiceValidationError as err:
connection.logger.error(err)
connection.logger.debug("", exc_info=err)
connection.send_error(
msg["id"],
const.ERR_SERVICE_VALIDATION_ERROR,
f"Validation error: {err}",
translation_domain=err.translation_domain,
translation_key=err.translation_key,
translation_placeholders=err.translation_placeholders,
)
except HomeAssistantError as err:
connection.logger.exception(err)
connection.send_error(msg["id"], const.ERR_HOME_ASSISTANT_ERROR, str(err))
connection.send_error(
msg["id"],
const.ERR_HOME_ASSISTANT_ERROR,
str(err),
translation_domain=err.translation_domain,
translation_key=err.translation_key,
translation_placeholders=err.translation_placeholders,
)
except Exception as err: # pylint: disable=broad-except
connection.logger.exception(err)
connection.send_error(msg["id"], const.ERR_UNKNOWN_ERROR, str(err))
Expand Down
23 changes: 20 additions & 3 deletions homeassistant/components/websocket_api/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,26 @@ def send_event(self, msg_id: int, event: Any | None = None) -> None:
self.send_message(messages.event_message(msg_id, event))

@callback
def send_error(self, msg_id: int, code: str, message: str) -> None:
"""Send a error message."""
self.send_message(messages.error_message(msg_id, code, message))
def send_error(
self,
msg_id: int,
code: str,
message: str,
translation_key: str | None = None,
translation_domain: str | None = None,
translation_placeholders: dict[str, Any] | None = None,
) -> None:
"""Send an error message."""
self.send_message(
messages.error_message(
msg_id,
code,
message,
translation_key=translation_key,
translation_domain=translation_domain,
translation_placeholders=translation_placeholders,
)
)

@callback
def async_handle_binary(self, handler_id: int, payload: bytes) -> None:
Expand Down
1 change: 1 addition & 0 deletions homeassistant/components/websocket_api/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
ERR_NOT_FOUND: Final = "not_found"
ERR_NOT_SUPPORTED: Final = "not_supported"
ERR_HOME_ASSISTANT_ERROR: Final = "home_assistant_error"
ERR_SERVICE_VALIDATION_ERROR: Final = "service_validation_error"
ERR_UNKNOWN_COMMAND: Final = "unknown_command"
ERR_UNKNOWN_ERROR: Final = "unknown_error"
ERR_UNAUTHORIZED: Final = "unauthorized"
Expand Down
21 changes: 19 additions & 2 deletions homeassistant/components/websocket_api/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,29 @@ def construct_result_message(iden: int, payload: str) -> str:
return f'{{"id":{iden},"type":"result","success":true,"result":{payload}}}'


def error_message(iden: int | None, code: str, message: str) -> dict[str, Any]:
def error_message(
iden: int | None,
code: str,
message: str,
translation_key: str | None = None,
translation_domain: str | None = None,
translation_placeholders: dict[str, Any] | None = None,
) -> dict[str, Any]:
"""Return an error result message."""
error_payload: dict[str, Any] = {
"code": code,
"message": message,
}
# In case `translation_key` is `None` we do not set it, nor the
# `translation`_placeholders` and `translation_domain`.
if translation_key is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to guard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be we don't need to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to remove it, but reverted that, it seems a lot of CI tests assert on the whole payload. If we leave the guard, we need to adjust all the tests.
I suggest that if this is needed we do this in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't complicate the code to avoid updating tests. Check with frontend developers if they prefer to have the keys always there or only there when needed.
There's no need consider saving data here IMHO since sending error responses is not the common case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I do not think it matters in the frontend, as there is no explit typing for the error responses. But we need to update quite a few tests. I would prefer to remove the guard in a different PR to avoid this one to become even bigger.
I asked Paul, if he says we should not add it, then we do not need to change is, but I assume he'll say that it does not matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a discussion with @piitaya on Discord. He believes it is the right way to by only adding the translation attributes when there translation information is defined/available for the exception.

error_payload["translation_key"] = translation_key
error_payload["translation_placeholders"] = translation_placeholders
error_payload["translation_domain"] = translation_domain
return {
"id": iden,
**BASE_ERROR_MESSAGE,
"error": {"code": code, "message": message},
"error": error_payload,
}


Expand Down
7 changes: 7 additions & 0 deletions homeassistant/components/websocket_api/strings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"exceptions": {
"child_service_not_found": {
"message": "Service {domain}.{service} called service {child_domain}.{child_service} which was not found."
}
}
}
27 changes: 25 additions & 2 deletions homeassistant/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,23 @@
class HomeAssistantError(Exception):
"""General Home Assistant exception occurred."""

def __init__(
self,
*args: object,
translation_domain: str | None = None,
translation_key: str | None = None,
translation_placeholders: dict[str, str] | None = None,
) -> None:
"""Initialize exception."""
super().__init__(*args)
self.translation_domain = translation_domain
self.translation_key = translation_key
self.translation_placeholders = translation_placeholders


class ServiceValidationError(HomeAssistantError):
"""A validation exception occurred when calling a service."""


class InvalidEntityFormatError(HomeAssistantError):
"""When an invalid formatted entity is encountered."""
Expand Down Expand Up @@ -165,13 +182,19 @@ class ServiceNotFound(HomeAssistantError):

def __init__(self, domain: str, service: str) -> None:
"""Initialize error."""
super().__init__(self, f"Service {domain}.{service} not found")
super().__init__(
self,
f"Service {domain}.{service} not found.",
translation_domain="homeassistant",
translation_key="service_not_found",
translation_placeholders={"domain": domain, "service": service},
)
self.domain = domain
self.service = service

def __str__(self) -> str:
"""Return string representation."""
return f"Unable to find service {self.domain}.{self.service}"
return f"Service {self.domain}.{self.service} not found."


class MaxLengthExceeded(HomeAssistantError):
Expand Down
4 changes: 4 additions & 0 deletions script/hassfest/translations.py
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,10 @@ def gen_strings_schema(config: Config, integration: Integration) -> vol.Schema:
),
slug_validator=cv.slug,
),
vol.Optional("exceptions"): cv.schema_with_slug_keys(
{vol.Optional("message"): translation_value_validator},
slug_validator=cv.slug,
),
vol.Optional("services"): cv.schema_with_slug_keys(
{
vol.Required("name"): translation_value_validator,
Expand Down
6 changes: 3 additions & 3 deletions tests/components/trace/test_websocket_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def next_id():
_assert_raw_config(domain, sun_config, trace)
assert trace["blueprint_inputs"] is None
assert trace["context"]
assert trace["error"] == "Unable to find service test.automation"
assert trace["error"] == "Service test.automation not found."
assert trace["state"] == "stopped"
assert trace["script_execution"] == "error"
assert trace["item_id"] == "sun"
Expand Down Expand Up @@ -893,7 +893,7 @@ def next_id():
assert len(_find_traces(response["result"], domain, "sun")) == 1
trace = _find_traces(response["result"], domain, "sun")[0]
assert trace["last_step"] == last_step[0].format(prefix=prefix)
assert trace["error"] == "Unable to find service test.automation"
assert trace["error"] == "Service test.automation not found."
assert trace["state"] == "stopped"
assert trace["script_execution"] == script_execution[0]
assert trace["timestamp"]
Expand Down Expand Up @@ -1632,7 +1632,7 @@ def next_id():
assert trace["config"]["id"] == "sun"
assert trace["blueprint_inputs"] == sun_config
assert trace["context"]
assert trace["error"] == "Unable to find service test.automation"
assert trace["error"] == "Service test.automation not found."
assert trace["state"] == "stopped"
assert trace["script_execution"] == "error"
assert trace["item_id"] == "sun"
Expand Down
63 changes: 60 additions & 3 deletions tests/components/websocket_api/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from homeassistant.components.websocket_api.const import FEATURE_COALESCE_MESSAGES, URL
from homeassistant.const import SIGNAL_BOOTSTRAP_INTEGRATIONS
from homeassistant.core import Context, HomeAssistant, State, callback
from homeassistant.exceptions import HomeAssistantError
from homeassistant.exceptions import HomeAssistantError, ServiceValidationError
from homeassistant.helpers import device_registry as dr
from homeassistant.helpers.dispatcher import async_dispatcher_send
from homeassistant.loader import async_get_integration
Expand Down Expand Up @@ -343,6 +343,13 @@ async def test_call_service_not_found(
assert msg["type"] == const.TYPE_RESULT
assert not msg["success"]
assert msg["error"]["code"] == const.ERR_NOT_FOUND
assert msg["error"]["message"] == "Service domain_test.test_service not found."
assert msg["error"]["translation_placeholders"] == {
"domain": "domain_test",
"service": "test_service",
}
assert msg["error"]["translation_key"] == "service_not_found"
assert msg["error"]["translation_domain"] == "homeassistant"


async def test_call_service_child_not_found(
Expand Down Expand Up @@ -370,6 +377,18 @@ async def serv_handler(call):
assert msg["type"] == const.TYPE_RESULT
assert not msg["success"]
assert msg["error"]["code"] == const.ERR_HOME_ASSISTANT_ERROR
assert (
msg["error"]["message"] == "Service non.existing called service "
"domain_test.test_service which was not found."
)
assert msg["error"]["translation_placeholders"] == {
"domain": "non",
"service": "existing",
"child_domain": "domain_test",
"child_service": "test_service",
}
assert msg["error"]["translation_key"] == "child_service_not_found"
assert msg["error"]["translation_domain"] == "websocket_api"


async def test_call_service_schema_validation_error(
Expand Down Expand Up @@ -450,10 +469,26 @@ async def test_call_service_error(

@callback
def ha_error_call(_):
raise HomeAssistantError("error_message")
raise HomeAssistantError(
"error_message",
translation_domain="test",
translation_key="custom_error",
translation_placeholders={"option": "bla"},
)

hass.services.async_register("domain_test", "ha_error", ha_error_call)

@callback
def service_error_call(_):
raise ServiceValidationError(
"error_message",
translation_domain="test",
translation_key="custom_error",
translation_placeholders={"option": "bla"},
)

hass.services.async_register("domain_test", "service_error", service_error_call)

async def unknown_error_call(_):
raise ValueError("value_error")

Expand All @@ -474,20 +509,42 @@ async def unknown_error_call(_):
assert msg["success"] is False
assert msg["error"]["code"] == "home_assistant_error"
assert msg["error"]["message"] == "error_message"
assert msg["error"]["translation_placeholders"] == {"option": "bla"}
assert msg["error"]["translation_key"] == "custom_error"
assert msg["error"]["translation_domain"] == "test"

await websocket_client.send_json(
{
"id": 6,
"type": "call_service",
"domain": "domain_test",
"service": "unknown_error",
"service": "service_error",
}
)

msg = await websocket_client.receive_json()
assert msg["id"] == 6
assert msg["type"] == const.TYPE_RESULT
assert msg["success"] is False
assert msg["error"]["code"] == "service_validation_error"
assert msg["error"]["message"] == "Validation error: error_message"
assert msg["error"]["translation_placeholders"] == {"option": "bla"}
assert msg["error"]["translation_key"] == "custom_error"
assert msg["error"]["translation_domain"] == "test"

await websocket_client.send_json(
{
"id": 7,
"type": "call_service",
"domain": "domain_test",
"service": "unknown_error",
}
)

msg = await websocket_client.receive_json()
assert msg["id"] == 7
assert msg["type"] == const.TYPE_RESULT
assert msg["success"] is False
assert msg["error"]["code"] == "unknown_error"
assert msg["error"]["message"] == "value_error"

Expand Down