Raise and suppress stack trace when reloading yaml fails #102410

jbouwh · 2023-10-20T18:22:12Z

Proposed change

When reloading yaml fails due to in invalid config. Or other known service error we want the service to be able to raise an error and avoid the need to re-raise in the integration code after when a None config has been received.
In these cases we explictly supply a detailed error message to the user and show it in the UI (translations supported). In these cases a stack trace is not warranted and we only log the stack trace if debug level logging is enabled.

#102592 introduces ServiceValidationError which is a subclass of HomeAssistantError. This PR introduces exception class ConfigValidationError. When this error is raised on a reload request, it is reraised to a ServiceValidationError.

Raising notifications for an invalid config has been moved to setup. In a follow-up PR notifications can be converted to registering issues in the issue registry.

Custom integrations that use config_util.async_process_component_config, e.g. to support reloading yaml, might break as this function now returns config and exception information. To process the returned exception information config_util.async_process_component_config can be used.

Another side effect is that calling these methods no longer will send persistent notifications. As correct exception handling should be handled properly in the UI.

To demonstrate the new code it is applied to the MQTT integration and the reload service.

Replaces: #101037 and #101762

Related architectural discussion:
home-assistant/architecture#992

Implementation plan:

Add the new Exception, add it to the UI service exception handler (including tests and translation support) (Add ServiceValidationError and translation support #102592)
Allow async_integration_yaml_config to raise (we can change the implementation always to raise).
Apply the new solution to the MQTT reload code.

Blog post PR: home-assistant/developers.home-assistant#1986

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add blog post on config validation and error handling developers.home-assistant#1986

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-10-20T18:22:20Z

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (websocket_api) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of websocket_api can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign websocket_api Removes the current integration label and assignees on the pull request, add the integration domain after the command.

home-assistant · 2023-10-20T18:22:20Z

Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign mqtt Removes the current integration label and assignees on the pull request, add the integration domain after the command.

elupus

Ps. This looks to be a better approach than before. But I think it could be simplified by being more specific in what error we are raising.

homeassistant/components/mqtt/__init__.py

jbouwh · 2023-11-06T15:28:04Z

~~Needs a translation_key and domain.~~
DONE

homeassistant/config.py

emontnemery · 2023-11-22T07:22:11Z

homeassistant/config.py

+                if TYPE_CHECKING:
+                    assert isinstance(ex, HomeAssistantError)


Why was this comment marked as resolved?

emontnemery · 2023-11-22T07:25:10Z

homeassistant/config.py

+        config_file: str = "?"
+        line: int | str = "?"


Yes, so? My point is that we don't need to resolve to a string here. Attach the output of find_annotation to config_error_messages instead.

homeassistant/config.py

emontnemery · 2023-11-22T07:42:55Z

homeassistant/components/homeassistant/strings.json

+    "platform_schema_validator_err": {
+      "message": "Unknown error validating config for {p_name} platform for {domain} component with PLATFORM_SCHEMA"
+    },
+    "platform_validator_unknown_err": {


Yes, but why does the user care? I don't see any reason to have both platform_schema_validator_err and platform_validator_unknown_err. Please remove one of them.

homeassistant/config.py

emontnemery · 2023-11-22T07:58:22Z

homeassistant/config.py

@@ -1183,36 +1374,6 @@ async def async_check_ha_config_file(hass: HomeAssistant) -> str | None:
    return res.error_str


-@callback
-def async_notify_setup_error(


This was previously called via async_log_schema_error and async_log_config_validator_error when processing the configuration.
Processing the configuration no longer calls async_log_schema_error and async_log_config_validator_error though, so.

Do we no longer get persistent notifications during startup of Home Assistant when setup calls async_process_component_and_handle_errors ?

I think what needs to happen is that setup.py needs to call async_process_component instead, and create the needed notifications.

There are other callers to async_log_schema_error and async_log_config_validator_error where we do not want notifications to be created. Please remove the calls to async_notify_setup_error from async_log_schema_error and async_log_config_validator_error

Since tests pass without these changes, I think a PR adding tests for the current notification behavior is needed and should be merged before this PR is merged.

Tests were added and merged see: #104384

Notifications are now added via setup.py The split was made.

jbouwh · 2023-11-22T23:18:14Z

#102410 (comment)
Not sure what it was.

emontnemery

This is starting to look really great, just minor comments 👍

Why are there no modifications to bootstrap.py?
bootstrap.py calls async_log_schema_error from here

core/homeassistant/bootstrap.py

Lines 295 to 296 in 91e0a53

    
           conf_util.async_log_schema_error(config_err, core.DOMAIN, core_config, hass) 
        
           return None

, which no longer created notifications. Please make sure that's covered by tests and do the needful in this PR to ensure notifications are still created 👍

homeassistant/components/homeassistant/strings.json

homeassistant/config.py

emontnemery · 2023-11-23T18:27:07Z

homeassistant/config.py

+    platform_name: str
+    config: ConfigType
+    integration_link: str | None = None
+    notify: bool = False


Please remove this, setup.py + bootstrap.py should check the translation key to decide if they want to notify or now

homeassistant/config.py

Co-authored-by: Erik Montnemery <[email protected]>

jbouwh · 2023-11-23T20:33:49Z

This is starting to look really great, just minor comments 👍

Why are there no modifications to bootstrap.py? bootstrap.py calls async_log_schema_error from here

core/homeassistant/bootstrap.py

Lines 295 to 296 in 91e0a53

conf_util.async_log_schema_error(config_err, core.DOMAIN, core_config, hass)

return None

, which no longer created notifications. Please make sure that's covered by tests and do the needful in this PR to ensure notifications are still created 👍

It is correct that bootstrap.py was not changed. As it calls async_log_schema_error it will also notity.
The reworked code in config does not use async_log_schema_error or async_log_homeassistant_error.

A test for bootstrap.py was added to ensure a persistent notification is made.

https://github.com/home-assistant/core/pull/104384/files

tests/test_config.py

homeassistant/config.py

emontnemery

This is great, thanks a lot @jbouwh 👍

As discussed on discord, we should look into removing the call to async_notify_setup_error from async_log_schema_error + async_log_homeassistant_error in a follow-up PR.

The requested changes have been implemented

jbouwh requested review from a team and emontnemery as code owners October 20, 2023 18:22

home-assistant bot added cla-signed code-quality core Hacktoberfest has-tests integration: mqtt integration: websocket_api labels Oct 20, 2023

home-assistant bot assigned emontnemery Oct 20, 2023

home-assistant bot added Quality Scale: internal by-code-owner Quality Scale: gold labels Oct 20, 2023

This was referenced Oct 20, 2023

Suppress logging stack trace on stable build for HomeAssistantError #101762

Closed

Allow async_integration_yaml_config to raise if config fails on reload #101037

Closed

jbouwh force-pushed the stacktraces-on-expected-errors branch from b39616e to 829a234 Compare October 21, 2023 16:44

jbouwh requested a review from elupus October 21, 2023 17:59

elupus reviewed Oct 22, 2023

View reviewed changes

homeassistant/components/mqtt/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/mqtt/__init__.py Show resolved Hide resolved

jbouwh force-pushed the stacktraces-on-expected-errors branch 2 times, most recently from cf598f2 to dd02a9b Compare October 23, 2023 09:35

jbouwh mentioned this pull request Oct 23, 2023

Add ServiceValidationError and translation support #102592

Merged

20 tasks

jbouwh marked this pull request as draft October 23, 2023 16:12

jbouwh force-pushed the stacktraces-on-expected-errors branch from dd02a9b to 4ce71a7 Compare October 27, 2023 08:17

jbouwh force-pushed the stacktraces-on-expected-errors branch from 4ce71a7 to 260133c Compare November 6, 2023 15:21

jbouwh marked this pull request as ready for review November 6, 2023 15:22

jbouwh marked this pull request as draft November 6, 2023 15:27

emontnemery reviewed Nov 7, 2023

View reviewed changes

homeassistant/config.py Outdated Show resolved Hide resolved

homeassistant/config.py Outdated Show resolved Hide resolved

homeassistant/config.py Outdated Show resolved Hide resolved

homeassistant/config.py Show resolved Hide resolved

emontnemery requested changes Nov 22, 2023

View reviewed changes

home-assistant bot marked this pull request as draft November 22, 2023 07:58

jbouwh mentioned this pull request Nov 22, 2023

Add blog post on config validation and error handling home-assistant/developers.home-assistant#1986

Merged

5 tasks

jbouwh and others added 2 commits November 22, 2023 23:01

Refactor and ensure notifications during setup

58cf8c7

Merge branch 'dev' into stacktraces-on-expected-errors

d9679fa

Remove redundat error, adjust tests notifications

9bb07c9

jbouwh marked this pull request as ready for review November 22, 2023 23:44

home-assistant bot requested a review from emontnemery November 22, 2023 23:44

Fix patch

9ebf8eb

emontnemery reviewed Nov 23, 2023

View reviewed changes

jbouwh and others added 2 commits November 23, 2023 19:38

Apply suggestions from code review

9dbfd3d

Co-authored-by: Erik Montnemery <[email protected]>

Follow up comments

4f95b2f

jbouwh and others added 2 commits November 23, 2023 21:23

Add call_back decorator

d09789e

Merge branch 'dev' into stacktraces-on-expected-errors

cb0fbe8

MartinHjelmare previously requested changes Nov 24, 2023

View reviewed changes

home-assistant bot marked this pull request as draft November 24, 2023 11:46

jbouwh added 2 commits November 24, 2023 13:56

Split long lines

592c854

Update exception abbreviations

b06d646

jbouwh marked this pull request as ready for review November 24, 2023 14:55

home-assistant bot requested review from emontnemery and MartinHjelmare November 24, 2023 14:55

emontnemery approved these changes Nov 24, 2023

View reviewed changes

Merge branch 'dev' into stacktraces-on-expected-errors

8ea683d

jbouwh merged commit af71c2b into dev Nov 24, 2023
53 checks passed

jbouwh deleted the stacktraces-on-expected-errors branch November 24, 2023 16:34

jbouwh mentioned this pull request Nov 24, 2023

Do not notify config errors during logging #104466

Merged

20 tasks

github-actions bot locked and limited conversation to collaborators Nov 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise and suppress stack trace when reloading yaml fails #102410

Raise and suppress stack trace when reloading yaml fails #102410

jbouwh commented Oct 20, 2023 •

edited

Loading

home-assistant bot commented Oct 20, 2023

home-assistant bot commented Oct 20, 2023

elupus left a comment

jbouwh commented Nov 6, 2023 •

edited

Loading

emontnemery Nov 22, 2023

emontnemery Nov 22, 2023

emontnemery Nov 22, 2023

emontnemery Nov 22, 2023

jbouwh Nov 22, 2023

jbouwh commented Nov 22, 2023

emontnemery left a comment

emontnemery Nov 23, 2023

jbouwh commented Nov 23, 2023

emontnemery left a comment

	conf_util.async_log_schema_error(config_err, core.DOMAIN, core_config, hass)
	return None

Raise and suppress stack trace when reloading yaml fails #102410

Raise and suppress stack trace when reloading yaml fails #102410

Conversation

jbouwh commented Oct 20, 2023 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Oct 20, 2023

home-assistant bot commented Oct 20, 2023

elupus left a comment

Choose a reason for hiding this comment

jbouwh commented Nov 6, 2023 • edited Loading

emontnemery Nov 22, 2023

Choose a reason for hiding this comment

emontnemery Nov 22, 2023

Choose a reason for hiding this comment

emontnemery Nov 22, 2023

Choose a reason for hiding this comment

emontnemery Nov 22, 2023

Choose a reason for hiding this comment

jbouwh Nov 22, 2023

Choose a reason for hiding this comment

jbouwh commented Nov 22, 2023

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery Nov 23, 2023

Choose a reason for hiding this comment

jbouwh commented Nov 23, 2023

emontnemery left a comment

Choose a reason for hiding this comment

jbouwh commented Oct 20, 2023 •

edited

Loading

jbouwh commented Nov 6, 2023 •

edited

Loading