From f41ed9fa050f21d5f522a6d2e3e392eecc2f5392 Mon Sep 17 00:00:00 2001 From: Alexandre Girard Date: Wed, 10 Aug 2022 17:05:01 -0700 Subject: [PATCH 1/7] unit test --- .../unit_tests/test_backward_compatibility.py | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py index 95e7c174a9668..8d17dd55ca210 100644 --- a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py @@ -513,6 +513,30 @@ def as_pytest_param(self): name="Not changing a spec should not fail", should_fail=False, ), + Transition( + ConnectorSpecification( + connectionSpecification={ + "type": "object", + "required": ["my_required_string"], + "additionalProperties": "false", + "properties": { + "my_required_string": {"type": "string"}, + }, + } + ), + ConnectorSpecification( + connectionSpecification={ + "type": "object", + "required": ["my_required_string"], + "additionalProperties": "true", + "properties": { + "my_required_string": {"type": "string"}, + }, + } + ), + name="Not changing a spec should not fail", + should_fail=False, + ), Transition( ConnectorSpecification( connectionSpecification={ @@ -898,7 +922,6 @@ def as_pytest_param(self): # Checking that all transitions in VALID_SPEC_TRANSITIONS have should_fail = False to prevent typos assert not all([transition.should_fail for transition in VALID_SPEC_TRANSITIONS]) - ALL_SPEC_TRANSITIONS_PARAMS = [transition.as_pytest_param() for transition in FAILING_SPEC_TRANSITIONS + VALID_SPEC_TRANSITIONS] @@ -1063,7 +1086,6 @@ def test_validate_previous_configs(previous_connector_spec, actual_connector_spe # Checking that all transitions in VALID_CATALOG_TRANSITIONS have should_fail = False to prevent typos assert not all([transition.should_fail for transition in VALID_CATALOG_TRANSITIONS]) - ALL_CATALOG_TRANSITIONS_PARAMS = [transition.as_pytest_param() for transition in FAILING_CATALOG_TRANSITIONS + VALID_CATALOG_TRANSITIONS] From e6d9d63f177a633ed39a3f34d615447e547028c2 Mon Sep 17 00:00:00 2001 From: Alexandre Girard Date: Wed, 10 Aug 2022 17:18:49 -0700 Subject: [PATCH 2/7] update unit test --- .../unit_tests/test_backward_compatibility.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py index 8d17dd55ca210..54863c86fe64b 100644 --- a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py @@ -518,7 +518,7 @@ def as_pytest_param(self): connectionSpecification={ "type": "object", "required": ["my_required_string"], - "additionalProperties": "false", + "additionalProperties": False, "properties": { "my_required_string": {"type": "string"}, }, @@ -528,13 +528,13 @@ def as_pytest_param(self): connectionSpecification={ "type": "object", "required": ["my_required_string"], - "additionalProperties": "true", + "additionalProperties": True, "properties": { "my_required_string": {"type": "string"}, }, } ), - name="Not changing a spec should not fail", + name="Changing the value of a boolean field should not fail", should_fail=False, ), Transition( From 6cab3d5a49a25e96c5e30d797a1e5294c67b559c Mon Sep 17 00:00:00 2001 From: Alexandre Girard Date: Wed, 10 Aug 2022 17:19:11 -0700 Subject: [PATCH 3/7] check list length --- .../source_acceptance_test/utils/backward_compatibility.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py index 26f5152c88a2e..20ce90ba13b76 100644 --- a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py @@ -40,7 +40,9 @@ def check_if_value_of_type_field_changed(self): # 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 self._diff.get("values_changed", []) if change.path(output_format="list")[-2] == "type" + change + for change in self._diff.get("values_changed", []) + if len(change.path(output_format="list")) > 1 and change.path(output_format="list")[-2] == "type" ] if type_values_changed or type_values_changed_in_list: self._raise_error("The'type' field value was changed.") From 3c1038dfd39793add6f6dfa9c6e82f878b360c3a Mon Sep 17 00:00:00 2001 From: Alexandre Girard Date: Wed, 10 Aug 2022 19:08:56 -0700 Subject: [PATCH 4/7] check if the field is additionalProperties --- .../source_acceptance_test/utils/backward_compatibility.py | 2 +- .../unit_tests/test_backward_compatibility.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py index 20ce90ba13b76..f9d94dc52a138 100644 --- a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py @@ -42,7 +42,7 @@ def check_if_value_of_type_field_changed(self): type_values_changed_in_list = [ change for change in self._diff.get("values_changed", []) - if len(change.path(output_format="list")) > 1 and change.path(output_format="list")[-2] == "type" + if change.path(output_format="list")[0] != "additionalProperties" and change.path(output_format="list")[-2] == "type" ] if type_values_changed or type_values_changed_in_list: self._raise_error("The'type' field value was changed.") diff --git a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py index 54863c86fe64b..91fbc1e63ee0e 100644 --- a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py @@ -534,7 +534,7 @@ def as_pytest_param(self): }, } ), - name="Changing the value of a boolean field should not fail", + name="Changing the value of additionalProperties should not fail", should_fail=False, ), Transition( From dccf9e106692902ec6afd8f3c4d6f612933c97b0 Mon Sep 17 00:00:00 2001 From: Alexandre Girard Date: Wed, 10 Aug 2022 19:13:12 -0700 Subject: [PATCH 5/7] comment --- .../source_acceptance_test/utils/backward_compatibility.py | 1 + 1 file changed, 1 insertion(+) diff --git a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py index f9d94dc52a138..8078799a80ab4 100644 --- a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py @@ -39,6 +39,7 @@ def check_if_value_of_type_field_changed(self): type_values_changed = [change for change in self._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"]): + # Skip changes to additionalProperties because it's not a spec property type_values_changed_in_list = [ change for change in self._diff.get("values_changed", []) From 6e4c1496fdccf9f1ecb88d7347ec9cfd3f71f99d Mon Sep 17 00:00:00 2001 From: alafanechere Date: Thu, 11 Aug 2022 08:39:23 +0200 Subject: [PATCH 6/7] only check type diff on properties children --- .../bases/source-acceptance-test/CHANGELOG.md | 3 +++ .../bases/source-acceptance-test/Dockerfile | 2 +- .../utils/backward_compatibility.py | 24 ++++++++--------- .../unit_tests/test_backward_compatibility.py | 26 ++++++++++++++++++- 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md b/airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md index e19a423358cd1..4f7362b1ccc6d 100644 --- a/airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md +++ b/airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## 0.1.63 +Don't fail on updating `additionalProperties`: fix IndexError [#15532](https://github.com/airbytehq/airbyte/pull/15532/) + ## 0.1.62 Backward compatibility tests: add syntactic validation of catalogs [#15486](https://github.com/airbytehq/airbyte/pull/15486/) diff --git a/airbyte-integrations/bases/source-acceptance-test/Dockerfile b/airbyte-integrations/bases/source-acceptance-test/Dockerfile index 2fdc11fa3f01c..dd99c21d3274c 100644 --- a/airbyte-integrations/bases/source-acceptance-test/Dockerfile +++ b/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.62 +LABEL io.airbyte.version=0.1.63 LABEL io.airbyte.name=airbyte/source-acceptance-test ENTRYPOINT ["python", "-m", "pytest", "-p", "source_acceptance_test.plugin", "-r", "fEsx"] diff --git a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py index 8078799a80ab4..b123deea1353c 100644 --- a/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/backward_compatibility.py @@ -34,18 +34,12 @@ def assert_is_backward_compatible(self): # pragma: no cover pass def check_if_value_of_type_field_changed(self): - """Check if a type was changed""" + """Check if a type was changed on a property""" # Detect type value change in case type field is declared as a string (e.g "str" -> "int"): - type_values_changed = [change for change in self._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"]): - # Skip changes to additionalProperties because it's not a spec property - type_values_changed_in_list = [ - change - for change in self._diff.get("values_changed", []) - if change.path(output_format="list")[0] != "additionalProperties" and change.path(output_format="list")[-2] == "type" + changes_on_property_type = [ + change for change in self._diff.get("values_changed", []) if {"properties", "type"}.issubset(change.path(output_format="list")) ] - if type_values_changed or type_values_changed_in_list: + if changes_on_property_type: self._raise_error("The'type' field value was changed.") def check_if_new_type_was_added(self): # pragma: no cover @@ -61,7 +55,7 @@ def check_if_new_type_was_added(self): # pragma: no cover def check_if_type_of_type_field_changed(self): """ - Detect the change of type of a type field + Detect the change of type of a type field on a property e.g: - "str" -> ["str"] VALID - "str" -> ["str", "null"] VALID @@ -71,7 +65,9 @@ def check_if_type_of_type_field_changed(self): - ["str"] -> "int" INVALID - ["str"] -> 1 INVALID """ - type_changes = [change for change in self._diff.get("type_changes", []) if change.path(output_format="list")[-1] == "type"] + type_changes = [ + change for change in self._diff.get("type_changes", []) if {"properties", "type"}.issubset(change.path(output_format="list")) + ] 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. @@ -124,7 +120,9 @@ def check_if_added_a_new_required_property(self): def check_if_field_was_made_not_nullable(self): """Detect when field was made not nullable but is still a list: e.g ["string", "null"] -> ["string"]""" removed_nullable = [ - change for change in self._diff.get("iterable_item_removed", []) if change.path(output_format="list")[-2] == "type" + change + for change in self._diff.get("iterable_item_removed", []) + if {"properties", "type"}.issubset(change.path(output_format="list")) ] if removed_nullable: self._raise_error("A field type was narrowed or made a field not nullable") diff --git a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py index 91fbc1e63ee0e..b64fa3cc72581 100644 --- a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py @@ -534,7 +534,31 @@ def as_pytest_param(self): }, } ), - name="Changing the value of additionalProperties should not fail", + name="Top level: Changing the value of additionalProperties should not fail", + should_fail=False, + ), + Transition( + ConnectorSpecification( + connectionSpecification={ + "type": "object", + "properties": { + "my_nested_object": {"type": "object", "properties": {"my_property": {"type": ["integer"]}}}, + }, + } + ), + ConnectorSpecification( + connectionSpecification={ + "type": "object", + "properties": { + "my_nested_object": { + "type": "object", + "additionalProperties": True, + "properties": {"my_property": {"type": ["integer"]}}, + }, + }, + } + ), + name="Nested level: Changing the value of additionalProperties should not fail", should_fail=False, ), Transition( From d90c470fdfb499ccb7b49b3f2308594d5d3585ec Mon Sep 17 00:00:00 2001 From: alafanechere Date: Thu, 11 Aug 2022 09:04:27 +0200 Subject: [PATCH 7/7] increase number of fake generated config to 200 --- .../unit_tests/test_backward_compatibility.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py index b64fa3cc72581..96ea38b49052f 100644 --- a/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py +++ b/airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py @@ -966,7 +966,7 @@ def test_spec_backward_compatibility(previous_connector_spec, actual_connector_s def test_validate_previous_configs(previous_connector_spec, actual_connector_spec, should_fail): expectation = pytest.raises(NonBackwardCompatibleError) if should_fail else does_not_raise() with expectation: - validate_previous_configs(previous_connector_spec, actual_connector_spec, 100) + validate_previous_configs(previous_connector_spec, actual_connector_spec, 200) FAILING_CATALOG_TRANSITIONS = [