We should detect duplicate keys when loading configuration.yaml

[jaharkes]

Home Assistant release (hass --version):

Python release (python3 --version):

Component/platform:
homeassistant/util/yaml.py

Description of problem:
yaml parser accepts duplicate keys. So when someone forgets to add '-' to a list of mapping type entries there may be no error message or the error is confusing.

On gitter.im I noticed a message from @frelev who had an automation that didn't load correctly (see below configuration sample). The error message was quite unhelpful,
": extra keys not allowed @ data['condition'][0]['weekday']."

The reason we get this error message is because the second platform: state entry overwrite platform: time and the config validation for a state automation does not expect or accept a weekday entry. I can easily imagine two independent platform:state conditions that would be silently accepted, but not work correctly.

Expected:
A duplicate key in a configuration file should trigger a load/parse error. But we cannot make the OrderedDict write-once for items so we probably need to introduce a proxy-object while loading the configuration that can catch whenever an existing key is about to be overwritten.

Problem-relevant configuration.yaml entries and steps to reproduce:

alias: Turn on bathroom player weekdays
trigger:
  platform: time
  after: '16:15'
condition:
  platform: time
  weekday:
    - mon
    - tue
    - wed
    - thu
    - fri
  platform: state
  entity_id: media_player.toalett
  state: 'off'
action:
  service: media_player.media_play
  entity_id: media_player.toalett

Traceback (if applicable):

Additional info:

[balloob]

We could extend ordered dict and have it raise an error when overwriting a
key. That way the yaml config loader would fail

On Tue, Apr 5, 2016, 09:31 Jan Harkes notifications@github.com wrote:

Home Assistant release (hass --version):

Python release (python3 --version):

Component/platform:
homeassistant/util/yaml.py

Description of problem:
yaml parser accepts duplicate keys. So when someone forgets to add '-' to
a list of mapping type entries there may be no error message or the error
is confusing.

On gitter.im I noticed a message from @frelev https://github.com/frelev
who had an automation that didn't load correctly (see below configuration
sample). The error message was quite unhelpful,
": extra keys not allowed @ data['condition'][0]['weekday']."

The reason we get this error message is because the second platform: state
entry overwrite platform: time and the config validation for a state
automation does not expect or accept a weekday entry. I can easily imagine
two independent platform:state conditions that would be silently
accepted, but not work correctly.

Expected:
A duplicate key in a configuration file should trigger a load/parse error.
But we cannot make the OrderedDict write-once for items so we probably need
to introduce a proxy-object while loading the configuration that can catch
whenever an existing key is about to be overwritten.

Problem-relevant configuration.yaml entries and steps to reproduce:

alias: Turn on bathroom player weekdaystrigger:
platform: time
after: '16:15'condition:
platform: time
weekday:
- mon
- tue
- wed
- thu
- fri
platform: state
entity_id: media_player.toalett
state: 'off'action:
service: media_player.media_play
entity_id: media_player.toalett

Traceback (if applicable):

Additional info:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1714

[jaharkes]

Yes, but I assume we are overwriting at least some keys with coercion/validation, so we probably have to copy the write-once ordered dict to a normal ordered dict between yaml parsing and config validation.

[balloob]

We could have our YAML parser method convert everything back before returning it (or maybe just have a boolean on our OrderedDict extension to unfreeze)

[jaharkes]

I was thinking, we might already be doing the conversion because the groups were out of order for a bit after config validation was added. But I like the boolean to prevent key overwrites too.

[balloob]

The group one was broken because at that time I did not understand voluptuous too well: https://github.com/balloob/home-assistant/blob/dev/homeassistant/components/group.py#L51

I guess that can be replaced with a simple dict validator.

[jaharkes]

Oh, this is going to be nice. The nodes parsed from yaml are all passed into the class initializer and not through setattr. So we don't really need to prevent updates through setattr, just check for duplicates in __init__.

[balloob]

Fixed! 🎉