Use class inheritance for DiscoveryInfoType

[epenet]

Currently DiscoveryInfoType is defined as Dict[str, Any]:

DiscoveryInfoType = Dict[str, Any]

In zeroconf, the discovered service information is defined as a TypedDict:

class ZeroconfServiceInfo(TypedDict):
    """Prepared info from mDNS entries."""

    host: str
    port: int | None
    hostname: str
    type: str
    name: str
    properties: dict[str, Any]

While this doesn't cause any issue in python, if we adjust the signature of async_step_zeroconf to have discovery_info: zeroconf.ZeroconfServiceInfo it causes issues for mypy.

I propose that we change DiscoveryInfoType to be a TypedDict that would be inherited by ZeroconfServiceInfo and the other discovery services. Or create a new parent/base class BaseServiceInfo or DiscoveryServiceInfo?

Linked PR: home-assistant/core#59503

[scop]

Very much +1 to typing this better. OTOH I'm afraid that discovery infos for different things contain quite different things and unifying all of it might be a bit of work.

[epenet]

I've investigated this further, and it does look like it doesn't make sense to unify.
I think that each discovery process should still have a different output, and should be cast explicitely to dict only when required.

• mqtt could have MqttServiceInfo(TypedDict) with six keys (topic, payload, qos, retain, subscribed_topic, timestamp)
• dhcp could have DhcpServiceInfo(TypedDict) with three keys (ip, hostname, macaddress)
• homekit could have HomekitServiceInfo inherited from ZeroconfServiceInfo (or just ZeroconfServiceInfo)
• usb either use the dataclass (USBDevice) or convert it to UsbDeviceInfo(TypedDict)

I am not sure (yet) what ServiceInfo would look like for hassio and ssdp, but I am happy to investigate it further (maybe with total=false to have only the most common properties defined if there are too many to list).

[epenet]

PRs in progress:

• zeroconf => Adjust async_step_zeroconf signature for strict typing core#59503
• homekit => Adjust async_step_homekit signature for strict typing core#59745
• dhcp => Adjust async_step_dhcp signature for strict typing core#59751
• mqtt => Adjust async_step_mqtt signature for strict typing core#59761
• usb => Adjust async_step_usb signature for strict typing core#59773
• ssdp => Adjust async_step_ssdp signature for strict typing core#59779
• hassio => TBD

[epenet]

I made an attempt at the ssdp signature, but I had difficulty finding out which elements are always present and which are optional, and what is the expected type for each.

I first attempted to use a TypedDict (in home-assistant/core#59779), but the changes in the components got really messy. Is there some documentation about which keys are compulsory, and of what type they are?

cc @chishm, @bdraco, @StevenLooman as the main "commiters" to the ssdp component.
(I am not sure what the best way is to contact the ssdp maintainers so I hope this is not taken badly if I make a mis-step here.)

[StevenLooman]

I am for using SsdpServiceInfo for ssdp discoveries. SSDP/UPnP-device-descriptions (those are merged in the ssdp-component) allows for custom/vendor specific properties. You are covering this with the total=False parameter. So I think this should work perfectly and is a more elegant solution than using a dict with some static/const keys.

[epenet]

I have updated the PR, in which only x-homeassistant-matching-domains is a compulsory key. All others are optional.
My main interrogation now is which keys to add as "mypy hints" and which ones to ignore.

[chishm]

I like this too. I think we can subclass the `SsdpServiceInfo` to include the UPnP details once they have been retrieved from the device (`ATTR_UPNP*` keys). That would make it more obvious what details are available when the discovery info is used.

[epenet]

I am not sure what you mean about subclassing SsdpServiceInfo. Are you referring to this comment? home-assistant/core#59779 (comment)

[chishm]

Yep, and the follow-up comment I left. Please let me know if I can add more details.

[chishm]

Also, it might be useful to have all of these discovery infos derive from a common class that includes one element, `unique_id`, that can be used in triggered config flows. But I'm not familiar with all the discovery types to say if this is feasible.

[epenet]

This was my original idea, but the various discovery components don't have any keys in comon (not even unique_id)

[chishm]

Perhaps the discovery components could each pick an existing field to copy into the base unique_id? But the more I think about it, the less utility I see in that.

[StevenLooman]

UPnP states the UDN is the unique device name (thus the unique identifier for the device.) Components are free to do as they please though.

Worse, some think it is wise to rotate the UDN (as seen with some routers) regularly, completely defying the use of uniquely identifying property. Not sure why, if it is just randomly generated during start up of the device/upnp handling software, or if this is some kind of "security" practice (you can scan for the device anyway and possibly identify it through other properties...)

[Jc2k]

I think it's potentially a little off topic for this exact topic, but @epenet said it would be good to mention here.

Although not strictly part of using class inheritance for DiscoveryInfoType we have started to more heavility user ATTR_ const. I wonder whether it is worth revisiting that now we have typing and dataclasses. First of all an example:

await self.async_set_unique_id(
    discovery_info[zeroconf.ATTR_PROPERTIES][zeroconf.ATTR_PROPERTIES_ID]
)

vs

await self.async_set_unique_id(discovery_info["properties]["id"])

I haven't been able to find the historical motivation for ATTR_ constants, but i'm pretty certain they predate our use of typing information (and certainly of TypedDict). But I assume the motivation was either:

• to provide a form of type safety (it's easy to type 'propertes' and it go missed without good testing)
• to allow a constants value to be changed globally

I don't think (2) is common, and we handle codebase wide changes all the time so i'm not entirely convinved this was a motivator. (1) feels like the common case, but I wonder if typing means it is not needed.

What have I got against ATTR const (crucially, specifically in well typed code)?

• They require duplication of what you put in the typing.TypedDict
• They are long. "id" becomes "zeroconf.ATTR_PROPERTIES_ID"
• With a typing.TypedDict, I am already blocked from typos. My example "propertes" is blocked by mypy.
• With a typing.TypedDict we get autocomplete in IDE's (only tested in VSCode, which is what HA reccommends). But the ATTR_const are not in anyway bound to the type information. So I get autocomplete for "properties", but not "zeroconf.ATTR_PROPERTIES".
• Because the const are not bound to the type information, there is no automated way to enforce their use. This means it must be done at code review by a person. It can be missed, or it adds extra burden on the reviewer.

So I guess the question I want to ask after considering these points is:

• Is discovery_info["properties] actually that bad given the above points?
• Would a dataclass be even better?
• Or can we create our own hybrid metaclass. Like a TypedDict where ZeroconfServiceInfo.id (class, not instance) returns "id". Or a dataclass that supported __getitem__ etc.

I don't have the answers to this, but especially after realising it makes the IDE experience worse, I thought it was an interesting conversation to have.

[epenet]

I have decided to hold off on adding the constants to mqtt. And maybe we should revert the new constants in usb (cc @bdraco: home-assistant/core#60086).

The whole config flow structure is based on dict:

• user_input is always a dict
• discovery_info is always a dict
• the result (FlowResult) is always a dict

This was the main reason why I avoided dataclass as I felt it would be more likely to bring in breaking changes.
If the ConfigFlow would support dataclass transparently (possibly using __getitem__, etc.), and there is a simple way to migrate from TypedDict to dataclass it's something that I can look at as a follow-up.

[bdraco]

From my persepective, a dataclass is clearly a better solution since it still quite fast, has less chance of error, and makes names more easily discoverable.

Thinking long term, I don't think we want both ways to access the data since it will make it more difficult to review and we won't always get the benefits of using a dataclass.

I'd suggest instead and logging a warning that __getitem__ will fail in version 2022.6 when its accessed via __getitem__ to use <cls>.<name> instead.

We don't want to start logging until everything in core is transitioned for each specific discovery type. Since other discovery types might longer transition periods it might be best to implement this as part of DhcpServiceInfo since it won't be possible to change every discovery flow in a single change.

[Jc2k]

I think a plain dataclass is a good soluton too, with the caveat I am not familiar enough with config flow internals to know if there are any implications to using a dataclass here.

[epenet]

This has now been implemented for zeroconf/homekit/dhcp/mqtt/usb/ssdp:

zeroconf => home-assistant/core#60206
homekit => home-assistant/core#60206
dhcp => home-assistant/core#60136
mqtt => home-assistant/core#60191
usb => home-assistant/core#60140
ssdp => home-assistant/core#59931

[epenet]

For Hassio, is there an appetite for this implement? home-assistant/core#60824

[frenck]

This has been implemented.