Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

emontnemery · 2019-01-18T18:37:40Z

Description:

PLATFORM_SCHEMA in config_validation.py sets extra=vol.ALLOW_EXTRA
This means platforms will silently swallow mistyped configuration variables.

To fix this:

Add new base schema PLATFORM_SCHEMA_BASE for use in base platforms. This schema sets extra=vol.ALLOW_EXTRA
Temporarily add a PLATFORM_SCHEMA_2 which doesn't set extra=vol.ALLOW_EXTRA
Update base platforms to expose both PLATFORM_SCHEMA_BASE and PLATFORM_SCHEMA_2 as PLATFORM_SCHEMA
Update config.py to check the component for either PLATFORM_SCHEMA_BASE or PLATFORM_SCHEMA, with priority for PLATFORM_SCHEMA_BASE
Start migrating components

When all are migrated:

Remove PLATFORM_SCHEMA_2 and remove extra=vol.ALLOW_EXTRA from PLATFORM_SCHEMA
Update config.py to only check for PLATFORM_SCHEMA_BASE

This first PR demonstrates the concept and uses it for alarm_control_panel.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

tests/components/mqtt/test_alarm_control_panel.py

homeassistant/config.py

homeassistant/components/mqtt/light/__init__.py

homeassistant/components/alarm_control_panel/__init__.py

tests/components/mqtt/test_alarm_control_panel.py

homeassistant/components/mqtt/light/__init__.py

balloob · 2019-01-18T18:41:30Z

homeassistant/components/alarm_control_panel/__init__.py

@@ -13,7 +13,8 @@
    ATTR_CODE, ATTR_CODE_FORMAT, ATTR_ENTITY_ID, SERVICE_ALARM_TRIGGER,
    SERVICE_ALARM_DISARM, SERVICE_ALARM_ARM_HOME, SERVICE_ALARM_ARM_AWAY,
    SERVICE_ALARM_ARM_NIGHT, SERVICE_ALARM_ARM_CUSTOM_BYPASS)
-from homeassistant.helpers.config_validation import PLATFORM_SCHEMA  # noqa
+from homeassistant.helpers.config_validation import (
+    COMPONENT_SCHEMA, PLATFORM_SCHEMA_2 as PLATFORM_SCHEMA)  # noqa


Importing platform schema here is wrong ? It should have allow_extra set ?

We now expose COMPONENT_SCHEMA with has allow_extra set, and PLATFORM_SCHEMA which does not. PLATFORM_SCHEMA may be used as base schema by platforms implementing the alarm component.

balloob · 2019-01-18T18:43:17Z

homeassistant/helpers/config_validation.py

@@ -499,11 +499,22 @@ def validator(value):

 # Schemas

+COMPONENT_SCHEMA = vol.Schema({


I would expect it to be structured like this:

PLATFORM_SCHEMA_BASE = vol.Schema({ vol.Required(CONF_PLATFORM): string, vol.Optional(CONF_SCAN_INTERVAL): time_period }) PLATFORM_SCHEMA = PLATFORM_SCHEMA_BASE.extend({ }, extra=vol.ALLOW_EXTRA)

And components will import PLATFORM_SCHEMA, platforms will import PLATFORM_SCHEMA_BASE

Fixed as you suggest, except for changing the names.

If also changing names, every single platform module will have to be updated.
Also, platforms are supposed to extend the component schema if I understood @MartinHjelmare correctly.

Example: https://github.com/home-assistant/home-assistant/blob/e80702a45ce575b89c39a0ca4e8b373bfc33e3e3/homeassistant/components/alarm_control_panel/alarmdotcom.py#L13

MartinHjelmare

Looks good! We should add or update the test for config.py.

emontnemery · 2019-01-19T14:30:45Z

@MartinHjelmare Tests added. Existing test updated + fixed.

balloob · 2019-01-25T06:49:36Z

homeassistant/config.py

@@ -743,13 +743,18 @@ def async_process_component_config(
            async_log_exception(ex, domain, config, hass)
            return None

-    elif hasattr(component, 'PLATFORM_SCHEMA'):
+    elif (hasattr(component, 'COMPONENT_SCHEMA') or


It's weird that the word COMPONENT_SCHEMA triggers platform validation.

The idea was: Oh it has a platform schema, let's treat the config as being a list of platforms that all have to validate. Can't we call it PLATFORM_BASE_SCHEMA, given that it is the schema that is the basis for all platform schemas of that entity component ?

So you're suggesting as before that the component will export:

PLATFORM_SCHEMA_BASE = vol.Schema({ vol.Required(CONF_PLATFORM): string, vol.Optional(CONF_SCAN_INTERVAL): time_period }) PLATFORM_SCHEMA = PLATFORM_SCHEMA_BASE.extend({ }, extra=vol.ALLOW_EXTRA)

If you do like that, a lot of modules implementing a platform have to be updated to import and extend PLATFORM_SCHEMA_BASE instead of PLATFORM_SCHEMA.

I don't really mind, but it's a lot of changes..

no no, we flip it. The BASE is not what normal platform extends from, BASE is validating that the bare minimum of a valid platform config is validated.

OK, that will be easier!

Just to double confirm, this is what you mean:

PLATFORM_SCHEMA = vol.Schema({ vol.Required(CONF_PLATFORM): string, vol.Optional(CONF_SCAN_INTERVAL): time_period }) PLATFORM_SCHEMA_BASE = PLATFORM_SCHEMA.extend({ }, extra=vol.ALLOW_EXTRA)

tests/test_setup.py

MartinHjelmare · 2019-01-27T12:39:08Z

tests/test_setup.py

+            'hello': str,
+        })
+        platform_schema_base = PLATFORM_SCHEMA.extend({
+            'hello': str,


Eg if we do:

Suggested change

'hello': str,

'hello': 'world',

We can below check if platforms validate this item correctly, instead of adding other keys, since the schema should allow extra.

Thanks for the explanation, fixed now!

balloob

Looks good!

emontnemery · 2019-02-02T10:10:16Z

Added breaking change label as suggested in discord beta channel.

veleek · 2019-02-15T07:41:43Z

@emontnemery - Do you have a good example of what I'm supposed to do to update a component to work properly with this change?

I'm running into this issue with the telegram_bot component because the telegram_bot.webhooks platform has additional parameters and the config validation fails when checking against the base platforms PLATFORM_SCHEMA.

What modifications need to be done to fix this? If the platform specific schema (e.g. PLATFORM_SCHEMA defined in telegram_bot/webhooks.py) extends the PLATFORM_SCHEMA in telegram_bot/init.py, why does the config validation bother validating against the base schema? Or is it just because of a misconfiguration of this component that that's an issue?

MartinHjelmare · 2019-02-15T07:46:18Z

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

Add COMPONENT_SCHEMA and use in alarm and mqtt

848d391

emontnemery requested a review from a team as a code owner January 18, 2019 18:37

homeassistant added cla-signed integration: alarm_control_panel integration: mqtt core labels Jan 18, 2019

ghost assigned emontnemery Jan 18, 2019

ghost added the in progress label Jan 18, 2019

houndci-bot reviewed Jan 18, 2019

View reviewed changes

balloob reviewed Jan 18, 2019

View reviewed changes

tests/components/mqtt/test_alarm_control_panel.py Outdated Show resolved Hide resolved

balloob reviewed Jan 18, 2019

View reviewed changes

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

balloob reviewed Jan 18, 2019

View reviewed changes

Revert MQTT changes

7dfa147

MartinHjelmare reviewed Jan 18, 2019

View reviewed changes

emontnemery added 2 commits January 19, 2019 10:28

Lint

b1107be

Small tweak

b41dd09

emontnemery removed the integration: mqtt label Jan 19, 2019

Add tests

0dd6114

emontnemery changed the title ~~Add COMPONENT_SCHEMA and use it in alarm and mqtt~~ Add COMPONENT_SCHEMA and use it in alarm_control_panel Jan 19, 2019

balloob reviewed Jan 25, 2019

View reviewed changes

Rename COMPONENT_SCHEMA to PLATFORM_SCHEMA_BASE

b261106

MartinHjelmare reviewed Jan 26, 2019

View reviewed changes

tests/test_setup.py Outdated Show resolved Hide resolved

tests/test_setup.py Show resolved Hide resolved

Fix tests

74b726c

MartinHjelmare reviewed Jan 27, 2019

View reviewed changes

Improve tests

08b1e5c

emontnemery force-pushed the component_schema_alarm branch from 549b051 to 08b1e5c Compare January 27, 2019 13:30

balloob approved these changes Jan 29, 2019

View reviewed changes

balloob merged commit d7ba2aa into home-assistant:dev Jan 29, 2019

ghost removed the in progress label Jan 29, 2019

emontnemery mentioned this pull request Jan 29, 2019

Use PLATFORM_SCHEMA_BASE as base schema for MQTT platforms. #20562

Merged

3 tasks

emontnemery changed the title ~~Add COMPONENT_SCHEMA and use it in alarm_control_panel~~ Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel Jan 29, 2019

emontnemery mentioned this pull request Jan 29, 2019

Use PLATFORM_SCHEMA_BASE as base schema for additional components. #20578

Merged

3 tasks

emontnemery deleted the component_schema_alarm branch January 30, 2019 11:52

emontnemery mentioned this pull request Feb 1, 2019

Add PLATFORM_SCHEMA_BASE support to check_config.py #20663

Merged

3 tasks

emontnemery added the breaking-change label Feb 2, 2019

emontnemery mentioned this pull request Feb 2, 2019

Add entity_namespace to PLATFORM_SCHEMA #20693

Merged

3 tasks

balloob mentioned this pull request Feb 6, 2019

0.87.0 #20794

Merged

home-assistant locked as resolved and limited conversation to collaborators Feb 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

emontnemery commented Jan 18, 2019 •

edited

balloob Jan 18, 2019

emontnemery Jan 18, 2019 •

edited

balloob Jan 18, 2019

emontnemery Jan 18, 2019 •

edited

MartinHjelmare left a comment

emontnemery commented Jan 19, 2019

balloob Jan 25, 2019

emontnemery Jan 25, 2019

balloob Jan 25, 2019

emontnemery Jan 26, 2019

balloob Jan 28, 2019

MartinHjelmare Jan 27, 2019

emontnemery Jan 27, 2019

balloob left a comment

emontnemery commented Feb 2, 2019

veleek commented Feb 15, 2019

MartinHjelmare commented Feb 15, 2019

		@@ -499,11 +499,22 @@ def validator(value):

		# Schemas

		COMPONENT_SCHEMA = vol.Schema({

Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

Conversation

emontnemery commented Jan 18, 2019 • edited

Description:

Checklist:

Choose a reason for hiding this comment

emontnemery Jan 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Jan 18, 2019 • edited

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

emontnemery commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

emontnemery commented Feb 2, 2019

veleek commented Feb 15, 2019

MartinHjelmare commented Feb 15, 2019

emontnemery commented Jan 18, 2019 •

edited

emontnemery Jan 18, 2019 •

edited

emontnemery Jan 18, 2019 •

edited