-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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.
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
Don't blow up if config entries have unhashable unique IDs #109966
Merged
emontnemery
merged 5 commits into
dev
from
config_entries_stringify_unhashable_unique_id
Feb 8, 2024
+94
−5
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
1734ee5
Don't blow up if config entries have unhashable unique IDs
emontnemery ea084f7
Add test
emontnemery 52fece4
Add comment on when we remove the guard
emontnemery ad7801c
Don't stringify hashable non string unique_id
emontnemery cd38db6
Merge branch 'dev' into config_entries_stringify_unhashable_unique_id
emontnemery File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading status checks…
Add comment on when we remove the guard
commit 52fece4572b77ccc1742b289e847b2e5c2a16f7d
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 indexThe 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 itThere 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was responding to bdraco's comment about string representation and JSON serializable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).