Merge pull request #821 from Aiven-Open/protobuf-recognize-tag-alter-…

…modification protobuf: recognize tag alter modification
Aiven-Open · Feb 23, 2024 · 021640d · 021640d
2 parents d9757f5 + c911847
commit 021640d
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 0 deletions.
diff --git a/karapace/protobuf/compare_result.py b/karapace/protobuf/compare_result.py
@@ -26,6 +26,7 @@ class Modification(Enum):
     FIELD_NAME_ALTER = auto()
     FIELD_KIND_ALTER = auto()
     FIELD_TYPE_ALTER = auto()
+    FIELD_TAG_ALTER = auto()
     ONE_OF_ADD = auto()
     ONE_OF_DROP = auto()
     ONE_OF_MOVE = auto()
@@ -43,6 +44,7 @@ def is_compatible(self) -> bool:
             self.FIELD_LABEL_ALTER,
             self.FIELD_KIND_ALTER,
             self.FIELD_TYPE_ALTER,
+            self.FIELD_TAG_ALTER,
             self.ONE_OF_FIELD_DROP,
             self.FEW_FIELDS_CONVERTED_TO_ONE_OF,
         ]  # type: ignore[comparison-overlap]

diff --git a/karapace/protobuf/message_element.py b/karapace/protobuf/message_element.py
@@ -94,14 +94,18 @@ def compare(self, other: TypeElement, result: CompareResult, types: CompareTypes
         if types.lock_message(self):
             self_tags = {}
             other_tags = {}
+            self_names = {}
+            other_names = {}
             self_one_ofs = {}
             other_one_ofs = {}
 
             for field in self.fields:
                 self_tags[field.tag] = field
+                self_names[field.name] = field
 
             for field in other.fields:
                 other_tags[field.tag] = field
+                other_names[field.name] = field
 
             for one_of in self.one_ofs:
                 self_one_ofs[one_of.name] = one_of
@@ -125,6 +129,20 @@ def compare(self, other: TypeElement, result: CompareResult, types: CompareTypes
             for tag in chain(self_tags.keys(), other_tags.keys() - self_tags.keys()):
                 result.push_path(str(tag))
 
+                if (
+                    # Check if field tag has been altered
+                    tag in self_tags
+                    and (field_name := self_tags[tag].name)
+                    and (self_field := self_names.get(field_name))
+                    and (other_field := other_names.get(field_name))
+                    and (self_field.tag != other_field.tag)
+                ):
+                    result.pop_path()
+                    result.push_path(field_name)
+                    result.add_modification(Modification.FIELD_TAG_ALTER)
+                    result.pop_path()
+                    result.push_path(str(tag))
+
                 if self_tags.get(tag) is None:
                     result.add_modification(Modification.FIELD_ADD)
                 elif other_tags.get(tag) is None:
@@ -133,6 +151,7 @@ def compare(self, other: TypeElement, result: CompareResult, types: CompareTypes
                     self_tags[tag].compare(other_tags[tag], result, types)
 
                 result.pop_path()
+
             # Compare OneOfs
             for name in chain(self_one_ofs.keys(), other_one_ofs.keys() - self_one_ofs.keys()):
                 result.push_path(str(name))

diff --git a/tests/unit/protobuf/test_protobuf_compatibility.py b/tests/unit/protobuf/test_protobuf_compatibility.py
@@ -300,3 +300,39 @@ def test_compatibility_ordering_change2():
     assert result.is_compatible()
     assert len(result.result) == 1
     assert result.result[0].modification == Modification.FIELD_ADD
+
+
+def test_compatibility_field_tag_change():
+    self_schema = """\
+syntax = "proto3";
+package pkg;
+message Foo {
+    string fieldA = 1;
+    string fieldB = 2;
+    string fieldC = 3;
+    string fieldX = 4;
+}
+"""
+
+    other_schema = """\
+syntax = "proto3";
+package pkg;
+message Foo {
+    string fieldA = 1;
+    string fieldB = 2;
+    string fieldC = 3;
+    string fieldX = 5;
+}
+"""
+
+    self_parsed: ProtoFileElement = ProtoParser.parse(location, self_schema)
+    other_parsed: ProtoFileElement = ProtoParser.parse(location, other_schema)
+
+    result = CompareResult()
+    self_parsed.compare(other_parsed, result)
+    assert not result.is_compatible()
+    assert {(error.modification, error.path) for error in result.result} == {
+        (Modification.FIELD_TAG_ALTER, "Foo.fieldX"),
+        (Modification.FIELD_DROP, "Foo.4"),
+        (Modification.FIELD_ADD, "Foo.5"),
+    }