Upgrade write-it to Django 1.7

mhl commented 8 years ago

This is a fairly significant upgrade, since it's the one where we switch from database migrations based on South to its replacement, the new Django core migrations. It's best to do this upgrade and check it's all working and then do the upgrade to Django 1.8 afterwards.

The trickiest bit of this is that it requires an update to django-tastypie which needs some corresponding updates to write-it's tastypie resources.

Note that I haven't been exhaustively through everything in the release notes; since there's such good test coverage, hopefully this has caught any major problems.

Connects to: #718

coveralls commented 8 years ago

Coverage remained the same at 98.466% when pulling d987e681516a767bb7950beba01b1dfc4880be27 on django-1.7-upgrade into d3b833b71923be7da5bb56d71426b224e6d2dd64 on master.

chrismytton commented 8 years ago

@mhl I'm getting an error when I try to run the migrations on this branch:

(writeit-virtualenv)vagrant@precise64:/vagrant$ ./manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: pagination, annoying, django_admin_bootstrapped, django_object_actions, subdomains, django_extensions, haystack, popit, celery_haystack, formtools, djangoplugins
  Apply all migrations: contactos, mailit, admin, sessions, djcelery, sites, auth, tastypie, default, contenttypes, nuntium
Synchronizing apps without migrations:
  Creating tables...
  Installing custom SQL...
  Installing indexes...
Running migrations:
  Applying contenttypes.0001_initial... FAKED
  Applying auth.0001_initial... FAKED
  Applying admin.0001_initial... FAKED
  Applying sites.0001_initial... FAKED
  Applying contactos.0001_initial... FAKED
  Applying nuntium.0001_initial... FAKED
  Applying contactos.0002_contact_writeitinstance... OK
  Applying default.0001_initial... FAKED
  Applying default.0002_add_related_name... OK
  Applying djcelery.0001_initial... FAKED
  Applying mailit.0001_initial... FAKED
  Applying mailit.0002_auto_20160705_0552...Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/base.py", line 338, in execute
    output = self.handle(*args, **options)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/commands/migrate.py", line 161, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/migrations/executor.py", line 68, in migrate
    self.apply_migration(migration, fake=fake)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/migrations/executor.py", line 102, in apply_migration
    migration.apply(project_state, schema_editor)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/migrations/migration.py", line 108, in apply
    operation.database_forwards(self.app_label, schema_editor, project_state, new_state)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/migrations/operations/fields.py", line 37, in database_forwards
    field,
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/sqlite3/schema.py", line 179, in add_field
    self._remake_table(model, create_fields=[field])
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/sqlite3/schema.py", line 146, in _remake_table
    self.quote_name(model._meta.db_table),
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/schema.py", line 111, in execute
    cursor.execute(sql, params)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 81, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/db/backends/sqlite3/base.py", line 485, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.IntegrityError: mailit_mailittemplate__new.writeitinstance_id may not be NULL

mhl commented 8 years ago

@chrismytton To explain what's happening here (as I understand it):

There are circular dependencies between the models in the three Django apps in write-it. When you run ./manage.py makemigrations to generate the new initial migrations, it's smart enough to notice this, and creates two step migrations to get around this; it creates the table without the problematic foreign keys in the first one, and then adds those foreign keys in the second. This works fine when being run from scratch, but if you have data in the database then the second migration needs to be faked. (The first migration is detected as being an initial migration for tables that already exist so is automatically faked in Django 1.7 (and would be automatically faked with --fake-initial under 1.8) but the second isn't automatically faked since the table already exists.) So one might think that all you should need to do when this migration fails is to do is to run:

./manage.py migrate --fake mailit 0002_auto_20160705_0552

The problematic things here are:

Some of the 0002 migrations that appeared to succeed were actually destroying data in SQLite :angry: (see below)
It would need manual intervention (or some updates to the continuous deploy script - where is that?) to deploy the upgrade.
I think you said that the production FCI instance is using MySQL - unfortunately MySQL doesn't support schema changes in transactions (!) so failed migrations might leave the database in an inconsistent state. (However, hopefully the migrations will fail in a more helpful way with MySQL than with SQLite - see below for more on that.)

n.b. Just for interest, this wouldn't be a problem in Django 1.9, because you could set Migration.initial on the second migration, and it then wouldn't try to add any fields that already exist: https://docs.djangoproject.com/en/1.9/topics/migrations/#django.db.migrations.Migration.initial - presumably that would be set in all the generated initial migrations under 1.9

You might be asking, though, "How did the contactos.0002_contact_writeitinstance succeed then?", which is a good question. The same question arises with the first two AddField operations in mailit.0002_auto_20160705_0552. I believe that with these Django 1.7 generated migrations with SQLite appear to succeed, but in fact are trashing any data that was in the foreign key previously.

For reasons I don't understand, the new foreign key columns aren't being added with an ALTER TABLE command; instead, the following sequence happens (to take the example of adding the answer OneToOneField to nuntium.Answer to rawincomingemail):

CREATE TABLE "mailit_rawincomingemail__new" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "answer_id" integer NULL UNIQUE REFERENCES "nuntium_answer" ("id"), "content" text NOT NULL, "problem" bool NOT NULL, "message_id" varchar(2048) NOT NULL)

INSERT INTO "mailit_rawincomingemail__new" ("content", "problem", "answer_id", "id", "message_id") SELECT "content", "problem", NULL, "id", "message_id" FROM "mailit_rawincomingemail"

DROP TABLE "mailit_rawincomingemail"

ALTER TABLE "mailit_rawincomingemail__new" RENAME TO "mailit_rawincomingemail"

This would be OK if there was no answer_id column in the existing table, but in this situation there will be, so it's replacing all that data with NULL. Unfortunately, this means that if you look at the contactos_contact table, you'll see that the 0002_contact_writeitinstance migration will have set the writeitinstance column to NULL, overwriting the data that was there :( The migration of mailit_mailittemplate only fails because the foreign key column doesn't allow NULL, whereas the others added in the 0002 migrations do.

So with SQLite, certainly, it's all really bad. We need to investigate:

What SQL will be run if MySQL is the database backend? If it's using ALTER TABLE ... ADD COLUMN, that should be OK; the 0002 migrations should fail at the first operation, and can be faked one-by-one. This would mean taking those steps manually on the server, though, so...
Whether we can adjust the deployment script to fake the right migrations if it detects that we're now using Django core migrations but none have been run yet.
Can we make the migrations error on SQLite so that other developers don't lose data in this situation?

Other alternative approaches:

Create new apps and move models between them to remove the circular dependencies between the apps before generating the initial migrations. That would involve more South migrations while we're still on Django 1.6, which would need to be deployed everywhere first.
Before deploying on the server, build the migration to django-popolo on top of this (so we can get rid of popit-django and its dependency on South which breaks under 1.8), then do the migration to Django 1.8, then to 1.9, and mark the 0001 and 0002 migrations with Migration.initial = True. I mention this option for completeness; I don't think it's very sensible. We may not want to migrate past 1.8 (the long term support version) and it's less likely that Python package dependencies will work on 1.9 than 1.8. Also it's a lot of work before being able to deploy anything live.

mhl commented 8 years ago

Create new apps and move models between them to remove the circular dependencies between the apps before generating the initial migrations. That would involve more South migrations while we're still on Django 1.6, which would need to be deployed everywhere first.

This is the approach I've tried instead; it consists of two PRs:

... so I'm closing this in favour of them.

ciudadanointeligente / write-it

Upgrade write-it to Django 1.7 #1161