Improve the detection of schema changes and add tests

python/podio_schema_evolution.py

@@ -4,6 +4,7 @@
 """
 
 import yaml
+import sys
 
 from podio_gen.podio_config_reader import PodioConfigReader
 
@@ -123,6 +124,68 @@ def __init__(self, name, member_name_old, member_name_new):
         )
 
 
+class AddedVectorMember(SchemaChange):
+    """Class representing an added VectorMember"""
+
+    def __init__(self, member, datatype):
+        self.member = member
+        self.klassname = datatype
+        super().__init__(f"'{self.klassname}' has added a vector member '{self.member}'")
+
+
+class DroppedVectorMember(SchemaChange):
+    """Class representing a dropped VectorMember"""
+
+    def __init__(self, member, datatype):
+        self.member = member
+        self.klassname = datatype
+        super().__init__(f"'{self.klassname}' has a dropped member '{self.member.name}")
+
+
+class AddedSingleRelation(SchemaChange):
+    """Class representing an added OneToOneRelation"""
+
+    def __init__(self, member, datatype):
+        self.member = member
+        self.klassname = datatype
+        super().__init__(
+            f"'{self.klassname}' has added a one-to-one relation '{self.member.name}'"
+        )
+
+
+class DroppedSingleRelation(SchemaChange):
+    """Class representing a dropped OneToOneRelation"""
+
+    def __init__(self, member, datatype):
+        self.member = member
+        self.klassname = datatype
+        super().__init__(
+            f"'{self.klassname}' has dropped a one-to-one relation '{self.member.name}'"
+        )
+
+
+class AddedMultiRelation(SchemaChange):
+    """Class representing an added OneToManyRelation"""
+
+    def __init__(self, member, datatype):
+        self.member = member
+        self.klassname = datatype
+        super().__init__(
+            f"'{self.klassname}' has added a one-to-many relation '{self.member.name}'"
+        )
+
+
+class DroppedMultiRelation(SchemaChange):
+    """Class representing a dropped OneToManyRelation"""
+
+    def __init__(self, member, datatype):
+        self.member = member
+        self.klassname = datatype
+        super().__init__(
+            f"'{self.klassname}' has dropped a one-to-many relation '{self.member.name}'"
+        )
+
+
 class RootIoRule:
     """A placeholder IORule class"""
 
@@ -205,7 +268,7 @@ def _compare_components(self) -> None:
             ]
         )
 
-        self._compare_definitions(
+        self._compare_members(
             kept_components,
             self.datamodel_new.components,
             self.datamodel_old.components,
@@ -226,14 +289,49 @@ def _compare_datatypes(self) -> None:
             [DroppedDatatype(self.datamodel_old.datatypes[name], name) for name in dropped_types]
         )
 
-        self._compare_definitions(
+        self._compare_members(
             kept_types,
             self.datamodel_new.datatypes,
             self.datamodel_old.datatypes,
             "Members",
         )
 
-    def _compare_definitions(self, definitions, first, second, category) -> None:
+        self._compare_members(
+            kept_types,
+            self.datamodel_new.datatypes,
+            self.datamodel_old.datatypes,
+            "VectorMembers",
+            AddedVectorMember,
+            DroppedVectorMember,
+        )
+
+        self._compare_members(
+            kept_types,
+            self.datamodel_new.datatypes,
+            self.datamodel_old.datatypes,
+            "OneToOneRelations",
+            AddedSingleRelation,
+            DroppedSingleRelation,
+        )
+
+        self._compare_members(
+            kept_types,
+            self.datamodel_new.datatypes,
+            self.datamodel_old.datatypes,
+            "OneToManyRelations",
+            AddedMultiRelation,
+            DroppedMultiRelation,
+        )
+
+    def _compare_members(
+        self,
+        definitions,
+        first,
+        second,
+        category,
+        added_change=AddedMember,
+        dropped_change=DroppedMember,
+    ) -> None:
         """compare member definitions in old and new datamodel"""
         for name in definitions:
             # we are only interested in members not the extracode
@@ -244,10 +342,10 @@ def _compare_definitions(self, definitions, first, second, category) -> None:
             )
             # Make findings known globally
             self.detected_schema_changes.extend(
-                [AddedMember(members1[member], name) for member in added_members]
+                [added_change(members1[member], name) for member in added_members]
             )
             self.detected_schema_changes.extend(
-                [DroppedMember(members2[member], name) for member in dropped_members]
+                [dropped_change(members2[member], name) for member in dropped_members]
             )
 
             # now let's compare old and new for the kept members
@@ -327,6 +425,33 @@ def heuristics(self):
         added_members = [change for change in schema_changes if isinstance(change, AddedMember)]
         self.heuristics_members(added_members, dropped_members, schema_changes)
 
+        for vmc in (c for c in schema_changes if isinstance(c, AddedVectorMember)):
+            self.errors.append(
+                f"Forbidden schema change in '{vmc.klassname}': Added vector member '{vmc.member}'"
+            )
+        for vmc in (c for c in schema_changes if isinstance(c, DroppedVectorMember)):
+            self.errors.append(
+                f"Forbidden schema change in '{vmc.klassname}': Added vector member '{vmc.member}'"
+            )
+
+        for rc in (c for c in schema_changes if isinstance(c, AddedSingleRelation)):
+            self.errors.append(
+                f"Forbidden schema chage in '{rc.klassname}': Added OneToOneRelation '{rc.member}'"
+            )
+        for rc in (c for c in schema_changes if isinstance(c, DroppedSingleRelation)):
+            self.errors.append(
+                f"Forbidden schema chage in '{rc.klassname}': Dropped OneToOneRelation '{rc.member}'"
+            )
+
+        for rc in (c for c in schema_changes if isinstance(c, AddedMultiRelation)):
+            self.errors.append(
+                f"Forbidden schema chage in '{rc.klassname}': Added OneToManyRelation '{rc.member}'"
+            )
+        for rc in (c for c in schema_changes if isinstance(c, DroppedMultiRelation)):
+            self.errors.append(
+                f"Forbidden schema chage in '{rc.klassname}': Dropped OneToManyRelation '{rc.member}'"
+            )
+
         # are the member changes actually supported/supportable?
         changed_members = [
             change for change in schema_changes if isinstance(change, ChangedMember)
@@ -415,6 +540,9 @@ def print_comparison(self):
             print("ERRORS:")
             for error in self.errors:
                 print(f" - {error}")
+            return False
+
+        return True
 
     def read(self) -> None:
         """read datamodels from yaml files"""
@@ -474,5 +602,6 @@ def read_evolution_file(self) -> None:
     comparator = DataModelComparator(args.new, args.old, evolution_file=args.evo)
     comparator.read()
     comparator.compare()
-    comparator.print_comparison()
+    if not comparator.print_comparison():
+        sys.exit(1)
     # print(comparator.get_changed_schemata(schema_filter=root_filter))

tests/schema_evolution/CMakeLists.txt

@@ -57,6 +57,8 @@ add_test(
     ${CMAKE_CURRENT_SOURCE_DIR}/datalayout_notpossible.yaml
     ${PROJECT_SOURCE_DIR}/tests/datalayout.yaml
 )
-set_property(TEST schema-evolution-script-with-failure PROPERTY WILL_FAIL)
+set_property(TEST schema-evolution-script-with-failure PROPERTY WILL_FAIL true)
+
+add_subdirectory(detection)
 
 add_subdirectory(root_io)

tests/schema_evolution/detection/CMakeLists.txt

@@ -0,0 +1,32 @@
+set(should_fail_cases
+  vector_members:add_new
+  vector_members:add_additional
+  vector_members:rm_one
+  relations:new_single_relation
+  relations:rm_single_relation
+  relations:new_multi_relation
+  relations:rm_multi_relation
+  relations:mv_single_to_multi
+  relations:mv_multi_to_single
+)
+
+set(should_succeed_cases
+  members:float_to_double
+)
+
+set(should_warn_cases
+  members:rename
+)
+
+foreach(test_case IN LISTS should_fail_cases should_succeed_cases should_warn_cases)
+  add_test(NAME schema_evol:detection:${test_case} COMMAND ${CMAKE_CURRENT_LIST_DIR}/run_test_case.sh ${test_case})
+  PODIO_SET_TEST_ENV(schema_evol:detection:${test_case})
+endforeach()
+
+foreach(test_case IN LISTS should_fail_cases)
+  set_property(TEST schema_evol:detection:${test_case} PROPERTY WILL_FAIL true)
+endforeach()
+
+foreach(test_case IN LISTS should_warn_cases)
+  set_property(TEST schema_evol:detection:${test_case} PROPERTY PASS_REGULAR_EXPRESSION "Warnings:")
+endforeach()

tests/schema_evolution/detection/README.md

@@ -0,0 +1,27 @@
+# *Detection* tests for `podio_schema_evolution.py`
+
+This folder contains small and targetted test cases to ensure that the
+`podio_schema_evolution.py` script reliably detects schema changes that are not
+trivial. These test cases only deal with detecting these occurences! Whether the
+detected schema evolution steps are supported by podio (yet) are not really
+interesting here and they (will be) tested in another place.
+
+## Setup of the tests
+
+In order to allow for some minimal form of automation and to avoid too much
+boilerplate the test cases are grouped into categories. Each category then has
+several *unit test like* setups where each test covers exactly one schema
+change. Each subfolder in this directory represents a category. In each
+subfolder there are for each test case (i.e. schema change) exactly two yaml
+files with the (minimal) schemas that have an example of the change. To allow
+for test automation these yaml files need to follow the naming convention
+`dm_<test-case-name>_{old,new}.yaml`, where the `old` yaml file needs to have a
+lower `schema_version` than the `new` yaml file.
+
+The `run_test_case.sh` script takes one argument in the form of
+`<category-name>:<test-case-name>`. It constructs the necessary file names from
+this input and then runs the `podio_schema_evolution.py` script on them.
+
+Finally, in the `CMakeLists.txt` file here, it is simply necessary to add new
+test cases to the `should_fail_cases`, `should_succeed_cases` or
+`should_warn_cases` lists and they will be automatically picked up.

tests/schema_evolution/detection/members/dm_float_to_double_new.yaml

@@ -0,0 +1,8 @@
+schema_version: 2
+
+datatypes:
+  TypeWithFloat:
+    Description: "A type with a double that was a float"
+    Author: "Thomas Madlener"
+    Members:
+      - double f // a float to become a double

tests/schema_evolution/detection/members/dm_float_to_double_old.yaml

@@ -0,0 +1,8 @@
+schema_version: 1
+
+datatypes:
+  TypeWithFloat:
+    Description: "A type with a float that will become a double"
+    Author: "Thomas Madlener"
+    Members:
+      - float f // a float to become a double

tests/schema_evolution/detection/members/dm_rename_new.yaml

@@ -0,0 +1,8 @@
+schema_version: 3
+
+datatypes:
+  TypeWithRenamedMember:
+    Description: "A type with a renamed member"
+    Author: "Thomas Madlener"
+    Members:
+      - int newName // the new name

tests/schema_evolution/detection/members/dm_rename_old.yaml

@@ -0,0 +1,8 @@
+schema_version: 1
+
+datatypes:
+  TypeWithRenamedMember:
+    Description: "A type with a member that will be renamed"
+    Author: "Thomas Madlener"
+    Members:
+      - int oldName // the old name

tests/schema_evolution/detection/relations/dm_mv_multi_to_single_new.yaml

@@ -0,0 +1,12 @@
+schema_version: 2
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithRelationMigration:
+    Description: "Type for testing the move from a OneToMany to a OneToOne relation"
+    Author: "Thomas Madlener"
+    OneToOneRelations:
+      - RelatedType rel // a relation

tests/schema_evolution/detection/relations/dm_mv_multi_to_single_old.yaml

@@ -0,0 +1,12 @@
+schema_version: 1
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithRelationMigration:
+    Description: "Type for testing the move from a OneToMany to a OneToOne relation"
+    Author: "Thomas Madlener"
+    OneToManyRelations:
+      - RelatedType rel // a relation

tests/schema_evolution/detection/relations/dm_mv_single_to_multi_new.yaml

@@ -0,0 +1,12 @@
+schema_version: 2
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithRelationMigration:
+    Description: "Type for testing the move from a OneToOne to a OneToMany relation"
+    Author: "Thomas Madlener"
+    OneToManyRelations:
+      - RelatedType rel // a relation

tests/schema_evolution/detection/relations/dm_mv_single_to_multi_old.yaml

@@ -0,0 +1,12 @@
+schema_version: 1
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithRelationMigration:
+    Description: "Type for testing the move from a OneToOne to a OneToMany relation"
+    Author: "Thomas Madlener"
+    OneToOneRelations:
+      - RelatedType rel // a relation

tests/schema_evolution/detection/relations/dm_new_multi_relation_new.yaml

@@ -0,0 +1,12 @@
+schema_version: 2
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithNewMultiRelation:
+    Description: "Type for testing the addition of new OneToManyRelations"
+    Author: "Thomas Madlener"
+    OneToManyRelations:
+      - RelatedType rel // a new relation

tests/schema_evolution/detection/relations/dm_new_multi_relation_old.yaml

@@ -0,0 +1,10 @@
+schema_version: 1
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithNewMultiRelation:
+    Description: "Type for testing the addition of new OneToManyRelations"
+    Author: "Thomas Madlener"

tests/schema_evolution/detection/relations/dm_new_single_relation_new.yaml

@@ -0,0 +1,12 @@
+schema_version: 2
+
+datatypes:
+  RelatedType:
+    Description: "A type we use in the relation"
+    Author: "Thomas Madlener"
+
+  DataTypeWithNewSingleRelation:
+    Description: "Type for testing the addition of new OneToOneRelations"
+    Author: "Thomas Madlener"
+    OneToOneRelations:
+      - RelatedType rel // a new relation