SAT: new tests for spec backward compatibility - syntactic validation (…

…#15194)

airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 0.1.59
+Backward compatibility tests: add syntactic validation of specs [#15194](https://github.com/airbytehq/airbyte/pull/15194/).
+
 ## 0.1.58
 Bootstrap spec backward compatibility tests. Add fixtures to retrieve a previous connector version spec [#14954](https://github.com/airbytehq/airbyte/pull/14954/).
 

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.1.58
+LABEL io.airbyte.version=0.1.59
 LABEL io.airbyte.name=airbyte/source-acceptance-test
 
 ENTRYPOINT ["python", "-m", "pytest", "-p", "source_acceptance_test.plugin", "-r", "fEsx"]

airbyte-integrations/bases/source-acceptance-test/pytest.ini

@@ -6,3 +6,4 @@ testpaths =
 
 markers =
     default_timeout
+    backward_compatibility

airbyte-integrations/bases/source-acceptance-test/setup.py

@@ -20,6 +20,7 @@
     "dpath~=2.0.1",
     "jsonschema~=3.2.0",
     "jsonref==0.2",
+    "deepdiff~=5.8.0",
     "requests-mock",
     "pytest-mock~=3.6.1",
 ]

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/config.py

@@ -17,6 +17,8 @@
 configured_catalog_path: Optional[str] = Field(default=None, description="Path to configured catalog")
 timeout_seconds: int = Field(default=None, description="Test execution timeout_seconds", ge=0)
 
+SEMVER_REGEX = r"(0|(?:[1-9]\d*))(?:\.(0|(?:[1-9]\d*))(?:\.(0|(?:[1-9]\d*)))?(?:\-([\w][\w\.\-_]*))?)?"
+
 
 class BaseConfig(BaseModel):
     class Config:
@@ -25,10 +27,10 @@ class Config:
 
 class BackwardCompatibilityTestsConfig(BaseConfig):
     previous_connector_version: str = Field(
-        default="latest", description="Previous connector version to use for backward compatibility tests."
+        regex=SEMVER_REGEX, default="latest", description="Previous connector version to use for backward compatibility tests."
     )
-    disable_backward_compatibility_tests_for_version: Optional[str] = Field(
-        default=None, description="Disable backward compatibility tests for a specific connector version."
+    disable_for_version: Optional[str] = Field(
+        regex=SEMVER_REGEX, default=None, description="Disable backward compatibility tests for a specific connector version."
     )
 
 

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/conftest.py

@@ -23,8 +23,9 @@
     Type,
 )
 from docker import errors
+from source_acceptance_test.base import BaseTest
 from source_acceptance_test.config import Config
-from source_acceptance_test.utils import ConnectorRunner, SecretDict, load_config, load_yaml_or_json_path
+from source_acceptance_test.utils import ConnectorRunner, SecretDict, filter_output, load_config, load_yaml_or_json_path
 
 
 @pytest.fixture(name="base_path")
@@ -210,6 +211,35 @@ def detailed_logger() -> Logger:
     return logger
 
 
+@pytest.fixture(name="actual_connector_spec")
+def actual_connector_spec_fixture(request: BaseTest, docker_runner: ConnectorRunner) -> ConnectorSpecification:
+    if not request.instance.spec_cache:
+        output = docker_runner.call_spec()
+        spec_messages = filter_output(output, Type.SPEC)
+        assert len(spec_messages) == 1, "Spec message should be emitted exactly once"
+        spec = spec_messages[0].spec
+        request.instance.spec_cache = spec
+    return request.instance.spec_cache
+
+
+@pytest.fixture(name="previous_connector_spec")
+def previous_connector_spec_fixture(
+    request: BaseTest, previous_connector_docker_runner: ConnectorRunner
+) -> Optional[ConnectorSpecification]:
+    if previous_connector_docker_runner is None:
+        logging.warning(
+            "\n We could not retrieve the previous connector spec as a connector runner for the previous connector version could not be instantiated."
+        )
+        return None
+    if not request.instance.previous_spec_cache:
+        output = previous_connector_docker_runner.call_spec()
+        spec_messages = filter_output(output, Type.SPEC)
+        assert len(spec_messages) == 1, "Spec message should be emitted exactly once"
+        spec = spec_messages[0].spec
+        request.instance.previous_spec_cache = spec
+    return request.instance.previous_spec_cache
+
+
 def pytest_sessionfinish(session, exitstatus):
     """Called after whole test run finished, right before returning the exit status to the system.
     https://docs.pytest.org/en/6.2.x/reference.html#pytest.hookspec.pytest_sessionfinish

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/plugin.py

@@ -16,6 +16,10 @@
 
 def pytest_configure(config):
     config.addinivalue_line("markers", "default_timeout: mark test to be wrapped by `timeout` decorator with default value")
+    config.addinivalue_line(
+        "markers",
+        "backward_compatibility: mark test to be part of the backward compatibility tests suite (deselect with '-m \"not backward_compatibility\"')",
+    )
 
 
 def pytest_load_initial_conftests(early_config: Config, parser: Parser, args: List[str]):

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

@@ -8,7 +8,7 @@
 from collections import Counter, defaultdict
 from functools import reduce
 from logging import Logger
-from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Set
+from typing import Any, Dict, List, Mapping, MutableMapping, Set
 
 import dpath.util
 import jsonschema
@@ -24,6 +24,7 @@
     TraceType,
     Type,
 )
+from deepdiff import DeepDiff
 from docker.errors import ContainerError
 from jsonschema._utils import flatten
 from source_acceptance_test.base import BaseTest
@@ -38,41 +39,38 @@ def connector_spec_dict_fixture(actual_connector_spec):
     return json.loads(actual_connector_spec.json())
 
 
-@pytest.fixture(name="actual_connector_spec")
-def actual_connector_spec_fixture(request: BaseTest, docker_runner: ConnectorRunner) -> ConnectorSpecification:
-    if not request.instance.spec_cache:
-        output = docker_runner.call_spec()
-        spec_messages = filter_output(output, Type.SPEC)
-        assert len(spec_messages) == 1, "Spec message should be emitted exactly once"
-        spec = spec_messages[0].spec
-        request.spec_cache = spec
-    return request.spec_cache
-
-
-@pytest.fixture(name="previous_connector_spec")
-def previous_connector_spec_fixture(
-    request: BaseTest, previous_connector_docker_runner: ConnectorRunner
-) -> Optional[ConnectorSpecification]:
-    if previous_connector_docker_runner is None:
-        logging.warning(
-            "\n We could not retrieve the previous connector spec as a connector runner for the previous connector version could not be instantiated."
-        )
-        return None
-    if not request.instance.previous_spec_cache:
-        output = previous_connector_docker_runner.call_spec()
-        spec_messages = filter_output(output, Type.SPEC)
-        assert len(spec_messages) == 1, "Spec message should be emitted exactly once"
-        spec = spec_messages[0].spec
-        request.instance.previous_spec_cache = spec
-    return request.instance.previous_spec_cache
-
-
 @pytest.mark.default_timeout(10)
 class TestSpec(BaseTest):
 
     spec_cache: ConnectorSpecification = None
     previous_spec_cache: ConnectorSpecification = None
 
+    @staticmethod
+    def compute_spec_diff(actual_connector_spec: ConnectorSpecification, previous_connector_spec: ConnectorSpecification):
+        return DeepDiff(previous_connector_spec.dict(), actual_connector_spec.dict(), view="tree", ignore_order=True)
+
+    @pytest.fixture(name="skip_backward_compatibility_tests")
+    def skip_backward_compatibility_tests_fixture(self, inputs: SpecTestConfig, previous_connector_docker_runner: ConnectorRunner) -> bool:
+        if previous_connector_docker_runner is None:
+            pytest.skip("The previous connector image could not be retrieved.")
+
+        # Get the real connector version in case 'latest' is used in the config:
+        previous_connector_version = previous_connector_docker_runner._image.labels.get("io.airbyte.version")
+
+        if previous_connector_version == inputs.backward_compatibility_tests_config.disable_for_version:
+            pytest.skip(f"Backward compatibility tests are disabled for version {previous_connector_version}.")
+        return False
+
+    @pytest.fixture(name="spec_diff")
+    def spec_diff_fixture(
+        self,
+        skip_backward_compatibility_tests: bool,
+        actual_connector_spec: ConnectorSpecification,
+        previous_connector_spec: ConnectorSpecification,
+    ) -> DeepDiff:
+        assert isinstance(actual_connector_spec, ConnectorSpecification) and isinstance(previous_connector_spec, ConnectorSpecification)
+        return self.compute_spec_diff(actual_connector_spec, previous_connector_spec)
+
     def test_config_match_spec(self, actual_connector_spec: ConnectorSpecification, connector_config: SecretDict):
         """Check that config matches the actual schema from the spec call"""
         # Getting rid of technical variables that start with an underscore
@@ -182,7 +180,108 @@ def test_oauth_flow_parameters(self, actual_connector_spec: ConnectorSpecificati
         diff = params - schema_path
         assert diff == set(), f"Specified oauth fields are missed from spec schema: {diff}"
 
-    def test_additional_properties_is_true(self, actual_connector_spec):
+    @pytest.mark.default_timeout(60)
+    @pytest.mark.backward_compatibility
+    def test_new_required_field_declaration(self, spec_diff: DeepDiff):
+        """Check if a 'required' field was added to the spec."""
+        added_required_fields = [
+            addition for addition in spec_diff.get("dictionary_item_added", []) if addition.path(output_format="list")[-1] == "required"
+        ]
+        assert len(added_required_fields) == 0, f"The current spec has a new required field: {spec_diff.pretty()}"
+
+    @pytest.mark.default_timeout(60)
+    @pytest.mark.backward_compatibility
+    def test_new_required_property(self, spec_diff: DeepDiff):
+        """Check if a a new property was added to the 'required' field."""
+        added_required_properties = [
+            addition for addition in spec_diff.get("iterable_item_added", []) if addition.up.path(output_format="list")[-1] == "required"
+        ]
+        assert len(added_required_properties) == 0, f"The current spec has a new required property: {spec_diff.pretty()}"
+
+    @pytest.mark.default_timeout(60)
+    @pytest.mark.backward_compatibility
+    def test_field_type_changed(self, spec_diff: DeepDiff):
+        """Check if the current spec is changing the types of properties."""
+
+        common_error_message = f"The current spec changed the value of a 'type' field:  {spec_diff.pretty()}"
+        # Detect type value change in case type field is declared as a string (e.g "str" -> "int"):
+        type_values_changed = [change for change in spec_diff.get("values_changed", []) if change.path(output_format="list")[-1] == "type"]
+
+        # Detect type value change in case type field is declared as a single item list (e.g ["str"] -> ["int"]):
+        type_values_changed_in_list = [
+            change for change in spec_diff.get("values_changed", []) if change.path(output_format="list")[-2] == "type"
+        ]
+
+        assert len(type_values_changed_in_list) == 0 and len(type_values_changed) == 0, common_error_message
+
+        # Detect type value added to type list if new type value is not None (e.g ["str"] -> ["str", "int"]):
+        # It works because we compute the diff with 'ignore_order=True'
+        new_values_in_type_list = [  # noqa: F841
+            change
+            for change in spec_diff.get("iterable_item_added", [])
+            if change.path(output_format="list")[-2] == "type"
+            if change.t2 != "null"
+        ]
+        # enable the assertion below if we want to disallow type expansion:
+        # assert len(new_values_in_type_list) == 0, common_error_message
+
+        # Detect the change of type of a type field
+        # e.g:
+        # - "str" -> ["str"] VALID
+        # - "str" -> ["str", "null"] VALID
+        # - "str" -> ["str", "int"] VALID
+        # - "str" -> 1 INVALID
+        # - ["str"] -> "str" VALID
+        # - ["str"] -> "int" INVALID
+        # - ["str"] -> 1 INVALID
+        type_changes = [change for change in spec_diff.get("type_changes", []) if change.path(output_format="list")[-1] == "type"]
+        for change in type_changes:
+            # We only accept change on the type field if the new type for this field is list or string
+            # This might be something already guaranteed by JSON schema validation.
+            if isinstance(change.t1, str):
+                assert isinstance(
+                    change.t2, list
+                ), f"The current spec change a type field from string to an invalid value: {spec_diff.pretty()}"
+                assert (
+                    0 < len(change.t2) <= 2
+                ), f"The current spec change a type field from string to an invalid value. The type list length should not be empty and have a maximum of two items {spec_diff.pretty()}."
+                # 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"]
+                assert (
+                    len(t2_not_null_types) == 1 and t2_not_null_types[0] == change.t1
+                ), "The current spec change a type field to a list with multiple invalid values."
+            if isinstance(change.t1, list):
+                assert isinstance(
+                    change.t2, str
+                ), f"The current spec change a type field from list to an invalid value:  {spec_diff.pretty()}"
+                assert len(change.t1) == 1 and change.t2 == change.t1[0], f"The current spec narrowed a field type: {spec_diff.pretty()}"
+
+        # Detect when field was made not nullable but is still a list: e.g ["string", "null"] -> ["string"]
+        removed_nullable = [
+            change for change in spec_diff.get("iterable_item_removed", []) if change.path(output_format="list")[-2] == "type"
+        ]
+        assert len(removed_nullable) == 0, f"The current spec narrowed a field type: {spec_diff.pretty()}"
+
+    @pytest.mark.default_timeout(60)
+    @pytest.mark.backward_compatibility
+    def test_enum_field_has_narrowed(self, spec_diff: DeepDiff):
+        """Check if the list of values in a enum was shortened in a spec."""
+        removals = [
+            removal for removal in spec_diff.get("iterable_item_removed", []) if removal.up.path(output_format="list")[-1] == "enum"
+        ]
+        assert len(removals) == 0, f"The current spec narrowed a field enum: {spec_diff.pretty()}"
+
+    @pytest.mark.default_timeout(60)
+    @pytest.mark.backward_compatibility
+    def test_new_enum_field_declaration(self, spec_diff: DeepDiff):
+        """Check if an 'enum' field was added to the spec."""
+        added_enum_fields = [
+            addition for addition in spec_diff.get("dictionary_item_added", []) if addition.path(output_format="list")[-1] == "enum"
+        ]
+        assert len(added_enum_fields) == 0, f"An 'enum' field was declared on an existing property of the spec: {spec_diff.pretty()}"
+
+    def test_additional_properties_is_true(self, actual_connector_spec: ConnectorSpecification):
         """Check that value of the "additionalProperties" field is always true.
         A spec declaring "additionalProperties": false introduces the risk of accidental breaking changes.
         Specifically, when removing a property from the spec, existing connector configs will no longer be valid.
@@ -196,24 +295,6 @@ def test_additional_properties_is_true(self, actual_connector_spec):
                 [additional_properties_value is True for additional_properties_value in additional_properties_values]
             ), "When set, additionalProperties field value must be true for backward compatibility."
 
-    @pytest.mark.default_timeout(60)  # Pulling the previous connector image can take more than 10 sec.
-    @pytest.mark.spec_backward_compatibility
-    def test_backward_compatibility(
-        self, inputs: SpecTestConfig, actual_connector_spec: ConnectorSpecification, previous_connector_spec: ConnectorSpecification
-    ):
-        """Run multiple checks to make sure the actual_connector_spec is backward compatible with the previous_connector_spec"""
-        if (
-            inputs.backward_compatibility_tests_config.disable_backward_compatibility_tests_for_version
-            == inputs.backward_compatibility_tests_config.previous_connector_version
-        ):
-            pytest.skip(
-                f"Backward compatibility tests are disabled for version {inputs.backward_compatibility_tests_config.disable_backward_compatibility_tests_for_version}."
-            )
-        if previous_connector_spec is None:
-            pytest.skip("The previous connector spec could not be retrieved.")
-        assert isinstance(actual_connector_spec, ConnectorSpecification) and isinstance(previous_connector_spec, ConnectorSpecification)
-        # TODO alafanechere: add the actual tests for backward compatibility below or in a dedicated module.
-
 
 @pytest.mark.default_timeout(30)
 class TestConnection(BaseTest):