Serialization: Host serialization cleanup

app/serialization.py

@@ -1,3 +1,5 @@
+from collections import defaultdict
+
 from app.exceptions import InputFormatException
 from app.models import Host as Host
 
@@ -18,72 +20,64 @@
 )
 
 
+def _serialize_datetime(dt):
+    return dt.isoformat() + "Z"
+
+
+def _serialize_uuid(u):
+    return str(u)
+
+
 def deserialize_host(data):
-    canonical_facts = _deserialize_canonical_facts(data)
-    facts = _deserialize_facts(data.get("facts"))
     return Host(
-        canonical_facts,
-        data.get("display_name", None),
+        _deserialize_canonical_facts(data),
+        data.get("display_name"),
         data.get("ansible_host"),
         data.get("account"),
-        facts,
-        data.get("system_profile", {}),
+        _deserialize_facts(data.get("facts")),
+        data.get("system_profile"),
     )
 
 
 def serialize_host(host):
-    json_dict = _serialize_canonical_facts(host.canonical_facts)
-    json_dict["id"] = str(host.id)
-    json_dict["account"] = host.account
-    json_dict["display_name"] = host.display_name
-    json_dict["ansible_host"] = host.ansible_host
-    json_dict["facts"] = _serialize_facts(host.facts)
-    json_dict["created"] = host.created_on.isoformat() + "Z"
-    json_dict["updated"] = host.modified_on.isoformat() + "Z"
-    return json_dict
+    return {
+        **_serialize_canonical_facts(host.canonical_facts),
+        "id": _serialize_uuid(host.id),
+        "account": host.account,
+        "display_name": host.display_name,
+        "ansible_host": host.ansible_host,
+        "facts": _serialize_facts(host.facts),
+        "created": _serialize_datetime(host.created_on),
+        "updated": _serialize_datetime(host.modified_on),
+    }
 
 
 def serialize_host_system_profile(host):
-    json_dict = {"id": str(host.id), "system_profile": host.system_profile_facts or {}}
-    return json_dict
+    return {"id": _serialize_uuid(host.id), "system_profile": host.system_profile_facts}
 
 
 def _deserialize_canonical_facts(data):
-    canonical_fact_list = {}
-    for cf in _CANONICAL_FACTS_FIELDS:
-        # Do not allow the incoming canonical facts to be None or ''
-        if cf in data and data[cf]:
-            canonical_fact_list[cf] = data[cf]
-    return canonical_fact_list
+    return {field: data[field] for field in _CANONICAL_FACTS_FIELDS if data.get(field)}
 
 
 def _serialize_canonical_facts(canonical_facts):
-    canonical_fact_dict = dict.fromkeys(_CANONICAL_FACTS_FIELDS, None)
-    for cf in _CANONICAL_FACTS_FIELDS:
-        if cf in canonical_facts:
-            canonical_fact_dict[cf] = canonical_facts[cf]
-    return canonical_fact_dict
+    return {field: canonical_facts.get(field) for field in _CANONICAL_FACTS_FIELDS}
 
 
 def _deserialize_facts(data):
-    if data is None:
-        data = []
-
-    fact_dict = {}
-    for fact in data:
-        if "namespace" in fact and "facts" in fact:
-            if fact["namespace"] in fact_dict:
-                fact_dict[fact["namespace"]].update(fact["facts"])
-            else:
-                fact_dict[fact["namespace"]] = fact["facts"]
-        else:
+    facts = defaultdict(lambda: {})
+    for item in data or []:
+        try:
+            old_facts = facts[item["namespace"]]
+            new_facts = item["facts"] or {}
+            facts[item["namespace"]] = {**old_facts, **new_facts}
+        except KeyError:
             # The facts from the request are formatted incorrectly
             raise InputFormatException(
                 "Invalid format of Fact object.  Fact must contain 'namespace' and 'facts' keys."
             )
-    return fact_dict
+    return facts
 
 
 def _serialize_facts(facts):
-    fact_list = [{"namespace": namespace, "facts": facts if facts else {}} for namespace, facts in facts.items()]
-    return fact_list
+    return [{"namespace": namespace, "facts": facts} for namespace, facts in facts.items()]

lib/host_repository.py

@@ -70,11 +70,12 @@ def _find_host_by_elevated_ids(account_number, canonical_facts):
 
 
 def _canonical_facts_host_query(account_number, canonical_facts):
+    cf_values = dict(filter(lambda item: item[1] is not None, canonical_facts.items()))
     return Host.query.filter(
         (Host.account == account_number)
         & (
-            Host.canonical_facts.comparator.contains(canonical_facts)
-            | Host.canonical_facts.comparator.contained_by(canonical_facts)
+            Host.canonical_facts.comparator.contains(cf_values)
+            | Host.canonical_facts.comparator.contained_by(cf_values)
         )
     )
 

test_unit.py

@@ -7,6 +7,7 @@
 from unittest import TestCase
 from unittest.mock import Mock
 from unittest.mock import patch
+from uuid import UUID
 from uuid import uuid4
 
 from api import api_operation
@@ -23,7 +24,9 @@
 from app.serialization import _deserialize_canonical_facts
 from app.serialization import _deserialize_facts
 from app.serialization import _serialize_canonical_facts
+from app.serialization import _serialize_datetime
 from app.serialization import _serialize_facts
+from app.serialization import _serialize_uuid
 from app.serialization import deserialize_host
 from app.serialization import serialize_host
 from app.serialization import serialize_host_system_profile
@@ -253,6 +256,26 @@ def test_config_development_settings(self):
             self.assertEqual(conf.db_pool_timeout, 3)
 
 
+class HostSerializeDatetime(TestCase):
+    def test_short_utc_timezone_is_included(self):
+        now = datetime.utcnow()
+        self.assertEqual(f"{now.isoformat()}Z", _serialize_datetime(now))
+
+    def test_iso_format_is_used(self):
+        dt = datetime(2019, 7, 3, 1, 1, 4, 20647)
+        self.assertEqual("2019-07-03T01:01:04.020647Z", _serialize_datetime(dt))
+
+
+class HostSerializeUuid(TestCase):
+    def test_uuid_has_hyphens_computed(self):
+        u = uuid4()
+        self.assertEqual(str(u), _serialize_uuid(u))
+
+    def test_uuid_has_hyphens_literal(self):
+        u = "4950e534-bbef-4432-bde2-aa3dd2bd0a52"
+        self.assertEqual(u, _serialize_uuid(UUID(u)))
+
+
 class HostOrderHowTestCase(TestCase):
     def test_asc(self):
         column = Mock()
@@ -381,6 +404,7 @@ def test_with_only_required_fields(self):
         host = deserialize_host(canonical_facts)
 
         self.assertIs(Host, type(host))
+
         self.assertEqual(canonical_facts, host.canonical_facts)
         self.assertIsNone(host.display_name)
         self.assertIsNone(host.ansible_host)
@@ -512,7 +536,7 @@ def test_without_system_profile(self, deserialize_canonical_facts, deserialize_f
             input["ansible_host"],
             input["account"],
             deserialize_facts.return_value,
-            {},
+            None,
         )
 
 
@@ -569,7 +593,22 @@ def test_with_all_fields(self):
 
     def test_with_only_required_fields(self):
         unchanged_data = {"display_name": None, "account": None}
-        host_init_data = {"canonical_facts": {"fqdn": "some fqdn"}, **unchanged_data, "facts": {}}
+        host_init_data = {
+            "canonical_facts": {
+                "insights_id": None,
+                "rhel_machine_id": None,
+                "subscription_manager_id": None,
+                "satellite_id": None,
+                "bios_uuid": None,
+                "ip_addresses": None,
+                "fqdn": "some fqdn",
+                "mac_addresses": None,
+                "external_id": None,
+                "ansible_host": None,
+            },
+            **unchanged_data,
+            "facts": {},
+        }
         host = Host(**host_init_data)
 
         host_attr_data = {"id": uuid4(), "created_on": datetime.utcnow(), "modified_on": datetime.utcnow()}
@@ -579,15 +618,6 @@ def test_with_only_required_fields(self):
         actual = serialize_host(host)
         expected = {
             **host_init_data["canonical_facts"],
-            "insights_id": None,
-            "rhel_machine_id": None,
-            "subscription_manager_id": None,
-            "satellite_id": None,
-            "bios_uuid": None,
-            "ip_addresses": None,
-            "mac_addresses": None,
-            "external_id": None,
-            "ansible_host": None,
             **unchanged_data,
             "facts": [],
             "id": str(host_attr_data["id"]),
@@ -658,7 +688,6 @@ def test_non_empty_profile_is_not_changed(self):
     def test_empty_profile_is_empty_dict(self):
         host = Host(canonical_facts={"fqdn": "some fqdn"}, display_name="some display name")
         host.id = uuid4()
-        host.system_profile_facts = None
 
         actual = serialize_host_system_profile(host)
         expected = {"id": str(host.id), "system_profile": {}}
@@ -711,19 +740,6 @@ def test_unknown_fields_are_rejected(self):
         result = _deserialize_canonical_facts(input)
         self.assertEqual(result, canonical_facts)
 
-    def test_empty_fields_are_rejected(self):
-        canonical_facts = {"fqdn": "some fqdn"}
-        input = {
-            **canonical_facts,
-            "insights_id": "",
-            "rhel_machine_id": None,
-            "ip_addresses": [],
-            "mac_addresses": tuple(),
-        }
-        result = _deserialize_canonical_facts(input)
-        self.assertEqual(result, canonical_facts)
-
-
 class SerializationSerializeCanonicalFactsTestCase(TestCase):
     def test_contains_all_values_unchanged(self):
         canonical_facts = {
@@ -739,20 +755,6 @@ def test_contains_all_values_unchanged(self):
         }
         self.assertEqual(canonical_facts, _serialize_canonical_facts(canonical_facts))
 
-    def test_missing_fields_are_filled_with_none(self):
-        canonical_fact_fields = (
-            "insights_id",
-            "rhel_machine_id",
-            "subscription_manager_id",
-            "satellite_id",
-            "bios_uuid",
-            "ip_addresses",
-            "fqdn",
-            "mac_addresses",
-            "external_id",
-        )
-        self.assertEqual({field: None for field in canonical_fact_fields}, _serialize_canonical_facts({}))
-
 
 class SerializationDeserializeFactsTestCase(TestCase):
     def test_non_empty_namespaces_become_dict_items(self):
@@ -762,14 +764,14 @@ def test_non_empty_namespaces_become_dict_items(self):
         ]
         self.assertEqual({item["namespace"]: item["facts"] for item in input}, _deserialize_facts(input))
 
-    def test_empty_namespaces_remain_unchanged(self):
+    def test_empty_namespaces_become_empty_dict(self):
         for empty_facts in ({}, None):
             with self.subTest(empty_facts=empty_facts):
                 input = [
                     {"namespace": "first namespace", "facts": {"first key": "first value"}},
                     {"namespace": "second namespace", "facts": empty_facts},
                 ]
-                self.assertEqual({item["namespace"]: item["facts"] for item in input}, _deserialize_facts(input))
+                self.assertEqual({item["namespace"]: item["facts"] or {} for item in input}, _deserialize_facts(input))
 
     def test_duplicate_namespaces_are_merged(self):
         input = [
@@ -815,13 +817,10 @@ def test_non_empty_namespaces_become_list_of_dicts(self):
         )
 
     def test_empty_namespaces_have_facts_as_empty_dicts(self):
-        for empty_value in {}, None:
-            with self.subTest(empty_value=empty_value):
-                facts = {"first namespace": empty_value, "second namespace": {"first key": "first value"}}
-                self.assertEqual(
-                    [{"namespace": namespace, "facts": facts or {}} for namespace, facts in facts.items()],
-                    _serialize_facts(facts),
-                )
+        facts = {"first namespace": {}, "second namespace": {"first key": "first value"}}
+        self.assertEqual(
+            [{"namespace": namespace, "facts": facts} for namespace, facts in facts.items()], _serialize_facts(facts)
+        )
 
 
 if __name__ == "__main__":