Correct expose_by_default interaction with expose_domains

[glentakahashi]

Home Assistant release with the issue:

0.80.3

Last working Home Assistant release (if known):

Operating environment (Hass.io/Docker/Windows/etc.):

Component/platform:

https://www.home-assistant.io/components/google_assistant/#expose_by_default

Description of problem:
Based on the documentation here: https://www.home-assistant.io/components/google_assistant/#expose_by_default it seems that expose_by_default means all devices should be exposed unless explicitly set to false, and that regardless if this is set domains in exposed_domains should be exposed.

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

google_assistant:
  project_id: xxx
  api_key: xxx
  expose_by_default: false
  exposed_domains:
    - switch
    - light

Traceback (if applicable):

Additional information:

[homeassistant]

Hi @glentakahashi,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[rohankapoorcom]

Unclear to me whether this is actually needed or not, but regardless it should be targeted at the dev branch not master

[balloob]

It's an and check so that people don't expose domains that we don't support.

[glentakahashi]

That doesn't make sense to me. The documentation for expose_by_default says:

Expose devices in all supported domains by default. If set to false, you need to either expose domains or add the expose configuration option to each entity in entity_config and set it to true.

I'm understanding this as, an entity will be exposed if:

1. expose_by_default is true and is not manually excluded
2. expose_by_default is false and either:
3. the domain is exposed by exposed_domains
4. the device is manually exposed in the entity_config

Right now (using and) the logic is coded to expose things if:

1. expose_by_default is true, the domain is in exposed_domains, and is not manually excluded
2. expose_by_default is false and the device is manually exposed in entity_config

So which one is right here? If the docs are wrong I can make a PR to fix those, but in my opinion the docs behavior is the more intuitive one and this PR will adjust the code to work like that.

[balloob]

If a domain is not in expose_by_default, it's not supported. So no need to route entities through the GA flow and then make it generate a ton of warnings.

[glentakahashi]

@balloob I still believe either the documentation or code needs changing here. The documentation doesn't match up with how the code works. Take this example. Let's say I have three devices in three domains, one of which is unsupported:

Devices:

• bedroom_light - LIGHT
• bedroom_switch - SWITCH
• bedroom_test - TEST

Current behavior:

expose_by_default: true

LIGHT, SWITCH are exposed

expose_by_default: true
exposed_domains: LIGHT

only LIGHT is exposed

expose_by_default: false
exposed_domains: LIGHT

no devices are exposed

expose_by_default: false
exposed_domains: LIGHT
devices:
  bedroom_switch:
    expose: true

only SWITCH is exposed

Using OR behavior:

expose_by_default: true

LIGHT, SWITCH are exposed

expose_by_default: true
exposed_domains: LIGHT

LIGHT, SWITCH are exposed

expose_by_default: false
exposed_domains: LIGHT

LIGHT is exposed

expose_by_default: false
exposed_domains: LIGHT
devices:
  bedroom_switch:
    expose: true

LIGHT, SWITCH are exposed

To me, use cases #3 and #4 are what seem wrong in the current behavior. I see someone updated the documentation today to make it clear this is how it works (home-assistant/home-assistant.io#7292) but I think the config can still be improved.

In my opinion, if not the OR behavior above there are a few other alternatives:

• remove expose_by_default. Defaults are handled in the default setting of exposed_domains (actually how it works already, https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/google_assistant/__init__.py#L48)
• Don't allow expose_by_default and exposed_domains to be set at the same time, or ignore expose_by_default if it's set.

Lastly, currently the code itself doesn't protect against unsupported domains in GA either. The only line of code that uses the filtered set of domains is in the default configuration above. To actually support this, I think instead in const.py there should be a list of supported_domains and that is intersected with the provided exposed_domains and device sets. Right now, if I were to add a exposed_domains: TEST, the code would actually expose the TEST domain to GA.

Sorry for the long post, but just want to clear this up.

[balloob]

I'm sorry, looks like you're right.

[balloob]

we need to revert this change because it's causing issues #18856

[glentakahashi]

oh no! Sorry about the issues that this is causing. Would you be okay with me putting out another PR with better functionality + testing after you revert?

[balloob]

If we do so, it should be done as a non breaking change.