diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index 96aced3f1..68f76ccdb 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -105,51 +105,54 @@ def _search_action( def _validate_parent(self, indexed_actions) -> ImportActionError | None: """ - Validate that `parent_id` names a tag that will exist once this import finishes. - - A match against a tag being renamed away in this same import (see - `_vacated_pks`) doesn't count as the real parent: it falls through to the - same "created or renamed-in this import" check as a brand-new parent. If - nothing lands on that vacated id either, the error names the id it was - renamed to, instead of the generic "unknown parent" message for a parent - never mentioned at all. + Validate that `parent_id` names the tag that will hold that id once this import finishes. + + That is the tag currently holding `parent_id`, unless this import renames it + away (see `_vacated_pks`). A renamed-away id, like a brand-new one, must be + given to a tag by a row of this file: a rename onto it (`_vacated_pks` maps + every renamed tag to its new id, whatever the row order) or a create action + built before this one (TagImportPlan builds a new parent before its + children). Even then it must not be this row's own current parent: a row + that names its parent's pre-import id while another row takes that id (a + swap) could mean "stay with my parent" or "move under the tag now called + `parent_id`", so it is rejected. The error for a renamed-away id names the + id it was renamed to, instead of the generic "unknown parent" message for + an id never mentioned at all. """ vacated_pks: dict[int, str] = indexed_actions.get("_vacated_pks", {}) - parent_tag = None - is_vacated = False - try: - # Validates that the parent exists on the taxonomy - parent_tag = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) - if parent_tag.pk in vacated_pks: - is_vacated = True - raise Tag.DoesNotExist - except Tag.DoesNotExist: - # 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, casefold=True - ) or self._search_action( - indexed_actions, RenameTagExternalId.name, "id", self.tag.parent_id, casefold=True + parent_tag = self.taxonomy.tag_set.filter(external_id=self.tag.parent_id).first() + new_id = vacated_pks.get(parent_tag.pk) if parent_tag is not None else None + if parent_tag is not None and new_id is None: + return None # The parent exists and keeps its id. + + found = (self.tag.parent_id or "").casefold() in { + renamed_to.casefold() for renamed_to in vacated_pks.values() + } or self._search_action(indexed_actions, CreateTag.name, "id", self.tag.parent_id, casefold=True) + if found and not (parent_tag is not None and self._is_current_parent(parent_tag)): + return None + if new_id is not None: + return ImportActionError( + action=self, + message=_( + "Parent tag {parent_id} is renamed to {new_id} in this file; " + "use {new_id} as the parent_id." + ).format(parent_id=self.tag.parent_id, new_id=new_id), ) - if not found: - if is_vacated: - # is_vacated is only set after parent_tag was assigned above. - assert parent_tag is not None - new_id = vacated_pks[parent_tag.pk] - return ImportActionError( - action=self, - message=_( - "Parent tag {parent_id} is renamed to {new_id} in this file; " - "use {new_id} as the parent_id." - ).format(parent_id=self.tag.parent_id, new_id=new_id), - ) - return ImportActionError( - action=self, - message=_( - "Unknown parent tag ({parent_id}). " - "You need to add parent before the child in your file." - ).format(parent_id=self.tag.parent_id), - ) - return None + return ImportActionError( + action=self, + message=_( + "Unknown parent tag ({parent_id}). " + "Add a row for the parent to your file." + ).format(parent_id=self.tag.parent_id), + ) + + def _is_current_parent(self, parent_tag: Tag) -> bool: + """ + Whether `parent_tag` is the parent that this row's tag has before the import. + """ + return self.taxonomy.tag_set.filter( + external_id=self.tag.previous_id or self.tag.id, parent=parent_tag, + ).exists() def _validate_value(self, indexed_actions) -> ImportActionError | None: """ @@ -326,33 +329,49 @@ class UpdateParentTag(ImportAction): """ name = "update_parent" + _description: str | None = None def __str__(self) -> str: - taxonomy_tag = self._get_tag() - - description_str = _("Update the parent of {tag} from parent {old_parent} to {new_parent}").format( - tag=taxonomy_tag.display_str(), - old_parent=taxonomy_tag.parent.display_str() if taxonomy_tag.parent else None, - new_parent=self.tag.parent_id, - ) - - return str(description_str) + # Describe the change as of the plan: the plan is logged before it runs, and the + # execution detaches every moved tag first, which would then read as "from None". + if self._description is None: + taxonomy_tag = self._get_tag() + self._description = str( + _("Update the parent of {tag} from parent {old_parent} to {new_parent}").format( + tag=taxonomy_tag.display_str(), + old_parent=taxonomy_tag.parent.display_str() if taxonomy_tag.parent else None, + new_parent=self.tag.parent_id, + ) + ) + return self._description @classmethod def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> bool: """ This action applies whenever there is a change on the parent. + A row that names its parent's current id is still a parent change when + this import gives that id to another tag: by renaming the parent away + from it, or by renaming another tag onto it once the parent is deleted. + Such a row used to count as "no change", so a stale id got through + `_validate_parent` and a reclaimed id lost the child to the delete + phase's promotion. + See ImportAction._resolve_update_target for when this doesn't apply regardless of the parent change (a rename_external_id row). """ taxonomy_tag = cls._resolve_update_target(taxonomy, tag) 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) + parent = taxonomy_tag.parent + if parent is None: + return tag.parent_id is not None + if parent.external_id != tag.parent_id: + return True + vacated_pks: dict[int, str] = indexed_actions.get("_vacated_pks", {}) if indexed_actions else {} + return parent.pk in vacated_pks or parent.external_id.casefold() in { + new_id.casefold() for new_id in vacated_pks.values() + } def validate(self, indexed_actions) -> list[ImportActionError]: """ @@ -641,6 +660,56 @@ def execute(self) -> None: self.taxonomy.tag_set.filter(pk=self.target_pk).update(external_id=placeholder) +class DetachTag(ImportAction): + """ + Action to move a tag to the root before the actions that change parents run. + + Synthesized by TagImportPlan._build_detach_actions for every existing tag + whose parent changes, not read from a file row. Tag.save checks the depth + of each step, and a cycle corrupts the tree silently, so a restructure that + is valid as a whole can still go wrong part-way through, depending on the + order of the actions. Once every moved tag is at the root, each tag is at + or above its final depth and none is an ancestor of its new parent, so the + actions that follow can attach tags in any order. + """ + + name = "detach" + + def __str__(self) -> str: + return str( + _( + "Move tag {tag} to the root first, so that the parent changes " + "in this file can run in any order." + ).format(tag=self.tag) + ) + + @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 the root, with its descendants. + + Looks the tag up by pk: a StageTagExternalIdForSwap action may have + moved it onto a placeholder id by now. + """ + # _build_detach_actions only builds this action with a target_pk. + assert self.target_pk is not None + tag = self.taxonomy.tag_set.get(pk=self.target_pk) + tag.parent = None + tag.save() + + class DeleteTag(ImportAction): """ Action for delete a Tag @@ -718,6 +787,7 @@ def execute(self) -> None: RenameTagExternalId, CreateTag, StageTagExternalIdForSwap, + DetachTag, DeleteTag, WithoutChanges, ] diff --git a/src/openedx_tagging/import_export/exceptions.py b/src/openedx_tagging/import_export/exceptions.py index 1f1caf1f6..d248f0006 100644 --- a/src/openedx_tagging/import_export/exceptions.py +++ b/src/openedx_tagging/import_export/exceptions.py @@ -80,6 +80,33 @@ def __init__(self, tag_id: str, plain_row: int, rename_row: int, **kargs): ).format(tag_id=tag_id, plain_row=plain_row, rename_row=rename_row) +class ParentCycleError(TagImportError): + """ + Exception raised when a row's parent_id would make its tag its own ancestor. + + ``row`` is the 1-based position of the row among the file's data rows, as + in DuplicateFinalIdError. + """ + + def __init__(self, row: int, parent_id: str, **kargs): + super().__init__(**kargs) + self.message = _( + "Row {row}'s parent_id ({parent_id}) creates a parent cycle." + ).format(row=row, parent_id=parent_id) + + +class TagTooDeepError(TagImportError): + """ + Exception raised when the import would leave a tag deeper than the taxonomy allows. + """ + + def __init__(self, tag_id: str, max_levels: int, **kargs): + super().__init__(**kargs) + self.message = _( + "This import would leave tag {tag_id} more than {max_levels} levels deep." + ).format(tag_id=tag_id, max_levels=max_levels) + + 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 b39cbddda..2b01ab959 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -3,12 +3,16 @@ """ from __future__ import annotations +from typing import NamedTuple + from attrs import define from django.db import transaction from ..models import Tag, TagImportTask, Taxonomy +from ..models.base import TAXONOMY_MAX_DEPTH from .actions import ( DeleteTag, + DetachTag, ImportAction, RenameTagExternalId, StageTagExternalIdForSwap, @@ -16,7 +20,28 @@ WithoutChanges, available_actions, ) -from .exceptions import DuplicateFinalIdError, StaleIdTargetedByPlainRowError, TagImportError +from .exceptions import ( + DuplicateFinalIdError, + ParentCycleError, + StaleIdTargetedByPlainRowError, + TagImportError, + TagTooDeepError, +) + + +class ExistingTag(NamedTuple): + """ + What the plan needs to know about a tag that exists before the import. + """ + + external_id: str + value: str + pk: int + parent_pk: int | None + + +# Stands in for the pk of a tag the import creates. Never equal to a real pk or to None. +NEW_TAG = -1 @define @@ -43,6 +68,22 @@ def __str__(self): class TagImportPlan: """ Class with functions to build an import plan and excute the plan + + A row's ``parent_id`` names the id its parent holds *after* the import. Three steps + make parent changes work whatever the order of the rows in the file: + + - The plan computes the tag tree as it will be after the import (``_end_state``) + and rejects a parent cycle or a tree deeper than the limit there. + - A DetachTag action moves every existing tag whose parent changes to the root + before the row actions (``_build_detach_actions``). After that each tag is at or + above its final depth, and none is an ancestor of its new parent, so no action can + exceed the depth limit or build a cycle part-way through. + - A row is built after the row that creates or renames-in its parent + (``_order_rows_parent_first``), so the per-action parent validation and the + execution both find the parent. + + "Row N" in an error is the N-th data row of the file, counting from 1: in a CSV file + that is spreadsheet line N + 1, because the header is line 1. """ actions: list[ImportAction] @@ -112,6 +153,9 @@ def _get_tag_id(self, tag: Tag) -> str: def _build_delete_actions(self, tags: dict): """ Adds delete actions for `tags` + + Every kept child of a deleted tag is first promoted to the root, before + any delete, because a delete cascades to the descendants. """ for tag in tags.values(): for child in tag.children.all(): @@ -129,6 +173,7 @@ def _build_delete_actions(self, tags: dict): ), ) + for tag in tags.values(): # Delete action self._build_action( DeleteTag, @@ -207,7 +252,8 @@ def _build_staging_actions(self, tags: list[TagItem]) -> None: or not: once a tag is renamed away from an external_id, that id is stale for anyone still referencing it via the live database, even if nothing in this import reuses it (see _validate_parent, which uses the recorded - new id to name it). + new id to name it). A case-only rename is neither staged nor recorded: + external_id is case-insensitive, so the tag keeps its id. """ target_ids = { tag.id.casefold() for tag in tags @@ -222,12 +268,170 @@ def _build_staging_actions(self, tags: list[TagItem]) -> None: target_pk = self._resolve_rename_target_pk(tag) if target_pk is None: continue + if tag.previous_id.casefold() == tag.id.casefold(): + continue # A case-only rename keeps the id: nothing is vacated or contended. vacated_pks[target_pk] = tag.id - is_case_only_rename = tag.previous_id.casefold() == tag.id.casefold() - if not is_case_only_rename and tag.previous_id.casefold() in target_ids: + if tag.previous_id.casefold() in target_ids: self._build_action(StageTagExternalIdForSwap, tag, target_pk=target_pk) self.indexed_actions["_vacated_pks"] = vacated_pks + def _existing_tags(self) -> dict[str, ExistingTag]: + """ + Return the taxonomy's tags before the import, keyed by casefolded external_id. One query. + """ + return { + external_id.casefold(): ExistingTag(external_id, value, pk, parent_pk) + for external_id, value, pk, parent_pk in self.taxonomy.tag_set.values_list( + "external_id", "value", "pk", "parent_id", + ) + } + + def _end_state( + self, tags: list[TagItem], existing: dict[str, ExistingTag], + ) -> tuple[dict[str, str | None], dict[str, int], dict[str, int]]: + """ + Compute the tag tree as it will be once the import is applied. + + Returns three dicts keyed by casefolded final id: ``parent_of`` (the parent's + final id, or None for a root), ``pk_of`` (the pk of the tag that will hold the + id, or NEW_TAG) and ``row_of`` (the position in ``tags`` of the id's row, for + the ids that have one). Each row contributes its own id and parent_id. An + existing tag without a row is either deleted or keeps its parent, named by + that parent's final id. Must run after the delete and staging actions are + built. + """ + vacated_pks: dict[int, str] = self.indexed_actions["_vacated_pks"] + deleted = {action.tag.id.casefold() for action in self.indexed_actions["delete"]} + key_of_pk = {info.pk: key for key, info in existing.items()} + parent_of: dict[str, str | None] = {} + pk_of: dict[str, int] = {} + row_of: dict[str, int] = {} + for position, tag in enumerate(tags): + key = tag.id.casefold() + if key in row_of: + continue # Rejected by _validate_no_duplicate_final_ids. + current = existing.get((tag.previous_id or tag.id).casefold()) + row_of[key] = position + pk_of[key] = current.pk if current is not None else NEW_TAG + parent_of[key] = tag.parent_id.casefold() if tag.parent_id else None + in_file = {key for tag in tags for key in (tag.id.casefold(), (tag.previous_id or "").casefold())} + for key, info in existing.items(): + if key in in_file or key in deleted: + continue + pk_of[key] = info.pk + parent_of[key] = None + if info.parent_pk is not None: + parent_of[key] = vacated_pks.get(info.parent_pk, key_of_pk[info.parent_pk]).casefold() + return parent_of, pk_of, row_of + + def _validate_hierarchy( + self, + tags: list[TagItem], + existing: dict[str, ExistingTag], + parent_of: dict[str, str | None], + pk_of: dict[str, int], + row_of: dict[str, int], + ) -> None: + """ + Reject a parent cycle, and a tree deeper than the taxonomy allows. + + A cycle is reported once, on its first row in the file. A cycle made only of + new tags is left to ``_validate_parent``, which reports one of them as an + unknown parent. A parent_id that is no tag of the end state counts as a root + here: it is unknown, which ``_validate_parent`` reports, or deleted without a + row reclaiming it, and the delete phase promotes its children. + """ + depth_of: dict[str, int] = {} + unresolved: set[str] = set() # on a cycle, or below one + for start in parent_of: + path: list[str] = [] + node: str | None = start + while node is not None and node not in depth_of and node not in unresolved and node not in path: + path.append(node) + node = parent_of.get(node) + if node not in parent_of: + node = None + if node in path: + assert node is not None + cycle = path[path.index(node):] + rows = [row_of[key] for key in cycle if key in row_of] + if rows and any(pk_of[key] != NEW_TAG for key in cycle): + position = min(rows) + self.errors.append(ParentCycleError(position + 1, tags[position].parent_id or "")) + if node is None or node in depth_of: + depth = depth_of[node] if node is not None else -1 + for key in reversed(path): + depth += 1 + depth_of[key] = depth + else: + unresolved.update(path) + + too_deep = [key for key, depth in depth_of.items() if depth > TAXONOMY_MAX_DEPTH] + if too_deep: + # Name the shallowest too-deep tag: the first one past the limit. + key = min(too_deep, key=lambda key: (depth_of[key], row_of.get(key, len(tags)), key)) + tag_id = tags[row_of[key]].id if key in row_of else existing[key].external_id + self.errors.append(TagTooDeepError(tag_id, TAXONOMY_MAX_DEPTH + 1)) + + def _build_detach_actions( + self, existing: dict[str, ExistingTag], parent_of: dict[str, str | None], pk_of: dict[str, int], + ) -> None: + """ + Add a DetachTag action for every existing tag whose parent after the import + is a different tag than before. + + A root tag has nothing to detach, a kept child of a deleted tag is promoted + by the delete phase, and a row whose parent_id names no tag of the end state + is left to ``_validate_parent``. + """ + by_pk = {info.pk: info for info in existing.values()} + deleted_pks = {existing[action.tag.id.casefold()].pk for action in self.indexed_actions["delete"]} + for key, pk in pk_of.items(): + if pk == NEW_TAG: + continue + info = by_pk[pk] + if info.parent_pk is None or info.parent_pk in deleted_pks: + continue + parent_key = parent_of[key] + if parent_key is not None and parent_key not in pk_of: + continue + final_parent_pk = pk_of[parent_key] if parent_key is not None else None + if final_parent_pk != info.parent_pk: + self._build_action(DetachTag, TagItem(id=info.external_id, value=info.value), target_pk=pk) + + @staticmethod + def _order_rows_parent_first(tags: list[TagItem], existing: dict[str, ExistingTag]) -> list[TagItem]: + """ + Return ``tags`` with each row after the row that creates or renames-in its parent, if any. + + The per-action parent validation and the execution look for a parent among the + existing tags and the actions built so far, so this makes both independent of + the row order. Rows keep their file order otherwise: a row is pulled forward only + when a later row needs it, and a file whose parents already come before their + children is left as it is. + """ + defining_row: dict[str, int] = {} + for position, tag in enumerate(tags): + is_rename = bool(tag.previous_id) and tag.id != tag.previous_id + if is_rename or tag.id.casefold() not in existing: + defining_row.setdefault(tag.id.casefold(), position) + ordered: list[TagItem] = [] + emitted: set[int] = set() + for start in range(len(tags)): + chain: list[int] = [] + position = start + while position not in emitted and position not in chain: + chain.append(position) + parent_id = tags[position].parent_id + parent_row = defining_row.get(parent_id.casefold()) if parent_id else None + if parent_row is None: + break + position = parent_row + for row in reversed(chain): + emitted.add(row) + ordered.append(tags[row]) + return ordered + def generate_actions( self, tags: list[TagItem], @@ -247,6 +451,7 @@ def generate_actions( self.actions.clear() self.errors.clear() self._init_indexed_actions() + existing = self._existing_tags() # Reject two or more rows claiming the same final id outright, # before staging or per-row action-building runs (see @@ -291,7 +496,14 @@ def generate_actions( # execution order regardless of how the rows are ordered in the file. self._build_staging_actions(tags) - for tag in tags: + # Reject what no single action can see: a cycle or an overflow of the tree the + # rows describe together. Then detach every tag whose parent changes, so that + # the row actions can run in any order. + parent_of, pk_of, row_of = self._end_state(tags, existing) + self._validate_hierarchy(tags, existing, parent_of, pk_of, row_of) + self._build_detach_actions(existing, parent_of, pk_of) + + for tag in self._order_rows_parent_first(tags, existing): has_action = False # Check all available actions and add which ones should be executed diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index cbec11b54..fad8727de 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -10,6 +10,7 @@ from openedx_tagging.import_export.actions import ( CreateTag, DeleteTag, + DetachTag, ImportAction, RenameTag, RenameTagExternalId, @@ -132,7 +133,7 @@ def test_validate_parent(self, parent_id: str, expected: bool): ( "Action error in 'import_action' (#100): " "Unknown parent tag (tag_100). " - "You need to add parent before the child in your file." + "Add a row for the parent to your file." ) ) @@ -140,16 +141,12 @@ 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. + action in the same import (recorded in `_vacated_pks` by the plan, + wherever that row is in the file), 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, - ) - ] + tag_3_pk = self.taxonomy.tag_set.get(external_id='tag_3').pk + indexed_actions: dict[str, list[ImportAction] | dict[int, str]] = dict(self.indexed_actions) + indexed_actions['_vacated_pks'] = {tag_3_pk: 'tag_60'} action = ImportAction( self.taxonomy, TagItem( @@ -1010,6 +1007,47 @@ def test_execute(self) -> None: self.assertTrue(tag.external_id.startswith("oel-import-staging:")) +class TestDetachTag(TestImportActionMixin, TestCase): + """ + Test for 'detach' action + """ + + def _action(self) -> DetachTag: + return DetachTag( + taxonomy=self.taxonomy, + tag=TagItem(id='tag_2', value='Tag 2', index=100), + index=100, + target_pk=self.taxonomy.tag_set.get(external_id='tag_2').pk, + ) + + def test_str(self) -> None: + self.assertEqual( + str(self._action()), + "Move tag (tag_2 / Tag 2) to the root first, " + "so that the parent changes in this file can run in any order." + ) + + def test_applies_for(self) -> None: + self.assertFalse(DetachTag.applies_for(self.taxonomy, tag=TagItem(id='tag_2', value='Tag 2', index=100))) + + def test_validate(self) -> None: + self.assertEqual(self._action().validate(self.indexed_actions), []) + + def test_execute(self) -> None: + """ + The tag moves to the root and keeps its pk; it is found by pk, so a + placeholder external_id from a staging action does not matter. + """ + action = self._action() + tag = self.taxonomy.tag_set.get(external_id='tag_2') + self.assertIsNotNone(tag.parent) + self.taxonomy.tag_set.filter(pk=tag.pk).update(external_id="oel-import-staging:x") + action.execute() + tag.refresh_from_db() + self.assertIsNone(tag.parent) + self.assertEqual(tag.depth, 0) + + 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 65517f8e7..0cb11df63 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -4,6 +4,7 @@ import json from io import BytesIO +import ddt # type: ignore[import] from django.test.testcases import TestCase import openedx_tagging.import_export.api as import_export_api @@ -13,6 +14,7 @@ from .mixins import TestImportExportMixin +@ddt.ddt class TestImportExportApi(TestImportExportMixin, TestCase): """ Test import/export API functions @@ -808,6 +810,49 @@ def test_import_swap_external_ids(self) -> None: assert exported_by_value["Tag 1"] == "tag_3" assert exported_by_value["Tag 3"] == "tag_1" + @ddt.data(True, False) + def test_import_swap_external_ids_with_children(self, children_first: bool) -> None: + """ + tag_1 and tag_3 swap ids, and each of their children names the id its + parent will hold after the import: tag_2 (under tag_1, which becomes + tag_3) says parent_id=tag_3, and tag_4 (under tag_3, which becomes tag_1) + says parent_id=tag_1. The full-replace import succeeds, whether the + children's rows come before or after the renames, and each child stays on + the same physical parent. See test_import_plan.TestTagImportPlan + .test_generate_actions_swap_child_naming_own_parent_stale_id_rejected + for the children naming the pre-import ids instead. + """ + 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 + + renames = [ + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + ] + children = [ + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_3"}, + {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"}, + ] + rows = children + renames if children_first else renames + children + importFile = BytesIO(json.dumps({"tags": rows}).encode()) + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + importFile, + self.parser_format, + replace=True, + ) + assert "Traceback" not in task.log + assert result, task.log + + tag_2 = self.taxonomy.tag_set.get(external_id="tag_2") + tag_4 = self.taxonomy.tag_set.get(external_id="tag_4") + assert tag_2.parent_id == old_pk_1 + assert tag_4.parent_id == old_pk_3 + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_parents = {tag["id"]: tag.get("parent_id") for tag in json.loads(output)["tags"]} + assert exported_parents == {"tag_3": None, "tag_2": "tag_3", "tag_1": None, "tag_4": "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 diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index 9fd40b074..32f5fef38 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -7,6 +7,7 @@ from openedx_tagging.import_export.actions import CreateTag from openedx_tagging.import_export.exceptions import TagImportError from openedx_tagging.import_export.import_plan import TagImportPlan, TagItem +from openedx_tagging.models import Tag, Taxonomy from .test_actions import TestImportActionMixin @@ -101,6 +102,10 @@ def test_build_delete_actions(self) -> None: False, 0, [ + { + 'name': 'detach', + 'id': 'tag_4' + }, { 'name': 'create', 'id': 'tag_31' @@ -181,6 +186,10 @@ def test_build_delete_actions(self) -> None: True, 0, [ + { + 'name': 'update_parent', + 'id': 'tag_4', + }, { 'name': 'delete', 'id': 'tag_1', @@ -189,10 +198,6 @@ def test_build_delete_actions(self) -> None: 'name': 'delete', 'id': 'tag_2', }, - { - 'name': 'update_parent', - 'id': 'tag_4', - }, { 'name': 'delete', 'id': 'tag_3', @@ -249,25 +254,27 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions False, "Import plan for Import Taxonomy Test\n" "--------------------------------\n" - "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" - "#2: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" - "#3: Rename tag value of (tag_1 / Tag 1) to 'Tag 2'\n" - "#4: Update the parent of (tag_4 / Tag 4) from parent " + "#1: Move tag (tag_2 / Tag 2) to the root first, so that the parent changes " + "in this file can run in any order.\n" + "#2: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" + "#3: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" + "#4: Rename tag value of (tag_1 / Tag 1) to 'Tag 2'\n" + "#5: Update the parent of (tag_4 / Tag 4) from parent " " (tag_3 / Tag 3) to tag_100\n" - "#5: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" - "#6: Update the parent of (tag_2 / Tag 2) from parent " + "#6: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" + "#7: Update the parent of (tag_2 / Tag 2) from parent " " (tag_1 / Tag 1) to None\n" - "#7: Rename tag value of (tag_2 / Tag 2) to 'Tag 31'\n" + "#8: Rename tag value of (tag_2 / Tag 2) to 'Tag 31'\n" "\nOutput errors\n" "--------------------------------\n" "Duplicate id (tag_31): file 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). " - "You need to add parent before the child in your file.\n" - "Conflict with 'create' (#5) and action #2: Duplicated tag value.\n" - "Conflict with 'rename' (#7) and action #1: Duplicated tag value.\n" + "Conflict with 'create' (#3) and action #2: Duplicated external_id tag.\n" + "Action error in 'rename' (#4): Duplicated tag value with tag in database (external_id=tag_2).\n" + "Action error in 'update_parent' (#5): Unknown parent tag (tag_100). " + "Add a row for the parent to your file.\n" + "Conflict with 'create' (#6) and action #3: Duplicated tag value.\n" + "Conflict with 'rename' (#8) and action #2: Duplicated tag value.\n" ), # Testing valid plan ( @@ -299,13 +306,15 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions False, "Import plan for Import Taxonomy Test\n" "--------------------------------\n" - "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" - "#2: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" - "#3: Rename tag value of (tag_2 / Tag 2) to 'Tag 2 v2'\n" - "#4: Update the parent of (tag_4 / Tag 4) from parent " + "#1: Move tag (tag_4 / Tag 4) to the root first, so that the parent changes " + "in this file can run in any order.\n" + "#2: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" + "#3: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" + "#4: Rename tag value of (tag_2 / Tag 2) to 'Tag 2 v2'\n" + "#5: Update the parent of (tag_4 / Tag 4) from parent " " (tag_3 / Tag 3) to tag_1\n" - "#5: Rename tag value of (tag_4 / Tag 4) to 'Tag 4 v2'\n" - "#6: No changes needed for (tag_1 / Tag 1)\n" + "#6: Rename tag value of (tag_4 / Tag 4) to 'Tag 4 v2'\n" + "#7: No changes needed for (tag_1 / Tag 1)\n" ), # Testing deletes (replace=True) ( @@ -319,10 +328,10 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions True, "Import plan for Import Taxonomy Test\n" "--------------------------------\n" - "#1: Delete tag (tag_1 / Tag 1)\n" - "#2: Delete tag (tag_2 / Tag 2)\n" - "#3: Update the parent of (tag_4 / Tag 4) from parent " + "#1: Update the parent of (tag_4 / Tag 4) from parent " " (tag_3 / Tag 3) to None\n" + "#2: Delete tag (tag_1 / Tag 1)\n" + "#3: Delete tag (tag_2 / Tag 2)\n" "#4: Delete tag (tag_3 / Tag 3)\n" "#5: No changes needed for (tag_4 / Tag 4)\n" ), @@ -619,6 +628,331 @@ def test_generate_actions_parent_id_new_id_after_earlier_rename_accepted(self) - self.import_plan.generate_actions(tags=tags, replace=False) self.assertEqual(self.import_plan.errors, []) + def test_generate_actions_existing_child_keeps_stale_parent_id_rejected(self) -> None: + """ + tag_2 is already under tag_1. The file renames tag_1 to tag_50 while the + tag_2 row still says parent_id=tag_1. That row matches the parent's + pre-import id, so it used to count as "no change" and slipped past + _validate_parent, although tag_1 names nothing after the import. It must + be rejected, and the error must name the parent's new id. + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual( + [str(error) for error in self.import_plan.errors], + [ + "Action error in 'update_parent' (#2): " + "Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id." + ], + ) + + @ddt.data( + ('tag_2', None, 'tag_1'), # exact pre-import id + ('tag_2', None, 'TAG_1'), # case variant of it + ('tag_20', 'tag_2', 'tag_1'), # the child is renamed too + ) + @ddt.unpack + def test_generate_actions_swap_child_naming_own_parent_stale_id_rejected( + self, child_id: str, child_previous_id: str | None, child_parent_id: str, + ) -> None: + """ + tag_2 is under tag_1, and the file swaps the ids of tag_1 and tag_3. After + the import "tag_1" names the old tag_3, so a tag_2 row that still says + parent_id=tag_1 could mean "stay with my parent" or "move under the tag + now called tag_1". It is rejected, once, and told to use tag_3. + """ + 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=child_id, value='Tag 2', previous_id=child_previous_id, parent_id=child_parent_id), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn( + f"Parent tag {child_parent_id} is renamed to tag_3 in this file; use tag_3 as the parent_id.", + str(self.import_plan.errors[0]), + ) + + @ddt.data('tag_1', 'TAG_1') + def test_generate_actions_case_only_parent_rename_keeps_child(self, child_parent_id: str) -> None: + """ + Renaming tag_1 to TAG_1 only changes its capitalization. external_id is + case-insensitive, so the id still names the same tag after the import, + and a tag_2 row naming it in either spelling is not a stale reference. + """ + tags = [ + TagItem(id='TAG_1', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id=child_parent_id), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + + @ddt.data( + # A new child listed before its new parent. + ( + [ + TagItem(id='new_child', value='New child', parent_id='new_parent'), + TagItem(id='new_parent', value='New parent'), + ], + ['new_parent', 'new_child'], + ), + # A row naming a parent's new id, spelled in another case, listed before the rename. + ( + [ + TagItem(id='tag_60', value='Tag 60', parent_id='TAG_50'), + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + ], + ['tag_50', 'tag_60'], + ), + # Parents already before their children: the plan keeps the file order. + ( + [ + TagItem(id='new_parent', value='New parent'), + TagItem(id='tag_4', value='Tag 4 v2', parent_id='tag_3'), + TagItem(id='new_child', value='New child', parent_id='new_parent'), + ], + ['new_parent', 'tag_4', 'new_child'], + ), + ) + @ddt.unpack + def test_generate_actions_orders_each_row_after_the_row_defining_its_parent( + self, tags: list[TagItem], expected_order: list[str], + ) -> None: + """ + A row is built after the row that creates or renames-in its parent, so a + file may list rows in any order. Rows are only pulled forward when needed. + """ + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual([action.tag.id for action in self.import_plan.actions], expected_order) + + def _plan_for_tree(self, tree: dict[str, str | None]) -> TagImportPlan: + """ + Create a taxonomy holding ``tree`` (id -> parent id, parents listed first) and + return a plan for it. The tag values are the ids. + """ + taxonomy = Taxonomy.objects.create(name='Tree taxonomy') + tags: dict[str, Tag] = {} + for tag_id, parent_id in tree.items(): + tags[tag_id] = Tag.objects.create( + taxonomy=taxonomy, value=tag_id, external_id=tag_id, + parent=tags[parent_id] if parent_id else None, + ) + return TagImportPlan(taxonomy) + + @staticmethod + def _tree(taxonomy: Taxonomy) -> dict[str, str | None]: + """ + The taxonomy's tags as id -> parent id. + """ + return {tag.external_id: tag.parent.external_id if tag.parent else None for tag in taxonomy.tag_set.all()} + + SIX_LEVELS: dict[str, str | None] = { + 'level_1': None, 'level_2': 'level_1', 'level_3': 'level_2', + 'level_4': 'level_3', 'level_5': 'level_4', 'level_6': 'level_5', + } + + @ddt.data( + # A parent and its child swap places. + ( + {'parent': None, 'child': 'parent'}, + [TagItem(id='parent', value='parent', parent_id='child'), TagItem(id='child', value='child')], + {'child': None, 'parent': 'child'}, + ), + # The ancestry is inverted through a new tag: new_root must leave old_root's + # subtree before old_root moves under new_middle. + ( + {'old_root': None, 'sibling': 'old_root', 'new_root': 'old_root'}, + [ + TagItem(id='old_root', value='old_root', parent_id='new_middle'), + TagItem(id='sibling', value='sibling', parent_id='new_middle'), + TagItem(id='new_middle', value='new_middle', parent_id='new_root'), + TagItem(id='new_root', value='new_root'), + ], + {'new_root': None, 'new_middle': 'new_root', 'old_root': 'new_middle', 'sibling': 'new_middle'}, + ), + # A deep tag moves to the root while new tags are created under it: created + # first, the new grandchild would be at a seventh level. + ( + {'level_1': None, 'level_2': 'level_1', 'level_3': 'level_2', 'level_4': 'level_3', 'deep': 'level_4'}, + [ + TagItem(id='new_grandchild', value='new_grandchild', parent_id='new_child'), + TagItem(id='new_child', value='new_child', parent_id='deep'), + TagItem(id='deep', value='deep'), + ], + { + 'level_1': None, 'level_2': 'level_1', 'level_3': 'level_2', 'level_4': 'level_3', + 'deep': None, 'new_child': 'deep', 'new_grandchild': 'new_child', + }, + ), + # A subtree moves under a tag that itself moves up: attached first, the + # grandchild would be at a seventh level. + ( + { + 'chain_1': None, 'chain_2': 'chain_1', 'chain_3': 'chain_2', 'moved_up': 'chain_3', + 'subtree': None, 'subtree_child': 'subtree', 'subtree_grandchild': 'subtree_child', + }, + [ + TagItem(id='subtree', value='subtree', parent_id='moved_up'), + TagItem(id='moved_up', value='moved_up'), + ], + { + 'chain_1': None, 'chain_2': 'chain_1', 'chain_3': 'chain_2', 'moved_up': None, + 'subtree': 'moved_up', 'subtree_child': 'subtree', 'subtree_grandchild': 'subtree_child', + }, + ), + ) + @ddt.unpack + def test_execute_parent_moves_whose_steps_depend_on_each_other( + self, tree: dict[str, str | None], rows: list[TagItem], expected: dict[str, str | None], + ) -> None: + """ + Restructures that fail when their actions run in file order, because an + intermediate step would build a cycle or a tree too deep, succeed: a + detach action moves every tag whose parent changes to the root before the + row actions. ``rows`` lists the worst order for each case. Omitted tags + keep their parent (replace=False). + """ + plan = self._plan_for_tree(tree) + plan.generate_actions(tags=rows, replace=False) + self.assertEqual([str(error) for error in plan.errors], []) + names = [action.name for action in plan.actions] + self.assertIn('detach', names) + self.assertLess( + max(index for index, name in enumerate(names) if name == 'detach'), + min(index for index, name in enumerate(names) if name != 'detach'), + ) + plan.execute() + self.assertEqual(self._tree(plan.taxonomy), expected) + + def test_execute_kept_tag_under_deleted_ancestors_survives(self) -> None: + """ + With replace=True, a kept tag whose parent and grandparent are deleted is + promoted to the root before the first delete, so the cascade does not + remove it, and it keeps its database row. + """ + plan = self._plan_for_tree({'deleted_root': None, 'deleted_child': 'deleted_root', 'kept': 'deleted_child'}) + kept_pk = plan.taxonomy.tag_set.get(external_id='kept').pk + plan.generate_actions(tags=[TagItem(id='kept', value='kept')], replace=True) + self.assertEqual(plan.errors, []) + names = [action.name for action in plan.actions] + self.assertLess(names.index('update_parent'), names.index('delete')) + self.assertNotIn('detach', names) # The promotion already moves the tag to the root. + plan.execute() + self.assertEqual(self._tree(plan.taxonomy), {'kept': None}) + self.assertEqual(plan.taxonomy.tag_set.get(external_id='kept').pk, kept_pk) + + def test_execute_child_of_deleted_parent_follows_rename_onto_its_id(self) -> None: + """ + With replace=True the file omits tag_1, so it is deleted, and renames tag_3 + onto the id tag_1. The tag_2 row names tag_1, so tag_2 ends up under the + renamed tag_3 rather than promoted to the root. + """ + tag_3_pk = self.taxonomy.tag_set.get(external_id='tag_3').pk + tags = [ + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + ] + self.import_plan.generate_actions(tags=tags, replace=True) + self.assertEqual(self.import_plan.errors, []) + self.import_plan.execute() + self.assertEqual(self.taxonomy.tag_set.get(external_id='tag_2').parent_id, tag_3_pk) + self.assertEqual(self.taxonomy.tag_set.get(pk=tag_3_pk).external_id, 'tag_1') + + @ddt.data( + # tag_1 under its own child tag_2. + ([TagItem(id='tag_1', value='Tag 1', parent_id='tag_2')], False, "Row 1's parent_id (tag_2)"), + # A tag under itself. + ([TagItem(id='tag_2', value='Tag 2', parent_id='tag_2')], False, "Row 1's parent_id (tag_2)"), + # tag_3 takes the id of the deleted tag_1 and names that id as its parent. + ( + [TagItem(id='tag_1', value='Tag 3', previous_id='tag_3', parent_id='tag_1')], + True, + "Row 1's parent_id (tag_1)", + ), + # Two renamed tags, on ids the import frees, name each other. + ( + [ + TagItem(id='tag_4', value='Tag 4', parent_id='tag_2'), + TagItem(id='tag_2', value='Tag 1', previous_id='tag_1', parent_id='tag_3'), + TagItem(id='tag_3', value='Tag 2', previous_id='tag_2', parent_id='tag_2'), + ], + True, + "Row 2's parent_id (tag_3)", + ), + ) + @ddt.unpack + def test_generate_actions_parent_cycle_rejected_once( + self, tags: list[TagItem], replace: bool, expected_prefix: str, + ) -> None: + """ + A parent_id that makes a tag its own ancestor after the import is rejected + with one error, on the cycle's first row in the file. + """ + self.import_plan.generate_actions(tags=tags, replace=replace) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 1, messages) + self.assertEqual(messages[0], f"{expected_prefix} creates a parent cycle.") + + def test_generate_actions_long_reversed_chain_terminates(self) -> None: + """ + A 1100-row chain of new tags listed child first is ordered and checked + without recursion, so it neither hits the recursion limit nor crashes: + every row is planned, and the chain is rejected as too deep. + """ + tags = [ + TagItem(id=f'n{i}', value=f'N{i}', parent_id=f'n{i - 1}' if i else None) + for i in reversed(range(1100)) + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.actions), 1100) + self.assertEqual(self.import_plan.actions[0].tag.id, 'n0') + self.assertEqual( + [str(error) for error in self.import_plan.errors], + ["This import would leave tag n6 more than 6 levels deep."], + ) + + def test_generate_actions_cycle_of_new_tags_reports_an_unknown_parent_once(self) -> None: + """ + New tags that name each other as parent get the existing "Unknown parent" + error for one of them, and no cycle error on top. + """ + tags = [ + TagItem(id='new_a', value='New A', parent_id='new_b'), + TagItem(id='new_b', value='New B', parent_id='new_a'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 1, messages) + self.assertIn("Unknown parent tag", messages[0]) + + FOUR_LEVELS: dict[str, str | None] = { + 'other_1': None, 'other_2': 'other_1', 'other_3': 'other_2', 'other_4': 'other_3', + } + + @ddt.data( + # A new tag under the sixth level. + (TagItem(id='level_7', value='level_7', parent_id='level_6'), 'level_7'), + # A four-level chain moved under the fourth level: other_3 and other_4 end up + # at the seventh and eighth level, although neither has a row. + (TagItem(id='other_1', value='other_1', parent_id='level_4'), 'other_3'), + ) + @ddt.unpack + def test_generate_actions_import_too_deep_rejected(self, row: TagItem, named_tag: str) -> None: + """ + An import whose result would leave a tag deeper than the taxonomy allows is + rejected at the plan step, with one error naming a tag that would be too deep. + """ + plan = self._plan_for_tree({**self.SIX_LEVELS, **self.FOUR_LEVELS}) + plan.generate_actions(tags=[row], replace=False) + self.assertEqual( + [str(error) for error in plan.errors], + [f"This import would leave tag {named_tag} more than 6 levels deep."], + ) + def test_generate_actions_genuine_collision_not_staged(self) -> None: """ A rename targeting an id held by an unrelated tag that is not