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.

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

Duplicate Connection: Added logic to query if a connection id exists before creating one #18161

Merged
merged 26 commits into from Oct 9, 2021

Conversation

subkanthi
Copy link
Contributor

Added logic to query if a connection exists before trying to create one in the duplicate connection flow.
closes: #18050


^ 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 UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Sep 11, 2021
airflow/www/views.py Outdated Show resolved Hide resolved
@subkanthi subkanthi closed this Sep 18, 2021
@subkanthi subkanthi reopened this Sep 23, 2021
airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
@subkanthi subkanthi closed this Oct 4, 2021
@subkanthi subkanthi reopened this Oct 4, 2021
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM - but how about adding a test for that one ?

@github-actions
Copy link

github-actions bot commented Oct 5, 2021

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.

@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 Oct 5, 2021
@subkanthi
Copy link
Contributor Author

LGTM - but how about adding a test for that one ?

Thanks , sure will do.

@potiuk
Copy link
Member

potiuk commented Oct 5, 2021

OK. Ping us when there are tests :)

@subkanthi
Copy link
Contributor Author

OK. Ping us when there are tests :)

There is a test in test_views_connection.py that tests the duplication logic, I can add one more to test the logic of erroring out when 10 connections already exists.

`def test_duplicate_connection(admin_client):
"""Test Duplicate multiple connection with suffix"""
conn1 = Connection(
conn_id='test_duplicate_gcp_connection',
conn_type='Google Cloud',
description='Google Cloud Connection',
)
conn2 = Connection(
conn_id='test_duplicate_mysql_connection',
conn_type='FTP',
description='MongoDB2',
host='localhost',
schema='airflow',
port=3306,
)
conn3 = Connection(
conn_id='test_duplicate_postgres_connection_copy1',
conn_type='FTP',
description='Postgres',
host='localhost',
schema='airflow',
port=3306,
)
with create_session() as session:
session.query(Connection).delete()
session.add_all([conn1, conn2, conn3])
session.commit()

data = {"action": "mulduplicate", "rowid": [conn1.id, conn3.id]}
resp = admin_client.post('/connection/action_post', data=data, follow_redirects=True)
expected_result = {
    'test_duplicate_gcp_connection',
    'test_duplicate_gcp_connection_copy1',
    'test_duplicate_mysql_connection',
    'test_duplicate_postgres_connection_copy1',
    'test_duplicate_postgres_connection_copy2',
}
response = {conn[0] for conn in session.query(Connection.conn_id).all()}
assert resp.status_code == 200
assert expected_result == response`

@potiuk
Copy link
Member

potiuk commented Oct 5, 2021

There is a test in test_views_connection.py that tests the duplication logic, I can add one more to test the logic of erroring out when 10 connections already exists.

sounds good :)

subkanthi and others added 2 commits October 8, 2021 06:29
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
@potiuk potiuk merged commit 3ddb365 into apache:main Oct 9, 2021
@potiuk
Copy link
Member

potiuk commented Oct 9, 2021

Wooho!

@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicating the same connection twice gives "Integrity error, probably unique constraint"
4 participants