Skip to content

Set conn_type as not-nullable #9187

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
Merged
mik-laj merged 2 commits into from
Jun 9, 2020
Merged

Set conn_type as not-nullable #9187

mik-laj merged 2 commits into from
Jun 9, 2020

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Jun 8, 2020

The conn_type column in the connection table must contain content. Previously, this rule was enforced by application logic, but was not enforced by the database schema.

Before:

                                          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)

After:

                                          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)  |           | not null |
 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)

I think we should also set conn_id as non-nullable:
#9067 (comment)

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.

@mik-laj mik-laj mentioned this pull request Jun 8, 2020
Merged
6 tasks
@mik-laj mik-laj force-pushed the not-null-conn_type branch from bb43524 to a40eb35 Compare June 9, 2020 06:53
@mik-laj mik-laj force-pushed the not-null-conn_type branch from a40eb35 to f03beb4 Compare June 9, 2020 07:41
@mik-laj mik-laj requested review from ashb, kaxil, potiuk and turbaszek June 9, 2020 08:17
turbaszek
turbaszek reviewed Jun 9, 2020
View reviewed changes
airflow/migrations/versions/8f966b9c467a_set_conn_type_as_non_nullable.py
"""Apply Set conn_type as non-nullable"""

with op.batch_alter_table("connection", schema=None) as batch_op:
batch_op.alter_column("conn_type", existing_type=sa.VARCHAR(length=500), nullable=False)
Copy link
Member

@turbaszek turbaszek Jun 9, 2020

Choose a reason for hiding this comment

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

Will this work without default value? I'm aware that there should be no connection without type but what if?

Copy link
Member Author

@mik-laj mik-laj Jun 9, 2020

Choose a reason for hiding this comment

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

If this happens, the migration will fail and the user will have to manually correct the data. The user will receive information on the specific migration, so user will be able to easily determine the source of the problem. I do not want to modify this table because the user may have had different assumptions based on this field. Airflow has always required this field not to be null. Only various hacks of the user could lead to this state, so he will best know the solution to this problem.

Copy link
Member

@ashb ashb Jun 9, 2020

Choose a reason for hiding this comment

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

Yeah, this sounds good to me -- there's no "sensible default" we could choose to put in this column, so the user will need to manually fix this up.

@ashb
Copy link
Member

ashb commented Jun 9, 2020

I think we should also set conn_id as non-nullable

Yes, agreed. Let's do that in one migration.

@mik-laj mik-laj merged commit 7ad827f into apache:master Jun 9, 2020
@mik-laj mik-laj deleted the not-null-conn_type branch June 9, 2020 16:21
ashb added a commit that referenced this pull request Jun 12, 2020
@ashb
Update (previously null) imap_default conn_type
a31ad49 
This is a test fixup to #9187, and updates the possibly existing
imap_default connection, and adds that hook type to the various lists
@ashb ashb mentioned this pull request Jun 12, 2020
Merged
6 tasks
ashb added a commit that referenced this pull request Jun 12, 2020
@ashb
Update (previously null) imap_default conn_type (#9256)
f90a141 
This is a test fixup to #9187, and updates the possibly existing
imap_default connection, and adds that hook type to the various lists
@kaxil kaxil mentioned this pull request Jun 12, 2020
Merged
6 tasks
kaxil added a commit to astronomer/airflow that referenced this pull request Jun 12, 2020
@kaxil
Fix failing TestSlackWebhookHook
7fdd81a 
`conn_type` was enforced by apache#9187
@kaxil kaxil mentioned this pull request Jun 12, 2020
Merged
6 tasks
kaxil added a commit that referenced this pull request Jun 12, 2020
@kaxil
Fix failing TestSlackWebhookHook (#9260)
f00dde6 
`conn_type` was enforced by #9187
ashb added a commit to astronomer/airflow that referenced this pull request Jun 12, 2020
@ashb
Fix PagerDuty and OpsGenie tests
2222f9e 
conn_type was enforced by apache#9187
@ashb ashb mentioned this pull request Jun 12, 2020
Merged
6 tasks
kaxil pushed a commit that referenced this pull request Jun 12, 2020
@ashb
Fix PagerDuty and OpsGenie tests (#9261)
cf5ab1d 
conn_type was enforced by #9187
ashb added a commit to astronomer/airflow that referenced this pull request Jun 12, 2020
@ashb
Fix Azure container registry hook tests
db45458 
conn_type was enforced by apache#9187
@ashb ashb mentioned this pull request Jun 12, 2020
Merged
6 tasks
kaxil pushed a commit that referenced this pull request Jun 12, 2020
@ashb
Fix Azure container registry hook tests (#9262)
ed09598 
conn_type was enforced by #9187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@turbaszek turbaszek turbaszek approved these changes

@ashb ashb Awaiting requested review from ashb

@potiuk potiuk Awaiting requested review from potiuk

@kaxil kaxil Awaiting requested review from kaxil

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mik-laj @ashb @turbaszek