Detect infinite recursion in Entity.to_xml

Summary

Addresses https://gitlab.indiscale.com/caosdb/customers/f-fit/management/-/issues/96 / https://gitlab.com/linkahead/linkahead-pylib/-/issues/73
When calling to_xml on an Entity that refers to itself, to_xml now detects the infinite recursion and stops after the first level of repeated information.

Focus

• What is the likelihood that a version refers to itself (predecessor / successor)? Do we need to add the same functionality to Version.to_xml?
• Does this change anything for Entities that could already successfully call to_xml before? Are there cases where this might break the communication with the server?

Test Environment

Manual Testing

Check List for the Author

• All automated tests pass
• Reference related issues
• Up-to-date CHANGELOG.md (or not necessary)
• Up-to-date JSON schema (or not necessary)
• Appropriate user and developer documentation (or not necessary)
• Annotations in code (Gitlab comments)

Check List for the Reviewer

• I understand the intent of this MR
• All automated tests pass
• Up-to-date CHANGELOG.md (or not necessary)
• Appropriate user and developer documentation (or not necessary)
• The test environment setup works and the intended behavior is reproducible in the test environment
• In-code documentation and comments are up-to-date.
• Check: Are there specifications? Are they satisfied?

src/linkahead/common/models.py

@@ -1232,6 +1232,7 @@ class Entity:
         xml: Optional[etree._Element] = None,
         add_properties: INHERITANCE = "ALL",
         local_serialization: bool = False,
+        visited_entities: Optional[list] = None
     ) -> etree._Element:
         """Generate an xml representation of this entity. If the parameter xml
         is given, all attributes, parents, properties, and messages of this
@@ -1239,14 +1240,25 @@ class Entity:
 
         Raise an error if xml is not a lxml.etree.Element
 
-        @param xml: an xml element to which all attributes, parents,
-            properties, and messages
-            are to be added.
-
-        FIXME: Add documentation for the add_properties parameter.
-        FIXME: Add docuemntation for the local_serialization parameter.
+        Parameters
+        ----------
+        xml : etree._Element, optional
+            an xml element to which all attributes, parents,
+            properties, and messages are to be added. Default is None.
+        visited_entities : list, optional
+            list of enties that are being printed for recursion check,
+            should never be set manually. Default is None.
+        add_properties : INHERITANCE, optional
+            FIXME: Add documentation for the add_properties
+            parameter. Default is "ALL".
+        local_serialization : bool, optional
+            FIXME: Add documentation for the local_serialization
+            parameter. Default is False.
 
-        @return: xml representation of this entity.
+        Returns
+        -------
+        xml : etree._Element
+            xml representation of this entity.
         """
 
         if xml is None:
@@ -1255,9 +1267,17 @@ class Entity:
             xml = etree.Element(elem_tag)
         assert isinstance(xml, etree._Element)
 
+        if visited_entities is None:
+            visited_entities = []
+        if self in visited_entities:
+            xml.text = xml2str(etree.Comment("Recursive reference"))
+            return xml
+        visited_entities.append(self)
+
         # unwrap wrapped entity
         if self._wrapped_entity is not None:
-            xml = self._wrapped_entity.to_xml(xml, add_properties)
+            xml = self._wrapped_entity.to_xml(xml, add_properties,
+                                              visited_entities=visited_entities.copy())
 
         if self.id is not None:
             xml.set("id", str(self.id))
@@ -1272,6 +1292,10 @@ class Entity:
             xml.set("description", str(self.description))
 
         if self.version is not None:
+            # If this ever causes problems, we might add
+            # visited_entities support here since it does have some
+            # recursion with predecessors / successors. But should be
+            # fine for now, since it is always set by the server.
             xml.append(self.version.to_xml())
 
         if self.value is not None:
@@ -1281,7 +1305,8 @@ class Entity:
                 elif self.value.name is not None:
                     xml.text = str(self.value.name)
                 else:
-                    xml.text = str(self.value)
+                    dt_str = xml2str(self.value.to_xml(visited_entities=visited_entities.copy()))
+                    xml.text = dt_str
             elif isinstance(self.value, list):
                 for v in self.value:
                     v_elem = etree.Element("Value")
@@ -1292,7 +1317,8 @@ class Entity:
                         elif v.name is not None:
                             v_elem.text = str(v.name)
                         else:
-                            v_elem.text = str(v)
+                            dt_str = xml2str(v.to_xml(visited_entities=visited_entities.copy()))
+                            v_elem.text = dt_str
                     elif v == "":
                         v_elem.append(etree.Element("EmptyString"))
                     elif v is None:
@@ -1314,7 +1340,10 @@ class Entity:
                 elif self.datatype.name is not None:
                     xml.set("datatype", str(self.datatype.name))
                 else:
-                    xml.set("datatype", str(self.datatype))
+                    dt_str = xml2str(self.datatype.to_xml(visited_entities=visited_entities.copy()))
+                    # Todo: Use for pretty-printing with calls from _repr_ only?
+                    # dt_str = dt_str.replace('<', 'ᐸ').replace('>', 'ᐳ').replace(' ', '⠀').replace('"', '\'').replace('\n', '')
+                    xml.set("datatype", dt_str)
             else:
                 xml.set("datatype", str(self.datatype))
 
@@ -1337,10 +1366,11 @@ class Entity:
             self.messages.to_xml(xml)
 
         if self.parents is not None:
-            self.parents.to_xml(xml)
+            self.parents.to_xml(xml, visited_entities=visited_entities.copy())
 
         if self.properties is not None:
-            self.properties.to_xml(xml, add_properties)
+            self.properties.to_xml(xml, add_properties,
+                                   visited_entities=visited_entities.copy())
 
         if len(self._flags) > 0:
             flagattr = ""
@@ -1948,11 +1978,16 @@ class Parent(Entity):
         xml: Optional[etree._Element] = None,
         add_properties: INHERITANCE = "NONE",
         local_serialization: bool = False,
+        visited_entities: Optional[Union[list, None]] = None,
     ):
         if xml is None:
             xml = etree.Element("Parent")
 
-        return super().to_xml(xml=xml, add_properties=add_properties)
+        if visited_entities is None:
+            visited_entities = []
+
+        return super().to_xml(xml=xml, add_properties=add_properties,
+                              visited_entities=visited_entities)
 
 
 class _EntityWrapper(object):
@@ -2023,14 +2058,19 @@ class Property(Entity):
         xml: Optional[etree._Element] = None,
         add_properties: INHERITANCE = "ALL",
         local_serialization: bool = False,
+        visited_entities: Optional[Union[list, None]] = None,
     ):
         if xml is None:
             xml = etree.Element("Property")
 
+        if visited_entities is None:
+            visited_entities = []
+
         return super(Property, self).to_xml(
             xml=xml,
             add_properties=add_properties,
             local_serialization=local_serialization,
+            visited_entities=visited_entities,
         )
 
     def is_reference(self, server_retrieval: bool = False) -> Optional[bool]:
@@ -2188,15 +2228,20 @@ class RecordType(Entity):
         xml: Optional[etree._Element] = None,
         add_properties: INHERITANCE = "ALL",
         local_serialization: bool = False,
+        visited_entities: Optional[Union[list, None]] = None,
     ) -> etree._Element:
         if xml is None:
             xml = etree.Element("RecordType")
 
+        if visited_entities is None:
+            visited_entities = []
+
         return Entity.to_xml(
             self,
             xml=xml,
             add_properties=add_properties,
             local_serialization=local_serialization,
+            visited_entities=visited_entities,
         )
 
 
@@ -2227,14 +2272,19 @@ class Record(Entity):
         xml: Optional[etree._Element] = None,
         add_properties: INHERITANCE = "ALL",
         local_serialization: bool = False,
+        visited_entities: Optional[Union[list, None]] = None,
     ):
         if xml is None:
             xml = etree.Element("Record")
 
+        if visited_entities is None:
+            visited_entities = []
+
         return super().to_xml(
             xml=xml,
             add_properties=add_properties,
             local_serialization=local_serialization,
+            visited_entities=visited_entities
         )
 
 
@@ -2304,6 +2354,7 @@ class File(Record):
         xml: Optional[etree._Element] = None,
         add_properties: INHERITANCE = "ALL",
         local_serialization: bool = False,
+        visited_entities: Optional[Union[list, None]] = None,
     ) -> etree._Element:
         """Convert this file to an xml element.
 
@@ -2313,8 +2364,12 @@ class File(Record):
         if xml is None:
             xml = etree.Element("File")
 
+        if visited_entities is None:
+            visited_entities = []
+
         return Entity.to_xml(self, xml=xml, add_properties=add_properties,
-                             local_serialization=local_serialization)
+                             local_serialization=local_serialization,
+                             visited_entities=visited_entities)
 
     def download(self, target: Optional[str] = None) -> str:
         """Download this file-entity's actual file from the file server. It
@@ -2476,15 +2531,20 @@ class PropertyList(list):
 
         return self
 
-    def to_xml(self, add_to_element: etree._Element, add_properties: INHERITANCE):
+    def to_xml(self, add_to_element: etree._Element, add_properties: INHERITANCE,
+               visited_entities: Optional[Union[list, None]] = None):
+
+        if visited_entities is None:
+            visited_entities = []
+
         p: Property
         for p in self:
             importance = self._importance.get(p)
 
             if add_properties == FIX and not importance == FIX:
                 continue
-
-            pelem = p.to_xml(xml=etree.Element("Property"), add_properties=FIX)
+            pelem = p.to_xml(xml=etree.Element("Property"), add_properties=FIX,
+                             visited_entities=visited_entities.copy())
 
             if p in self._importance:
                 pelem.set("importance", str(importance))
@@ -2631,7 +2691,12 @@ class ParentList(list):
 
         return self
 
-    def to_xml(self, add_to_element: etree._Element):
+    def to_xml(self, add_to_element: etree._Element,
+               visited_entities: Optional[Union[list, None]] = None):
+
+        if visited_entities is None:
+            visited_entities = []
+
         for p in self:
             pelem = etree.Element("Parent")