Revert "Revert "Extend SATs to capture UI limitations (#21451)" (#21896)"

This reverts commit 74b5dbf.

Author: Joe Reuter

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

@@ -1,10 +1,7 @@
 # Changelog
 
-## 0.4.0
-Revert 0.3.0
-
 ## 0.3.0
-(Broken) Add various stricter checks for specs (see PR for details). [#21451](https://github.com/airbytehq/airbyte/pull/21451)
+Add various stricter checks for specs (see PR for details). [#21451](https://github.com/airbytehq/airbyte/pull/21451)
 
 ## 0.2.26
 Check `future_state` only for incremental streams. [#21248](https://github.com/airbytehq/airbyte/pull/21248)

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.4.0
+LABEL io.airbyte.version=0.3.0
 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/source_acceptance_test/tests/test_core.py

@@ -76,12 +76,20 @@ def secret_property_names_fixture():
     )
 
 
+DATE_PATTERN = "^[0-9]{2}-[0-9]{2}-[0-9]{4}$"
+DATETIME_PATTERN = "^[0-9]{4}-[0-9]{2}-[0-9]{2}(T[0-9]{2}:[0-9]{2}:[0-9]{2})?$"
+
+
 @pytest.mark.default_timeout(10)
 class TestSpec(BaseTest):
 
     spec_cache: ConnectorSpecification = None
     previous_spec_cache: ConnectorSpecification = None
 
+    @pytest.fixture(name="connection_specification", scope="class")
+    def connection_specification_fixture(self, connector_spec_dict: dict) -> dict:
+        return connector_spec_dict["connectionSpecification"]
+
     @pytest.fixture(name="skip_backward_compatibility_tests")
     def skip_backward_compatibility_tests_fixture(
         self,
@@ -219,22 +227,15 @@ def _property_can_store_secret(prop: dict) -> bool:
         Some fields can not hold a secret by design, others can.
         Null type as well as boolean can not hold a secret value.
         A string, a number or an integer type can always store secrets.
-        Objects and arrays can hold a secret in case they are generic,
-        meaning their inner structure is not described in details with properties/items.
+        Secret objects and arrays can not be rendered correctly in the UI:
         A field with a constant value can not hold a secret as well.
         """
         unsecure_types = {"string", "integer", "number"}
         type_ = prop["type"]
-        is_property_generic_object = type_ == "object" and not any(
-            [prop.get("properties", {}), prop.get("anyOf", []), prop.get("oneOf", []), prop.get("allOf", [])]
-        )
-        is_property_generic_array = type_ == "array" and not any([prop.get("items", []), prop.get("prefixItems", [])])
         is_property_constant_value = bool(prop.get("const"))
         can_store_secret = any(
             [
                 isinstance(type_, str) and type_ in unsecure_types,
-                is_property_generic_object,
-                is_property_generic_array,
                 isinstance(type_, list) and (set(type_) & unsecure_types),
             ]
         )
@@ -252,7 +253,7 @@ def test_secret_is_properly_marked(self, connector_spec_dict: dict, detailed_log
         secrets_exposed = []
         non_secrets_hidden = []
         spec_properties = connector_spec_dict["connectionSpecification"]["properties"]
-        for type_path, value in dpath.util.search(spec_properties, "**/type", yielded=True):
+        for type_path, type_value in dpath.util.search(spec_properties, "**/type", yielded=True):
             _, is_property_name_secret = self._is_spec_property_name_secret(type_path, secret_property_names)
             if not is_property_name_secret:
                 continue
@@ -268,7 +269,7 @@ def test_secret_is_properly_marked(self, connector_spec_dict: dict, detailed_log
 
         if non_secrets_hidden:
             properties = "\n".join(non_secrets_hidden)
-            detailed_logger.warning(
+            pytest.fail(
                 f"""Some properties are marked with `airbyte_secret` although they probably should not be.
                 Please double check them. If they're okay, please fix this test.
                 {properties}"""
@@ -280,6 +281,139 @@ def test_secret_is_properly_marked(self, connector_spec_dict: dict, detailed_log
                     {properties}"""
             )
 
+    def _fail_on_errors(self, errors: List[str]):
+        if len(errors) > 0:
+            pytest.fail("\n".join(errors))
+
+    def test_property_type_is_not_array(self, connector_specification: dict):
+        """
+        Each field has one or multiple types, but the UI only supports a single type and optionally "null" as a second type.
+        """
+        errors = []
+        for type_path, type_value in dpath.util.search(connector_specification, "**/properties/*/type", yielded=True):
+            if isinstance(type_value, List):
+                number_of_types = len(type_value)
+                if number_of_types != 2 and number_of_types != 1:
+                    errors.append(
+                        f"{type_path} is not either a simple type or an array of a simple type plus null: {type_value} (for example: type: [string, null])"
+                    )
+                if number_of_types == 2 and type_value[1] != "null":
+                    errors.append(
+                        f"Second type of {type_path} is not null: {type_value}. Type can either be a simple type or an array of a simple type plus null (for example: type: [string, null])"
+                    )
+        self._fail_on_errors(errors)
+
+    def test_object_not_empty(self, connector_specification: dict):
+        """
+        Each object field needs to have at least one property as the UI won't be able to show them otherwise.
+        If the whole spec is empty, it's allowed to have a single empty object at the top level
+        """
+        schema_helper = JsonSchemaHelper(connector_specification)
+        errors = []
+        for type_path, type_value in dpath.util.search(connector_specification, "**/type", yielded=True):
+            if type_path == "type":
+                # allow empty root object
+                continue
+            if type_value == "object":
+                property = schema_helper.get_parent(type_path)
+                if "oneOf" not in property and ("properties" not in property or len(property["properties"]) == 0):
+                    errors.append(
+                        f"{type_path} is an empty object which will not be represented correctly in the UI. Either remove or add specific properties"
+                    )
+        self._fail_on_errors(errors)
+
+    def test_array_type(self, connector_specification: dict):
+        """
+        Each array has one or multiple types for its items, but the UI only supports a single type which can either be object, string or an enum
+        """
+        schema_helper = JsonSchemaHelper(connector_specification)
+        errors = []
+        for type_path, type_type in dpath.util.search(connector_specification, "**/type", yielded=True):
+            property_definition = schema_helper.get_parent(type_path)
+            if type_type != "array":
+                # unrelated "items", not an array definition
+                continue
+            items_value = property_definition.get("items", None)
+            if items_value is None:
+                continue
+            elif isinstance(items_value, List):
+                errors.append(f"{type_path} is not just a single item type: {items_value}")
+            elif items_value.get("type") not in ["object", "string", "number", "integer"] and "enum" not in items_value:
+                errors.append(f"Items of {type_path} has to be either object or string or define an enum")
+        self._fail_on_errors(errors)
+
+    def test_forbidden_complex_types(self, connector_specification: dict):
+        """
+        not, anyOf, patternProperties, prefixItems, allOf, if, then, else, dependentSchemas and dependentRequired are not allowed
+        """
+        forbidden_keys = [
+            "not",
+            "anyOf",
+            "patternProperties",
+            "prefixItems",
+            "allOf",
+            "if",
+            "then",
+            "else",
+            "dependentSchemas",
+            "dependentRequired",
+        ]
+        found_keys = set()
+        for forbidden_key in forbidden_keys:
+            for path, value in dpath.util.search(connector_specification, f"**/{forbidden_key}", yielded=True):
+                found_keys.add(path)
+
+        for forbidden_key in forbidden_keys:
+            # remove forbidden keys if they are used as properties directly
+            for path, _value in dpath.util.search(connector_specification, f"**/properties/{forbidden_key}", yielded=True):
+                found_keys.remove(path)
+
+        if len(found_keys) > 0:
+            key_list = ", ".join(found_keys)
+            pytest.fail(f"Found the following disallowed JSON schema features: {key_list}")
+
+    def test_date_pattern(self, connector_specification: dict, detailed_logger):
+        """
+        Properties with format date or date-time should always have a pattern defined how the date/date-time should be formatted
+        that corresponds with the format the datepicker component is creating.
+        """
+        schema_helper = JsonSchemaHelper(connector_specification)
+        for format_path, format in dpath.util.search(connector_specification, "**/format", yielded=True):
+            if not isinstance(format, str):
+                # format is not a format definition here but a property named format
+                continue
+            property_definition = schema_helper.get_parent(format_path)
+            pattern = property_definition.get("pattern")
+            if format == "date" and not pattern == DATE_PATTERN:
+                detailed_logger.warning(
+                    f"{format_path} is defining a date format without the corresponding pattern. Consider setting the pattern to {DATE_PATTERN} to make it easier for users to edit this field in the UI."
+                )
+            if format == "date-time" and not pattern == DATETIME_PATTERN:
+                detailed_logger.warning(
+                    f"{format_path} is defining a date-time format without the corresponding pattern Consider setting the pattern to {DATETIME_PATTERN} to make it easier for users to edit this field in the UI."
+                )
+
+    def test_date_format(self, connector_specification: dict, detailed_logger):
+        """
+        Properties with a pattern that looks like a date should have their format set to date or date-time.
+        """
+        schema_helper = JsonSchemaHelper(connector_specification)
+        for pattern_path, pattern in dpath.util.search(connector_specification, "**/pattern", yielded=True):
+            if not isinstance(pattern, str):
+                # pattern is not a pattern definition here but a property named pattern
+                continue
+            if pattern == DATE_PATTERN or pattern == DATETIME_PATTERN:
+                property_definition = schema_helper.get_parent(pattern_path)
+                format = property_definition.get("format")
+                if not format == "date" and pattern == DATE_PATTERN:
+                    detailed_logger.warning(
+                        f"{pattern_path} is defining a pattern that looks like a date without setting the format to `date`. Consider specifying the format to make it easier for users to edit this field in the UI."
+                    )
+                if not format == "date-time" and pattern == DATETIME_PATTERN:
+                    detailed_logger.warning(
+                        f"{pattern_path} is defining a pattern that looks like a date-time without setting the format to `date-time`. Consider specifying the format to make it easier for users to edit this field in the UI."
+                    )
+
     def test_defined_refs_exist_in_json_spec_file(self, connector_spec_dict: dict):
         """Checking for the presence of unresolved `$ref`s values within each json spec file"""
         check_result = list(find_all_values_for_key_in_schema(connector_spec_dict, "$ref"))

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/json_schema_helper.py

@@ -6,6 +6,7 @@
 from functools import reduce
 from typing import Any, Dict, List, Mapping, Optional, Set, Text, Union
 
+import dpath.util
 import pendulum
 from jsonref import JsonRef
 
@@ -121,6 +122,16 @@ def get_node(self, path: List[str]) -> Any:
             node = node[segment]
         return node
 
+    def get_parent(self, path: str) -> Any:
+        """
+        Returns the parent dict of a given path within the `obj` dict
+        """
+        absolute_path = f"/{path}"
+        parent_path, _ = absolute_path.rsplit(sep="/", maxsplit=1)
+        if parent_path == "":
+            return self._schema
+        return dpath.util.get(self._schema, parent_path)
+
     def find_nodes(self, keys: List[str]) -> List[List[str]]:
         """Find all paths that lead to nodes with the specified keys.