From 762e4925308ef38b3ba99881b1e089721da38f2d Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 09:58:04 -0700 Subject: [PATCH 1/7] Update README for performing DB migrations, add new indexes for complex queries, update migration params --- README.md | 43 ++++++++- migrations/env.py | 36 ++++++- migrations/versions/7be9deabf3ab_.py | 96 +++++++++++++++++++ .../cdcadbe08ab3_manually_update_indexes.py | 77 +++++++++++++++ 4 files changed, 249 insertions(+), 3 deletions(-) create mode 100644 migrations/versions/7be9deabf3ab_.py create mode 100644 migrations/versions/cdcadbe08ab3_manually_update_indexes.py diff --git a/README.md b/README.md index 33d5562d..0fa41ce9 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ The access service exists to help answer the following questions for each person - All Users - What do I have access to? - - What does a teammate have access to that I don’t? + - What does a teammate have access to that I don't? - What groups and roles are available? - Can I get access? - Team Leads @@ -341,6 +341,47 @@ flask init Visit [http://localhost:3000/](http://localhost:3000/) to view your running version of Access! +### Database Migrations + +Access uses [Alembic](https://alembic.sqlalchemy.org/) for database schema migrations. When you make changes to the database models, you'll need to create and apply a migration. + +#### Creating a New Migration + +To create a new migration after making changes to your models: + +1. Make your changes to the SQLAlchemy models in the `api/models` directory +2. Generate a new migration using: + ``` + flask db migrate -m "Description of your changes" + ``` + This will create a new migration file in the `migrations/versions` directory with a name like `xxxxxxxxxxxx_description_of_your_changes.py` + + For changes that don't involve schema modifications (like adding indexes), you may need to manually create the migration: + ``` + flask db revision -m "Description of your changes" + ``` + Then edit the generated migration file to include your changes, for example: + ```python + def upgrade(): + op.create_index('ix_table_column', 'table', ['column']) + + def downgrade(): + op.drop_index('ix_table_column', 'table') + ``` + +3. Review the generated migration file in `migrations/versions` to ensure it correctly captures your changes +4. Apply the migration using: + ``` + flask db upgrade + ``` + +If you need to rollback a migration, you can use: +``` +flask db downgrade +``` + +For more information about Alembic migrations, see the [Alembic documentation](https://alembic.sqlalchemy.org/). + ### Kubernetes Deployment and CronJobs As Access is a web application packaged with Docker, it can easily be deployed to a Kubernetes cluster. We've included example Kubernetes yaml objects you can use to deploy Access in the [examples/kubernetes](https://github.com/discord/access/tree/main/examples/kubernetes) directory. diff --git a/migrations/env.py b/migrations/env.py index 0f2292e1..aacd10fe 100644 --- a/migrations/env.py +++ b/migrations/env.py @@ -3,9 +3,8 @@ import logging from logging.config import fileConfig -from flask import current_app - from alembic import context +from flask import current_app # this is the Alembic Config object, which provides # access to the values within the .ini file in use. @@ -30,6 +29,27 @@ # ... etc. +def include_object(object, name, type_, reflected, compare_to): + """ + Control which database objects are included in auto-generated migrations. + """ + # Always include indexes to ensure they're properly tracked + if type_ == "index": + return True + + # Exclude Flask-Migrate's own version table + if type_ == "table" and name == "alembic_version": + return False + + # Exclude any PostgreSQL system tables/schemas + if hasattr(object, "schema"): + if object.schema in ["information_schema", "pg_catalog", "pg_toast"]: + return False + + # Include everything else (tables, columns, etc.) + return True + + def run_migrations_offline(): """Run migrations in 'offline' mode. @@ -69,6 +89,18 @@ def process_revision_directives(context, revision, directives): connectable = current_app.extensions["migrate"].db.get_engine() + configure_args = current_app.extensions["migrate"].configure_args.copy() + + configure_args.update( + { + "compare_type": True, # Detect column type changes + "compare_server_default": True, # Detect default value changes + "include_schemas": True, # Include schema information + "include_object": include_object, # Use our custom filter + "render_as_batch": True, # For SQLite compatibility + } + ) + with connectable.connect() as connection: context.configure( connection=connection, diff --git a/migrations/versions/7be9deabf3ab_.py b/migrations/versions/7be9deabf3ab_.py new file mode 100644 index 00000000..0e566e94 --- /dev/null +++ b/migrations/versions/7be9deabf3ab_.py @@ -0,0 +1,96 @@ +"""empty message + +Revision ID: 7be9deabf3ab +Revises: cdcadbe08ab3 +Create Date: 2025-06-13 09:56:35.414358 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '7be9deabf3ab' +down_revision = 'cdcadbe08ab3' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('access_request', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('idx_access_request_approved_membership_id')) + batch_op.drop_index(batch_op.f('idx_access_request_requested_group_id')) + batch_op.drop_index(batch_op.f('idx_access_request_requester_user_id')) + batch_op.drop_index(batch_op.f('idx_access_request_resolver_user_id')) + batch_op.drop_index(batch_op.f('idx_access_request_status_resolved')) + + with op.batch_alter_table('app_group', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('idx_app_group_app_id')) + + with op.batch_alter_table('okta_user', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('idx_okta_user_manager_id')) + + with op.batch_alter_table('okta_user_group_member', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_created_actor_id')) + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_ended_actor_id')) + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_group_ended')) + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_group_id')) + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_role_group_map_id')) + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_user_group')) + batch_op.drop_index(batch_op.f('idx_okta_user_group_member_user_id')) + + with op.batch_alter_table('role_group_map', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('idx_role_group_map_created_actor_id')) + batch_op.drop_index(batch_op.f('idx_role_group_map_ended_actor_id')) + batch_op.drop_index(batch_op.f('idx_role_group_map_group_id')) + batch_op.drop_index(batch_op.f('idx_role_group_map_role_id')) + + with op.batch_alter_table('role_request', schema=None) as batch_op: + batch_op.drop_index(batch_op.f('idx_role_request_approved_membership_id')) + batch_op.drop_index(batch_op.f('idx_role_request_requested_group_id')) + batch_op.drop_index(batch_op.f('idx_role_request_requester_role_id')) + batch_op.drop_index(batch_op.f('idx_role_request_requester_user_id')) + batch_op.drop_index(batch_op.f('idx_role_request_resolver_user_id')) + + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('role_request', schema=None) as batch_op: + batch_op.create_index(batch_op.f('idx_role_request_resolver_user_id'), ['resolver_user_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_request_requester_user_id'), ['requester_user_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_request_requester_role_id'), ['requester_role_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_request_requested_group_id'), ['requested_group_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_request_approved_membership_id'), ['approved_membership_id'], unique=False) + + with op.batch_alter_table('role_group_map', schema=None) as batch_op: + batch_op.create_index(batch_op.f('idx_role_group_map_role_id'), ['role_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_group_map_group_id'), ['group_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_group_map_ended_actor_id'), ['ended_actor_id'], unique=False) + batch_op.create_index(batch_op.f('idx_role_group_map_created_actor_id'), ['created_actor_id'], unique=False) + + with op.batch_alter_table('okta_user_group_member', schema=None) as batch_op: + batch_op.create_index(batch_op.f('idx_okta_user_group_member_user_id'), ['user_id'], unique=False) + batch_op.create_index(batch_op.f('idx_okta_user_group_member_user_group'), ['user_id', 'group_id'], unique=False) + batch_op.create_index(batch_op.f('idx_okta_user_group_member_role_group_map_id'), ['role_group_map_id'], unique=False) + batch_op.create_index(batch_op.f('idx_okta_user_group_member_group_id'), ['group_id'], unique=False) + batch_op.create_index(batch_op.f('idx_okta_user_group_member_group_ended'), ['group_id', 'ended_at'], unique=False) + batch_op.create_index(batch_op.f('idx_okta_user_group_member_ended_actor_id'), ['ended_actor_id'], unique=False) + batch_op.create_index(batch_op.f('idx_okta_user_group_member_created_actor_id'), ['created_actor_id'], unique=False) + + with op.batch_alter_table('okta_user', schema=None) as batch_op: + batch_op.create_index(batch_op.f('idx_okta_user_manager_id'), ['manager_id'], unique=False) + + with op.batch_alter_table('app_group', schema=None) as batch_op: + batch_op.create_index(batch_op.f('idx_app_group_app_id'), ['app_id'], unique=False) + + with op.batch_alter_table('access_request', schema=None) as batch_op: + batch_op.create_index(batch_op.f('idx_access_request_status_resolved'), ['status', 'resolved_at'], unique=False) + batch_op.create_index(batch_op.f('idx_access_request_resolver_user_id'), ['resolver_user_id'], unique=False) + batch_op.create_index(batch_op.f('idx_access_request_requester_user_id'), ['requester_user_id'], unique=False) + batch_op.create_index(batch_op.f('idx_access_request_requested_group_id'), ['requested_group_id'], unique=False) + batch_op.create_index(batch_op.f('idx_access_request_approved_membership_id'), ['approved_membership_id'], unique=False) + + # ### end Alembic commands ### diff --git a/migrations/versions/cdcadbe08ab3_manually_update_indexes.py b/migrations/versions/cdcadbe08ab3_manually_update_indexes.py new file mode 100644 index 00000000..0223a2f8 --- /dev/null +++ b/migrations/versions/cdcadbe08ab3_manually_update_indexes.py @@ -0,0 +1,77 @@ +"""manually update indexes + +Revision ID: cdcadbe08ab3 +Revises: 6d2a03b326f9 +Create Date: 2025-06-13 09:44:45.673469 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "cdcadbe08ab3" +down_revision = "6d2a03b326f9" +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_index("idx_okta_user_group_member_user_id", "okta_user_group_member", ["user_id"]) + op.create_index("idx_okta_user_group_member_group_id", "okta_user_group_member", ["group_id"]) + op.create_index("idx_okta_user_group_member_role_group_map_id", "okta_user_group_member", ["role_group_map_id"]) + op.create_index("idx_okta_user_group_member_created_actor_id", "okta_user_group_member", ["created_actor_id"]) + op.create_index("idx_okta_user_group_member_ended_actor_id", "okta_user_group_member", ["ended_actor_id"]) + + op.create_index("idx_access_request_requester_user_id", "access_request", ["requester_user_id"]) + op.create_index("idx_access_request_requested_group_id", "access_request", ["requested_group_id"]) + op.create_index("idx_access_request_resolver_user_id", "access_request", ["resolver_user_id"]) + op.create_index("idx_access_request_approved_membership_id", "access_request", ["approved_membership_id"]) + + op.create_index("idx_role_request_requester_user_id", "role_request", ["requester_user_id"]) + op.create_index("idx_role_request_requester_role_id", "role_request", ["requester_role_id"]) + op.create_index("idx_role_request_requested_group_id", "role_request", ["requested_group_id"]) + op.create_index("idx_role_request_resolver_user_id", "role_request", ["resolver_user_id"]) + op.create_index("idx_role_request_approved_membership_id", "role_request", ["approved_membership_id"]) + + op.create_index("idx_role_group_map_role_id", "role_group_map", ["role_id"]) + op.create_index("idx_role_group_map_group_id", "role_group_map", ["group_id"]) + op.create_index("idx_role_group_map_created_actor_id", "role_group_map", ["created_actor_id"]) + op.create_index("idx_role_group_map_ended_actor_id", "role_group_map", ["ended_actor_id"]) + + op.create_index("idx_app_group_app_id", "app_group", ["app_id"]) + op.create_index("idx_okta_user_manager_id", "okta_user", ["manager_id"]) + + op.create_index("idx_okta_user_group_member_user_group", "okta_user_group_member", ["user_id", "group_id"]) + op.create_index("idx_okta_user_group_member_group_ended", "okta_user_group_member", ["group_id", "ended_at"]) + op.create_index("idx_access_request_status_resolved", "access_request", ["status", "resolved_at"]) + + +def downgrade(): + op.drop_index("idx_okta_user_group_member_user_id", "okta_user_group_member") + op.drop_index("idx_okta_user_group_member_group_id", "okta_user_group_member") + op.drop_index("idx_okta_user_group_member_role_group_map_id", "okta_user_group_member") + op.drop_index("idx_okta_user_group_member_created_actor_id", "okta_user_group_member") + op.drop_index("idx_okta_user_group_member_ended_actor_id", "okta_user_group_member") + + op.drop_index("idx_access_request_requester_user_id", "access_request") + op.drop_index("idx_access_request_requested_group_id", "access_request") + op.drop_index("idx_access_request_resolver_user_id", "access_request") + op.drop_index("idx_access_request_approved_membership_id", "access_request") + + op.drop_index("idx_role_request_requester_user_id", "role_request") + op.drop_index("idx_role_request_requester_role_id", "role_request") + op.drop_index("idx_role_request_requested_group_id", "role_request") + op.drop_index("idx_role_request_resolver_user_id", "role_request") + op.drop_index("idx_role_request_approved_membership_id", "role_request") + + op.drop_index("idx_role_group_map_role_id", "role_group_map") + op.drop_index("idx_role_group_map_group_id", "role_group_map") + op.drop_index("idx_role_group_map_created_actor_id", "role_group_map") + op.drop_index("idx_role_group_map_ended_actor_id", "role_group_map") + + op.drop_index("idx_app_group_app_id", "app_group") + op.drop_index("idx_okta_user_manager_id", "okta_user") + + op.drop_index("idx_okta_user_group_member_user_group", "okta_user_group_member") + op.drop_index("idx_okta_user_group_member_group_ended", "okta_user_group_member") + op.drop_index("idx_access_request_status_resolved", "access_request") From 59975b22ff6d41f281ff8bb4516846f1a2954f19 Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 10:17:21 -0700 Subject: [PATCH 2/7] remove dupe --- migrations/versions/7be9deabf3ab_.py | 96 ------------ .../cdcadbe08ab3_manually_update_indexes.py | 142 +++++++++++------- 2 files changed, 86 insertions(+), 152 deletions(-) delete mode 100644 migrations/versions/7be9deabf3ab_.py diff --git a/migrations/versions/7be9deabf3ab_.py b/migrations/versions/7be9deabf3ab_.py deleted file mode 100644 index 0e566e94..00000000 --- a/migrations/versions/7be9deabf3ab_.py +++ /dev/null @@ -1,96 +0,0 @@ -"""empty message - -Revision ID: 7be9deabf3ab -Revises: cdcadbe08ab3 -Create Date: 2025-06-13 09:56:35.414358 - -""" -from alembic import op -import sqlalchemy as sa - - -# revision identifiers, used by Alembic. -revision = '7be9deabf3ab' -down_revision = 'cdcadbe08ab3' -branch_labels = None -depends_on = None - - -def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - with op.batch_alter_table('access_request', schema=None) as batch_op: - batch_op.drop_index(batch_op.f('idx_access_request_approved_membership_id')) - batch_op.drop_index(batch_op.f('idx_access_request_requested_group_id')) - batch_op.drop_index(batch_op.f('idx_access_request_requester_user_id')) - batch_op.drop_index(batch_op.f('idx_access_request_resolver_user_id')) - batch_op.drop_index(batch_op.f('idx_access_request_status_resolved')) - - with op.batch_alter_table('app_group', schema=None) as batch_op: - batch_op.drop_index(batch_op.f('idx_app_group_app_id')) - - with op.batch_alter_table('okta_user', schema=None) as batch_op: - batch_op.drop_index(batch_op.f('idx_okta_user_manager_id')) - - with op.batch_alter_table('okta_user_group_member', schema=None) as batch_op: - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_created_actor_id')) - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_ended_actor_id')) - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_group_ended')) - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_group_id')) - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_role_group_map_id')) - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_user_group')) - batch_op.drop_index(batch_op.f('idx_okta_user_group_member_user_id')) - - with op.batch_alter_table('role_group_map', schema=None) as batch_op: - batch_op.drop_index(batch_op.f('idx_role_group_map_created_actor_id')) - batch_op.drop_index(batch_op.f('idx_role_group_map_ended_actor_id')) - batch_op.drop_index(batch_op.f('idx_role_group_map_group_id')) - batch_op.drop_index(batch_op.f('idx_role_group_map_role_id')) - - with op.batch_alter_table('role_request', schema=None) as batch_op: - batch_op.drop_index(batch_op.f('idx_role_request_approved_membership_id')) - batch_op.drop_index(batch_op.f('idx_role_request_requested_group_id')) - batch_op.drop_index(batch_op.f('idx_role_request_requester_role_id')) - batch_op.drop_index(batch_op.f('idx_role_request_requester_user_id')) - batch_op.drop_index(batch_op.f('idx_role_request_resolver_user_id')) - - # ### end Alembic commands ### - - -def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - with op.batch_alter_table('role_request', schema=None) as batch_op: - batch_op.create_index(batch_op.f('idx_role_request_resolver_user_id'), ['resolver_user_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_request_requester_user_id'), ['requester_user_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_request_requester_role_id'), ['requester_role_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_request_requested_group_id'), ['requested_group_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_request_approved_membership_id'), ['approved_membership_id'], unique=False) - - with op.batch_alter_table('role_group_map', schema=None) as batch_op: - batch_op.create_index(batch_op.f('idx_role_group_map_role_id'), ['role_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_group_map_group_id'), ['group_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_group_map_ended_actor_id'), ['ended_actor_id'], unique=False) - batch_op.create_index(batch_op.f('idx_role_group_map_created_actor_id'), ['created_actor_id'], unique=False) - - with op.batch_alter_table('okta_user_group_member', schema=None) as batch_op: - batch_op.create_index(batch_op.f('idx_okta_user_group_member_user_id'), ['user_id'], unique=False) - batch_op.create_index(batch_op.f('idx_okta_user_group_member_user_group'), ['user_id', 'group_id'], unique=False) - batch_op.create_index(batch_op.f('idx_okta_user_group_member_role_group_map_id'), ['role_group_map_id'], unique=False) - batch_op.create_index(batch_op.f('idx_okta_user_group_member_group_id'), ['group_id'], unique=False) - batch_op.create_index(batch_op.f('idx_okta_user_group_member_group_ended'), ['group_id', 'ended_at'], unique=False) - batch_op.create_index(batch_op.f('idx_okta_user_group_member_ended_actor_id'), ['ended_actor_id'], unique=False) - batch_op.create_index(batch_op.f('idx_okta_user_group_member_created_actor_id'), ['created_actor_id'], unique=False) - - with op.batch_alter_table('okta_user', schema=None) as batch_op: - batch_op.create_index(batch_op.f('idx_okta_user_manager_id'), ['manager_id'], unique=False) - - with op.batch_alter_table('app_group', schema=None) as batch_op: - batch_op.create_index(batch_op.f('idx_app_group_app_id'), ['app_id'], unique=False) - - with op.batch_alter_table('access_request', schema=None) as batch_op: - batch_op.create_index(batch_op.f('idx_access_request_status_resolved'), ['status', 'resolved_at'], unique=False) - batch_op.create_index(batch_op.f('idx_access_request_resolver_user_id'), ['resolver_user_id'], unique=False) - batch_op.create_index(batch_op.f('idx_access_request_requester_user_id'), ['requester_user_id'], unique=False) - batch_op.create_index(batch_op.f('idx_access_request_requested_group_id'), ['requested_group_id'], unique=False) - batch_op.create_index(batch_op.f('idx_access_request_approved_membership_id'), ['approved_membership_id'], unique=False) - - # ### end Alembic commands ### diff --git a/migrations/versions/cdcadbe08ab3_manually_update_indexes.py b/migrations/versions/cdcadbe08ab3_manually_update_indexes.py index 0223a2f8..2a4107aa 100644 --- a/migrations/versions/cdcadbe08ab3_manually_update_indexes.py +++ b/migrations/versions/cdcadbe08ab3_manually_update_indexes.py @@ -16,62 +16,92 @@ def upgrade(): - op.create_index("idx_okta_user_group_member_user_id", "okta_user_group_member", ["user_id"]) - op.create_index("idx_okta_user_group_member_group_id", "okta_user_group_member", ["group_id"]) - op.create_index("idx_okta_user_group_member_role_group_map_id", "okta_user_group_member", ["role_group_map_id"]) - op.create_index("idx_okta_user_group_member_created_actor_id", "okta_user_group_member", ["created_actor_id"]) - op.create_index("idx_okta_user_group_member_ended_actor_id", "okta_user_group_member", ["ended_actor_id"]) - - op.create_index("idx_access_request_requester_user_id", "access_request", ["requester_user_id"]) - op.create_index("idx_access_request_requested_group_id", "access_request", ["requested_group_id"]) - op.create_index("idx_access_request_resolver_user_id", "access_request", ["resolver_user_id"]) - op.create_index("idx_access_request_approved_membership_id", "access_request", ["approved_membership_id"]) - - op.create_index("idx_role_request_requester_user_id", "role_request", ["requester_user_id"]) - op.create_index("idx_role_request_requester_role_id", "role_request", ["requester_role_id"]) - op.create_index("idx_role_request_requested_group_id", "role_request", ["requested_group_id"]) - op.create_index("idx_role_request_resolver_user_id", "role_request", ["resolver_user_id"]) - op.create_index("idx_role_request_approved_membership_id", "role_request", ["approved_membership_id"]) - - op.create_index("idx_role_group_map_role_id", "role_group_map", ["role_id"]) - op.create_index("idx_role_group_map_group_id", "role_group_map", ["group_id"]) - op.create_index("idx_role_group_map_created_actor_id", "role_group_map", ["created_actor_id"]) - op.create_index("idx_role_group_map_ended_actor_id", "role_group_map", ["ended_actor_id"]) - - op.create_index("idx_app_group_app_id", "app_group", ["app_id"]) - op.create_index("idx_okta_user_manager_id", "okta_user", ["manager_id"]) - - op.create_index("idx_okta_user_group_member_user_group", "okta_user_group_member", ["user_id", "group_id"]) - op.create_index("idx_okta_user_group_member_group_ended", "okta_user_group_member", ["group_id", "ended_at"]) - op.create_index("idx_access_request_status_resolved", "access_request", ["status", "resolved_at"]) + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op: + batch_op.create_index(batch_op.f("idx_okta_user_group_member_user_id"), ["user_id"], unique=False) + batch_op.create_index(batch_op.f("idx_okta_user_group_member_group_id"), ["group_id"], unique=False) + batch_op.create_index( + batch_op.f("idx_okta_user_group_member_role_group_map_id"), ["role_group_map_id"], unique=False + ) + batch_op.create_index( + batch_op.f("idx_okta_user_group_member_created_actor_id"), ["created_actor_id"], unique=False + ) + batch_op.create_index(batch_op.f("idx_okta_user_group_member_ended_actor_id"), ["ended_actor_id"], unique=False) + batch_op.create_index( + batch_op.f("idx_okta_user_group_member_user_group"), ["user_id", "group_id"], unique=False + ) + batch_op.create_index( + batch_op.f("idx_okta_user_group_member_group_ended"), ["group_id", "ended_at"], unique=False + ) + + with op.batch_alter_table("access_request", schema=None) as batch_op: + batch_op.create_index(batch_op.f("idx_access_request_requester_user_id"), ["requester_user_id"], unique=False) + batch_op.create_index(batch_op.f("idx_access_request_requested_group_id"), ["requested_group_id"], unique=False) + batch_op.create_index(batch_op.f("idx_access_request_resolver_user_id"), ["resolver_user_id"], unique=False) + batch_op.create_index( + batch_op.f("idx_access_request_approved_membership_id"), ["approved_membership_id"], unique=False + ) + batch_op.create_index(batch_op.f("idx_access_request_status_resolved"), ["status", "resolved_at"], unique=False) + + with op.batch_alter_table("role_request", schema=None) as batch_op: + batch_op.create_index(batch_op.f("idx_role_request_requester_user_id"), ["requester_user_id"], unique=False) + batch_op.create_index(batch_op.f("idx_role_request_requester_role_id"), ["requester_role_id"], unique=False) + batch_op.create_index(batch_op.f("idx_role_request_requested_group_id"), ["requested_group_id"], unique=False) + batch_op.create_index(batch_op.f("idx_role_request_resolver_user_id"), ["resolver_user_id"], unique=False) + batch_op.create_index( + batch_op.f("idx_role_request_approved_membership_id"), ["approved_membership_id"], unique=False + ) + + with op.batch_alter_table("role_group_map", schema=None) as batch_op: + batch_op.create_index(batch_op.f("idx_role_group_map_role_id"), ["role_id"], unique=False) + batch_op.create_index(batch_op.f("idx_role_group_map_group_id"), ["group_id"], unique=False) + batch_op.create_index(batch_op.f("idx_role_group_map_created_actor_id"), ["created_actor_id"], unique=False) + batch_op.create_index(batch_op.f("idx_role_group_map_ended_actor_id"), ["ended_actor_id"], unique=False) + + with op.batch_alter_table("app_group", schema=None) as batch_op: + batch_op.create_index(batch_op.f("idx_app_group_app_id"), ["app_id"], unique=False) + + with op.batch_alter_table("okta_user", schema=None) as batch_op: + batch_op.create_index(batch_op.f("idx_okta_user_manager_id"), ["manager_id"], unique=False) + + # ### end Alembic commands ### def downgrade(): - op.drop_index("idx_okta_user_group_member_user_id", "okta_user_group_member") - op.drop_index("idx_okta_user_group_member_group_id", "okta_user_group_member") - op.drop_index("idx_okta_user_group_member_role_group_map_id", "okta_user_group_member") - op.drop_index("idx_okta_user_group_member_created_actor_id", "okta_user_group_member") - op.drop_index("idx_okta_user_group_member_ended_actor_id", "okta_user_group_member") - - op.drop_index("idx_access_request_requester_user_id", "access_request") - op.drop_index("idx_access_request_requested_group_id", "access_request") - op.drop_index("idx_access_request_resolver_user_id", "access_request") - op.drop_index("idx_access_request_approved_membership_id", "access_request") - - op.drop_index("idx_role_request_requester_user_id", "role_request") - op.drop_index("idx_role_request_requester_role_id", "role_request") - op.drop_index("idx_role_request_requested_group_id", "role_request") - op.drop_index("idx_role_request_resolver_user_id", "role_request") - op.drop_index("idx_role_request_approved_membership_id", "role_request") - - op.drop_index("idx_role_group_map_role_id", "role_group_map") - op.drop_index("idx_role_group_map_group_id", "role_group_map") - op.drop_index("idx_role_group_map_created_actor_id", "role_group_map") - op.drop_index("idx_role_group_map_ended_actor_id", "role_group_map") - - op.drop_index("idx_app_group_app_id", "app_group") - op.drop_index("idx_okta_user_manager_id", "okta_user") - - op.drop_index("idx_okta_user_group_member_user_group", "okta_user_group_member") - op.drop_index("idx_okta_user_group_member_group_ended", "okta_user_group_member") - op.drop_index("idx_access_request_status_resolved", "access_request") + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("okta_user", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("idx_okta_user_manager_id")) + + with op.batch_alter_table("app_group", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("idx_app_group_app_id")) + + with op.batch_alter_table("role_group_map", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("idx_role_group_map_ended_actor_id")) + batch_op.drop_index(batch_op.f("idx_role_group_map_created_actor_id")) + batch_op.drop_index(batch_op.f("idx_role_group_map_group_id")) + batch_op.drop_index(batch_op.f("idx_role_group_map_role_id")) + + with op.batch_alter_table("role_request", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("idx_role_request_approved_membership_id")) + batch_op.drop_index(batch_op.f("idx_role_request_resolver_user_id")) + batch_op.drop_index(batch_op.f("idx_role_request_requested_group_id")) + batch_op.drop_index(batch_op.f("idx_role_request_requester_role_id")) + batch_op.drop_index(batch_op.f("idx_role_request_requester_user_id")) + + with op.batch_alter_table("access_request", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("idx_access_request_status_resolved")) + batch_op.drop_index(batch_op.f("idx_access_request_approved_membership_id")) + batch_op.drop_index(batch_op.f("idx_access_request_resolver_user_id")) + batch_op.drop_index(batch_op.f("idx_access_request_requested_group_id")) + batch_op.drop_index(batch_op.f("idx_access_request_requester_user_id")) + + with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_group_ended")) + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_user_group")) + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_ended_actor_id")) + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_created_actor_id")) + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_role_group_map_id")) + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_group_id")) + batch_op.drop_index(batch_op.f("idx_okta_user_group_member_user_id")) + + # ### end Alembic commands ### From d185d238d83c5387826a06506789e41586acf963 Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 10:28:47 -0700 Subject: [PATCH 3/7] do this the flask way instead --- api/models/core_models.py | 36 ++++++ .../0a113a8bc0df_create_new_indexes.py | 95 ++++++++++++++++ .../cdcadbe08ab3_manually_update_indexes.py | 107 ------------------ 3 files changed, 131 insertions(+), 107 deletions(-) create mode 100644 migrations/versions/0a113a8bc0df_create_new_indexes.py delete mode 100644 migrations/versions/cdcadbe08ab3_manually_update_indexes.py diff --git a/api/models/core_models.py b/api/models/core_models.py index 602bc2e0..6dfaec3f 100644 --- a/api/models/core_models.py +++ b/api/models/core_models.py @@ -40,6 +40,16 @@ class OktaUserGroupMember(db.Model): created_reason: Mapped[str] = mapped_column(db.Unicode(1024), nullable=False, default="", server_default="") + __table_args__ = ( + db.Index("idx_okta_user_group_member_user_id", "user_id"), + db.Index("idx_okta_user_group_member_group_id", "group_id"), + db.Index("idx_okta_user_group_member_role_group_map_id", "role_group_map_id"), + db.Index("idx_okta_user_group_member_created_actor_id", "created_actor_id"), + db.Index("idx_okta_user_group_member_ended_actor_id", "ended_actor_id"), + db.Index("idx_okta_user_group_member_user_group", "user_id", "group_id"), + db.Index("idx_okta_user_group_member_group_ended", "group_id", "ended_at"), + ) + # See more details on specifying alternative join conditions for relationships at # https://docs.sqlalchemy.org/en/14/orm/join_conditions.html#specifying-alternate-join-conditions group: Mapped["OktaGroup"] = db.relationship( @@ -136,6 +146,7 @@ class OktaUser(db.Model): postgresql_where=db.text("deleted_at IS NULL"), sqlite_where=db.text("deleted_at IS NULL"), ), + db.Index("idx_okta_user_manager_id", "manager_id"), ) # A JSON field for storing the user profile, including extra user attribute data from Okta @@ -455,6 +466,13 @@ class RoleGroupMap(db.Model): created_reason: Mapped[str] = mapped_column(db.Unicode(1024), nullable=False, default="", server_default="") + __table_args__ = ( + db.Index("idx_role_group_map_role_id", "role_id"), + db.Index("idx_role_group_map_group_id", "group_id"), + db.Index("idx_role_group_map_created_actor_id", "created_actor_id"), + db.Index("idx_role_group_map_ended_actor_id", "ended_actor_id"), + ) + # See more details on specifying alternative join conditions for relationships at # https://docs.sqlalchemy.org/en/14/orm/join_conditions.html#specifying-alternate-join-conditions role_group: Mapped["RoleGroup"] = db.relationship( @@ -589,6 +607,8 @@ class AppGroup(OktaGroup): # group to administer and manage membership of the app owner group is_owner: Mapped[bool] = mapped_column(db.Boolean, nullable=False, server_default=expression.false(), default=False) + __table_args__ = (db.Index("idx_app_group_app_id", "app_id"),) + # SQLAlchemy doesn't seem to support loading # group.active_role_associated_group_[member|owner]_mappings.active_group when a group_id or user_id is specified # in GET /api/audit/users so we have to enable "select" lazy loading. @@ -714,6 +734,14 @@ class AccessRequest(db.Model): db.ForeignKey("okta_user_group_member.id"), ) + __table_args__ = ( + db.Index("idx_access_request_requester_user_id", "requester_user_id"), + db.Index("idx_access_request_requested_group_id", "requested_group_id"), + db.Index("idx_access_request_resolver_user_id", "resolver_user_id"), + db.Index("idx_access_request_approved_membership_id", "approved_membership_id"), + db.Index("idx_access_request_status_resolved", "status", "resolved_at"), + ) + requester: Mapped[OktaUser] = db.relationship( "OktaUser", back_populates="all_access_requests", @@ -801,6 +829,14 @@ class RoleRequest(db.Model): db.ForeignKey("role_group_map.id"), ) + __table_args__ = ( + db.Index("idx_role_request_requester_user_id", "requester_user_id"), + db.Index("idx_role_request_requester_role_id", "requester_role_id"), + db.Index("idx_role_request_requested_group_id", "requested_group_id"), + db.Index("idx_role_request_resolver_user_id", "resolver_user_id"), + db.Index("idx_role_request_approved_membership_id", "approved_membership_id"), + ) + requester: Mapped[OktaUser] = db.relationship( "OktaUser", primaryjoin="OktaUser.id == RoleRequest.requester_user_id", diff --git a/migrations/versions/0a113a8bc0df_create_new_indexes.py b/migrations/versions/0a113a8bc0df_create_new_indexes.py new file mode 100644 index 00000000..17b049ac --- /dev/null +++ b/migrations/versions/0a113a8bc0df_create_new_indexes.py @@ -0,0 +1,95 @@ +"""create new indexes + +Revision ID: 0a113a8bc0df +Revises: 6d2a03b326f9 +Create Date: 2025-06-13 10:28:06.674777 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "0a113a8bc0df" +down_revision = "6d2a03b326f9" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("access_request", schema=None) as batch_op: + batch_op.create_index("idx_access_request_approved_membership_id", ["approved_membership_id"], unique=False) + batch_op.create_index("idx_access_request_requested_group_id", ["requested_group_id"], unique=False) + batch_op.create_index("idx_access_request_requester_user_id", ["requester_user_id"], unique=False) + batch_op.create_index("idx_access_request_resolver_user_id", ["resolver_user_id"], unique=False) + batch_op.create_index("idx_access_request_status_resolved", ["status", "resolved_at"], unique=False) + + with op.batch_alter_table("app_group", schema=None) as batch_op: + batch_op.create_index("idx_app_group_app_id", ["app_id"], unique=False) + + with op.batch_alter_table("okta_user", schema=None) as batch_op: + batch_op.create_index("idx_okta_user_manager_id", ["manager_id"], unique=False) + + with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op: + batch_op.create_index("idx_okta_user_group_member_created_actor_id", ["created_actor_id"], unique=False) + batch_op.create_index("idx_okta_user_group_member_ended_actor_id", ["ended_actor_id"], unique=False) + batch_op.create_index("idx_okta_user_group_member_group_ended", ["group_id", "ended_at"], unique=False) + batch_op.create_index("idx_okta_user_group_member_group_id", ["group_id"], unique=False) + batch_op.create_index("idx_okta_user_group_member_role_group_map_id", ["role_group_map_id"], unique=False) + batch_op.create_index("idx_okta_user_group_member_user_group", ["user_id", "group_id"], unique=False) + batch_op.create_index("idx_okta_user_group_member_user_id", ["user_id"], unique=False) + + with op.batch_alter_table("role_group_map", schema=None) as batch_op: + batch_op.create_index("idx_role_group_map_created_actor_id", ["created_actor_id"], unique=False) + batch_op.create_index("idx_role_group_map_ended_actor_id", ["ended_actor_id"], unique=False) + batch_op.create_index("idx_role_group_map_group_id", ["group_id"], unique=False) + batch_op.create_index("idx_role_group_map_role_id", ["role_id"], unique=False) + + with op.batch_alter_table("role_request", schema=None) as batch_op: + batch_op.create_index("idx_role_request_approved_membership_id", ["approved_membership_id"], unique=False) + batch_op.create_index("idx_role_request_requested_group_id", ["requested_group_id"], unique=False) + batch_op.create_index("idx_role_request_requester_role_id", ["requester_role_id"], unique=False) + batch_op.create_index("idx_role_request_requester_user_id", ["requester_user_id"], unique=False) + batch_op.create_index("idx_role_request_resolver_user_id", ["resolver_user_id"], unique=False) + + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("role_request", schema=None) as batch_op: + batch_op.drop_index("idx_role_request_resolver_user_id") + batch_op.drop_index("idx_role_request_requester_user_id") + batch_op.drop_index("idx_role_request_requester_role_id") + batch_op.drop_index("idx_role_request_requested_group_id") + batch_op.drop_index("idx_role_request_approved_membership_id") + + with op.batch_alter_table("role_group_map", schema=None) as batch_op: + batch_op.drop_index("idx_role_group_map_role_id") + batch_op.drop_index("idx_role_group_map_group_id") + batch_op.drop_index("idx_role_group_map_ended_actor_id") + batch_op.drop_index("idx_role_group_map_created_actor_id") + + with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op: + batch_op.drop_index("idx_okta_user_group_member_user_id") + batch_op.drop_index("idx_okta_user_group_member_user_group") + batch_op.drop_index("idx_okta_user_group_member_role_group_map_id") + batch_op.drop_index("idx_okta_user_group_member_group_id") + batch_op.drop_index("idx_okta_user_group_member_group_ended") + batch_op.drop_index("idx_okta_user_group_member_ended_actor_id") + batch_op.drop_index("idx_okta_user_group_member_created_actor_id") + + with op.batch_alter_table("okta_user", schema=None) as batch_op: + batch_op.drop_index("idx_okta_user_manager_id") + + with op.batch_alter_table("app_group", schema=None) as batch_op: + batch_op.drop_index("idx_app_group_app_id") + + with op.batch_alter_table("access_request", schema=None) as batch_op: + batch_op.drop_index("idx_access_request_status_resolved") + batch_op.drop_index("idx_access_request_resolver_user_id") + batch_op.drop_index("idx_access_request_requester_user_id") + batch_op.drop_index("idx_access_request_requested_group_id") + batch_op.drop_index("idx_access_request_approved_membership_id") + + # ### end Alembic commands ### diff --git a/migrations/versions/cdcadbe08ab3_manually_update_indexes.py b/migrations/versions/cdcadbe08ab3_manually_update_indexes.py deleted file mode 100644 index 2a4107aa..00000000 --- a/migrations/versions/cdcadbe08ab3_manually_update_indexes.py +++ /dev/null @@ -1,107 +0,0 @@ -"""manually update indexes - -Revision ID: cdcadbe08ab3 -Revises: 6d2a03b326f9 -Create Date: 2025-06-13 09:44:45.673469 - -""" - -from alembic import op - -# revision identifiers, used by Alembic. -revision = "cdcadbe08ab3" -down_revision = "6d2a03b326f9" -branch_labels = None -depends_on = None - - -def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op: - batch_op.create_index(batch_op.f("idx_okta_user_group_member_user_id"), ["user_id"], unique=False) - batch_op.create_index(batch_op.f("idx_okta_user_group_member_group_id"), ["group_id"], unique=False) - batch_op.create_index( - batch_op.f("idx_okta_user_group_member_role_group_map_id"), ["role_group_map_id"], unique=False - ) - batch_op.create_index( - batch_op.f("idx_okta_user_group_member_created_actor_id"), ["created_actor_id"], unique=False - ) - batch_op.create_index(batch_op.f("idx_okta_user_group_member_ended_actor_id"), ["ended_actor_id"], unique=False) - batch_op.create_index( - batch_op.f("idx_okta_user_group_member_user_group"), ["user_id", "group_id"], unique=False - ) - batch_op.create_index( - batch_op.f("idx_okta_user_group_member_group_ended"), ["group_id", "ended_at"], unique=False - ) - - with op.batch_alter_table("access_request", schema=None) as batch_op: - batch_op.create_index(batch_op.f("idx_access_request_requester_user_id"), ["requester_user_id"], unique=False) - batch_op.create_index(batch_op.f("idx_access_request_requested_group_id"), ["requested_group_id"], unique=False) - batch_op.create_index(batch_op.f("idx_access_request_resolver_user_id"), ["resolver_user_id"], unique=False) - batch_op.create_index( - batch_op.f("idx_access_request_approved_membership_id"), ["approved_membership_id"], unique=False - ) - batch_op.create_index(batch_op.f("idx_access_request_status_resolved"), ["status", "resolved_at"], unique=False) - - with op.batch_alter_table("role_request", schema=None) as batch_op: - batch_op.create_index(batch_op.f("idx_role_request_requester_user_id"), ["requester_user_id"], unique=False) - batch_op.create_index(batch_op.f("idx_role_request_requester_role_id"), ["requester_role_id"], unique=False) - batch_op.create_index(batch_op.f("idx_role_request_requested_group_id"), ["requested_group_id"], unique=False) - batch_op.create_index(batch_op.f("idx_role_request_resolver_user_id"), ["resolver_user_id"], unique=False) - batch_op.create_index( - batch_op.f("idx_role_request_approved_membership_id"), ["approved_membership_id"], unique=False - ) - - with op.batch_alter_table("role_group_map", schema=None) as batch_op: - batch_op.create_index(batch_op.f("idx_role_group_map_role_id"), ["role_id"], unique=False) - batch_op.create_index(batch_op.f("idx_role_group_map_group_id"), ["group_id"], unique=False) - batch_op.create_index(batch_op.f("idx_role_group_map_created_actor_id"), ["created_actor_id"], unique=False) - batch_op.create_index(batch_op.f("idx_role_group_map_ended_actor_id"), ["ended_actor_id"], unique=False) - - with op.batch_alter_table("app_group", schema=None) as batch_op: - batch_op.create_index(batch_op.f("idx_app_group_app_id"), ["app_id"], unique=False) - - with op.batch_alter_table("okta_user", schema=None) as batch_op: - batch_op.create_index(batch_op.f("idx_okta_user_manager_id"), ["manager_id"], unique=False) - - # ### end Alembic commands ### - - -def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - with op.batch_alter_table("okta_user", schema=None) as batch_op: - batch_op.drop_index(batch_op.f("idx_okta_user_manager_id")) - - with op.batch_alter_table("app_group", schema=None) as batch_op: - batch_op.drop_index(batch_op.f("idx_app_group_app_id")) - - with op.batch_alter_table("role_group_map", schema=None) as batch_op: - batch_op.drop_index(batch_op.f("idx_role_group_map_ended_actor_id")) - batch_op.drop_index(batch_op.f("idx_role_group_map_created_actor_id")) - batch_op.drop_index(batch_op.f("idx_role_group_map_group_id")) - batch_op.drop_index(batch_op.f("idx_role_group_map_role_id")) - - with op.batch_alter_table("role_request", schema=None) as batch_op: - batch_op.drop_index(batch_op.f("idx_role_request_approved_membership_id")) - batch_op.drop_index(batch_op.f("idx_role_request_resolver_user_id")) - batch_op.drop_index(batch_op.f("idx_role_request_requested_group_id")) - batch_op.drop_index(batch_op.f("idx_role_request_requester_role_id")) - batch_op.drop_index(batch_op.f("idx_role_request_requester_user_id")) - - with op.batch_alter_table("access_request", schema=None) as batch_op: - batch_op.drop_index(batch_op.f("idx_access_request_status_resolved")) - batch_op.drop_index(batch_op.f("idx_access_request_approved_membership_id")) - batch_op.drop_index(batch_op.f("idx_access_request_resolver_user_id")) - batch_op.drop_index(batch_op.f("idx_access_request_requested_group_id")) - batch_op.drop_index(batch_op.f("idx_access_request_requester_user_id")) - - with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op: - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_group_ended")) - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_user_group")) - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_ended_actor_id")) - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_created_actor_id")) - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_role_group_map_id")) - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_group_id")) - batch_op.drop_index(batch_op.f("idx_okta_user_group_member_user_id")) - - # ### end Alembic commands ### From 901b1d27f8482517f1f1c41cec61f444f1e2e1c9 Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 11:25:45 -0700 Subject: [PATCH 4/7] update tests --- tests/test_group_membership_sync.py | 3 +++ tests/test_group_ownership_sync.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/tests/test_group_membership_sync.py b/tests/test_group_membership_sync.py index 2023b222..b7edd0fa 100644 --- a/tests/test_group_membership_sync.py +++ b/tests/test_group_membership_sync.py @@ -319,6 +319,9 @@ def run_sync( groups_with_rules: set[str] = set(), ) -> list[OktaGroup]: with Session(db.engine) as session: + # Initialize Okta service + okta.initialize("test.okta.com", "test-token") + mocker.patch.object(okta, "list_groups", return_value=okta_groups) mocker.patch.object(okta, "list_users_for_group", side_effect=user_membership_func) diff --git a/tests/test_group_ownership_sync.py b/tests/test_group_ownership_sync.py index a4d03ac8..234d6831 100644 --- a/tests/test_group_ownership_sync.py +++ b/tests/test_group_ownership_sync.py @@ -397,6 +397,9 @@ def run_sync( groups_with_rules: set[str] = set(), ) -> list[OktaGroup]: with Session(db.engine) as session: + # Initialize Okta service with group owners API enabled + okta.initialize("test.okta.com", "test-token", use_group_owners_api=True) + mocker.patch.object(okta, "list_groups", return_value=okta_groups) mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func) From ccdd9fb0b631213f9a2c71429b4593463519a3ce Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 11:39:16 -0700 Subject: [PATCH 5/7] update tests --- tests/test_group_membership_sync.py | 4 ++-- tests/test_group_ownership_sync.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/test_group_membership_sync.py b/tests/test_group_membership_sync.py index b7edd0fa..ec08c873 100644 --- a/tests/test_group_membership_sync.py +++ b/tests/test_group_membership_sync.py @@ -323,10 +323,10 @@ def run_sync( okta.initialize("test.okta.com", "test-token") mocker.patch.object(okta, "list_groups", return_value=okta_groups) - mocker.patch.object(okta, "list_users_for_group", side_effect=user_membership_func) - mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules) + mocker.patch.object(okta, "add_user_to_group", return_value=None) + mocker.patch.object(okta, "async_add_user_to_group", return_value=None) sync_group_memberships(act_as_authority) diff --git a/tests/test_group_ownership_sync.py b/tests/test_group_ownership_sync.py index 234d6831..d7dc3257 100644 --- a/tests/test_group_ownership_sync.py +++ b/tests/test_group_ownership_sync.py @@ -401,10 +401,13 @@ def run_sync( okta.initialize("test.okta.com", "test-token", use_group_owners_api=True) mocker.patch.object(okta, "list_groups", return_value=okta_groups) - mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func) - mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules) + # Mock ownership-related methods + mocker.patch.object(okta, "add_owner_to_group", return_value=None) + mocker.patch.object(okta, "async_add_owner_to_group", return_value=None) + mocker.patch.object(okta, "remove_owner_from_group", return_value=None) + mocker.patch.object(okta, "async_remove_owner_from_group", return_value=None) sync_group_ownerships(act_as_authority) From 5f71b19ad81c40bd1b291082cf1ac28e03fa3a8e Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 12:14:08 -0700 Subject: [PATCH 6/7] revert test changes --- tests/test_group_membership_sync.py | 7 ++----- tests/test_group_ownership_sync.py | 10 ++-------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/tests/test_group_membership_sync.py b/tests/test_group_membership_sync.py index ec08c873..2023b222 100644 --- a/tests/test_group_membership_sync.py +++ b/tests/test_group_membership_sync.py @@ -319,14 +319,11 @@ def run_sync( groups_with_rules: set[str] = set(), ) -> list[OktaGroup]: with Session(db.engine) as session: - # Initialize Okta service - okta.initialize("test.okta.com", "test-token") - mocker.patch.object(okta, "list_groups", return_value=okta_groups) + mocker.patch.object(okta, "list_users_for_group", side_effect=user_membership_func) + mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules) - mocker.patch.object(okta, "add_user_to_group", return_value=None) - mocker.patch.object(okta, "async_add_user_to_group", return_value=None) sync_group_memberships(act_as_authority) diff --git a/tests/test_group_ownership_sync.py b/tests/test_group_ownership_sync.py index d7dc3257..a4d03ac8 100644 --- a/tests/test_group_ownership_sync.py +++ b/tests/test_group_ownership_sync.py @@ -397,17 +397,11 @@ def run_sync( groups_with_rules: set[str] = set(), ) -> list[OktaGroup]: with Session(db.engine) as session: - # Initialize Okta service with group owners API enabled - okta.initialize("test.okta.com", "test-token", use_group_owners_api=True) - mocker.patch.object(okta, "list_groups", return_value=okta_groups) + mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func) + mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules) - # Mock ownership-related methods - mocker.patch.object(okta, "add_owner_to_group", return_value=None) - mocker.patch.object(okta, "async_add_owner_to_group", return_value=None) - mocker.patch.object(okta, "remove_owner_from_group", return_value=None) - mocker.patch.object(okta, "async_remove_owner_from_group", return_value=None) sync_group_ownerships(act_as_authority) From e3ee017413f0f68a200c129e86a13e59b61ee8e4 Mon Sep 17 00:00:00 2001 From: Jill Date: Fri, 13 Jun 2025 12:26:13 -0700 Subject: [PATCH 7/7] resolve test mock race condition in run_sync --- tests/test_group_ownership_sync.py | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/test_group_ownership_sync.py b/tests/test_group_ownership_sync.py index a4d03ac8..9ac7d14e 100644 --- a/tests/test_group_ownership_sync.py +++ b/tests/test_group_ownership_sync.py @@ -396,16 +396,32 @@ def run_sync( act_as_authority: bool, groups_with_rules: set[str] = set(), ) -> list[OktaGroup]: - with Session(db.engine) as session: - mocker.patch.object(okta, "list_groups", return_value=okta_groups) - - mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func) - - mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules) + # Initialize Okta service first + okta.initialize("test.okta.com", "test-token") + + # Mock the okta module functions + mocker.patch.object(okta, "list_groups", return_value=okta_groups) + mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func) + mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules) + + # Check if these are already mocked by the test + if not hasattr(okta.add_owner_to_group, "_mock_name"): + mocker.patch.object(okta, "add_owner_to_group", return_value=None) + if not hasattr(okta.remove_owner_from_group, "_mock_name"): + mocker.patch.object(okta, "remove_owner_from_group", return_value=None) + + # Mock OktaService for ModifyGroupUsers + mock_service = mocker.MagicMock() + mock_service.okta_client = mocker.MagicMock() + mock_service.use_group_owners_api = False + mocker.patch("api.services.okta_service.OktaService", return_value=mock_service) + with Session(db.engine) as session: sync_group_ownerships(act_as_authority) + session.commit() - return session.query(OktaUserGroupMember).all() + db.session.expire_all() + return db.session.query(OktaUserGroupMember).all() def _get_group_owners(db: SQLAlchemy, group_id: str) -> dict[str, OwnershipDetails]: