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

simple-history 2.6.0 generates a migration while 2.5.1 doesn't #512

Closed
atodorov opened this Issue Jan 14, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@atodorov
Copy link

atodorov commented Jan 14, 2019

Describe the bug
Kiwi TCMS uses simple-history as dependency. Until recently we had version 2.5.1 and we've upgraded to 2.6.0. The problem is that with 2.5.1 ./manage.py migrate/makemigrations doesn't detect any new changes but with 2.6.0 it does.

Here's how the initial migration looks like:

from django.db import migrations, models

        migrations.AddField(
            model_name='testplan',
            name='parent',
            field=models.ForeignKey(blank=True, null=True, on_delete=models.deletion.CASCADE,
                                    related_name='child_set', to='testplans.TestPlan'),
        ),

        migrations.CreateModel(
            name='HistoricalTestPlan',
            fields=[
                ('parent', models.ForeignKey(blank=True, db_constraint=False, null=True,
                                             on_delete=models.deletion.DO_NOTHING, related_name='+',
                                             to='testplans.TestPlan')),
            ]
        )

Here's what makemigrations produces with 2.6.0:

import django.db.models.deletion

        migrations.AlterField(
            model_name='historicaltestplan',
            name='parent',
            field=models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='testplans.HistoricalTestPlan'),
        ),

The only difference I see is how we reference the on_delete constraint.

To Reproduce
Steps to reproduce the behavior:

  1. git clone https://github.com/kiwitcms/Kiwi/
  2. pip install -r requirements/devel.txt
  3. ./manage.py migrate
  4. ./manage.py makemigrations <--- produces a new migration

Repeat the above with 2.5.1 instead and the last step tells you there are no changes detected

Environment (please complete the following information):

  • OS: Linux, RHEL 7
  • Django Simple History Version: 2.5.1 and 2.6
  • Django Version: 2.1.5
  • Database Version: SQLite

atodorov added a commit to kiwitcms/Kiwi that referenced this issue Jan 14, 2019

Test for missing migrations
originally reported in:
https://stackoverflow.com/questions/54177838/

depends on ./manage.py migrate exiting with non-zero:
django/django#10844

Also see:
treyhunner/django-simple-history#512
@rossmechanic

This comment has been minimized.

Copy link
Collaborator

rossmechanic commented Jan 14, 2019

Hey @atodorov thanks for this. There seems to be a difference in the to field. My guess is that the issue stems from #462 and I'm looking into it now. Will let you know when I have an update

@rossmechanic

This comment has been minimized.

Copy link
Collaborator

rossmechanic commented Jan 14, 2019

Found the issue @atodorov ! It's this line here https://github.com/treyhunner/django-simple-history/pull/462/files#diff-db0d12c32869d48141bfca2c94659322L203. In adding the deconstruct method to that code, we accidentally got rid of that line. DSH still supports foreign keys to the same object, but we accidentally got rid of support for using 'self' as the ForeignKey (as you do here https://github.com/kiwitcms/Kiwi/blob/master/tcms/testplans/models.py#L49). I'll get a fix together for this, but if you change 'self' to 'TestPlan' in line 49, that will also fix the issue on your end. Let me know if that helps!

rossmechanic added a commit that referenced this issue Jan 14, 2019

atodorov added a commit to kiwitcms/Kiwi that referenced this issue Jan 14, 2019

@atodorov

This comment has been minimized.

Copy link
Author

atodorov commented Jan 14, 2019

Found the issue @atodorov !

@rossmechanic thank you for the lightning fast reaction!

I'll get a fix together for this, but if you change 'self' to 'TestPlan' in line 49, that will also fix the issue on your end. Let me know if that helps!

This WA works for me and doesn't produce any more migrations.

Unfortunately we do have a live version of Kiwi TCMS out there which customers have deployed and are hitting this issue. As we are shipping with pinned dependencies in our Docker image even if you do release a fix there's not much to be done on our end except publishing a new docker image. Sigh.

atodorov added a commit to kiwitcms/Kiwi that referenced this issue Jan 14, 2019

@rossmechanic

This comment has been minimized.

Copy link
Collaborator

rossmechanic commented Jan 14, 2019

@atodorov sorry about that! That should have been caught in automated tests, but evidently whoever wrote that feature originally didn't test it... I do have a PR up that should be merged by tomorrow, and I've been planning to have a new release by end of week. Sorry for the additional work 😞

rossmechanic added a commit that referenced this issue Jan 14, 2019

GH-512: allow foreign key to reference self using self str (#513)
* GH-512: Fix bug that disallowed using 'self' str when creating ForeignKey to self

* Formatted

* Updated CHANGES.rst

* last change

* Fixed missing on_delete

atodorov added a commit to kiwitcms/Kiwi that referenced this issue Jan 14, 2019

atodorov added a commit to kiwitcms/Kiwi that referenced this issue Jan 15, 2019

atodorov added a commit to kiwitcms/Kiwi that referenced this issue Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment