Fix coordinator data mutation in YouTube diagnostics#170183
Conversation
|
Hey there @joostlek, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Fixes a bug in the YouTube integration diagnostics where diagnostics generation could mutate (or previously even break on) the coordinator’s live data, by ensuring diagnostics operate on copies and safely handle latest_video=None.
Changes:
- Avoid mutating
coordinator.databy copying per-channel data and rebuildinglatest_videowithoutdescription. - Add diagnostics tests to verify coordinator data immutability,
descriptionredaction, and correct handling oflatest_video=None.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
homeassistant/components/youtube/diagnostics.py |
Stops in-place mutation of coordinator data and safely redacts latest_video.description without altering live state. |
tests/components/youtube/test_diagnostics.py |
Adds regression tests ensuring diagnostics don’t mutate coordinator data and handle latest_video=None while keeping description out of output. |
jbouwh
left a comment
There was a problem hiding this comment.
Please fill i the PR description.
Split the fixed code and the Config Entry typing improvements
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
No, worries. Thanks for the feedback! I've updated the PR to contain only the diagnostics mutation bug fix. coordinator.py and init.py changes have been removed. The YouTubeConfigEntry typing improvements will be submitted as a separate PR. |
| await setup_integration() | ||
| entry = hass.config_entries.async_entries(DOMAIN)[0] | ||
| coordinator = hass.data[DOMAIN][entry.entry_id][COORDINATOR] | ||
|
|
| await setup_integration() | ||
| entry = hass.config_entries.async_entries(DOMAIN)[0] | ||
| coordinator = hass.data[DOMAIN][entry.entry_id][COORDINATOR] | ||
|
|
Hi. Take 2. I think the Config Entry typing changes in init.py and coordinator.py are actually a prerequisite for this fix — diagnostics.py needs to access the coordinator via entry.runtime_data because the updated async_setup_entry no longer populates hass.data. Would it be acceptable to include both changes in this PR, or would you prefer I submit the typing refactor first and rebase this on top of it? |
|
I'd like the fix (mutation in the test) to be as small as possible, as this gets in to the patch release. Yeah is the first one. The second one is about code quality, and adds the typing. |
Thank you! Given this, I see two options for your consideration: Happy to go whichever route you prefer just let me know! |
|
I just rebased (you need to do a |
|
I don't think the additional tests are needed. Just copy the coordinator data like: async def async_get_config_entry_diagnostics(
hass: HomeAssistant, entry: YouTubeConfigEntry
) -> dict[str, Any]:
"""Return diagnostics for a config entry."""
coordinator = entry.runtime_data
sensor_data: dict[str, Any] = {}
for channel_id, channel_data in dict(coordinator.data).items():
channel_data.get(ATTR_LATEST_VIDEO, {}).pop(ATTR_DESCRIPTION)
sensor_data[channel_id] = channel_data
return sensor_data |
| from homeassistant.components.youtube.const import ( | ||
| ATTR_DESCRIPTION, | ||
| ATTR_LATEST_VIDEO, | ||
| COORDINATOR, | ||
| DOMAIN, | ||
| ) |
There was a problem hiding this comment.
| from homeassistant.components.youtube.const import ( | |
| ATTR_DESCRIPTION, | |
| ATTR_LATEST_VIDEO, | |
| COORDINATOR, | |
| DOMAIN, | |
| ) | |
| from homeassistant.components.youtube.const import DOMAIN |
| ) -> None: | ||
| """Test diagnostics.""" | ||
| await setup_integration() | ||
| entry = hass.config_entries.async_entries(DOMAIN)[0] |
There was a problem hiding this comment.
| entry = hass.config_entries.async_entries(DOMAIN)[0] | |
| entry = hass.config_entries.async_entries(DOMAIN)[0] | |
|
|
||
|
|
||
| async def test_diagnostics_does_not_mutate_coordinator_data( | ||
| hass: HomeAssistant, | ||
| hass_client: ClientSessionGenerator, | ||
| setup_integration: ComponentSetup, | ||
| ) -> None: | ||
| """Test that fetching diagnostics does not mutate the coordinator's data. | ||
|
|
||
| Previously, diagnostics called .pop(ATTR_DESCRIPTION) directly on the | ||
| coordinator's data dict, permanently removing the description from the | ||
| live data after the first diagnostics fetch. | ||
| """ | ||
| await setup_integration() | ||
| entry = hass.config_entries.async_entries(DOMAIN)[0] | ||
| coordinator = hass.data[DOMAIN][entry.entry_id][COORDINATOR] | ||
|
|
There was a problem hiding this comment.
| async def test_diagnostics_does_not_mutate_coordinator_data( | |
| hass: HomeAssistant, | |
| hass_client: ClientSessionGenerator, | |
| setup_integration: ComponentSetup, | |
| ) -> None: | |
| """Test that fetching diagnostics does not mutate the coordinator's data. | |
| Previously, diagnostics called .pop(ATTR_DESCRIPTION) directly on the | |
| coordinator's data dict, permanently removing the description from the | |
| live data after the first diagnostics fetch. | |
| """ | |
| await setup_integration() | |
| entry = hass.config_entries.async_entries(DOMAIN)[0] | |
| coordinator = hass.data[DOMAIN][entry.entry_id][COORDINATOR] |
| descriptions_before = { | ||
| channel_id: channel_data[ATTR_LATEST_VIDEO][ATTR_DESCRIPTION] | ||
| for channel_id, channel_data in coordinator.data.items() | ||
| if channel_data.get(ATTR_LATEST_VIDEO) is not None | ||
| } | ||
|
|
||
| assert descriptions_before, ( | ||
| "Test setup should include at least one channel with a latest video" | ||
| ) | ||
|
|
||
| await get_diagnostics_for_config_entry(hass, hass_client, entry) | ||
|
|
||
| for channel_id, description in descriptions_before.items(): | ||
| assert ( | ||
| coordinator.data[channel_id][ATTR_LATEST_VIDEO][ATTR_DESCRIPTION] | ||
| == description | ||
| ), ( | ||
| f"Coordinator data was mutated for channel {channel_id}: " | ||
| "description was removed by diagnostics fetch" | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
| descriptions_before = { | |
| channel_id: channel_data[ATTR_LATEST_VIDEO][ATTR_DESCRIPTION] | |
| for channel_id, channel_data in coordinator.data.items() | |
| if channel_data.get(ATTR_LATEST_VIDEO) is not None | |
| } | |
| assert descriptions_before, ( | |
| "Test setup should include at least one channel with a latest video" | |
| ) | |
| await get_diagnostics_for_config_entry(hass, hass_client, entry) | |
| for channel_id, description in descriptions_before.items(): | |
| assert ( | |
| coordinator.data[channel_id][ATTR_LATEST_VIDEO][ATTR_DESCRIPTION] | |
| == description | |
| ), ( | |
| f"Coordinator data was mutated for channel {channel_id}: " | |
| "description was removed by diagnostics fetch" | |
| ) |
6a007e6 to
5733314
Compare
| entry.runtime_data = coordinator | ||
| await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) |
Proposed change
The
async_get_config_entry_diagnosticsfunction was mutating thecoordinator's live data on every diagnostics fetch.
The previous code called
.pop(ATTR_DESCRIPTION)directly onchannel_data, which is a reference intocoordinator.data— not acopy. This permanently removed
descriptionfromATTR_LATEST_VIDEOfor the lifetime of the config entry, meaning any code reading that
field after a diagnostics fetch would find it missing.
Additionally, when
ATTR_LATEST_VIDEOisNone, the previous codecalled
.get(ATTR_LATEST_VIDEO, {}).pop(...)on a throwaway emptydict, silently doing nothing rather than handling the
Nonecaseexplicitly.
This fix builds a shallow copy of each channel's data and constructs
a new
ATTR_LATEST_VIDEOdict excludingdescription, leavingcoordinator data untouched.
Type of change
Additional information
correctly excluded)
redaction, and explicit
Nonelatest video handlingChecklist