Issue 19733 cdk clarify config error message for config files #20019

maxi297 · 2022-12-02T13:28:58Z

What

This is a duplicate of #19931 but since it was opened from a fork, here is the correct pull request from the branch directly on airbyte repository. Some very relevant comments have already been added to #19931 and should be fixed in this pull request.

This change address #19733 which would cause the error message from reading config files to be unclear.

How

As #19733 states, We should rename BaseConnector.read_config() to read_json_file() to accurately reflect what it is doing. The existing usages of read_config() should be updated. and The Source.read_state() method should start using this newly renamed read_json_file() to read the state JSON file instead of duplicating all the same file reading logic.

🚨 User Impact 🚨

Previous error message: Expecting value: line 1 column 1 (char 0)
New error message: Could not read json file secrets/config.json: Expecting value: line 1 column 1 (char 0)

~~The solution proposed in #19733 is a breaking change since the public method read_config is changed to read_json_file.~~ We have decided to avoid this breaking change since source-s3 re-implements this method and we therefore can't reuse it for parsing the state.

Pre-merge Checklist

Community member or Airbyter

Unit & integration tests added and passing
Code reviews completed
PR name follows PR naming conventions

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

Other Notes

Running mypy airbyte_cdk (as mentioned in the README.md) before creating changes to the repository gave me the following output Found 134 errors in 60 files (checked 140 source files). Is this expected?

brianjlai

changes look good! just a reminder to bump the version after pulling the latest master and there a formatting error in the CI run since /sources/source.py no longer uses the json.loads() method so we don't need to import it anymore

evantahler · 2022-12-03T00:44:34Z

/test connector=connectors/source-s3

🕑 connectors/source-s3 https://github.com/airbytehq/airbyte/actions/runs/3606167448
❌ connectors/source-s3 https://github.com/airbytehq/airbyte/actions/runs/3606167448
🐛 https://gradle.com/s/7dto2nvwqw5ze

Build Failed

Test summary info:

	 =========================== short test summary info ============================
	 ERROR unit_tests/test_source.py::test_check_connection_exception - AttributeE...
	 ERROR unit_tests/test_source.py::test_check_connection - AttributeError: 'sup...
	 ERROR unit_tests/test_source.py::test_streams - AttributeError: 'super' objec...
	 FAILED unit_tests/test_source.py::test_transform_backslash_t_to_tab - Attribu...
	 �[31m======= �[31m�[1m1 failed�[0m, �[32m159 passed�[0m, �[33m10 warnings�[0m, �[31m�[1m3 errors�[0m�[31m in 65.53s (0:01:05)�[0m�[31m ========�[0m

…ge-for-config-files

maxi297 · 2022-12-05T20:12:51Z

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3623629132
✅ https://github.com/airbytehq/airbyte/actions/runs/3623629132

maxi297 · 2022-12-05T20:28:28Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3623737947
✅ https://github.com/airbytehq/airbyte/actions/runs/3623737947

maxi297 · 2022-12-05T20:52:00Z

/test connector=connectors/source-s3

🕑 connectors/source-s3 https://github.com/airbytehq/airbyte/actions/runs/3623886750
❌ connectors/source-s3 https://github.com/airbytehq/airbyte/actions/runs/3623886750
🐛 https://gradle.com/s/37zk6k3j4z4k2

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs1]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs2]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs3]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs4]
FAILED test_incremental.py::TestIncremental::test_two_sequential_reads[inputs5]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs1]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs2]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs3]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs4]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs5]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs1]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs2]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs3]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs4]
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs5]
============ 18 failed, 93 passed, 31 warnings in 227.12s (0:03:47) ============

roman-yermilov-gl · 2022-12-06T16:21:45Z

@maxi297 Could you please sync with latest master because of changes in airbyte_cdk?
#20143

…ge-for-config-files

airbyte-cdk/python/CHANGELOG.md

brianjlai

approving again! good work on the second iteration that makes it non-breaking but still let's us reuse the try/catch

airbyte-cdk/python/unit_tests/test_connector.py

maxi297 · 2022-12-06T22:29:42Z

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3634021709
❌ https://github.com/airbytehq/airbyte/actions/runs/3634021709

maxi297 · 2022-12-06T22:44:13Z

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3634109848
✅ https://github.com/airbytehq/airbyte/actions/runs/3634109848

maxi297 · 2022-12-06T22:59:20Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3634191159
✅ https://github.com/airbytehq/airbyte/actions/runs/3634191159

maxi297 · 2022-12-06T23:08:21Z

/test connector=connectors/source-s3

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

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     208     92    56%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-280, 288-301, 306-312, 319-330, 337-353
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1599    332    79%
Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
source_s3/source_files_abstract/formats/parquet_spec.py               9      0   100%
source_s3/source_files_abstract/formats/jsonl_spec.py                13      0   100%
source_s3/source_files_abstract/formats/csv_spec.py                  16      0   100%
source_s3/source_files_abstract/formats/avro_spec.py                  5      0   100%
source_s3/s3file.py                                                  37      0   100%
source_s3/s3_utils.py                                                20      0   100%
source_s3/__init__.py                                                 2      0   100%
source_s3/source_files_abstract/storagefile.py                       23      1    96%
source_s3/stream.py                                                  43      3    93%
source_s3/source_files_abstract/stream.py                           242     17    93%
source_s3/source_files_abstract/formats/abstract_file_parser.py      39      3    92%
source_s3/source.py                                                  27      4    85%
source_s3/source_files_abstract/formats/csv_parser.py                89     20    78%
source_s3/source_files_abstract/file_info.py                         26      8    69%
source_s3/utils.py                                                   31     10    68%
source_s3/source_files_abstract/source.py                            37     14    62%
source_s3/exceptions.py                                              10      4    60%
source_s3/source_files_abstract/spec.py                              44     22    50%
source_s3/source_files_abstract/formats/jsonl_parser.py              45     27    40%
source_s3/source_files_abstract/formats/avro_parser.py               39     25    36%
source_s3/source_files_abstract/formats/parquet_parser.py            64     44    31%
-------------------------------------------------------------------------------------
TOTAL                                                               861    202    77%
Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
source_s3/source_files_abstract/storagefile.py                       23      0   100%
source_s3/source_files_abstract/spec.py                              44      0   100%
source_s3/source_files_abstract/formats/parquet_spec.py               9      0   100%
source_s3/source_files_abstract/formats/jsonl_spec.py                13      0   100%
source_s3/source_files_abstract/formats/csv_spec.py                  16      0   100%
source_s3/source_files_abstract/formats/avro_spec.py                  5      0   100%
source_s3/source.py                                                  27      0   100%
source_s3/s3file.py                                                  37      0   100%
source_s3/s3_utils.py                                                20      0   100%
source_s3/exceptions.py                                              10      0   100%
source_s3/__init__.py                                                 2      0   100%
source_s3/source_files_abstract/formats/jsonl_parser.py              45      1    98%
source_s3/stream.py                                                  43      1    98%
source_s3/source_files_abstract/formats/abstract_file_parser.py      39      1    97%
source_s3/source_files_abstract/formats/parquet_parser.py            64      2    97%
source_s3/source_files_abstract/source.py                            37      2    95%
source_s3/source_files_abstract/formats/avro_parser.py               39      3    92%
source_s3/source_files_abstract/file_info.py                         26      3    88%
source_s3/source_files_abstract/stream.py                           242     40    83%
source_s3/source_files_abstract/formats/csv_parser.py                89     20    78%
source_s3/utils.py                                                   31     10    68%
-------------------------------------------------------------------------------------
TOTAL                                                               861     83    90%

Build Passed

Test summary info:

All Passed

maxi297 requested a review from a team December 2, 2022 13:28

octavia-squidington-iv added area/connectors Connector related issues CDK Connector Development Kit connectors/source/s3 labels Dec 2, 2022

maxi297 changed the title ~~Issue 19733 cdk clarify config error message for config files~~ 🚨🚨 Issue 19733 cdk clarify config error message for config files Dec 2, 2022

maxi297 mentioned this pull request Dec 2, 2022

🚨🚨 [ISSUE #19733] Executing a source with an invalid JSON file should display a clear error #19931

Closed

3 tasks

brianjlai approved these changes Dec 2, 2022

View reviewed changes

maxi297 added 5 commits December 2, 2022 14:42

[ISSUE-19733] clarify error message when reading config files

a5db9a3

[ISSUE #19733] code review and adding validation for spec file as well

91c3360

[ISSUE #19733] updating typing of read_json_file

5ab6b1a

[ISSUE #19733] fix flake8 error

0623bbb

[ISSUE #19733] fix linting error

3c16721

maxi297 force-pushed the issue-19733_cdk-clarify-config-error-message-for-config-files branch from ab1df04 to 3c16721 Compare December 2, 2022 19:42

Merge branch 'master' into issue-19733_cdk-clarify-config-error-messa…

40e08b2

…ge-for-config-files

roman-yermilov-gl mentioned this pull request Dec 6, 2022

airbyte-cdk: bump version #20143

Merged

[ISSUE #19733] remove breaking change

b9c933e

octavia-squidington-iv removed the area/connectors Connector related issues label Dec 6, 2022

maxi297 added 2 commits December 6, 2022 14:16

Merge branch 'master' into issue-19733_cdk-clarify-config-error-messa…

9e7c3dd

…ge-for-config-files

[ISSUE #19733] bump airbyte cdk version

bcae1d4

maxi297 commented Dec 6, 2022

View reviewed changes

airbyte-cdk/python/CHANGELOG.md Show resolved Hide resolved

maxi297 mentioned this pull request Dec 6, 2022

[ISSUE #15628] apply lookback window on earliest datetime between sta… #20156

Merged

brianjlai approved these changes Dec 6, 2022

View reviewed changes

airbyte-cdk/python/unit_tests/test_connector.py Show resolved Hide resolved

[ISSUE #19733] add test for invalid json file on read_state

67c55b8

maxi297 changed the title ~~🚨🚨 Issue 19733 cdk clarify config error message for config files~~ Issue 19733 cdk clarify config error message for config files Dec 6, 2022

[ISSUE #19733] bump version

a14bfa3

maxi297 merged commit 2568d61 into master Dec 6, 2022

maxi297 deleted the issue-19733_cdk-clarify-config-error-message-for-config-files branch December 6, 2022 23:55

This was referenced Dec 7, 2022

[CDK] Refactor reading JSON inputs for syncs under one shared method #19733

Closed

CDK: Improve error message if an input JSON cannot be deserialized #18946

Closed

octavia-squidington-iii mentioned this pull request Dec 7, 2022

Bump Airbyte version from 0.40.23 to 0.40.24 #20206

Merged

maxi297 mentioned this pull request Dec 16, 2022

[low-code] convert request.body to a dict when converting to AirbyteLogMessage #20557

Merged

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 19733 cdk clarify config error message for config files #20019

Issue 19733 cdk clarify config error message for config files #20019

maxi297 commented Dec 2, 2022 •

edited

Loading

brianjlai left a comment

evantahler commented Dec 3, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 5, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 5, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 5, 2022 •

edited by github-actions bot

Loading

roman-yermilov-gl commented Dec 6, 2022

brianjlai left a comment

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

Issue 19733 cdk clarify config error message for config files #20019

Issue 19733 cdk clarify config error message for config files #20019

Conversation

maxi297 commented Dec 2, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Tests

Other Notes

brianjlai left a comment

Choose a reason for hiding this comment

evantahler commented Dec 3, 2022 • edited by github-actions bot Loading

Build Failed

maxi297 commented Dec 5, 2022 • edited by github-actions bot Loading

maxi297 commented Dec 5, 2022 • edited by github-actions bot Loading

maxi297 commented Dec 5, 2022 • edited by github-actions bot Loading

Build Failed

roman-yermilov-gl commented Dec 6, 2022

brianjlai left a comment

Choose a reason for hiding this comment

maxi297 commented Dec 6, 2022 • edited by github-actions bot Loading

maxi297 commented Dec 6, 2022 • edited by github-actions bot Loading

maxi297 commented Dec 6, 2022 • edited by github-actions bot Loading

maxi297 commented Dec 6, 2022 • edited by github-actions bot Loading

Build Passed

maxi297 commented Dec 2, 2022 •

edited

Loading

evantahler commented Dec 3, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 5, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 5, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 5, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading

maxi297 commented Dec 6, 2022 •

edited by github-actions bot

Loading