Skip to content

Commit

Permalink
Revert "Extend SATs to capture UI limitations (#21451)" (#21896)
Browse files Browse the repository at this point in the history 
* Revert "Extend SATs to capture UI limitations (#21451)"

This reverts commit fa2c03a.

* bump to 0.4.0
  • Loading branch information
@girarda
girarda authored Jan 26, 2023
1 parent 3d92cb6 commit 74b5dbf
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 373 deletions.
5 changes: 4 additions & 1 deletion airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
# Changelog

## 0.4.0
Revert 0.3.0

## 0.3.0
Add various stricter checks for specs (see PR for details). [#21451](https://github.com/airbytehq/airbyte/pull/21451)
(Broken) 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)
Expand Down
2 changes: 1 addition & 1 deletion airbyte-integrations/bases/source-acceptance-test/Dockerfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ COPY pytest.ini setup.py ./
COPY source_acceptance_test ./source_acceptance_test
RUN pip install .

LABEL io.airbyte.version=0.3.0
LABEL io.airbyte.version=0.4.0
LABEL io.airbyte.name=airbyte/source-acceptance-test

ENTRYPOINT ["python", "-m", "pytest", "-p", "source_acceptance_test.plugin", "-r", "fEsx"]
154 changes: 10 additions & 144 deletions airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,12 @@ 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,
Expand Down Expand Up @@ -227,15 +219,22 @@ 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.
Secret objects and arrays can not be rendered correctly in the UI:
Objects and arrays can hold a secret in case they are generic,
meaning their inner structure is not described in details with properties/items.
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),
]
)
Expand All @@ -253,7 +252,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, type_value in dpath.util.search(spec_properties, "**/type", yielded=True):
for type_path, 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
Expand All @@ -269,7 +268,7 @@ def test_secret_is_properly_marked(self, connector_spec_dict: dict, detailed_log

if non_secrets_hidden:
properties = "\n".join(non_secrets_hidden)
pytest.fail(
detailed_logger.warning(
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}"""
Expand All @@ -281,139 +280,6 @@ 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"))
Expand Down
11 changes: 0 additions & 11 deletions ...egrations/bases/source-acceptance-test/source_acceptance_test/utils/json_schema_helper.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from functools import reduce
from typing import Any, Dict, List, Mapping, Optional, Set, Text, Union

import dpath.util
import pendulum
from jsonref import JsonRef

Expand Down Expand Up @@ -122,16 +121,6 @@ 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.
Expand Down
Loading

0 comments on commit 74b5dbf

Please sign in to comment.