Revert "Merge branch 'revert-5ce8e910' into 'dev'"

This reverts commit 5d81222a, reversing
changes made to 5ce8e910.

CHANGELOG.md

@@ -22,7 +22,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Removed ###
 
 ### Fixed ###
+
 - Fixed `src/caosdb/utils/checkFileSystemConsistency.py`
+- `compare_entities` now corretly raises a `NotImplementedError` in case of
+  comparing two `Property` entities or in case of entities with multi
+  properties.
 
 ### Security ###
 

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] = {}
 

unittests/test_apiutils.py

@@ -101,14 +101,15 @@ def test_compare_entities():
     r2 = db.Record()
     r1.add_parent("bla")
     r2.add_parent("bla")
-    r1.add_parent("lopp")
+    r1.add_parent("lopp")  # r1 has one additional parent
     r1.add_property("test", value=2)
-    r2.add_property("test", value=2)
+    r2.add_property("test", value=2)  # a property with the same value
     r1.add_property("tests", value=3)
-    r2.add_property("tests", value=45)
+    r2.add_property("tests", value=45)  # a property with different value
     r1.add_property("tester", value=3)
-    r2.add_property("tester", )
+    r2.add_property("tester", )  # a property where r2 has no value
     r1.add_property("tests_234234", value=45)
+    # one additional property that the other one does not have for both
     r2.add_property("tests_TT", value=45)
 
     diff_r1, diff_r2 = compare_entities(r1, r2)
@@ -130,6 +131,33 @@ def test_compare_entities():
     assert "tests_234234" in diff_r1["properties"]
     assert "tests_TT" in diff_r2["properties"]
 
+    # Check the value:
+    assert diff_r1["properties"]["tests"]["value"] == 3
+    assert diff_r2["properties"]["tests"]["value"] == 45
+
+
+def test_compare_equality():
+    r1 = db.Record()
+    r2 = db.Record()
+    diff_r1, diff_r2 = compare_entities(r1, r2)
+    for i in ("parents", "properties"):
+        assert len(diff_r1[i]) == 0
+        assert len(diff_r2[i]) == 0
+    assert empty_diff(r1, r2)
+
+    r1.add_parent("bla")
+    r2.add_parent("bla")
+    assert empty_diff(r1, r2)
+
+    r1.add_property("test", value=2)
+    r2.add_property("test", value=2)  # a property with the same value
+    diff_r1, diff_r2 = compare_entities(r1, r2)
+    assert empty_diff(r1, r2)
+
+    r1.add_parent("blobb")
+    r2.add_parent("blobb")
+    assert empty_diff(r1, r2)
+
 
 def test_compare_entities_units():
     r1 = db.Record()
@@ -165,6 +193,10 @@ def test_compare_entities_units():
     assert diff_r1["properties"]["test"]["unit"] == "cm"
     assert diff_r2["properties"]["test"]["unit"] == "m"
 
+    # Check the value:
+    assert diff_r1["properties"]["tests"]["value"] == 3
+    assert diff_r2["properties"]["tests"]["value"] == 45
+
 
 def test_compare_special_properties():
     # Test for all known special properties:
@@ -212,46 +244,114 @@ def test_compare_special_properties():
         assert len(diff_r2["properties"]) == 0
 
 
-@pytest.mark.xfail
+def test_compare_importances():
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=2, unit="cm", importance="SUGGESTED")
+    r2.add_property("test", value=2, unit="cm")
+    assert not empty_diff(r1, r2)
+    diff_r1, diff_r2 = compare_entities(r1, r2)
+    assert diff_r1["properties"]["test"]["importance"] == "SUGGESTED"
+    assert diff_r2["properties"]["test"]["importance"] == "FIX"
+    for diff in (diff_r1, diff_r2):
+        assert len(diff["properties"]["test"]) == 1
+        assert len(diff["parents"]) == 0
+
+
 def test_compare_properties():
     p1 = db.Property()
     p2 = db.Property()
 
-    diff_r1, diff_r2 = compare_entities(p1, p2)
-    assert len(diff_r1["parents"]) == 0
-    assert len(diff_r2["parents"]) == 0
-    assert len(diff_r1["properties"]) == 0
-    assert len(diff_r2["properties"]) == 0
+    with pytest.raises(NotImplementedError, match=".*abstract properties.*"):
+        compare_entities(p1, p2)
 
-    p1.importance = "SUGGESTED"
-    diff_r1, diff_r2 = compare_entities(p1, p2)
-    assert len(diff_r1["parents"]) == 0
-    assert len(diff_r2["parents"]) == 0
-    assert len(diff_r1["properties"]) == 0
-    assert len(diff_r2["properties"]) == 0
-    assert "importance" in diff_r1
-    assert diff_r1["importance"] == "SUGGESTED"
 
-    # TODO: I'm not sure why it is not like this:
-    # assert diff_r2["importance"] is None
-    # ... but:
-    assert "importance" not in diff_r2
+def test_multi_properties():
+    # This test is rather lengthy, because:
+    # - previously the check for multi-properties was only implemented for the
+    #   new_entity paramter of the function.
+    # - Because of the API of pylib the behavior depended on the order of adding the
+    #   properties to the records.
+
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=2)
+    r1.add_property("test", value=4)
+    r2.add_property("test", value=2)
+    # That would be expected:
+    # assert not empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
+
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=4)
+    r1.add_property("test", value=2)
+    r2.add_property("test", value=2)
+    # That would be expected:
+    # assert not empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
 
-    p2.importance = "SUGGESTED"
-    p1.value = 42
-    p2.value = 4
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=4)
+    r1.add_property("test", value=2)
+    r2.add_property("test", value=2)
+    r2.add_property("test", value=4)
+    # That would be expected:
+    # assert empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
 
-    diff_r1, diff_r2 = compare_entities(p1, p2)
-    assert len(diff_r1["parents"]) == 0
-    assert len(diff_r2["parents"]) == 0
-    assert len(diff_r1["properties"]) == 0
-    assert len(diff_r2["properties"]) == 0
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=4)
+    r2.add_property("test", value=4)
+    r2.add_property("test", value=2)
+    # That would be expected:
+    # assert not empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
+
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=4)
+    r2.add_property("test", value=2)
+    r2.add_property("test", value=4)
+    # That would be expected:
+    # assert not empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
 
-    # Comparing values currently does not seem to be implemented:
-    assert "value" in diff_r1
-    assert diff_r1["value"] == 42
-    assert "value" in diff_r2
-    assert diff_r2["value"] == 4
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=4)
+    r1.add_property("test", value=2)
+    r2.add_property("test", value=2)
+    r2.add_property("test", value=5)
+    # That would be expected:
+    # assert empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
+
+    r1 = db.Record()
+    r2 = db.Record()
+    r1.add_property("test", value=4)
+    r1.add_property("test", value=2)
+    # That would be expected:
+    # assert empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
+
+    r1 = db.Record()
+    r2 = db.Record()
+    r2.add_property("test", value=2)
+    r2.add_property("test", value=5)
+    # That would be expected:
+    # assert empty_diff(r1, r2)
+    with pytest.raises(NotImplementedError, match=".*multi-properties.*"):
+        compare_entities(r1, r2)
 
 
 def test_copy_entities():