From a46ba4805b733b36eb6737d14568effd89303d1e Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Thu, 15 Mar 2018 16:40:28 +0100 Subject: [PATCH] Cleanup for the `Block.__eq__` method (#4378) * Fix incorrect assertion in TestSystemCheck It only worked by accident, due to the definition of equality on block objects. * Explicitly cast child_blocks.items() to a list when deconstructing blocks The result of items() is a lazily-evaluated ItemsView object, which Django's deep_deconstruct function (used to detect changes for migrations) doesn't recognise; as a result, the blocks inside it don't get deconstructed. Luckily, this doesn't break migration change detection, because we define __eq__ on Block so that two blocks with matching definitions are considered equal. Nevertheless, it's best that we don't rely on that behaviour; it was only implemented originally as a workaround for https://code.djangoproject.com/ticket/24340 in Django <1.9 (where deep_deconstruct didn't recurse into lists either). * Bring comment for Block.__eq__ up to date --- wagtail/core/blocks/base.py | 24 ++++++++++++------------ wagtail/core/blocks/stream_block.py | 2 +- wagtail/core/blocks/struct_block.py | 2 +- wagtail/core/fields.py | 2 +- wagtail/core/tests/test_blocks.py | 2 +- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/wagtail/core/blocks/base.py b/wagtail/core/blocks/base.py index 2e872c617f..89ca09a5bc 100644 --- a/wagtail/core/blocks/base.py +++ b/wagtail/core/blocks/base.py @@ -358,19 +358,19 @@ class Block(metaclass=BaseBlock): def __eq__(self, other): """ - The deep_deconstruct method in django.db.migrations.autodetector.MigrationAutodetector does not - recurse into arbitrary lists and dicts. As a result, when it is passed a field such as: - StreamField([ - ('heading', CharBlock()), - ]) - the CharBlock object will be left in its constructed form. This causes problems when - MigrationAutodetector compares two separate instances of the StreamField from different project - states: since the CharBlocks are different objects, it will report a change where there isn't one. + Implement equality on block objects so that two blocks with matching definitions are considered + equal. (Block objects are intended to be immutable with the exception of set_name(), so here + 'matching definitions' means that both the 'name' property and the constructor args/kwargs - as + captured in _constructor_args - are equal on both blocks.) - To prevent this, we implement the equality operator on Block instances such that the two CharBlocks - are reported as equal. Since block objects are intended to be immutable with the exception of - set_name(), it is sufficient to compare the 'name' property and the constructor args/kwargs of the - two block objects. The 'deconstruct' method provides a convenient way to access the latter. + This was originally necessary as a workaround for https://code.djangoproject.com/ticket/24340 + in Django <1.9; the deep_deconstruct function used to detect changes for migrations did not + recurse into the block lists, and left them as Block instances. This __eq__ method therefore + came into play when identifying changes within migrations. + + As of Django >=1.9, this *probably* isn't required any more. However, it may be useful in + future as a way of identifying blocks that can be re-used within StreamField definitions + (https://github.com/wagtail/wagtail/issues/4298#issuecomment-367656028). """ if not isinstance(other, Block): diff --git a/wagtail/core/blocks/stream_block.py b/wagtail/core/blocks/stream_block.py index 12838f9bb9..2413681735 100644 --- a/wagtail/core/blocks/stream_block.py +++ b/wagtail/core/blocks/stream_block.py @@ -298,7 +298,7 @@ class BaseStreamBlock(Block): to a custom subclass in the user's models.py that may or may not stick around. """ path = 'wagtail.core.blocks.StreamBlock' - args = [self.child_blocks.items()] + args = [list(self.child_blocks.items())] kwargs = self._constructor_kwargs return (path, args, kwargs) diff --git a/wagtail/core/blocks/struct_block.py b/wagtail/core/blocks/struct_block.py index f771effb92..8ec75c05cb 100644 --- a/wagtail/core/blocks/struct_block.py +++ b/wagtail/core/blocks/struct_block.py @@ -183,7 +183,7 @@ class BaseStructBlock(Block): to a custom subclass in the user's models.py that may or may not stick around. """ path = 'wagtail.core.blocks.StructBlock' - args = [self.child_blocks.items()] + args = [list(self.child_blocks.items())] kwargs = self._constructor_kwargs return (path, args, kwargs) diff --git a/wagtail/core/fields.py b/wagtail/core/fields.py index b7a41ae12d..8bb51dab6e 100644 --- a/wagtail/core/fields.py +++ b/wagtail/core/fields.py @@ -56,7 +56,7 @@ class StreamField(models.Field): def deconstruct(self): name, path, _, kwargs = super().deconstruct() - block_types = self.stream_block.child_blocks.items() + block_types = list(self.stream_block.child_blocks.items()) args = [block_types] return name, path, args, kwargs diff --git a/wagtail/core/tests/test_blocks.py b/wagtail/core/tests/test_blocks.py index 5dbce615ff..a8ef6267f1 100644 --- a/wagtail/core/tests/test_blocks.py +++ b/wagtail/core/tests/test_blocks.py @@ -3010,7 +3010,7 @@ class TestSystemCheck(TestCase): self.assertEqual(errors[0].obj, failing_block_1) self.assertEqual(errors[1].id, 'wagtailcore.E001') self.assertEqual(errors[1].hint, "Block names cannot contain spaces") - self.assertEqual(errors[0].obj, failing_block_2) + self.assertEqual(errors[1].obj, failing_block_2) class TestTemplateRendering(TestCase):