Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup migrations #142

Merged
merged 6 commits into from
Jun 17, 2018
Merged

Cleanup migrations #142

merged 6 commits into from
Jun 17, 2018

Conversation

agateblue
Copy link
Owner

See #141 and #140.

CC @czlee @marcofvera @gonvaled

I cannot seem to reproduce the issue you are facing using the example project.

here is what I do:

cd /tmp
git clone git@github.com:EliotBerriot/django-dynamic-preferences.git
cd django-dynamic-preferences
git checkout cleanup-migrations
# create a virtualen env (I use vex, but you can use something else)
vex -m --python=python3.6 django-dynamic-preferences
# install the package
pip install -e .
pip install -r requirements.txt
cd example
python manage.py migrate

On the migrate part, this is the output I get:

Operations to perform:
  Apply all migrations: admin, auth, contenttypes, dynamic_preferences, dynamic_preferences_users, example, sessions, sites
Running migrations:
  Applying contenttypes.0001_initial... OK
  Applying auth.0001_initial... OK
  Applying admin.0001_initial... OK
  Applying admin.0002_logentry_remove_auto_add... OK
  Applying contenttypes.0002_remove_content_type_name... OK
  Applying auth.0002_alter_permission_name_max_length... OK
  Applying auth.0003_alter_user_email_max_length... OK
  Applying auth.0004_alter_user_username_opts... OK
  Applying auth.0005_alter_user_last_login_null... OK
  Applying auth.0006_require_contenttypes_0002... OK
  Applying auth.0007_alter_validators_add_error_messages... OK
  Applying auth.0008_alter_user_username_max_length... OK
  Applying auth.0009_alter_user_last_name_max_length... OK
  Applying dynamic_preferences.0001_initial... OK
  Applying dynamic_preferences.0002_auto_20150712_0332... OK
  Applying dynamic_preferences.0003_auto_20151223_1407... OK
  Applying dynamic_preferences.0004_move_user_model... OK
  Applying dynamic_preferences_users.0001_initial... OK
  Applying example.0001_initial... OK
  Applying sessions.0001_initial... OK
  Applying sites.0001_initial... OK
  Applying sites.0002_alter_domain_unique... OK

If I remove db.sqlite3 and try this again, without dynamic_preferences.users.apps.UserPreferencesConfig in example/settings.py, I get the same output (without the dynamic_preferences_users part, of course).

python manage.py flush works as well.

I'm not saying there is no issue in the package. There is probably one, and having the table lying around in the DB even if the user app is not installed is problematic.

However, I'm never encountering the same issues as you described in your comments (Here, for instance). Thus, I'm a bit blocked :/

By any chance, would someone facing the issue be able to share the complete and minimum set of instructions to reproduce this reliably?

@czlee
Copy link
Collaborator

czlee commented Jun 14, 2018

Thanks for having a look at this! I'll have a look within the next week (hopefully).

@czlee
Copy link
Collaborator

czlee commented Jun 14, 2018

I was too curious and ended up looking at it now. Turns out it's database backend-dependent. With SQLite it works fine; with PostgreSQL it fails (at least for me).

By the way, for me it works fine when 'dynamic_preferences.users.apps.UserPreferencesConfig' is in INSTALLED_APPS (as it is in the example project), so all commentary below assumes that line is removed/commented out.

Reproduction instructions

Perhaps annoyingly, since I've only got it to fail on PostgreSQL, reproducing it requires you to have PostgreSQL, and all the corresponding setup.

I put the offending configuration in a commit on my fork, on a branch called migrations-psql. You'll need to change <username> and <password> to relevant values for your PostgreSQL setup. (Sorry, I couldn't think of an easier way to do this.)

git clone git@github.com:czlee/django-dynamic-preferences.git
cd django-dynamic-preferences
git checkout migrations-psql
# Create and activate virtual environment (these are the commands I use, in case it matters)
# vex -m --python=python3.6 django-dynamic-preferences
python -m venv venv
source venv/bin/activate
# I think you have to use the requirements.txt in the example directory, right?
cd example
pip install -r requirements.txt
# Create database (if you haven't already); I called it 'preferences' in settings.py
createdb preferences
python manage.py migrate
python manage.py flush

What does flush do, and why does it work on SQLite but not PostgreSQL?

I did some more digging, and it turns out the offending SQL command (TRUNCATE) actually doesn't exist in SQLite.

With an SQLite backend, on the example project (with 'dynamic_preferences.users.apps.UserPreferencesConfig' removed from INSTALLED_APPS), python manage.py sqlflush (which prints the commands that would be run by python manage.py flush) shows this:

BEGIN;
DELETE FROM "auth_user";
DELETE FROM "django_content_type";
DELETE FROM "auth_permission";
DELETE FROM "auth_group";
DELETE FROM "django_site";
DELETE FROM "dynamic_preferences_globalpreferencemodel";
DELETE FROM "auth_user_groups";
DELETE FROM "example_mymodel";
DELETE FROM "auth_group_permissions";
DELETE FROM "django_admin_log";
DELETE FROM "django_session";
DELETE FROM "auth_user_user_permissions";
COMMIT;

When I inspected the SQLite database, the dynamic_preferences_users_userpreferencemodel table was still there (along with all the others). Presumably if you had data in it, it just stays untouched (and inconsequential), except that it might now have broken references to auth_user. Or maybe it cascade-deletes everything (to enforce foreign key constraints)—an operation I'm guessing is prohibited in the TRUNCATE command?

With a PostgreSQL backend, python manage.py sqlflush shows this:

BEGIN;
TRUNCATE "auth_user_groups", "example_mymodel", "django_site", "auth_group_permissions", "django_session", "dynamic_preferences_globalpreferencemodel", "auth_permission", "django_admin_log", "django_content_type", "auth_user", "auth_group", "auth_user_user_permissions";
SELECT setval(pg_get_serial_sequence('"django_admin_log"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"auth_permission"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"auth_group"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"auth_user"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"django_content_type"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"django_site"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"dynamic_preferences_globalpreferencemodel"','id'), 1, false);
SELECT setval(pg_get_serial_sequence('"example_mymodel"','id'), 1, false);
COMMIT;

As discussed in #140, that TRUNCATE command is where it fails.

(I haven't been able to reproduce #141, but I also haven't tried very hard.)

I realize this isn't a complete answer—I'm learning all this as I go—so I'm definitely open to further discussion, and appreciate your efforts with looking into this! Let me know if anything was unclear.

@agateblue
Copy link
Owner Author

Well, this is a fantastic insight. I only tried on SQLite, and knowing it fails on postgres will help reproduce the issue, thanks!

@agateblue
Copy link
Owner Author

@czlee I think I found a proper solution: I removed every remains of the user models from dynamic_preferences migrations, and made the initial migration for the dynamic_preferences_user app a real one.

Everything should work fine and consistently now. I tried it locally with a postgres database, and applying and reverting migrations work as expected.

Can you try this locally and tell me how it goes for you/review the PR if you have the time. I know it includes some unrelated CI/Django2 stuff, but it was blocking me while working on this.

If it works, I'll merge and publish a release and, hopefully, this will be behind us!

@agateblue agateblue force-pushed the cleanup-migrations branch 2 times, most recently from 687d6a9 to 833bac5 Compare June 16, 2018 09:43
@agateblue agateblue force-pushed the cleanup-migrations branch from 833bac5 to 983330b Compare June 16, 2018 09:46
@agateblue agateblue force-pushed the cleanup-migrations branch from 983330b to f40a00e Compare June 16, 2018 09:50
@czlee
Copy link
Collaborator

czlee commented Jun 17, 2018

Thanks! This works when migrating a blank database, as in, the flush command works great and the dynamic_preferences_users_userpreferencemodel is nowhere to be found when the users app isn't listed.

As expected, it neither helps nor harms my existing installation. I agree with the approach you've taken and described in the changelog, that is, the manual workaround of dropping the dynamic_preferences_users_userpreferencemodel table from the database if you're not using the users app.

So it looks good to me with respect to #140. 😃 I tried it both with the example project, and my actual project.

Thanks again!

@agateblue agateblue merged commit d0afd8f into develop Jun 17, 2018
@agateblue agateblue deleted the cleanup-migrations branch June 17, 2018 10:07
agateblue referenced this pull request Jun 17, 2018
1.6 (2018-06-17)
****************

- Fixed #141 and #141: migrations issues (see below)
- Dropped support for django < 1.11
- Dropped support for Python 3.4
- Better namespaces for urls

Better namespaces for urls
--------------------------

Historically, the package included multiple urls. To ensure compatibility with django 2
and better namespacing, you should update any references to those urls as described below:

+-------------------------------------+-------------------------------------+
| Old url                             | New url                             |
+=====================================+=====================================+
| dynamic_preferences.global          | dynamic_preferences:global          |
+-------------------------------------+-------------------------------------+
| dynamic_preferences.global.section  | dynamic_preferences:global.section  |
+-------------------------------------+-------------------------------------+
| dynamic_preferences.user            | dynamic_preferences:user            |
+-------------------------------------+-------------------------------------+
| dynamic_preferences.user.section    | dynamic_preferences:user.section    |
+-------------------------------------+-------------------------------------+

Migration cleanup
-----------------

This version includes a proper fix for migration issues.
Full background is available at https://github.com/EliotBerriot/django-dynamic-preferences/pull/142,
but here is the gist of it:

1. Early versions of dynamic_preferences included the user and global preferences models
   in the same app
2. The community requested a way to disable user preferences. The only way to do that
   was to move the user preference model in a dedicated app (dynamic_preferences_user
3. A migration was written to handle that transparently, but this was not actually possible
   to have something that worked for both existing and new installations
4. Thus, we ended up with issues such as #140 or #141, inconsistent db state, tables
   lying around in the database, etc.

I'd like to apologize to everyone impacted. By trying to make 3. completely transparent to everyone and
avoid a manual migration step for new installations, I actually made things worse.

This release should fix all that: any remains of the user app was removed from the main
app migrations. For any new user, it will be like nothing happened.

For existing installations with user preferences disabled, there is nothing to do,
apart from deleting the `dynamic_preferences_users_userpreferencemodel` table in your database.

For existing installations with user preferences enabled, there is nothing to do. You should have
``'dynamic_preferences.users.apps.UserPreferencesConfig'`` in your installed apps. If ``python manage.py migrate``
fails with ``django.db.utils.ProgrammingError: relation "dynamic_preferences_users_userpreferencemodel" already exists``,
this probably means you are upgrading for a really old release. In such event, simply skip the initial migration for the
``dynamic_preferences_user`` app by running ``python manage.py migrate dynamic_preferences_users 0001 --fake``.

Many thanks to all people who helped clearing this mess, especially @czlee.
@agateblue
Copy link
Owner Author

release 1.6 pushed on pypi :)

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.

3 participants