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

fetzerch · 2023-09-06T19:09:10Z

Proposed change

With pyoverkiz >= 1.10.1 the library uses StrEnums that have a different string representation than the previously used Enums (enum value vs. enum class + option name). Keep using enum class + option name in order to ensure stable unique_ids.

This fixes the problem that upgrading pyoverkiz would lead to new entities.

See iMicknl/python-overkiz-api#923 (comment) for a more detailed problem description.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes ONLY 2023.9.0 Overkiz breaks some unique_id [fixed on 2023.9.1] #99404
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

With pyoverkiz >= 1.10.1 the library uses StrEnums that have a different string representation than the previously used Enums (enum value vs. enum class + option name). Keep using enum class + option name in order to ensure stable unique_ids. This fixes the problem that upgrading pyoverkiz would lead to new entities.

emontnemery · 2023-09-07T09:24:45Z

homeassistant/components/overkiz/entity.py

@@ -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.


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.

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.

emontnemery · 2023-09-07T09:27:23Z

homeassistant/components/overkiz/entity.py

+        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}"


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)

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

@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.

First try to implement your suggestion: #101024.

home-assistant · 2023-09-07T09:27:30Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

emontnemery · 2023-09-07T09:34:54Z

PR #99742 reverted the library bump, this PR should include the bump too.

home-assistant · 2023-09-07T10:08:37Z

Hey there @iMicknl, @vlebourl, @tetienne, @nyroDev, mind taking a look at this pull request as it has been labeled with an integration (overkiz) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of overkiz can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign overkiz Removes the current integration label and assignees on the pull request, add the integration domain after the command.

Quentame · 2023-09-08T20:35:17Z

@fetzerch @emontnemery

What do you think of that #99950 (comment) ?

Quentame · 2023-09-08T20:37:29Z

We can just add an alert to prevent users for 2023.9.0 breaking change and advice to skip .0 and go directly to 2023.9.1+ !

emontnemery · 2023-09-11T07:22:06Z

We can just add an alert to prevent users for 2023.9.0 breaking change and advice to skip .0 and go directly to 2023.9.1+ !

I still think we should clean this up as requested in #99404 (comment), I paste it here again:

The issue will hurt everyone who upgraded to 2023.9.0, because they will now have duplicated overkiz entities, with the old entities' state set to "UNAVAILABLE". The duplicates added by 2023.9.0 will have a new entity ID meaning any automation etc. referencing them will no longer work.

To properly fix this mess I think the following is needed:

Decide if the library changes should be adjusted for backwards compatibility
- For backwards compatibility, a possibility would be have the library use custom StrEnum class like this:
```
from enum import StrEnum

class CompabilityStrEnum(StrEnum):
    TEST = "test"

    def __str__(self) -> str:
        """Override StrEnum.__str__ for compability."""
        return f"{self.__class__.__name__}.{self.name}"

print(f"{CompabilityStrEnum.TEST}")
```
- Without the backwards compatibility, i.e. the library changes are not reverted, rewrite the unique id as in Keep overkiz unique ids stable for pyoverkiz >= 1.10.1 #99765 (this PR)
Add cleanup code in overkiz async_setup_entry which searches for duplicated entities added in 2023.9.0 and removes them

fetzerch · 2023-09-11T11:50:33Z

I can't really decide if this PR or the CompabilityStrEnum is the better solution as I'm relatively new to Home Assistant (and the Overkiz integration). I fear that this would be something for the integration maintainers to decide.

emontnemery · 2023-09-11T13:32:13Z

@fetzerch Right, it's up to @iMicknl and @Quentame to decide with path they want to go

iMicknl · 2023-09-24T18:23:10Z

Thanks @fetzerch, @Quentame and @emontnemery for working on this! I am back from holidays and will have a look at this issue. My preference would be to add a migration for this. It would be cleaner to have core:DiscreteRSSILevelState as a suffix for sensors, instead of OverkizState.CORE_DISCRETE_RSSI_LEVEL imo.

However, easiest would be to override StrEnum.__str__ for backwards compatibility.

What would be your preference?

iMicknl · 2023-09-30T10:47:34Z

@fetzerch my proposal for now is https://github.com/home-assistant/core/pull/101024/files. Lets see if we can get this merged in. Feel free to test my fix.

I will review your other PR's after this has been fixed, thanks for spotting the issue and thanks for your other contributions!!

fetzerch · 2023-09-30T13:19:50Z

@iMicknl Commented on the other PR. Indeed, let's get your fix in first that's clearly the most important change.

Shall we close this PR then? Or keep it open until yours is merged (for the history/discussion)?

iMicknl · 2023-10-01T15:51:37Z

If you agree that the other way is preferred, lets close this PR.

fetzerch requested a review from iMicknl as a code owner September 6, 2023 19:09

fetzerch mentioned this pull request Sep 6, 2023

ONLY 2023.9.0 Overkiz breaks some unique_id [fixed on 2023.9.1] #99404

Closed

emontnemery added this to the 2023.9.1 milestone Sep 7, 2023

emontnemery requested changes Sep 7, 2023

View reviewed changes

home-assistant bot marked this pull request as draft September 7, 2023 09:27

joostlek added cla-recheck bugfix integration: overkiz Quality Scale: No score labels Sep 7, 2023

home-assistant bot added cla-signed and removed cla-recheck labels Sep 7, 2023

home-assistant bot assigned iMicknl Sep 7, 2023

MartinHjelmare changed the title ~~overkiz: Keep unique ids stable for pyoverkiz >= 1.10.1~~ Keep overkiz unique ids stable for pyoverkiz >= 1.10.1 Sep 7, 2023

Quentame self-assigned this Sep 7, 2023

bramkragten modified the milestones: 2023.9.1, 2023.9.2 Sep 8, 2023

Quentame mentioned this pull request Sep 8, 2023

2023.9.1 #99950

Merged

balloob modified the milestones: 2023.9.2, 2023.9.3 Sep 12, 2023

frenck modified the milestones: 2023.9.3, 2023.9.4 Sep 23, 2023

iMicknl mentioned this pull request Sep 27, 2023

Bump pyOverkiz to 3.11 and migrate unique ids for select entries #101024

Merged

20 tasks

iMicknl removed this from the 2023.9.4 milestone Sep 30, 2023

fetzerch closed this Oct 2, 2023

fetzerch deleted the overkiz-stable-unique_ids branch October 2, 2023 20:29

github-actions bot locked and limited conversation to collaborators Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fetzerch commented Sep 6, 2023

emontnemery Sep 7, 2023

fetzerch Sep 11, 2023

emontnemery Sep 7, 2023

emontnemery Sep 7, 2023 •

edited

Loading

iMicknl Sep 27, 2023 •

edited

Loading

iMicknl Sep 27, 2023

home-assistant bot commented Sep 7, 2023

emontnemery commented Sep 7, 2023 •

edited

Loading

home-assistant bot commented Sep 7, 2023

Quentame commented Sep 8, 2023

Quentame commented Sep 8, 2023

emontnemery commented Sep 11, 2023 •

edited

Loading

fetzerch commented Sep 11, 2023

emontnemery commented Sep 11, 2023

iMicknl commented Sep 24, 2023 •

edited

Loading

iMicknl commented Sep 30, 2023

fetzerch commented Sep 30, 2023

iMicknl commented Oct 1, 2023

	# 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.

	@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)

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

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

Conversation

fetzerch commented Sep 6, 2023

Proposed change

Type of change

Additional information

Checklist

emontnemery Sep 7, 2023

Choose a reason for hiding this comment

fetzerch Sep 11, 2023

Choose a reason for hiding this comment

emontnemery Sep 7, 2023

Choose a reason for hiding this comment

emontnemery Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

iMicknl Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

iMicknl Sep 27, 2023

Choose a reason for hiding this comment

home-assistant bot commented Sep 7, 2023

emontnemery commented Sep 7, 2023 • edited Loading

home-assistant bot commented Sep 7, 2023

Quentame commented Sep 8, 2023

Quentame commented Sep 8, 2023

emontnemery commented Sep 11, 2023 • edited Loading

fetzerch commented Sep 11, 2023

emontnemery commented Sep 11, 2023

iMicknl commented Sep 24, 2023 • edited Loading

iMicknl commented Sep 30, 2023

fetzerch commented Sep 30, 2023

iMicknl commented Oct 1, 2023

emontnemery Sep 7, 2023 •

edited

Loading

iMicknl Sep 27, 2023 •

edited

Loading

emontnemery commented Sep 7, 2023 •

edited

Loading

emontnemery commented Sep 11, 2023 •

edited

Loading

iMicknl commented Sep 24, 2023 •

edited

Loading