Improving the compare_entities functions

Summary

I reviewed the compare_entities function from apiutils and identified some problematic cases (e.g. multi-properties). Corresponding unit tests have been created and the functionality repaired.

This is a preparation for a comprehensive review of the merge_entities function. I think it's a good idea to merge this in a first step as it's probably independent from the review of the merge function.

Focus

This one can probably be reviewed line-by-line. Maybe look for missing test-corner-cases that might be needed. The code for checking the value for equality is still not ideal, but I think a more comprehensive review (also adding the recursive reference checker) should be postponed.

Test Environment

There are unit tests.

Check List for the Author

Please, prepare your MR for a review. Be sure to write a summary and a focus and create gitlab comments for the reviewer. They should guide the reviewer through the changes, explain your changes and also point out open questions. For further good practices have a look at our review guidelines

• 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)
  • How do I use the software? Assume "stupid" users.
  • How do I develop or debug the software? Assume novice developers.
• Annotations in code (Gitlab comments)
  • Intent of new code
  • Problems with old code
  • Why this implementation?

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?

For further good practices have a look at our review guidelines.

src/caosdb/apiutils.py

@@ -195,7 +195,8 @@ def getCommitIn(folder):
         return t.readline().strip()
 
 
-def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_records: bool = False):
+def compare_entities(old_entity: Entity, new_entity: Entity,
+                     compare_referenced_records: bool = False):
     """Compare two entites.
 
     Return a tuple of dictionaries, the first index belongs to additional information for old
@@ -207,7 +208,7 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
       - Each property lists either an additional property or a property with a changed:
         - datatype
         - importance or
-        - value (not implemented yet)
+        - value
 
         In case of changed information the value listed under the respective key shows the
         value that is stored in the respective entity.
@@ -217,6 +218,9 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
     `compare_referenced_records = False` to prevent infinite recursion in case
     of circular references).
 
+    NOTE: This function does not work for abstract properties! I.e. it is not possible
+          to directly compare two entities that are of class caosdb.Property.
+
     Parameters
     ----------
     old_entity, new_entity : Entity
@@ -229,6 +233,12 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
         identical records are stored in different objects. Default is False.
 
     """
+
+    for entity in (old_entity, new_entity):
+        if isinstance(entity, Property):
+            raise NotImplementedError("The function compare_entities does not work for "
+                                      "comparing abstract properties.")
+    
     olddiff: Dict[str, Any] = {"properties": {}, "parents": []}
     newdiff: Dict[str, Any] = {"properties": {}, "parents": []}
 
@@ -256,7 +266,7 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
         if not old_entity_attr_exists and not new_entity_attr_exists:
             continue
 
-        if ((old_entity_attr_exists ^ new_entity_attr_exists)
+        if ((old_entity_attr_exists != new_entity_attr_exists)
                 or (oldattr != newattr)):
 
             if old_entity_attr_exists:
@@ -268,8 +278,16 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
     # properties
 
     for prop in old_entity.properties:
+        # Find the corresponding property in new_entity:
         matching = [p for p in new_entity.properties if p.name == prop.name]
 
+        # This is needed for checking for multi properties in old_entity:
+        # TODO: is there a better way?
+        matching_old = [p for p in old_entity.properties if p.name == prop.name]
+        if len(matching_old) != 1:
+            raise NotImplementedError(
+                "Comparison not implemented for multi-properties.")
+
         if len(matching) == 0:
             olddiff["properties"][prop.name] = {}
         elif len(matching) == 1:
@@ -300,6 +318,7 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
                     # scalar reference
                     if isinstance(prop.value, Entity) and isinstance(matching[0].value, Entity):
                         # explicitely not recursive to prevent infinite recursion
+                        # TODO: why not use a recursion detection with a cache?
                         same_value = empty_diff(
                             prop.value, matching[0].value, compare_referenced_records=False)
                     # list of references