[#351] PR feedback

src/objects/api/utils.py

@@ -1,8 +1,10 @@
 from datetime import date
-from typing import Any, Dict, Union
+from typing import Dict, Union
 
 from djchoices import DjangoChoices
 
+from objects.typing import JSONValue
+
 
 def string_to_value(value: str) -> Union[str, float, date]:
     if is_number(value):
@@ -45,20 +47,22 @@ def display_choice_values_for_help_text(choices: DjangoChoices) -> str:
     return "\n".join(items)
 
 
-def merge_patch(target: Any, patch: Any) -> Dict[str, Any]:
-    """An implementation of https://datatracker.ietf.org/doc/html/rfc7396 - JSON Merge Patch"""
+def merge_patch(target: JSONValue, patch: JSONValue) -> Dict[str, JSONValue]:
+    """Merge two objects together recursively.
+
+    This is inspired by https://datatracker.ietf.org/doc/html/rfc7396 - JSON Merge Patch,
+    but deviating in some cases to suit our needs.
+    """
+
     if not isinstance(patch, dict):
         return patch
 
     if not isinstance(target, dict):
         # Ignore the contents and set it to an empty dict
         target = {}
     for k, v in patch.items():
-        if v is None:
-            if k in target:
-                # remove the key/value pair from target
-                del target[k]
-        else:
-            target[k] = merge_patch(target.get(k), v)
+        # According to RFC, we should remove k from target
+        # if v is None. This is where we deviate.
+        target[k] = merge_patch(target.get(k), v)
 
     return target

src/objects/api/v1/openapi.yaml

@@ -349,6 +349,8 @@ paths:
     patch:
       operationId: object_partial_update
       description: Update the OBJECT by creating a new RECORD with the updates values.
+        The provided `record.data` value will be merged recursively with the existing
+        record data.
       parameters:
       - in: header
         name: Accept-Crs

src/objects/api/v1/views.py

@@ -39,7 +39,8 @@
         description="Update the OBJECT by creating a new RECORD with the updates values."
     ),
     partial_update=extend_schema(
-        description="Update the OBJECT by creating a new RECORD with the updates values."
+        description="Update the OBJECT by creating a new RECORD with the updates values. "
+        "The provided `record.data` value will be merged recursively with the existing record data."
     ),
     destroy=extend_schema(
         description="Delete an OBJECT and all RECORDs belonging to it.",

src/objects/api/v2/openapi.yaml

@@ -377,8 +377,8 @@ paths:
     patch:
       operationId: object_partial_update
       description: Update the OBJECT by creating a new RECORD with the updates values.
-        The provided `record.data` value will be merged with the existing record data,
-        according to [RFC7396](https://datatracker.ietf.org/doc/html/rfc7396).
+        The provided `record.data` value will be merged recursively with the existing
+        record data.
       parameters:
       - in: header
         name: Accept-Crs

src/objects/api/v2/views.py

@@ -43,8 +43,7 @@
     ),
     partial_update=extend_schema(
         description="Update the OBJECT by creating a new RECORD with the updates values. "
-        "The provided `record.data` value will be merged with the existing record data, according "
-        "to [RFC7396](https://datatracker.ietf.org/doc/html/rfc7396)."
+        "The provided `record.data` value will be merged recursively with the existing record data."
     ),
     destroy=extend_schema(
         description="Delete an OBJECT and all RECORDs belonging to it.",

src/objects/tests/test_merge_patch.py

@@ -9,19 +9,24 @@ def test_merge_patch(self):
         test_data = [
             ({"a": "b"}, {"a": "c"}, {"a": "c"}),
             ({"a": "b"}, {"b": "c"}, {"a": "b", "b": "c"}),
-            ({"a": "b"}, {"a": None}, {}),
-            ({"a": "b", "b": "c"}, {"a": None}, {"b": "c"}),
+            ({"a": "b"}, {"a": None}, {"a": None}),
+            ({"a": "b", "b": "c"}, {"a": None}, {"a": None, "b": "c"}),
             ({"a": ["b"]}, {"a": "c"}, {"a": "c"}),
             ({"a": "c"}, {"a": ["b"]}, {"a": ["b"]}),
-            ({"a": {"b": "c"}}, {"a": {"b": "d", "c": None}}, {"a": {"b": "d"}}),
+            (
+                {"a": {"b": "c"}},
+                {"a": {"b": "d", "c": None}},
+                {"a": {"b": "d", "c": None}},
+            ),
             ({"a": [{"b": "c"}]}, {"a": [1]}, {"a": [1]}),
             (["a", "b"], ["c", "d"], ["c", "d"]),
             ({"a": "b"}, ["c"], ["c"]),
             ({"a": "foo"}, None, None),
             ({"a": "foo"}, "bar", "bar"),
             ({"e": None}, {"a": 1}, {"e": None, "a": 1}),
-            ([1, 2], {"a": "b", "c": None}, {"a": "b"}),
-            ({}, {"a": {"bb": {"ccc": None}}}, {"a": {"bb": {}}}),
+            ([1, 2], {"a": "b", "c": None}, {"a": "b", "c": None}),
+            ({}, {"a": {"bb": {"ccc": None}}}, {"a": {"bb": {"ccc": None}}}),
+            ({"a": "b"}, {"a": "b"}, {"a": "b"}),
         ]
 
         for target, patch, expected in test_data:

src/objects/tests/v2/test_object_api.py

@@ -1,13 +1,14 @@
 import json
 import uuid
 from datetime import date, timedelta
+from typing import cast
 
 import requests_mock
 from freezegun import freeze_time
 from rest_framework import status
 from rest_framework.test import APITestCase
 
-from objects.core.models import Object
+from objects.core.models import Object, ObjectType
 from objects.core.tests.factories import (
     ObjectFactory,
     ObjectRecordFactory,
@@ -33,7 +34,9 @@ class ObjectApiTests(TokenAuthMixin, APITestCase):
     def setUpTestData(cls):
         super().setUpTestData()
 
-        cls.object_type = ObjectTypeFactory(service__api_root=OBJECT_TYPES_API)
+        cls.object_type = cast(
+            ObjectType, ObjectTypeFactory(service__api_root=OBJECT_TYPES_API)
+        )
         PermissionFactory.create(
             object_type=cls.object_type,
             mode=PermissionModes.read_and_write,
@@ -237,7 +240,7 @@ def test_patch_object_record(self, m):
         url = reverse("object-detail", args=[object.uuid])
         data = {
             "record": {
-                "data": {"plantDate": "2020-04-12", "diameter": 30},
+                "data": {"plantDate": "2020-04-12", "diameter": 30, "name": None},
                 "startAt": "2020-01-01",
                 "correctionFor": initial_record.index,
             },
@@ -257,7 +260,7 @@ def test_patch_object_record(self, m):
         # The actual behavior of the data merging is in test_merge_patch.py:
         self.assertEqual(
             current_record.data,
-            {"plantDate": "2020-04-12", "diameter": 30, "name": "Name"},
+            {"plantDate": "2020-04-12", "diameter": 30, "name": None},
         )
         self.assertEqual(current_record.start_at, date(2020, 1, 1))
         self.assertEqual(current_record.registration_at, date(2020, 8, 8))

src/objects/typing.py

@@ -0,0 +1,7 @@
+from typing import Dict, List, Union
+
+JSONPrimitive = Union[str, int, None, float, bool]
+
+JSONValue = Union[JSONPrimitive, "JSONObject", List["JSONValue"]]
+
+JSONObject = Dict[str, JSONValue]