Fix ViCare setup by passing the accessor to fetch_all_features - #180315
Conversation
home-assistant
Bot
added
bugfix
cla-signed
has-tests
integration: vicare
small-pr
|
Hey there @CFenner, 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 ViCare setup with PyViCare 2.61.0 by supplying the required device accessor.
Changes:
- Passes the accessor when fetching device features.
- Makes the test mock enforce the real method signature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
homeassistant/components/vicare/coordinator.py |
Passes the device accessor during refresh. |
tests/components/vicare/conftest.py |
Updates the mock signature to catch missing accessors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
TheJulianJES
left a comment
TheJulianJES
left a comment
There was a problem hiding this comment.
Thanks! Looks like #173776 is in b0, so tagged this for b1.
TheJulianJES
left a comment
TheJulianJES
left a comment
There was a problem hiding this comment.
Actually, I’ll have a bot review land in a few minutes. Please check if it’s accurate or not, as soon as it’s there. It seems like it might still not work with just this bugfix.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Thanks for turning this around so fast. The one-line change is correct: I checked against the installed PyViCare 2.61.0 that fetch_all_features(self, accessor) is required-positional on all four service classes, and that Device.accessor is the right object to pass.
But the fix is incomplete — with only this change ViCare still does not set up, because coordinator.py:49 has the same bug from the same commit. That line is outside the diff so I couldn't anchor a comment on it; details below. Since this is milestoned for 2026.9.0, both need to land in the same beta, otherwise the integration is dead on arrival either way.
1. [blocker] coordinator.py:49 — device.service.accessor.id raises AttributeError
name=f"{DOMAIN}_{device.service.accessor.id}",accessor lives on PyViCareDevice and PyViCareDeviceConfig, never on a service. ViCareService.__init__ sets only oauth_manager and roles; the cached variants add _cache*. Confirmed for all four service classes:
ViCareService hasattr accessor: False | vars: ['oauth_manager', 'roles']
ViCareCachedService hasattr accessor: False | vars: ['_cache', '_cacheDuration', '_cacheLock', '_cacheTime', 'oauth_manager', 'roles']
ViCareServiceViaGateway hasattr accessor: False | vars: ['oauth_manager', 'roles']
ViCareCachedServiceViaGateway hasattr accessor: False | vars: ['_cache', '_cacheDuration', '_cacheLock', '_cacheTime', 'oauth_manager', 'roles']
Building the objects exactly the way _setup_vicare_api does (PyViCare() -> setCacheDuration(60) -> initWithExternalOAuth(...) -> asAutoDetectDevice()) and then constructing the real coordinator:
File "homeassistant/components/vicare/coordinator.py", line 49, in __init__
name=f"{DOMAIN}_{device.service.accessor.id}",
^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'ViCareCachedService' object has no attribute 'accessor'
__init__.py:176 calls ViCareCoordinator(hass, entry, device.api, device_count) unconditionally in async_setup_entry, and line 49 is evaluated as an argument to super().__init__, so it raises before anything else — an uncaught AttributeError, not ConfigEntryNotReady. Reproduced for all 16 asXxx() device classes.
git blame puts the line in the same commit as the fetch_all_features() bug, so the PR description's framing holds — it is just half the regression.
Suggested fix, same substitution you already applied on line 63:
name=f"{DOMAIN}_{device.accessor.id}",One caveat: the accessor is built as ViCareDeviceAccessor(installation.id, gateway.serial, device.id), so .id is the installation id, shared by every device on the installation — two devices then produce the identical coordinator name (vicare_1234 twice, verified). The unique form would be:
name=f"{DOMAIN}_{device.accessor.serial}_{device.accessor.device_id}",2. [must-address] coordinator.py:62-63 — the fetch does not warm the cache, and doubles the API budget
The class docstring says the refresh pulls the payload "into PyViCare's internal cache so entity value_getter lambdas read fresh data on each tick". On the service class HA actually builds, it doesn't.
HA never enables viaGateway (zero references outside the library) and always passes cacheDuration >= 60, so __buildService returns ViCareCachedService. Its MRO is ViCareCachedService -> ViCareCachedServiceBase -> ViCareService, and neither subclass overrides fetch_all_features — it resolves to ViCareService.fetch_all_features, a bare return self.oauth_manager.get(url) that never writes self._cache. ViCareCachedServiceViaGateway does override it to route through _get_or_update_cache; the non-gateway class is the asymmetric one.
Measured with a counting oauth manager, running the literal body of _refresh():
| service | _cache populated after fetch_all_features |
HTTP GETs after first entity read |
|---|---|---|
ViCareCachedService (what HA gets) |
False | 2 |
ViCareCachedServiceViaGateway |
True | 1 |
So it is 2 requests per device per interval where the design intends 1. That matters here specifically because __init__.py:241 deliberately scales the cache duration by device count "to fit rate limit to number of devices" — doubling the calls puts the integration roughly 2x over the budget that comment is sizing.
Two knock-on effects worth weighing:
- Before the coordinator migration,
ViCareSensorexposed a syncupdate(), which HA runs in an executor. It is now anative_valueproperty, read in the event loop viaCoordinatorEntity._handle_coordinator_update->async_write_ha_state()(no override inentity.py). With the cache left empty by_refresh, that property performs the second, blockingrequestsGET inside the event loop — which is whatblock_async_ioguards viaHTTPConnection.putrequest(strict=True, strict_core=True). Moving that I/O into the executor was the point of the migration; as written it isn't achieved. _get_or_update_cache's stale-cache fallback (added for issue #67052) can never fire, since_cacheis alwaysNoneat read time.
To be fair to the current code: entities are not left stale, and the discarded fetch still surfaces auth/comm errors correctly, so this is wasted work plus doubled budget rather than a data-correctness bug.
There is no clean HA-side-only fix — the only warming entry point, _get_or_update_cache, is private. The tidy fix is upstream in PyViCare, hoisting the override into ViCareCachedServiceBase:
def fetch_all_features(self, accessor: ViCareDeviceAccessor) -> Any:
return self._get_or_update_cache(accessor)which also makes the gateway subclass's copy redundant. Entirely reasonable to split that out rather than block this hotfix — but then the docstring's cache claim should be corrected and the doubled rate-limit usage noted.
3. [suggestion] the test mock still diverges from the real API in the same way
conftest.py:105 sets self.accessor = ViCareDeviceAccessor(...) on MockViCareService — an attribute no real service has. This is exactly the mock/reality divergence being fixed four lines up, and it is precisely what hid the line-49 bug.
Verified: stripping accessor from the service objects after MockPyViCare.__init__ (via a throwaway pytest plugin, no tracked file touched) turns the green suite into 25 failures whose traceback is the production AttributeError.
Since .accessor is only needed to build PyViCareDeviceConfig at lines 52 and 73, giving MockPyViCare its own ViCareDeviceAccessor and dropping the attribute from MockViCareService would close the gap.
Separately, self.clear_cache = Mock() is a no-op and getProperty reads the fixture directly without consulting a cache, so cache-warming behaviour and call counts are invisible to tests — which is why pytest tests/components/vicare reports 67 passed both with and without the real semantics.
For transparency: this is an automated review. Every claim above was reproduced against the pinned PyViCare 2.61.0 rather than inferred, and the two findings were independently confirmed by a second-opinion model pass; the repro steps are included so you can check them directly. Happy to be told I've misread something.
| @@ -60,7 +60,7 @@ def _refresh(self) -> None: | |||
| """Force a fresh fetch from the Viessmann API.""" | |||
| try: | |||
| self._device.service.clear_cache() | |||
There was a problem hiding this comment.
Not introduced by this PR, but this pair does not do what the class docstring claims ("into PyViCare's internal cache so entity value_getter lambdas read fresh data on each tick") on the code path HA actually uses.
HA never enables viaGateway and always passes cacheDuration >= 60, so the service is a ViCareCachedService. Its MRO is ViCareCachedService -> ViCareCachedServiceBase -> ViCareService, and neither subclass overrides fetch_all_features — it resolves to ViCareService.fetch_all_features, which is just return self.oauth_manager.get(url) and never writes self._cache. ViCareCachedServiceViaGateway does override it to go through _get_or_update_cache; the non-gateway class is the asymmetric one.
Measured by running this exact body against real objects with a request-counting oauth manager:
| service | _cache populated afterwards |
GETs after the first entity read |
|---|---|---|
ViCareCachedService (what HA gets) |
False | 2 |
ViCareCachedServiceViaGateway |
True | 1 |
So the cache is cleared, the fetched payload is discarded, and the next entity read fetches again: 2 requests per device per interval where the design intends 1. That directly undercuts the __init__.py:241 logic that scales cache duration by device count "to fit rate limit to number of devices".
Also worth noting: native_value is now a property read in the event loop (via CoordinatorEntity._handle_coordinator_update -> async_write_ha_state()), where the old code used a sync update() that HA ran in an executor. With the cache left empty here, that second blocking requests GET happens in the event loop, which block_async_io guards against via HTTPConnection.putrequest (strict_core=True).
The clean fix looks like it belongs upstream in PyViCare — hoisting the override into ViCareCachedServiceBase:
def fetch_all_features(self, accessor: ViCareDeviceAccessor) -> Any:
return self._get_or_update_cache(accessor)Fine to split that into a follow-up rather than hold up this hotfix, but the docstring shouldn't keep claiming the cache is warmed meanwhile.
|
Both findings check out, verified on a live installation rather than on paper: I built The blocker is real. Setup dies before the first refresh:
The library and #173776 had got out of sync: PyViCare 2.61.0 moved the accessor out of the service, and my own testing had already moved to the PR stacked on top of this one (#176163), which passes the accessor explicitly. So the merged shape never actually ran anywhere. The test mock hid it as well, it carried an The second finding is also real and also lands in 2026.9. 49 of those per refresh here, one per entity. Two ways out, and it is your call which one you want in b1: #180344, which is self-contained and needs nothing else, or the proper fix in the library (openviess/PyViCare#813), which requires a release. I am trying to reach the maintainer, but that could be tight. |
There was a problem hiding this comment.
Both findings are addressed. I verified the fix itself rather than the commit message, against the pinned PyViCare 2.61.0.
coordinator.py:49 now resolves on the real library, and the name is unique per device — two devices on one gateway give vicare_GW-SERIAL-A_0 / _1, where the old expression raised AttributeError and where a plain .id would have collided (it is the installation id, identical for every device on the account). Also checked the format is not breaking: DataUpdateCoordinator.name is only used for log messages and background-task names, never a registry key or unique_id, and no vicare code reads it.
The mock fix is the part I am happiest about, because it is now load-bearing rather than cosmetic. Reintroducing device.service.accessor.id on this head gives 25 failed, 42 passed; before, the same mutation passed 67/67. The regression guard actually bites.
The remaining cache issue is tracked in #180344, so I have left that thread open here and put the follow-up findings there instead. Worth noting for sequencing: #180344 is stacked on this PR (it carries c9d7c77 and e380550), so merging this first and #180344 straight after is the clean order — and #180344 alone would fix everything, without needing a PyViCare release.
For context on sequencing: 2026.9.0b0 already ships this regression — it contains #173776 and none of these fixes, so ViCare fails to set up entirely on the current beta. That makes this worth landing for b1 on its own merits, independently of how the cache issue is resolved. The cleaner long-term fix for that one is openviess/PyViCare#813 in the library, with #180344 as the Home Assistant-side option that needs no release.
TheJulianJES
left a comment
TheJulianJES
left a comment
There was a problem hiding this comment.
Thanks! Other bug in beta will be followed-up with the bump in #180395
Proposed change
fetch_all_features()requires an accessor in PyViCare 2.61.0, so every coordinator refresh raisedTypeErrorand setup failed. Pass the device's accessor.The test mock accepted any signature, which is why this was not caught; it now mirrors the real one.
Nothing released is affected, the bug only exists on
devsince today, but if it ships as is ViCare will not set up at all in 2026.9. If the 2026.9 beta is already cut, this needs to go into it.@joostlek this is a bug in #173776, which you merged earlier today. It slipped past me because my local instance runs the follow-up branch (#176163) on top, and that one already passes the accessor. Sorry for the extra round.
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.To help with the load of incoming pull requests: