refactor(version-history): FTRS-1627 update DeepDiff usage in version history and simplify logging

Author: prjh2-nhs

application/packages/python/ftrs_data_layer/logbase.py

@@ -296,36 +296,27 @@ class VersionHistoryLogBase(LogBase):
         level=DEBUG,
         message="Processing stream record: entity={entity_name}, record_id={record_id}, field={field_name}, event_type={event_type}",
     )
-    VH_PROCESSOR_004 = LogReference(
-        level=DEBUG,
-        message="Change detected: old_value={old_value}, new_value={new_value}",
-    )
-    VH_PROCESSOR_005 = LogReference(
-        level=DEBUG, message="Writing version history item to table: {version_item}"
-    )
     VH_PROCESSOR_006 = LogReference(
         level=DEBUG,
         message=(
             "Extracted stream record data: entity_name={entity_name}, "
             "record_id={record_id}, field_name={field_name}, "
-            "has_old_image={has_old_image}, has_new_image={has_new_image}, keys={keys}"
+            "has_old_image={has_old_image}, has_new_image={has_new_image}"
         ),
     )
     VH_PROCESSOR_007 = LogReference(
         level=DEBUG,
         message=(
             "Resolved event configuration: event_name={event_name}, "
             "change_type={change_type}, old_val_present={old_val_present}, "
-            "new_val_present={new_val_present}, old_val={old_val}, "
-            "new_val={new_val}, old_type={old_type}, new_type={new_type}"
+            "new_val_present={new_val_present}"
         ),
     )
     VH_PROCESSOR_008 = LogReference(
         level=DEBUG,
         message=(
             "Computed field delta: field_name={field_name}, "
-            "delta_keys={delta_keys}, has_diff={has_diff}, "
-            "delta={delta}, values_equal={values_equal}"
+            "delta_keys={delta_keys}, has_changes={has_changes}"
         ),
     )
     VH_PROCESSOR_009 = LogReference(
@@ -339,40 +330,13 @@ class VersionHistoryLogBase(LogBase):
         level=DEBUG,
         message="Prepared version history identifiers: entity_id={entity_id}, timestamp={timestamp}",
     )
-    VH_PROCESSOR_013 = LogReference(
-        level=DEBUG,
-        message=(
-            "Skipping UPDATE - simple values are equal: "
-            "entity={entity_name}/{record_id}/{field_name}, "
-            "old={old_val}, new={new_val}"
-        ),
-    )
-    VH_PROCESSOR_014 = LogReference(
-        level=DEBUG,
-        message=(
-            "Proceeding with UPDATE - simple values differ: "
-            "old={old_val}, new={new_val}"
-        ),
-    )
     VH_PROCESSOR_015 = LogReference(
-        level=DEBUG,
+        level=INFO,
         message=(
-            "Skipping UPDATE - complex values have empty diff: "
+            "Skipping UPDATE - no changes detected: "
             "entity={entity_name}/{record_id}/{field_name}"
         ),
     )
-    VH_PROCESSOR_016 = LogReference(
-        level=DEBUG,
-        message="Proceeding with UPDATE - complex values have diff: {diff}",
-    )
-    VH_PROCESSOR_017 = LogReference(
-        level=DEBUG,
-        message=(
-            "Extracting values: field_name={field_name}, "
-            "is_document_field={is_document_field}, "
-            "system_fields_excluded={system_fields_count}"
-        ),
-    )
 
 
 class UtilsLogBase(LogBase):

services/data-migration/src/version_history/stream_processor.py

@@ -53,7 +53,7 @@ def _extract_record_metadata(
     new_image = record.dynamodb.new_image
 
     record_id = keys.get("id")
-    # Field name for composite key: "document" for gp org entities, may differ for pharmacy
+    # Field name for composite key: defaults to "document" for gp org entities, may differ for pharmacy service types
     field_name = keys.get("field", "document")
     event_name = record.get("eventName", "MODIFY")
 
@@ -95,13 +95,6 @@ def _extract_field_values(
         old_value = old_image.get(field_name) if old_image else None
         new_value = new_image.get(field_name) if new_image else None
 
-    LOGGER.log(
-        VersionHistoryLogBase.VH_PROCESSOR_017,
-        field_name=field_name,
-        is_document_field=(field_name == "document"),
-        system_fields_count=len(SYSTEM_FIELDS),
-    )
-
     return old_value, new_value
 
 
@@ -125,10 +118,6 @@ def _determine_change_type(
         change_type=change_type,
         old_val_present=old_value is not None,
         new_val_present=new_value is not None,
-        old_val=old_value,
-        new_val=new_value,
-        old_type=type(old_value).__name__,
-        new_type=type(new_value).__name__,
     )
 
     return change_type
@@ -145,20 +134,15 @@ def _should_skip_update(
     if change_type != "UPDATE":
         return False
 
-    # Check for empty diff in complex values
-    if not field_delta.get("diff"):
+    # Check for empty diff (no changes detected by DeepDiff)
+    if not field_delta:
         LOGGER.log(
             VersionHistoryLogBase.VH_PROCESSOR_015,
             entity_name=entity_name,
             record_id=record_id,
             field_name=field_name,
         )
         return True
-    else:
-        LOGGER.log(
-            VersionHistoryLogBase.VH_PROCESSOR_016,
-            diff=field_delta.get("diff"),
-        )
 
     return False
 
@@ -216,7 +200,6 @@ def process_stream_record(
         field_name=field_name,
         has_old_image=old_image is not None,
         has_new_image=new_image is not None,
-        keys=keys,
     )
 
     LOGGER.log(
@@ -234,23 +217,15 @@ def process_stream_record(
     LOGGER.log(
         VersionHistoryLogBase.VH_PROCESSOR_008,
         field_name=field_name,
-        delta_keys=list(field_delta.keys()),
-        has_diff="diff" in field_delta,
-        delta=field_delta,
-        values_equal=field_delta.get("old") == field_delta.get("new"),
+        delta_keys=list(field_delta.keys()) if field_delta else [],
+        has_changes=bool(field_delta),
     )
 
     if _should_skip_update(
         change_type, field_delta, entity_name, record_id, field_name
     ):
         return
 
-    LOGGER.log(
-        VersionHistoryLogBase.VH_PROCESSOR_004,
-        old_value=old_value,
-        new_value=new_value,
-    )
-
     version_item = _create_version_item(
         entity_name,
         record_id,
@@ -261,11 +236,6 @@ def process_stream_record(
         field_name,
     )
 
-    LOGGER.log(
-        VersionHistoryLogBase.VH_PROCESSOR_005,
-        version_item=version_item,
-    )
-
     version_history_table.put_item(Item=version_item)
 
     LOGGER.log(

services/data-migration/src/version_history/utils.py

@@ -53,31 +53,22 @@ def extract_changed_by(new_image: Dict[str, Any]) -> Dict[str, str]:
 
 def compute_field_delta(old_value: Any, new_value: Any) -> Dict[str, Any]:  # noqa: ANN401
     """
-    Compute a structured delta between old and new field values.
+    Compute a structured delta between old and new field values using DeepDiff.
 
-    For simple values (str, int, bool, None), returns {"old": old_value, "new": new_value}.
-    For complex values (dict, list), uses DeepDiff to compute a detailed delta.
+    Returns DeepDiff's native structure which provides detailed change information:
+    - values_changed: {"root['field']": {"old_value": ..., "new_value": ...}}
+    - dictionary_item_added: {"root['field']": value}
+    - dictionary_item_removed: {"root['field']": value}
+    - iterable_item_added: {"root[index]": value}
+    - iterable_item_removed: {"root[index]": value}
 
     Args:
         old_value: Previous field value
         new_value: New field value
 
     Returns:
-        Dictionary with delta information:
-        - For simple values: {"old": <value>, "new": <value>}
-        - For complex values: {"old": <value>, "new": <value>, "diff": <deepdiff_json>}
+        Dictionary with DeepDiff structure. Empty dict if values are identical.
     """
-    # Simple types: return old/new
-    if not isinstance(old_value, (dict, list)) and not isinstance(
-        new_value, (dict, list)
-    ):
-        return {"old": old_value, "new": new_value}
-
-    # Type mismatch: return old/new
-    if type(old_value) is not type(new_value):
-        return {"old": old_value, "new": new_value}
-
-    # Complex types: compute detailed diff
     diff = DeepDiff(
         old_value,
         new_value,
@@ -86,5 +77,4 @@ def compute_field_delta(old_value: Any, new_value: Any) -> Dict[str, Any]:  # no
         ignore_order=True,
     )
 
-    diff_json = json.loads(diff.to_json())
-    return {"old": old_value, "new": new_value, "diff": diff_json}
+    return json.loads(diff.to_json())

services/data-migration/tests/unit/version_history/test_stream_processor.py

@@ -33,22 +33,21 @@ def test_process_organisation_document_field_change(
         assert item["change_type"] == "UPDATE"
         assert "document" in item["changed_fields"]
 
-        # Verify that the document field contains a diff of the changes
+        # Verify that the document field contains DeepDiff structure
         document_delta = item["changed_fields"]["document"]
-        assert "diff" in document_delta
-        assert "old" in document_delta
-        assert "new" in document_delta
-
-        # Verify system fields were excluded from the values
-        old_val = document_delta["old"]
-        new_val = document_delta["new"]
-        assert "id" not in old_val
-        assert "field" not in old_val
-        assert "created" not in old_val
-        assert "lastUpdated" not in old_val
-        assert "name" in old_val
-        assert old_val["name"] == "Old Practice Name"
-        assert new_val["name"] == "New Practice Name"
+        assert isinstance(document_delta, dict)
+
+        # Should show the name field change
+        assert "values_changed" in document_delta
+        assert "root['name']" in document_delta["values_changed"]
+        assert (
+            document_delta["values_changed"]["root['name']"]["old_value"]
+            == "Old Practice Name"
+        )
+        assert (
+            document_delta["values_changed"]["root['name']"]["new_value"]
+            == "New Practice Name"
+        )
 
         assert item["changed_by"]["type"] == "app"
         assert item["changed_by"]["value"] == "INTERNAL001"
@@ -173,18 +172,22 @@ def test_process_document_field_handles_insert_event(
         item = call_args.kwargs["Item"]
 
         assert item["change_type"] == "CREATE"
-        expected_changed_fields = {
-            "document": {
-                "old": None,
-                "new": {
-                    "name": "New Practice Name",
-                    "active": True,
-                    "identifier_ODS_ODSCode": "B98765",
-                    "type": "GP Practice",
-                },
-            }
-        }
-        assert item["changed_fields"] == expected_changed_fields
+        # For CREATE, DeepDiff compares None to new value
+        document_delta = item["changed_fields"]["document"]
+        assert "values_changed" in document_delta or "type_changes" in document_delta
+        # Verify it captured the new document
+        if "type_changes" in document_delta:
+            # DeepDiff reports None -> dict as type change
+            assert document_delta["type_changes"]["root"]["old_value"] is None
+            new_val = document_delta["type_changes"]["root"]["new_value"]
+            assert new_val["name"] == "New Practice Name"
+            assert new_val["active"] is True
+            assert new_val["identifier_ODS_ODSCode"] == "B98765"
+            assert new_val["type"] == "GP Practice"
+        else:
+            assert document_delta["values_changed"]["root"]["old_value"] is None
+            new_val = document_delta["values_changed"]["root"]["new_value"]
+            assert new_val["name"] == "New Practice Name"
         assert item["changed_by"]["type"] == "app"
         assert item["changed_by"]["value"] == "INTERNAL001"
 
@@ -238,16 +241,20 @@ def test_process_document_field_handles_remove_event(
         item = call_args.kwargs["Item"]
 
         assert item["change_type"] == "DELETE"
-        expected_changed_fields = {
-            "document": {
-                "old": {
-                    "name": "Old Location Name",
-                    "status": "active",
-                    "physicalType": "building",
-                },
-                "new": None,
-            }
-        }
-        assert item["changed_fields"] == expected_changed_fields
+        # For DELETE, DeepDiff compares old value to None
+        document_delta = item["changed_fields"]["document"]
+        assert "values_changed" in document_delta or "type_changes" in document_delta
+        # Verify it captured the old document
+        if "type_changes" in document_delta:
+            # DeepDiff reports dict -> None as type change
+            old_val = document_delta["type_changes"]["root"]["old_value"]
+            assert old_val["name"] == "Old Location Name"
+            assert old_val["status"] == "active"
+            assert old_val["physicalType"] == "building"
+            assert document_delta["type_changes"]["root"]["new_value"] is None
+        else:
+            old_val = document_delta["values_changed"]["root"]["old_value"]
+            assert old_val["name"] == "Old Location Name"
+            assert document_delta["values_changed"]["root"]["new_value"] is None
         assert item["changed_by"]["type"] == "user"
         assert item["changed_by"]["value"] == "user-456"