From a7fb786b7e7cf471ce7316dbbeeb80346ddbdd49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20Tomsa?= Date: Wed, 3 Jul 2019 16:26:31 +0200 Subject: [PATCH] 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. --- app/serialization.py | 26 +++++++-------- lib/host_repository.py | 5 +-- test_unit.py | 72 ++++++++++++++---------------------------- 3 files changed, 40 insertions(+), 63 deletions(-) diff --git a/app/serialization.py b/app/serialization.py index d66007022..915ac51c4 100644 --- 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"), validated_data.get("stale_timestamp"), validated_data.get("reporter"), ) @@ -76,7 +77,7 @@ def serialize_host(host, staleness_offset): 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): @@ -88,13 +89,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( @@ -104,7 +104,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): @@ -113,8 +118,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 index 81084b269..194b0a39e 100644 --- a/lib/host_repository.py +++ b/lib/host_repository.py @@ -60,11 +60,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 index b6ccdfa65..b7a9dd54a 100755 --- a/test_unit.py +++ b/test_unit.py @@ -580,6 +580,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) @@ -794,7 +795,7 @@ def test_without_system_profile( input["account"], deserialize_facts.return_value, deserialize_tags.return_value, - {}, + None, input["stale_timestamp"], input["reporter"], ) @@ -942,7 +943,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()} @@ -953,15 +969,6 @@ def test_with_only_required_fields(self): actual = serialize_host(host, staleness_offset) 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"]), @@ -1075,7 +1082,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": {}} @@ -1128,19 +1134,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 = { @@ -1156,20 +1149,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): @@ -1179,14 +1158,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 = [ @@ -1232,13 +1211,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):