refactor: dedup naming helpers, close observe-cache merge race
/simplify pass on the issue #27 fix: extract a shared _snake_to_title between entity.py and discovery.py, factor discover()'s two binding loops through one _bind() helper, and close a TOCTOU race the cache merge introduced -- apply() is the sole path StateCache mutations flow through, so the read-then-write is now serialized under one lock instead of two independently-locked calls.
This commit is contained in:
@@ -8,7 +8,7 @@ from homeassistant.helpers.device_registry import DeviceInfo
|
||||
from homeassistant.const import EntityCategory
|
||||
|
||||
from .registry.adapter import _key
|
||||
from .registry.discovery import BoundEntity
|
||||
from .registry.discovery import BoundEntity, _snake_to_title
|
||||
|
||||
from .const import DOMAIN
|
||||
from .coordinator import LocalThingsCoordinator
|
||||
@@ -44,7 +44,7 @@ def _derive_name(state_key: str) -> str:
|
||||
instance number with a space: "door_cooler_open1" → "Door Cooler Open 1".
|
||||
"""
|
||||
name = re.sub(r'(\d+)$', lambda m: f' {m.group()}' if int(m.group()) > 0 else '', state_key)
|
||||
return name.replace('_', ' ').title().strip()
|
||||
return _snake_to_title(name).strip()
|
||||
|
||||
|
||||
class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]):
|
||||
|
||||
@@ -69,6 +69,7 @@ class ObserveManager:
|
||||
self.last_mode_change_wall = time.time()
|
||||
self._settle_until: dict[str, float] = {}
|
||||
self._settle_lock = threading.Lock()
|
||||
self._cache_lock = threading.Lock()
|
||||
self.subscribed_hrefs: set[str] = set()
|
||||
self._notified: set[str] = set()
|
||||
self._last_notify_ts: float | None = None
|
||||
@@ -105,13 +106,22 @@ class ObserveManager:
|
||||
exists_fn/options_field gates on the moment one partial update
|
||||
comes through, even though nothing about the device's actual
|
||||
supported options changed.
|
||||
|
||||
`apply()` is the sole path StateCache mutations flow through in
|
||||
this component (poll, sweep, and OBSERVE notify all funnel here),
|
||||
so `_cache_lock` serializes the read-then-write across those
|
||||
threads (DTLS reader for notifies, executor threads for poll/
|
||||
sweep) -- without it, two concurrent updates for the same href
|
||||
could each read the same prior rep and the second writer would
|
||||
silently lose the first's fields, reintroducing the exact bug
|
||||
this merge fixes.
|
||||
"""
|
||||
if self._is_settling(href):
|
||||
self.log.debug("dropping %s update for %s (settling)", source, href)
|
||||
return False
|
||||
prior = self.cache.get(href)
|
||||
merged = {**prior, **rep} if prior else rep
|
||||
return self.cache.apply_rep(href, merged, source=source)
|
||||
with self._cache_lock:
|
||||
merged = {**(self.cache.get(href) or {}), **rep}
|
||||
return self.cache.apply_rep(href, merged, source=source)
|
||||
|
||||
def on_notification(self, href: str, payload: bytes) -> None:
|
||||
"""Wired as DtlsCoapSession.on_notification. Runs on the DTLS
|
||||
|
||||
@@ -30,6 +30,12 @@ class BoundEntity:
|
||||
instance_name: Optional[str] = None
|
||||
|
||||
|
||||
def _snake_to_title(s: str) -> str:
|
||||
"""'CUBED_ICE'/'cubed_ice' -> 'Cubed Ice'. Shared with entity.py's
|
||||
_derive_name, which applies the same transform to a state key."""
|
||||
return s.replace('_', ' ').title()
|
||||
|
||||
|
||||
def _instance_name(cap: Capability, rep: dict) -> Optional[str]:
|
||||
"""Normalize `cap.name_field`'s raw value ("CUBED_ICE" -> "Cubed Ice")
|
||||
for use as a display-name prefix, or None if the cap doesn't declare
|
||||
@@ -39,7 +45,7 @@ def _instance_name(cap: Capability, rep: dict) -> Optional[str]:
|
||||
raw = rep.get(cap.name_field)
|
||||
if not isinstance(raw, str) or not raw:
|
||||
return None
|
||||
return raw.replace('_', ' ').title()
|
||||
return _snake_to_title(raw)
|
||||
|
||||
|
||||
def instance_suffix(href: str) -> str:
|
||||
@@ -50,6 +56,18 @@ def instance_suffix(href: str) -> str:
|
||||
return ''
|
||||
|
||||
|
||||
def _bind(cap: Capability, href: str, inst: str, inst_name: Optional[str],
|
||||
key_prefix: Optional[str] = None) -> list[BoundEntity]:
|
||||
"""Build one BoundEntity per entity on `cap`, sharing the instance/
|
||||
key-prefix/instance-name computed once by the caller."""
|
||||
return [
|
||||
BoundEntity(href=href, capability=cap, desc=desc, instance=inst,
|
||||
key_override=f'{key_prefix}_{desc.key}' if key_prefix else None,
|
||||
instance_name=inst_name)
|
||||
for desc in cap.entities
|
||||
]
|
||||
|
||||
|
||||
def discover(
|
||||
resources: dict[str, dict],
|
||||
registry: dict[str, list[Capability]],
|
||||
@@ -71,11 +89,7 @@ def discover(
|
||||
if cap.match_fn is not None and not cap.match_fn(rep, resources):
|
||||
continue
|
||||
inst = instance_suffix(href)
|
||||
inst_name = _instance_name(cap, rep)
|
||||
for desc in cap.entities:
|
||||
out.append(BoundEntity(href=href, capability=cap,
|
||||
desc=desc, instance=inst,
|
||||
instance_name=inst_name))
|
||||
out.extend(_bind(cap, href, inst, _instance_name(cap, rep)))
|
||||
matched = True
|
||||
|
||||
if matched:
|
||||
@@ -90,15 +104,10 @@ def discover(
|
||||
if cap.match_fn is not None and not cap.match_fn(rep, resources):
|
||||
continue
|
||||
inst = instance_suffix(href)
|
||||
inst_name = _instance_name(cap, rep)
|
||||
# Auto-derive key prefix from href segments (skip digits and 'vs')
|
||||
src = href[len(cap.href_prefix):] if (cap.strip_prefix_in_key and cap.href_prefix) else href
|
||||
segs = [s for s in src.strip('/').split('/') if s and not s.isdigit() and s != 'vs']
|
||||
for desc in cap.entities:
|
||||
key_override = '_'.join(segs) + '_' + desc.key
|
||||
out.append(BoundEntity(href=href, capability=cap, desc=desc,
|
||||
instance=inst, key_override=key_override,
|
||||
instance_name=inst_name))
|
||||
out.extend(_bind(cap, href, inst, _instance_name(cap, rep), '_'.join(segs)))
|
||||
matched = True
|
||||
break
|
||||
|
||||
|
||||
Reference in New Issue
Block a user