Merge pull request #16 from GuvHas/refactor/tdd-single-account-optimization

Refactor/tdd single account optimization

Author: GuvHas

README.md

@@ -33,11 +33,13 @@ sensors for bottle count and cellar value, plus a searchable, sortable dashboard
 The integration is a standard modern custom component: UI config flow, a
 `DataUpdateCoordinator` for polling, and entities grouped under a device per account.
 
-**How it fetches data.** It uses the [`cellartracker`](https://pypi.org/project/cellartracker/)
-library, which calls CellarTracker's `xlquery.asp` export endpoint and requests the **`Inventory`
-table in tab-separated format**. One HTTP request per refresh returns every bottle you own, with
-66 columns each. The blocking call runs in an executor with a 60-second bound, and parsing runs
-in an executor too, so neither touches Home Assistant's event loop.
+**How it fetches data.** One request per refresh to CellarTracker's `xlquery.asp` export
+endpoint for the **`Inventory` table in tab-separated format**, returning every bottle you own
+with 66 columns each. The request uses Home Assistant's shared `aiohttp` session under a
+60-second `asyncio.timeout`, so a hung server is cancelled cleanly rather than parking a
+worker thread. Parsing runs in an executor, so the event loop is never blocked. The
+[`cellartracker`](https://pypi.org/project/cellartracker/) library supplies the endpoint URL
+and error semantics; its own `requests`-based transport sets no timeout and is not used.
 
 **Features**
 
@@ -50,7 +52,8 @@ in an executor too, so neither touches Home Assistant's event loop.
   the integration and is served from it, so there is nothing to copy into `<config>/www`.
 - Reauthentication: if your password changes, Home Assistant prompts you to re-enter it rather
   than silently failing.
-- Multiple CellarTracker accounts, each as its own device.
+- One account per installation, enforced by the config flow, so there is no ambiguity
+  about which cellar an entity or endpoint refers to.
 - Upstream error pages are rejected rather than being recorded as a genuine zero, so an outage
   cannot punch a hole in your cellar-value history.
 
@@ -224,11 +227,10 @@ and history all stay as they were.
 There is no proactive "change my password now" form. If you would rather not wait for the next
 refresh to notice, use **⋮ → Reload** on the integration to trigger one immediately.
 
-### Multiple accounts
+### One account per installation
 
-Add the integration more than once. Each account becomes its own device, named after that
-account. When more than one is configured, the REST endpoints require you to say which one you
-mean (see below); with a single account nothing changes.
+The config flow allows a single CellarTracker account. Adding it a second time aborts rather
+than creating a duplicate. To switch accounts, delete the existing entry and add it again.
 
 ### A note on the refresh interval
 
@@ -257,18 +259,9 @@ It gives you search across wine name, location and bin; sortable columns; bottle
 in your configured currency; links to each wine on CellarTracker; drink-window colouring (green =
 ready, red = too early or past); and light/dark theme following your Home Assistant theme.
 
-**With more than one account configured**, name the one you want:
-
-```yaml
-type: iframe
-url: /cellartracker/cellar.html?entry_id=YOUR_ENTRY_ID
-aspect_ratio: 100%
-title: Alice's Cellar
-```
-
-The entry ID is the last path segment of the URL when you open the integration under
-**Settings → Devices & Services**. If you omit it with several accounts configured, the page
-tells you which IDs exist rather than guessing.
+The card needs no account parameter — one account is supported per installation, so the
+endpoints have nothing to disambiguate. A stale `?entry_id=...` left over from a card configured
+against v0.0.16 is accepted and ignored, so those cards keep working unchanged.
 
 **Upgrading from before v0.0.16?** You once had to copy the page into `<config>/www` yourself.
 That copy still works — `/local/cellar.html` is Home Assistant's own static mount and this change
@@ -471,10 +464,11 @@ standalone browser tab rather than embedded in an iframe card. Use the card desc
 [The dashboard](#the-dashboard). The page is deliberately unauthenticated static content; the data
 behind it is not, so the API calls it makes need your session.
 
-### The dashboard says several accounts are configured
+### Can I add a second CellarTracker account?
 
-Expected with more than one account. Add `?entry_id=...` to the iframe URL. The error message
-lists the available IDs.
+No. One account per installation is enforced: a second attempt aborts with "CellarTracker
+is already configured". Remove the existing entry under **Settings → Devices & Services**
+first if you want to switch accounts.
 
 ### Passing `?token=` in the dashboard URL
 
@@ -541,12 +535,34 @@ Tags are bare version numbers with no `v` prefix, optionally with a single-lette
 already exists is safe: the workflow checks that tag out and validates it, rather than validating
 the branch and publishing the tag.
 
-### Known limitation
+### Why the library's transport is not used
+
+The `cellartracker` library calls `requests.get(url, params)` with no `timeout=`
+([`api.py`](https://github.com/mathroule/cellartracker/blob/master/cellartracker/api.py)), so the
+socket has no deadline. Running that on an executor thread means an application-level timeout can
+stop Home Assistant *waiting*, but cannot interrupt the worker: `concurrent.futures` has no way to
+cancel a thread that is already running, so it stays in `recv()` until the OS gives up. For a
+server that accepts a connection and then never replies, that is the TCP keepalive interval —
+7200 seconds by default — with the account password sitting in the thread's stack frame.
+
+So the integration does its own HTTP with Home Assistant's shared `aiohttp` session, where
+cancellation genuinely cancels and no thread is involved. The library still supplies the endpoint
+URL, the not-logged-in marker, the table and format enums, and the exception types: it owns the
+contract, just not the transport.
+
+**Possible future contribution.** Adding `timeout=` to `cellartracker`'s `api.py` would fix this
+at the root for every consumer — roughly:
+
+```python
+DEFAULT_TIMEOUT = 60
+
+def execute(self, url=BASE_URL, params={}, timeout=DEFAULT_TIMEOUT):
+    ...
+    reponse = requests.get(url, params, timeout=timeout)
+```
 
-The upstream `cellartracker` library calls `requests.get()` without a `timeout`. This integration
-bounds how long Home Assistant waits, so a hung request fails cleanly and retries on schedule, but
-it cannot cancel a worker thread already blocked in the library. A fully robust fix needs
-`timeout=` upstream.
+That is worth submitting upstream if anyone feels like it, but **this integration does not depend
+on it** — it no longer calls that code path at all. Noted here so the reasoning is not lost.
 
 ---
 

custom_components/card.yaml

@@ -13,11 +13,5 @@ title: My Wine Collection
 # deprecated - a long-lived token is full-account and never expires, so a URL
 # carrying one leaks it into history and server logs.
 #
-# With more than one CellarTracker account configured, name the account
-# explicitly - the API will not guess which cellar to show:
-#
-#   url: /cellartracker/cellar.html?entry_id=YOUR_ENTRY_ID
-#
-# The entry id is the last path segment of the integration's URL under
-# Settings / Devices & Services, and is also listed in the error message the
-# dashboard shows when the account is ambiguous.
+# One CellarTracker account is supported per Home Assistant installation,
+# so the card needs no account parameter.

custom_components/cellar_tracker/cellar_data.py

@@ -1,22 +1,30 @@
 import asyncio
+import csv
 import hashlib
+import io
 import logging
 from collections import defaultdict
 from datetime import timedelta
 
-# Imported at module scope: Home Assistant imports integration modules in an
-# executor, so the file I/O happens off the event loop. Importing inside
-# __init__ runs it *on* the loop and trips HA's blocking-call detector.
+# The library still owns the endpoint contract - its URL, the marker that
+# signals a rejected login, and the exception types - but not the transport:
+# its requests.get() sets no timeout. See _fetch_payload.
 #
 # The exception *types* are also the only reliable way to classify failures:
 # the library raises them bare (`raise AuthenticationError`), so `str(err)` is
 # always the empty string and message sniffing can never match.
-from cellartracker import cellartracker
+#
+# Imported at module scope: Home Assistant imports integration modules in an
+# executor, so this file I/O happens off the event loop.
+import aiohttp
+from cellartracker.const import BASE_URL, NOT_LOGGED_REPONSE
+from cellartracker.enum import CellarTrackerFormat, CellarTrackerTable
 from cellartracker.errors import AuthenticationError, CannotConnect
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.const import CONF_PASSWORD, CONF_SCAN_INTERVAL, CONF_USERNAME
 from homeassistant.core import HomeAssistant
 from homeassistant.exceptions import ConfigEntryAuthFailed
+from homeassistant.helpers.aiohttp_client import async_get_clientsession
 from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed
 
 from .const import (
@@ -34,12 +42,14 @@
 # the first such poll to protect the statistics, then believe a repeat.
 TOLERATED_SUSPICIOUS_EMPTY_POLLS = 1
 
-# The library calls requests.get() without a timeout, so a server that accepts
-# the connection and never replies holds the worker until TCP keepalive expires
-# (~2h by default). This bounds what Home Assistant waits for; it cannot cancel
-# the blocked thread, which needs timeout= upstream in cellartracker.
+# Enforced by asyncio.timeout around an aiohttp request, so it genuinely
+# cancels. The library's own requests.get() sets no timeout, which is why the
+# transport is no longer routed through it.
 REQUEST_TIMEOUT = 60
 
+TABLE_INVENTORY = CellarTrackerTable.Inventory.value
+FORMAT_TAB = CellarTrackerFormat.tab.value
+
 # Columns that identify a physical bottle. Volatile columns are deliberately
 # excluded: Valuation moves whenever CellarTracker re-prices a wine, and an id
 # that changed on every re-pricing would be useless to anything keying on it.
@@ -74,6 +84,46 @@ def _row_fingerprint(bottle: dict) -> str:
     return hashlib.sha256(payload.encode("utf-8")).hexdigest()
 
 
+async def async_fetch_inventory_payload(hass, username: str, password: str) -> str:
+    """Fetch the raw inventory export for an account.
+
+    Shared by the coordinator and by the config flow's credential check, so the
+    two cannot drift apart on transport or on what counts as an auth failure.
+
+    Home Assistant's shared aiohttp session replaces the library's
+    ``requests.get()``, which sets no timeout: an ``asyncio.timeout`` around an
+    executor job bounds the wait but cannot interrupt a worker already blocked
+    in ``recv()``. Cancelling an aiohttp request actually cancels it, and no
+    thread is involved.
+
+    Raises:
+        AuthenticationError: CellarTracker rejected the credentials.
+        CannotConnect: the export could not be retrieved.
+    """
+    session = async_get_clientsession(hass)
+    params = {
+        "User": username,
+        "Password": password,
+        "Table": TABLE_INVENTORY,
+        "Format": FORMAT_TAB,
+        "Location": "1",
+    }
+
+    try:
+        async with asyncio.timeout(REQUEST_TIMEOUT):
+            async with session.get(BASE_URL, params=params) as response:
+                response.raise_for_status()
+                payload = await response.text()
+    except aiohttp.ClientError as err:
+        raise CannotConnect from err
+
+    # An auth failure arrives as HTTP 200 with a marker in the body.
+    if NOT_LOGGED_REPONSE in payload:
+        raise AuthenticationError
+
+    return payload
+
+
 class WineCellarData(DataUpdateCoordinator):
     """Fetch and process CellarTracker inventory data."""
 
@@ -101,8 +151,6 @@ def __init__(self, hass: HomeAssistant, entry: ConfigEntry):
             always_update=False,
         )
 
-        self._client = cellartracker.CellarTracker(self._username, self._password)
-
         # Consecutive polls that reported an empty cellar after it held stock.
         self._suspicious_empty_polls = 0
 
@@ -219,13 +267,16 @@ def _process_inventory(self, inventory: list, previous: dict | None = None) -> d
             "bottles": processed_bottles,
         }
 
+    async def _fetch_payload(self) -> str:
+        """Fetch the raw inventory export for this entry's account."""
+        return await async_fetch_inventory_payload(
+            self._hass, self._username, self._password
+        )
+
     async def _async_update_data(self) -> dict:
         """Fetch inventory from CellarTracker."""
         try:
-            async with asyncio.timeout(REQUEST_TIMEOUT):
-                inventory_list = await self._hass.async_add_executor_job(
-                    self._client.get_inventory
-                )
+            payload = await self._fetch_payload()
         except AuthenticationError as err:
             # Surfaces as a reauth flow (see async_step_reauth in config_flow).
             raise ConfigEntryAuthFailed(
@@ -238,9 +289,14 @@ async def _async_update_data(self) -> dict:
             _LOGGER.exception("Unexpected error fetching CellarTracker inventory")
             raise UpdateFailed(f"Unexpected CellarTracker error: {err!r}") from err
 
-        # Parsing a large cellar means hashing every row and copying every dict,
-        # so keep it off the event loop. self.data is the last successful
-        # result, or None on the first poll.
+        # I/O no longer needs a thread, but parsing still does: a large cellar
+        # means splitting 66 columns per row, hashing each one and copying every
+        # dict. self.data is the last successful result, or None on first poll.
         return await self._hass.async_add_executor_job(
-            self._process_inventory, inventory_list, self.data
+            self._parse_and_process, payload, self.data
         )
+
+    def _parse_and_process(self, payload: str, previous: dict | None) -> dict:
+        """Parse the tab-separated export, then summarise it. Runs in an executor."""
+        rows = list(csv.DictReader(io.StringIO(payload), dialect="excel-tab"))
+        return self._process_inventory(rows, previous=previous)

custom_components/cellar_tracker/config_flow.py

@@ -6,12 +6,12 @@
 
 # Classify failures by exception type: the library raises these bare, so
 # `str(err)` is always "" and message sniffing can never match.
-from cellartracker import cellartracker
 from cellartracker.errors import AuthenticationError, CannotConnect
 from homeassistant import config_entries
 from homeassistant.const import CONF_PASSWORD, CONF_SCAN_INTERVAL, CONF_USERNAME
 from homeassistant.core import callback
 
+from .cellar_data import async_fetch_inventory_payload
 from .const import (
     CONF_CURRENCY,
     CURRENCY_OPTIONS,
@@ -38,16 +38,6 @@
 REAUTH_SCHEMA = vol.Schema({vol.Required(CONF_PASSWORD): str})
 
 
-def _validate_credentials(username: str, password: str) -> None:
-    """Authenticate against CellarTracker. Blocking - run in an executor.
-
-    Raises:
-        AuthenticationError: the username/password pair was rejected.
-        CannotConnect: CellarTracker was unreachable.
-    """
-    cellartracker.CellarTracker(username, password).get_inventory()
-
-
 class CellarTrackerConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     """Handle a config flow for CellarTracker."""
 
@@ -56,9 +46,9 @@ class CellarTrackerConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     async def _async_check_credentials(self, username: str, password: str) -> dict:
         """Return a form-errors dict; empty means the credentials are valid."""
         try:
-            await self.hass.async_add_executor_job(
-                _validate_credentials, username, password
-            )
+            # The same non-blocking fetch the coordinator uses, so a hung
+            # server cannot stall setup on a worker thread.
+            await async_fetch_inventory_payload(self.hass, username, password)
         except AuthenticationError:
             _LOGGER.warning("CellarTracker rejected the credentials for %s", username)
             return {"base": "invalid_auth"}
@@ -72,6 +62,13 @@ async def _async_check_credentials(self, username: str, password: str) -> dict:
 
     async def async_step_user(self, user_input=None):
         """Handle the initial user step."""
+        # One CellarTracker account per installation. Checked before anything
+        # else so a duplicate is refused without a round trip to CellarTracker,
+        # and checked via the entry list rather than the unique id alone so
+        # that legacy entries - keyed on the username - also block.
+        if self._async_current_entries():
+            return self.async_abort(reason="single_instance_allowed")
+
         errors = {}
 
         if user_input is not None:
@@ -83,7 +80,9 @@ async def async_step_user(self, user_input=None):
                 user_input[CONF_USERNAME], user_input[CONF_PASSWORD]
             )
             if not errors:
-                await self.async_set_unique_id(user_input[CONF_USERNAME].lower())
+                # The domain, not the username: a second entry is a duplicate
+                # whichever account it names.
+                await self.async_set_unique_id(DOMAIN)
                 self._abort_if_unique_id_configured()
                 return self.async_create_entry(
                     title=user_input[CONF_USERNAME], data=user_input

custom_components/cellar_tracker/manifest.json

@@ -1,14 +1,19 @@
 {
   "domain": "cellar_tracker",
   "name": "CellarTracker",
-  "codeowners": ["@GuvHas"],
+  "codeowners": [
+    "@GuvHas"
+  ],
   "config_flow": true,
-  "dependencies": ["http"],
+  "dependencies": [
+    "http"
+  ],
   "documentation": "https://github.com/GuvHas/cellartracker",
   "integration_type": "service",
   "iot_class": "cloud_polling",
   "issue_tracker": "https://github.com/GuvHas/cellartracker/issues",
-  "loggers": ["cellartracker"],
-  "requirements": ["cellartracker==1.1.1"],
+  "requirements": [
+    "cellartracker==1.1.1"
+  ],
   "version": "0.0.16"
 }

custom_components/cellar_tracker/strings.json

@@ -26,7 +26,8 @@
     },
     "abort": {
       "already_configured": "This CellarTracker account is already configured.",
-      "reauth_successful": "CellarTracker was re-authenticated successfully."
+      "reauth_successful": "CellarTracker was re-authenticated successfully.",
+      "single_instance_allowed": "CellarTracker is already configured. Only one account is supported per Home Assistant installation."
     }
   },
   "options": {

custom_components/cellar_tracker/translations/en.json

@@ -26,7 +26,8 @@
     },
     "abort": {
       "already_configured": "This CellarTracker account is already configured.",
-      "reauth_successful": "CellarTracker was re-authenticated successfully."
+      "reauth_successful": "CellarTracker was re-authenticated successfully.",
+      "single_instance_allowed": "CellarTracker is already configured. Only one account is supported per Home Assistant installation."
     }
   },
   "options": {