Skip to content

CDK: don't filter failed interpolated vars for request_options_provider.request_body_json #19297

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.

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

Merged
merged 11 commits into from
Nov 11, 2022

Conversation

grubberr
Copy link
Contributor

@grubberr grubberr commented Nov 10, 2022

Signed-off-by: Sergey Chvalyuk [email protected]

What

In current implementation of Interpolation this does not work:

request_options_provider:
    request_body_json:
       key1: "{{ False }}"
       key2: "{{ 0 }}"

key1, key2 will be removed because of this:
https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/python/airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_input_provider.py#L48

I propose do this "empty value" filtering only for:
request_options_provider.get_request_params
request_options_provider.get_request_headers
request_options_provider.get_request_body_data

but for request_options_provider.request_body_json skip this filtering

@grubberr grubberr requested a review from a team November 10, 2022 18:51
@grubberr grubberr self-assigned this Nov 10, 2022
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr changed the title CDK: skip dict filtering for request_options_provider.request_body_json CDK: don't filter failed interpolated vars for request_options_provider.request_body_json Nov 10, 2022
("test_integer_falsy_value", {"integer_falsy": "{{ 0 }}"}, {"integer_falsy": 0}),
("test_number_falsy_value", {"number_falsy": "{{ 0.0 }}"}, {"number_falsy": 0.0}),
("test_string_falsy_value", {"string_falsy": "{{ '' }}"}, {"string_falsy": None}),
("test_none_value", {"none_value": "{{ None }}"}, {"none_value": None}),
Copy link
Contributor

Choose a reason for hiding this comment

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

when would we want to return None in the request body?

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 theory I can imagine something like this:

curl -d '{"key1":null}' -X POST http://api.com/api

But to be honest we did not encounter in this for now. For now we need only this:

curl -d '{"key1":false}' -X POST http://api.com/api

I have re-implemented doing less radical changes
What do you think now?

Signed-off-by: Sergey Chvalyuk <[email protected]>
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Nov 11, 2022
@grubberr grubberr requested a review from girarda November 11, 2022 04:10
@grubberr
Copy link
Contributor Author

grubberr commented Nov 11, 2022

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3443615837
https://github.com/airbytehq/airbyte/actions/runs/3443615837

Signed-off-by: Sergey Chvalyuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CDK: don't filter failed interpolated vars for request_options_provider.request_body_json
3 participants