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 · Nov 22, 2019 · 0e8a11e · 0e8a11e
1 parent ec17174
commit 0e8a11e
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 63 deletions.
diff --git a/app/serialization.py b/app/serialization.py
@@ -1,3 +1,4 @@
+from collections import defaultdict
 from datetime import timezone
 
 from marshmallow import ValidationError
@@ -41,7 +42,7 @@ def deserialize_host(raw_data):
         validated_data.get("account"),
         facts,
         tags,
-        validated_data.get("system_profile", {}),
+        validated_data.get("system_profile"),
     )
 
 
@@ -61,7 +62,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):
@@ -73,13 +74,12 @@ def serialize_canonical_facts(canonical_facts):
 
 
 def _deserialize_facts(data):
-    facts = {}
+    facts = defaultdict(lambda: {})
     for fact in [] if data is None else data:
         try:
-            if fact["namespace"] in facts:
-                facts[fact["namespace"]].update(fact["facts"])
-            else:
-                facts[fact["namespace"]] = fact["facts"]
+            old_facts = facts[fact["namespace"]]
+            new_facts = fact["facts"] or {}
+            facts[fact["namespace"]] = {**old_facts, **new_facts}
         except KeyError:
             # The facts from the request are formatted incorrectly
             raise InputFormatException(
@@ -89,7 +89,12 @@ 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()]
+
+
+def _deserialize_tags(tags):
+    # TODO: Move the deserialization logic to this method.
+    return Tag.create_nested_from_tags(Tag.create_structered_tags_from_tag_data_list(tags))
 
 
 def _serialize_datetime(dt):
@@ -98,8 +103,3 @@ def _serialize_datetime(dt):
 
 def _serialize_uuid(u):
     return str(u)
-
-
-def _deserialize_tags(tags):
-    # TODO: Move the deserialization logic to this method.
-    return Tag.create_nested_from_tags(Tag.create_structered_tags_from_tag_data_list(tags))
diff --git a/lib/host_repository.py b/lib/host_repository.py
@@ -61,11 +61,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
@@ -561,6 +561,7 @@ def test_with_only_required_fields(self):
         host = deserialize_host({"account": account, **canonical_facts})
 
         self.assertIs(Host, type(host))
+
         self.assertEqual(canonical_facts, host.canonical_facts)
         self.assertIsNone(host.display_name)
         self.assertIsNone(host.ansible_host)
@@ -761,7 +762,7 @@ def test_without_system_profile(
             input["account"],
             deserialize_facts.return_value,
             deserialize_tags.return_value,
-            {},
+            None,
         )
 
     def test_host_validation(
@@ -849,7 +850,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()}
@@ -859,15 +875,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"]),
@@ -938,7 +945,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": {}}
@@ -991,19 +997,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 = {
@@ -1019,20 +1012,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):
@@ -1042,14 +1021,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 = [
@@ -1095,13 +1074,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)
+        )
 
 
 class SerializationSerializeDatetime(TestCase):