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

Fix ORM vs migration files inconsistencies #44221

Merged
merged 14 commits into from
Dec 1, 2024

Conversation

ephraimbuddy
Copy link
Contributor

There have been some inconsistences between ORM and migration files but it doesn't fail in tests. This is an attempt to fix the inconsistency and also have it fail in tests

@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch 5 times, most recently from 031bfd8 to 4ac20ad Compare November 22, 2024 11:13
@ephraimbuddy ephraimbuddy marked this pull request as ready for review November 22, 2024 11:14
@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch from 4ac20ad to 4538776 Compare November 22, 2024 12:38
@ephraimbuddy ephraimbuddy added the full tests needed We need to run full set of tests for this PR to merge label Nov 22, 2024
@ephraimbuddy ephraimbuddy reopened this Nov 22, 2024
@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch from 4538776 to 5f5adbc Compare November 22, 2024 16:12
@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch from 5f5adbc to 47128e9 Compare November 24, 2024 12:12
@ephraimbuddy ephraimbuddy self-assigned this Nov 25, 2024
@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch 2 times, most recently from de89bce to ea2d0d4 Compare November 25, 2024 19:07
@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch from e32cc49 to bf818bd Compare November 26, 2024 11:54
@kaxil
Copy link
Member

kaxil commented Nov 27, 2024

Old sqlalchemy versions don't support drop table if exists. Should we have the _xcom_archive table in the ORM? cc @kaxil see: https://github.com/apache/airflow/actions/runs/12034647961/job/33552179419?pr=44221#step:7:57121

oh, huh, we can remove the if_exists then. We can use almebic get tables to do it

def get_current_table_names():
    connection = op.get_bind()

    metadata = MetaData()
    metadata.reflect(bind=connection)

    return metadata.tables.keys()

or

airflow/airflow/utils/db.py

Lines 1008 to 1029 in 761cedd

def reflect_tables(tables: list[MappedClassProtocol | str] | None, session):
"""
When running checks prior to upgrades, we use reflection to determine current state of the database.
This function gets the current state of each table in the set of models
provided and returns a SqlAlchemy metadata object containing them.
"""
import sqlalchemy.schema
bind = session.bind
metadata = sqlalchemy.schema.MetaData()
if tables is None:
metadata.reflect(bind=bind, resolve_fks=False)
else:
for tbl in tables:
try:
table_name = tbl if isinstance(tbl, str) else tbl.__tablename__
metadata.reflect(bind=bind, only=[table_name], extend_existing=True, resolve_fks=False)
except exc.InvalidRequestError:
continue
return metadata

@ephraimbuddy
Copy link
Contributor Author

Old sqlalchemy versions don't support drop table if exists. Should we have the _xcom_archive table in the ORM? cc @kaxil see: https://github.com/apache/airflow/actions/runs/12034647961/job/33552179419?pr=44221#step:7:57121

oh, huh, we can remove the if_exists then. We can use almebic get tables to do it

def get_current_table_names():
    connection = op.get_bind()

    metadata = MetaData()
    metadata.reflect(bind=connection)

    return metadata.tables.keys()

or

airflow/airflow/utils/db.py

Lines 1008 to 1029 in 761cedd

def reflect_tables(tables: list[MappedClassProtocol | str] | None, session):
"""
When running checks prior to upgrades, we use reflection to determine current state of the database.
This function gets the current state of each table in the set of models
provided and returns a SqlAlchemy metadata object containing them.
"""
import sqlalchemy.schema
bind = session.bind
metadata = sqlalchemy.schema.MetaData()
if tables is None:
metadata.reflect(bind=bind, resolve_fks=False)
else:
for tbl in tables:
try:
table_name = tbl if isinstance(tbl, str) else tbl.__tablename__
metadata.reflect(bind=bind, only=[table_name], extend_existing=True, resolve_fks=False)
except exc.InvalidRequestError:
continue
return metadata

I think that would not work well with offline sql scripts. I’ll verify

@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch from 59e3756 to ee9706c Compare November 28, 2024 08:09
@ephraimbuddy
Copy link
Contributor Author

Didn't think of using raw SQL. I just used it now. Let's see how it goes

@ephraimbuddy ephraimbuddy requested a review from kaxil November 28, 2024 12:39
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch 3 times, most recently from 59b3fd5 to 7b2433a Compare December 1, 2024 07:38
@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch from 7b2433a to 9ce384a Compare December 1, 2024 14:24
@ephraimbuddy ephraimbuddy merged commit c8d4aae into apache:main Dec 1, 2024
97 checks passed
@ephraimbuddy ephraimbuddy deleted the investigate-migrations branch December 1, 2024 22:58
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
* Fix ORM vs migration files inconsistencies

There have been some inconsistences between ORM and migration files
but it doesn't fail in tests. This is an attempt to fix the inconsistency
and also have it fail in tests

* fix for mysql and postgres

* fixup! fix for mysql and postgres

* fix for sqlite

* fixup! fix for sqlite

* fixup! fixup! fix for sqlite

* use TIMESTAMP from db_types

* skip_archive should not delete _xcom_archive tables since that was created by migration

* fix conflicts

* fixup! fix conflicts

* drop _xcom_archive table if it exists

* use sql for dropping xcom_archive table

* fix conflicts

* remove added migration file and make it work in one file
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
* Fix ORM vs migration files inconsistencies

There have been some inconsistences between ORM and migration files
but it doesn't fail in tests. This is an attempt to fix the inconsistency
and also have it fail in tests

* fix for mysql and postgres

* fixup! fix for mysql and postgres

* fix for sqlite

* fixup! fix for sqlite

* fixup! fixup! fix for sqlite

* use TIMESTAMP from db_types

* skip_archive should not delete _xcom_archive tables since that was created by migration

* fix conflicts

* fixup! fix conflicts

* drop _xcom_archive table if it exists

* use sql for dropping xcom_archive table

* fix conflicts

* remove added migration file and make it work in one file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:db-migrations PRs with DB migration area:dev-tools full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants