SATS: fix backwards compatibility (#16429)

* SATS: fix backwards compatibility * #16429 source acceptance tests: modify validation for specs only * #16429 source acceptance tests: add typing * SATs: fix changelog Co-authored-by: Augustin <augustin.lafanechere@gmail.com>
airbytehq · Sep 12, 2022 · f46aaff · f46aaff
1 parent 0ab777f
commit f46aaff
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 9 deletions.
diff --git a/airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md b/airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md
@@ -1,5 +1,8 @@
 # Changelog
 
+## 0.2.3
+Backward compatibility tests: improve `check_if_type_of_type_field_changed` to make it less radical when validating specs and allow `'str' -> ['str', '<another_type>']` type changes.[#16429](https://github.com/airbytehq/airbyte/pull/16429/)
+
 ## 0.2.2
 Backward compatibility tests: improve `check_if_cursor_field_was_changed` to make it less radical and allow stream addition to catalog.[#15835](https://github.com/airbytehq/airbyte/pull/15835/)
 

diff --git a/airbyte-integrations/bases/source-acceptance-test/Dockerfile b/airbyte-integrations/bases/source-acceptance-test/Dockerfile
@@ -33,7 +33,7 @@ COPY pytest.ini setup.py ./
 COPY source_acceptance_test ./source_acceptance_test
 RUN pip install .
 
-LABEL io.airbyte.version=0.2.2
+LABEL io.airbyte.version=0.2.3
 LABEL io.airbyte.name=airbyte/source-acceptance-test
 
 ENTRYPOINT ["python", "-m", "pytest", "-p", "source_acceptance_test.plugin", "-r", "fEsx"]
diff --git a/...tions/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py b/...tions/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py
@@ -70,7 +70,7 @@ def check_if_new_type_was_added(self, diff: DeepDiff):  # pragma: no cover
         if new_values_in_type_list:
             self._raise_error("A new value was added to a 'type' field")
 
-    def check_if_type_of_type_field_changed(self, diff: DeepDiff):
+    def check_if_type_of_type_field_changed(self, diff: DeepDiff, allow_type_widening: bool = False):
         """
         Detect the change of type of a type field on a property
         e.g:
@@ -91,11 +91,20 @@ def check_if_type_of_type_field_changed(self, diff: DeepDiff):
             if isinstance(change.t1, str):
                 if not isinstance(change.t2, list):
                     self._raise_error("A 'type' field was changed from string to an invalid value.", diff)
-                # If the new type field is a list we want to make sure it only has the original type (t1) and null: e.g. "str" -> ["str", "null"]
-                # We want to raise an error otherwise.
-                t2_not_null_types = [_type for _type in change.t2 if _type != "null"]
-                if not (len(t2_not_null_types) == 1 and t2_not_null_types[0] == change.t1):
-                    self._raise_error("The 'type' field was changed to a list with multiple invalid values", diff)
+                if allow_type_widening:
+                    # If the new type field is a list we want to make sure it contains the original type (t1):
+                    # e.g. "str" -> ["str", "null"]
+                    #      "int" -> ["int", "str"]
+                    # We want to raise an error otherwise.
+                    if change.t1 not in change.t2:
+                        self._raise_error("The 'type' field was changed to a list which does not contain previous type", diff)
+                else:
+                    # If the new type field is a list we want to make sure it only has the original type (t1)
+                    # and null: e.g. "str" -> ["str", "null"]
+                    # We want to raise an error otherwise.
+                    t2_not_null_types = [_type for _type in change.t2 if _type != "null"]
+                    if not (len(t2_not_null_types) == 1 and t2_not_null_types[0] == change.t1):
+                        self._raise_error("The 'type' field was changed to a list with multiple invalid values", diff)
             if isinstance(change.t1, list):
                 if not isinstance(change.t2, str):
                     self._raise_error("The 'type' field was changed from a list to an invalid value", diff)
@@ -121,7 +130,7 @@ def assert_is_backward_compatible(self):
         self.check_if_added_a_new_required_property(self.connection_specification_diff)
         self.check_if_value_of_type_field_changed(self.connection_specification_diff)
         # self.check_if_new_type_was_added(self.connection_specification_diff) We want to allow type expansion atm
-        self.check_if_type_of_type_field_changed(self.connection_specification_diff)
+        self.check_if_type_of_type_field_changed(self.connection_specification_diff, allow_type_widening=True)
         self.check_if_field_was_made_not_nullable(self.connection_specification_diff)
         self.check_if_enum_was_narrowed(self.connection_specification_diff)
         self.check_if_declared_new_enum_field(self.connection_specification_diff)
@@ -213,7 +222,7 @@ def compute_diffs(self):
     def assert_is_backward_compatible(self):
         self.check_if_stream_was_removed(self.streams_json_schemas_diff)
         self.check_if_value_of_type_field_changed(self.streams_json_schemas_diff)
-        self.check_if_type_of_type_field_changed(self.streams_json_schemas_diff)
+        self.check_if_type_of_type_field_changed(self.streams_json_schemas_diff, allow_type_widening=False)
         self.check_if_cursor_field_was_changed(self.streams_cursor_fields_diff)
 
     def check_if_stream_was_removed(self, diff: DeepDiff):

diff --git a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py
@@ -939,6 +939,26 @@ def as_pytest_param(self):
         name="Nested level: Removing the enum field should not fail.",
         should_fail=False,
     ),
+    Transition(
+        ConnectorSpecification(
+            connectionSpecification={
+                "type": "object",
+                "properties": {
+                    "my_string": {"type": "integer"},
+                },
+            }
+        ),
+        ConnectorSpecification(
+            connectionSpecification={
+                "type": "object",
+                "properties": {
+                    "my_string": {"type": ["integer", "string"]},
+                },
+            }
+        ),
+        name="Changing a 'type' field from a string to a list containing that same string should not fail.",
+        should_fail=False,
+    ),
 ]
 
 # Checking that all transitions in FAILING_SPEC_TRANSITIONS have should_fail == True to prevent typos
@@ -1131,6 +1151,28 @@ def test_validate_previous_configs(previous_connector_spec, actual_connector_spe
             ),
         },
     ),
+    Transition(
+        name="Changing a 'type' field from a string to something else than a list containing just that string and null should fail.",
+        should_fail=True,
+        previous={
+            "test_stream": AirbyteStream.parse_obj(
+                {
+                    "name": "test_stream",
+                    "json_schema": {"properties": {"user": {"type": "object", "properties": {"username": {"type": "integer"}}}}},
+                    "default_cursor_field": ["a"],
+                }
+            ),
+        },
+        current={
+            "test_stream": AirbyteStream.parse_obj(
+                {
+                    "name": "test_stream",
+                    "json_schema": {"properties": {"user": {"type": "object", "properties": {"username": {"type": ["integer", "string"]}}}}},
+                    "default_cursor_field": ["b"],
+                }
+            ),
+        },
+    )
 ]
 
 VALID_CATALOG_TRANSITIONS = [