#55856: add if_not_exists=True to FAB migration

[plutaniano]

closes: #55856

Adds if_not_exists=True to the FAB table ab_view_menu to match all other tables in this migration.

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[vincbeck]

Could you explain more why we need this flag? Also, updating an existing migration is not recommended. It is better to create a new migration

[plutaniano]

@vincbeck All other tables in this migration have this flag set to True and I can't see the reason why it shouldn't also be True for this one. It seems like it wasn't included by mistake. I'm getting an error during an upgrade from 2.11.0 to 3.0.6 because of this (details in the issue).

Altering this migration should be fine, because it's there just to make sure that FAB tables exist. It's idempotent.

Users that have already ran the migration will be in the same state as a user that is running the new version of the migration.

[vincbeck]

@vincbeck All other tables in this migration have this flag set to True and I can't see the reason why it shouldn't also be True for this one. It seems like it wasn't included by mistake. I'm getting an error during an upgrade from 2.11.0 to 3.0.6 because of this (details in the issue).

Altering this migration should be fine, because it's there just to make sure that FAB tables exist. It's idempotent.

Users that have already ran the migration will be in the same state as a user that is running the new version of the migration.

You convinced me :)

[ashb]

Yeah, this was likely an oversight on my part -- normally in migrations we don't need IF EXISTS style checks, but here it's appropriate as we are going from "fab managed this table itself" to "migrations manage it" and need to deal with both fresh installs, and people upgrading from 2.x.

Thanks for the PR @plutaniano !

[pdellarciprete]

Hello @ashb @vincbeck ,

how we can inherit this fix, I saw the last fab provider 2.4.4 doesn't contain the fix. We are in the middle of the migration from 2.9.3 to 3.1.0 and we need it.

Thanks in advance!

[pdellarciprete]

Hello,

I saw the release of the 3.0.0rc1 of airflow-providers-fab, once we will have the 3.0.0 released, also the airflow 3 constraints will be updated?

[vincbeck]

Hello,

I saw the release of the 3.0.0rc1 of airflow-providers-fab, once we will have the 3.0.0 released, also the airflow 3 constraints will be updated?

Not sure about it, maybe @potiuk ?

[eladkal]

I saw the release of the 3.0.0rc1 of airflow-providers-fab, once we will have the 3.0.0 released, also the airflow 3 constraints will be updated?

No. Constraints for previously released versions of Airflow will not be changed. See policy https://airflow.apache.org/docs/apache-airflow/stable/installation/installing-from-pypi.html#fixing-constraints-at-release-time

You can always create your own constraints file. You don't have to use the one provided by the Airflow.

[meher1993]

Hello @ashb - Facing a similar issue when migrating from 2.10.4 to 3.1.0. Shouldn't similar changes be applied to the batch_alter_table during creation of indexes for the following ?

idx_user_group_id
idx_user_id
idx_group_id
idx_group_role_id

For example,

batch_op.create_index("idx_group_id", ["group_id"], unique=False)

will be

batch_op.create_index("idx_group_id", ["group_id"], unique=False, if_not_exists=True)`

sqlalchemy.exc.ProgrammingError: (psycopg2.errors.DuplicateTable) relation "idx_group_id" already exists
[SQL: CREATE INDEX idx_group_id ON ab_group_role (group_id)]
(Background on this error at: https://sqlalche.me/e/14/f405)`

[vincbeck]

I'd say probably. Could you create a PR for it?

[meher1993]

yep, will do

[meher1993]

@vincbeck - submitted #56328 for this. Thanks