From 06507fbd906d630beef1abedbd754c39a3a53d4f 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 | 19 +++---- lib/host.py | 5 +- test_api.py | 5 ++ test_unit.py | 117 +++++++++++++++++++++++-------------------- 4 files changed, 81 insertions(+), 65 deletions(-) diff --git a/app/serialization.py b/app/serialization.py index 931f1be8ba..19ced477ad 100644 --- a/app/serialization.py +++ b/app/serialization.py @@ -1,3 +1,5 @@ +from collections import defaultdict + from app.models import Host as ModelsHost from app.exceptions import InputFormatException @@ -22,7 +24,7 @@ def from_json(cls, data): data.get("ansible_host"), data.get("account"), Facts.from_json(data.get("facts")), - data.get("system_profile", {}), + data.get("system_profile") ) @classmethod @@ -42,7 +44,7 @@ def to_json(cls, host): def to_system_profile_json(cls, host): return { "id": _serialize_uuid(host.id), - "system_profile": host.system_profile_facts or {} + "system_profile": host.system_profile_facts } @@ -70,7 +72,7 @@ class CanonicalFacts: @classmethod def from_json(cls, data): - return {field: data[field] for field in cls.fields if data.get(field)} + return {field: data[field] or None for field in cls.fields if data.get(field)} @classmethod def to_json(cls, canonical_facts): @@ -89,13 +91,12 @@ class Facts: @classmethod def from_json(cls, 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("Invalid format of Fact object. Fact " @@ -105,6 +106,6 @@ def from_json(cls, data): @classmethod def to_json(cls, facts): return [ - {"namespace": namespace, "facts": facts or {}} + {"namespace": namespace, "facts": facts} for namespace, facts in facts.items() ] diff --git a/lib/host.py b/lib/host.py index 6fe3451dde..3a836bdf3c 100644 --- a/lib/host.py +++ b/lib/host.py @@ -70,11 +70,12 @@ def find_host_by_insights_id(account_number, insights_id): def _canonical_facts_host_query(account_number, canonical_facts): + cf_values = dict(filter(lambda item: item[1] is not None, canonical_facts.items())) return ModelsHost.query.filter( (ModelsHost.account == account_number) & ( - ModelsHost.canonical_facts.comparator.contains(canonical_facts) - | ModelsHost.canonical_facts.comparator.contained_by(canonical_facts) + ModelsHost.canonical_facts.comparator.contains(cf_values) + | ModelsHost.canonical_facts.comparator.contained_by(cf_values) ) ) diff --git a/test_api.py b/test_api.py index 981007ce46..d7966c72be 100755 --- a/test_api.py +++ b/test_api.py @@ -220,6 +220,11 @@ def _validate_host(self, received_host, expected_host, class CreateHostsTestCase(DBAPITestCase): def test_create_and_update(self): + from logging import DEBUG, getLogger, INFO + + logger = getLogger("sqlalchemy.engine") + logger.setLevel(INFO) + facts = None host_data = HostWrapper(test_data(facts=facts)) diff --git a/test_unit.py b/test_unit.py index a0d0fe8053..94e30e3dac 100755 --- a/test_unit.py +++ b/test_unit.py @@ -398,11 +398,25 @@ def test_with_all_fields(self): self.assertEqual(value, getattr(actual, key)) def test_with_only_required_fields(self): - canonical_facts = {"fqdn": "some fqdn"} - host = SerializationHost.from_json(canonical_facts) + create_canonical_facts = {"fqdn": "some fqdn"} + host = SerializationHost.from_json(create_canonical_facts) self.assertIs(ModelsHost, type(host)) - self.assertEqual(canonical_facts, host.canonical_facts) + + expected_canonical_facts = { + **create_canonical_facts, + **{field: None for field in ( + "insights_id", + "rhel_machine_id", + "subscription_manager_id", + "satellite_id", + "bios_uuid", + "ip_addresses", + "mac_addresses", + "external_id", + )} + } + self.assertEqual(expected_canonical_facts, host.canonical_facts) self.assertIsNone(host.display_name) self.assertIsNone(host.ansible_host) self.assertIsNone(host.account) @@ -601,7 +615,18 @@ def test_with_only_required_fields(self): "account": None } host_init_data = { - "canonical_facts": {"fqdn": "some fqdn"}, + "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": {} } @@ -618,15 +643,6 @@ def test_with_only_required_fields(self): actual = SerializationHost.to_json(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"]), @@ -721,7 +737,6 @@ def test_empty_profile_is_empty_dict(self): display_name="some display name" ) host.id = uuid4() - host.system_profile_facts = None actual = SerializationHost.to_system_profile_json(host) expected = {"id": str(host.id), "system_profile": {}} @@ -778,17 +793,29 @@ def test_unknown_fields_are_rejected(self): result = CanonicalFacts.from_json(input) self.assertEqual(result, canonical_facts) - def test_empty_fields_are_rejected(self): - canonical_facts = {"fqdn": "some fqdn"} - input = { + def test_empty_fields_become_none(self): + canonical_facts = { + "insights_id": str(uuid4()), + "rhel_machine_id": str(uuid4()), + "subscription_manager_id": str(uuid4()), + "satellite_id": str(uuid4()), + "bios_uuid": str(uuid4()), + "ip_addresses": (), + "fqdn": "", + "mac_addresses": [], + "external_id": None + } + actual = CanonicalFacts.from_json(canonical_facts) + expected = { **canonical_facts, - "insights_id": "", - "rhel_machine_id": None, - "ip_addresses": [], - "mac_addresses": tuple() + **{ + "ip_addresses": None, + "fqdn": None, + "mac_addresses": None, + "external_id": None + } } - result = CanonicalFacts.from_json(input) - self.assertEqual(result, canonical_facts) + self.assertEqual(actual, expected) class SerializationCanonicalFactsToJsonTestCase(TestCase): @@ -807,22 +834,6 @@ def test_contains_all_values_unchanged(self): } self.assertEqual(canonical_facts, CanonicalFacts.to_json(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}, CanonicalFacts.to_json({}) - ) - class SerializationFactsFromJsonTestCase(TestCase): @@ -841,7 +852,7 @@ def test_non_empty_namespaces_become_dict_items(self): {item["namespace"]: item["facts"] for item in input}, Facts.from_json(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 = [ @@ -855,7 +866,7 @@ def test_empty_namespaces_remain_unchanged(self): } ] self.assertEqual( - {item["namespace"]: item["facts"] for item in input}, + {item["namespace"]: item["facts"] or {} for item in input}, Facts.from_json(input) ) @@ -932,19 +943,17 @@ 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() - ], - Facts.to_json(facts) - ) + facts = { + "first namespace": {}, + "second namespace": {"first key": "first value"} + } + self.assertEqual( + [ + {"namespace": namespace, "facts": facts} + for namespace, facts in facts.items() + ], + Facts.to_json(facts) + ) if __name__ == "__main__":