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

Remove tcms.core.migrations.0001_django_comments__object_pk #157

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Feb 23, 2017

this migration is modifying a 3rd party model and was overriding
app_label to django_comments in order to gain access to the model!

However due to how Django records executed migrations it was
recorded as django_comments|0001... instead of core|0001...! Thus
the migration was seen as still pending and could lead to problems
when applying further migrations.

@tkdchen please review. I think we've got the migration right this time.

Show outdated Hide outdated CHANGELOG.rst
@@ -13,6 +13,8 @@ Change Log
- Add migration for django_comments
- Upgrade Django to 1.8.14
- Upgrade to django-tinymce 2.4.0 (Mr. Senko)
- Refactor tcms.core.migrations.0001_django_comments__object_pk.
PR #157 (Mr. Senko)

This comment has been minimized.

@tkdchen

tkdchen Feb 26, 2017

Member

Part #157 is not in the summary.

When make new release, I'll make a full list of change history. I think it is not necessary to change this file every time along with a patch.

@tkdchen

tkdchen Feb 26, 2017

Member

Part #157 is not in the summary.

When make new release, I'll make a full list of change history. I think it is not necessary to change this file every time along with a patch.

Show outdated Hide outdated tcms/core/migrations/0001_django_comments__object_pk.py
@@ -4,16 +4,41 @@
from django.db import migrations, models
class DjangoCommentsAlterField(migrations.AlterField):
"""
Custom migration operation which alters the `django_comments`

This comment has been minimized.

@tkdchen

tkdchen Feb 26, 2017

Member

I'd like to keep docstring in this format throughout Nitrate.

"""summary

More description (could be multiple lines)
"""

Please update this dostring.

@tkdchen

tkdchen Feb 26, 2017

Member

I'd like to keep docstring in this format throughout Nitrate.

"""summary

More description (could be multiple lines)
"""

Please update this dostring.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Feb 26, 2017

Member

Generally LGTM except the two comments.

Member

tkdchen commented Feb 26, 2017

Generally LGTM except the two comments.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 2, 2017

Member

In what cases the problem would happen?

Member

tkdchen commented Mar 2, 2017

In what cases the problem would happen?

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 2, 2017

Contributor

In what cases the problem would happen?

  1. execute ./manage.py migrate (possibly for the first time)
  2. Because of how Django works (described in the first comment) this migration will be considered as still pending, although it was already applied
  3. The next time you try to execute ./manage.py migrate Django will try to apply the same migration and will fail.

Also during local development, when I start Nitrate via ./manage.py runserver Django was showing a warning (in the console) that I still have unapplied migrations.

Contributor

atodorov commented Mar 2, 2017

In what cases the problem would happen?

  1. execute ./manage.py migrate (possibly for the first time)
  2. Because of how Django works (described in the first comment) this migration will be considered as still pending, although it was already applied
  3. The next time you try to execute ./manage.py migrate Django will try to apply the same migration and will fail.

Also during local development, when I start Nitrate via ./manage.py runserver Django was showing a warning (in the console) that I still have unapplied migrations.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 2, 2017

Member

After run migrate for the first time, 0001_django_comments__object_pk is marked as applied. Then, run migrate again, there is message

  Your models have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

It seems Django does not know that migration, and I got this

./manage.py makemigrations --dry-run -v 3
Migrations for 'django_comments':
  0004_auto_20170403_0004.py:
    - Alter field object_pk on comment
Full migrations file '0004_auto_20170403_0004.py':
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

    dependencies = [
        ('django_comments', '0003_add_submit_date_index'),
    ]

    operations = [
        migrations.AlterField(
            model_name='comment',
            name='object_pk',
            field=models.TextField(verbose_name='object ID'),
        ),
    ]

File 0004_auto_20170403_0004.py is put into django_comments/migrations/.

Django treats it as a model change, and change it migrated int field back to a text field. Why does this happen?

Member

tkdchen commented Apr 2, 2017

After run migrate for the first time, 0001_django_comments__object_pk is marked as applied. Then, run migrate again, there is message

  Your models have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

It seems Django does not know that migration, and I got this

./manage.py makemigrations --dry-run -v 3
Migrations for 'django_comments':
  0004_auto_20170403_0004.py:
    - Alter field object_pk on comment
Full migrations file '0004_auto_20170403_0004.py':
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

    dependencies = [
        ('django_comments', '0003_add_submit_date_index'),
    ]

    operations = [
        migrations.AlterField(
            model_name='comment',
            name='object_pk',
            field=models.TextField(verbose_name='object ID'),
        ),
    ]

File 0004_auto_20170403_0004.py is put into django_comments/migrations/.

Django treats it as a model change, and change it migrated int field back to a text field. Why does this happen?

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 2, 2017

Contributor

File 0004_auto_20170403_0004.py is put into django_comments/migrations/.

Django treats it as a model change, and change it migrated int field back to a text field. Why does this happen?

@tkdchen this is because we are modifying a model from a 3rd party application inside our migration. That whole operation is one big mess which we aren't supposed to be doing. What's happening (even when PR is applied) is this:

  1. core/migrations/0001_django_comments__object_pk is applied and django_comments.object_pk is converted to int
  2. With this PR applied the migration is marked as applied. Without it it is marked as non applied, while at the same time the same migration is recorded as applied but for the django_comments app
  3. Because the stock django_comments model is now different the next time you try to run migrate it will detect the difference.
  4. If you run makemigrations Django will correctly create a new migration file to reflect the current state of the models. However Django will place this new file under django_comments/migrations because this is where the changes occurred.

Using RawSQL like it was before would help IMO (I will test it in a sec) because the underlying model will be different and we're back to 4).

To make things worse the 'ALTER COLUMN' statement is not supported on SQLite but using non-SQL migrations Django is able to workaround this. Also there is a bug in handling the migration on PostgreSQL which has been fixed in later versions of Django, see this commit from #171:
ef493e2

To recap we are modifying django_comments' models OUTSIDE of django_comments and this really confuses the Django migration subsystem. The only alternative I can think about is to take our migration file and tell setup.py to install it under django_comments/migrations. This isn't necessarily better and has its own set of problems.

We can also vendor in the entire django_comments app, add the migration there and carry it around in the Nitrate source code but that defeats the purpose of using packages as dependencies.

The other alternative would be to leave object_pk unchanged, maybe add an index and use int() casting where necessary. I've had something like that in one of my older PRs but then we decided to go ahead with the migration instead.

Contributor

atodorov commented Apr 2, 2017

File 0004_auto_20170403_0004.py is put into django_comments/migrations/.

Django treats it as a model change, and change it migrated int field back to a text field. Why does this happen?

@tkdchen this is because we are modifying a model from a 3rd party application inside our migration. That whole operation is one big mess which we aren't supposed to be doing. What's happening (even when PR is applied) is this:

  1. core/migrations/0001_django_comments__object_pk is applied and django_comments.object_pk is converted to int
  2. With this PR applied the migration is marked as applied. Without it it is marked as non applied, while at the same time the same migration is recorded as applied but for the django_comments app
  3. Because the stock django_comments model is now different the next time you try to run migrate it will detect the difference.
  4. If you run makemigrations Django will correctly create a new migration file to reflect the current state of the models. However Django will place this new file under django_comments/migrations because this is where the changes occurred.

Using RawSQL like it was before would help IMO (I will test it in a sec) because the underlying model will be different and we're back to 4).

To make things worse the 'ALTER COLUMN' statement is not supported on SQLite but using non-SQL migrations Django is able to workaround this. Also there is a bug in handling the migration on PostgreSQL which has been fixed in later versions of Django, see this commit from #171:
ef493e2

To recap we are modifying django_comments' models OUTSIDE of django_comments and this really confuses the Django migration subsystem. The only alternative I can think about is to take our migration file and tell setup.py to install it under django_comments/migrations. This isn't necessarily better and has its own set of problems.

We can also vendor in the entire django_comments app, add the migration there and carry it around in the Nitrate source code but that defeats the purpose of using packages as dependencies.

The other alternative would be to leave object_pk unchanged, maybe add an index and use int() casting where necessary. I've had something like that in one of my older PRs but then we decided to go ahead with the migration instead.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 2, 2017

Contributor

Update: using RunSQL doesn't seem to suffer from this problem, but it does suffer from the unsupported on SQLite problem, see:
https://travis-ci.org/MrSenko/Nitrate/builds/217867272

I will have to investigate more before I'm certain what is happening inside of Django. It looks like we could special case MySQL and Postgres and use RunSQL here but the model wouldn't know the field changed to int (so do we really gain anything) ?? And need to figure out how to deal with SQLite for local development. I will look at this the next week and update this PR.

Contributor

atodorov commented Apr 2, 2017

Update: using RunSQL doesn't seem to suffer from this problem, but it does suffer from the unsupported on SQLite problem, see:
https://travis-ci.org/MrSenko/Nitrate/builds/217867272

I will have to investigate more before I'm certain what is happening inside of Django. It looks like we could special case MySQL and Postgres and use RunSQL here but the model wouldn't know the field changed to int (so do we really gain anything) ?? And need to figure out how to deal with SQLite for local development. I will look at this the next week and update this PR.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 4, 2017

Member

So, for this complicated case, I think this migration can be dropped, as this is an improvement for Nitrate, it's good to have, but not must-have and also not a blocker for using Nitrate. The change in this migration could be documented well as a suggestion. For now, I think it is not worth to spend too much time to fix this. However, I'm glad to see a solution to make it possible.

Member

tkdchen commented Apr 4, 2017

So, for this complicated case, I think this migration can be dropped, as this is an improvement for Nitrate, it's good to have, but not must-have and also not a blocker for using Nitrate. The change in this migration could be documented well as a suggestion. For now, I think it is not worth to spend too much time to fix this. However, I'm glad to see a solution to make it possible.

Remove tcms.core.migrations.0001_django_comments__object_pk
this migration is modifying a 3rd party model and Django gets
really confused about this! See the discussion in #157 about why
we've decided to drop the migration!

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 9, 2017

Coverage Status

Coverage decreased (-0.02%) to 45.38% when pulling 24fc88e on MrSenko:fix_django_comments_migration into 980b07b on Nitrate:develop.

Coverage Status

Coverage decreased (-0.02%) to 45.38% when pulling 24fc88e on MrSenko:fix_django_comments_migration into 980b07b on Nitrate:develop.

@atodorov atodorov changed the title from Refactor tcms.core.migrations.0001_django_comments__object_pk to Remove tcms.core.migrations.0001_django_comments__object_pk Apr 9, 2017

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 9, 2017

Contributor

@tkdchen I've converted this into an empty migration to avoid disrupting existing installations. There are tests which cover add/remove comments for test case runs, I have also verified it manually. Everything seems to be working fine. Also rebased to latest.

Can you merge this PR so I can move on to the next ones ?

Contributor

atodorov commented Apr 9, 2017

@tkdchen I've converted this into an empty migration to avoid disrupting existing installations. There are tests which cover add/remove comments for test case runs, I have also verified it manually. Everything seems to be working fine. Also rebased to latest.

Can you merge this PR so I can move on to the next ones ?

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 10, 2017

Member

Cool. Thank you very much. I'll look at this latest changes soon, today or tomorrow.

Member

tkdchen commented Apr 10, 2017

Cool. Thank you very much. I'll look at this latest changes soon, today or tomorrow.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 10, 2017

Member

Tested against a fresh db in steps

./manage migrate
./manage migrate --list
./manage migrate
make test

Each of them looks work well, there is no issue found. Merge this change. Thank you very much.

Member

tkdchen commented Apr 10, 2017

Tested against a fresh db in steps

./manage migrate
./manage migrate --list
./manage migrate
make test

Each of them looks work well, there is no issue found. Merge this change. Thank you very much.

@tkdchen tkdchen merged commit 8f45beb into Nitrate:develop Apr 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment