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

Fixed #24225, #24264, #24282 -- Recursively rebuilt all related models as soon as one changed #4097

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@MarkusH
Member

MarkusH commented Feb 9, 2015

The current (Django 1.8+) ProjectState implementation suffers from incomplete updates of relations between models.

Given the models A, B, C are related by foreign keys like A.bs --> B and B.cs --> C.

Right now: If a field is added to A, the model is removed from a state and rerendered as A*. In order to fix the reverse relation from B to A*, B is rerendered as B*. So far so good. However, Django doesn't rerender C which still points to B and not B*. This eventually leads to problems, not just the one shown in #24225 but also #24264 and #24282.

The pull request introduces recursively removing and reloading of all referenced models. This should fix the errors mentioned above.

However, I'm currently struggling with fixing 4 test cases:

======================================================================
ERROR: test_alter_order_with_respect_to (migrations.test_operations.OperationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/markus/Coding/django/tests/migrations/test_operations.py", line 1261, in test_alter_order_with_respect_to
    rendered_state.get_model("test_alorwrtto", "Rider").objects.create(pony=pony, friend_id=1)
  File "/home/markus/Coding/django/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/markus/Coding/django/django/db/models/query.py", line 399, in create
    obj = self.model(**kwargs)
  File "/home/markus/Coding/django/django/db/models/base.py", line 460, in __init__
    setattr(self, field.name, rel_obj)
  File "/home/markus/Coding/django/django/db/models/fields/related.py", line 627, in __set__
    self.field.rel.to._meta.object_name,
ValueError: Cannot assign "<Pony: Pony object>": "Rider.pony" must be a "Pony" instance.

======================================================================
ERROR: test_create_model_m2m (migrations.test_operations.OperationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/markus/Coding/django/tests/migrations/test_operations.py", line 274, in test_create_model_m2m
    stable.ponies.add(p1, p2)
  File "/home/markus/Coding/django/django/db/models/fields/related.py", line 991, in add
    self._add_items(self.source_field_name, self.target_field_name, *objs)
  File "/home/markus/Coding/django/django/db/models/fields/related.py", line 1124, in _add_items
    (self.model._meta.object_name, obj)
TypeError: 'Pony' instance expected, got <Pony: Pony object>

======================================================================
ERROR: test_rename_model_with_m2m (migrations.test_operations.OperationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/markus/Coding/django/tests/migrations/test_operations.py", line 568, in test_rename_model_with_m2m
    pony.riders.add(rider)
  File "/home/markus/Coding/django/django/db/models/fields/related.py", line 991, in add
    self._add_items(self.source_field_name, self.target_field_name, *objs)
  File "/home/markus/Coding/django/django/db/models/fields/related.py", line 1124, in _add_items
    (self.model._meta.object_name, obj)
TypeError: 'Rider' instance expected, got <Rider: Rider object>

======================================================================
ERROR: test_run_python (migrations.test_operations.OperationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/markus/Coding/django/tests/migrations/test_operations.py", line 1567, in test_run_python
    operation.database_backwards("test_runpython", editor, project_state, new_state)
  File "/home/markus/Coding/django/django/db/migrations/operations/special.py", line 185, in database_backwards
    self.reverse_code(from_state.apps, schema_editor)
  File "/home/markus/Coding/django/tests/migrations/test_operations.py", line 1551, in inner_method_reverse
    Pony.objects.filter(pink=1, weight=3.55).delete()
  File "/home/markus/Coding/django/django/db/models/query.py", line 592, in delete
    collector.collect(del_query)
  File "/home/markus/Coding/django/django/db/models/deletion.py", line 225, in collect
    sub_objs = self.related_objects(related, batch)
  File "/home/markus/Coding/django/django/db/models/deletion.py", line 245, in related_objects
    **{"%s__in" % related.field.name: objs}
  File "/home/markus/Coding/django/django/db/models/query.py", line 781, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "/home/markus/Coding/django/django/db/models/query.py", line 799, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "/home/markus/Coding/django/django/db/models/sql/query.py", line 1199, in add_q
    clause, _ = self._add_q(q_object, self.used_aliases)
  File "/home/markus/Coding/django/django/db/models/sql/query.py", line 1224, in _add_q
    current_negated=current_negated, connector=connector, allow_joins=allow_joins)
  File "/home/markus/Coding/django/django/db/models/sql/query.py", line 1129, in build_filter
    self.check_related_objects(field, value, opts)
  File "/home/markus/Coding/django/django/db/models/sql/query.py", line 1034, in check_related_objects
    self.check_query_object_type(v, opts)
  File "/home/markus/Coding/django/django/db/models/sql/query.py", line 1015, in check_query_object_type
    (value, opts.object_name))
ValueError: Cannot query "Pony object": Must be "Pony" instance.

----------------------------------------------------------------------

While debugging and trying to fix the first test (test_alter_order_with_respect_to) I added the following lines right before line 1260:

print("Pony(old):", id(project_state.apps.get_model("test_alorwrtto", "Pony")), "Pony(new):", id(new_state.apps.get_model("test_alorwrtto", "Pony")))
print("Pony(old).apps:", project_state.apps.get_model("test_alorwrtto", "Pony")._meta.apps, "Pony(new).apps:", new_state.apps.get_model("test_alorwrtto", "Pony")._meta.apps)
print("Rider(old).apps:", project_state.apps.get_model("test_alorwrtto", "Rider")._meta.apps, "Rider(new).apps:", new_state.apps.get_model("test_alorwrtto", "Rider")._meta.apps)
print("Pony via Rider.pony(old):", id(project_state.apps.get_model("test_alorwrtto", "Rider").pony.field.rel.to), "Pony via Rider.pony(new):", id(new_state.apps.get_model("test_alorwrtto", "Rider").pony.field.rel.to))
print("Pony via Rider.pony(old).apps:", project_state.apps.get_model("test_alorwrtto", "Rider").pony.field.rel.to._meta.apps, "Pony via Rider.pony(new).apps:", new_state.apps.get_model("test_alorwrtto", "Rider").pony.field.rel.to._meta.apps)

and got the following output

Pony(old): 42174888 Pony(new): 42185624
Pony(old).apps: <django.db.migrations.state.StateApps object at 0x7fce09ae87f0> Pony(new).apps: <django.db.migrations.state.StateApps object at 0x7fce09ae8a90>
Rider(old).apps: <django.db.migrations.state.StateApps object at 0x7fce09ae87f0> Rider(new).apps: <django.db.migrations.state.StateApps object at 0x7fce09ae8a90>
Pony via Rider.pony(old): 41647288 Pony via Rider.pony(new): 42185624
Pony via Rider.pony(old).apps: <django.db.migrations.state.StateApps object at 0x7fce09ae87f0> Pony via Rider.pony(new).apps: <django.db.migrations.state.StateApps object at 0x7fce09ae8a90>

What one can see: the "Pony" model in new_state is correctly refreshed and linked in the foreign key from "Rider". However, the "Pony" model in project_state (old state) differs from the one referenced by the foreign key from the old "Rider" model.

If anybody has an idea what I'm missing since a couple of hours, I'm grateful for any suggestions!

@claudep

This comment has been minimized.

Show comment
Hide comment
@claudep

claudep Feb 9, 2015

Member

Models are shared between states as long as they are not affected by migrations. But when you reload a model for any migration change, it's very hard to know exactly the extent of inter-model relations. And when a model is changed before being first reloaded, it unfortunately affects older states. Very, very tricky :-( But you know all that probably.

Member

claudep commented Feb 9, 2015

Models are shared between states as long as they are not affected by migrations. But when you reload a model for any migration change, it's very hard to know exactly the extent of inter-model relations. And when a model is changed before being first reloaded, it unfortunately affects older states. Very, very tricky :-( But you know all that probably.

@MarkusH

This comment has been minimized.

Show comment
Hide comment
@MarkusH

MarkusH Feb 9, 2015

Member

Thanks, @claudep , I know that. The thing I'm struggling with is where these inconsistencies are introduced. In other words, where do I forget to clone or rerender (a subset of) models 😒

Member

MarkusH commented Feb 9, 2015

Thanks, @claudep , I know that. The thing I'm struggling with is where these inconsistencies are introduced. In other words, where do I forget to clone or rerender (a subset of) models 😒

@timgraham

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated
@timgraham

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated
@timgraham

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated
@timgraham

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated
@timgraham

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated
@timgraham

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated

@MarkusH MarkusH changed the title from Fixed #24225, #24264 -- Recursively rebuilt all related models as soon as one changed to [WIP] Fixed #24225, #24264 -- Recursively rebuilt all related models as soon as one changed Feb 10, 2015

@timgraham

View changes

Show outdated Hide outdated django/db/models/options.py Outdated
@timgraham

View changes

Show outdated Hide outdated django/db/models/options.py Outdated

@MarkusH MarkusH changed the title from [WIP] Fixed #24225, #24264 -- Recursively rebuilt all related models as soon as one changed to Fixed #24225, #24264 -- Recursively rebuilt all related models as soon as one changed Feb 12, 2015

@claudep

This comment has been minimized.

Show comment
Hide comment
@claudep

claudep Feb 12, 2015

Member

Wow, great to see progress here, and what looks like solid tests. The real judge will now be real-world complex apps...

Member

claudep commented Feb 12, 2015

Wow, great to see progress here, and what looks like solid tests. The real judge will now be real-world complex apps...

@claudep

View changes

Show outdated Hide outdated django/db/migrations/state.py Outdated
@timgraham

View changes

Show outdated Hide outdated tests/migrations/test_executor.py Outdated
@timgraham

View changes

Show outdated Hide outdated tests/migrations/test_state.py Outdated
@timgraham

View changes

Show outdated Hide outdated tests/migrations/test_state.py Outdated
@MarkusH

This comment has been minimized.

Show comment
Hide comment
@MarkusH

MarkusH Feb 13, 2015

Member

A few more test cases I want to add:

  • Inheriting from abstract models
  • Inheriting from proxy models
Member

MarkusH commented Feb 13, 2015

A few more test cases I want to add:

  • Inheriting from abstract models
  • Inheriting from proxy models

@MarkusH MarkusH changed the title from Fixed #24225, #24264 -- Recursively rebuilt all related models as soon as one changed to Fixed #24225, #24264, #24282 -- Recursively rebuilt all related models as soon as one changed Feb 13, 2015

@timgraham

View changes

Show outdated Hide outdated tests/migrations/test_operations.py Outdated
@timgraham

View changes

Show outdated Hide outdated tests/migrations/test_executor.py Outdated

claudep and others added some commits Jan 29, 2015

Refs #24225 -- Added failing test case for removing a previously adde…
…d field in migrations

When a related field is deleted, the related model must be updated. As
unchanged models are shared in migration states, the related model must
be re-rendered so that the change applies to a new copy of the related
model.

Thanks Henrik Heimbuerger for the report.
Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration …
…project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.
Refs #24264 -- Added failing test case for updating a FK when changin…
…g a PK

When the primary key column is altered, foreign keys of referencing
models must be aware of a possible data type change as well and thus
need to be re-rendered.

Thanks Tim Graham for the report.
Refs #24282 -- Added failing test case for assigning models of wrong …
…type to FK

Thanks Jeff Singer for the test case.
@MarkusH

This comment has been minimized.

Show comment
Hide comment
@MarkusH
Member

MarkusH commented Feb 16, 2015

@MarkusH MarkusH closed this Feb 16, 2015

@MarkusH MarkusH deleted the MarkusH:ticket24225 branch Feb 16, 2015

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