Fix recorder crash for long state string - enforce at core level #9696
Conversation
|
A state should be not bigger as 255 characters. I don't know what should be the best solution. Maybe we trim it before we write it into state table... @balloob ? |
|
Yes, 255 characters is very long for normal sensors etc. There are components, which allow long state text properly - for example Persistent notification, which I am using to simulate this problem. And probably others, like weather alerts, forecasts, etc. |
|
I don't want to increase the column length over 255 characters. Instead we should update persistent notification to store the message in the attributes. |
|
I don't like this fix either. Silently failing is bad, we should probably enforce this at the core level. |
|
Yes, this first commit is not intended as permanent and systematic fix and is not a good way to solve it. Just try to avoid first recorder thread stop by workaround and open a discussion. Exception is not completely silenced - errors are still logged. It is question, how many components are cause of long states - some are mentioned in linked issues |
|
I had also reported this. #9092 |
milanvo
changed the title
|
Hi @pvizeli discovering core module, please confirm this is right place (State object) to check and enforce state length If longer than 255, we should truncate it to 255 and log warning (or error ?) or throw exception ? Thanks |
|
We should throw an exception in the State constructor if a state is too long. However, we should first update the persistent_notification component to no longer set long states. |
|
Sorry for mess in commits - I was confused by merges and reverts 😉 @balloob please advise regarding persistent_notification - should it be redesigned to store message in attributes (will require GUI changes also ?) ? What should be stored in persistent_notification state then ? Thank you |
milanvo
changed the title
|
Hi, make some progress regarding fixes:
|
|
Sorry for the delay in getting back to you. Other PR is great approach. Will close this PR as the other one is the proper fix. |
|
This fix is ok |
|
Can be merged after #9967 has been merged. |
|
Thanks for review. Because this can break components which set "invalid" state, it could be helpful to warn users / developers in release notes to report / fix these components. |
|
That's what the breaking change tag is for 👍 Thanks for the PR! 🐬 |
|
This change will break getting weather alerts from WUnderground and a list of similar things documented in #9366. For the record, I think this is the wrong direction and the column should be made larger instead. |
|
@ahknight It will truncate values larger than 255 chars, so it should not break anything. Let us know if you have tried out this fix. 255 chars is already long. |
|
The weather component does not write alerts as state. If this is about a weather related platform for a different component, they should all be moved to weather component or move value to attribute |
Description:
Updated:
Recorder thread throws exception and stop recording for external DB engines (PostgreSQL, etc.), when state string is longer than 255 characters.
Fix long state of persistent notification: #9967
Breaking change: State value of entities is limited to 255 characters.
Related issue (if applicable): fixes #9366
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass