Catch URLError in tests when trying to download local attr values (DK…

…ISTDC#407)

* Catch URLError in tests when trying to download local attr values

Closes DKISTDC#381

* Add changelog

* Catch URLError in tests when trying to download local attr values

Closes DKISTDC#381

* Add changelog

* Split out download and save to reduce delete

* Pedantry

* Seems like an easy fix but I think this should be correct

* An incomplete pedantry is the hobgoblin of little tests

---------

Co-authored-by: Stuart Mumford <[email protected]>

changelog/407.bugfix.rst

@@ -0,0 +1 @@
+Catch URLError when trying to download attr values in tests so that the existing file isn't assumed to be corrupted and therefore deleted.

dkist/net/attrs_values.py

@@ -37,6 +37,12 @@
 }
 
 
+class UserCacheMissing(Exception):
+    """
+    An exception for when we have deleted the user cache.
+    """
+
+
 def _get_file_age(path: Path) -> dt.timedelta:
     last_modified = dt.datetime.fromtimestamp(path.stat().st_mtime)
     now = dt.datetime.now()
@@ -65,15 +71,19 @@ def _get_cached_json() -> list[Path, bool]:
     return return_file, update_needed
 
 
-def _fetch_values_to_file(filepath: Path, *, timeout: int = 1):
+def _fetch_values(timeout: int = 1) -> bytes:
+    """
+    Make a request for new values.
+
+    This is a separate function mostly for mocking it in the tests.
+    """
     data = urllib.request.urlopen(
         net_conf.dataset_endpoint + net_conf.dataset_search_values_path, timeout=timeout
     )
-    with open(filepath, "wb") as f:
-        f.write(data.read())
+    return data.read()
 
 
-def attempt_local_update(*, timeout: int = 1, user_file: Path = None, silence_errors: bool = True) -> bool:
+def attempt_local_update(*, timeout: int = 1, user_file: Path = None, silence_net_errors: bool = True) -> bool:
     """
     Attempt to update the local data copy of the values.
 
@@ -86,8 +96,8 @@ def attempt_local_update(*, timeout: int = 1, user_file: Path = None, silence_er
     user_file
         The file to save the updated attrs JSON to. If `None` platformdirs will
         be used to get the user data path.
-    silence_errors
-        If `True` catch all errors in this function.
+    silence_net_errors
+        If `True` catch all errors caused by downloading new values in this function.
 
     Returns
     -------
@@ -97,37 +107,35 @@ def attempt_local_update(*, timeout: int = 1, user_file: Path = None, silence_er
     if user_file is None:
         user_file = platformdirs.user_data_path("dkist") / "api_search_values.json"
     user_file = Path(user_file)
-    user_file.parent.mkdir(exist_ok=True, parents=True)
+    if not user_file.exists():
+        user_file.parent.mkdir(exist_ok=True, parents=True)
 
     log.info(f"Fetching updated search values for the DKIST client to {user_file}")
 
-    success = False
     try:
-        _fetch_values_to_file(user_file, timeout=timeout)
-        success = True
+        data = _fetch_values(timeout)
     except Exception as err:
-        log.error("Failed to download new attrs values.")
+        log.error("Failed to download new dkist attrs values. attr values for dkist may be outdated.")
         log.debug(str(err))
-        # If an error has occurred then remove the local file so it isn't
-        # corrupted or invalid.
-        user_file.unlink(missing_ok=True)
-        if not silence_errors:
+        if not silence_net_errors:
             raise
+        return False
 
-        return success
-
-    # Test that the file we just saved can be parsed as json
     try:
+        # Save the data
+        with open(user_file, "wb") as f:
+            f.write(data)
+
+        # Test that the file we just saved can be parsed as json
         with open(user_file) as f:
             json.load(f)
-    except Exception:
-        log.error("Downloaded file is not valid JSON.")
-        user_file.unlink(missing_ok=True)
-        if not silence_errors:
-            raise
-        success = False
 
-    return success
+        return True
+
+    except Exception as err:
+        log.error("Downloaded file could not be saved or is not valid JSON, removing cached file.")
+        user_file.unlink(missing_ok=True)
+        raise UserCacheMissing from err
 
 
 def get_search_attrs_values(*, allow_update: bool = True, timeout: int = 1) -> dict:
@@ -152,11 +160,15 @@ def get_search_attrs_values(*, allow_update: bool = True, timeout: int = 1) -> d
     """
     local_path, update_needed = _get_cached_json()
     if allow_update and update_needed:
-        attempt_local_update(timeout=timeout)
+        try:
+            attempt_local_update(timeout=timeout)
+        except UserCacheMissing:
+            # if we have deleted the user cache we must use the file shipped with the package
+            local_path = importlib.resources.files(dkist.data) / "api_search_values.json"
 
     if not update_needed:
-        log.debug("No update to attr values needed.")
-        log.debug("Using attr values from %s", local_path)
+        log.debug("No update to dkist attr values needed.")
+        log.debug("Using dkist attr values from %s", local_path)
 
     with open(local_path) as f:
         search_values = json.load(f)

dkist/net/tests/test_attrs_values.py

@@ -5,14 +5,15 @@
 import datetime
 import importlib
 from platform import system
+from urllib.error import URLError
 
 import platformdirs
 import pytest
 
 from sunpy.net import attrs as a
 
 import dkist.data
-from dkist.net.attrs_values import (_fetch_values_to_file, _get_cached_json,
+from dkist.net.attrs_values import (UserCacheMissing, _fetch_values, _get_cached_json,
                                     attempt_local_update, get_search_attrs_values)
 
 PACKAGE_FILE = importlib.resources.files(dkist.data) / "api_search_values.json"
@@ -79,31 +80,25 @@ def test_get_cached_json_local_not_quite_out_of_date(tmp_homedir, values_in_home
 
 
 @pytest.mark.remote_data
-def test_fetch_values_to_file(tmp_path):
-    json_file = tmp_path / "api_search_values.json"
-
-    assert json_file.exists() is False
-    _fetch_values_to_file(json_file)
-    assert json_file.exists() is True
+def test_fetch_values_to_file():
+    data = _fetch_values()
+    assert isinstance(data, bytes)
 
-    # Check we can load the file as json and it looks very roughly like what we
-    # would expect from the API response
-    with open(json_file) as f:
-        data = json.load(f)
-    assert "parameterValues" in data.keys()
-    assert isinstance(data["parameterValues"], list)
+    jdata = json.loads(data)
+    assert "parameterValues" in jdata.keys()
+    assert isinstance(jdata["parameterValues"], list)
 
 
-def _local_fetch_values(user_file, *, timeout):
-    user_file.parent.mkdir(parents=True, exist_ok=True)
-    shutil.copy(PACKAGE_FILE, user_file)
+def _local_fetch_values(timeout):
+    with open(PACKAGE_FILE, "rb") as fobj:
+        return fobj.read()
 
 
 def test_attempt_local_update(mocker, tmp_path, caplog_dkist):
     json_file = tmp_path / "api_search_values.json"
-    mocker.patch("dkist.net.attrs_values._fetch_values_to_file",
+    mocker.patch("dkist.net.attrs_values._fetch_values",
                  new_callable=lambda: _local_fetch_values)
-    success = attempt_local_update(user_file=json_file, silence_errors=False)
+    success = attempt_local_update(user_file=json_file, silence_net_errors=False)
     assert success
 
     assert caplog_dkist.record_tuples == [
@@ -116,39 +111,51 @@ def raise_error(*args, **kwargs):
 
 
 def test_attempt_local_update_error_download(mocker, caplog_dkist, tmp_homedir, user_file):
-    mocker.patch("dkist.net.attrs_values._fetch_values_to_file",
+    mocker.patch("dkist.net.attrs_values._fetch_values",
                  side_effect=raise_error)
-    success = attempt_local_update(silence_errors=True)
+    success = attempt_local_update(silence_net_errors=True)
     assert not success
 
     assert caplog_dkist.record_tuples == [
         ("dkist", logging.INFO, f"Fetching updated search values for the DKIST client to {user_file}"),
-        ("dkist", logging.ERROR, "Failed to download new attrs values."),
+        ("dkist", logging.ERROR, "Failed to download new dkist attrs values. attr values for dkist may be outdated."),
     ]
 
     with pytest.raises(ValueError, match="This is a value error"):
-        success = attempt_local_update(silence_errors=False)
+        success = attempt_local_update(silence_net_errors=False)
 
 
-def _definately_not_json(user_file, *, timeout):
-    with open(user_file, "w") as f:
-        f.write("This is not json")
+def _definitely_not_json(timeout):
+    return b"alskdjalskdjaslkdj!!"
 
 
-def test_attempt_local_update_fail_invalid_download(mocker, tmp_path, caplog_dkist):
+def test_attempt_local_update_fail_invalid_json(mocker, user_file, tmp_path, caplog_dkist):
+    # test that the file is removed after
     json_file = tmp_path / "api_search_values.json"
-    mocker.patch("dkist.net.attrs_values._fetch_values_to_file",
-                 new_callable=lambda: _definately_not_json)
-    success = attempt_local_update(user_file=json_file, silence_errors=True)
-    assert not success
+    mocker.patch("dkist.net.attrs_values._fetch_values",
+                 new_callable=lambda: _definitely_not_json)
+    with pytest.raises(UserCacheMissing):
+        success = attempt_local_update(user_file=json_file)
+
+    # File should have been deleted if the update has got as far as returning this error
+    assert not json_file.exists()
+
+
+def test_get_search_attrs_values_fail_invalid_download(mocker, user_file, values_in_home, tmp_path, caplog_dkist):
+    """
+    Given: An existing cache file
+    When: JSON is invalid
+    Then: File is removed, and attr values are still loaded
+    """
+    mocker.patch("dkist.net.attrs_values._fetch_values",
+                 new_callable=lambda: _definitely_not_json)
+    ten_ago = (datetime.datetime.now() - datetime.timedelta(days=10)).timestamp()
+    os.utime(user_file, (ten_ago, ten_ago))
 
-    assert caplog_dkist.record_tuples == [
-        ("dkist", logging.INFO, f"Fetching updated search values for the DKIST client to {json_file}"),
-        ("dkist", logging.ERROR, "Downloaded file is not valid JSON."),
-    ]
+    attr_values = get_search_attrs_values()
+    assert not user_file.exists()
 
-    with pytest.raises(json.JSONDecodeError):
-        success = attempt_local_update(user_file=json_file, silence_errors=False)
+    assert {a.Instrument, a.dkist.HeaderVersion, a.dkist.WorkflowName}.issubset(attr_values.keys())
 
 
 @pytest.mark.parametrize(("user_file", "update_needed", "allow_update", "should_update"), [
@@ -172,3 +179,22 @@ def test_get_search_attrs_values(mocker, caplog_dkist, values_in_home, user_file
     assert isinstance(attr_values, dict)
     # Test that some known attrs are in the result
     assert {a.Instrument, a.dkist.HeaderVersion, a.dkist.WorkflowName}.issubset(attr_values.keys())
+
+
+def _fetch_values_urlerror(*args):
+    raise URLError("it hates you")
+
+
+def test_failed_download(mocker, caplog_dkist, user_file, values_in_home):
+    mock = mocker.patch("dkist.net.attrs_values._fetch_values",
+                        new_callable=lambda: _fetch_values_urlerror)
+
+    ten_ago = (datetime.datetime.now() - datetime.timedelta(days=10)).timestamp()
+    os.utime(user_file, (ten_ago, ten_ago))
+
+    attr_values = get_search_attrs_values(allow_update=True)
+
+    assert caplog_dkist.record_tuples == [
+        ("dkist", logging.INFO, f"Fetching updated search values for the DKIST client to {user_file}"),
+        ("dkist", logging.ERROR, "Failed to download new dkist attrs values. attr values for dkist may be outdated."),
+    ]