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

Jump to bottom

[Low-Code CDK] Separate request path from RequestOption component #22398

Merged
merged 7 commits into from
Feb 8, 2023
Merged

[Low-Code CDK] Separate request path from RequestOption component #22398

merged 7 commits into from
Feb 8, 2023

Conversation

brianjlai
Copy link
Contributor

@brianjlai brianjlai commented Feb 6, 2023 •
edited
Loading

Closes #20792 #21095

What

This PR addresses a variety of the issues brought up in the linked tickets.

  • The usability of the RequestOptions component is not very intuitive because request options are interspersed with request path injection so it is not explicitly clear what combinations will fail and run time. By splitting these components apart we can make the schema more clear and make it more obvious how to configure these components
  • Fixes a bug with how the substream slicer injects request options

How

This introduces a breaking change where any usage of the path RequestOption has been moved into its own dedicated component RequestPath. With this new component, we can tear out a lot of the internal validation used during component initialization because we can catch it ahead of time while parsing manifests into Pydantic models. This is reflected in how the schema has changed.

Fixes a small bug where we were accidentally injecting request options using the stream_slice_field instead of the request option on outbound requests. The only reason this wasn't caught was the only usage of this was on source-braze where both fields were the same.

I manually replaced inject_into: path with the component definition since there were not too many uses. And I did some manual replacement via pycharm to explicitly mark the type: RequestOption because a default value makes a lot less sense for a paginator where there are two options now.

Recommended reading order

  1. request_path.py
  2. request_option.py
  3. declarative_component_schema.yaml
  4. default_paginatory.py
  5. datetime_stream_slicer.py
  6. substream_slicer.py
  7. list_stream_slicer.py

brianjlai
brianjlai commented Feb 6, 2023
View reviewed changes
airbyte-cdk/python/airbyte_cdk/sources/declarative/requesters/request_option.py
inject_into: RequestOptionType
parameters: InitVar[Mapping[str, Any]]
field_name: Optional[str] = None

def __post_init__(self, parameters: Mapping[str, Any]):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original implementation, only path RequestOption could skip field_name. Now that its been extracted, field_name is required and we can get rid of this entire validation

brianjlai
brianjlai commented Feb 6, 2023
View reviewed changes
airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py
@@ -31,7 +31,6 @@
# DefaultPaginator
"DefaultPaginator.decoder": "JsonDecoder",
"DefaultPaginator.page_size_option": "RequestOption",
"DefaultPaginator.page_token_option": "RequestOption",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting rid of a default type because this could be a RequestPath now

@brianjlai brianjlai marked this pull request as ready for review February 6, 2023 07:03
@brianjlai brianjlai requested a review from a team as a code owner February 6, 2023 07:03
@brianjlai brianjlai mentioned this pull request Feb 6, 2023
Closed
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Feb 7, 2023
clnoll
clnoll approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@clnoll clnoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @brianjlai! Just one question, not a blocker.

airbyte-cdk/python/unit_tests/sources/declarative/requesters/paginators/test_request_option.py
@@ -9,15 +9,9 @@
@pytest.mark.parametrize(
"test_name, option_type, field_name, should_raise",
[
("test_limit_path_no_field_name", RequestOptionType.path, None, False),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like none of these should_raise anymore so the test body can be updated.

That said, it's not immediately obvious to me why the non-.path cases are being deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes good point i'll clean this test up

@brianjlai brianjlai merged commit 1fd1852 into low_code_cdk_to_beta Feb 8, 2023
@brianjlai brianjlai deleted the brian/split_request_option branch February 8, 2023 04:00
@flash1293 flash1293 mentioned this pull request Feb 13, 2023
Merged
brianjlai added a commit that referenced this pull request Feb 16, 2023
@brianjlai @maxi297
@clnoll @lmossman
Low code cdk to beta (#22853)
1c482ee 
* [ISSUE #19410] remove request_options_provider from the … (#21403)

* [ISSUE #19410] (incomplete) remove request_options_provider from the manifest

* [ISSUE #19410] (incomplete) incomplete cleanup config_component_schema.json as well

* [ISSUE #19410] update source-monday

* [ISSUE #19410] code review

* [ISSUE #19410] formatting files

* [Low-Code CDK] Replace the $options keyword with $parameters (#21632)

* refactor flows and tests to use parameters instead of options

* update documentation to reflect the change from options to parameters

* create migration script to replace options with parameters in existing manifests

* update template to use parameters instead of options

* fix tests after rebasing from the branch

* address pr feedback and extra uses of options that I missed

* additional changes needed after rebasing from master

* migrate low-code connectors to use parameters instead of options

* 🚨🚨 [Low Code CDK] Update `*ref` format to `#/` (#21434)

* [Low-Code CDK] Remove JsonSchema type in favor of JsonSchemaFileLoader (#21832)

* fully deprecate JsonSchema in favor of JsonFileSchemaLoader

* remove usage in the legacy registry

* Update migration scripts according to manifest file rename (#21920)

* Issue 21866 remove legacy factory and validation flow (#21878)

* [ISSUE #21866] clean ManifestDeclarativeSource validation

* [ISSUE #21866] remove dataclasses-jsonschema

* [ISSUE #21866] code review

* [ISSUE-21866] flake8

* [ISSUE #21559] remove DefaultPaginator.url_base (#21823)

* [ISSUE #21559] remove DefaultPaginator.url_base

* [ISSUE #21559] code review

* [ISSUE #21559] update migration script

* [ISSUE #21559] code review

* [ISSUE #21559] update documentation

* [ISSUE #21559] run migration (#21824)

* [ISSUE #21559] remove DefaultPaginator.url_base (#21823)

* [ISSUE #21559] remove DefaultPaginator.url_base

* [ISSUE #21559] code review

* [ISSUE #21559] update migration script

* [ISSUE #21559] code review

* [ISSUE #21559] update documentation

* [ISSUE #21559] run migration (#21824)

* [ISSUE #21559] fix manifests

* [ISSUE #21926] setup server to allow for local tests (#21974)

* [Low Code CDK] remove checkpoint_interval from DeclarativeStream component (#22120)

* Issue #21576 rename dpathextractor fieldpointer (#21990)

* [ISSUE #21926] setup server to allow for local tests

* [ISSUE #21576] Rename DpathExtractor.field_pointer to field_path

* [ISSUE #21576] migration script

* [ISSUE #21576] update source-monday and source-pocket as well

* [ISSUE #21576] migration (#21997)

* [ISSUE #21576] code review

* Remove checkpoint_interval from source-prestashop manifest (#22141)

* replacing options with parameters for a few connectors I missed or were newly added

* [Low-Code CDK] Rremove stream_cursor_field from stream and derive it from stream_slicer (#22294)

* update schema to derive cursor_field from a stream slicer if it exists

* remove usage of stream_cursor_field on simple connector use cases

* fixing some of the more complex usage of stream_cursor_field that rely on cartesian product stream slicers

* fix documentation to replace references to stream_cursor_field

* Low Code CDK: Remove `name` and `primary_key` from non-DeclarativeStream components (#21891)

* fix eslint issues for webapp (#22462)

* 🪟 🔧 Connector Builder frontend fixes for low_code_cdk_to_beta (#22375)

* bump connector builder server to latest CDK version

* fix breaking CDK changes in connector builder FE

* [Low-Code CDK] Separate request path from RequestOption component (#22398)

* split apart path from RequestOption and fix usages and cleanup the code

* replace usage of path with RequestPath and get rid of default to RequestOption

* fix bug where stream_slice_field was used in outbound request instead of request_option field_name

* organize yaml schema names and update documentation for RequestOption and RequestPath

* clean up tests

* regenerate models

* [ISSUE #19961] refactor stream slices (#22225)

* [ISSUE #19961] add 'incremental' and partially remove CartesianProductStreamSlicer - Google PageSpeed Insights not working yet

* [ISSUE #19961] fixing Google PageSpeed Insights

* move incremental_sync field to the stream level and perform merging into one stream slicer at that level

* add tests to merging incremental and iterable into cartesian

* rewrite documentation to separate incremental sync and iterator concepts

* update documentation to use partition router and revise the tutorial to reflect the new changes to the components

* [ISSUE #19961] update code to newest CDK version and clean autogenerated files (#22670)

* [ISSUE #19961] rename stream_slicer to partition_router and update ma… (#22590)

* [ISSUE #19961] rename stream_slicer to partition_router and update manifests (for incremental_sync as well)

* [ISSUE 19961] rename CustomStreamSlicer (#22598)

* [ISSUE 19961] rename CustomStreamSlicer

* [ISSUE #19961] code review CustomStreamSlicer

* [ISSUE #19961] fix source_square incremental sync

* [ISSUE #19961] rename SingleSlice to SinglePartitionRouter (#22591)

* [ISSUE #19961] rename SingleSlice to SinglePartitionRouter

* remove SinglePartitionRouter from the schema

---------

Co-authored-by: brianjlai <brian.lai@airbyte.io>

* [ISSUE #19961] rename SubstreamSlicer  to SubstreamPartitionRouter (#22596)

* [ISSUE #19961] TMP rename SubstreamSlicer  to SubstreamPartitionRouter

* [ISSUE #19961] revert DatetimeStreamSlicer.stream_state_field_start and DatetimeStreamSlicer.stream_state_field_end

* [ISSUE #19961] rename ListStreamSlicer to ListPartitionRouter (#22593)

---------

Co-authored-by: brianjlai <brian.lai@airbyte.io>

* [ISSUE #19961] clean faulty merge

* [ISSUE #19961] rename DatetimeStreamSlicer (#22617)

* [ISSUE #19961] rename stream_slicer to partition_router and update manifests (for incremental_sync as well)

* [ISSUE 19961] rename CustomStreamSlicer (#22598)

* [ISSUE 19961] rename CustomStreamSlicer

* [ISSUE #19961] code review CustomStreamSlicer

* [ISSUE #19961] fix source_square incremental sync

* [ISSUE #19961] rename SingleSlice to SinglePartitionRouter (#22591)

* [ISSUE #19961] rename SingleSlice to SinglePartitionRouter

* remove SinglePartitionRouter from the schema

---------

Co-authored-by: brianjlai <brian.lai@airbyte.io>

* [ISSUE #19961] rename DatetimeStreamSlicer

* [ISSUE #19961] rename SubstreamSlicer  to SubstreamPartitionRouter (#22596)

* [ISSUE #19961] TMP rename SubstreamSlicer  to SubstreamPartitionRouter

* [ISSUE #19961] revert DatetimeStreamSlicer.stream_state_field_start and DatetimeStreamSlicer.stream_state_field_end

* [ISSUE #19961] rename ListStreamSlicer to ListPartitionRouter (#22593)

---------

Co-authored-by: brianjlai <brian.lai@airbyte.io>

* Update docs/connector-development/config-based/understanding-the-yaml-file/partition-router.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* Update docs/connector-development/config-based/understanding-the-yaml-file/partition-router.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* Update docs/connector-development/config-based/understanding-the-yaml-file/yaml-overview.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* Update docs/connector-development/config-based/understanding-the-yaml-file/partition-router.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* Update docs/connector-development/config-based/understanding-the-yaml-file/partition-router.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* Update docs/connector-development/config-based/understanding-the-yaml-file/partition-router.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* Update docs/connector-development/config-based/understanding-the-yaml-file/incremental-syncs.md

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>

* update docs

* [ISSUE #19961] clean unit tests files

* [ISSUE #19961] code review

---------

Co-authored-by: brianjlai <brian.lai@airbyte.io>
Co-authored-by: Brian Lai <51336873+brianjlai@users.noreply.github.com>

* [Low-Code CDK] Allow for children of custom components to specify parameters that are normally derived (#22379)

* Fix a bug where child components of a custom component cannot receive fields from other components

* add tests, documentation and commenting

* fix test from merge

* add better error message for nested initialization failures

* 🪟 🔧 Connector Builder frontend fixes for low_code_cdk_to_beta (#22880)

* restrict name to stream level

* remove checkpoint interval

* adjust logic for new request options

* refactor slicers

* wording

* review comments

* make oldest supported version explicit

* separate the frontend and connector builder changes from the low-code to beta release

* [Low-Code CDK] Add script to run low code unit tests and address issues with a few connectors (#23123)

* consolidate all the changes into a new PR after I messed up the merge on the side branch

* add set to allow this to be called externally if necessary later

* remove last few extra fields i found and fix docs links

* fix docs one more time

---------

Co-authored-by: Maxime Carbonneau-Leclerc <maxi297@users.noreply.github.com>
Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com>
Co-authored-by: maxi297 <maxime@airbyte.io>
Co-authored-by: Lake Mossman <lake@airbyte.io>
Co-authored-by: Joe Reuter <joe@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clnoll clnoll clnoll approved these changes

@maxi297 maxi297 Awaiting requested review from maxi297

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/source/activecampaign connectors/source/aha connectors/source/ashby connectors/source/braze connectors/source/callrail connectors/source/chartmogul connectors/source/clickup-api connectors/source/close-com connectors/source/coinmarketcap connectors/source/convertkit connectors/source/courier connectors/source/datascope connectors/source/delighted connectors/source/emailoctopus connectors/source/facebook-pages connectors/source/getlago connectors/source/gocardless connectors/source/gong connectors/source/greenhouse connectors/source/gutendex connectors/source/intruder connectors/source/k6-cloud connectors/source/launchdarkly connectors/source/lokalise connectors/source/mailerlite connectors/source/mailersend connectors/source/mailjet-mail connectors/source/mailjet-sms connectors/source/n8n connectors/source/news-api connectors/source/newsdata connectors/source/omnisend connectors/source/oura connectors/source/partnerstack connectors/source/pexels-api connectors/source/pocket connectors/source/posthog connectors/source/postmarkapp connectors/source/punk-api connectors/source/recreation connectors/source/reply-io connectors/source/rocket-chat connectors/source/secoda connectors/source/sendgrid connectors/source/sendinblue connectors/source/senseforce connectors/source/sentry connectors/source/smaily connectors/source/sonar-cloud connectors/source/square connectors/source/statuspage connectors/source/survey-sparrow connectors/source/tempo connectors/source/the-guardian-api connectors/source/tmdb connectors/source/toggl connectors/source/twilio-taskrouter connectors/source/twitter connectors/source/vantage connectors/source/vitally connectors/source/woocommerce connectors/source/workable connectors/source/workramp connectors/source/zenloop connectors/source/zoom
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brianjlai @clnoll @octavia-squidington-iii