Skip to content

Comments

swev-id: django__django-7530 | Fix makemigrations router.allow_migrate() consistency-check model selection#545

Open
rowan-stein wants to merge 9 commits intodjango__django-7530from
fix/allow-migrate-consistency-check-django__django-7530
Open

swev-id: django__django-7530 | Fix makemigrations router.allow_migrate() consistency-check model selection#545
rowan-stein wants to merge 9 commits intodjango__django-7530from
fix/allow-migrate-consistency-check-django__django-7530

Conversation

@rowan-stein
Copy link
Collaborator

This PR addresses Issue #543: makemigrations router.allow_migrate() calls for consistency checks use incorrect (app_label, model) pairs.

Summary

  • Replace per-app_label x all-models evaluation with per-app_config migratable models evaluation using router.get_migratable_models(...), avoiding invalid (app_label, model_name) combinations during consistency checks.
  • Aims to preserve behavior: run MigrationLoader.check_consistent_history(connection) only for DB aliases that have at least one migratable model.

Upstream reference: django#7530

Reproduction steps (manual)

  • Environment: Use a Python version compatible with this Django snapshot (e.g., Python 3.6). Ensure PYTHONPATH points to this repo so manage.py uses this code.
  • Create a demo project with sharded router and two databases as follows:
# Setup (example)
export PYTHONPATH=/workspace/django
# Create project/apps
/workspace/django/django/bin/django-admin.py startproject shard_demo /workspace/shard_demo
cd /workspace/shard_demo
python manage.py startapp app_a
python manage.py startapp app_b

# shard_router.py
cat <<'ROUTER' > shard_router.py
class ShardRouter(object):
    routing_map = {
        ('app_a', 'AModel'): 'default',
        ('app_b', 'BModel'): 'shard_b',
    }
    def allow_migrate(self, db, app_label, model_name=None, **hints):
        if model_name is None:
            return None
        target = self.routing_map.get((app_label, model_name))
        if target is None:
            raise ValueError('Invalid app/model pair: %s.%s' % (app_label, model_name))
        return target == db
ROUTER

# app_a/models.py
cat <<'A' > app_a/models.py
from django.db import models
class AModel(models.Model):
    name = models.CharField(max_length=20)
A

# app_b/models.py
cat <<'B' > app_b/models.py
from django.db import models
class BModel(models.Model):
    label = models.CharField(max_length=20)
B

# Update settings: add apps, add a second DB, enable router
python - <<'PY'
from pathlib import Path
s = Path('shard_demo/settings.py')
t = s.read_text()
t = t.replace(
    "    'django.contrib.staticfiles',\n]\n",
    "    'django.contrib.staticfiles',\n    'app_a',\n    'app_b',\n]\n",
)
t = t.replace(
    "'NAME': os.path.join(BASE_DIR, 'db.sqlite3'),\n    }\n}\n",
    "'NAME': os.path.join(BASE_DIR, 'db.sqlite3'),\n    },\n    'shard_b': {\n        'ENGINE': 'django.db.backends.sqlite3',\n        'NAME': os.path.join(BASE_DIR, 'db_shard_b.sqlite3'),\n    }\n}\n",
)
if "DATABASE_ROUTERS" not in t:
    t += "\nDATABASE_ROUTERS = ['shard_router.ShardRouter']\n"
s.write_text(t)
PY

# Reproduce
python manage.py makemigrations

Observed failure (representative stack trace before fix)

Traceback (most recent call last):
  File ".../django/core/management/commands/makemigrations.py", line 106, in handle
    router.allow_migrate(connection.alias, app_label, model_name=model._meta.object_name)
  File ".../django/db/utils.py", line 300, in allow_migrate
    allow = method(db, app_label, **hints)
  File ".../shard_demo/shard_router.py", line XX, in allow_migrate
    raise ValueError("Invalid app/model pair: %s.%s" % (app_label, model_name))
ValueError: Invalid app/model pair: app_b.AModel

Validation after fix

  • With this change, makemigrations should complete without raising due to invalid app/model pairs.
  • For each DB alias, loader.check_consistent_history(connection) runs iff at least one model is migratable for that alias across the installed apps.

Notes

@rowan-stein rowan-stein requested a review from a team December 24, 2025 18:31
Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

The production change looks reasonable, but it changes the way we invoke . The regression test in still asserts the old call signature, so the suite will fail until we update it.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for adding the explicit assertions in ; the regression now covers the new contract (model hint + lowercase name). Looks good to me.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

The migrations logic and tests look good, but the new uses of collections.abc break Django’s supported Python 2.7 runtimes. Please keep using the existing aliases (or route through six.moves.collections_abc) so we don’t regress compatibility.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Compatibility fix looks good—the usage now falls back cleanly on Python 2, so we preserve support while keeping the migrations change and tests intact. Thanks!

@rowan-stein rowan-stein changed the base branch from django__django-7530 to main January 11, 2026 20:18
@rowan-stein rowan-stein changed the base branch from main to django__django-7530 January 12, 2026 20:13
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.

2 participants