From 20dcfaaee341b5dc050bea6be95c8f8a15df0330 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 3 Sep 2026 18:02:16 +0400 Subject: [PATCH 1/9] feat: support editing an existing tag's external_id on taxonomy re-import --- src/openedx_tagging/import_export/actions.py | 134 ++++++++++- .../import_export/import_plan.py | 3 + src/openedx_tagging/import_export/parsers.py | 18 +- .../import_export/test_actions.py | 227 +++++++++++++++++- .../openedx_tagging/import_export/test_api.py | 80 ++++++ .../import_export/test_import_plan.py | 42 ++++ .../import_export/test_parsers.py | 65 +++++ 7 files changed, 564 insertions(+), 5 deletions(-) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index a501c9e30..ce32d7c3c 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -103,10 +103,13 @@ def _validate_parent(self, indexed_actions) -> ImportActionError | None: # Validates that the parent exists on the taxonomy self.taxonomy.tag_set.get(external_id=self.tag.parent_id) except Tag.DoesNotExist: - # Or if the parent is created on previous actions - if not self._search_action( + # Or if the parent is created or renamed-in on previous actions + found = self._search_action( indexed_actions, CreateTag.name, "id", self.tag.parent_id - ): + ) or self._search_action( + indexed_actions, RenameTagExternalId.name, "id", self.tag.parent_id + ) + if not found: return ImportActionError( action=self, message=_( @@ -157,6 +160,15 @@ def _validate_value(self, indexed_actions) -> ImportActionError | None: self.tag.value, ) + if not action: + # Validates value duplication on rename_external_id actions + action = self._search_action( + indexed_actions, + RenameTagExternalId.name, + "value", + self.tag.value, + ) + if action: return ImportActionConflict( action=self, @@ -197,6 +209,8 @@ def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ This action applies whenever the tag does not exist """ + if tag.previous_id and tag.id != tag.previous_id: + return False try: taxonomy.tag_set.get(external_id=tag.id) return False @@ -371,6 +385,119 @@ def execute(self) -> None: taxonomy_tag.save() +class RenameTagExternalId(ImportAction): + """ + Action to rename an existing tag's external_id in place. + + Action created when a row's `previous_id` matches an existing tag's + external_id in the taxonomy, and the row's `id` differs from it. + Preserves the tag's primary key and associations across the + rename, instead of deleting the old tag and creating a new one. + + Validations: + - previous_id must match an existing tag's external_id. + - The new id must not collide with a different existing tag, or with a + prior create/rename action in the same import. + - Value duplicates with tags on the database, if the value is changing. + - Parent validation, if parent_id is set. + """ + + name = "rename_external_id" + + def __str__(self) -> str: + return str( + _( + "Rename external_id of tag with previous_id={previous_id} to " + "'{id}' (value={value}, parent_id={parent_id})." + ).format( + previous_id=self.tag.previous_id, + id=self.tag.id, + value=self.tag.value, + parent_id=self.tag.parent_id, + ) + ) + + @classmethod + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + This action applies whenever previous_id is set and differs from id + """ + return bool(tag.previous_id) and tag.id != tag.previous_id + + def _validate_new_id(self, indexed_actions) -> ImportActionError | None: + """ + Check that the new id doesn't collide with a different existing tag, + or with a prior create/rename action in the same import. + """ + if self.taxonomy.tag_set.filter(external_id=self.tag.id).exists(): + return ImportActionError( + action=self, + message=_("A tag with external_id ({id}) already exists.").format(id=self.tag.id), + ) + + action = self._search_action(indexed_actions, CreateTag.name, "id", self.tag.id) + if not action: + action = self._search_action(indexed_actions, self.name, "id", self.tag.id) + + if action: + return ImportActionConflict( + action=self, + conflict_action_index=action.index, + message=_("Duplicated external_id tag."), + ) + + return None + + def validate(self, indexed_actions) -> list[ImportActionError]: + """ + Validates the rename_external_id action + """ + errors = [] + + try: + matched_tag = self.taxonomy.tag_set.get(external_id=self.tag.previous_id) + except Tag.DoesNotExist: + matched_tag = None + errors.append( + ImportActionError( + action=self, + message=_( + "Unknown previous_id ({previous_id}). No tag with that " + "external_id exists in this taxonomy." + ).format(previous_id=self.tag.previous_id), + ) + ) + + error = self._validate_new_id(indexed_actions) + if error: + errors.append(error) + + if matched_tag is not None and matched_tag.value != self.tag.value: + error = self._validate_value(indexed_actions) + if error: + errors.append(error) + + if self.tag.parent_id: + error = self._validate_parent(indexed_actions) + if error: + errors.append(error) + + return errors + + def execute(self) -> None: + """ + Renames a tag's external_id in place, and updates its value and parent + """ + target = self.taxonomy.tag_set.get(external_id=self.tag.previous_id) + target.external_id = self.tag.id + target.value = self.tag.value + target.parent = ( + self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + if self.tag.parent_id else None + ) + target.save() + + class DeleteTag(ImportAction): """ Action for delete a Tag @@ -445,6 +572,7 @@ def execute(self) -> None: available_actions = [ UpdateParentTag, RenameTag, + RenameTagExternalId, CreateTag, DeleteTag, WithoutChanges, diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index 6502c2c1b..e92e55322 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -21,6 +21,7 @@ class TagItem: value: str index: int | None = 0 parent_id: str | None = None + previous_id: str | None = None def __str__(self): """ @@ -162,6 +163,8 @@ def generate_actions( for tag in tags: if tag.id in tags_for_delete: tags_for_delete.pop(tag.id) + if tag.previous_id: + tags_for_delete.pop(tag.previous_id, None) # Delete all not readed tags self._build_delete_actions(tags_for_delete) diff --git a/src/openedx_tagging/import_export/parsers.py b/src/openedx_tagging/import_export/parsers.py index 38e8fb337..656062c90 100644 --- a/src/openedx_tagging/import_export/parsers.py +++ b/src/openedx_tagging/import_export/parsers.py @@ -43,13 +43,16 @@ class Parser: It can convert in both directions, for use during import or export. If you want to add a new field, you can add it to - `required_fields` or `optional_fields` depending on the field type + `required_fields` or `optional_fields` depending on the field type. + `import_only_fields` holds fields that are parsed but never required or + optional for header validation, and are never exported. To create a new Parser you need to implement `_load_data` and `_export_data` """ required_fields = ["id", "value"] optional_fields = ["parent_id"] + import_only_fields = ["previous_id"] # Set the format associated to the parser format: ParserFormat @@ -180,6 +183,19 @@ def _parse_tags(cls, tags_data: list[dict]) -> tuple[list[TagItem], list[TagPars errors.append(cls.invalid_field_error(tag, field=req_field, row=row)) has_error = True + # import_only_fields are parsed but never required/optional for header + # validation, and never appear in _load_tags_for_export. + for io_field in cls.import_only_fields: + value = tag.get(io_field) or None + if isinstance(value, int): + value = str(value) # Technically int is invalid but we coerce to str to be more resilient + + if isinstance(value, str) or value is None: + tag_data[io_field] = value + else: + errors.append(cls.invalid_field_error(tag, field=io_field, row=row)) + has_error = True + tags.append(TagItem(**tag_data)) return tags, errors diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index 71e76a48c..046fac961 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -12,6 +12,7 @@ DeleteTag, ImportAction, RenameTag, + RenameTagExternalId, UpdateParentTag, WithoutChanges, ) @@ -52,7 +53,8 @@ def setUp(self) -> None: # Note: we must specify '-> None' to opt in to type ch ), index=1, ) - ] + ], + 'rename_external_id': [], } @@ -133,6 +135,33 @@ def test_validate_parent(self, parent_id: str, expected: bool): ) ) + def test_validate_parent_with_rename_external_id_action(self) -> None: + """ + Regression: a parent referenced by external_id that doesn't exist in + the DB yet, but is being renamed-in via a `RenameTagExternalId` + action earlier in the same import, must validate as a known parent. + """ + indexed_actions = dict(self.indexed_actions) + indexed_actions['rename_external_id'] = [ + RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_60', value='Tag 60', previous_id='tag_3', index=1), + index=1, + ) + ] + action = ImportAction( + self.taxonomy, + TagItem( + id='tag_110', + value='_', + parent_id='tag_60', + index=100, + ), + index=100, + ) + error = action._validate_parent(indexed_actions) # pylint: disable=protected-access + self.assertIsNone(error) + @ddt.data( ( 'Tag 1', @@ -174,6 +203,35 @@ def test_validate_value(self, value: str, expected: str | None): else: self.assertEqual(str(error), expected) + def test_validate_value_with_rename_external_id_action(self) -> None: + """ + Regression: a value collision with a `RenameTagExternalId` action + already queued in the same import must be caught, not only + collisions with `create`/`rename` actions. + """ + indexed_actions = dict(self.indexed_actions) + indexed_actions['rename_external_id'] = [ + RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_60', value='Shared', previous_id='tag_3', index=1), + index=1, + ) + ] + action = ImportAction( + self.taxonomy, + TagItem( + id='tag_110', + value='Shared', + index=100, + ), + index=100, + ) + error = action._validate_value(indexed_actions) # pylint: disable=protected-access + self.assertEqual( + str(error), + "Conflict with 'import_action' (#100) and action #1: Duplicated tag value." + ) + @ddt.ddt class TestCreateTag(TestImportActionMixin, TestCase): @@ -197,6 +255,23 @@ def test_applies_for(self, tag_id: str, expected: bool): ) self.assertEqual(result, expected) + def test_applies_for_previous_id_guard(self) -> None: + """ + A row with a `previous_id` that differs from `id` is a rename + candidate, not a create: `RenameTagExternalId` should handle it + even though no tag exists yet with the new id. + """ + result = CreateTag.applies_for( + self.taxonomy, + TagItem( + id='tag_100', + value='_', + previous_id='tag_99', + index=100, + ) + ) + self.assertFalse(result) + @ddt.data( ('tag_10', False), ('tag_100', True), @@ -496,6 +571,156 @@ def test_execute(self) -> None: assert tag.value == value +@ddt.ddt +class TestRenameTagExternalId(TestImportActionMixin, TestCase): + """ + Test for 'rename_external_id' action + """ + + @ddt.data( + (None, 'tag_50', False), # No previous_id + ('tag_1', 'tag_1', False), # previous_id == id + ('tag_1', 'tag_50', True), # Valid rename + ) + @ddt.unpack + def test_applies_for(self, previous_id: str | None, tag_id: str, expected: bool): + result = RenameTagExternalId.applies_for( + taxonomy=self.taxonomy, + tag=TagItem( + id=tag_id, + value='_', + previous_id=previous_id, + index=100, + ) + ) + self.assertEqual(result, expected) + + def test_validate_unmatched_previous_id(self) -> None: + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_50', + value='Tag 50', + previous_id='tag_100', + index=100, + ), + index=100, + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), 1) + self.assertIn("Unknown previous_id (tag_100)", str(errors[0])) + + def test_validate_new_id_collides_with_db_tag(self) -> None: + # previous_id matches tag_1, but the new id (tag_2) already belongs + # to a different tag in the same taxonomy. + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_2', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), 1) + self.assertIn("already exists", str(errors[0])) + + def test_validate_new_id_collides_with_create_action(self) -> None: + # The new id (tag_10) matches a pending 'create' action from + # self.indexed_actions (see TestImportActionMixin.setUp). + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_10', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), 1) + self.assertIn("Duplicated external_id tag", str(errors[0])) + + def test_validate_new_id_collides_with_prior_rename_external_id_action(self) -> None: + indexed_actions = dict(self.indexed_actions) + indexed_actions['rename_external_id'] = [ + RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_60', value='Tag 60', previous_id='tag_3', index=1), + index=1, + ) + ] + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_60', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(indexed_actions) + self.assertEqual(len(errors), 1) + self.assertIn("Duplicated external_id tag", str(errors[0])) + + def test_validate_no_error_when_value_unchanged(self) -> None: + # The row's value matches tag_1's current value, so _validate_value's + # duplicate check is skipped, and nothing else is wrong. + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_50', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(errors, []) + + def test_validate_parent(self) -> None: + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_50', + value='Tag 1', + previous_id='tag_1', + parent_id='tag_100', + index=100, + ), + index=100, + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), 1) + self.assertIn("Unknown parent tag (tag_100)", str(errors[0])) + + def test_execute(self) -> None: + tag = self.taxonomy.tag_set.get(external_id='tag_1') + pk = tag.pk + tag_item = TagItem( + id='tag_50', + value='Tag 50', + previous_id='tag_1', + parent_id='tag_3', + ) + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=tag_item, + index=100, + ) + action.execute() + tag.refresh_from_db() + self.assertEqual(tag.pk, pk) + self.assertEqual(tag.external_id, 'tag_50') + self.assertEqual(tag.value, 'Tag 50') + self.assertEqual(tag.parent.external_id, 'tag_3') + + class TestDeleteTag(TestImportActionMixin, TestCase): """ Test for 'delete' action diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index bdb04a86a..8e57a80b2 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -324,6 +324,86 @@ def test_import_removing_with_childs_no_external_id(self) -> None: ) assert result + def test_import_rename_external_id_preserves_pk(self) -> None: + """ + Importing a row with a matching `previous_id` renames the tag's + external_id in place, preserving its primary key (see ADR 0010). + """ + old_pk = self.taxonomy.tag_set.get(external_id="tag_1").pk + + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_50", "value": "Tag 1 Renamed", "previous_id": "tag_1"}, + ]}).encode()) + result, _task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert result + + renamed_tag = Tag.objects.get(pk=old_pk) + assert renamed_tag.external_id == "tag_50" + assert renamed_tag.value == "Tag 1 Renamed" + assert not self.taxonomy.tag_set.filter(external_id="tag_1").exists() + + def test_import_rename_external_id_then_export(self) -> None: + """ + A follow-up export after a rename contains the new id, and neither + the old id nor a `previous_id` field, since `previous_id` is + import-only and never persisted (see ADR 0010). + """ + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_50", "value": "Tag 1 Renamed", "previous_id": "tag_1"}, + ]}).encode()) + result, _task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert result + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_tags = json.loads(output).get("tags") + exported_ids = [tag.get("id") for tag in exported_tags] + assert "tag_50" in exported_ids + assert "tag_1" not in exported_ids + for tag in exported_tags: + assert "previous_id" not in tag + + def test_import_rename_external_id_unmatched_previous_id_rejected(self) -> None: + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_50", "value": "Tag 50", "previous_id": "tag_999"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert not result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Unknown previous_id" in log + assert not self.taxonomy.tag_set.filter(external_id="tag_50").exists() + + def test_import_rename_external_id_colliding_new_id_rejected(self) -> None: + tag_before = self.taxonomy.tag_set.get(external_id="tag_1") + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_2", "value": "Tag 1", "previous_id": "tag_1"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert not result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "already exists" in log + + tag_after = self.taxonomy.tag_set.get(external_id="tag_1") + assert tag_after.pk == tag_before.pk + assert tag_after.value == tag_before.value + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index 88f24a8b2..cc7e79d51 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -407,6 +407,48 @@ def test_execute(self, tags, replace): external_ids = list(self.taxonomy.tag_set.values_list("external_id", flat=True)) assert tag_external_ids == external_ids + def test_generate_actions_rename_external_id(self) -> None: + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 0) + self.assertEqual(len(self.import_plan.actions), 1) + self.assertEqual(self.import_plan.actions[0].name, 'rename_external_id') + self.assertEqual(self.import_plan.actions[0].tag.id, 'tag_50') + + def test_generate_actions_rename_external_id_replace_skips_delete(self) -> None: + # tag_1 is renamed to tag_50 (previous_id='tag_1'); under replace=True + # its old id must not be swept up in the delete pass, since it is the + # same underlying tag, not a removed one. + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2'), + TagItem(id='tag_3', value='Tag 3'), + TagItem(id='tag_4', value='Tag 4', parent_id='tag_3'), + ] + self.import_plan.generate_actions(tags=tags, replace=True) + self.assertEqual(len(self.import_plan.errors), 0) + delete_targets = [ + action.tag.id for action in self.import_plan.actions if action.name == 'delete' + ] + self.assertNotIn('tag_1', delete_targets) + + def test_generate_actions_rename_external_id_value_collision_with_create(self) -> None: + """ + Regression: a value collision between a `RenameTagExternalId` action + and a later `CreateTag` action in the same import must be caught at + validate time, not silently pass through to `execute()` and hit the + database's `unique_together(taxonomy, value)` constraint. + """ + tags = [ + TagItem(id='tag_50', value='Shared', previous_id='tag_1'), + TagItem(id='tag_60', value='Shared'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("Duplicated tag value", str(self.import_plan.errors[0])) + def test_error_in_execute(self): created_tag = 'tag_31' tags = [ diff --git a/tests/openedx_tagging/import_export/test_parsers.py b/tests/openedx_tagging/import_export/test_parsers.py index 5cfda2137..a7c55f106 100644 --- a/tests/openedx_tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/import_export/test_parsers.py @@ -238,6 +238,53 @@ def test_import_with_export_output(self) -> None: if output_tag.get("parent_id"): assert output_tag.get("parent_id") == tag.parent_id + @ddt.data( + ( + {"tags": [ + {"id": "tag_2", "value": "Tag 2", "previous_id": "tag_1"}, + ]}, + "tag_1", + ), + ( + {"tags": [ + {"id": "tag_2", "value": "Tag 2"}, + ]}, + None, + ), + ( + {"tags": [ + {"id": "tag_2", "value": "Tag 2", "previous_id": ""}, + ]}, + None, + ), + ( + {"tags": [ + {"id": "tag_2", "value": "Tag 2", "previous_id": None}, + ]}, + None, + ), + ( + {"tags": [ + {"id": "tag_2", "value": "Tag 2", "previous_id": 123}, + ]}, + "123", + ), + ) + @ddt.unpack + def test_parse_previous_id(self, json_data: dict, expected_previous_id: str | None) -> None: + json_file = BytesIO(json.dumps(json_data).encode()) + tags, errors = JSONParser.parse_import(json_file) + self.assertEqual(len(errors), 0) + self.assertEqual(len(tags), 1) + self.assertEqual(tags[0].previous_id, expected_previous_id) + + def test_export_does_not_include_previous_id(self) -> None: + result = JSONParser.export(self.taxonomy) + tags = json.loads(result).get("tags") + assert len(tags) > 0 + for tag in tags: + assert "previous_id" not in tag + @ddt.ddt class TestCSVParser(TestImportExportMixin, TestCase): @@ -363,3 +410,21 @@ def test_import_with_export_output(self) -> None: assert tag.value == taxonomy_tag.value if tag.parent_id: assert tag.parent_id == taxonomy_tag.parent.external_id + + @ddt.data( + ("id,value,previous_id\ntag_2,Tag 2,tag_1\n", "tag_1"), + ("id,value,previous_id\ntag_2,Tag 2,\n", None), + ("id,value\ntag_2,Tag 2\n", None), + ) + @ddt.unpack + def test_parse_previous_id(self, csv_data: str, expected_previous_id: str | None) -> None: + csv_file = BytesIO(csv_data.encode()) + tags, errors = CSVParser.parse_import(csv_file) + self.assertEqual(len(errors), 0) + self.assertEqual(len(tags), 1) + self.assertEqual(tags[0].previous_id, expected_previous_id) + + def test_export_does_not_include_previous_id(self) -> None: + output = CSVParser.export(self.taxonomy) + header = output.splitlines()[0] + assert "previous_id" not in header.split(",") From cf9ff0680488e76769c3f37e3ea058512ce4118b Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Thu, 3 Sep 2026 18:41:01 +0400 Subject: [PATCH 2/9] fix: removed whitespace --- src/openedx_tagging/import_export/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index ce32d7c3c..28837f515 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -390,7 +390,7 @@ class RenameTagExternalId(ImportAction): Action to rename an existing tag's external_id in place. Action created when a row's `previous_id` matches an existing tag's - external_id in the taxonomy, and the row's `id` differs from it. + external_id in the taxonomy, and the row's `id` differs from it. Preserves the tag's primary key and associations across the rename, instead of deleting the old tag and creating a new one. From 9b26760c99b72e7f3feed05cbc6fdd6790f4d3d8 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 7 Sep 2026 10:59:21 +0400 Subject: [PATCH 3/9] fix: reject duplicate previous_id values within the same import Two rows sharing the same previous_id both resolve to the same tag at validate time, since nothing has executed yet. The second row's execute() then looks up that previous_id again after the first rename has already moved it away, raising an uncaught Tag.DoesNotExist instead of a clean rejection. --- src/openedx_tagging/import_export/actions.py | 8 ++++++ .../import_export/test_actions.py | 26 +++++++++++++++++++ .../openedx_tagging/import_export/test_api.py | 26 +++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index 28837f515..dcc80bd8c 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -446,6 +446,14 @@ def _validate_new_id(self, indexed_actions) -> ImportActionError | None: message=_("Duplicated external_id tag."), ) + action = self._search_action(indexed_actions, self.name, "previous_id", self.tag.previous_id) + if action: + return ImportActionConflict( + action=self, + conflict_action_index=action.index, + message=_("Duplicated previous_id tag."), + ) + return None def validate(self, indexed_actions) -> list[ImportActionError]: diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index 046fac961..356f89741 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -667,6 +667,32 @@ def test_validate_new_id_collides_with_prior_rename_external_id_action(self) -> self.assertEqual(len(errors), 1) self.assertIn("Duplicated external_id tag", str(errors[0])) + def test_validate_new_id_collides_with_prior_previous_id_action(self) -> None: + # Two rows sharing the same previous_id both target the same old + # tag; the second must be rejected at validate time instead of + # crashing at execute time once the first rename has already run. + indexed_actions = dict(self.indexed_actions) + indexed_actions['rename_external_id'] = [ + RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_60', value='Tag 1', previous_id='tag_1', index=1), + index=1, + ) + ] + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_70', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(indexed_actions) + self.assertEqual(len(errors), 1) + self.assertIn("Duplicated previous_id tag", str(errors[0])) + def test_validate_no_error_when_value_unchanged(self) -> None: # The row's value matches tag_1's current value, so _validate_value's # duplicate check is skipped, and nothing else is wrong. diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 8e57a80b2..0f84130e3 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -404,6 +404,32 @@ def test_import_rename_external_id_colliding_new_id_rejected(self) -> None: assert tag_after.pk == tag_before.pk assert tag_after.value == tag_before.value + def test_import_rename_external_id_duplicate_previous_id_rejected(self) -> None: + """ + Two rows sharing the same previous_id both target the same old tag. + This must be rejected cleanly at the plan step, not crash at execute + time once the first rename has already renamed the old tag away. + """ + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_50", "value": "Tag 50", "previous_id": "tag_1"}, + {"id": "tag_60", "value": "Tag 60", "previous_id": "tag_1"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert not result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Duplicated previous_id" in log + assert "Traceback" not in log + + tag_after = self.taxonomy.tag_set.get(external_id="tag_1") + assert tag_after.external_id == "tag_1" + assert not self.taxonomy.tag_set.filter(external_id="tag_50").exists() + assert not self.taxonomy.tag_set.filter(external_id="tag_60").exists() + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() From 966961ab2e4767ae4be5d90261c144a3077125f0 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 7 Sep 2026 11:02:10 +0400 Subject: [PATCH 4/9] fix: allow reusing an external_id freed by a replace-mode delete RenameTagExternalId's collision check ran against the database as it is right now, not as it will be once other actions in the same import have executed. A row renaming a tag onto an external_id that a replace-mode delete sweep is simultaneously freeing up (because that id's row is omitted from the file) was rejected as already existing, even though the delete runs before the rename at execute time. Also fixes the delete-sweep itself: a rename row's id is the new target, not confirmation that the tag currently holding it should survive, so it must not protect that tag from deletion the way a normal row's id does. --- src/openedx_tagging/import_export/actions.py | 12 +++++-- .../import_export/import_plan.py | 7 +++- .../import_export/test_actions.py | 26 +++++++++++++++ .../openedx_tagging/import_export/test_api.py | 33 +++++++++++++++++++ 4 files changed, 75 insertions(+), 3 deletions(-) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index dcc80bd8c..322bc6b2a 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -427,9 +427,17 @@ def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: def _validate_new_id(self, indexed_actions) -> ImportActionError | None: """ Check that the new id doesn't collide with a different existing tag, - or with a prior create/rename action in the same import. + or with a prior create/rename action in the same import. A tag that + a replace-mode delete sweep is removing in this same import doesn't + count as a collision, since the delete executes before this action + (see TagImportPlan._build_delete_actions). """ - if self.taxonomy.tag_set.filter(external_id=self.tag.id).exists(): + is_freed_by_delete = any( + self.tag.id == action.tag.id + for action in indexed_actions["delete"] + ) if "delete" in indexed_actions else False + + if not is_freed_by_delete and self.taxonomy.tag_set.filter(external_id=self.tag.id).exists(): return ImportActionError( action=self, message=_("A tag with external_id ({id}) already exists.").format(id=self.tag.id), diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index e92e55322..0efa1d02a 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -161,7 +161,12 @@ def generate_actions( } for tag in tags: - if tag.id in tags_for_delete: + # A rename row's `id` is the new target, not confirmation + # that the tag currently holding that external_id should be + # kept: only `previous_id` protects an existing tag from + # this delete sweep in that case. + is_rename = bool(tag.previous_id) and tag.id != tag.previous_id + if not is_rename and tag.id in tags_for_delete: tags_for_delete.pop(tag.id) if tag.previous_id: tags_for_delete.pop(tag.previous_id, None) diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index 356f89741..e36e97084 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -627,6 +627,32 @@ def test_validate_new_id_collides_with_db_tag(self) -> None: self.assertEqual(len(errors), 1) self.assertIn("already exists", str(errors[0])) + def test_validate_new_id_freed_by_queued_delete_action(self) -> None: + # Same setup as test_validate_new_id_collides_with_db_tag (new id + # tag_2 still exists in the DB), but this time a replace-mode delete + # sweep has already queued tag_2 for deletion in this same import, + # so reusing its external_id is not a real collision. + indexed_actions = dict(self.indexed_actions) + indexed_actions['delete'] = [ + DeleteTag( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_2', value='Tag 2', index=1), + index=1, + ) + ] + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_2', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(indexed_actions) + self.assertEqual(errors, []) + def test_validate_new_id_collides_with_create_action(self) -> None: # The new id (tag_10) matches a pending 'create' action from # self.indexed_actions (see TestImportActionMixin.setUp). diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 0f84130e3..1c793f93a 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -430,6 +430,39 @@ def test_import_rename_external_id_duplicate_previous_id_rejected(self) -> None: assert not self.taxonomy.tag_set.filter(external_id="tag_50").exists() assert not self.taxonomy.tag_set.filter(external_id="tag_60").exists() + def test_import_rename_external_id_reuses_id_freed_by_replace_delete(self) -> None: + """ + Replace-mode import that omits tag_1 (so the delete sweep queues it + for deletion) and, in the same file, renames tag_2 onto id="tag_1", + reusing the external_id that tag_1's deletion is about to free up. + This must succeed end-to-end: tag_1 being still physically present + (but already queued for deletion) at validate time must not be + treated as a real collision. + """ + old_tag_1_pk = self.taxonomy.tag_set.get(external_id="tag_1").pk + old_tag_2_pk = self.taxonomy.tag_set.get(external_id="tag_2").pk + + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_1", "value": "Tag 1", "previous_id": "tag_2"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + replace=True, + ) + assert result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + + # tag_1's old row was genuinely deleted, not merely renamed away. + assert not Tag.objects.filter(pk=old_tag_1_pk).exists() + + # tag_2 is the same underlying row, now wearing tag_1's freed-up id. + renamed_tag = Tag.objects.get(pk=old_tag_2_pk) + assert renamed_tag.external_id == "tag_1" + assert renamed_tag.value == "Tag 1" + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() From 4eb3b65fbcac405f9e7096880f7ddb8bce780cdb Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Mon, 7 Sep 2026 12:01:12 +0400 Subject: [PATCH 5/9] fix: don't let RenameTag/UpdateParentTag misfire on a tag queued for deletion RenameTag.applies_for and UpdateParentTag.applies_for look up a tag by external_id against live database state, unaware that a replace-mode delete sweep might be about to remove that exact tag in the same import. A row reusing a freed-up external_id via RenameTagExternalId could therefore also trigger RenameTag/UpdateParentTag against the tag that's about to be deleted, producing a spurious duplicate-value conflict or an uncaught Tag.DoesNotExist once the delete runs first. Both applies_for methods now skip firing when the matched tag is already queued for deletion in the same import; indexed_actions is threaded through all six applies_for implementations so the check has what it needs at the single call site in generate_actions. --- src/openedx_tagging/import_export/actions.py | 36 ++++++++++---- .../import_export/import_plan.py | 2 +- .../import_export/test_actions.py | 49 +++++++++++++++++++ .../openedx_tagging/import_export/test_api.py | 26 ++++++++-- 4 files changed, 99 insertions(+), 14 deletions(-) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index 322bc6b2a..cdf347ae1 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -47,7 +47,7 @@ def __str__(self) -> str: return self.__repr__() @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ Implement this to meet the conditions that a `TagItem` needs to have for this action. If this function returns `True` for `tag` @@ -205,7 +205,7 @@ def __str__(self) -> str: ) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ This action applies whenever the tag does not exist """ @@ -291,12 +291,21 @@ def __str__(self) -> str: return str(description_str) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ - This action applies whenever there is a change on the parent + This action applies whenever there is a change on the parent. + + Does not apply if the matched tag is queued for deletion in this + same import: a row reusing that tag's freed-up external_id via + `previous_id` is handled by RenameTagExternalId instead. """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) + if indexed_actions and any( + taxonomy_tag.external_id == action.tag.id + for action in indexed_actions.get("delete", []) + ): + return False return ( taxonomy_tag.parent is not None and taxonomy_tag.parent.external_id != tag.parent_id @@ -353,12 +362,21 @@ def __str__(self) -> str: return str(description_str) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ - This action applies whenever there is a change on the tag value + This action applies whenever there is a change on the tag value. + + Does not apply if the matched tag is queued for deletion in this + same import: a row reusing that tag's freed-up external_id via + `previous_id` is handled by RenameTagExternalId instead. """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) + if indexed_actions and any( + taxonomy_tag.external_id == action.tag.id + for action in indexed_actions.get("delete", []) + ): + return False return taxonomy_tag.value != tag.value except Tag.DoesNotExist: return False @@ -418,7 +436,7 @@ def __str__(self) -> str: ) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ This action applies whenever previous_id is set and differs from id """ @@ -529,7 +547,7 @@ def __str__(self) -> str: name = "delete" @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ This action is an exception. These actions are created in `TagImportPlan.generate_actions` if `replace=True` @@ -566,7 +584,7 @@ def __str__(self) -> str: return str(_("No changes needed for {tag}").format(tag=self.tag)) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: """ No validations necessary """ diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index 0efa1d02a..35627896b 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -179,7 +179,7 @@ def generate_actions( # Check all available actions and add which ones should be executed for action_cls in available_actions: - if action_cls.applies_for(self.taxonomy, tag): + if action_cls.applies_for(self.taxonomy, tag, self.indexed_actions): self._build_action(action_cls, tag) has_action = True diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index e36e97084..148e3bc55 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -462,6 +462,31 @@ def test_applies_for(self, tag_id: str, parent_id: str | None, expected: bool): ) self.assertEqual(result, expected) + def test_applies_for_ignores_tag_queued_for_delete(self) -> None: + # Same as the ('tag_2', 'tag_3', True) case above (parent genuinely + # changes), but tag_2 is queued for deletion in this same import + # (e.g. its external_id is being reused by a RenameTagExternalId + # row via previous_id): this action must not also fire against the + # doomed tag. + indexed_actions = {'delete': [ + DeleteTag( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_2', value='Tag 2', index=1), + index=1, + ) + ]} + result = UpdateParentTag.applies_for( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_2', + value='_', + parent_id='tag_3', + index=100, + ), + indexed_actions=indexed_actions, + ) + self.assertFalse(result) + @ddt.data( ('tag_2', 'tag_30', 1), # Invalid parent ('tag_2', None, 0), # Without parent @@ -532,6 +557,30 @@ def test_applies_for(self, tag_id: str, value: str, expected: bool): ) self.assertEqual(result, expected) + def test_applies_for_ignores_tag_queued_for_delete(self) -> None: + # Same as the ('tag_1', 'Tag 1 v2', True) case above (value + # genuinely changes), but tag_1 is queued for deletion in this same + # import (e.g. its external_id is being reused by a + # RenameTagExternalId row via previous_id): this action must not + # also fire against the doomed tag. + indexed_actions = {'delete': [ + DeleteTag( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_1', value='Tag 1', index=1), + index=1, + ) + ]} + result = RenameTag.applies_for( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_1', + value='Tag 1 v2', + index=100, + ), + indexed_actions=indexed_actions, + ) + self.assertFalse(result) + @ddt.data( ('Tag 2', 1), # There is a tag with the same value on database ('Tag 10', 1), # There is a tag with the same value on create action diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 1c793f93a..7fcf6b8e4 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -438,12 +438,23 @@ def test_import_rename_external_id_reuses_id_freed_by_replace_delete(self) -> No This must succeed end-to-end: tag_1 being still physically present (but already queued for deletion) at validate time must not be treated as a real collision. + + The row's value ("Renamed From Tag 2") and parent_id ("tag_3") both + genuinely differ from the doomed tag_1's current value ("Tag 1") and + parent (None). This is deliberate: with matching values, RenameTag + and UpdateParentTag's DB-only lookups (unaware that tag_1 is queued + for deletion in this same import) would never fire in the first + place, so the test would pass even without the fix that makes them + skip a tag queued for deletion, and end up proving nothing about it. + tag_3 gets its own no-op row so it survives as a valid parent target, + instead of also being swept up by the same replace-mode delete. """ old_tag_1_pk = self.taxonomy.tag_set.get(external_id="tag_1").pk old_tag_2_pk = self.taxonomy.tag_set.get(external_id="tag_2").pk importFile = BytesIO(json.dumps({"tags": [ - {"id": "tag_1", "value": "Tag 1", "previous_id": "tag_2"}, + {"id": "tag_1", "value": "Renamed From Tag 2", "previous_id": "tag_2", "parent_id": "tag_3"}, + {"id": "tag_3", "value": "Tag 3"}, ]}).encode()) result, task, _plan = import_export_api.import_tags( self.taxonomy, @@ -451,17 +462,24 @@ def test_import_rename_external_id_reuses_id_freed_by_replace_delete(self) -> No self.parser_format, replace=True, ) - assert result log = import_export_api.get_last_import_log(self.taxonomy) assert log == task.log + assert "Traceback" not in log + assert "Duplicated tag value" not in log + assert result # tag_1's old row was genuinely deleted, not merely renamed away. assert not Tag.objects.filter(pk=old_tag_1_pk).exists() - # tag_2 is the same underlying row, now wearing tag_1's freed-up id. + # tag_2 is the same underlying row, now wearing tag_1's freed-up id, + # with the row's OWN new value and parent, not tag_1's old ones: + # proof that RenameTag/UpdateParentTag did not sneak in and mutate + # the doomed tag_1 before it got deleted. renamed_tag = Tag.objects.get(pk=old_tag_2_pk) assert renamed_tag.external_id == "tag_1" - assert renamed_tag.value == "Tag 1" + assert renamed_tag.value == "Renamed From Tag 2" + assert renamed_tag.parent is not None + assert renamed_tag.parent.external_id == "tag_3" def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") From a3415e220c7997a3640ee0562c2756ffeb1a46d7 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Tue, 8 Sep 2026 19:32:38 +0400 Subject: [PATCH 6/9] feat: support renaming tags to each other's prior external_ids in one import Two or more tags renaming onto each other's current external_id values in the same import (a swap, or a longer cycle) had no valid execution order: (taxonomy, external_id) is a DB-level unique constraint enforced per-statement on every backend this project runs on, so whichever rename executes first collides with the other tag still holding its target id. Add a StageTagExternalId action that moves a tag off a contended external_id, through a temporary placeholder, before the row claiming that id lands on it. TagImportPlan.generate_actions detects contention up front (a tag's current external_id is another row's target) and stages only those tags, so plain renames are unaffected. Actions now carry a resolved target_pk instead of re-resolving by external_id at execute time, since a staged tag's external_id no longer matches its previous_id by the time it executes. Also fixes RenameTag.applies_for and UpdateParentTag.applies_for, which looked up a tag by its new external_id and could therefore match the wrong tag for any previous_id rename row, not just contended ones. Amends ADR 0010 to document that renames within one import are now order-independent, and that this doesn't extend to swapping values. --- .../0010-mutable-tag-external-id.rst | 21 +- src/openedx_tagging/import_export/actions.py | 100 +++++++++- .../import_export/import_plan.py | 52 ++++- .../import_export/test_actions.py | 179 ++++++++++++++++++ .../openedx_tagging/import_export/test_api.py | 112 +++++++++++ .../import_export/test_import_plan.py | 77 ++++++++ 6 files changed, 528 insertions(+), 13 deletions(-) diff --git a/docs/openedx_tagging/decisions/0010-mutable-tag-external-id.rst b/docs/openedx_tagging/decisions/0010-mutable-tag-external-id.rst index 0ee8acea6..5ea616ed4 100644 --- a/docs/openedx_tagging/decisions/0010-mutable-tag-external-id.rst +++ b/docs/openedx_tagging/decisions/0010-mutable-tag-external-id.rst @@ -62,10 +62,18 @@ enable a pathway for its value to change, rather than adding a new field. identifiers for now; keeping that history within Open edX itself could be a future phase of work. - The existing per-taxonomy uniqueness constraint on ``external_id`` - (``unique_together`` on ``(taxonomy, external_id)``) is unchanged and still applies - to a rename: if the new ``id`` collides with a different existing tag in the same - taxonomy, the import rejects the row, the same way a duplicate ``external_id`` on - tag creation already does today. + (``unique_together`` on ``(taxonomy, external_id)``) is unchanged: if the new ``id`` + collides with a tag that isn't itself part of a rename in the same import, the + import rejects the row, the same way a duplicate ``external_id`` on tag creation + already does today. +- Renames within a single import are order-independent: two or more tags can rename + onto each other's current ``external_id`` values in the same file (a swap, or a + longer cycle), regardless of row order. Any tag whose current ``external_id`` is + another row's target is moved through a temporary, internal identifier first, then + landed on its final value, so no two tags ever collide mid-import. This applies to + ``external_id`` only: a row that also tries to take on the other tag's current + ``value`` in the same swap is still rejected, since ``value`` carries the same + per-taxonomy uniqueness constraint without the same staging treatment. - No schema change and no migration: ``external_id`` already permits writes at the model layer, and ``previous_id`` is read per-row from the import file and consumed only while generating the import plan. @@ -116,6 +124,11 @@ institutions hit when they rename an identifier. Changelog --------- +2026-09-05: + +* Revised: renames within one import are now order-independent, so two or more tags + can swap or cycle through each other's ``external_id`` values in a single file. + 2026-07-07: * Revised: dropped the new ``Tag.code`` field. ``Tag.external_id`` becomes mutable diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index cdf347ae1..cf84d461c 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -3,6 +3,8 @@ """ from __future__ import annotations +from uuid import uuid4 + from django.utils.translation import gettext as _ from ..models import Tag, Taxonomy @@ -35,10 +37,11 @@ class ImportAction: name = "import_action" - def __init__(self, taxonomy: Taxonomy, tag, index: int): + def __init__(self, taxonomy: Taxonomy, tag, index: int, target_pk: int | None = None): self.taxonomy = taxonomy self.tag = tag self.index = index + self.target_pk = target_pk def __repr__(self) -> str: return str(_("Action {name} (index={index},id={id})").format(name=self.name, index=self.index, id=self.tag.id)) @@ -101,7 +104,17 @@ def _validate_parent(self, indexed_actions) -> ImportActionError | None: """ try: # Validates that the parent exists on the taxonomy - self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + parent_tag = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + # A parent that is staged away in this same import is about to + # lose this external_id, so it must not be accepted at face + # value: fall through to the same "created/renamed-in earlier + # in this import" check below. + is_staged_away = any( + parent_tag.pk == action.target_pk + for action in indexed_actions.get("stage_external_id", []) + ) + if is_staged_away: + raise Tag.DoesNotExist except Tag.DoesNotExist: # Or if the parent is created or renamed-in on previous actions found = self._search_action( @@ -298,7 +311,14 @@ def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: Does not apply if the matched tag is queued for deletion in this same import: a row reusing that tag's freed-up external_id via `previous_id` is handled by RenameTagExternalId instead. + + Also does not apply if `previous_id` is set and differs from `id`: + that shape is a rename_external_id row, and looking it up by its new + `id` here would resolve to a *different* tag than the one actually + being renamed (e.g. the other tag in a swap). """ + if tag.previous_id and tag.id != tag.previous_id: + return False try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) if indexed_actions and any( @@ -369,7 +389,14 @@ def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: Does not apply if the matched tag is queued for deletion in this same import: a row reusing that tag's freed-up external_id via `previous_id` is handled by RenameTagExternalId instead. + + Also does not apply if `previous_id` is set and differs from `id`: + that shape is a rename_external_id row, and looking it up by its new + `id` here would resolve to a *different* tag than the one actually + being renamed (e.g. the other tag in a swap). """ + if tag.previous_id and tag.id != tag.previous_id: + return False try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) if indexed_actions and any( @@ -448,14 +475,22 @@ def _validate_new_id(self, indexed_actions) -> ImportActionError | None: or with a prior create/rename action in the same import. A tag that a replace-mode delete sweep is removing in this same import doesn't count as a collision, since the delete executes before this action - (see TagImportPlan._build_delete_actions). + (see TagImportPlan._build_delete_actions). Neither does a tag that is + staged away to a placeholder external_id in this same import, since + it executes before this action too (see StageTagExternalId). """ is_freed_by_delete = any( self.tag.id == action.tag.id for action in indexed_actions["delete"] ) if "delete" in indexed_actions else False - if not is_freed_by_delete and self.taxonomy.tag_set.filter(external_id=self.tag.id).exists(): + existing = self.taxonomy.tag_set.filter(external_id=self.tag.id).first() + is_staged_away = existing is not None and any( + existing.pk == action.target_pk + for action in indexed_actions.get("stage_external_id", []) + ) + + if not is_freed_by_delete and not is_staged_away and existing is not None: return ImportActionError( action=self, message=_("A tag with external_id ({id}) already exists.").format(id=self.tag.id), @@ -521,8 +556,17 @@ def validate(self, indexed_actions) -> list[ImportActionError]: def execute(self) -> None: """ Renames a tag's external_id in place, and updates its value and parent + + Resolves the target tag by primary key rather than by looking up + `previous_id` again, since by execution time a StageTagExternalId + action may have already moved it off that external_id onto a + placeholder (see TagImportPlan._build_staging_actions). """ - target = self.taxonomy.tag_set.get(external_id=self.tag.previous_id) + # target_pk is only None for an unmatched previous_id, which + # validate() already turns into a plan error; TagImportPlan.execute() + # never calls execute() on any action when errors are present. + assert self.target_pk is not None + target = self.taxonomy.tag_set.get(pk=self.target_pk) target.external_id = self.tag.id target.value = self.tag.value target.parent = ( @@ -532,6 +576,51 @@ def execute(self) -> None: target.save() +class StageTagExternalId(ImportAction): + """ + Action to move a tag off a contended external_id before another action + in the same import lands on it. + + Action created (not from a file row, but synthesized by + TagImportPlan._build_staging_actions) when a tag's current external_id + is the target of another RenameTagExternalId row in the same import. + Two or more tags renaming onto each other's ids (a swap or an N-cycle) + have no valid execution order without this: (taxonomy, external_id) is + a DB-level unique constraint enforced per-statement, not deferred, on + every backend this project runs on. + """ + + name = "stage_external_id" + + def __str__(self) -> str: + return str(_("Stage tag (pk={target_pk}) off its current external_id.").format(target_pk=self.target_pk)) + + @classmethod + def applies_for(cls, taxonomy: Taxonomy, tag, indexed_actions=None) -> bool: + """ + This action is an exception: synthesized in TagImportPlan.generate_actions. + """ + return False + + def validate(self, indexed_actions) -> list[ImportActionError]: + """ + No validations necessary + """ + return [] + + def execute(self) -> None: + """ + Moves the tag to a placeholder external_id, freeing its old one for + another action in this same import to land on. + """ + # Staging actions are only built (in _build_staging_actions) with a + # resolved target_pk; there is no code path that constructs one with + # target_pk=None. + assert self.target_pk is not None + placeholder = f"oel-import-staging:{uuid4().hex}" + self.taxonomy.tag_set.filter(pk=self.target_pk).update(external_id=placeholder) + + class DeleteTag(ImportAction): """ Action for delete a Tag @@ -608,6 +697,7 @@ def execute(self) -> None: RenameTag, RenameTagExternalId, CreateTag, + StageTagExternalId, DeleteTag, WithoutChanges, ] diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index 35627896b..d96f981b9 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -7,7 +7,15 @@ from django.db import transaction from ..models import Tag, TagImportTask, Taxonomy -from .actions import DeleteTag, ImportAction, UpdateParentTag, WithoutChanges, available_actions +from .actions import ( + DeleteTag, + ImportAction, + RenameTagExternalId, + StageTagExternalId, + UpdateParentTag, + WithoutChanges, + available_actions, +) from .exceptions import ImportActionError @@ -58,14 +66,14 @@ def _init_indexed_actions(self): for action in available_actions: self.indexed_actions[action.name] = [] - def _build_action(self, action_cls: type[ImportAction], tag: TagItem): + def _build_action(self, action_cls: type[ImportAction], tag: TagItem, target_pk: int | None = None): """ Build an action with `tag`. Run action validation and adds the errors to the errors lists Add to the action list and the indexed actions """ - action = action_cls(self.taxonomy, tag, len(self.actions) + 1) + action = action_cls(self.taxonomy, tag, len(self.actions) + 1, target_pk=target_pk) # We validate if there are no inconsistencies when executing this action self.errors.extend(action.validate(self.indexed_actions)) @@ -134,6 +142,33 @@ def _build_delete_actions(self, tags: dict): ), ) + def _resolve_rename_target_pk(self, tag: TagItem) -> int | None: + """ + Resolve the pk of the tag a RenameTagExternalId row targets, via its + previous_id. Returns None if no such tag exists (an unmatched + previous_id -- RenameTagExternalId.validate() already rejects this). + """ + return self.taxonomy.tag_set.filter(external_id=tag.previous_id).values_list("pk", flat=True).first() + + def _build_staging_actions(self, tags: list[TagItem]) -> None: + """ + Stage any tag whose current external_id is the target `id` of another + rename row in this same import, so no two tags collide on external_id + regardless of execution order (see StageTagExternalId). + """ + target_ids = { + tag.id for tag in tags + if RenameTagExternalId.applies_for(self.taxonomy, tag) + } + for tag in tags: + if not RenameTagExternalId.applies_for(self.taxonomy, tag): + continue + if tag.previous_id not in target_ids: + continue + target_pk = self._resolve_rename_target_pk(tag) + if target_pk is not None: + self._build_action(StageTagExternalId, tag, target_pk=target_pk) + def generate_actions( self, tags: list[TagItem], @@ -174,13 +209,22 @@ def generate_actions( # Delete all not readed tags self._build_delete_actions(tags_for_delete) + # Stage tags whose external_id is contended by another rename row in + # this same import, so a swap or an N-cycle of renames has a valid + # execution order regardless of how the rows are ordered in the file. + self._build_staging_actions(tags) + for tag in tags: has_action = False # Check all available actions and add which ones should be executed for action_cls in available_actions: if action_cls.applies_for(self.taxonomy, tag, self.indexed_actions): - self._build_action(action_cls, tag) + target_pk = ( + self._resolve_rename_target_pk(tag) + if action_cls is RenameTagExternalId else None + ) + self._build_action(action_cls, tag, target_pk=target_pk) has_action = True if not has_action: diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index 148e3bc55..fad4cadca 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -13,6 +13,7 @@ ImportAction, RenameTag, RenameTagExternalId, + StageTagExternalId, UpdateParentTag, WithoutChanges, ) @@ -162,6 +163,70 @@ def test_validate_parent_with_rename_external_id_action(self) -> None: error = action._validate_parent(indexed_actions) # pylint: disable=protected-access self.assertIsNone(error) + def test_validate_parent_staged_away_accepted_when_landing_row_queued(self) -> None: + """ + A parent referenced by external_id (tag_1) currently exists, but its + holder is queued to be staged away in this same import, because + another row (renaming tag_2) is landing on id=tag_1. This validates + as a known parent, since after the import a tag will hold tag_1 + again (just a different underlying tag) -- same convention as + referencing a newly-created tag. + """ + parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk + staged_tag = TagItem(id='tag_90', value='_', previous_id='tag_1', index=1) + landing_tag = TagItem(id='tag_1', value='_', previous_id='tag_2', index=2) + indexed_actions = dict(self.indexed_actions) + indexed_actions['stage_external_id'] = [ + StageTagExternalId(taxonomy=self.taxonomy, tag=staged_tag, index=1, target_pk=parent_pk) + ] + indexed_actions['rename_external_id'] = [ + RenameTagExternalId(taxonomy=self.taxonomy, tag=landing_tag, index=2, target_pk=parent_pk) + ] + action = ImportAction( + self.taxonomy, + TagItem( + id='tag_110', + value='_', + parent_id='tag_1', + index=100, + ), + index=100, + ) + error = action._validate_parent(indexed_actions) # pylint: disable=protected-access + self.assertIsNone(error) + + def test_validate_parent_staged_away_rejected_when_landing_row_not_queued(self) -> None: + """ + Same setup as above, but no RenameTagExternalId row lands on + id=tag_1 (e.g. it would appear later in file order, or doesn't + exist): the parent reference must be cleanly rejected, not crash. + """ + parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk + staged_tag = TagItem(id='tag_90', value='_', previous_id='tag_1', index=1) + indexed_actions = dict(self.indexed_actions) + indexed_actions['stage_external_id'] = [ + StageTagExternalId(taxonomy=self.taxonomy, tag=staged_tag, index=1, target_pk=parent_pk) + ] + action = ImportAction( + self.taxonomy, + TagItem( + id='tag_110', + value='_', + parent_id='tag_1', + index=100, + ), + index=100, + ) + error = action._validate_parent(indexed_actions) # pylint: disable=protected-access + self.assertEqual( + str(error), + ( + "Action error in 'import_action' (#100): " + "Unknown parent tag (tag_1). " + "You need to add parent before the child in your file." + ) + ) + @ddt.data( ( 'Tag 1', @@ -487,6 +552,23 @@ def test_applies_for_ignores_tag_queued_for_delete(self) -> None: ) self.assertFalse(result) + def test_applies_for_swap_previous_id_guard(self) -> None: + # In a swap (tag_1 <-> tag_2 external_ids), this row's new `id` + # (tag_1) resolves via external_id lookup to the *other* tag in the + # swap (still holding external_id=tag_1 at this point), not to the + # tag actually being renamed (tag_2, via previous_id). Must not fire. + result = UpdateParentTag.applies_for( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_1', + value='Tag 2', + previous_id='tag_2', + parent_id='tag_3', + index=100, + ), + ) + self.assertFalse(result) + @ddt.data( ('tag_2', 'tag_30', 1), # Invalid parent ('tag_2', None, 0), # Without parent @@ -581,6 +663,22 @@ def test_applies_for_ignores_tag_queued_for_delete(self) -> None: ) self.assertFalse(result) + def test_applies_for_swap_previous_id_guard(self) -> None: + # In a swap (tag_1 <-> tag_2 external_ids), this row's new `id` + # (tag_1) resolves via external_id lookup to the *other* tag in the + # swap (still holding external_id=tag_1 at this point), not to the + # tag actually being renamed (tag_2, via previous_id). Must not fire. + result = RenameTag.applies_for( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_1', + value='Tag 2', + previous_id='tag_2', + index=100, + ), + ) + self.assertFalse(result) + @ddt.data( ('Tag 2', 1), # There is a tag with the same value on database ('Tag 10', 1), # There is a tag with the same value on create action @@ -702,6 +800,35 @@ def test_validate_new_id_freed_by_queued_delete_action(self) -> None: errors = action.validate(indexed_actions) self.assertEqual(errors, []) + def test_validate_new_id_exempted_by_staging(self) -> None: + # Same setup as test_validate_new_id_collides_with_db_tag (new id + # tag_2 still exists in the DB), but this time tag_2 is queued to be + # staged away to a placeholder external_id in this same import (e.g. + # as the other half of a swap), so reusing its external_id is not a + # real collision: the staging action executes before this one. + tag_2_pk = self.taxonomy.tag_set.get(external_id='tag_2').pk + indexed_actions = dict(self.indexed_actions) + indexed_actions['stage_external_id'] = [ + StageTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_1', value='Tag 2', previous_id='tag_2', index=1), + index=1, + target_pk=tag_2_pk, + ) + ] + action = RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_2', + value='Tag 1', + previous_id='tag_1', + index=100, + ), + index=100, + ) + errors = action.validate(indexed_actions) + self.assertEqual(errors, []) + def test_validate_new_id_collides_with_create_action(self) -> None: # The new id (tag_10) matches a pending 'create' action from # self.indexed_actions (see TestImportActionMixin.setUp). @@ -813,6 +940,7 @@ def test_execute(self) -> None: taxonomy=self.taxonomy, tag=tag_item, index=100, + target_pk=pk, ) action.execute() tag.refresh_from_db() @@ -822,6 +950,57 @@ def test_execute(self) -> None: self.assertEqual(tag.parent.external_id, 'tag_3') +class TestStageTagExternalId(TestImportActionMixin, TestCase): + """ + Test for 'stage_external_id' action + """ + + def test_applies_for(self) -> None: + result = StageTagExternalId.applies_for( + self.taxonomy, + tag=TagItem( + id='tag_2', + value='_', + previous_id='tag_1', + index=100, + ), + ) + self.assertFalse(result) + + def test_validate(self) -> None: + action = StageTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_2', + value='_', + previous_id='tag_1', + index=100, + ), + index=100, + target_pk=self.taxonomy.tag_set.get(external_id='tag_1').pk, + ) + self.assertEqual(action.validate(self.indexed_actions), []) + + def test_execute(self) -> None: + tag = self.taxonomy.tag_set.get(external_id='tag_1') + pk = tag.pk + action = StageTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem( + id='tag_2', + value='_', + previous_id='tag_1', + index=100, + ), + index=100, + target_pk=pk, + ) + action.execute() + tag.refresh_from_db() + self.assertEqual(tag.pk, pk) + self.assertTrue(tag.external_id.startswith("oel-import-staging:")) + + class TestDeleteTag(TestImportActionMixin, TestCase): """ Test for 'delete' action diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 7fcf6b8e4..09a964843 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -481,6 +481,118 @@ def test_import_rename_external_id_reuses_id_freed_by_replace_delete(self) -> No assert renamed_tag.parent is not None assert renamed_tag.parent.external_id == "tag_3" + def test_import_swap_external_ids(self) -> None: + """ + A 2-tag swap (tag_1 <-> tag_3 external_ids, both root tags with no + parent, so parent handling doesn't complicate the assertions) has no + valid plain execution order: renaming either tag onto the other's + external_id first collides with a per-statement, non-deferred DB + unique constraint on (taxonomy, external_id). Each tag must be + staged through a placeholder id first (see ADR 0010 amendment). + """ + old_pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + old_pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Traceback" not in log + assert result + + # Both tags keep their original pks: this was a rename, not a + # delete-and-recreate. + tag_1 = Tag.objects.get(pk=old_pk_1) + tag_3 = Tag.objects.get(pk=old_pk_3) + assert tag_1.external_id == "tag_3" + assert tag_1.value == "Tag 1" + assert tag_3.external_id == "tag_1" + assert tag_3.value == "Tag 3" + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_tags = json.loads(output).get("tags") + for tag in exported_tags: + assert not tag.get("id", "").startswith("oel-import-staging:") + exported_by_value = {tag["value"]: tag["id"] for tag in exported_tags} + assert exported_by_value["Tag 1"] == "tag_3" + assert exported_by_value["Tag 3"] == "tag_1" + + def test_import_three_cycle_external_ids(self) -> None: + """ + End-to-end 3-cycle: tag_1 -> tag_2 -> tag_3 -> tag_1. Same staging + mechanism as a 2-tag swap, generalized to any cycle length. + """ + old_pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + old_pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + old_pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_2", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_3", "value": "Tag 2", "previous_id": "tag_2"}, + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Traceback" not in log + assert result + + assert Tag.objects.get(pk=old_pk_1).external_id == "tag_2" + assert Tag.objects.get(pk=old_pk_2).external_id == "tag_3" + assert Tag.objects.get(pk=old_pk_3).external_id == "tag_1" + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_tags = json.loads(output).get("tags") + for tag in exported_tags: + assert not tag.get("id", "").startswith("oel-import-staging:") + + def test_import_swap_external_ids_with_colliding_values_rejected(self) -> None: + """ + A contended external_id swap where each row ALSO tries to take on + the other tag's current value: this must still cleanly reject, not + raise an IntegrityError or otherwise crash. Value swaps are an + explicit, documented limitation (see ADR 0010 amendment): + (taxonomy, value) has the identical unique-constraint shape as + (taxonomy, external_id), but staging is only implemented for + external_id. + """ + old_pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + old_pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_3", "value": "Tag 3", "previous_id": "tag_1"}, + {"id": "tag_1", "value": "Tag 1", "previous_id": "tag_3"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Traceback" not in log + assert "Duplicated tag value" in log + assert not result + + # Nothing changed: neither tag's external_id or value moved. + tag_1 = Tag.objects.get(pk=old_pk_1) + tag_3 = Tag.objects.get(pk=old_pk_3) + assert tag_1.external_id == "tag_1" + assert tag_1.value == "Tag 1" + assert tag_3.external_id == "tag_3" + assert tag_3.value == "Tag 3" + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index cc7e79d51..cf3333f40 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -465,3 +465,80 @@ def test_error_in_execute(self): assert not self.taxonomy.tag_set.filter(external_id=created_tag).exists() assert not self.import_plan.execute() assert not self.taxonomy.tag_set.filter(external_id=created_tag).exists() + + def test_generate_actions_swap_stages_and_renames(self) -> None: + """ + A 2-tag swap (tag_1 <-> tag_3 external_ids, both root tags with no + parent) has no valid plain execution order, since (taxonomy, + external_id) is unique and enforced per-statement: each tag must be + staged through a placeholder id before landing on the other's old + id. + """ + tags = [ + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual(len(self.import_plan.indexed_actions['stage_external_id']), 2) + self.assertEqual(len(self.import_plan.indexed_actions['rename_external_id']), 2) + self.assertEqual(self.import_plan.indexed_actions['rename'], []) + self.assertEqual(self.import_plan.indexed_actions['update_parent'], []) + + def test_generate_actions_three_cycle_stages_all(self) -> None: + """ + A 3-cycle (tag_1 -> tag_2 -> tag_3 -> tag_1) is staged the same way + as a 2-tag swap: every tag in the cycle is contended by another row + in the same import, so all three are staged first. + """ + tags = [ + TagItem(id='tag_2', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_3', value='Tag 2', previous_id='tag_2'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual(len(self.import_plan.indexed_actions['stage_external_id']), 3) + self.assertEqual(len(self.import_plan.indexed_actions['rename_external_id']), 3) + + def test_generate_actions_chain_stages_only_contended_tags(self) -> None: + """ + A chain where each row (except the last) renames onto an id + currently held by the *next* row's tag: tag_2 -> tag_3, tag_3 -> + tag_4, tag_4 -> tag_90 (a fresh, uncontended id). Only tag_3 and + tag_4 are contended (their current external_id is some other row's + target `id`); tag_2's current id (tag_2) is nobody's target, so it + is not staged. + """ + tag_2_pk = self.taxonomy.tag_set.get(external_id='tag_2').pk + tag_3_pk = self.taxonomy.tag_set.get(external_id='tag_3').pk + tag_4_pk = self.taxonomy.tag_set.get(external_id='tag_4').pk + + tags = [ + TagItem(id='tag_3', value='Tag 2', previous_id='tag_2'), + TagItem(id='tag_4', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_90', value='Tag 4', previous_id='tag_4'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + staged_pks = { + action.target_pk for action in self.import_plan.indexed_actions['stage_external_id'] + } + self.assertEqual(staged_pks, {tag_3_pk, tag_4_pk}) + self.assertNotIn(tag_2_pk, staged_pks) + self.assertEqual(len(self.import_plan.indexed_actions['rename_external_id']), 3) + + def test_generate_actions_genuine_collision_not_staged(self) -> None: + """ + A rename targeting an id held by an unrelated tag that is not + itself being renamed or deleted in this import is a real collision, + not a staging candidate: the tag holding tag_2 is not a party to + any rename row in this file. + """ + tags = [ + TagItem(id='tag_2', value='Tag 1', previous_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.indexed_actions['stage_external_id'], []) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("already exists", str(self.import_plan.errors[0])) From e8a1db037b58c6c99b19f3fac26dd9990dc7d948 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Tue, 8 Sep 2026 19:54:30 +0400 Subject: [PATCH 7/9] fix: reject a parent_id referencing a tag's stale, pre-rename external_id parent_id already means a tag's desired end-state parent, not the parent it had before this import, since UpdateParentTag lets a row move a tag to a different parent mid-import. _validate_parent didn't enforce that consistently: a reference to a parent's new external_id only validated when the rename came first in the file, but a reference to the parent's old external_id validated unconditionally, since the not-yet-renamed tag still resolves live in the database during validation. That was already an execute-time crash whenever the rename ran first. It gets worse with same-import swaps: a stale old id can resolve to a different tag entirely instead of just failing. _build_staging_actions now records every rename row's resolved target pk, not only the ones contended enough to need staging, so _validate_parent can reject any parent_id matching a tag being renamed away from that exact external_id in this same import, regardless of whether another tag reuses it. --- src/openedx_tagging/import_export/actions.py | 20 +++--- .../import_export/import_plan.py | 15 +++- .../import_export/test_actions.py | 70 +++++++++++++++---- .../openedx_tagging/import_export/test_api.py | 29 ++++++++ .../import_export/test_import_plan.py | 30 ++++++++ 5 files changed, 138 insertions(+), 26 deletions(-) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index cf84d461c..ee190449e 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -101,19 +101,21 @@ def _search_action( def _validate_parent(self, indexed_actions) -> ImportActionError | None: """ Helper method to validate that the parent tag has already been defined. + + parent_id must reference a tag's desired end-state external_id, not + whatever external_id currently resolves to some tag in the database: + UpdateParentTag/RenameTagExternalId already let a tag's own identity + change mid-import, so a parent_id matching a tag that's being renamed + away from that exact external_id in this same import is stale and must + not be accepted at face value -- fall through to the same + "landed/created earlier in this import" check already used for a + brand-new or renamed-in parent, so a reference to the correct, new id + still works when that rename comes first in the file. """ try: # Validates that the parent exists on the taxonomy parent_tag = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) - # A parent that is staged away in this same import is about to - # lose this external_id, so it must not be accepted at face - # value: fall through to the same "created/renamed-in earlier - # in this import" check below. - is_staged_away = any( - parent_tag.pk == action.target_pk - for action in indexed_actions.get("stage_external_id", []) - ) - if is_staged_away: + if parent_tag.pk in indexed_actions.get("_vacated_pks", set()): raise Tag.DoesNotExist except Tag.DoesNotExist: # Or if the parent is created or renamed-in on previous actions diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index d96f981b9..a04befdf8 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -155,19 +155,28 @@ def _build_staging_actions(self, tags: list[TagItem]) -> None: Stage any tag whose current external_id is the target `id` of another rename row in this same import, so no two tags collide on external_id regardless of execution order (see StageTagExternalId). + + Also records every rename row's resolved target pk in + indexed_actions["_vacated_pks"], whether staged or not: a tag being + renamed away from an external_id makes that external_id stale for + anyone else to reference (e.g. as a parent_id) via the live database, + even when nothing reuses it in this same import (see _validate_parent). """ target_ids = { tag.id for tag in tags if RenameTagExternalId.applies_for(self.taxonomy, tag) } + vacated_pks = set() for tag in tags: if not RenameTagExternalId.applies_for(self.taxonomy, tag): continue - if tag.previous_id not in target_ids: - continue target_pk = self._resolve_rename_target_pk(tag) - if target_pk is not None: + if target_pk is None: + continue + vacated_pks.add(target_pk) + if tag.previous_id in target_ids: self._build_action(StageTagExternalId, tag, target_pk=target_pk) + self.indexed_actions["_vacated_pks"] = vacated_pks def generate_actions( self, diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index fad4cadca..6d7108e55 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -163,22 +163,20 @@ def test_validate_parent_with_rename_external_id_action(self) -> None: error = action._validate_parent(indexed_actions) # pylint: disable=protected-access self.assertIsNone(error) - def test_validate_parent_staged_away_accepted_when_landing_row_queued(self) -> None: + def test_validate_parent_vacated_accepted_when_landing_row_queued(self) -> None: """ - A parent referenced by external_id (tag_1) currently exists, but its - holder is queued to be staged away in this same import, because - another row (renaming tag_2) is landing on id=tag_1. This validates - as a known parent, since after the import a tag will hold tag_1 - again (just a different underlying tag) -- same convention as + A parent referenced by external_id (tag_1) currently exists, but that + external_id is vacated in this same import (some row's rename target + pk resolves to it -- see TagImportPlan._build_staging_actions), + because another row (renaming tag_2) is landing on id=tag_1. This + validates as a known parent, since after the import a tag will hold + tag_1 again (just a different underlying tag) -- same convention as referencing a newly-created tag. """ parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk - staged_tag = TagItem(id='tag_90', value='_', previous_id='tag_1', index=1) landing_tag = TagItem(id='tag_1', value='_', previous_id='tag_2', index=2) indexed_actions = dict(self.indexed_actions) - indexed_actions['stage_external_id'] = [ - StageTagExternalId(taxonomy=self.taxonomy, tag=staged_tag, index=1, target_pk=parent_pk) - ] + indexed_actions['_vacated_pks'] = {parent_pk} indexed_actions['rename_external_id'] = [ RenameTagExternalId(taxonomy=self.taxonomy, tag=landing_tag, index=2, target_pk=parent_pk) ] @@ -195,18 +193,62 @@ def test_validate_parent_staged_away_accepted_when_landing_row_queued(self) -> N error = action._validate_parent(indexed_actions) # pylint: disable=protected-access self.assertIsNone(error) - def test_validate_parent_staged_away_rejected_when_landing_row_not_queued(self) -> None: + def test_validate_parent_vacated_rejected_when_landing_row_not_queued(self) -> None: """ Same setup as above, but no RenameTagExternalId row lands on id=tag_1 (e.g. it would appear later in file order, or doesn't exist): the parent reference must be cleanly rejected, not crash. + This is also the core regression this fix closes for a plain, + non-contended rename: tag_1 is vacated by some rename elsewhere in + the import, and nothing reuses "tag_1", so a reference to it must + not be accepted just because the tag still physically exists in the + database under that external_id at validate time. """ parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk - staged_tag = TagItem(id='tag_90', value='_', previous_id='tag_1', index=1) indexed_actions = dict(self.indexed_actions) - indexed_actions['stage_external_id'] = [ - StageTagExternalId(taxonomy=self.taxonomy, tag=staged_tag, index=1, target_pk=parent_pk) + indexed_actions['_vacated_pks'] = {parent_pk} + action = ImportAction( + self.taxonomy, + TagItem( + id='tag_110', + value='_', + parent_id='tag_1', + index=100, + ), + index=100, + ) + error = action._validate_parent(indexed_actions) # pylint: disable=protected-access + self.assertEqual( + str(error), + ( + "Action error in 'import_action' (#100): " + "Unknown parent tag (tag_1). " + "You need to add parent before the child in your file." + ) + ) + + def test_validate_parent_rejected_for_vacated_old_id(self) -> None: + """ + Regression: a parent_id referencing a tag's *old* external_id, when + that tag is being renamed away from it in this same import (a plain, + non-contended rename -- nothing reuses the old id), must be rejected + even though the tag still physically exists in the database under + that external_id at validate time: parent_id names the desired + end-state parent, not whichever tag currently resolves to that + external_id in the database. Paired with + test_validate_parent_with_rename_external_id_action, which confirms + the same rename's *new* id (tag_60) is accepted. + """ + tag_1_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk + indexed_actions = dict(self.indexed_actions) + indexed_actions['rename_external_id'] = [ + RenameTagExternalId( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_60', value='Tag 60', previous_id='tag_1', index=1), + index=1, + ) ] + indexed_actions['_vacated_pks'] = {tag_1_pk} action = ImportAction( self.taxonomy, TagItem( diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 09a964843..57348b20d 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -593,6 +593,35 @@ def test_import_swap_external_ids_with_colliding_values_rejected(self) -> None: assert tag_3.external_id == "tag_3" assert tag_3.value == "Tag 3" + def test_import_rename_referencing_stale_old_id_rejected(self) -> None: + """ + Regression: a plain (non-contended) rename of tag_1 to tag_50, with + a different row's parent_id referencing tag_1's OLD id, must be + rejected cleanly at the plan step -- not crash at execute time. The + rename row comes first in the file, so if the stale reference were + accepted, the second row's own execute() would raise an uncaught + Tag.DoesNotExist once the rename runs before it, since no tag would + hold external_id="tag_1" any more. + """ + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_60", "value": "Tag 60", "parent_id": "tag_1"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert not result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Unknown parent tag (tag_1)" in log + assert "Traceback" not in log + + assert self.taxonomy.tag_set.filter(external_id="tag_1").exists() + assert not self.taxonomy.tag_set.filter(external_id="tag_50").exists() + assert not self.taxonomy.tag_set.filter(external_id="tag_60").exists() + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index cf3333f40..d810133d7 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -528,6 +528,36 @@ def test_generate_actions_chain_stages_only_contended_tags(self) -> None: self.assertNotIn(tag_2_pk, staged_pks) self.assertEqual(len(self.import_plan.indexed_actions['rename_external_id']), 3) + def test_generate_actions_parent_id_stale_after_plain_rename_rejected(self) -> None: + """ + Regression: tag_1 is renamed to tag_50 in this same import, and + nothing reuses "tag_1" (a plain, non-contended rename, so tag_1 is + never staged). A different row's parent_id references the now-stale + old id "tag_1" -- this must be rejected, since after the import no + tag will hold that external_id at all. + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_60', value='Tag 60', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("Unknown parent tag (tag_1)", str(self.import_plan.errors[0])) + + def test_generate_actions_parent_id_new_id_after_earlier_rename_accepted(self) -> None: + """ + Same rename as above (tag_1 -> tag_50), but the other row's + parent_id references the *new* id "tag_50" instead of the stale old + one, and the rename row comes first in the file: this must be + accepted, same convention as referencing a newly-created parent. + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_60', value='Tag 60', parent_id='tag_50'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + def test_generate_actions_genuine_collision_not_staged(self) -> None: """ A rename targeting an id held by an unrelated tag that is not From f5ac773634c6571884b9709445b52415ab50affd Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 9 Sep 2026 00:50:17 +0400 Subject: [PATCH 8/9] test: cover CSV rename round-trip, replace-mode, and idempotent re-import Closes end-to-end coverage gaps against issue #673's acceptance criteria: existing rename round-trip tests only ran through JSON, the replace-mode path (the one Studio's import wizard always uses) was only checked at generate_actions() level, and previous_id == id had no end-to-end test. --- .../openedx_tagging/import_export/test_api.py | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 57348b20d..2580571f7 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -370,6 +370,107 @@ def test_import_rename_external_id_then_export(self) -> None: for tag in exported_tags: assert "previous_id" not in tag + def test_import_rename_external_id_preserves_pk_csv(self) -> None: + """ + Same as `test_import_rename_external_id_preserves_pk`, but through + the .csv format (see ADR 0010). + """ + old_pk = self.taxonomy.tag_set.get(external_id="tag_1").pk + + importFile = BytesIO("id,value,previous_id\ntag_50,Tag 1 Renamed,tag_1\n".encode()) + result, _task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + ParserFormat.CSV, + ) + assert result + + renamed_tag = Tag.objects.get(pk=old_pk) + assert renamed_tag.external_id == "tag_50" + assert renamed_tag.value == "Tag 1 Renamed" + assert not self.taxonomy.tag_set.filter(external_id="tag_1").exists() + + def test_import_rename_external_id_then_export_csv(self) -> None: + """ + Same as `test_import_rename_external_id_then_export`, but through + the .csv format: the follow-up export contains the new id, not the + old id, and its header row has no `previous_id` column at all, + since `previous_id` is import-only and never persisted (see ADR + 0010). + """ + importFile = BytesIO("id,value,previous_id\ntag_50,Tag 1 Renamed,tag_1\n".encode()) + result, _task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + ParserFormat.CSV, + ) + assert result + + output = import_export_api.export_tags(self.taxonomy, ParserFormat.CSV) + header = output.splitlines()[0].split(",") + assert "previous_id" not in header + + exported_ids = [line.split(",")[0] for line in output.splitlines()[1:]] + assert "tag_50" in exported_ids + assert "tag_1" not in exported_ids + + def test_import_rename_external_id_survives_replace_mode(self) -> None: + """ + Issue #673: the Studio taxonomy import wizard always runs with + replace=True (a full replace), so a rename must be verified through + that exact end-to-end path, not just at generate_actions() level (see + test_import_plan.TestTagImportPlan.test_generate_actions_rename_external_id_replace_skips_delete + for the plan-level check that the old id is excluded from the delete + sweep). + """ + old_pk = self.taxonomy.tag_set.get(external_id="tag_1").pk + + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_50", "value": "Tag 1 Renamed", "previous_id": "tag_1"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + replace=True, + ) + assert result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + + renamed_tag = Tag.objects.get(pk=old_pk) + assert renamed_tag.external_id == "tag_50" + assert renamed_tag.value == "Tag 1 Renamed" + assert not self.taxonomy.tag_set.filter(external_id="tag_1").exists() + + def test_import_rename_external_id_previous_id_equals_id_is_noop(self) -> None: + """ + Issue #673: previous_id equal to id is idempotent re-import support. + RenameTagExternalId.applies_for declines to fire in that case (see its + unit-level coverage in test_actions.py), and normal update/no-op + handling applies instead. This confirms the actual end-to-end + scenario: re-importing a tag with previous_id set to its own current + external_id succeeds with no error, and a follow-up export still shows + the same id. + """ + importFile = BytesIO(json.dumps({"tags": [ + {"id": "tag_1", "value": "Tag 1", "previous_id": "tag_1"}, + ]}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + ) + assert result + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Traceback" not in log + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_tags = json.loads(output).get("tags") + exported_ids = [tag.get("id") for tag in exported_tags] + assert "tag_1" in exported_ids + def test_import_rename_external_id_unmatched_previous_id_rejected(self) -> None: importFile = BytesIO(json.dumps({"tags": [ {"id": "tag_50", "value": "Tag 50", "previous_id": "tag_999"}, From 7b40387457677d749583eab176d1942d0fdd4ee1 Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Wed, 9 Sep 2026 01:00:46 +0400 Subject: [PATCH 9/9] fix: types --- tests/openedx_tagging/import_export/test_actions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index 6d7108e55..63c561cb1 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -175,7 +175,7 @@ def test_validate_parent_vacated_accepted_when_landing_row_queued(self) -> None: """ parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk landing_tag = TagItem(id='tag_1', value='_', previous_id='tag_2', index=2) - indexed_actions = dict(self.indexed_actions) + indexed_actions: dict[str, list[ImportAction] | set[int]] = dict(self.indexed_actions) indexed_actions['_vacated_pks'] = {parent_pk} indexed_actions['rename_external_id'] = [ RenameTagExternalId(taxonomy=self.taxonomy, tag=landing_tag, index=2, target_pk=parent_pk) @@ -205,7 +205,7 @@ def test_validate_parent_vacated_rejected_when_landing_row_not_queued(self) -> N database under that external_id at validate time. """ parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk - indexed_actions = dict(self.indexed_actions) + indexed_actions: dict[str, list[ImportAction] | set[int]] = dict(self.indexed_actions) indexed_actions['_vacated_pks'] = {parent_pk} action = ImportAction( self.taxonomy, @@ -240,7 +240,7 @@ def test_validate_parent_rejected_for_vacated_old_id(self) -> None: the same rename's *new* id (tag_60) is accepted. """ tag_1_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk - indexed_actions = dict(self.indexed_actions) + indexed_actions: dict[str, list[ImportAction] | set[int]] = dict(self.indexed_actions) indexed_actions['rename_external_id'] = [ RenameTagExternalId( taxonomy=self.taxonomy,