From 40fb6ea80ec42af13800f679ddff930ea39908cd Mon Sep 17 00:00:00 2001 From: Rishi Gupta Date: Tue, 14 Mar 2017 19:48:55 -0700 Subject: [PATCH] lint: Prevent importing from zerver in migrations. --- docs/schema-migrations.md | 33 ++++++++++++++++++++++++--------- tools/lint-all | 7 +++++-- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/docs/schema-migrations.md b/docs/schema-migrations.md index aba4eac369..2f224ca627 100644 --- a/docs/schema-migrations.md +++ b/docs/schema-migrations.md @@ -35,15 +35,30 @@ migrations. to use the batch update tools in `zerver/lib/migrate.py` (originally written to work with South) for doing larger database migrations. -* **Accessing models in RunPython migrations**. When writing a - migration that includes custom python code (aka `RunPython`), you - cannot just use `from zerver.models import UserProfile` to access - models; that would import the model as it is right now. What you - want is to import a version of model "as of just before the - currently executing migration". You can do this inside the relevant - migration function with `apps.get_model`. We have a linter rule to - warn about this sort of issue, since it often manifests long after - the actual mistake. +* **Accessing code and models in RunPython migrations**. When writing + a migration that includes custom python code (aka `RunPython`), you + almost never want to import code from `zerver` or anywhere else in + the codebase. If you imagine the process of upgrading a Zulip + server, it goes as follows: first a server admin checks out a recent + version of the code, and then runs any migrations that were added + between the last time they upgraded and the current check out. Note + that for each migration, this means the migration is run using the + code in the server admin's check out, and not the code that was there at the + time the migration was written. This can be a difference of + thousands of commits for installations that are only upgraded + occasionally. It is hard to reason about the effect of a code change + on a migration that imported it so long ago, so we recommend just + copying any code you're tempted to import into the migration file + directly, and have a linter rule enforcing this. + + There is one special case where this doesn't work: you can't copy + the definition of a model (like `Realm`) into a migration, and you + can't import it from `zerver.models` for the reasons above. In this + situation you should use Django's `apps.get_model` to get access to + a model as it is at the time of a migration. Note that this will + work for doing something like `Realm.objects.filter(..)`, but + shouldn't be used for accessing `Realm.subdomain` or anything not + related to the Django ORM. * **Making large migrations work**. Major migrations should have a few properties: diff --git a/tools/lint-all b/tools/lint-all index 16f3ed2caa..f40d0100fe 100755 --- a/tools/lint-all +++ b/tools/lint-all @@ -410,9 +410,12 @@ def build_custom_checkers(by_lang): 'include_only': set(["zerver/views/"]), 'description': 'Please use access_stream_by_*() to fetch Stream objects', }, - {'pattern': 'from zerver.models import', + {'pattern': '^from (zerver|analytics|confirmation)', 'include_only': set(["/migrations/"]), - 'description': "Don't import models in migrations; see docs/schema-migrations.md for solution", + 'exclude': set(['zerver/migrations/0032_verify_all_medium_avatar_images.py', + 'zerver/migrations/0041_create_attachments_for_old_messages.py', + 'zerver/migrations/0060_move_avatars_to_be_uid_based.py']), + 'description': "Don't import models or other code in migrations; see docs/schema-migrations.md", }, {'pattern': 'datetime[.](now|utcnow)', 'include_only': set(["zerver/", "analytics/"]),