bug: Support Profile update partially fails

.pre-commit-config.yaml

@@ -1,7 +1,7 @@
 repos:
   # Using this mirror lets us use mypyc-compiled black, which is about 2x faster
   - repo: https://github.com/psf/black-pre-commit-mirror
-    rev: 25.1.0
+    rev: 26.1.0
     hooks:
       - id: black
         # It is recommended to specify the latest version of Python

python/nwsc_proxy/ncp_web_service.py

@@ -66,7 +66,7 @@ def handler(self):
         if request.method == "DELETE":
             return self._handle_delete()
 
-        if request.method == "PATCH":
+        if request.method == "PUT":
             return self._handle_update()
 
         # otherwise, must be 'GET' operation
@@ -133,7 +133,7 @@ def _handle_create(self) -> Response:
 
     def _handle_update(self) -> Response:
         if not request.data:
-            return jsonify({"message": "PATCH requires request body"}), 400
+            return jsonify({"message": "PUT requires request body"}), 400
 
         request_body: dict = request.json
         profile_id = request.args.get("id", request.args.get("uuid"))
@@ -168,7 +168,7 @@ def __init__(self, base_dir: str):
             "/all-events",
             "events",
             view_func=events_route.handler,
-            methods=["GET", "POST", "PATCH", "DELETE"],
+            methods=["GET", "POST", "PUT", "DELETE"],
         )
 
     def run(self, **kwargs):

python/nwsc_proxy/src/profile_store.py

@@ -12,15 +12,13 @@
 import os
 import json
 import logging
-from dataclasses import dataclass
+
 from datetime import datetime, UTC
 from glob import glob
 from math import inf
 
 from dateutil.parser import parse as dt_parse
 
-from src.utils import deep_update
-
 # constants controlling the subdirectory where new vs. existing Profiles are saved
 NEW_SUBDIR = "new"
 EXISTING_SUBDIR = "existing"
@@ -29,7 +27,6 @@
 logger = logging.getLogger(__name__)
 
 
-@dataclass
 class CachedProfile:
     """Data class to hold Support Profile's data and metadata ("new" vs "existing" status),
     as well as some derived properties extracted from the `data` JSON (e.g.
@@ -41,13 +38,14 @@ class CachedProfile:
         is_new (bool): track if Support Profile has ever been processed. Ought to start as True
     """
 
-    # pylint: disable=invalid-name
-    data: dict
-    is_new: bool
+    def __init__(self, data: dict, is_new: bool):
+        self.data = data
+        self.is_new = is_new
 
     @property
     def id(self) -> str:
         """The Support Profile UUID"""
+        # pylint: disable=invalid-name
         return self.data.get("id")
 
     @property
@@ -116,7 +114,7 @@ def __init__(self, base_dir: str):
         logger.info("Scanning base directory for raw NWS Connect API response files: %s", base_dir)
         for response_filename in glob("*.json", root_dir=base_dir):
             response_filepath = os.path.join(base_dir, response_filename)
-            logger.warning("Loading profiles from raw API response file: %s", response_filepath)
+            logger.info("Loading profiles from raw API response file: %s", response_filepath)
 
             with open(response_filepath, "r", encoding="utf-8") as infile:
                 data: dict = json.load(infile)
@@ -131,16 +129,16 @@ def __init__(self, base_dir: str):
                         json.dump(profile, outfile)
 
         # populate cache of JSON data of all Support Profiles, marked as new vs. existing
-        existing_profiles = [
-            CachedProfile(profile, is_new=False)
+        existing_profiles = {
+            profile["id"]: CachedProfile(profile, is_new=False)
             for profile in self._load_profiles_from_filesystem(self._existing_dir)
-        ]
-        new_profiles = [
-            CachedProfile(profile, is_new=True)
+        }
+        new_profiles = {
+            profile["id"]: CachedProfile(profile, is_new=True)
             for profile in self._load_profiles_from_filesystem(self._new_dir)
-        ]
+        }
 
-        self.profile_cache = existing_profiles + new_profiles
+        self.profile_cache: dict[str, CachedProfile] = {**existing_profiles, **new_profiles}
 
     def get_all(self, data_source="ANY", is_new=False, include_inactive=False) -> list[dict]:
         """Get all Support Profile JSONs persisted in this API, filtering by status='new'
@@ -154,7 +152,7 @@ def get_all(self, data_source="ANY", is_new=False, include_inactive=False) -> li
         """
         profiles_by_status = [
             cached_profile
-            for cached_profile in self.profile_cache
+            for cached_profile in self.profile_cache.values()
             if (
                 # is new, if client requested new profiles, or is existing
                 cached_profile.is_new == is_new
@@ -186,31 +184,15 @@ def save(self, profile: dict, is_new=True) -> str | None:
         logger.debug("Now saving new profile: %s", profile)
 
         # if profile ID is already in the cache, reject this save
-        existing_profile = next(
-            (
-                (
-                    cached_obj
-                    for cached_obj in self.profile_cache
-                    if cached_obj.id == profile.get("id")
-                )
-            ),
-            None,
-        )
-        if existing_profile:
+        if existing_profile := self.profile_cache.get(profile.get("id")):
             logger.warning("Cannot save profile; already exists %s", existing_profile.id)
             return None
 
         cached_profile = CachedProfile(profile, is_new=is_new)
-
-        # save Profile JSON to filesystem
-        file_dir = self._new_dir if is_new else self._existing_dir
-        filepath = os.path.join(file_dir, f"{cached_profile.id}.json")
-        logger.debug("Now saving profile to path: %s", filepath)
-        with open(filepath, "w", encoding="utf-8") as file:
-            json.dump(profile, file)
+        filepath = self._save_profile_to_filesystem(cached_profile)
 
         # add profile to in-memory cache
-        self.profile_cache.append(cached_profile)
+        self.profile_cache[cached_profile.id] = cached_profile
         logger.info("Saved profile to cache, file location: %s", filepath)
         return cached_profile.id
 
@@ -222,9 +204,7 @@ def mark_as_existing(self, profile_id: str) -> bool:
             bool: True on success. False if JSON with this profile_id not found on filesystem
         """
         # find the profile data from the new_profiles cache and move it to existing_profiles
-        cached_profile = next(
-            (profile for profile in self.profile_cache if profile.id == profile_id), None
-        )
+        cached_profile = self.profile_cache.get(profile_id)
         if not cached_profile:
             # profile is not in cache; it must not exist
             logger.warning(
@@ -242,7 +222,9 @@ def mark_as_existing(self, profile_id: str) -> bool:
         # move the JSON file from the "new" to the "existing" directory and update cache
         existing_filepath = os.path.join(self._existing_dir, f"{profile_id}.json")
         os.rename(new_filepath, existing_filepath)
+
         cached_profile.is_new = False
+        self.profile_cache[profile_id] = cached_profile
 
         return True
 
@@ -251,37 +233,40 @@ def update(self, profile_id: str, data: dict) -> dict:
 
         Args:
             profile_id (str): The UUID of the Support Profile to update
-            data (dict): The JSON attributes to apply. Can be partial Support Profile
+            data (dict): The JSON attributes to apply. Must be complete Support Profile
 
         Returns:
-            dict: the latest version of the Profile, with all attribute changes applied
+            dict: the latest version of the Profile, with the profile overwritten by `data`, or
+                `None` on error (existing profile will be unchanged)
 
         Raises:
             FileNotFoundError: if no Support Profile exists with the provided id
         """
-        logger.info("Updating profile_id %s with new attributes: %s", profile_id, data)
+        logger.info("Updating profile_id %s with new data: %s", profile_id, data)
 
-        # find the profile data from the new_profiles cache, then apply updates and save over it
-        cached_profile = next(
-            (profile for profile in self.profile_cache if profile.id == profile_id), None
-        )
+        # find the profile data from the new_profiles cache, then save over it
+        cached_profile = self.profile_cache.get(profile_id)
         if not cached_profile:
             raise FileNotFoundError  # Profile with this ID does not exist in cache
 
-        # Apply dict of edits on top of existing cached profile (combining any nested attributes)
-        new_profile_data = deep_update(cached_profile.data, data)
-        is_new_profile = cached_profile.is_new
-
-        # a bit hacky, but the fastest and least duplicative way to update a Profile
-        # in cache + disk is to delete the existing profile and re-save with modified JSON data
-        self.delete(profile_id)
-        update_success = self.save(new_profile_data, is_new_profile)
+        if profile_id != data.get("id"):
+            raise ValueError(
+                (
+                    f"Refusing to overwrite existing profile ID {profile_id} with mismatched data "
+                    f"including id {data.get('id')}"
+                )
+            )
 
-        if not update_success:
+        updated_profile = CachedProfile(data, cached_profile.is_new)
+        # update disk with latest data; if the write fails, reject update
+        saved_file = self._save_profile_to_filesystem(updated_profile)
+        if not saved_file:
             logger.warning("Unable to update Profile ID %s for some reason", profile_id)
             return None
 
-        return new_profile_data
+        # update in-memory cache to overwrite previous profile by ID
+        self.profile_cache[profile_id] = updated_profile
+        return updated_profile.data
 
     def delete(self, profile_id: str) -> bool:
         """Delete a Support Profile profile from storage, based on its UUID.
@@ -305,18 +290,39 @@ def delete(self, profile_id: str) -> bool:
                 )
                 return False
 
+        # drop profile from disk
         logger.debug("Attempting to delete profile at path: %s", filepath)
         os.remove(filepath)
-
         # drop profile from cache
-        self.profile_cache = [
-            cached_profile
-            for cached_profile in self.profile_cache
-            if cached_profile.id != profile_id
-        ]
+        del self.profile_cache[profile_id]
         return True
 
-    def _load_profiles_from_filesystem(self, dir_: str) -> list[dict]:
+    def _save_profile_to_filesystem(self, profile: CachedProfile) -> str | None:
+        """Save CachedProfile data (dict) to filesystem so it persists through service restarts"""
+        profile_id = profile.data.get("id")
+        if not profile_id:
+            raise ValueError("Cannot save CachedProfile to file that has no `id` attribute")
+
+        file_dir = self._new_dir if profile.is_new else self._existing_dir
+        filepath = os.path.join(file_dir, f"{profile_id}.json")
+        logger.debug("Now saving profile to path: %s", filepath)
+        try:
+            with open(filepath, "w", encoding="utf-8") as file:
+                json.dump(profile.data, file)
+        except (PermissionError, json.JSONDecodeError, TypeError) as exc:
+            logger.error(
+                "Failed to save Profile %s to file %s due to: (%s) %s",
+                profile_id,
+                filepath,
+                type(exc),
+                str(exc),
+            )
+            return None
+
+        return filepath
+
+    @staticmethod
+    def _load_profiles_from_filesystem(dir_: str) -> list[dict]:
         """Read all JSON files from one of this ProfileStore's subdirectories, and return list of
         the discovered files' json data.
 

python/nwsc_proxy/test/test_ncp_web_service.py

@@ -233,20 +233,20 @@ def test_delete_profile_failure(wrapper: AppWrapper, mock_request: Mock, mock_pr
 
 
 def test_update_profile_success(wrapper: AppWrapper, mock_request: Mock, mock_profile_store: Mock):
-    mock_request.method = "PATCH"
+    mock_request.method = "PUT"
     mock_request.args = MultiDict({"id": EXAMPLE_UUID})
-    mock_request.json = {"name": "Some new name"}
-    updated_profile = {"id": EXAMPLE_UUID, "name": "Some new name"}
+    expected_data = {"id": EXAMPLE_UUID, "name": "Some new name"}
+    mock_request.json = expected_data
+    mock_profile_store.return_value.update.return_value = expected_data
 
-    mock_profile_store.return_value.update.return_value = updated_profile
     result: tuple[Response, int] = wrapper.app.view_functions["events"]()
 
     assert result[1] == 200
-    assert result[0].json["profile"] == updated_profile
+    assert result[0].json["profile"] == expected_data
 
 
 def test_update_no_body(wrapper: AppWrapper, mock_request: Mock, mock_profile_store: Mock):
-    mock_request.method = "PATCH"
+    mock_request.method = "PUT"
     mock_request.args = MultiDict({"uuid": EXAMPLE_UUID})
     mock_request.data = None
 
@@ -256,7 +256,7 @@ def test_update_no_body(wrapper: AppWrapper, mock_request: Mock, mock_profile_st
 
 
 def test_update_profile_missing(wrapper: AppWrapper, mock_request: Mock, mock_profile_store: Mock):
-    mock_request.method = "PATCH"
+    mock_request.method = "PUT"
     mock_request.args = MultiDict({"uuid": EXAMPLE_UUID})
     mock_profile_store.return_value.update.side_effect = FileNotFoundError
 

python/nwsc_proxy/test/test_profile_store.py

@@ -73,16 +73,16 @@ def store():
 
 # tests
 def test_profile_store_loads_api_responses(store: ProfileStore):
-    assert sorted([c.id for c in store.profile_cache]) == [
+    assert sorted(list(store.profile_cache.keys())) == [
         "a08370c6-ab87-4808-bd51-a8597e58410d",
         "e1033860-f198-4c6a-a91b-beaec905132f",
         "fd35adec-d2a0-49a9-a320-df20a7b6d681",
     ]
 
-    for cache_obj in store.profile_cache:
+    for cache_id, cache_obj in store.profile_cache.items():
         # should have loaded all profiles as status "existing", file should exist in that subdir
         assert not cache_obj.is_new
-        filepath = os.path.join(STORE_BASE_DIR, EXISTING_SUBDIR, f"{cache_obj.id}.json")
+        filepath = os.path.join(STORE_BASE_DIR, EXISTING_SUBDIR, f"{cache_id}.json")
         assert os.path.exists(filepath)
 
     # new directory should be empty to begin with
@@ -186,24 +186,37 @@ def test_update_profile_success(store: ProfileStore):
     profile_id = EXAMPLE_SUPPORT_PROFILE["id"]
     new_start_dt = "2026-01-01T12:00:00Z"
     new_name = "A different name"
-    new_profile_data = {"name": new_name, "setting": {"timing": {"start": new_start_dt}}}
+    new_profile = deepcopy(EXAMPLE_SUPPORT_PROFILE)
+    new_profile["name"] = new_name
+    new_profile["setting"] = {"timing": {"start": new_start_dt}}
 
-    updated_profile = store.update(profile_id, new_profile_data)
+    updated_profile = store.update(profile_id, new_profile)
 
     # data returned should have updated attributes
     assert updated_profile["name"] == new_name
     assert updated_profile["setting"]["timing"]["start"] == new_start_dt
-    # attributes at same level as any nested updated ones should not have changed
-    assert (
-        updated_profile["setting"]["timing"].get("durationInMinutes")
-        == EXAMPLE_SUPPORT_PROFILE["setting"]["timing"]["durationInMinutes"]
-    )
     # profile in cache should have indeed been changed
-    refetched_profile = next((p for p in store.profile_cache if p.id == profile_id), None)
+    refetched_profile = store.profile_cache.get(profile_id)
     assert refetched_profile.name == new_name
     assert datetime.fromtimestamp(refetched_profile.start_timestamp, UTC) == dt_parse(new_start_dt)
 
 
+def test_update_profile_error_rollback(store: ProfileStore):
+    profile_id = EXAMPLE_SUPPORT_PROFILE["id"]
+    new_name = "A different name"
+    new_profile = deepcopy(EXAMPLE_SUPPORT_PROFILE)
+    new_profile["name"] = new_name
+    # inject unhashable content into JSON data, will cause save() to fail and return None
+    new_profile["unhashable"] = set(["foo", "bar"])
+
+    updated_profile = store.update(profile_id, new_profile)
+
+    assert updated_profile is None
+    # profile in cache is still there, no changes were made to data
+    refetched_profile = store.profile_cache.get(profile_id)
+    assert refetched_profile.name != new_name
+
+
 def test_update_profile_not_found(store: ProfileStore):
     profile_id = "11111111-2222-3333-444444444444"  # fake ID does not exist in ProfileStore
     new_profile_data = {"name": "A different name"}