Merge pull request #43 from networktocode/gfm-get_by_uids-error

get_by_uids() should raise ObjectNotFound when appropriate

Author: glennmatthews

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- `DiffSync.get_by_uids()` now raises `ObjectNotFound` if any of the provided uids cannot be located.
+- `DiffSync.get()` raises `ObjectNotFound` or `ValueError` on failure, instead of returning `None`.
 - #34 - in diff dicts, change keys `src`/`dst`/`_src`/`_dst` to `-` and `+`
 - #37 - add `sync_complete` callback, triggered on `sync_from` completion with changes.
 - #41 - add `summary` API for Diff and DiffElement objects.

diffsync/__init__.py

@@ -214,11 +214,11 @@ def str(self, include_children: bool = True, indent: int = 0) -> str:
                 output += f": {child_ids}"
             else:
                 for child_id in child_ids:
-                    child = self.diffsync.get(modelname, child_id)
-                    if not child:
-                        output += f"\n{margin}    {child_id} (details unavailable)"
-                    else:
+                    try:
+                        child = self.diffsync.get(modelname, child_id)
                         output += "\n" + child.str(include_children=include_children, indent=indent + 4)
+                    except ObjectNotFound:
+                        output += f"\n{margin}    {child_id} (ERROR: details unavailable)"
         return output
 
     @classmethod
@@ -428,6 +428,13 @@ def __init_subclass__(cls):
                     f'Incorrect field name - {value.__name__} has type name "{value.get_type()}", not "{name}"'
                 )
 
+        for name in cls.top_level:
+            if not hasattr(cls, name):
+                raise AttributeError(f'top_level references attribute "{name}" but it is not a class attribute!')
+            value = getattr(cls, name)
+            if not isclass(value) or not issubclass(value, DiffSyncModel):
+                raise AttributeError(f'top_level references attribute "{name}" but it is not a DiffSyncModel subclass!')
+
     def __str__(self):
         """String representation of a DiffSync."""
         if self.type != self.name:
@@ -455,6 +462,8 @@ def str(self, indent: int = 0) -> str:
         margin = " " * indent
         output = ""
         for modelname in self.top_level:
+            if output:
+                output += "\n"
             output += f"{margin}{modelname}"
             models = self.get_all(modelname)
             if not models:
@@ -539,7 +548,10 @@ def _sync_from_diff_element(
         # So let's live with the slightly too high number of branches (14/12) for now.
         log = logger or self._log
         object_class = getattr(self, element.type)
-        obj = self.get(object_class, element.keys)
+        try:
+            obj: Optional[DiffSyncModel] = self.get(object_class, element.keys)
+        except ObjectNotFound:
+            obj = None
         # Get the attributes that actually differ between source and dest
         diffs = element.get_attrs_diffs()
         log = log.bind(
@@ -634,12 +646,16 @@ def diff_to(
 
     def get(
         self, obj: Union[Text, DiffSyncModel, Type[DiffSyncModel]], identifier: Union[Text, Mapping]
-    ) -> Optional[DiffSyncModel]:
+    ) -> DiffSyncModel:
         """Get one object from the data store based on its unique id.
 
         Args:
             obj: DiffSyncModel class or instance, or modelname string, that defines the type of the object to retrieve
             identifier: Unique ID of the object to retrieve, or dict of unique identifier keys/values
+
+        Raises:
+            ValueError: if obj is a str and identifier is a dict (can't convert dict into a uid str without a model class)
+            ObjectNotFound: if the requested object is not present
         """
         if isinstance(obj, str):
             modelname = obj
@@ -656,13 +672,14 @@ def get(
         elif object_class:
             uid = object_class.create_unique_id(**identifier)
         else:
-            self._log.warning(
-                f"Tried to look up a {modelname} by identifier {identifier}, "
-                "but don't know how to convert that to a uid string",
+            raise ValueError(
+                f"Invalid args: ({obj}, {identifier}): "
+                f"either {obj} should be a class/instance or {identifier} should be a str"
             )
-            return None
 
-        return self._data[modelname].get(uid)
+        if uid not in self._data[modelname]:
+            raise ObjectNotFound(f"{modelname} {uid} not present in {self.name}")
+        return self._data[modelname][uid]
 
     def get_all(self, obj: Union[Text, DiffSyncModel, Type[DiffSyncModel]]):
         """Get all objects of a given type.
@@ -688,17 +705,20 @@ def get_by_uids(
         Args:
             uids: List of unique id / key identifying object in the database.
             obj: DiffSyncModel class or instance, or modelname string, that defines the type of the objects to retrieve
+
+        Raises:
+            ObjectNotFound: if any of the requested UIDs are not found in the store
         """
         if isinstance(obj, str):
             modelname = obj
         else:
             modelname = obj.get_type()
 
-        # TODO: should this raise an exception if any or all of the uids are not found?
         results = []
         for uid in uids:
-            if uid in self._data[modelname]:
-                results.append(self._data[modelname][uid])
+            if uid not in self._data[modelname]:
+                raise ObjectNotFound(f"{modelname} {uid} not present in {self.name}")
+            results.append(self._data[modelname][uid])
         return results
 
     def add(self, obj: DiffSyncModel):
@@ -735,7 +755,7 @@ def remove(self, obj: DiffSyncModel, remove_children: bool = False):
         uid = obj.get_unique_id()
 
         if uid not in self._data[modelname]:
-            raise ObjectNotFound(f"Object {uid} not present")
+            raise ObjectNotFound(f"{modelname} {uid} not present in {self.name}")
 
         if obj.diffsync is self:
             obj.diffsync = None
@@ -745,9 +765,12 @@ def remove(self, obj: DiffSyncModel, remove_children: bool = False):
         if remove_children:
             for child_type, child_fieldname in obj.get_children_mapping().items():
                 for child_id in getattr(obj, child_fieldname):
-                    child_obj = self.get(child_type, child_id)
-                    if child_obj:
+                    try:
+                        child_obj = self.get(child_type, child_id)
                         self.remove(child_obj, remove_children=remove_children)
+                    except ObjectNotFound:
+                        # Since this is "cleanup" code, log an error and continue, instead of letting the exception raise
+                        self._log.error(f"Unable to remove child {child_id} of {modelname} {uid} - not found!")
 
 
 # DiffSyncModel references DiffSync and DiffSync references DiffSyncModel. Break the typing loop:

tests/unit/conftest.py

@@ -140,14 +140,24 @@ def generic_diffsync():
     return DiffSync()
 
 
+class UnusedModel(DiffSyncModel):
+    """Concrete DiffSyncModel subclass that can be referenced as a class attribute but never has any data."""
+
+    _modelname = "unused"
+    _identifiers = ("name",)
+
+    name: str
+
+
 class GenericBackend(DiffSync):
     """An example semi-abstract subclass of DiffSync."""
 
     site = Site  # to be overridden by subclasses
     device = Device
     interface = Interface
+    unused = UnusedModel
 
-    top_level = ["site"]
+    top_level = ["site", "unused"]
 
     DATA: dict = {}
 

tests/unit/test_diffsync.py

@@ -56,19 +56,25 @@ def test_diffsync_sync_self_with_no_data_is_noop(generic_diffsync):
     assert not generic_diffsync.sync_complete.called
 
 
-def test_diffsync_get_with_no_data_is_none(generic_diffsync):
-    assert generic_diffsync.get("anything", "myname") is None
-    assert generic_diffsync.get(DiffSyncModel, "") is None
+def test_diffsync_get_with_no_data_fails(generic_diffsync):
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get("anything", "myname")
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get(DiffSyncModel, "")
 
 
 def test_diffsync_get_all_with_no_data_is_empty_list(generic_diffsync):
     assert list(generic_diffsync.get_all("anything")) == []
     assert list(generic_diffsync.get_all(DiffSyncModel)) == []
 
 
-def test_diffsync_get_by_uids_with_no_data_is_empty_list(generic_diffsync):
-    assert generic_diffsync.get_by_uids(["any", "another"], "anything") == []
-    assert generic_diffsync.get_by_uids(["any", "another"], DiffSyncModel) == []
+def test_diffsync_get_by_uids_with_no_data(generic_diffsync):
+    assert generic_diffsync.get_by_uids([], "anything") == []
+    assert generic_diffsync.get_by_uids([], DiffSyncModel) == []
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get_by_uids(["any", "another"], "anything")
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get_by_uids(["any", "another"], DiffSyncModel)
 
 
 def test_diffsync_add(generic_diffsync, generic_diffsync_model):
@@ -83,12 +89,15 @@ def test_diffsync_get_with_generic_model(generic_diffsync, generic_diffsync_mode
     # The generic_diffsync_model has an empty identifier/unique-id
     assert generic_diffsync.get(DiffSyncModel, "") == generic_diffsync_model
     assert generic_diffsync.get(DiffSyncModel.get_type(), "") == generic_diffsync_model
-    # DiffSync doesn't know how to construct a uid str for a "diffsyncmodel"
-    assert generic_diffsync.get(DiffSyncModel.get_type(), {}) is None
+    # DiffSync doesn't know how to construct a uid str for a "diffsyncmodel" (it needs the class or instance, not a str)
+    with pytest.raises(ValueError):
+        generic_diffsync.get(DiffSyncModel.get_type(), {})
     # Wrong object-type - no match
-    assert generic_diffsync.get("", "") is None
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get("", "")
     # Wrong unique-id - no match
-    assert generic_diffsync.get(DiffSyncModel, "myname") is None
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get(DiffSyncModel, "myname")
 
 
 def test_diffsync_get_all_with_generic_model(generic_diffsync, generic_diffsync_model):
@@ -104,9 +113,11 @@ def test_diffsync_get_by_uids_with_generic_model(generic_diffsync, generic_diffs
     assert generic_diffsync.get_by_uids([""], DiffSyncModel) == [generic_diffsync_model]
     assert generic_diffsync.get_by_uids([""], DiffSyncModel.get_type()) == [generic_diffsync_model]
     # Wrong unique-id - no match
-    assert generic_diffsync.get_by_uids(["myname"], DiffSyncModel) == []
-    # Valid unique-id mixed in with unknown ones - return the successful matches?
-    assert generic_diffsync.get_by_uids(["aname", "", "anothername"], DiffSyncModel) == [generic_diffsync_model]
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get_by_uids(["myname"], DiffSyncModel)
+    # Valid unique-id mixed in with unknown ones
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get_by_uids(["aname", "", "anothername"], DiffSyncModel)
 
 
 def test_diffsync_remove_with_generic_model(generic_diffsync, generic_diffsync_model):
@@ -115,18 +126,19 @@ def test_diffsync_remove_with_generic_model(generic_diffsync, generic_diffsync_m
     with pytest.raises(ObjectNotFound):
         generic_diffsync.remove(generic_diffsync_model)
 
-    assert generic_diffsync.get(DiffSyncModel, "") is None
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get(DiffSyncModel, "")
     assert list(generic_diffsync.get_all(DiffSyncModel)) == []
-    assert generic_diffsync.get_by_uids([""], DiffSyncModel) == []
+    with pytest.raises(ObjectNotFound):
+        generic_diffsync.get_by_uids([""], DiffSyncModel)
 
 
-def test_diffsync_subclass_validation():
-    """Test the declaration-time checks on a DiffSync subclass."""
+def test_diffsync_subclass_validation_name_mismatch():
     # pylint: disable=unused-variable
     with pytest.raises(AttributeError) as excinfo:
 
         class BadElementName(DiffSync):
-            """Model with a DiffSyncModel attribute whose name does not match the modelname."""
+            """DiffSync with a DiffSyncModel attribute whose name does not match the modelname."""
 
             dev_class = Device  # should be device = Device
 
@@ -135,6 +147,35 @@ class BadElementName(DiffSync):
     assert "dev_class" in str(excinfo.value)
 
 
+def test_diffsync_subclass_validation_missing_top_level():
+    # pylint: disable=unused-variable
+    with pytest.raises(AttributeError) as excinfo:
+
+        class MissingTopLevel(DiffSync):
+            """DiffSync whose top_level references an attribute that does not exist on the class."""
+
+            top_level = ["missing"]
+
+    assert "top_level" in str(excinfo.value)
+    assert "missing" in str(excinfo.value)
+    assert "is not a class attribute" in str(excinfo.value)
+
+
+def test_diffsync_subclass_validation_top_level_not_diffsyncmodel():
+    # pylint: disable=unused-variable
+    with pytest.raises(AttributeError) as excinfo:
+
+        class TopLevelNotDiffSyncModel(DiffSync):
+            """DiffSync whose top_level references an attribute that is not a DiffSyncModel subclass."""
+
+            age = 0
+            top_level = ["age"]
+
+    assert "top_level" in str(excinfo.value)
+    assert "age" in str(excinfo.value)
+    assert "is not a DiffSyncModel" in str(excinfo.value)
+
+
 def test_diffsync_dict_with_data(backend_a):
     assert backend_a.dict() == {
         "device": {
@@ -238,7 +279,9 @@ def test_diffsync_str_with_data(backend_a):
           interface: rdu-spine2__eth0: {'interface_type': 'ethernet', 'description': 'Interface 0'}
           interface: rdu-spine2__eth1: {'interface_type': 'ethernet', 'description': 'Interface 1'}
     people
-      person: Glenn Matthews: {}"""
+      person: Glenn Matthews: {}
+unused: []\
+"""
     )
 
 
@@ -308,17 +351,21 @@ def test_diffsync_sync_from(backend_a, backend_b):
     site_atl_a = backend_a.get("site", "atl")
     assert isinstance(site_atl_a, Site)
     assert site_atl_a.name == "atl"
-    assert backend_a.get(Site, "rdu") is None
-    assert backend_a.get("nothing", "") is None
+    with pytest.raises(ObjectNotFound):
+        backend_a.get(Site, "rdu")
+    with pytest.raises(ObjectNotFound):
+        backend_a.get("nothing", "")
 
     assert list(backend_a.get_all(Site)) == [site_nyc_a, site_sfo_a, site_atl_a]
     assert list(backend_a.get_all("site")) == [site_nyc_a, site_sfo_a, site_atl_a]
     assert list(backend_a.get_all("nothing")) == []
 
     assert backend_a.get_by_uids(["nyc", "sfo"], Site) == [site_nyc_a, site_sfo_a]
     assert backend_a.get_by_uids(["sfo", "nyc"], "site") == [site_sfo_a, site_nyc_a]
-    assert backend_a.get_by_uids(["nyc", "sfo"], Device) == []
-    assert backend_a.get_by_uids(["nyc", "sfo"], "device") == []
+    with pytest.raises(ObjectNotFound):
+        backend_a.get_by_uids(["nyc", "sfo"], Device)
+    with pytest.raises(ObjectNotFound):
+        backend_a.get_by_uids(["nyc", "sfo"], "device")
 
 
 def test_diffsync_subclass_default_name_type(backend_a):
@@ -354,6 +401,18 @@ def test_diffsync_add_get_remove_with_subclass_and_data(backend_a):
         backend_a.remove(site_atl_a)
 
 
+def test_diffsync_remove_missing_child(log, backend_a):
+    rdu_spine1 = backend_a.get(Device, "rdu-spine1")
+    rdu_spine1_eth0 = backend_a.get(Interface, "rdu-spine1__eth0")
+    # Usage error - remove rdu_spine1_eth0 from backend_a, but rdu_spine1 still has a reference to it
+    backend_a.remove(rdu_spine1_eth0)
+    # Should log an error but continue removing other child objects
+    backend_a.remove(rdu_spine1, remove_children=True)
+    assert log.has("Unable to remove child rdu-spine1__eth0 of device rdu-spine1 - not found!", diffsync=backend_a)
+    with pytest.raises(ObjectNotFound):
+        backend_a.get(Interface, "rdu-spine1__eth1")
+
+
 def test_diffsync_sync_from_exceptions_are_not_caught_by_default(error_prone_backend_a, backend_b):
     with pytest.raises(ObjectCrudException):
         error_prone_backend_a.sync_from(backend_b)
@@ -430,8 +489,10 @@ def test_diffsync_diff_with_skip_unmatched_both_flag(
 def test_diffsync_sync_with_skip_unmatched_src_flag(backend_a, backend_a_with_extra_models):
     backend_a.sync_from(backend_a_with_extra_models, flags=DiffSyncFlags.SKIP_UNMATCHED_SRC)
     # New objects should not have been created
-    assert backend_a.get(backend_a.site, "lax") is None
-    assert backend_a.get(backend_a.device, "nyc-spine3") is None
+    with pytest.raises(ObjectNotFound):
+        backend_a.get(backend_a.site, "lax")
+    with pytest.raises(ObjectNotFound):
+        backend_a.get(backend_a.device, "nyc-spine3")
     assert "nyc-spine3" not in backend_a.get(backend_a.site, "nyc").devices
 
 
@@ -491,7 +552,8 @@ class NoDeleteInterfaceDiffSync(BackendA):
     # NoDeleteInterface.delete() should not be called since we're deleting its parent only
     extra_models.sync_from(backend_a)
     # The extra interface should have been removed from the DiffSync without calling its delete() method
-    assert extra_models.get(extra_models.interface, extra_interface.get_unique_id()) is None
+    with pytest.raises(ObjectNotFound):
+        extra_models.get(extra_models.interface, extra_interface.get_unique_id())
     # The sync should be complete, regardless
     diff = extra_models.diff_from(backend_a)
     print(diff.str())  # for debugging of any failure

tests/unit/test_diffsync_model.py

@@ -195,7 +195,7 @@ def test_diffsync_model_str_with_children(generic_diffsync, make_site, make_devi
   devices
     device: device1: {'role': 'default'}
       interfaces
-        device1__eth0 (details unavailable)\
+        device1__eth0 (ERROR: details unavailable)\
 """
     )