basnijholt / adaptive-lighting

Adaptive Lighting custom component for Home Assistant
https://basnijholt.github.io/adaptive-lighting/
Apache License 2.0
1.72k stars 128 forks source link

ULID must be 26 characters (HA 2023.4.) #493

Closed thatslolo closed 1 year ago

thatslolo commented 1 year ago

Version information:

1.7.0 core-2023.4.0b2

Description:

Every change of the lights is followed by the following error of recorder:

2023-03-31 12:33:19.384 ERROR (Recorder) [homeassistant.components.recorder.core] Error while processing event EventTask(event=<Event call_service[L]: domain=light, service=turn_on, service_data=entity_id=light.lightstrip_regal_nanoleaf_light_strip, brightness=255, color_temp_kelvin=5460>): ULID must be 26 characters Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/components/recorder/core.py", line 825, in _guarded_process_one_task_or_recover self._process_one_task_or_recover(task) File "/usr/src/homeassistant/homeassistant/components/recorder/core.py", line 836, in _process_one_task_or_recover return task.run(self) File "/usr/src/homeassistant/homeassistant/components/recorder/tasks.py", line 281, in run instance._process_one_event(self.event) File "/usr/src/homeassistant/homeassistant/components/recorder/core.py", line 944, in _process_one_event self._process_non_state_changed_event_into_session(event) File "/usr/src/homeassistant/homeassistant/components/recorder/core.py", line 953, in _process_non_state_changed_event_into_session dbevent = Events.from_event(event) File "/usr/src/homeassistant/homeassistant/components/recorder/db_schema.py", line 274, in from_event context_id_bin=ulid_to_bytes_or_none(event.context.id), File "/usr/src/homeassistant/homeassistant/components/recorder/models/context.py", line 15, in ulid_to_bytes_or_none return ulid_to_bytes(ulid) File "src/ulid_transform/_ulid_impl.pyx", line 22, in ulid_transform._ulid_impl._ulid_to_bytes ValueError: ULID must be 26 characters

Issue persists since 2023.4.0b0.

thatslolo commented 1 year ago

home-assistant/core#90574

th3w1zard1 commented 1 year ago

It looks like home assistant lowered the max length of a context id from 36 to 26 for some reason in core-2023.4.0b0

That's rather annoying.

If you go to line 200 or so in /config/custom_components/adaptive-lighting/switch.py you'll see the line:

context_id = f"{_DOMAIN_SHORT}:{name_hash}:{which}:{index_packed}"[:36] just change the 36 to a 26 and restart home assistant core.

If you're uncomfortable changing the code yourself, I could upload the hotfix here if you like.

Most likely nothing will be released into Adaptive Lighting until core-2023.4.0 is released and all the breaking changes are posted.

th3w1zard1 commented 1 year ago

core-2023.4.0 has been released today, but I don't see the context length change posted for the breaking changes.

bdraco commented 1 year ago

The context id needs to be a ULID. This won't be labeled as breaking change as HA isn't designed to accept arbitrary data in the Context id field and it won't be supported. The only thing that should be generating the id is the Context object or HA internals.

In practice you can base32 encode any string https://www.crockford.com/base32.html and pad it to get 26 characters but there are no promises it won't break in the future.

You might be able to subclass Context and add a new field instead of abusing the id field. That wouldn't be supported either but its probably less likely to break in the future if it works

th3w1zard1 commented 1 year ago

@bdraco Thanks for the very informative answers. I have a few questions.

bdraco commented 1 year ago

@bdraco Thanks for the very informative answers. I have a few questions.

  • Why was the length shortened? It seems like such an arbitrary change and the PRs pulled into 4.0-beta weren't very clear either. Why is there a limit at all? Performance reasons?

We now store them as 16 bytes in the database as it was taking up 20-30% of the states table.

  • What's the official supported way of sending data around to functions/listeners that a component isn't calling directly? Certainly there's other integrations that need to check what function fired a certain event? (e.g. a state_changed listener may need to know the origin of a state change)

You should listen for all events. https://github.com/home-assistant/core/blob/2b46734bd3470a2c12644709276730601721676e/homeassistant/components/recorder/core.py#L293

But there is no official way to tell where its coming from. Logbook tries to work it out but thats the only thing that does

th3w1zard1 commented 1 year ago

That seems fine but the component's only use of context ids is for use within the integration only. Ex: component calls light.turn_on and we need our listener to determine if that was called by the integration or not. The listener's currently setup to just check if the context id starts with adapt_lgt to determine that.

th3w1zard1 commented 1 year ago

I'm not too familiar with the event bus so I'm doing research to figure out what needs to be done to future-proof this

I could fire an event before the light.turn_on but then I'm dealing with multiple threads transferring over data instead of direct data being sent to the listener.

basnijholt commented 1 year ago

550 should fix this issue for now. However, since there is no official way to tell where the call came from, we make it only partially conform to the ULID standard now (i.e., we keep the : separators).

bdraco commented 1 year ago

https://github.com/basnijholt/adaptive-lighting/pull/550 is probably fine.

We aren't likely to change the context id format again anytime soon but there is always a chance.

basnijholt commented 1 year ago

Closed as not planned, but in reality, this problem shouldn't pop up anymore.

Our context ids are now 26 chars.

Update to 1.10.1 to get the fix :)

bdraco commented 1 year ago

As long as ulid_transform.ulid_to_bytes gives you back bytes everything should work

>>> ulid_transform.ulid_to_bytes('0001HZX0NW00GW0X476W5TVBFE')
b'\x00\x00c\xfe\x82\xbc\x00!\xc0t\x877\x0b\xad\xad\xee'
bdraco commented 1 year ago

Also if you want to send a PR to https://github.com/bdraco/ulid-transform/blob/main/tests/test_init.py to update the tests to ensure we continue to allow something like this:

>>> ulid_transform.ulid_to_bytes('not_uppercase_b32_data_:::')
b'\xa0\xff\xffk;\x0cV_\xf5\x8c\xffj\xb4\xff\xff\xff'

It might save you some change flux in the future if someone decides to add b32 upper validation to the library

cbhiii commented 1 year ago

Seems to be working for me. Thank you for the quick fix!!!

domoritz commented 1 year ago

I'm glad I found the fix. What's the easiest way to update the component without access to the UI? Is it to patch https://github.com/basnijholt/adaptive-lighting/issues/493#issuecomment-1492798426?

Screenshot 2023-04-07 at 17 21 35
basnijholt commented 1 year ago

Logging in with ssh or using the samba drive.

domoritz commented 1 year ago

I don't have the ssh or samba add-on but I could log into the running image with docker exec -it homeassistant /bin/bash and then edit the file using vi.