diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index c94038a508b5..3350a5dda99a 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -2132,6 +2132,57 @@ def test_move_parented_child(self): [self.problem_usage_key], # problem child created in setUp ) + def test_move_parented_child_with_duplicate_children(self): + """ + Test that moving a child from one parent to another removes ALL + references from the old parent, even when duplicates exist (data corruption). + + Regression test for a bug where list.remove() only removed the first + occurrence, leaving dangling references in the old parent. + """ + unit_1_key = self.response_usage_key( + self.create_xblock( + parent_usage_key=self.seq_usage_key, + category="vertical", + display_name="unit 1", + ) + ) + unit_2_key = self.response_usage_key( + self.create_xblock( + parent_usage_key=self.seq2_usage_key, + category="vertical", + display_name="unit 2", + ) + ) + + # Corrupt: add unit_1 to seq1's children 3 more times (4 total) + seq1 = self.get_item_from_modulestore(self.seq_usage_key) + for _ in range(3): + seq1.children.append(unit_1_key) + self.store.update_item(seq1, self.user.id) + + # Verify corruption + seq1 = self.get_item_from_modulestore(self.seq_usage_key) + self.assertEqual(seq1.children.count(unit_1_key), 4) + + # Move unit_1 from seq1 to seq2 by setting seq2's children + resp = self.client.ajax_post( + self.seq2_update_url, data={"children": [str(unit_1_key), str(unit_2_key)]} + ) + self.assertEqual(resp.status_code, 200) + + # Verify: unit_1 completely removed from seq1, present in seq2 + seq1_after = self.get_item_from_modulestore(self.seq_usage_key) + self.assertNotIn( + unit_1_key, + seq1_after.children, + "Dangling references to moved block remain in old parent's children list", + ) + self.assertListEqual( + self.get_item_from_modulestore(self.seq2_usage_key).children, + [unit_1_key, unit_2_key], + ) + def test_move_orphaned_child_error(self): """ Test moving an orphan returns an error diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index ae7492ddbc91..2417078f3f32 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -373,7 +373,7 @@ def _save_xblock( old_parent_location = store.get_parent_location(new_child) if old_parent_location: old_parent = store.get_item(old_parent_location) - old_parent.children.remove(new_child) + old_parent.children = [c for c in old_parent.children if c != new_child] old_parent = save_xblock_with_callback(old_parent, user) else: # the Studio UI currently doesn't present orphaned children, so assume this is an error diff --git a/xmodule/modulestore/draft_and_published.py b/xmodule/modulestore/draft_and_published.py index d69e06763cdc..7f9b4d6b792a 100644 --- a/xmodule/modulestore/draft_and_published.py +++ b/xmodule/modulestore/draft_and_published.py @@ -163,7 +163,7 @@ def update_item_parent(self, item_location, new_parent_location, old_parent_loca # Remove item from the list of children of old parent. if source_item.location in old_parent_item.children: - old_parent_item.children.remove(source_item.location) + old_parent_item.children = [c for c in old_parent_item.children if c != source_item.location] self.update_item(old_parent_item, user_id) # pylint: disable=no-member log.info( '%s removed from %s children', diff --git a/xmodule/modulestore/split_mongo/split.py b/xmodule/modulestore/split_mongo/split.py index b2124c4025ec..9802b6f9bdeb 100644 --- a/xmodule/modulestore/split_mongo/split.py +++ b/xmodule/modulestore/split_mongo/split.py @@ -2540,7 +2540,7 @@ def delete_item(self, usage_locator, user_id, force=False): # lint-amnesty, pyl parent_block_keys = self._get_parents_from_structure(block_key, original_structure) for parent_block_key in parent_block_keys: parent_block = new_blocks[parent_block_key] - parent_block.fields['children'].remove(block_key) + parent_block.fields['children'] = [c for c in parent_block.fields['children'] if c != block_key] parent_block.edit_info.edited_on = datetime.datetime.now(UTC) parent_block.edit_info.edited_by = user_id parent_block.edit_info.previous_version = parent_block.edit_info.update_version diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 1f4bcfac7b0c..7a65d99d4b80 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1701,6 +1701,47 @@ def test_move_1_moved_1_deleted(self, store_type): assert problem_item2.parent == self.vertical_x1a # lint-amnesty, pylint: disable=no-member assert problem_item2.location in problem_item2.get_parent().children + @ddt.data(ModuleStoreEnum.Type.split) + def test_update_item_parent_with_duplicate_children(self, store_type): + """ + Test that update_item_parent removes ALL references to a block from the + old parent's children list when duplicates exist (data corruption). + + Regression test for a bug where list.remove() only removed the first + occurrence, leaving dangling references in the old parent. + """ + self.initdb(store_type) + self._create_block_hierarchy() + self.course = self.store.publish(self.course.location, self.user_id) # lint-amnesty, pylint: disable=attribute-defined-outside-init + + item_location = self.problem_x1a_1 # lint-amnesty, pylint: disable=no-member + new_parent_location = self.vertical_y1a # lint-amnesty, pylint: disable=no-member + old_parent_location = self.vertical_x1a # lint-amnesty, pylint: disable=no-member + + # Corrupt: add the item to old parent's children 3 more times + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + old_parent = self.store.get_item(old_parent_location) + for _ in range(3): + old_parent.children.append(item_location) + self.store.update_item(old_parent, self.user_id) + + # Verify corruption: item appears 4 times + old_parent = self.store.get_item(old_parent_location) + assert old_parent.children.count(item_location) == 4 + + # Move item - should remove ALL 4 references from old parent + self.store.update_item_parent( + item_location, new_parent_location, old_parent_location, self.user_id + ) + + # Verify: item is in new parent and completely gone from old parent + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + old_parent = self.store.get_item(old_parent_location) + new_parent = self.store.get_item(new_parent_location) + assert item_location not in old_parent.children, \ + "Dangling references to moved block remain in old parent's children list" + assert item_location in new_parent.children + @ddt.data(ModuleStoreEnum.Type.split) def test_get_parent_locations_moved_child(self, default_ms): self.initdb(default_ms) diff --git a/xmodule/modulestore/tests/test_split_modulestore.py b/xmodule/modulestore/tests/test_split_modulestore.py index c574d345d952..ca5b7944ad3d 100644 --- a/xmodule/modulestore/tests/test_split_modulestore.py +++ b/xmodule/modulestore/tests/test_split_modulestore.py @@ -1552,6 +1552,66 @@ def check_subtree(node): check_subtree(sub) check_subtree(nodes[0]) + def test_delete_item_with_duplicate_children(self): + """ + Test that delete_item removes ALL references to a block from its parent's + children list, even when the block appears multiple times (data corruption). + + Regression test for a bug where list.remove() only removed the first + occurrence, leaving dangling references that crashed Studio. + """ + store = modulestore() + user = self.user_id + + # Create a course with a chapter containing a vertical + course = store.create_course('testx', 'dup_children', 'run', user, BRANCH_NAME_DRAFT) + course_loc = course.location.version_agnostic() + chapter = store.create_child(user, course_loc, 'chapter') + chapter_loc = chapter.location.map_into_course(course_loc.course_key) + vertical = store.create_child(user, chapter_loc, 'vertical') + vertical_loc = vertical.location.map_into_course(course_loc.course_key) + + # Manually corrupt: add the vertical to the chapter's children 3 more times + # (simulating the production data corruption) + # pylint: disable=protected-access + course_key = course_loc.course_key.for_branch(BRANCH_NAME_DRAFT) + course_structure = store._lookup_course(course_key) + structure = course_structure.structure + new_structure = store.version_structure(course_key, structure, user) + new_blocks = new_structure['blocks'] + new_id = new_structure['_id'] + + chapter_block_key = BlockKey.from_usage_key(chapter_loc) + vertical_block_key = BlockKey.from_usage_key(vertical_loc) + chapter_block = new_blocks[chapter_block_key] + + # Verify it starts with 1 child + assert chapter_block.fields['children'].count(vertical_block_key) == 1 + + # Add 3 more duplicates (total 4, matching the production corruption) + for _ in range(3): + chapter_block.fields['children'].append(vertical_block_key) + assert chapter_block.fields['children'].count(vertical_block_key) == 4 + + chapter_block.edit_info.previous_version = chapter_block.edit_info.update_version + chapter_block.edit_info.update_version = new_id + store.update_structure(course_key, new_structure) + index_entry = store._get_index_if_valid(course_key) + if index_entry: + store._update_head(course_key, index_entry, course_key.branch, new_id) + + # Now delete the vertical - this should remove ALL 4 references + store.delete_item(vertical_loc, user) + + # Verify: vertical is gone and chapter has no dangling references + assert not store.has_item(vertical_loc.version_agnostic()) + + # Re-fetch the chapter and check its children list + updated_chapter = store.get_item(chapter_loc.version_agnostic()) + assert vertical_block_key not in [ + BlockKey.from_usage_key(c) for c in updated_chapter.children + ], "Dangling references to deleted block remain in parent's children list" + def create_course_for_deletion(self): """ Create a course we can delete