Skip to content

fix(#1591): Fixes schema connection for RunPython migration steps#1678

Open
igor-petrik-invitae wants to merge 3 commits into
django-oauth:masterfrom
igor-petrik-invitae:fix-#1591
Open

fix(#1591): Fixes schema connection for RunPython migration steps#1678
igor-petrik-invitae wants to merge 3 commits into
django-oauth:masterfrom
igor-petrik-invitae:fix-#1591

Conversation

@igor-petrik-invitae

Copy link
Copy Markdown

Fixes #1591

Description of the Change

Adds use of schema_editor connection for RunPython migrations, as recommended by Django.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated (N/A?)
  • CHANGELOG.md updated (only for user relevant changes) (N/A?)
  • author name in AUTHORS
  • tests/app/idp updated to demonstrate new features (N/A?)
  • tests/app/rp updated to demonstrate new features (N/A?)

@igor-petrik-invitae igor-petrik-invitae changed the title fix(#1591): Adds reproducible test fix(#1591): Fixes schema connection for RunPython migration steps. Apr 1, 2026
@igor-petrik-invitae igor-petrik-invitae changed the title fix(#1591): Fixes schema connection for RunPython migration steps. fix(#1591): Fixes schema connection for RunPython migration steps Apr 1, 2026
@dopry dopry requested a review from Copilot April 1, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses Django’s guidance for RunPython migrations by ensuring data migrations use the schema_editor connection (rather than implicitly using the default DB connection), which helps prevent multi-DB connection/locking issues like the hang reported in #1591.

Changes:

  • Update RunPython migration functions to query/save using schema_editor.connection.alias.
  • Add a new tox environment to run migrations against a non-default database configuration.
  • Add contributor to AUTHORS.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tox.ini Adds migrate_non_default tox env to exercise migrations on a non-default DB alias.
tests/non_default_db_settings.py Introduces settings intended to validate migration behavior when using --database=secondary.
oauth2_provider/migrations/0012_add_token_checksum.py Uses schema_editor DB alias for iteration and saves in RunPython.
oauth2_provider/migrations/0006_alter_application_client_secret.py Uses schema_editor DB alias for queryset and saves in RunPython.
AUTHORS Adds a new author entry.

Comment thread tests/non_default_db_settings.py Outdated
Comment thread oauth2_provider/migrations/0006_alter_application_client_secret.py
@codecov

codecov Bot commented Apr 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dopry

dopry commented Apr 1, 2026

Copy link
Copy Markdown
Member

@igor-petrik-invitae Thanks for the PR! I was giving it a review. When I check out the first commit, the migration is failing on 0006_.... Are we sure this is truly replicating the issue for migration 0012_... and not exposing a a different, but valid bug, in migrations handling of multiple databases?

@igor-petrik-invitae

Copy link
Copy Markdown
Author

I want to point out that this is not multiple databases; the point of the test is to test different where the connection used for migrations is a different connection to the same database.

You are right, that 0006 does not fail on my Postgres database, where 0012 does. I was not sure how to write a test in this repository that would use a postgres database, but if you take the same config as here and substitute a postgres database instead of a sqlite database, that should reproduce the original deadlock issue.

@dopry

dopry commented Apr 3, 2026

Copy link
Copy Markdown
Member

If you're at all interested in setting up the tests with postgres we need to cover local and CI environmnts.
For local I tend use docker compose. Here is a subset of one of my projects.. It's single instance not multiple instance...
docker-compose.yml

# re-usable yaml anchors
x-task: &task
  # docker stack deploy restart policy for tasks
  deploy:
    restart_policy:
      condition: on-failure
    replicas: 1
  # docker compose stand-alone restart policy for tasks
  restart: on-failure

x-task-ensure-database: &task-ensure-database
  <<: *task
  image: postgres:15
  entrypoint: |
    sh -c "
    echo $PGPASSWORD;
    psql -v ON_ERROR_STOP=0 --host postgres --username \"$${POSTGRES_USER}\" <<EOSQL
      CREATE ROLE $${DB_NAME} LOGIN PASSWORD '$${DB_PASS}';
      CREATE DATABASE $${DB_NAME};
      GRANT ALL ON DATABASE $${DB_NAME} TO $${DB_NAME};
    EOSQL"
  # you must set the following environment variables where you use this anchor.
  # environment:
    # - POSTGRES_USER
    # - PGPASSWORD=${POSTGRES_PASS:-jangl}
    # - DB_NAME=service
    # - DB_PASS=password

volumes:
  postgres-data:

services:
  # a postgres container for providing database access.
  postgres:
    image: postgres:15
    environment:
      - POSTGRES_USER=postgres
      - POSTGRES_PASSWORD=password
    ports:
      - "5432:5432"
    volumes:
      - postgres-data:/var/lib/postgresql/data

ensure-db-alpha:
    <<: *task-ensure-database
    environment:
      - PGPASSWORD=password
      - POSTGRES_USER=postgres
      - DB_NAME=alpha
      - DB_PASS=password
    depends_on:
      - postgres

in CI you can add postgres to the workflow with the services key. I set it up on another project I work on a while back wagtail-grapple. I would love to have testing on postgres.. there are quite a few issues that never surface when testing soley with sqlite.

@dopry dopry added this to the 3.3.1 milestone May 30, 2026
@dopry

dopry commented May 30, 2026

Copy link
Copy Markdown
Member

@igor-petrik-invitae I've added postgres primary/replica testing to our test environments. that should provide a better baseline for testing this feature. Do you have time to update this PR?

@dopry dopry force-pushed the fix-#1591 branch 4 times, most recently from 5d65116 to 3d99b87 Compare May 30, 2026 18:56
@dopry dopry requested a review from Copilot May 30, 2026 21:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comment thread tests/non_default_db_settings.py Outdated
Comment thread tests/test_migration_db_alias.py Outdated
Comment thread .vscode/settings.json Outdated
@dopry dopry force-pushed the fix-#1591 branch 3 times, most recently from 9ca1a53 to c0596f8 Compare June 1, 2026 17:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New install hangs on 0012 migration

3 participants