Make conn_id unique in Connections table #9067

Hasan-J · 2020-05-29T20:40:18Z

Added an alembic revision, if the user already has multiple records with the same conn_id
in the connections table, it throws an exception during airflow db upgrade with a clear
message.

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

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.
Read the Pull Request Guidelines for more information.

boring-cyborg · 2020-05-29T20:40:21Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://apache-airflow-slack.herokuapp.com/

airflow/migrations/versions/8d48763f6d53_add_unique_constraint_to_conn_id.py

potiuk

I think we need a note in UPDATING.md about this change. Also (but probably later) some migration tool for airflow 2.0 should also deal with this case if someone already has connection in their database . We will have to figure out migration scheme, but I think a note in UPDATING.md will be enough to remember about doing it.

Hasan-J · 2020-06-01T16:26:46Z

I have removed the test method test_connection_metastore_secrets_backend in tests/secrets/test_secrets_backends.py based on the fact that it's using mutiple connections with the same conn_id. Maybe someone can confirm that this indeed should be removed !

tests/secrets/test_secrets_backends.py

mik-laj · 2020-06-08T22:05:16Z

I wonder if we should mark this field as non nullable.
Here is the master table:

                                          Table "public.connection"
       Column       |          Type           | Collation | Nullable |                Default
--------------------+-------------------------+-----------+----------+----------------------------------------
 id                 | integer                 |           | not null | nextval('connection_id_seq'::regclass)
 conn_id            | character varying(250)  |           |          |
 conn_type          | character varying(500)  |           |          |
 host               | character varying(500)  |           |          |
 schema             | character varying(500)  |           |          |
 login              | character varying(500)  |           |          |
 password           | character varying(5000) |           |          |
 port               | integer                 |           |          |
 extra              | character varying(5000) |           |          |
 is_encrypted       | boolean                 |           |          |
 is_extra_encrypted | boolean                 |           |          |
Indexes:
    "connection_pkey" PRIMARY KEY, btree (id)

conn_type should also not be empty, but that's another issue.

mik-laj · 2020-06-09T07:48:49Z

Would it be a big problem to use this field as the primary key instead of just adding a second unique index? The primary key is also a unique index.

airflow/models/connection.py

ashb

I think the airflow.secrets.local_filesystem needs updating too -- it still supports multiple connections with the same id.

Hasan-J · 2020-06-16T18:29:50Z

I think the airflow.secrets.local_filesystem needs updating too -- it still supports multiple connections with the same id.

Oh, this is my first time reading about this backend. Should we stop and raise an exception in case of multiple connections or just load the last one specified ?

mik-laj · 2020-06-21T11:53:26Z

@Hasan-J I think It would be a better idea to raise an exception.

airflow/models/connection.py

As written in apache#9067, Airflow 2.0 introduces a change that requires that all Airflow connections be unique (whereas Airflow 1.10.x technically supports two connections with the same name and potentially the same values). With this commit, we show "warning" in the Airflow UI for users with duplicate connections

apache/airflow#9067 made conn_id unique, and this is effective from 2.0.*. Due to this change, BaseHook.get_connections() will return a List of length 1, or raise Exception. In such a case, we should simply always get the only element from the result from BaseHook.get_connections(), and drop random.choice() in BaseHook.get_connection(), which was only applicable for the earlier setting (multiple connections is allowed for single conn_id) GitOrigin-RevId: 74ed92b3ff34fa3b6af27c17f44ad4573b517bbd

boring-cyborg bot added the area:docs label May 29, 2020

xinbinhuang reviewed May 29, 2020

View reviewed changes

airflow/migrations/versions/8d48763f6d53_add_unique_constraint_to_conn_id.py Outdated Show resolved Hide resolved

feluelle self-requested a review May 30, 2020 07:20

potiuk reviewed May 30, 2020

View reviewed changes

Hasan-J force-pushed the unique_conn_id branch from a2f635d to baa4d1b Compare June 1, 2020 16:21

Hasan-J requested a review from potiuk June 1, 2020 16:47

feluelle reviewed Jun 2, 2020

View reviewed changes

tests/secrets/test_secrets_backends.py Outdated Show resolved Hide resolved

Hasan-J force-pushed the unique_conn_id branch from baa4d1b to 48cb74a Compare June 3, 2020 07:37

mik-laj mentioned this pull request Jun 8, 2020

Set conn_type as not-nullable #9187

Merged

6 tasks

ashb reviewed Jun 15, 2020

View reviewed changes

airflow/models/connection.py Outdated Show resolved Hide resolved

ashb reviewed Jun 15, 2020

View reviewed changes

ashb changed the title ~~[AIRFLOW-8608] Make conn_id unique in Connections table~~ Make conn_id unique in Connections table Jun 15, 2020

ashb removed the area:docs label Jun 15, 2020

mik-laj added this to the Airflow 2.0.0 milestone Jun 21, 2020

Hasan-J force-pushed the unique_conn_id branch from 48cb74a to 540c4ea Compare June 22, 2020 20:31

Hasan-J requested review from ashb and feluelle June 22, 2020 21:42

ashb reviewed Jun 25, 2020

View reviewed changes

airflow/models/connection.py Outdated Show resolved Hide resolved

Hasan-J force-pushed the unique_conn_id branch from 540c4ea to ab22be1 Compare June 25, 2020 10:42

mik-laj self-requested a review June 25, 2020 13:19

Hasan-J force-pushed the unique_conn_id branch from ab22be1 to 5795fc6 Compare June 25, 2020 19:15

Hasan-J closed this Jun 29, 2020

Hasan-J reopened this Jun 29, 2020

Hasan-J force-pushed the unique_conn_id branch from 5795fc6 to 667e231 Compare July 2, 2020 13:34

Hasan-J deleted the unique_conn_id branch August 15, 2022 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make conn_id unique in Connections table #9067

Make conn_id unique in Connections table #9067

Uh oh!

Hasan-J commented May 29, 2020 •

edited

Loading

Uh oh!

boring-cyborg bot commented May 29, 2020

Uh oh!

Uh oh!

potiuk left a comment

Uh oh!

Hasan-J commented Jun 1, 2020

Uh oh!

Uh oh!

mik-laj commented Jun 8, 2020

Uh oh!

mik-laj commented Jun 9, 2020

Uh oh!

Uh oh!

ashb left a comment

Uh oh!

Hasan-J commented Jun 16, 2020 •

edited

Loading

Uh oh!

mik-laj commented Jun 21, 2020 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Make conn_id unique in Connections table #9067

Make conn_id unique in Connections table #9067

Uh oh!

Conversation

Hasan-J commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented May 29, 2020

Uh oh!

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Hasan-J commented Jun 1, 2020

Uh oh!

Uh oh!

mik-laj commented Jun 8, 2020

Uh oh!

mik-laj commented Jun 9, 2020

Uh oh!

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

Hasan-J commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Jun 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Hasan-J commented May 29, 2020 •

edited

Loading

Hasan-J commented Jun 16, 2020 •

edited

Loading

mik-laj commented Jun 21, 2020 •

edited

Loading