Skip to content

Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

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.

Sign up for GitHub

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

Merged
balloob merged 8 commits into from
Jan 29, 2019
Merged

Add PLATFORM_SCHEMA_BASE and use it in alarm_control_panel #20224

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
848d391
Add COMPONENT_SCHEMA and use in alarm and mqtt
emontnemery Jan 18, 2019
7dfa147
Revert MQTT changes
emontnemery Jan 18, 2019
b1107be
Lint
emontnemery Jan 19, 2019
b41dd09
Small tweak
emontnemery Jan 19, 2019
0dd6114
Add tests
emontnemery Jan 19, 2019
b261106
Rename COMPONENT_SCHEMA to PLATFORM_SCHEMA_BASE
emontnemery Jan 26, 2019
74b726c
Fix tests
emontnemery Jan 27, 2019
08b1e5c
Improve tests
emontnemery Jan 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion homeassistant/components/alarm_control_panel/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ( # noqa
COMPONENT_SCHEMA, PLATFORM_SCHEMA_2 as PLATFORM_SCHEMA)
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity import Entity
from homeassistant.helpers.entity_component import EntityComponent
Expand Down
11 changes: 8 additions & 3 deletions homeassistant/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@balloob balloob Jan 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ?

@emontnemery emontnemery Jan 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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..

@balloob balloob Jan 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@emontnemery emontnemery Jan 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)

@balloob balloob Jan 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes

hasattr(component, 'PLATFORM_SCHEMA')):
platforms = []
for p_name, p_config in config_per_platform(config, domain):
# Validate component specific platform schema
try:
p_validated = component.PLATFORM_SCHEMA( # type: ignore
p_config)
if hasattr(component, 'COMPONENT_SCHEMA'):
p_validated = component.COMPONENT_SCHEMA( # type: ignore
p_config)
else:
p_validated = component.PLATFORM_SCHEMA( # type: ignore
p_config)
except vol.Invalid as ex:
async_log_exception(ex, domain, config, hass)
continue
Expand Down
9 changes: 9 additions & 0 deletions homeassistant/helpers/config_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,15 @@ def validator(value):
vol.Optional(CONF_SCAN_INTERVAL): time_period
}, extra=vol.ALLOW_EXTRA)

# This will replace PLATFORM_SCHEMA once all base components are updated
PLATFORM_SCHEMA_2 = vol.Schema({
vol.Required(CONF_PLATFORM): string,
vol.Optional(CONF_SCAN_INTERVAL): time_period
})

COMPONENT_SCHEMA = PLATFORM_SCHEMA_2.extend({
}, extra=vol.ALLOW_EXTRA)

EVENT_SCHEMA = vol.Schema({
vol.Optional(CONF_ALIAS): string,
vol.Required('event'): string,
Expand Down
9 changes: 6 additions & 3 deletions tests/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,9 @@ class MockModule:

# pylint: disable=invalid-name
def __init__(self, domain=None, dependencies=None, setup=None,
requirements=None, config_schema=None, platform_schema=None,
async_setup=None, async_setup_entry=None,
async_unload_entry=None):
requirements=None, component_schema=None, config_schema=None,
platform_schema=None, async_setup=None,
async_setup_entry=None, async_unload_entry=None):
"""Initialize the mock module."""
self.DOMAIN = domain
self.DEPENDENCIES = dependencies or []
Expand All @@ -448,6 +448,9 @@ def __init__(self, domain=None, dependencies=None, setup=None,
if platform_schema is not None:
self.PLATFORM_SCHEMA = platform_schema

if component_schema is not None:
self.COMPONENT_SCHEMA = component_schema

if setup is not None:
# We run this in executor, wrap it in function
self.setup = lambda *args: setup(*args)
Expand Down
107 changes: 104 additions & 3 deletions tests/test_setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
import homeassistant.config as config_util
from homeassistant import setup, loader
import homeassistant.util.dt as dt_util
from homeassistant.helpers.config_validation import PLATFORM_SCHEMA
from homeassistant.helpers.config_validation import (
COMPONENT_SCHEMA, PLATFORM_SCHEMA_2 as PLATFORM_SCHEMA)
from homeassistant.helpers import discovery

from tests.common import \
Expand Down Expand Up @@ -94,18 +95,24 @@ def test_validate_platform_config(self):
platform_schema = PLATFORM_SCHEMA.extend({
'hello': str,
})
component_schema = COMPONENT_SCHEMA.extend({
})
loader.set_component(
self.hass,
'platform_conf',
MockModule('platform_conf', platform_schema=platform_schema))
MockModule('platform_conf',
component_schema=component_schema))

loader.set_component(
self.hass,
'platform_conf.whatever', MockPlatform('whatever'))
'platform_conf.whatever',
MockPlatform('whatever',
platform_schema=platform_schema))

with assert_setup_component(0):
assert setup.setup_component(self.hass, 'platform_conf', {
'platform_conf': {
'platform': 'whatever',
'hello': 'world',
'invalid': 'extra',
}
Expand All @@ -121,6 +128,7 @@ def test_validate_platform_config(self):
'hello': 'world',
},
'platform_conf 2': {
'platform': 'whatever',
'invalid': True
}
})
Expand Down Expand Up @@ -175,6 +183,99 @@ def test_validate_platform_config(self):
assert 'platform_conf' in self.hass.config.components
assert not config['platform_conf'] # empty

def test_validate_platform_config_2(self):
"""Test component COMPONENT_SCHEMA prioritized over PLATFORM_SCHEMA."""
platform_schema = PLATFORM_SCHEMA.extend({
'cheers': str,
'hello': str,
})
component_schema = PLATFORM_SCHEMA.extend({
'hello': str,
Comment thread
emontnemery marked this conversation as resolved.
Outdated

@MartinHjelmare MartinHjelmare Jan 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@emontnemery emontnemery Jan 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation, fixed now!

})
loader.set_component(
self.hass,
'platform_conf',
MockModule('platform_conf',
component_schema=component_schema,
platform_schema=platform_schema))

loader.set_component(
self.hass,
'platform_conf.whatever',
MockPlatform('whatever',
platform_schema=platform_schema))

with assert_setup_component(0):
assert setup.setup_component(self.hass, 'platform_conf', {
'platform_conf': {
'hello': 'world',
'invalid': 'extra',
}
})

self.hass.data.pop(setup.DATA_SETUP)
self.hass.config.components.remove('platform_conf')

with assert_setup_component(1):
assert setup.setup_component(self.hass, 'platform_conf', {
'platform_conf': {
'platform': 'whatever',
'hello': 'world',
},
'platform_conf 2': {
'platform': 'whatever',
'hello': 'world',
'cheers': 'mate'
}
})

self.hass.data.pop(setup.DATA_SETUP)
self.hass.config.components.remove('platform_conf')

def test_validate_platform_config_3(self):
"""Test fallback to component PLATFORM_SCHEMA."""
Comment thread
emontnemery marked this conversation as resolved.
platform_schema = PLATFORM_SCHEMA.extend({
'hello': str,
})
loader.set_component(
self.hass,
'platform_conf',
MockModule('platform_conf',
platform_schema=platform_schema))

loader.set_component(
self.hass,
'platform_conf.whatever',
MockPlatform('whatever',
platform_schema=platform_schema))

with assert_setup_component(0):
assert setup.setup_component(self.hass, 'platform_conf', {
'platform_conf': {
'hello': 'world',
'invalid': 'extra',
}
})

self.hass.data.pop(setup.DATA_SETUP)
self.hass.config.components.remove('platform_conf')

with assert_setup_component(1):
assert setup.setup_component(self.hass, 'platform_conf', {
'platform_conf': {
'platform': 'whatever',
'hello': 'world',
},
'platform_conf 2': {
'platform': 'whatever',
'hello': 'world',
'cheers': 'mate'
}
})

self.hass.data.pop(setup.DATA_SETUP)
self.hass.config.components.remove('platform_conf')

def test_component_not_found(self):
"""setup_component should not crash if component doesn't exist."""
assert not setup.setup_component(self.hass, 'non_existing')
Expand Down