Skip to content
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

Keep overkiz unique ids stable for pyoverkiz >= 1.10.1 #99765

Closed
wants to merge 1 commit into from
Closed

Keep overkiz unique ids stable for pyoverkiz >= 1.10.1 #99765

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion homeassistant/components/overkiz/entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Parent class for every Overkiz device."""
from __future__ import annotations

from enum import StrEnum
from typing import cast

from pyoverkiz.enums import OverkizAttribute, OverkizState
Expand Down Expand Up @@ -114,7 +115,12 @@ def __init__(
"""Initialize the device."""
super().__init__(device_url, coordinator)
self.entity_description = description
self._attr_unique_id = f"{super().unique_id}-{self.entity_description.key}"

# Keep ids stable for pyoverkiz >= 1.10.1 where enums changed to StrEnum.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment is wrong, shouldn't it be like this?

Suggested change
# Keep ids stable for pyoverkiz >= 1.10.1 where enums changed to StrEnum.
# Keep ids stable for pyoverkiz >= 1.10.0 where enums changed to StrEnum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This needs to be 1.10.0 here and in the commit message. There's a chance that github would lose the discussion below if I change the commit in the PR. I'd therefore only do that in a later step if we go on with this PR.

unique_id_ext = self.entity_description.key
if isinstance(unique_id_ext, StrEnum):
unique_id_ext = f"{unique_id_ext.__class__.__name__}.{unique_id_ext.name}"
self._attr_unique_id = f"{super().unique_id}-{unique_id_ext}"
Comment on lines +120 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, can we rewrite unique IDs via a migration step?

Here's an example of how it can be done:

core/homeassistant/components/bmw_connected_drive/__init__.py

Lines 82 to 110 in 714fc53

@callback
def update_unique_id(entry: er.RegistryEntry) -> dict[str, str] | None:
replacements = {
"charging_level_hv": "remaining_battery_percent",
"fuel_percent": "remaining_fuel_percent",
}
if (key := entry.unique_id.split("-")[-1]) in replacements:
new_unique_id = entry.unique_id.replace(key, replacements[key])
_LOGGER.debug(
"Migrating entity '%s' unique_id from '%s' to '%s'",
entry.entity_id,
entry.unique_id,
new_unique_id,
)
if existing_entity_id := entity_registry.async_get_entity_id(
entry.domain, entry.platform, new_unique_id
):
_LOGGER.debug(
"Cannot migrate to unique_id '%s', already exists for '%s'",
new_unique_id,
existing_entity_id,
)
return None
return {
"new_unique_id": new_unique_id,
}
return None
await er.async_migrate_entries(hass, config_entry.entry_id, update_unique_id)

Copy link
Contributor

@emontnemery emontnemery Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought, this may be very tricky to get 100% correct, rewriting the unique_id as suggested in this PR is fine.

Copy link
Contributor

@iMicknl iMicknl Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@emontnemery it sucks that we didn't catch this before Python 3.11 was added to HA (no tests in Overkiz is the main cause). The Python3.11 update had a breaking change in how (str, Enum) are handled. See https://blog.pecar.me/python-enum.

History:
< 2023.3: unique_id="io://XXXX-XXXX-XXXX/XXXXXXX-core:DiscreteRSSILevelState"
> 2023.3 (with Python 3.11): unique_id="io://XXXX-XXXX-XXXX/XXXXXXX-OverkizState.CORE_DISCRETE_RSSI_LEVEL"
2023.9.0 (move to StrEnum in pyOverkiz): unique_id="io://XXXX-XXXX-XXXX/XXXXXXX-core:DiscreteRSSILevelState"
2023.9.1 (revert pyOverkiz): unique_id="io://XXXX-XXXX-XXXX/XXXXXXX-OverkizState.CORE_DISCRETE_RSSI_LEVEL"

So by not catching it, we actually broke the entity registry multiple times for our users. This happens to entities that I am not actively using myself, thus I didn't spot it earlier. Would be good to fix it now once and for all and move back to the original unique id.

Copy link
Contributor

Choose a reason for hiding this comment

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

First try to implement your suggestion: #101024.


if self.is_sub_device:
# In case of sub device, use the provided label
Expand Down