Mqtt client_id fix for #8315

[heinemml]

Description:

The fix supplied in #8336 and released with 0.48.1 only fixes cases where an embedded: mqtt config is given. This fix handles also empty mqtt: configs.

Related issue (if applicable): fixes #8315

[houndci-bot]

missing whitespace after ','

[andrey-git]

Could you cover the new code path with a test?

[andrey-git]

Please put 'home-assistant' in a constant and put it as a default value in the schema.

[pvizeli]

I don't think that is a good idea. We had only add this fix to not break our internal broker. And we hope hbmqtt will fix that soon. With this change we are not compilant to protocol, also if the other Broker will make it correct.

I think we should add a info to our docs that in this case, the user need set the client_id manual

[heinemml]

My question would then be: why was the previous fix even merged? It does exactly what you claim "we should not do". It sets a client_id if a broker is configured and no client_id is in the configuration.

[heinemml]

Maybe I elaborate, correct me if I miss something: the currently broken case is the default case which I guess most users use who don't use an external broker. Which is

mqtt:

The current 'bugfix' only fixes cases where the user has the custom version

mqtt:
  embedded:

and as side-effect it sets the client_id for all other broker configurations if it is not set in the config.

So 0.48 broke mqtt for all users of the internal broker and 0.48.1 only fixed it for users of the internal broker which use a custom config but might have broken it for all users of external brokers (at least it forces a client_id).

So what this patch does: fix it for the users of the internal broker.

What might be a better solution:

    if CONF_BROKER in conf:
        broker = conf[CONF_BROKER]
        port = conf[CONF_PORT]
        username = conf.get(CONF_USERNAME)
        password = conf.get(CONF_PASSWORD)
        certificate = conf.get(CONF_CERTIFICATE)
        client_key = conf.get(CONF_CLIENT_KEY)
        client_cert = conf.get(CONF_CLIENT_CERT)
        tls_insecure = conf.get(CONF_TLS_INSECURE)
        protocol = conf[CONF_PROTOCOL]

        # hbmqtt requires a client id to be set.
        if  if CONF_EMBEDDED in conf and client_id is None:
            client_id = 'home-assistant'
    elif broker_config:
        # If no broker passed in, auto config to internal server
        broker, port, username, password, certificate, protocol = broker_config
        # Embedded broker doesn't have some ssl variables
        client_key, client_cert, tls_insecure = None, None, None
        # hbmqtt requires a client id to be set.
        if client_id is None:
            client_id = 'home-assistant'

So you would work around the issue with HBMQTT, don't touch external broker configurations and do not break on updates.

[balloob]

broker_config is set to a value on line 306 but only when the embedded broker will be used. https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/mqtt/__init__.py#L306

So the proper fix is to move the client_id check to the elif block instead of the if block.

[heinemml]

@balloob I applied your suggestion to the pull-request. I agree that this is the most common case which is problematic. This also shouldn't interfere with any other broker configuration.

Maybe a remark in the docs would still make sense for anyone who is using the embedded: config approach and didn't set a client_id?

[balloob]

If they didn't set a client id and use embedded:, we will now always set one 👍 Sorry for putting the patch in the wrong place initially.