Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 26, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67e3fc0
Revert "Extend SATs to capture UI limitations (#21451)"
girarda Jan 26, 2023
ea66cee
bump to 0.4.0
girarda Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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