Merge remote-tracking branch 'origin/external-files-etag' into integration

esphome/external_files.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import contextlib
 from datetime import UTC, datetime
 import logging
 from pathlib import Path
@@ -8,7 +9,8 @@ import requests
 
 import esphome.config_validation as cv
 from esphome.const import __version__
-from esphome.core import CORE, TimePeriodSeconds
+from esphome.core import CORE, EsphomeError, TimePeriodSeconds
+from esphome.helpers import write_file
 
 _LOGGER = logging.getLogger(__name__)
 CODEOWNERS = ["@landonr"]
@@ -16,12 +18,38 @@ CODEOWNERS = ["@landonr"]
 NETWORK_TIMEOUT = 30
 
 IF_MODIFIED_SINCE = "If-Modified-Since"
+IF_NONE_MATCH = "If-None-Match"
+ETAG = "ETag"
 CACHE_CONTROL = "Cache-Control"
 CACHE_CONTROL_MAX_AGE = "max-age="
 CONTENT_DISPOSITION = "content-disposition"
 TEMP_DIR = "temp"
 
 
+def _etag_sidecar_path(local_file_path: Path) -> Path:
+    return local_file_path.parent / f".{local_file_path.name}.etag"
+
+
+def _read_etag(local_file_path: Path) -> str | None:
+    try:
+        etag = _etag_sidecar_path(local_file_path).read_text().strip()
+    except OSError:
+        return None
+    return etag or None
+
+
+def _write_etag(local_file_path: Path, etag: str | None) -> None:
+    etag_path = _etag_sidecar_path(local_file_path)
+    if not etag:
+        with contextlib.suppress(FileNotFoundError):
+            etag_path.unlink()
+        return
+    try:
+        write_file(etag_path, etag)
+    except EsphomeError as e:
+        _LOGGER.debug("Could not save ETag for %s: %s", local_file_path, e)
+
+
 def has_remote_file_changed(url: str, local_file_path: Path) -> bool:
     if local_file_path.exists():
         _LOGGER.debug("has_remote_file_changed: File exists at %s", local_file_path)
@@ -35,14 +63,18 @@ def has_remote_file_changed(url: str, local_file_path: Path) -> bool:
                 IF_MODIFIED_SINCE: local_modification_time_str,
                 CACHE_CONTROL: CACHE_CONTROL_MAX_AGE + "3600",
             }
+            etag = _read_etag(local_file_path)
+            if etag:
+                headers[IF_NONE_MATCH] = etag
             response = requests.head(
                 url, headers=headers, timeout=NETWORK_TIMEOUT, allow_redirects=True
             )
 
             _LOGGER.debug(
-                "has_remote_file_changed: File %s, Local modified %s, response code %d",
+                "has_remote_file_changed: File %s, Local modified %s, ETag %s, response code %d",
                 local_file_path,
                 local_modification_time_str,
+                etag or "<none>",
                 response.status_code,
             )
 
@@ -51,6 +83,9 @@ def has_remote_file_changed(url: str, local_file_path: Path) -> bool:
                     "has_remote_file_changed: File not modified since %s",
                     local_modification_time_str,
                 )
+                new_etag = response.headers.get(ETAG)
+                if new_etag and new_etag != etag:
+                    _write_etag(local_file_path, new_etag)
                 return False
             _LOGGER.debug("has_remote_file_changed: File modified")
             return True
@@ -112,7 +147,7 @@ def download_content(url: str, path: Path, timeout: int = NETWORK_TIMEOUT) -> by
             return path.read_bytes()
         raise cv.Invalid(f"Could not download from {url}: {e}") from e
 
-    path.parent.mkdir(parents=True, exist_ok=True)
     data = req.content
-    path.write_bytes(data)
+    write_file(path, data)
+    _write_etag(path, req.headers.get(ETAG))
     return data

tests/unit_tests/test_external_files.py

@@ -98,6 +98,7 @@ def test_has_remote_file_changed_not_modified(
 
     mock_response = MagicMock()
     mock_response.status_code = 304
+    mock_response.headers = {}
     mock_head.return_value = mock_response
 
     url = "https://example.com/file.txt"
@@ -122,6 +123,7 @@ def test_has_remote_file_changed_modified(
 
     mock_response = MagicMock()
     mock_response.status_code = 200
+    mock_response.headers = {}
     mock_head.return_value = mock_response
 
     url = "https://example.com/file.txt"
@@ -166,6 +168,7 @@ def test_has_remote_file_changed_timeout(
 
     mock_response = MagicMock()
     mock_response.status_code = 304
+    mock_response.headers = {}
     mock_head.return_value = mock_response
 
     url = "https://example.com/file.txt"
@@ -175,6 +178,68 @@ def test_has_remote_file_changed_timeout(
     assert call_args[1]["timeout"] == external_files.NETWORK_TIMEOUT
 
 
+@patch("esphome.external_files.requests.head")
+def test_has_remote_file_changed_uses_etag(
+    mock_head: MagicMock, setup_core: Path
+) -> None:
+    """Test has_remote_file_changed sends If-None-Match when ETag is cached."""
+    test_file = setup_core / "cached.txt"
+    test_file.write_text("cached content")
+    external_files._etag_sidecar_path(test_file).write_text('"abc123"')
+
+    mock_response = MagicMock()
+    mock_response.status_code = 304
+    mock_response.headers = {}
+    mock_head.return_value = mock_response
+
+    url = "https://example.com/file.txt"
+    result = external_files.has_remote_file_changed(url, test_file)
+
+    assert result is False
+    headers = mock_head.call_args[1]["headers"]
+    assert headers[external_files.IF_NONE_MATCH] == '"abc123"'
+
+
+@patch("esphome.external_files.requests.head")
+def test_has_remote_file_changed_no_etag_no_if_none_match(
+    mock_head: MagicMock, setup_core: Path
+) -> None:
+    """Test has_remote_file_changed omits If-None-Match when no ETag is cached."""
+    test_file = setup_core / "cached.txt"
+    test_file.write_text("cached content")
+
+    mock_response = MagicMock()
+    mock_response.status_code = 304
+    mock_response.headers = {}
+    mock_head.return_value = mock_response
+
+    url = "https://example.com/file.txt"
+    external_files.has_remote_file_changed(url, test_file)
+
+    headers = mock_head.call_args[1]["headers"]
+    assert external_files.IF_NONE_MATCH not in headers
+
+
+@patch("esphome.external_files.requests.head")
+def test_has_remote_file_changed_refreshes_etag_on_304(
+    mock_head: MagicMock, setup_core: Path
+) -> None:
+    """Test has_remote_file_changed updates the cached ETag when the 304 sends a new one."""
+    test_file = setup_core / "cached.txt"
+    test_file.write_text("cached content")
+    external_files._etag_sidecar_path(test_file).write_text('"old"')
+
+    mock_response = MagicMock()
+    mock_response.status_code = 304
+    mock_response.headers = {external_files.ETAG: '"new"'}
+    mock_head.return_value = mock_response
+
+    url = "https://example.com/file.txt"
+    external_files.has_remote_file_changed(url, test_file)
+
+    assert external_files._etag_sidecar_path(test_file).read_text() == '"new"'
+
+
 def test_compute_local_file_dir_creates_parent_dirs(setup_core: Path) -> None:
     """Test compute_local_file_dir creates parent directories."""
     domain = "level1/level2/level3/level4"
@@ -273,6 +338,7 @@ def test_download_content_skip_external_update_downloads_when_missing(
     mock_has_changed.return_value = True
     mock_response = MagicMock()
     mock_response.content = new_content
+    mock_response.headers = {}
     mock_response.raise_for_status = MagicMock()
     mock_get.return_value = mock_response
 
@@ -282,3 +348,59 @@ def test_download_content_skip_external_update_downloads_when_missing(
 
     assert result == new_content
     assert test_file.read_bytes() == new_content
+
+
+@patch("esphome.external_files.requests.get")
+@patch("esphome.external_files.has_remote_file_changed")
+def test_download_content_saves_etag(
+    mock_has_changed: MagicMock,
+    mock_get: MagicMock,
+    setup_core: Path,
+) -> None:
+    """Test download_content writes the ETag sidecar after a successful download."""
+    test_file = setup_core / "fresh.txt"
+    new_content = b"fresh content"
+
+    mock_has_changed.return_value = True
+    mock_response = MagicMock()
+    mock_response.content = new_content
+    mock_response.headers = {external_files.ETAG: '"deadbeef"'}
+    mock_response.raise_for_status = MagicMock()
+    mock_get.return_value = mock_response
+
+    url = "https://example.com/file.txt"
+    external_files.download_content(url, test_file)
+
+    assert external_files._etag_sidecar_path(test_file).read_text() == '"deadbeef"'
+
+
+@patch("esphome.external_files.requests.get")
+@patch("esphome.external_files.has_remote_file_changed")
+def test_download_content_atomic_write_no_partial_on_failure(
+    mock_has_changed: MagicMock,
+    mock_get: MagicMock,
+    setup_core: Path,
+) -> None:
+    """Test download_content does not corrupt the existing file if the write step fails."""
+    test_file = setup_core / "cached.txt"
+    original_content = b"original content"
+    test_file.write_bytes(original_content)
+
+    mock_has_changed.return_value = True
+    mock_response = MagicMock()
+    # Accessing .content raises, simulating a streaming/decode failure
+    type(mock_response).content = property(
+        lambda self: (_ for _ in ()).throw(OSError("disk full"))
+    )
+    mock_response.raise_for_status = MagicMock()
+    mock_get.return_value = mock_response
+
+    url = "https://example.com/file.txt"
+    with pytest.raises(OSError, match="disk full"):
+        external_files.download_content(url, test_file)
+
+    # Original file is untouched (atomic rename never happened)
+    assert test_file.read_bytes() == original_content
+    # No leftover temp files from tempfile.NamedTemporaryFile
+    leftover_tmps = list(setup_core.glob("tmp*"))
+    assert leftover_tmps == []