Skip to content

Commit

Permalink
SAT: check oneOf common property has only const keyword, no `defaul…
Browse files Browse the repository at this point in the history 
…t` and `enum` keywords (#11704)

* improve test_oneof_usage

Signed-off-by: Sergey Chvalyuk <[email protected]>
Co-authored-by: Sherif A. Nada <[email protected]>
  • Loading branch information
@grubberr @sherifnada
grubberr and sherifnada authored Apr 7, 2022
1 parent d778ac7 commit 6092bee
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 11 deletions.
3 changes: 3 additions & 0 deletions airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog

## 0.1.48
Add checking that oneOf common property has only `const` keyword, no `default` and `enum` keywords: [#11704](https://github.com/airbytehq/airbyte/pull/11704)

## 0.1.47
Added local test success message containing git hash: [#11497](https://github.com/airbytehq/airbyte/pull/11497)

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.1.47
LABEL io.airbyte.version=0.1.48
LABEL io.airbyte.name=airbyte/source-acceptance-test

ENTRYPOINT ["python", "-m", "pytest", "-p", "source_acceptance_test.plugin", "-r", "fEsx"]
26 changes: 21 additions & 5 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 @@ -88,12 +88,28 @@ def test_oneof_usage(self, actual_connector_spec: ConnectorSpecification):
for variant in variants:
assert "properties" in variant, f"Each item in the oneOf array should be a property with type object. {docs_msg}"

variant_props = [set(list(v["properties"].keys())) for v in variants]
oneof_path = ".".join(variant_path)
variant_props = [set(v["properties"].keys()) for v in variants]
common_props = set.intersection(*variant_props)
assert common_props, "There should be at least one common property for oneOf subobjects"
assert any(
[all(["const" in var["properties"][prop] for var in variants]) for prop in common_props]
), f"Any of {common_props} properties in {'.'.join(variant_path)} has no const keyword. {docs_msg}"
assert common_props, f"There should be at least one common property for {oneof_path} subobjects. {docs_msg}"

const_common_props = set()
for common_prop in common_props:
if all(["const" in variant["properties"][common_prop] for variant in variants]):
const_common_props.add(common_prop)
assert (
len(const_common_props) == 1
), f"There should be exactly one common property with 'const' keyword for {oneof_path} subobjects. {docs_msg}"

const_common_prop = const_common_props.pop()
for n, variant in enumerate(variants):
prop_obj = variant["properties"][const_common_prop]
assert (
"default" not in prop_obj
), f"There should not be 'default' keyword in common property {oneof_path}[{n}].{const_common_prop}. Use `const` instead. {docs_msg}"
assert (
"enum" not in prop_obj
), f"There should not be 'enum' keyword in common property {oneof_path}[{n}].{const_common_prop}. Use `const` instead. {docs_msg}"

def test_required(self):
"""Check that connector will fail if any required field is missing"""
Expand Down
84 changes: 84 additions & 0 deletions airbyte-integrations/bases/source-acceptance-test/unit_tests/test_spec.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,90 @@ def parametrize_test_case(*test_cases: Dict[str, Any]) -> Callable:
},
"should_fail": True,
},
{
"test_id": "no_common_property_for_all_oneof_subobjects",
"connector_spec": {
"type": "object",
"properties": {
"credentials": {
"type": "object",
"oneOf": [
{
"type": "object",
"properties": {
"option1": {"type": "string"},
"option2": {"type": "string"},
},
},
{
"type": "object",
"properties": {
"option3": {"type": "string"},
"option4": {"type": "string"},
},
},
],
}
},
},
"should_fail": True,
},
{
"test_id": "two_common_properties_with_const_keyword",
"connector_spec": {
"type": "object",
"properties": {
"credentials": {
"type": "object",
"oneOf": [
{
"type": "object",
"properties": {
"common1": {"type": "string", "const": "common1"},
"common2": {"type": "string", "const": "common2"},
},
},
{
"type": "object",
"properties": {
"common1": {"type": "string", "const": "common1"},
"common2": {"type": "string", "const": "common2"},
},
},
],
}
},
},
"should_fail": True,
},
{
"test_id": "default_keyword_in_common_property",
"connector_spec": {
"type": "object",
"properties": {
"credentials": {
"type": "object",
"oneOf": [
{
"type": "object",
"properties": {
"common": {"type": "string", "const": "option1", "default": "option1"},
"option1": {"type": "string"},
},
},
{
"type": "object",
"properties": {
"common": {"type": "string", "const": "option2", "default": "option2"},
"option2": {"type": "string"},
},
},
],
}
},
},
"should_fail": True,
},
)
def test_oneof_usage(connector_spec, should_fail):
t = _TestSpec()
Expand Down
6 changes: 1 addition & 5 deletions docs/connector-development/connector-specification-reference.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ In order for the Airbyte UI to correctly render a specification, however, a few

Let's look at the [source-file](../integrations/sources/file.md) implementation as an example. In this example, we have `provider` as a dropdown list option, which allows the user to select what provider their file is being hosted on. We note that the `oneOf` keyword lives under the `provider` object as follows:

In each item in the `oneOf` array, the `option_title` string field exists with the aforementioned `const`, `default` and `enum` value unique to that item. There is a [Github issue](https://github.com/airbytehq/airbyte/issues/6384) to improve it and use only `const` in the specification. This helps the UI and the connector distinguish between the option that was chosen by the user. This can be displayed with adapting the file source spec to this example:
In each item in the `oneOf` array, the `option_title` string field exists with the aforementioned `const` value unique to that item. This helps the UI and the connector distinguish between the option that was chosen by the user. This can be displayed with adapting the file source spec to this example:

```javascript
{
Expand Down Expand Up @@ -127,8 +127,6 @@ In each item in the `oneOf` array, the `option_title` string field exists with t
"option_title": {
"type": "string",
"const": "HTTPS: Public Web",
"enum": ["HTTPS: Public Web"],
"default": "HTTPS: Public Web",
"order": 0
}
}
Expand All @@ -141,8 +139,6 @@ In each item in the `oneOf` array, the `option_title` string field exists with t
"option_title": {
"type": "string",
"const": "GCS: Google Cloud Storage",
"enum": ["GCS: Google Cloud Storage"],
"default": "GCS: Google Cloud Storage",
"order": 0
},
"service_account_json": {
Expand Down

0 comments on commit 6092bee

Please sign in to comment.