Don't blow up if config entries have unhashable unique IDs #109966
Conversation
emontnemery
force-pushed
the
config_entries_stringify_unhashable_unique_id
branch
from
5a50ee4
to
1734ee5
Compare
| # Guard against integrations using unhashable unique_id | ||
| # In HA Core 2024.9, we should remove the guard and instead fail | ||
| if not isinstance(entry.unique_id, Hashable): | ||
| unique_id_hash = str(entry.unique_id) # type: ignore[unreachable] |
There was a problem hiding this comment.
I think we should not load the config entry if the data is incorrect. I don't think we should try to be smart about it.
There was a problem hiding this comment.
I don't agree, what's the point of that?
In my opinion, integration authors should be given some time to update the invalid unique id. It's not our users' fault that we do not validate the data and then suddenly made assumptions about the unvalidated data.
There was a problem hiding this comment.
By making it a string we are relying on the custom component author who created it with a non string to be able to clean it up later.
I guess we are stuck with an assumption that they aren't actually using the unique id to store a list of data that they need to set up the integration. I'm not sure if it's a reasonable position to accommodate that use case as it's pretty far away from how the API is expected to be used.. This was wrong, the change is only to the index
The safest course would be to revert the original PR and give custom component authors a few months to fix their integrations but unfortunately there is already other work built on top of it There is probably too much risk to do that.
For most cases it probably works fine to stringify it and load it. It's the most user friendly option at least. Given how the frequency of issue reports this may be the way to go.
There was a problem hiding this comment.
The only semi-realistic risk I can come up with that seems possible is they have stored some object in the unique id that has a string representation that isn't unique but is somehow JSON serializable. That would be some type of impressive abuse of the system so it seems like a very contrived case and acceptable risk.
There was a problem hiding this comment.
As example:
#109908 (comment)
Also, this is also still an issue for entity unique_ids (I found that out a few betas ago when I accidentally broke tellduslive)
There was a problem hiding this comment.
@joostlek I don't understand your comment, your example does not point to a case where there's a risk of duplicates
There was a problem hiding this comment.
I was responding to bdraco's comment about string representation and JSON serializable
There was a problem hiding this comment.
I think we should not load the config entry if the data is incorrect. I don't think we should try to be smart about it.
We discussed this in a meeting and there were more of us in favor of allowing the config entries to load (as the PR does) than to not load the config entry (as Martin suggests).
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Testing this by manually mutating some to lists seems to work as expected |
|
Verified I can
|
|
I'm kinda surprised list as unique ids didn't blow up in other ways before from testing |
|
Verified all cases where the dict is accessed have the workaround |
emontnemery
dismissed
MartinHjelmare’s stale review
We discussed this PR in a meeting and there were more of us in favor of allowing the config entries to load (as the PR does) than to not load the config entry (as Martin suggests).
* Don't blow up if config entries have unhashable unique IDs * Add test * Add comment on when we remove the guard * Don't stringify hashable non string unique_id
Proposed change
Don't blow up if config entries have unhashable unique IDs, instead use a string representation of the unique_id as hash
This is a more forgiving approach than #109959 and #109962
We're guaranteed that:
Hence, I don't think it's possible that there are collisions in the stringified representation
In a follow-up, we should prevent creating new config entries with non-string unique IDs.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: