Trust the deserialized values

Change the (de)serialization logic, so the serialization expects that the object is valid. All default values and validation happens now upon deserialization. That concentrates the logic into a single place.
RedHatInsights · Aug 6, 2019 · 5c099fe · 5c099fe
1 parent 356e4d7
commit 5c099fe
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 58 deletions.
diff --git a/app/serialization.py b/app/serialization.py
@@ -1,3 +1,5 @@
+from collections import defaultdict
+
 from app.exceptions import InputFormatException
 from app.models import Host as Host
 
@@ -33,7 +35,7 @@ def deserialize_host(data):
         data.get("ansible_host"),
         data.get("account"),
         _deserialize_facts(data.get("facts")),
-        data.get("system_profile", {}),
+        data.get("system_profile"),
     )
 
 
@@ -51,7 +53,7 @@ def serialize_host(host):
 
 
 def serialize_host_system_profile(host):
-    return {"id": _serialize_uuid(host.id), "system_profile": host.system_profile_facts or {}}
+    return {"id": _serialize_uuid(host.id), "system_profile": host.system_profile_facts}
 
 
 def _deserialize_canonical_facts(data):
@@ -63,13 +65,12 @@ def _serialize_canonical_facts(canonical_facts):
 
 
 def _deserialize_facts(data):
-    facts = {}
+    facts = defaultdict(lambda: {})
     for item in data or []:
         try:
-            if item["namespace"] in facts:
-                facts[item["namespace"]].update(item["facts"])
-            else:
-                facts[item["namespace"]] = item["facts"]
+            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(
@@ -79,4 +80,4 @@ def _deserialize_facts(data):
 
 
 def _serialize_facts(facts):
-    return [{"namespace": namespace, "facts": facts or {}} for namespace, facts in facts.items()]
+    return [{"namespace": namespace, "facts": facts} for namespace, facts in facts.items()]
diff --git a/lib/host_repository.py b/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)
         )
     )
 

diff --git a/test_unit.py b/test_unit.py
@@ -404,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)
@@ -535,7 +536,7 @@ def test_without_system_profile(self, deserialize_canonical_facts, deserialize_f
             input["ansible_host"],
             input["account"],
             deserialize_facts.return_value,
-            {},
+            None,
         )
 
 
@@ -592,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()}
@@ -602,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"]),
@@ -681,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": {}}
@@ -734,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 = {
@@ -762,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):
@@ -785,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 = [
@@ -838,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__":