Repository: DKISTDC/dkist

Issue:
Fetching local attr values can raise an exception
There is an edge case in the attr values code which can happen (I think) in the following case:

1. Successfully trigger a download of the values to the local data dir.
2. Attempt to refresh the values but have the download fail.
3. Error is raised because the failed download has deleted the pre-existing file.

I think this should be reliably triggered by running:

```bash
$ python -m dkist.net
$ touch -d "2020/01/01T00:00:00" ~/.local/share/dkist/api_search_values.json
$ tox -e py311
```

as the tox refresh will fail due to pytest-remotedata.

Discussion:
codspeed-hq[bot]: ## [CodSpeed Performance Report](https://codspeed.io/DKISTDC/dkist/branches/SolarDrew:attr-fix)

### Merging #407 will **not alter performance**

<sub>Comparing <code>SolarDrew:attr-fix</code> (20bf6b1) with <code>main</code> (02b0c04)</sub>



### Summary

`✅ 3` untouched benchmarks






Patch:
diff --git a/changelog/407.bugfix.rst b/changelog/407.bugfix.rst
new file mode 100644
index 0000000..1d50ddc
--- /dev/null
+++ b/changelog/407.bugfix.rst
@@ -0,0 +1,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.
diff --git a/changelog/417.feature.rst b/changelog/417.feature.rst
new file mode 100644
index 0000000..65cf5c2
--- /dev/null
+++ b/changelog/417.feature.rst
@@ -0,0 +1,1 @@
+Add "status" to the list of know dataset inventory fields.
diff --git a/dkist/net/attrs_values.py b/dkist/net/attrs_values.py
index ccee14c..441941f 100644
--- a/dkist/net/attrs_values.py
+++ b/dkist/net/attrs_values.py
@@ -37,6 +37,12 @@ INVENTORY_ATTR_MAP = {
 }
 
 
+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)
diff --git a/dkist/utils/inventory.py b/dkist/utils/inventory.py
index cc54d73..9f9f77c 100644
--- a/dkist/utils/inventory.py
+++ b/dkist/utils/inventory.py
@@ -72,7 +72,8 @@ INVENTORY_KEY_MAP: dict[str, str] = DefaultMap(None, {
         "headerVersion": "Header Specification Version",
         "headerDocumentationUrl": "Header Documentation URL",
         "infoUrl": "Info URL",
-        "calibrationDocumentationUrl": "Calibration Documentation URL"
+        "calibrationDocumentationUrl": "Calibration Documentation URL",
+        "status": "Status",
 })
 
 


Test patch:
diff --git a/dkist/net/tests/test_attrs_values.py b/dkist/net/tests/test_attrs_values.py
index 34305a7..e902ba6 100644
--- a/dkist/net/tests/test_attrs_values.py
+++ b/dkist/net/tests/test_attrs_values.py
@@ -5,6 +5,7 @@ import logging
 import datetime
 import importlib
 from platform import system
+from urllib.error import URLError
 
 import platformdirs
 import pytest
@@ -12,7 +13,7 @@ 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."),
+    ]