DRAFT: Improve the compare_entities function

Summary

See #197. After the premature merge and following revert of !103 (merged), this MR re-introduces the changes.

TODO: Make these changes compatible to caosadvancedtools.

This MR has diverged very much from dev! It's probably easier to create a new MR with these changes based on the current dev branch.

The main problem is:

• caosadvancedtools relies on the function compare_entities to be able to compare abstract properties.
• This MR introduces NotImplementedErrors which are raised upon comparison of abstract properties.
• Implement support to compare abstract properties as a special case.

Focus

Point the reviewer to the core of the code change. Where should they start reading? What should they focus on (e.g. security, performance, maintainability, user-friendliness, compliance with the specs, finding more corner cases, concrete questions)?

Test Environment

Apart from unit tests, also check manually with caosdb crawler and advanced user tools.

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

@@ -178,7 +178,8 @@ def getCommitIn(folder):
     return get_commit_in(folder)
 
 
-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
@@ -189,8 +190,9 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
     - Information about properties:
       - Each property lists either an additional property or a property with a changed:
         - datatype
+        - unit
         - 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.
@@ -200,6 +202,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
@@ -212,6 +217,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": []}
 
@@ -239,7 +250,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:
@@ -251,9 +262,21 @@ 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) > 1:
+            raise NotImplementedError(
+                "Comparison not implemented for multi-properties.")
+
         if len(matching) == 0:
+            # There is no matching property in new_entity:
             olddiff["properties"][prop.name] = {}
         elif len(matching) == 1:
             newdiff["properties"][prop.name] = {}
@@ -283,6 +306,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
@@ -306,11 +330,14 @@ def compare_entities(old_entity: Entity, new_entity: Entity, compare_referenced_
                 newdiff["properties"].pop(prop.name)
                 olddiff["properties"].pop(prop.name)
 
-        else:
+    # Check whether there are missing properties in old_entity, additionally
+    # check for multi-properties that are currently not supported:
+    for prop in new_entity.properties:
+        matching = [p for p in new_entity.properties if p.name == prop.name]
+        if len(matching) > 1:
             raise NotImplementedError(
                 "Comparison not implemented for multi-properties.")
 
-    for prop in new_entity.properties:
         if len([0 for p in old_entity.properties if p.name == prop.name]) == 0:
             newdiff["properties"][prop.name] = {}