SAT: all `additionalProperties` fields must be set to `true` #14878

alafanechere · 2022-07-20T13:02:12Z

What

Closing #14600
Add tests to make sure all the connector setting the additionalProperties field in their spec or stream schema set the value of this field to true. Check #14196 to understand why this is required.

How

Add a find_all_values_for_key_in_schema function to retrieve all the values for a specific field in a schema. (And unit test it)
Add two tests to test_core.py using this function to check all the values for the additionalProperties field are true:
- In TestSpec
- In TestDiscovery

🚨 User Impact 🚨

I think this will cause a lot of build failures because a lot of connectors are not following this rule and use "additionalProperties" : false.

I think we also need to update our tutorials and code generation template (*.hbs) that are using "additionalProperties": false. I created an issue for this.

…e set to True + write unit test

alafanechere · 2022-07-20T13:04:37Z

/test connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/2704924511
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/2704924511
No Python unittests run

Build Passed

Test summary info:

All Passed

alafanechere · 2022-07-20T13:06:44Z

I'm going to test if running test with the slash command uses a dev image of SAT and run a test for connectors that whose acceptance tests should failed on:

source-open-weather's spec has "additionalProperties": false
source-amazon-seller-partner has streams with "additionalProperties": false

alafanechere · 2022-07-20T13:07:01Z

/test connector=connectors/source-openweather

🕑 connectors/source-openweather https://github.com/airbytehq/airbyte/actions/runs/2704938055
❌ connectors/source-openweather https://github.com/airbytehq/airbyte/actions/runs/2704938055
🐛 https://gradle.com/s/7ymtq57svny6s

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_additional_properties_is_true[inputs0] - ...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestFullRefresh.test_sequential_reads because not found in the config
=================== 1 failed, 25 passed, 1 skipped in 21.35s ===================

alafanechere · 2022-07-20T13:07:15Z

/test connector=connectors/source-amazon-seller-partner

🕑 connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/2704939369
❌ connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/2704939369
🐛 https://gradle.com/s/ld3gpfmarbcvw

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestDiscovery::test_additional_properties_is_true[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestBasicRead.test_read because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestFullRefresh.test_sequential_reads because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
=================== 1 failed, 19 passed, 3 skipped in 6.56s ====================

alafanechere · 2022-07-20T13:17:04Z

☝️ Both test failures above are expected and show that the new tests work as expected.

pedroslopez

Nice job 👍🏼

There's an existing helper method that could have been used here, but I'll leave it up to you if you want to move over to that or keep the new one. My initial thought would be to use one or the other since they're so similar.

With regards to this change breaking builds for many connectors, do you know how many (python) sources are affected? We should check in with the dbs/dws team on priority for this existing issue #14718. If it will be done soon then maybe we hold off as to not impact publishes, if not then we could prioritize GA/Beta connectors and do those first.

pedroslopez · 2022-07-21T01:34:47Z

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

+def find_all_values_for_key_in_schema(schema: dict, searched_key: str):
+    """Retrieve all (nested) values in a schema for a specific searched key"""
+    if isinstance(schema, list):
+        for schema_item in schema:
+            yield from find_all_values_for_key_in_schema(schema_item, searched_key)
+    if isinstance(schema, dict):
+        for key, value in schema.items():
+            if key == searched_key:
+                yield value
+            if isinstance(value, dict) or isinstance(value, list):
+                yield from find_all_values_for_key_in_schema(value, searched_key)


It's not directly equivalent since this new method retrieves all values, but the existing find_key_inside_schema method can be used to find the first instance of a given key (e.g. find_key_inside_schema(schema, "additionalProperties") would return {"additionalProperties": False} or None if not found)

Yep I tried find_key_inside_schema, it's indeed working but only returns the first schema item having the searched key. I could have implemented a recursion in the final test function to use this function. But I preferred to use my new function to defer the recursion to the helper to boost the readability of the test. And this function can be helpful in the future if we have other keys we want to validate value on.

alafanechere · 2022-07-21T08:33:41Z

My initial thought would be to use one or the other since they're so similar.

@pedroslopez
I replaced find_key_inside_schema with find_all_values_for_key_in_schema in tests checking for $ref. Original unit tests on functions that used find_key_inside_schema are still passing.

alafanechere · 2022-07-21T09:34:47Z

do you know how many (python) sources are affected

I created a list I shared in #14718

alafanechere · 2022-07-21T14:20:18Z

/test connector=connectors/source-amplitude

🕑 connectors/source-amplitude https://github.com/airbytehq/airbyte/actions/runs/2712454558
❌ connectors/source-amplitude https://github.com/airbytehq/airbyte/actions/runs/2712454558
🐛 https://gradle.com/s/i6oqeh4t3uhw4

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_additional_properties_is_true[inputs0] - ...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:23: `future_state_path` not specified, skipping
============= 1 failed, 25 passed, 1 skipped in 191.57s (0:03:11) ==============

alafanechere · 2022-07-22T10:20:42Z

I made the required changes to beta and GA source (non-jdbc) connectors in #14924

alafanechere · 2022-07-22T10:23:20Z

/test connector=connectors/source-amplitude

🕑 connectors/source-amplitude https://github.com/airbytehq/airbyte/actions/runs/2717953897
✅ connectors/source-amplitude https://github.com/airbytehq/airbyte/actions/runs/2717953897
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        77      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/tests/test_incremental.py       121     25    79%
source_acceptance_test/utils/common.py                  77     17    78%
source_acceptance_test/tests/test_core.py              307    106    65%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
------------------------------------------------------------------------
TOTAL                                                  970    246    75%
Name                           Stmts   Miss  Cover
--------------------------------------------------
source_amplitude/source.py        24      0   100%
source_amplitude/__init__.py       2      0   100%
source_amplitude/errors.py        12      1    92%
source_amplitude/api.py          170     22    87%
--------------------------------------------------
TOTAL                            208     23    89%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:23: `future_state_path` not specified, skipping
================== 26 passed, 1 skipped in 149.73s (0:02:29) ===================

alafanechere · 2022-07-22T10:23:48Z

☝️ The test above should now pass after #14924 was merged

alafanechere · 2022-07-22T11:41:30Z

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/2718343998

Connector	Did it publish?	Were definitions generated?
bases/source-acceptance-test	❌	❌

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

alafanechere · 2022-07-22T12:48:45Z

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/2718686815

Connector	Did it publish?	Were definitions generated?
bases/source-acceptance-test	✅	❌

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

alafanechere added 5 commits July 20, 2022 11:00

add utils funtion to retrieve all values for a key in a schema

9384cee

move does_not_raise to conftest

43256e1

add new test in SAT to check that all additionalProperties in spec ar…

209bd95

…e set to True + write unit test

clean test for spec

678712d

add same test for streams

2581129

alafanechere linked an issue Jul 20, 2022 that may be closed by this pull request

SAT: Assert that additionalProperties: false is not specified in json schemas #14600

Closed

revert unrelated changes

331002c

alafanechere marked this pull request as ready for review July 20, 2022 13:11

alafanechere requested a review from pedroslopez July 20, 2022 13:11

alafanechere mentioned this pull request Jul 20, 2022

CDK and doc: use "additionalProperties": true instead of false #14879

Closed

alafanechere mentioned this pull request Jul 20, 2022

cdk and docs: remove "additionalProperties" #14881

Merged

pedroslopez approved these changes Jul 21, 2022

View reviewed changes

replace find_key_inside_schema with find_all_values_for_key_in_schema

edce556

alafanechere mentioned this pull request Jul 21, 2022

non-jdbc source connectors: Remove additional properties from beta/GA specs and schemas #14924

Merged

Merge branch 'master' into augustin/sat/not-additional-properties

a0b78ee

bump

1a02200

alafanechere merged commit 54b003b into master Jul 22, 2022

alafanechere deleted the augustin/sat/not-additional-properties branch July 22, 2022 13:16

octavia-squidington-iii mentioned this pull request Jul 23, 2022

Bump Airbyte version from 0.39.37-alpha to 0.39.38-alpha #14976

Merged

lazebnyi mentioned this pull request Jul 26, 2022

non-jdbc source connectors: Update additional properties from beta/GA specs and schemas #15042

Merged

alafanechere mentioned this pull request Aug 2, 2022

SAT: Syntactic validation of catalog update for backward compatibility #15057

Closed

monai mentioned this pull request Aug 12, 2022

🎉 Source Amazon Seller Partner: add restock inventory reports #15063

Merged

13 tasks

monai mentioned this pull request Aug 24, 2022

Source Zenloop: add new stream properties #15843

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAT: all `additionalProperties` fields must be set to `true` #14878

SAT: all `additionalProperties` fields must be set to `true` #14878

alafanechere commented Jul 20, 2022 •

edited

Loading

alafanechere commented Jul 20, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 20, 2022

alafanechere commented Jul 20, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 20, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 20, 2022

pedroslopez left a comment

pedroslopez Jul 21, 2022

alafanechere Jul 21, 2022

alafanechere commented Jul 21, 2022 •

edited

Loading

alafanechere commented Jul 21, 2022

alafanechere commented Jul 21, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 22, 2022 •

edited

Loading

alafanechere commented Jul 22, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 22, 2022

alafanechere commented Jul 22, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 22, 2022 •

edited by github-actions bot

Loading

SAT: all additionalProperties fields must be set to true #14878

SAT: all additionalProperties fields must be set to true #14878

Conversation

alafanechere commented Jul 20, 2022 • edited Loading

What

How

🚨 User Impact 🚨

alafanechere commented Jul 20, 2022 • edited by github-actions bot Loading

Build Passed

alafanechere commented Jul 20, 2022

alafanechere commented Jul 20, 2022 • edited by github-actions bot Loading

Build Failed

alafanechere commented Jul 20, 2022 • edited by github-actions bot Loading

Build Failed

alafanechere commented Jul 20, 2022

pedroslopez left a comment

Choose a reason for hiding this comment

pedroslopez Jul 21, 2022

Choose a reason for hiding this comment

alafanechere Jul 21, 2022

Choose a reason for hiding this comment

alafanechere commented Jul 21, 2022 • edited Loading

alafanechere commented Jul 21, 2022

alafanechere commented Jul 21, 2022 • edited by github-actions bot Loading

Build Failed

alafanechere commented Jul 22, 2022 • edited Loading

alafanechere commented Jul 22, 2022 • edited by github-actions bot Loading

Build Passed

alafanechere commented Jul 22, 2022

alafanechere commented Jul 22, 2022 • edited by github-actions bot Loading

alafanechere commented Jul 22, 2022 • edited by github-actions bot Loading

SAT: all `additionalProperties` fields must be set to `true` #14878

SAT: all `additionalProperties` fields must be set to `true` #14878

alafanechere commented Jul 20, 2022 •

edited

Loading

alafanechere commented Jul 20, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 20, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 20, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 21, 2022 •

edited

Loading

alafanechere commented Jul 21, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 22, 2022 •

edited

Loading

alafanechere commented Jul 22, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 22, 2022 •

edited by github-actions bot

Loading

alafanechere commented Jul 22, 2022 •

edited by github-actions bot

Loading