Merge branch 'f-get-parents-recursively' into 'dev'

Get parents recursively

See merge request !94

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added ###
 
 ### Changed ###
+- Renamed `_Parents` to `_ParentList`.
 
 ### Deprecated ###
 
@@ -17,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed ###
 
+* [caosdb-pylib#90](https://gitlab.com/caosdb/caosdb-pylib/-/issues/90): `Entity.get_parents_recursively()` did not work for unretrieved parents.
+
 ### Security ###
 
 ### Documentation ###

src/caosdb/common/models.py

@@ -34,6 +34,7 @@ All additional classes are either important for the entities or the
 transactions.
 """
 from __future__ import print_function, unicode_literals
+from __future__ import annotations  # Can be removed with 3.10.
 
 import re
 import sys
@@ -82,7 +83,7 @@ SPECIAL_ATTRIBUTES = ["name", "role", "datatype", "description",
                       "id", "path", "checksum", "size"]
 
 
-class Entity(object):
+class Entity:
 
     """Entity is a generic CaosDB object.
 
@@ -101,6 +102,8 @@ class Entity(object):
         self._checksum = None
         self._size = None
         self._upload = None
+        # If an entity is used (e.g. as parent), it is wrapped instead of being used directly.
+        # see Entity._wrap()
         self._wrapped_entity = None
         self._version = None
         self._cuid = None
@@ -111,7 +114,7 @@ class Entity(object):
         self.value = value
         self.messages = _Messages()
         self.properties = _Properties()
-        self.parents = _Parents()
+        self.parents = _ParentList()
         self.path = None
         self.file = None
         self.unit = None
@@ -530,7 +533,7 @@ class Entity(object):
                                 value=value, unit=unit)
 
         if abstract_property is not None:
-            new_property._wrap(property)
+            new_property._wrap(abstract_property)
 
             # FIXME: this really necessary?
 
@@ -621,24 +624,44 @@ class Entity(object):
 
         return self
 
-    def has_parent(self, parent, recursive=True,
-                   check_name=True, check_id=False):
-        """Checks if this entity has a given parent.
+    def has_parent(self, parent: Entity, recursive: bool = True, retrieve: bool = True,
+                   check_name: bool = True, check_id: bool = False):
+        """Check if this entity has a given parent.
 
         If 'check_name' and 'check_id' are both False, test for identity
         on the Python level. Otherwise use the name and/or ID for the
         check. Note that, if checked, name or ID should not be None,
         lest the check fail.
 
-        @param parent: Check for this parent.
-        @param recursive: Whether to check recursively.
-        @param check_name: Whether to use the name for ancestry check.
-        @param check_id: Whether to use the ID for ancestry check.
-        @return: True if 'parent' is a true parent, False otherwise.
+Parameters
+----------
+
+parent: Entity
+  Check for this parent.
+
+recursive: bool, optional
+  Whether to check recursively.
+
+check_name: bool, optional
+  Whether to use the name for ancestry check.
+
+check_id: bool, optional
+  Whether to use the ID for ancestry check.
+
+retrieve: bool, optional
+  If False, do not retrieve parents from the server.
+
+Returns
+-------
+out: bool
+  True if ``parent`` is a true parent, False otherwise.
 """
 
         if recursive:
-            parents = self.get_parents_recursively()
+            parents = self.get_parents_recursively(retrieve=retrieve)
+        else:
+            if retrieve:
+                parents = [pp.retrieve()._wrapped_entity for pp in self.parents]
             else:
                 parents = [pp._wrapped_entity for pp in self.parents]
 
@@ -659,39 +682,61 @@ class Entity(object):
     def get_parents(self):
         """Get all parents of this entity.
 
-        @return: _Parents(list)
+        @return: _ParentList(list)
         """
 
         return self.parents
 
-    def get_parents_recursively(self):
+    def get_parents_recursively(self, retrieve: bool = True):
         """Get all ancestors of this entity.
 
-        @return: list of Entities
+Parameters
+----------
+
+retrieve: bool, optional
+  If False, do not retrieve parents from the server.
+
+Returns
+-------
+out: List[Entity]
+  The parents of this Entity
 """
 
-        all_parents = _Parents()
-        self._get_parent_recursively(all_parents)
+        all_parents = []
+        self._get_parent_recursively(all_parents, retrieve=retrieve)
 
         return all_parents
 
-    def _get_parent_recursively(self, all_parents):
+    def _get_parent_recursively(self, all_parents: list, retrieve: bool = True):
         """Get all ancestors with a little helper.
 
         As a side effect of this method, the ancestors are added to
         all_parents.
 
-        @param all_parents: The added parents so far.
+        @param all_parents: list, The added parents so far.
 
         @return: None, but see side effects.
         """
 
         for parent in self.parents:
+            # TODO:
+            # Comment on _wrap and _wrapped_entity
+            # Currently, I (henrik) do not why the wrapping is necessary (and it is not
+            # documented). However, the following illustrates, why I think, it is a bad idea.
+            # First you add a parent with rec.add_parent(parent), but then you cannot access
+            # attributes of parent when you use rec.parents[0] for example becasue you do not get
+            # the same object but a wrapping object and you need to know that you only get the
+            # original by accessing the private (!) _wrapped_entity object.
             w_parent = parent._wrapped_entity
+            if retrieve:
+                parent.retrieve()
+                for next_parent in parent.parents:
+                    w_parent.add_parent(next_parent)
 
-            if w_parent not in all_parents:
+            if (w_parent.id, w_parent.name) not in [
+                    (all_p.id, all_p.name) for all_p in all_parents]:
                 all_parents.append(w_parent)
-                w_parent._get_parent_recursively(all_parents)
+                w_parent._get_parent_recursively(all_parents, retrieve=retrieve)
 
     def get_parent(self, key):
         """Return the first parent matching the key or None if no match exists.
@@ -1135,7 +1180,7 @@ class Entity(object):
             else:
                 raise TypeError(
                     'Child was neither a Property, nor a Parent, nor a Message.\
-                    Was ' + str(type(child)))
+                    Was ' + str(type(child)) + "\n" + str(child))
 
         # add VALUE
         value = None
@@ -1296,6 +1341,12 @@ class Entity(object):
             flags=flags)[0]
 
     def _wrap(self, entity):
+        """
+        When entity shall be used as parent or property it is not added to the corresponding list
+        (such as the parent list) directly, but another Entity object is created and the original
+        Entity is wrapped using this function
+        TODO: document here and in dev docs why this is done.
+        """
         self._wrapped_entity = entity
 
         return self
@@ -2094,7 +2145,8 @@ class _Properties(list):
         raise KeyError(str(prop) + " not found.")
 
 
-class _Parents(list):
+class _ParentList(list):
+    # TODO unclear why this class is private. Isn't it use full for users?
 
     def _get_entity_by_cuid(self, cuid):
         '''
@@ -2697,9 +2749,11 @@ class Container(list):
         elif isinstance(entity, QueryTemplate):
             super().append(entity)
         else:
-            raise TypeError(
-                "Entity was neither an id nor a name nor an entity." +
-                " (was " + str(type(entity)) + ")")
+            warn("Entity was neither an id nor a name nor an entity." +
+                 " (was " + str(type(entity)) + ":\n" + str(entity) + ")")
+            # raise TypeError(
+            #     "Entity was neither an id nor a name nor an entity." +
+            #     " (was " + str(type(entity)) + "\n" + str(entity) + ")")
 
         return self
 
@@ -3647,6 +3701,7 @@ class Container(list):
             for p in e.get_properties():
                 if p.id is None:
                     if p.name is not None:
+                        # TODO using try except for normal execution flow is bad style
                         try:
                             w = self.get_entity_by_name(p.name)
                             p._wrap(w)
@@ -3658,6 +3713,7 @@ class Container(list):
             for p in e.get_parents():
                 if p.id is None:
                     if p.name is not None:
+                        # TODO using try except for normal execution flow is bad style
                         try:
                             p._wrap(self.get_entity_by_name(p.name))
                         except KeyError: