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 a501c9e30..23ec59a3b 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -3,11 +3,17 @@ """ from __future__ import annotations +from typing import TYPE_CHECKING +from uuid import uuid4 + from django.utils.translation import gettext as _ from ..models import Tag, Taxonomy from .exceptions import ImportActionConflict, ImportActionError +if TYPE_CHECKING: + from .import_plan import TagItem + class ImportAction: """ @@ -35,10 +41,11 @@ class ImportAction: name = "import_action" - def __init__(self, taxonomy: Taxonomy, tag, index: int): + def __init__(self, taxonomy: Taxonomy, tag: TagItem, 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)) @@ -47,7 +54,7 @@ def __str__(self) -> str: return self.__repr__() @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, 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` @@ -84,7 +91,7 @@ def _search_action( indexed_actions: dict, action_name: str, attr: str, - search_value: str, + search_value: str | None, ): """ Use this function to find and action using an `attr` of `TagItem` @@ -98,15 +105,25 @@ 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 match a tag's *end-state* external_id, not one + that's being renamed away in this same import (see `_vacated_pks`): + a stale match falls through to the same "created or renamed-in + earlier in this import" check used for a brand-new parent. """ 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) + if parent_tag.pk in indexed_actions.get("_vacated_pks", set()): + raise Tag.DoesNotExist 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 +174,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, @@ -166,6 +192,28 @@ def _validate_value(self, indexed_actions) -> ImportActionError | None: return None + @classmethod + def _resolve_update_target(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> Tag | None: + """ + Resolve the tag that RenameTag/UpdateParentTag would update by `id`, + or None if this row isn't theirs to handle: a rename_external_id row + (previous_id set and different from id, where `id` may belong to a + different tag entirely), no match, or a tag already queued for + deletion in this same import. + """ + if tag.previous_id and tag.id != tag.previous_id: + return None + try: + taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) + except Tag.DoesNotExist: + return None + if indexed_actions and any( + taxonomy_tag.external_id == action.tag.id + for action in indexed_actions.get("delete", []) + ): + return None + return taxonomy_tag + class CreateTag(ImportAction): """ @@ -193,10 +241,12 @@ def __str__(self) -> str: ) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> 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 @@ -277,18 +327,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: TagItem, 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. + + See ImportAction._resolve_update_target for when this doesn't apply + regardless of the parent change (queued for deletion, or a + rename_external_id row). """ - try: - taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) - return ( - taxonomy_tag.parent is not None - and taxonomy_tag.parent.external_id != tag.parent_id - ) or (taxonomy_tag.parent is None and tag.parent_id is not None) - except Tag.DoesNotExist: + taxonomy_tag = cls._resolve_update_target(taxonomy, tag, indexed_actions) + if taxonomy_tag is None: return False + return ( + taxonomy_tag.parent is not None + and taxonomy_tag.parent.external_id != tag.parent_id + ) or (taxonomy_tag.parent is None and tag.parent_id is not None) def validate(self, indexed_actions) -> list[ImportActionError]: """ @@ -339,15 +392,18 @@ def __str__(self) -> str: return str(description_str) @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, 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. + + See ImportAction._resolve_update_target for when this doesn't apply + regardless of the value change (queued for deletion, or a + rename_external_id row). """ - try: - taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) - return taxonomy_tag.value != tag.value - except Tag.DoesNotExist: + taxonomy_tag = cls._resolve_update_target(taxonomy, tag, indexed_actions) + if taxonomy_tag is None: return False + return taxonomy_tag.value != tag.value def validate(self, indexed_actions) -> list[ImportActionError]: """ @@ -371,6 +427,189 @@ def execute(self) -> None: taxonomy_tag.save() +class RenameTagExternalId(ImportAction): + """ + Action to rename an existing tag's external_id in place. + + Applies when a row's `previous_id` matches an existing tag and `id` + differs from it. Preserves the tag's primary key and associations, + 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 another existing tag or action. + - Value duplicates, only 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: TagItem, indexed_actions=None) -> 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 another existing tag or a + prior create/rename action in this import. A tag already slated for + delete or staged onto a placeholder id doesn't count as a collision, + since both execute before this action (see _build_delete_actions, + StageTagExternalIdForSwap). + """ + is_freed_by_delete = any( + self.tag.id == action.tag.id + for action in indexed_actions["delete"] + ) if "delete" in indexed_actions else False + + existing = self.taxonomy.tag_set.filter(external_id=self.tag.id).first() + is_awaiting_placeholder_swap = 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_awaiting_placeholder_swap and existing is not None: + 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."), + ) + + 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]: + """ + 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 the tag's external_id in place, and updates its value and parent. + + Looks up the target by pk, not by `previous_id`: a + StageTagExternalIdForSwap action may have already moved it onto a + placeholder id by execution time (see _build_staging_actions). + """ + # validate() rejects an unmatched previous_id, and execute() never + # runs when errors are present, so target_pk is always set here. + 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 = ( + self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + if self.tag.parent_id else None + ) + target.save() + + +class StageTagExternalIdForSwap(ImportAction): + """ + Action to move a tag off a contended external_id before another row in + this import claims it. + + Synthesized by TagImportPlan._build_staging_actions, not read from a + file row, when a tag's current external_id is another row's rename + target. Needed for a swap or N-cycle of renames: (taxonomy, + external_id) is a DB-level unique constraint enforced per-statement on + every backend this project runs on, so nothing else gives such renames + a valid execution order. + """ + + 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: TagItem, 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. + """ + # _build_staging_actions only builds this action with a resolved + # target_pk, so it's never None here. + 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 @@ -386,7 +625,7 @@ def __str__(self) -> str: name = "delete" @classmethod - def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> bool: """ This action is an exception. These actions are created in `TagImportPlan.generate_actions` if `replace=True` @@ -423,7 +662,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: TagItem, indexed_actions=None) -> bool: """ No validations necessary """ @@ -445,7 +684,9 @@ def execute(self) -> None: available_actions = [ UpdateParentTag, RenameTag, + RenameTagExternalId, CreateTag, + StageTagExternalIdForSwap, DeleteTag, WithoutChanges, ] diff --git a/src/openedx_tagging/import_export/exceptions.py b/src/openedx_tagging/import_export/exceptions.py index 444af038c..5c3eddabc 100644 --- a/src/openedx_tagging/import_export/exceptions.py +++ b/src/openedx_tagging/import_export/exceptions.py @@ -49,6 +49,24 @@ def __init__(self, action: ImportAction, message: str, **kargs): ).format(name=action.name, index=action.index, message=message) +class DuplicateFinalIdError(TagImportError): + """ + Exception raised when two or more rows in the same import claim the + same final id, so it's ambiguous which row's changes should land on + that tag. + """ + + def __init__(self, tag_id: str, row_indexes: list[int], **kargs): + super().__init__(**kargs) + self.message = _( + "Duplicate id ({tag_id}): rows {row_indexes} all claim it as " + "their final id. Each row's id must be unique within a single import." + ).format( + tag_id=tag_id, + row_indexes=", ".join(f"#{index}" for index in row_indexes), + ) + + class ImportActionConflict(ImportActionError): """ Exception used when exists a conflict between actions diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index 6502c2c1b..f851139d5 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -7,8 +7,16 @@ from django.db import transaction from ..models import Tag, TagImportTask, Taxonomy -from .actions import DeleteTag, ImportAction, UpdateParentTag, WithoutChanges, available_actions -from .exceptions import ImportActionError +from .actions import ( + DeleteTag, + ImportAction, + RenameTagExternalId, + StageTagExternalIdForSwap, + UpdateParentTag, + WithoutChanges, + available_actions, +) +from .exceptions import DuplicateFinalIdError, TagImportError @define @@ -21,6 +29,7 @@ class TagItem: value: str index: int | None = 0 parent_id: str | None = None + previous_id: str | None = None def __str__(self): """ @@ -37,7 +46,7 @@ class TagImportPlan: """ actions: list[ImportAction] - errors: list[ImportActionError] + errors: list[TagImportError] indexed_actions: dict actions_dict: dict taxonomy: Taxonomy @@ -57,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)) @@ -133,6 +142,67 @@ 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 _validate_no_duplicate_final_ids(self, tags: list[TagItem]) -> None: + """ + Reject two or more rows in the same import that claim the same + final `id`, before staging or per-row action-building runs. This + also catches a row colliding with a staged tag's target, since a + tag is only ever staged because its external_id is already another + row's target id. + + Without this, the outcome depended on row order: a silent + overwrite of whichever row landed first, or an uncaught crash at + execute time. + + Rows are identified by their 1-based position in `tags`, not + `TagItem.index`: index is parser-assigned and optional, and left + at its default for hand-built rows (e.g. in tests), while position + is always defined. + """ + positions_by_id: dict[str, list[int]] = {} + for position, tag in enumerate(tags, start=1): + positions_by_id.setdefault(tag.id, []).append(position) + + for tag_id, positions in positions_by_id.items(): + if len(positions) > 1: + self.errors.append(DuplicateFinalIdError(tag_id, positions)) + + def _build_staging_actions(self, tags: list[TagItem]) -> None: + """ + Stage any tag whose current external_id is another rename row's + target in this import, so renames never collide on external_id + regardless of file order (see StageTagExternalIdForSwap). + + Also records every rename's resolved target pk in + indexed_actions["_vacated_pks"], staged or not: once a tag is being + renamed away from an external_id, that id is stale for anyone still + referencing it via the live database (see _validate_parent), even + if nothing in this import reuses it. + """ + 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 + target_pk = self._resolve_rename_target_pk(tag) + if target_pk is None: + continue + vacated_pks.add(target_pk) + if tag.previous_id in target_ids: + self._build_action(StageTagExternalIdForSwap, tag, target_pk=target_pk) + self.indexed_actions["_vacated_pks"] = vacated_pks + def generate_actions( self, tags: list[TagItem], @@ -152,6 +222,12 @@ def generate_actions( self.actions.clear() self.errors.clear() self._init_indexed_actions() + + # Reject two or more rows claiming the same final id outright, + # before staging or per-row action-building runs (see + # _validate_no_duplicate_final_ids). + self._validate_no_duplicate_final_ids(tags) + tags_for_delete = {} if replace: @@ -160,19 +236,35 @@ 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) # 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._build_action(action_cls, tag) + if action_cls.applies_for(self.taxonomy, tag, self.indexed_actions): + 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/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..8ca6899ba 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -12,6 +12,8 @@ DeleteTag, ImportAction, RenameTag, + RenameTagExternalId, + StageTagExternalIdForSwap, UpdateParentTag, WithoutChanges, ) @@ -52,7 +54,8 @@ def setUp(self) -> None: # Note: we must specify '-> None' to opt in to type ch ), index=1, ) - ] + ], + 'rename_external_id': [], } @@ -133,6 +136,132 @@ 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) + + def test_validate_parent_vacated_accepted_when_landing_row_queued(self) -> None: + """ + tag_1 is vacated (another row's rename target resolves to it) but + also landed on by a different row in this same import. A parent_id + referencing tag_1 must validate, since a tag will hold that id + again after the import -- same convention as referencing a + newly-created tag. + """ + 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[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) + ] + 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_vacated_rejected_when_landing_row_not_queued(self) -> None: + """ + Same setup as above, but nothing lands on id=tag_1 in this import: + the core regression this fix closes. tag_1 is vacated by a rename + elsewhere and nothing reuses it, so a parent_id referencing it must + be rejected cleanly, not accepted just because the tag still + physically exists in the database at validate time. + """ + parent_pk = self.taxonomy.tag_set.get(external_id='tag_1').pk + indexed_actions: dict[str, list[ImportAction] | set[int]] = dict(self.indexed_actions) + 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, + while that tag is being renamed away from it (plain, non-contended + -- nothing reuses the old id), must be rejected: parent_id names + the desired end-state parent, not whatever currently resolves to + that external_id. 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[str, list[ImportAction] | set[int]] = 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( + 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', @@ -174,6 +303,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 +355,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), @@ -387,6 +562,48 @@ 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) + + 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 @@ -457,6 +674,46 @@ 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) + + 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 @@ -496,13 +753,296 @@ 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_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_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'] = [ + StageTagExternalIdForSwap( + 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). + 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_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. + 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, + target_pk=pk, + ) + 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 TestStageTagExternalIdForSwap(TestImportActionMixin, TestCase): + """ + Test for 'stage_external_id' action + """ + + def test_applies_for(self) -> None: + result = StageTagExternalIdForSwap.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 = StageTagExternalIdForSwap( + 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 = StageTagExternalIdForSwap( + 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 """ def test_applies_for(self) -> None: - assert not DeleteTag.applies_for(self.taxonomy, None) + assert not DeleteTag.applies_for(self.taxonomy, TagItem(id='_', value='_')) def test_validate(self) -> None: action = DeleteTag( diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index bdb04a86a..e75ce20d0 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -324,6 +324,462 @@ 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_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 via .csv: + the export contains the new id, not the old one, and its header + has no `previous_id` column, since it's 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: + """ + Studio's taxonomy import wizard always runs with replace=True, so + this must be verified end-to-end, 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: + """ + previous_id equal to id is a no-op: RenameTagExternalId.applies_for + declines to fire (see test_actions.py), and normal update handling + applies instead. End-to-end: re-importing a tag with previous_id + set to its own external_id succeeds, and 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"}, + ]}).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_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_rename_external_id_reuses_id_freed_by_replace_delete(self) -> None: + """ + Replace-mode import that omits tag_1 (queuing it for deletion) and + renames tag_2 onto id="tag_1" in the same file, reusing the id + tag_1's deletion is about to free. Must succeed end-to-end: tag_1 + still being physically present (though queued for deletion) at + validate time must not count as a collision. + + The new value and parent_id deliberately differ from tag_1's + current ones: with matching values, RenameTag/UpdateParentTag's + DB-only lookups would fire instead and pass even without this fix, + proving nothing. tag_3 gets its own no-op row so it survives as a + valid parent target, rather than being swept up by the same 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": "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, + importFile, + self.parser_format, + replace=True, + ) + 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, + # 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 == "Renamed From Tag 2" + 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, both root tags so parent handling + doesn't complicate the assertions) has no valid plain execution + order: either rename collides with the DB's per-statement 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 takes the other + tag's value: must cleanly reject, not raise an IntegrityError. + Value swaps are an explicit, documented limitation (see ADR 0010 + amendment): (taxonomy, value) has the same 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_duplicate_final_id_rejected(self) -> None: + """ + Two rows can't claim the same final id: a tag_1<->tag_3 swap plus + an unrelated third row also targeting id=tag_1 is ambiguous. Must + reject outright at the plan step, not resolve by row order + (previously: a silent overwrite or an uncaught crash, depending on + which row came first). + """ + 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"}, + {"id": "tag_1", "value": "Something Else Entirely"}, + ]}).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 "Duplicate id" 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_duplicate_final_id_rejected_regardless_of_order(self) -> None: + """ + Same collision as test_import_duplicate_final_id_rejected, with + the unrelated row moved to the front: rejection must not depend on + row order. Previously this ordering hit an uncaught + Tag.DoesNotExist crash at execute time instead of a clean + plan-time rejection. + """ + 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_1", "value": "Something Else Entirely"}, + {"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 "Duplicate id" in log + assert not result + + 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_rename_referencing_stale_old_id_rejected(self) -> None: + """ + Regression: a plain rename of tag_1 to tag_50, with a different + row's parent_id referencing tag_1's OLD id, must be rejected at + the plan step, not crash at execute time. The rename comes first + in the file, so an accepted stale reference would hit an uncaught + Tag.DoesNotExist once no tag holds "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 88f24a8b2..3d177e373 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -149,7 +149,7 @@ def test_build_delete_actions(self) -> None: }, ], False, - 3, + 4, [ { 'name': 'create', @@ -260,6 +260,8 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "#7: Rename tag value of (tag_2 / Tag 2) to 'Tag 31'\n" "\nOutput errors\n" "--------------------------------\n" + "Duplicate id (tag_31): rows #1, #2 all claim it as their final id. " + "Each row's id must be unique within a single import.\n" "Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n" "Action error in 'rename' (#3): Duplicated tag value with tag in database (external_id=tag_2).\n" "Action error in 'update_parent' (#4): Unknown parent tag (tag_100). " @@ -407,6 +409,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 = [ @@ -423,3 +467,144 @@ 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, both root tags) 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 first. + """ + 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 tag_2 -> tag_3 -> tag_4 -> tag_90 (fresh, uncontended). + Only tag_3 and tag_4 are contended (their current id is another + row's target); tag_2's current id is nobody's target, so it isn't + 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_parent_id_stale_after_plain_rename_rejected(self) -> None: + """ + Regression: tag_1 is renamed to tag_50 and nothing reuses "tag_1" + (a plain, non-contended rename, never staged). A different row's + parent_id references the now-stale "tag_1" -- must be rejected, + since no tag will hold that external_id after the import. + """ + 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 + 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])) + + def test_generate_actions_rejects_duplicate_final_id(self) -> None: + """ + Two rows can't claim the same final id: a tag_1<->tag_3 swap plus + an unrelated third row also targeting id=tag_1 is ambiguous. + Reject the whole import outright, naming both offending rows by + position, instead of letting swap-staging silently treat the + collision as valid (see DuplicateFinalIdError). + """ + tags = [ + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_1', value='Something Else Entirely'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + error = str(self.import_plan.errors[0]) + self.assertIn("tag_1", error) + self.assertIn("#2", error) + self.assertIn("#3", error) + + def test_generate_actions_rejects_duplicate_final_id_regardless_of_order(self) -> None: + """ + Same collision as test_generate_actions_rejects_duplicate_final_id, + but with the unrelated row moved to the front of the file: the + rejection doesn't depend on row order. + """ + tags = [ + TagItem(id='tag_1', value='Something Else Entirely'), + 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(len(self.import_plan.errors), 1) + error = str(self.import_plan.errors[0]) + self.assertIn("tag_1", error) + self.assertIn("#1", error) + self.assertIn("#3", error) 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(",")