Skip to content

Conversation

@josh-fell
Copy link
Contributor

Related: #22607

There are a few connections which have default values for particular extra fields namely Google (extra__google_cloud_platform__num_retries: 5), Kubernetes (extra__kubernetes__in_cluster: False), and Snowflake (extra__snowflake__insecure_mode: False). Since all extra fields are available in the form data, a check is needed to only add extra fields that are germane to the connection type of the submission. Otherwise, the extras with default values could be added to a connection's URI and extra_dejson incorrectly.

For example, before this change, assuming a user submits a connection like so:
image

The connection data stores extra information for extra__google_cloud_platform__num_retries:
image

With this change, the connection data does not contain extras which are not of the same connection type as the form submission:
image


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Apr 26, 2022
@josh-fell
Copy link
Contributor Author

CC @dstandish

@josh-fell josh-fell requested a review from dstandish April 26, 2022 03:50
@dstandish dstandish force-pushed the extra_field_name_mapping-conn_type-check branch from 3d33d91 to 6e7466a Compare April 26, 2022 13:13
@dstandish
Copy link
Contributor

@josh-fell is this a bug introduced by 22607?

@josh-fell
Copy link
Contributor Author

@josh-fell is this a bug introduced by 22607?

Yes, I could have been much more clear about that 🤦

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 26, 2022
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@dstandish
Copy link
Contributor

yeah i verified the bad behavior in 22607 thank you very much for this.
one question with if value vs if value != "" is it not the case that the form data is always string at this point?

@josh-fell
Copy link
Contributor Author

josh-fell commented Apr 26, 2022

yeah i verified the bad behavior in 22607 thank you very much for this. one question with if value vs if value != "" is it not the case that the form data is always string at this point?

Yes in the vast majority of cases except for BooleanField and IntegerField widgets.

@dstandish
Copy link
Contributor

ah yes of course 🤦 damn widgets!

@ashb ashb merged commit a9ab02f into apache:main Apr 26, 2022
@ashb ashb added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 26, 2022
@josh-fell josh-fell deleted the extra_field_name_mapping-conn_type-check branch April 26, 2022 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants