Infinite loop in migration code

[Luke Plant]

I came across a bug with an infinite loop in migration dependency
searching code. This is fixed here:

https://github.com/django/django/commit/ff3d746e8d8e8fbe6de287bd0f4c3a9fa23c18dc

(another person reviewing it would be good, though I think it is correct).

My question is, should we backport this to 1.7.x? For me, the bug
manifested itself with migrations that were automatically created by
Django itself, in a project with apps A, B, and C:

App B depends on app A
App A depends on app B (it didn't initially, but does now)
App C depends on one/both of them.

After doing makemigrations for A and B, makemigrations for C then went
into an infinite loop.

So this is not a really obscure case, and could affect a fair number of
people attempting to upgrade to Django 1.7, as I was.

Regards,

Luke


--
"We may not return the affection of those who like us, but we
always respect their good judgement." -- Libbie Fudim

Luke Plant || http://lukeplant.me.uk/

[Markus Holtermann]

Hey Luke,

It would be interesting to see why A.1 and B.1 depend on each other. If there are e.g. FK constraints pointing to models in the other app the autodetector should end up with e.g. A.1 <-- B.1 <-- B.2 <-- A.2 (or optimized A.1 <-- B.1 <-- A.2), in which case you wouldn't end up with a cycle. C.1 would then depend on B.2 (B.1 respectively in the optimized graph).

/Markus

[Andrew Godwin]

Hi Luke,

That was already a fix for infinite looping on the previous iteration that I committed at the DUTH sprints, but your fix looks more understandable and cleaner, so I say commit it for sure.

As for backporting - I think we should, as this is potentially a crash bug (though not a data loss bug) and it's a pretty small thing to backport anyway.

Andrew

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5e547550-1002-4a6b-82f0-a8dc7e3ed2b0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Luke Plant]

On 25/11/14 16:23, Markus Holtermann wrote:
> Hey Luke,
>
> It would be interesting to see why A.1 and B.1 depend on each other. If
> there are e.g. FK constraints pointing to models in the other app the
> autodetector should end up with e.g. A.1 <-- B.1 <-- B.2 <-- A.2 (or
> optimized A.1 <-- B.1 <-- A.2), in which case you wouldn't end up with a
> cycle. C.1 would then depend on B.2 (B.1 respectively in the optimized
> graph).

I didn't realise the autodetector could handle that. Looking more
closely, it looks like I have more of an edge case:

App B has a model with a FK to a model in app A

App A has a model with a ManyToMany field 'through' a model in app B.
(It's actually added that way for the sake of the admin for the models
in app A).

So it isn't the straight-forward A has FK to B. It might not be worth
fixing the autodetector for this, as fixing the migrations is relatively
easy. But I think fixing the infinite loop is another matter, and I'll
go ahead and backport that.

Thanks for the input,

[Markus Holtermann]

Can you open a ticket with your models so the issue doesn't get lost. I'm happy to work on it.

Although it's somewhat uncommon, people normally have the through model in the app that has the m2m field (why don't you define it on the other model?) this is still something that shouldn't happen.

/Markus

[Luke Plant]

Hi Markus,


It was basically this:

== app camps ==

class Camp:

invited_officers = M2M(auth.User, through='officers.Invitation')


== app officers ==

class Invitation:
timestamp = models.DateTimeField(default=datetime.now)
camp = FK(camps.Camp)
user = FK(auth.User)


=====

I ran makemigrations for 'camps', then 'officers', and the generated
migrations ended up depending on each other.

Hopefully that's enough, let me know if that doesn't reproduce it.
Sorry, don't have time to put it together more formally.

Thanks,

Luke

> --
> You received this message because you are subscribed to the Google
> Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to django-develop...@googlegroups.com

> <mailto:django-develop...@googlegroups.com>.

> To post to this group, send email to django-d...@googlegroups.com

> <mailto:django-d...@googlegroups.com>.

> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/f2924e77-7604-4632-a7c1-1920a3bfb4d0%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/f2924e77-7604-4632-a7c1-1920a3bfb4d0%40googlegroups.com?utm_medium=email&utm_source=footer>.

> For more options, visit https://groups.google.com/d/optout.

--
"In your presence there is fullness of joy; at your right hand are
pleasures forevermore" Psalm 16:11

Luke Plant || http://lukeplant.me.uk/

[Markus Holtermann]

Hey Luke,

I cannot reproduce your problem on neither master nor stable/1.7.x. When I run "python manage.py makemigrations camps" the migrations for the "officers" app are also generated (as I expect it). "testapp" is your "camps" app, "otherapp" is your "officers" app: https://gist.github.com/MarkusH/d84618db929fd6fcdb9f

The dependencies look fine to me, and migrating works like a charm. I also tried both the string representation 'auth.User' and importing d.c.a.m.User and using the class reference in the FK fields, but that doesn't make any difference.

If you can reproduce the issue, could you please push a sample project to GitHub. I'm happy to look into it then.

/Markus