diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index 96aced3f1..980e0b7c0 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -146,7 +146,7 @@ def _validate_parent(self, indexed_actions) -> ImportActionError | None: action=self, message=_( "Unknown parent tag ({parent_id}). " - "You need to add parent before the child in your file." + "Add a row for the parent to your file." ).format(parent_id=self.tag.parent_id), ) return None @@ -343,12 +343,30 @@ def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> """ This action applies whenever there is a change on the parent. + A row whose parent_id equals the current parent's external_id is "no + change" and skips `_validate_parent`. TagImportPlan validates the + invalid cases of such rows itself: a parent renamed away, and parent + cycles. The exception is a parent that this import deletes while a + rename reclaims its id: the row applies, to move the child under the + renamed tag. + 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 + parent = taxonomy_tag.parent + # The parent is deleted by this import and a rename reclaims its id, so + # the child must move under the renamed tag instead of being promoted. + if ( + parent is not None + and parent.external_id == tag.parent_id + and indexed_actions + and any(a.tag.id.casefold() == parent.external_id.casefold() for a in indexed_actions["delete"]) + and any(a.tag.id.casefold() == parent.external_id.casefold() for a in indexed_actions["rename_external_id"]) + ): + return True return ( taxonomy_tag.parent is not None and taxonomy_tag.parent.external_id != tag.parent_id diff --git a/src/openedx_tagging/import_export/exceptions.py b/src/openedx_tagging/import_export/exceptions.py index 1f1caf1f6..ee7c3619c 100644 --- a/src/openedx_tagging/import_export/exceptions.py +++ b/src/openedx_tagging/import_export/exceptions.py @@ -162,3 +162,52 @@ class InvalidCSVField(TagParserError): def __init__(self, tag: dict | None, field: str, row: int, **kargs): super().__init__(tag, **kargs) self.message = _("Invalid '{field}' field on the row {row}").format(field=field, row=row) + + +class ParentIdRenamedAwayError(TagImportError): + """ + Exception raised when a row's parent_id is its current parent's pre-import + id, and the same import renames that parent to a different id. + + After the import that id names no tag or, after a swap of ids, a different + tag, so the message asks for the parent's new id instead. ``row`` is the + 1-based position of the row among the file's data rows: row 1 is the first + row after a CSV header, or the first entry of a JSON file's ``tags``. + """ + + def __init__(self, row: int, parent_id: str, new_id: str, **kargs): + super().__init__(**kargs) + self.message = _( + "Row {row}'s parent_id ({parent_id}) names an id this file renames away. " + "Use '{new_id}' as the parent_id." + ).format(row=row, parent_id=parent_id, new_id=new_id) + + +class ParentCycleError(TagImportError): + """ + Exception raised when the parent_id of a row makes the tag its own + ancestor once the import is applied. + + ``row`` is numbered like in ``ParentIdRenamedAwayError``. + """ + + 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 InvalidParentHierarchyError(TagImportError): + """ + Exception raised when applying the import would build a tag tree that is too + deep or otherwise invalid, even when the importer first moves every tag whose + parent changes to the root. + """ + + def __init__(self, max_levels: int, **kargs): + super().__init__(**kargs) + self.message = _( + "This import would leave tags deeper than {max_levels} levels, or build an invalid " + "parent hierarchy, so it cannot be applied." + ).format(max_levels=max_levels) diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index b39cbddda..95157688a 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -3,10 +3,13 @@ """ 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, ImportAction, @@ -16,7 +19,28 @@ WithoutChanges, available_actions, ) -from .exceptions import DuplicateFinalIdError, StaleIdTargetedByPlainRowError, TagImportError +from .exceptions import ( + DuplicateFinalIdError, + InvalidParentHierarchyError, + ParentCycleError, + ParentIdRenamedAwayError, + StaleIdTargetedByPlainRowError, + TagImportError, +) + + +class ExistingTag(NamedTuple): + """ + The fields of an existing tag that the plan-level validations need. + """ + + pk: int + parent_pk: int | None + parent_external_id: str | None + + +# Existing tags by casefolded external_id. +ExistingTags = dict[str, ExistingTag] @define @@ -40,9 +64,141 @@ def __str__(self): return f"<{self.__class__.__name__}> ({self.value})" +class _ParentForest: + """ + In-memory copy of a taxonomy's parent links, used to simulate the parent effects of import + actions without writing to the database. + + Tags are identified by pk, and new tags get negative ids. Tags are looked up by their casefolded + external_id, which follows renames. + """ + + def __init__(self, existing: ExistingTags): + self.node_of: dict[str, int] = {} + self.key_of: dict[int, str] = {} + self.parent: dict[int, int | None] = {} + self.children: dict[int, set[int]] = {} + self.new_nodes = 0 + for key, info in existing.items(): + self.node_of[key] = info.pk + self.key_of[info.pk] = key + self.parent[info.pk] = info.parent_pk + if info.parent_pk is not None: + self.children.setdefault(info.parent_pk, set()).add(info.pk) + + def _depth(self, node: int) -> int: + """ + Return the number of ancestors of ``node``. + """ + result, ancestor = 0, self.parent.get(node) + while ancestor is not None and result <= len(self.parent) + 1: + result += 1 + ancestor = self.parent.get(ancestor) + return result + + def _height(self, node: int) -> int: + """ + Return the number of levels below ``node``, stopping once it exceeds the maximum depth. + """ + result, level = 0, [node] + while level and result <= TAXONOMY_MAX_DEPTH: + level = [child for member in level for child in self.children.get(member, ())] + result += 1 if level else 0 + return result + + def _attach(self, node: int, new_parent: int | None) -> bool: + """ + Move ``node`` under ``new_parent``; return False if that makes a cycle or a tree too deep. + """ + if new_parent is not None: + ancestor: int | None = new_parent + for _step in range(len(self.parent) + 2): + if ancestor is None: + break + if ancestor == node: + return False + ancestor = self.parent.get(ancestor) + if self._depth(new_parent) + 1 + self._height(node) > TAXONOMY_MAX_DEPTH: + return False + old_parent = self.parent.get(node) + if old_parent is not None: + self.children.get(old_parent, set()).discard(node) + self.parent[node] = new_parent + if new_parent is not None: + self.children.setdefault(new_parent, set()).add(node) + return True + + def _rekey(self, node: int, key: str) -> None: + """ + Make ``node``, new or existing, answer to the external_id ``key``. + """ + self.node_of.pop(self.key_of.get(node, ""), None) + self.key_of[node] = key + self.node_of[key] = node + + def _delete(self, key: str) -> None: + """ + Remove the tag with external_id ``key`` and all its descendants, if it exists. + """ + deleted_node = self.node_of.get(key) + pending = [deleted_node] if deleted_node is not None else [] + while pending: + node = pending.pop() + pending.extend(self.children.pop(node, ())) + old_parent = self.parent.pop(node, None) + if old_parent is not None: + self.children.get(old_parent, set()).discard(node) + self.node_of.pop(self.key_of.pop(node, ""), None) + + def apply(self, action: ImportAction) -> bool: + """ + Apply the parent effects of ``action``; return False if it would fail at execution. + """ + tag = action.tag + if action.name == "stage_external_id": + if action.target_pk not in self.parent: + return False + # A staged tag holds a temporary external_id until its rename runs. + self._rekey(action.target_pk, f"staged:{action.target_pk}") + elif action.name == "delete": + self._delete(tag.id.casefold()) + elif action.name in ("update_parent", "create", "rename_external_id"): + new_parent = self.node_of.get(tag.parent_id.casefold()) if tag.parent_id else None + if tag.parent_id and new_parent is None: + return False + if action.name == "create": + self.new_nodes -= 1 + self.parent[self.new_nodes] = None + if not self._attach(self.new_nodes, new_parent): + return False + self._rekey(self.new_nodes, tag.id.casefold()) + elif action.name == "update_parent": + target = self.node_of.get(tag.id.casefold()) + return target is not None and self._attach(target, new_parent) + else: + target = action.target_pk if action.target_pk in self.parent else None + if target is None or not self._attach(target, new_parent): + return False + self._rekey(target, tag.id.casefold()) + return True + + class TagImportPlan: """ Class with functions to build an import plan and excute the plan + + Terms used by the plan-level validations below: + + - A *rename row* has a ``previous_id`` that differs from its ``id``. It renames the tag + that holds ``previous_id`` to ``id``. + - A tag is *renamed away* from an id when a rename row gives it a different id. Its + pre-import id is then *stale*: after the import that id names no tag, or, after a + swap of ids, a different tag. + - The *end state* is the taxonomy as it will be once the whole import is applied. A + row's ``parent_id`` names its parent's id in the end state. + - "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 (the CSV parser's own + errors use spreadsheet line numbers). In a JSON file it is the N-th entry of ``tags``. """ actions: list[ImportAction] @@ -112,6 +268,9 @@ def _get_tag_id(self, tag: Tag) -> str: def _build_delete_actions(self, tags: dict): """ Adds delete actions for `tags` + + Every kept child is promoted to a root before the first delete, because + deleting a tag also deletes all its descendants. """ for tag in tags.values(): for child in tag.children.all(): @@ -129,6 +288,7 @@ def _build_delete_actions(self, tags: dict): ), ) + for tag in tags.values(): # Delete action self._build_action( DeleteTag, @@ -161,7 +321,8 @@ def _validate_no_duplicate_final_ids(self, tags: list[TagItem]) -> None: 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. + is always defined. For the admin, that position is "Row N" as the + class docstring defines it. """ positions_by_id: dict[str, list[int]] = {} display_id_by_key: dict[str, str] = {} @@ -228,6 +389,316 @@ def _build_staging_actions(self, tags: list[TagItem]) -> None: self._build_action(StageTagExternalIdForSwap, tag, target_pk=target_pk) self.indexed_actions["_vacated_pks"] = vacated_pks + def _existing_tags(self) -> ExistingTags: + """ + Return the taxonomy's tags keyed by casefolded external_id, with their pk, parent pk and + parent external_id. + + One query, so the plan-level validations don't query per row. + """ + return { + external_id.casefold(): ExistingTag(pk, parent_pk, parent_external_id) + for external_id, pk, parent_pk, parent_external_id in self.taxonomy.tag_set.values_list( + "external_id", "pk", "parent_id", "parent__external_id", + ) + } + + def _validate_parent_id_not_stale(self, tags: list[TagItem], existing: ExistingTags) -> None: + """ + Reject a row whose ``parent_id`` is the stale id of the row's current parent, + that is, the parent's pre-import id when this import renames that parent away. + + A plain rename makes the id meaningless. A swap makes it ambiguous. For example, + before the import ``tag_2`` is under ``tag_1``, and the file swaps the ids of + ``tag_1`` and ``tag_3``. After the import the id ``tag_1`` belongs to the tag that + was ``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``". The row is + rejected, and the error tells the admin to use ``tag_3``, the parent's new id. + A child that names the end-state id is accepted: ``tag_4``, under the old + ``tag_3``, may say ``parent_id: tag_1``. + + A rename that only changes the parent's capitalization is not flagged. A case + variant of the stale id (``TAG_1`` for ``tag_1``) is rejected like the exact + spelling. Must run after ``_build_staging_actions``, which fills + ``indexed_actions["_vacated_pks"]`` with the new id of every renamed tag. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + vacated_pks: dict[int, str] = self.indexed_actions["_vacated_pks"] + if not vacated_pks: + return + rename_final_ids = { + tag.id.casefold() for tag in tags + if tag.previous_id and tag.id != tag.previous_id + } + for position, tag in enumerate(tags, start=1): + if not tag.parent_id: + continue + existing_tag = existing.get((tag.previous_id or tag.id).casefold()) + if existing_tag is None: + continue # A new tag has no current parent. + parent_key = tag.parent_id.casefold() + names_current_parent = (existing_tag.parent_external_id or "").casefold() == parent_key + if not names_current_parent: + continue # A parent change, which _validate_parent checks. + new_parent_id = vacated_pks.get(existing_tag.parent_pk) if existing_tag.parent_pk is not None else None + if new_parent_id is None or new_parent_id.casefold() == parent_key: + continue # The parent keeps its id, or only changes its capitalization. + + # With the exact spelling, UpdateParentTag.applies_for sees "no change" and + # _validate_parent never runs, so only this check can catch the row. A case + # variant is a parent change for UpdateParentTag, and _validate_parent reports + # it, except when a rename row claims the id: _validate_parent then accepts it. + is_exact_spelling = existing_tag.parent_external_id == tag.parent_id + is_claimed_by_rename = parent_key in rename_final_ids + # A rename row's own validation reports a stale parent_id, except in a swap, + # where the stale id is also another rename's final id. + is_rename_row = bool(tag.previous_id) and tag.id != tag.previous_id + is_reported_elsewhere = is_rename_row and not is_claimed_by_rename + if (is_exact_spelling or is_claimed_by_rename) and not is_reported_elsewhere: + self.errors.append(ParentIdRenamedAwayError(position, tag.parent_id, new_parent_id)) + + def _end_state_parents( + self, tags: list[TagItem], existing: ExistingTags, + ) -> tuple[dict[str, str], dict[str, int]]: + """ + Return the end-state parent graph and the position of each id's first row. + + The graph maps a casefolded final id to its parent's casefolded final id: + each row contributes ``id -> parent_id``, and each remaining tag (not + named by any row's id or previous_id, and not deleted) contributes its + id and its current parent's id, or that parent's new id if it is renamed. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + :return: the graph, and the position in ``tags`` of each final id's first row. + """ + # Not set until _build_staging_actions runs; callers may use this before it. + vacated_pks: dict[int, str] = self.indexed_actions.get("_vacated_pks", {}) + deleted_ids = {action.tag.id.casefold() for action in self.indexed_actions["delete"]} + parent_of: dict[str, str] = {} + row_position: dict[str, int] = {} + represented: set[str] = set() + for position, tag in enumerate(tags): + key = tag.id.casefold() + represented.add(key) + if tag.previous_id: + represented.add(tag.previous_id.casefold()) + if key in row_position: + continue + row_position[key] = position + if tag.parent_id: + parent_of[key] = tag.parent_id.casefold() + for key, tag_info in existing.items(): + parent_id = tag_info.parent_external_id + if key in represented or key in deleted_ids: + continue # The tag's row gives its parent, or the tag is deleted. + if tag_info.parent_pk is None or parent_id is None: + continue # A root tag. + if parent_id.casefold() in deleted_ids: + continue # The delete phase promotes the tag to root. + parent_of[key] = vacated_pks.get(tag_info.parent_pk, parent_id).casefold() + return parent_of, row_position + + def _validate_no_parent_cycles(self, tags: list[TagItem], existing: ExistingTags) -> None: + """ + Reject a row whose ``parent_id`` makes a tag its own ancestor (a + self-parent is a cycle of length one) once this import is applied. + + The check runs on the end-state graph from ``_end_state_parents``. A cycle + is reported once, on its first row in the file. A cycle made only of rows + that define their id (see ``_defines_id``) and whose ids no existing tag + holds is skipped: ``_validate_parent`` already reports it as an unknown + parent, because none of those ids exists in the database yet. + Must run after ``_build_staging_actions``. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + parent_of, row_position = self._end_state_parents(tags, existing) + row_defines_id = {key: self._defines_id(tags[position], existing) for key, position in row_position.items()} + + state: dict[str, int] = {} # 1 on the current path, 2 once fully walked + for tag in tags: + path: list[str] = [] + node: str | None = tag.id.casefold() + while node is not None and node not in state: + state[node] = 1 + path.append(node) + node = parent_of.get(node) + if node is not None and state[node] == 1: + cycle = path[path.index(node):] + rows = [row_position[key] for key in cycle if key in row_position] + # A cycle made only of ids that no existing tag holds. + is_reported_as_unknown_parent = ( + all(row_defines_id.get(key, False) for key in cycle) + and not any(key in existing for key in cycle) + ) + if rows and not is_reported_as_unknown_parent: + position = min(rows) + self.errors.append(ParentCycleError(position + 1, tags[position].parent_id or "")) + for key in path: + state[key] = 2 + + @staticmethod + def _defines_id(tag: TagItem, existing: ExistingTags) -> bool: + """ + Return whether the row defines its final id, so that rows naming that id as + ``parent_id`` depend on it: a rename row, or a row that creates a tag (its id is + not held by an existing tag). + """ + is_rename = bool(tag.previous_id) and tag.id != tag.previous_id + return is_rename or tag.id.casefold() not in existing + + def _order_rows_by_parent_dependency(self, tags: list[TagItem], existing: ExistingTags) -> list[TagItem]: + """ + Return ``tags`` reordered so that parent references resolve and no move + builds a cycle. + + A row follows the row that defines its ``parent_id`` (a rename row or a + new-tag row with that final id, case-insensitive). A row that moves an + existing tag under a new parent also follows the rows of the tags on + that parent's end-state ancestor chain that currently sit inside its + subtree, so a move out of a subtree runs before a move into it. + Validation and execution both follow action order, so this makes both + independent of row order. Only a row and its dependencies are moved: + unrelated rows between them end up after both. Rows that are already in + a valid order keep their position, so their plan is unchanged. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + defining_row: dict[str, int] = {} + for position, tag in enumerate(tags): + if self._defines_id(tag, existing): + defining_row.setdefault(tag.id.casefold(), position) + parent_of, row_position = self._end_state_parents(tags, existing) + parent_pk_of = {info.pk: info.parent_pk for info in existing.values()} + + def dependencies(position: int) -> list[int]: + """ + Return the positions of the rows that must run before the row at ``position``. + """ + tag = tags[position] + if not tag.parent_id: + return [] + result = [] + parent_row = defining_row.get(tag.parent_id.casefold()) + if parent_row is not None: + result.append(parent_row) + moved_tag = existing.get((tag.previous_id or tag.id).casefold()) + if moved_tag is None: + return result + # Walk the new parent's end-state ancestor chain. + node: str | None = tag.parent_id.casefold() + visited: set[str] = set() + while node is not None and node not in visited: + visited.add(node) + ancestor_position = row_position.get(node) + ancestor_row = tags[ancestor_position] if ancestor_position is not None else None + ancestor_tag = ( + existing.get((ancestor_row.previous_id or ancestor_row.id).casefold()) if ancestor_row else None + ) + if ancestor_tag is not None and ancestor_tag.pk != moved_tag.pk: + # Walk the ancestor's current parents to see if it sits inside the moved subtree. + pk = ancestor_tag.parent_pk + while pk is not None and pk != moved_tag.pk: + pk = parent_pk_of.get(pk) + is_inside_moved_subtree = pk is not None + if is_inside_moved_subtree and ancestor_position is not None: + result.append(ancestor_position) + node = parent_of.get(node) + return result + + return self._order_by_dependencies(tags, [dependencies(position) for position in range(len(tags))]) + + @staticmethod + def _order_by_dependencies(tags: list[TagItem], dependencies: list[list[int]]) -> list[TagItem]: + """ + Return ``tags`` in an order where each row follows the rows listed in + ``dependencies[position]``, keeping file order otherwise. A dependency + cycle is broken at its second visit, leaving a best-effort order. + """ + ordered: list[int] = [] + seen: set[int] = set() # emitted or in progress; the latter breaks cycles + for start in range(len(tags)): + stack = [(start, False)] + while stack: + position, done = stack.pop() + if done: + ordered.append(position) + continue + if position in seen: + continue + seen.add(position) + stack.append((position, True)) + stack.extend((dependency, False) for dependency in reversed(dependencies[position])) + return [tags[position] for position in ordered] + + def _order_rows_parent_first(self, tags: list[TagItem], existing: ExistingTags) -> list[TagItem]: + """ + Return ``tags`` with each row after the row whose final id is its ``parent_id``. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + _parent_of, row_position = self._end_state_parents(tags, existing) + dependencies: list[list[int]] = [] + for position, tag in enumerate(tags): + parent_row = row_position.get(tag.parent_id.casefold()) if tag.parent_id else None + # A row whose parent_id is its own id has no other row to follow. + if parent_row is not None and parent_row != position: + dependencies.append([parent_row]) + else: + dependencies.append([]) + return self._order_by_dependencies(tags, dependencies) + + def _build_detach_actions(self, tags: list[TagItem], existing: ExistingTags) -> None: + """ + Add an update-to-root action for every existing tag whose row changes its parent. + + After these actions every remaining parent link is an unchanged one, so no later step + sees a tree deeper or more cyclic than the final one. Tags that the delete phase has + already promoted are skipped. Must run before ``_build_staging_actions``, because the + actions find their tag by its pre-import external_id. + + :param tags: the rows of the import file, in file order. + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + end_id_by_pk: dict[int, str] = {} + for tag in tags: + current = existing.get((tag.previous_id or tag.id).casefold()) + if current is not None: + end_id_by_pk.setdefault(current.pk, tag.id) + deleted_ids = {action.tag.id.casefold() for action in self.indexed_actions["delete"]} + for tag in tags: + current = existing.get((tag.previous_id or tag.id).casefold()) + if current is None or current.parent_pk is None or current.parent_external_id is None: + continue # A new tag or a root tag has nothing to detach. + if current.parent_external_id.casefold() in deleted_ids: + continue # already promoted by the delete phase + parent_end_id = end_id_by_pk.get(current.parent_pk, current.parent_external_id).casefold() + if (tag.parent_id or "").casefold() != parent_end_id: + self._build_action( + UpdateParentTag, TagItem(id=tag.previous_id or tag.id, value=tag.value, parent_id=None), + ) + + def _parent_changes_are_valid(self, existing: ExistingTags) -> bool: + """ + Return whether every parent change in the built actions leaves a valid tree. + + Actions run in order and ``Tag.save`` validates the depth of each step, so a plan can + fail at execution although its end state is valid. This simulates the parent effects of + ``self.actions`` in memory, with no database access, and returns False if a step would + exceed ``TAXONOMY_MAX_DEPTH``, use a tag that an earlier step removed, or create a cycle + (a second guard: ``_validate_no_parent_cycles`` already rejects cyclic end states). + + :param existing: the taxonomy's tags, from ``_existing_tags``. + """ + forest = _ParentForest(existing) + return all(forest.apply(action) for action in self.actions) + def generate_actions( self, tags: list[TagItem], @@ -244,6 +715,28 @@ def generate_actions( but then its parent needs to be updated because its parent is deleted. Those two actions should be merged. """ + existing = self._existing_tags() + self._generate_actions(tags, replace, existing, detach_first=False) + # A plan can have a valid end state while one of its steps is invalid, for example + # a deep tag that gets new children before it moves up. Such a plan is rebuilt with + # detach_first (see _generate_actions). If that is still invalid, no order of the + # actions works. + if not self.errors and not self._parent_changes_are_valid(existing): + self._generate_actions(tags, replace, existing, detach_first=True) + if not self._parent_changes_are_valid(existing): + self.errors.append(InvalidParentHierarchyError(TAXONOMY_MAX_DEPTH + 1)) + + def _generate_actions( + self, tags: list[TagItem], replace: bool, existing: ExistingTags, detach_first: bool, + ) -> None: + """ + Build the actions for ``tags``, with the rows ordered by ``_order_rows_by_parent_dependency``. + + With ``detach_first``, every existing tag whose parent changes is first moved to the + root (``_build_detach_actions``), and the rows are then ordered parent first + (``_order_rows_parent_first``). No step then builds a tree deeper or more cyclic than + the end state. + """ self.actions.clear() self.errors.clear() self._init_indexed_actions() @@ -286,12 +779,23 @@ def generate_actions( # Delete all not readed tags self._build_delete_actions(tags_for_delete) + if detach_first: + self._build_detach_actions(tags, existing) + # 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: + # Reject parent references that the per-row actions never validate. + self._validate_parent_id_not_stale(tags, existing) + self._validate_no_parent_cycles(tags, existing) + + if detach_first: + rows = self._order_rows_parent_first(tags, existing) + else: + rows = self._order_rows_by_parent_dependency(tags, existing) + for tag in rows: 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..c237ee31d 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -132,7 +132,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." ) ) diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index 65517f8e7..633945528 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -1,14 +1,20 @@ """ Test for import/export API """ +import csv +import itertools import json -from io import BytesIO +from collections.abc import Sequence +from contextlib import contextmanager +from io import BytesIO, StringIO +from django.db import transaction from django.test.testcases import TestCase import openedx_tagging.import_export.api as import_export_api from openedx_tagging.import_export import ParserFormat -from openedx_tagging.models import Tag, TagImportTask, TagImportTaskState, Taxonomy +from openedx_tagging.import_export.import_plan import TagItem +from openedx_tagging.models import ObjectTag, Tag, TagImportTask, TagImportTaskState, Taxonomy from .mixins import TestImportExportMixin @@ -51,6 +57,27 @@ def setUp(self) -> None: ) return super().setUp() + def _import_rows(self, rows: list[dict], replace: bool = False): + """ + Import `rows` as a JSON file and return (result, log), checking the + task log matches the last import log. + """ + result, task, _plan = import_export_api.import_tags( + self.taxonomy, + BytesIO(json.dumps({"tags": rows}).encode()), + self.parser_format, + replace=replace, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + return result, log + + def _snapshot(self) -> set: + """ + Return the (pk, external_id, parent pk) of every tag in the taxonomy. + """ + return set(self.taxonomy.tag_set.values_list("pk", "external_id", "parent_id")) + def test_check_status(self) -> None: TagImportTask.create(self.taxonomy) status = import_export_api.get_last_import_status(self.taxonomy) @@ -1052,6 +1079,417 @@ def test_import_rename_referencing_stale_old_id_rejected(self) -> None: assert not self.taxonomy.tag_set.filter(external_id="tag_50").exists() assert not self.taxonomy.tag_set.filter(external_id="tag_60").exists() + def test_import_existing_child_names_renamed_away_parent_rejected(self) -> None: + """ + An existing child that keeps its parent's pre-import id is rejected when the file + renames that parent, in either row order, and neither the taxonomy nor its export + changes. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_2 (parent_id tag_1) + """ + rename = {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"} + child = {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"} + snapshot_before = self._snapshot() + export_before = import_export_api.export_tags(self.taxonomy, self.parser_format) + for rows in ([rename, child], [child, rename]): + with self.subTest(first=rows[0]["id"]): + result, log = self._import_rows(rows) + assert not result + assert f"Row {rows.index(child) + 1}'s parent_id (tag_1) names an id this file renames away." in log + assert "Use 'tag_50' as the parent_id." in log + assert "Traceback" not in log + + assert self._snapshot() == snapshot_before + assert import_export_api.export_tags(self.taxonomy, self.parser_format) == export_before + + def test_import_child_and_renamed_parent_row_claiming_freed_id(self) -> None: + """ + With replace=True the file omits tag_1, which frees the id tag_1, and renames tag_3 + onto it. A child row naming tag_1 succeeds in either row order: the renamed tag keeps + its pk, the old tag_1 is gone, and the child is under the renamed tag. + + Before After + tag_1 tag_1 (was tag_3) + tag_2 tag_4 + tag_3 + tag_4 + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + child = {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"} + rename = {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"} + for rows in ([child, rename], [rename, child]): + with self.subTest(first=rows[0]["id"]), transaction.atomic(): + result, log = self._import_rows(rows, replace=True) + assert "Traceback" not in log + assert result + + assert Tag.objects.get(pk=pk_3).external_id == "tag_1" + assert not Tag.objects.filter(pk=pk_1).exists() + assert self.taxonomy.tag_set.get(external_id="tag_4").parent_id == pk_3 + transaction.set_rollback(True) + + def test_import_swap_with_children_in_any_row_order(self) -> None: + """ + Swapping the ids of two parents whose children name the end-state ids gives the same + result in every row order. Each child stays on the same physical parent. + + Before After + tag_1 tag_3 (was tag_1) + tag_2 tag_2 + tag_3 tag_1 (was tag_3) + tag_4 tag_4 + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + pk_3 = self.taxonomy.tag_set.get(external_id="tag_3").pk + pk_4 = self.taxonomy.tag_set.get(external_id="tag_4").pk + rows = [ + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_3"}, + {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_1"}, + ] + # Every one of the 24 row orders, because the swap and the children's references + # resolve differently depending on which rows come first. + for order in itertools.permutations(range(4)): + with self.subTest(order=order), transaction.atomic(): + result, log = self._import_rows([rows[i] for i in order], replace=True) + assert "Traceback" not in log + assert result + + assert Tag.objects.get(pk=pk_1).external_id == "tag_3" + assert Tag.objects.get(pk=pk_3).external_id == "tag_1" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + assert Tag.objects.get(pk=pk_4).parent_id == pk_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_id = {tag["id"]: tag for tag in exported_tags} + assert exported_by_id["tag_2"]["parent_id"] == "tag_3" + assert exported_by_id["tag_4"]["parent_id"] == "tag_1" + transaction.set_rollback(True) + + def test_import_swap_children_stale_ids_rejected_in_any_row_order(self) -> None: + """ + Children that name the stale pre-import ids of swapped parents are rejected in every + row order. Each error names the child's row number and its parent's new id, and the + taxonomy is unchanged. + + Before File rows + tag_1 tag_1 (previous_id tag_3) + tag_2 tag_3 (previous_id tag_1) + tag_3 tag_2 (parent_id tag_1) + tag_4 tag_4 (parent_id tag_3) + """ + rows = [ + {"id": "tag_1", "value": "Tag 3", "previous_id": "tag_3"}, + {"id": "tag_3", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_1"}, + {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_3"}, + ] + before = self._snapshot() + new_ids = {"tag_2": ("tag_1", "tag_3"), "tag_4": ("tag_3", "tag_1")} + # Every one of the 24 row orders, because each error must name the row's own number. + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + file_rows = [rows[i] for i in order] + result, log = self._import_rows(file_rows, replace=True) + assert not result + assert "Traceback" not in log + for position, row in enumerate(file_rows, start=1): + if row["id"] in new_ids: + stale_id, new_id = new_ids[row["id"]] + assert f"Row {position}'s parent_id ({stale_id}) names an id this file renames away." in log + assert f"Use '{new_id}' as the parent_id." in log + assert self._snapshot() == before + + def test_import_child_of_invalid_parent_row_rejected_in_any_order(self) -> None: + """ + A new child whose parent_id names a row that is itself rejected (a rename with an + unmatched previous_id, or a new tag with a duplicate value) is rejected cleanly in + either row order and leaves the taxonomy unchanged. + + File rows (two cases) + new_parent (previous_id tag_99) or new_parent (value "Tag 1", held by tag_1) + new_child (parent_id new_parent) + """ + cases = { + "unmatched previous_id": ( + {"id": "new_parent", "value": "New parent", "previous_id": "tag_99"}, + "Unknown previous_id (tag_99). No tag with that external_id exists in this taxonomy.", + ), + "duplicate value": ({"id": "new_parent", "value": "Tag 1"}, None), + } + child = {"id": "new_child", "value": "New child", "parent_id": "new_parent"} + before = self._snapshot() + for name, (parent, cause) in cases.items(): + for rows in ([parent, child], [child, parent]): + with self.subTest(case=name, first=rows[0]["id"]): + result, log = self._import_rows(rows) + assert not result + assert "Traceback" not in log + assert "Output errors" in log + if cause: + assert cause in log + assert self._snapshot() == before + + def test_import_cyclic_parent_ids_rejected(self) -> None: + """ + Two new tags naming each other as parent are rejected, with and without replace, + and nothing is created. The cycle is reported once, as an unknown parent, because + neither id exists before the import. + + File rows + new_a (parent_id new_b) + new_b (parent_id new_a) + """ + rows = [ + {"id": "new_a", "value": "New A", "parent_id": "new_b"}, + {"id": "new_b", "value": "New B", "parent_id": "new_a"}, + ] + before = self._snapshot() + for replace in (False, True): + with self.subTest(replace=replace): + result, log = self._import_rows(rows, replace=replace) + assert not result + assert log.count("Unknown parent tag") == 1 + assert "creates a parent cycle" not in log + assert "Traceback" not in log + assert self._snapshot() == before + + def test_import_child_of_new_row_with_value_freed_by_replace(self) -> None: + """ + With replace=True, a new tag may reuse the value of an existing tag that the file + omits, because the delete of the omitted tag frees the value. Its child is nested + under it in every row order. If the file keeps the existing tag, the same value is a + real duplicate and the import is rejected. + + Before After (file omits tag_1) + tag_1 new_parent (value "Tag 1") + tag_2 new_child + """ + new_parent = {"id": "new_parent", "value": "Tag 1"} + new_child = {"id": "new_child", "value": "New child", "parent_id": "new_parent"} + kept = {"id": "tag_1", "value": "Tag 1"} + before = self._snapshot() + for order in itertools.permutations(range(2)): + with self.subTest(case="omitted", order=order), transaction.atomic(): + result, log = self._import_rows([[new_parent, new_child][i] for i in order], replace=True) + assert "Traceback" not in log + assert result + parent = self.taxonomy.tag_set.get(external_id="new_parent") + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == parent.pk + transaction.set_rollback(True) + for order in itertools.permutations(range(3)): + with self.subTest(case="kept", order=order): + result, log = self._import_rows([[new_parent, new_child, kept][i] for i in order], replace=True) + assert not result + assert "Traceback" not in log + assert "Output errors" in log + assert self._snapshot() == before + + def test_import_longer_and_mixed_parent_cycles_rejected_in_any_row_order(self) -> None: + """ + A cycle of three new tags, and a cycle through a rename row, are rejected in every + row order and leave the taxonomy unchanged. Each cycle is reported once, as an + unknown parent, because none of its ids exists before the import. + + File rows (two cases) + new_a (parent_id new_b) tag_50 (previous_id tag_1, parent_id new_tag) + new_b (parent_id new_c) new_tag (parent_id tag_50) + new_c (parent_id new_a) + """ + cases = { + "three new tags": [ + {"id": "new_a", "value": "New A", "parent_id": "new_b"}, + {"id": "new_b", "value": "New B", "parent_id": "new_c"}, + {"id": "new_c", "value": "New C", "parent_id": "new_a"}, + ], + "rename row and new tag": [ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "new_tag"}, + {"id": "new_tag", "value": "New tag", "parent_id": "tag_50"}, + ], + } + before = self._snapshot() + for name, rows in cases.items(): + for order in itertools.permutations(range(len(rows))): + with self.subTest(case=name, order=order): + result, log = self._import_rows([rows[i] for i in order]) + assert not result + assert "Traceback" not in log + assert log.count("Unknown parent tag") == 1 + assert "creates a parent cycle" not in log + assert self._snapshot() == before + + def test_import_parent_cycles_through_existing_tags_rejected(self) -> None: + """ + A cycle that includes an existing tag is rejected in every row order and leaves the + taxonomy unchanged. The error is reported once, on the cycle's first row in the file. + + Before Cases (file rows) + tag_1 existing and new tag: tag_2 (parent_id new_tag), new_tag (parent_id tag_2) + tag_2 self parent: tag_2 (parent_id tag_2) + rename row and existing tag: tag_50 (previous_id tag_1, parent_id tag_2), + tag_2 (parent_id tag_50) + through a tag not in the file: tag_1 (parent_id tag_2) + """ + cases = { + "existing and new tag": [ + {"id": "tag_2", "value": "Tag 2", "parent_id": "new_tag"}, + {"id": "new_tag", "value": "New tag", "parent_id": "tag_2"}, + ], + "self parent": [{"id": "tag_2", "value": "Tag 2", "parent_id": "tag_2"}], + "rename row and existing tag": [ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1", "parent_id": "tag_2"}, + {"id": "tag_2", "value": "Tag 2", "parent_id": "tag_50"}, + ], + "through a tag not in the file": [{"id": "tag_1", "value": "Tag 1", "parent_id": "tag_2"}], + } + before = self._snapshot() + for name, rows in cases.items(): + for order in itertools.permutations(range(len(rows))): + with self.subTest(case=name, order=order): + file_rows = [rows[i] for i in order] + result, log = self._import_rows(file_rows) + assert not result + assert "Traceback" not in log + assert log.count("creates a parent cycle.") == 1 + assert f"Row 1's parent_id ({file_rows[0]['parent_id']}) creates a parent cycle." in log + assert self._snapshot() == before + + def test_import_existing_tag_moved_under_later_new_parent(self) -> None: + """ + An existing tag moved under a new parent whose row comes later in the file is + accepted, and the tag ends up under the new parent. + + Before After + tag_1 tag_1 + tag_2 new_parent + tag_2 + """ + result, log = self._import_rows([ + {"id": "tag_2", "value": "Tag 2", "parent_id": "new_parent"}, + {"id": "new_parent", "value": "New parent"}, + ]) + assert result + assert "Traceback" not in log + parent = self.taxonomy.tag_set.get(external_id="new_parent") + assert self.taxonomy.tag_set.get(external_id="tag_2").parent_id == parent.pk + + def test_import_rename_child_stale_parent_id_without_swap_reported_once(self) -> None: + """ + Without a swap, a renamed child that names its parent's stale id gets only the rename + action's error, once. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_20 (previous_id tag_2, parent_id tag_1) + """ + before = self._snapshot() + result, log = self._import_rows([ + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_1"}, + ]) + assert not result + assert "Traceback" not in log + assert log.count("Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id.") == 1 + assert "names an id this file renames away" not in log + assert self._snapshot() == before + + def test_import_case_only_parent_rename_keeps_child_parent(self) -> None: + """ + Renaming a parent only by case succeeds in either row order, and a child that names + the parent in any case stays under the same parent. + + Before After + tag_1 TAG_1 + tag_2 tag_2 (parent_id tag_1, TAG_1 or Tag_1) + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + rename = {"id": "TAG_1", "value": "Tag 1", "previous_id": "tag_1"} + for parent_id in ("tag_1", "TAG_1", "Tag_1"): + child = {"id": "tag_2", "value": "Tag 2", "parent_id": parent_id} + for rows in ([rename, child], [child, rename]): + with self.subTest(parent_id=parent_id, first=rows[0]["id"]), transaction.atomic(): + result, log = self._import_rows(rows) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "TAG_1" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + transaction.set_rollback(True) + + def test_import_new_child_before_new_parent_row(self) -> None: + """ + A new child row listed before its new parent row succeeds, the child is nested under + the new parent, and the export shows its parent_id. + + File rows After + new_child (parent_id new_parent) new_parent + new_parent new_child + """ + result, log = self._import_rows([ + {"id": "new_child", "value": "New child", "parent_id": "new_parent"}, + {"id": "new_parent", "value": "New parent"}, + ]) + assert result + assert "Traceback" not in log + parent = self.taxonomy.tag_set.get(external_id="new_parent") + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == parent.pk + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + exported_by_id = {tag["id"]: tag for tag in json.loads(output).get("tags")} + assert exported_by_id["new_child"]["parent_id"] == "new_parent" + + def test_import_child_first_references_plain_rename_new_id(self) -> None: + """ + A new child listed before the row that renames its parent, and naming the parent's + new id, succeeds and is nested under the renamed tag. + + Before After + tag_1 tag_50 (was tag_1) + tag_2 tag_2 + new_child + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + result, log = self._import_rows([ + {"id": "new_child", "value": "New child", "parent_id": "tag_50"}, + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + ]) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "tag_50" + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == pk_1 + + def test_import_plain_and_rename_children_of_renamed_parent_in_reverse_order(self) -> None: + """ + A new child and a renamed child, both naming the renamed parent's new id and listed + before the parent's rename row, succeed and end up under the renamed parent. + + Before After + tag_1 tag_50 (was tag_1) + tag_2 tag_20 (was tag_2) + new_child + """ + pk_1 = self.taxonomy.tag_set.get(external_id="tag_1").pk + pk_2 = self.taxonomy.tag_set.get(external_id="tag_2").pk + result, log = self._import_rows([ + {"id": "tag_20", "value": "Tag 2", "previous_id": "tag_2", "parent_id": "tag_50"}, + {"id": "new_child", "value": "New child", "parent_id": "tag_50"}, + {"id": "tag_50", "value": "Tag 1", "previous_id": "tag_1"}, + ]) + assert result + assert "Traceback" not in log + assert Tag.objects.get(pk=pk_1).external_id == "tag_50" + assert Tag.objects.get(pk=pk_2).external_id == "tag_20" + assert Tag.objects.get(pk=pk_2).parent_id == pk_1 + assert self.taxonomy.tag_set.get(external_id="new_child").parent_id == pk_1 + def test_import_same_value_without_external_id(self) -> None: new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() @@ -1072,3 +1510,965 @@ def test_import_same_value_without_external_id(self) -> None: replace=True, ) assert result + + +class TestImportAcceptanceScenarios(TestCase): + """ + The acceptance scenarios of #673 and #858, run the way the Studio import wizard runs an + import: a full replace import of a CSV or JSON file, checked through the import log and + the exported file. + + Rows are written as ``TagItem`` objects, whose fields are the file's columns. Some #673 + scenarios are covered by tests in ``TestImportExportApi`` and are not repeated here: + + - "re-import is idempotent (previous_id equals the new id)": + ``test_import_rename_external_id_previous_id_equals_id_is_noop``. + - "previous_id is present but matches no existing tag (likely typo)": + ``test_import_rename_external_id_unmatched_previous_id_rejected``. + - "a rename claims an id the same import frees by removing its holder": + ``test_import_rename_external_id_reuses_id_freed_by_replace_delete``, and + ``test_import_child_and_renamed_parent_row_claiming_freed_id`` for both row orders. + - "a child row referencing its parent's pre-import external_id is rejected": + ``test_import_existing_child_names_renamed_away_parent_rejected``. + - "a parent and its child are both renamed in one import": + ``test_import_rename_parent_and_child_both_renamed``. + + The #858 swap criteria are covered by ``test_import_swap_with_children_in_any_row_order`` + and ``test_import_swap_children_stale_ids_rejected_in_any_row_order``. + """ + + formats = (ParserFormat.JSON, ParserFormat.CSV) + taxonomy: Taxonomy + + @classmethod + def setUpTestData(cls) -> None: + super().setUpTestData() + cls.taxonomy = Taxonomy.objects.create(name="Competencies") + + def _render(self, rows: list[TagItem], parser_format: ParserFormat) -> BytesIO: + """ + Render ``rows`` as an import file of the given format, leaving out unset fields. + """ + file_rows = [ + { + key: value + for key, value in ( + ("id", row.id), + ("value", row.value), + ("parent_id", row.parent_id), + ("previous_id", row.previous_id), + ) + if value + } + for row in rows + ] + if parser_format == ParserFormat.JSON: + return BytesIO(json.dumps({"tags": file_rows}).encode()) + buffer = StringIO() + writer = csv.DictWriter(buffer, fieldnames=["id", "value", "parent_id", "previous_id"]) + writer.writeheader() + writer.writerows(file_rows) + return BytesIO(buffer.getvalue().encode()) + + def _import(self, rows: list[TagItem], parser_format: ParserFormat, plan_only: bool = False): + """ + Run a full replace import and return (result, log, plan). + """ + result, task, plan = import_export_api.import_tags( + self.taxonomy, + self._render(rows, parser_format), + parser_format, + replace=True, + plan_only=plan_only, + ) + log = import_export_api.get_last_import_log(self.taxonomy) + assert log == task.log + assert "Traceback" not in log + return result, log, plan + + def _seed(self, rows: list[TagItem]) -> dict[str, int]: + """ + Create the starting tags and return their pks by external_id. + """ + result, _log, _plan = self._import(rows, ParserFormat.JSON) + assert result + return {tag.external_id: tag.pk for tag in self.taxonomy.tag_set.all()} + + def _associate(self, pks: dict[str, int], *external_ids: str) -> dict[int, int]: + """ + Tag a content object with each tag and return the ObjectTag pks by tag pk. + """ + object_tags = {} + for external_id in external_ids: + tag = Tag.objects.get(pk=pks[external_id]) + object_tag = ObjectTag.objects.create( + object_id=f"object-{tag.pk}", taxonomy=self.taxonomy, tag=tag, _value=tag.value, + ) + object_tags[tag.pk] = object_tag.pk + return object_tags + + def _assert_associations_kept(self, object_tags: dict[int, int]) -> None: + """ + Check each ObjectTag still points to the same tag, not a recreated one. + """ + for tag_pk, object_tag_pk in object_tags.items(): + assert ObjectTag.objects.get(pk=object_tag_pk).tag_id == tag_pk + + def _export(self, parser_format: ParserFormat) -> str: + """ + Export the taxonomy in the given format. + """ + return import_export_api.export_tags(self.taxonomy, parser_format) + + def _exported_rows(self, parser_format: ParserFormat) -> list[dict]: + """ + Parse the export and check it only carries the id, value and parent_id fields. + """ + output = self._export(parser_format) + if parser_format == ParserFormat.JSON: + rows = json.loads(output)["tags"] + fields = {key for row in rows for key in row} + else: + reader = csv.DictReader(StringIO(output)) + assert reader.fieldnames == ["id", "value", "parent_id"] + rows = list(reader) + fields = set(reader.fieldnames) + assert fields <= {"id", "value", "parent_id"} + return [{"id": row["id"], "value": row["value"], "parent_id": row.get("parent_id") or None} for row in rows] + + def _exported_ids(self, parser_format: ParserFormat) -> dict[str, str]: + """ + Return the exported id of each tag, by tag value. + """ + return {row["value"]: row["id"] for row in self._exported_rows(parser_format)} + + def _exported_parents(self, parser_format: ParserFormat) -> dict[str, str | None]: + """ + Return the exported parent_id of each tag, by tag value. + """ + return {row["value"]: row["parent_id"] for row in self._exported_rows(parser_format)} + + @contextmanager + def _rolled_back(self): + """ + Run a block in a savepoint that is rolled back, so a scenario can be repeated. + """ + with transaction.atomic(): + yield + transaction.set_rollback(True) + + def _assert_rejected( + self, + rows: list[TagItem], + parser_format: ParserFormat, + *, + messages: Sequence[str], + error_count: int | None = None, + ) -> str: + """ + Check the import is rejected, its log has each of ``messages``, and nothing changed. + + With ``error_count``, also check the plan has exactly that many errors. + """ + snapshot = set(self.taxonomy.tag_set.values_list("pk", "external_id", "value", "parent_id")) + before = self._export(parser_format) + result, log, plan = self._import(rows, parser_format) + assert not result + for message in messages: + assert message in log + if error_count is not None: + assert plan is not None + assert len(plan.errors) == error_count + assert set(self.taxonomy.tag_set.values_list("pk", "external_id", "value", "parent_id")) == snapshot + assert self._export(parser_format) == before + return log + + def test_rename_shows_in_export_without_duplicate_or_previous_id(self) -> None: + """ + #673 scenarios "rename an external_id, verified via CSV export", "rename an + external_id, verified via JSON export" and "renaming an external_id preserves the + competency's content associations": the export shows the tag once, with its new id, + no row for the old id and no previous_id field, and the tag keeps its pk and its + content associations. + + Before After + COMP-A COMP-B (same tag) + """ + pks = self._seed([TagItem(id="COMP-A", value="Alpha")]) + object_tags = self._associate(pks, "COMP-A") + for parser_format in self.formats: + with self.subTest(parser_format=parser_format), self._rolled_back(): + count = len(self._exported_rows(parser_format)) + result, _log, _plan = self._import( + [TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A")], parser_format, + ) + assert result + assert [row["id"] for row in self._exported_rows(parser_format)] == ["COMP-B"] + assert len(self._exported_rows(parser_format)) == count + assert self.taxonomy.tag_set.count() == 1 + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" + self._assert_associations_kept(object_tags) + + def test_new_id_without_previous_id_is_created(self) -> None: + """ + #673 scenario "no previous_id, new id (create behavior preserved)": a row with a new + id and no previous_id creates a tag that shows in the export. + + Before After + COMP-A COMP-A + COMP-Z + """ + self._seed([TagItem(id="COMP-A", value="Alpha")]) + for parser_format in self.formats: + with self.subTest(parser_format=parser_format), self._rolled_back(): + result, _log, _plan = self._import( + [TagItem(id="COMP-A", value="Alpha"), TagItem(id="COMP-Z", value="Zulu")], parser_format, + ) + assert result + assert self._exported_ids(parser_format) == {"Alpha": "COMP-A", "Zulu": "COMP-Z"} + + def test_rename_onto_id_of_kept_tag_rejected(self) -> None: + """ + #673 scenarios "new id collides with a different tag's external_id in the same + taxonomy" and "a rename onto an id whose holder the same import keeps is rejected": + the rename is rejected in either row order, whether the file keeps the holder as a + plain row or as a row whose previous_id equals its id. + + Before File rows + COMP-Z COMP-Z (previous_id COMP-A) + COMP-A COMP-Z (the holder, kept) + """ + self._seed([TagItem(id="COMP-Z", value="Xray"), TagItem(id="COMP-A", value="Yank")]) + rename = TagItem(id="COMP-Z", value="Yank", previous_id="COMP-A") + keeps = { + "plain": TagItem(id="COMP-Z", value="Xray"), + "previous_id equal to id": TagItem(id="COMP-Z", value="Xray", previous_id="COMP-Z"), + } + for parser_format in self.formats: + for name, keep in keeps.items(): + for rows in ([rename, keep], [keep, rename]): + with self.subTest(parser_format=parser_format, keep=name, first=rows[0].value): + self._assert_rejected(rows, parser_format, messages=["COMP-Z"]) + + def test_swap_keeps_tags_and_associations(self) -> None: + """ + #673 scenario "two competencies swap external_ids in one import": the swap succeeds + in either row order, creates and deletes nothing, keeps both tags' pks and content + associations, and the export shows the swapped ids. + + Before After + COMP-A COMP-B (was COMP-A) + COMP-B COMP-A (was COMP-B) + """ + pks = self._seed([TagItem(id="COMP-A", value="Alpha"), TagItem(id="COMP-B", value="Beta")]) + object_tags = self._associate(pks, "COMP-A", "COMP-B") + rows = [ + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-A", value="Beta", previous_id="COMP-B"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert self.taxonomy.tag_set.count() == 2 + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" + assert Tag.objects.get(pk=pks["COMP-B"]).external_id == "COMP-A" + self._assert_associations_kept(object_tags) + assert self._exported_ids(parser_format) == {"Alpha": "COMP-B", "Beta": "COMP-A"} + + def test_three_way_rotation_in_any_row_order(self) -> None: + """ + #673 scenario "three competencies rotate external_ids in one import": the rotation + succeeds in all six row orders, and each tag holds the id its own row specified. + + Before After + COMP-A COMP-B (was COMP-A) + COMP-B COMP-C (was COMP-B) + COMP-C COMP-A (was COMP-C) + """ + pks = self._seed([ + TagItem(id="COMP-A", value="Alpha"), + TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-C", value="Gamma"), + ]) + rows = [ + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-C", value="Beta", previous_id="COMP-B"), + TagItem(id="COMP-A", value="Gamma", previous_id="COMP-C"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(3)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert Tag.objects.get(pk=pks["COMP-A"]).external_id == "COMP-B" + assert Tag.objects.get(pk=pks["COMP-B"]).external_id == "COMP-C" + assert Tag.objects.get(pk=pks["COMP-C"]).external_id == "COMP-A" + assert self._exported_ids(parser_format) == { + "Alpha": "COMP-B", "Beta": "COMP-C", "Gamma": "COMP-A", + } + + def test_rename_chain_colliding_with_kept_tag_rejected_whole(self) -> None: + """ + #673 scenario "a rename chain that collides with a competency the file keeps is + rejected whole": the import is rejected in all six row orders, with the id in the log + and all three tags unchanged. + + Before File rows + COMP-A COMP-B (previous_id COMP-A) + COMP-B COMP-C (previous_id COMP-B) + COMP-C COMP-C (kept) + """ + self._seed([ + TagItem(id="COMP-A", value="Alpha"), + TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-C", value="Gamma"), + ]) + rows = [ + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-C", value="Beta", previous_id="COMP-B"), + TagItem(id="COMP-C", value="Gamma"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(3)): + with self.subTest(parser_format=parser_format, order=order): + self._assert_rejected([rows[i] for i in order], parser_format, messages=["COMP-C"]) + + def test_child_names_new_id_of_renamed_parent(self) -> None: + """ + #673 scenario "a child row references its renamed parent's new external_id": the + import succeeds in either row order, the child stays under the same parent tag, and + the export shows it nested under the new id. + + Before After + COMP-P COMP-P2 (was COMP-P) + COMP-K COMP-K + """ + pks = self._seed([TagItem(id="COMP-P", value="Parent"), TagItem(id="COMP-K", value="Kid", parent_id="COMP-P")]) + rows = [ + TagItem(id="COMP-P2", value="Parent", previous_id="COMP-P"), + TagItem(id="COMP-K", value="Kid", parent_id="COMP-P2"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert Tag.objects.get(pk=pks["COMP-K"]).parent_id == pks["COMP-P"] + assert self._exported_ids(parser_format)["Parent"] == "COMP-P2" + assert self._exported_parents(parser_format)["Kid"] == "COMP-P2" + + def test_export_has_no_previous_id_and_round_trips_without_changes(self) -> None: + """ + #673 scenario "export does not carry previous_id (round-trip safety), CSV and JSON": + the export only has id, value and parent_id, and importing it back unchanged plans no + change. + + Before and after + COMP-P + COMP-K + COMP-A + """ + self._seed([ + TagItem(id="COMP-P", value="Parent"), + TagItem(id="COMP-K", value="Kid", parent_id="COMP-P"), + TagItem(id="COMP-A", value="Alpha"), + ]) + for parser_format in self.formats: + with self.subTest(parser_format=parser_format): + exported = self._exported_rows(parser_format) + assert len(exported) == 3 + result, _log, plan = self._import( + [TagItem(id=row["id"], value=row["value"], parent_id=row["parent_id"]) for row in exported], + parser_format, + plan_only=True, + ) + assert result + assert plan is not None + assert {action.name for action in plan.actions} == {"without_changes"} + + def test_every_rejected_row_is_in_the_one_log(self) -> None: + """ + Several rejected rows of different kinds each have their message in the same log, + because the wizard shows the log as one block (#673). The row numbers count data + rows, so they are the same in JSON and CSV. + + Before File rows + COMP-P 1: COMP-P2 (previous_id COMP-P) + COMP-K1 2: COMP-K1 (parent_id COMP-P, stale) + COMP-K2 3: COMP-K2 (parent_id COMP-P, stale) + COMP-A 4: COMP-B (previous_id COMP-GHOST, unmatched) + """ + self._seed([ + TagItem(id="COMP-P", value="Parent"), + TagItem(id="COMP-K1", value="Kid 1", parent_id="COMP-P"), + TagItem(id="COMP-K2", value="Kid 2", parent_id="COMP-P"), + TagItem(id="COMP-A", value="Alpha"), + ]) + rows = [ + TagItem(id="COMP-P2", value="Parent", previous_id="COMP-P"), + TagItem(id="COMP-K1", value="Kid 1", parent_id="COMP-P"), + TagItem(id="COMP-K2", value="Kid 2", parent_id="COMP-P"), + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-GHOST"), + ] + for parser_format in self.formats: + with self.subTest(parser_format=parser_format): + self._assert_rejected( + rows, + parser_format, + messages=[ + "Row 2's parent_id (COMP-P) names an id this file renames away.", + "Row 3's parent_id (COMP-P) names an id this file renames away.", + "COMP-GHOST", + ], + ) + + def test_rename_onto_freed_id_forming_parent_cycle_rejected(self) -> None: + """ + A rename onto an id that the same import frees can close a parent cycle through the + renamed tag. Both cases below are rejected at the Plan step with exactly one error, + the cycle error on the cycle's first row, in either row order. + + Self parent: + Before File rows + COMP-X COMP-X (previous_id COMP-Y, parent_id COMP-X) + COMP-Y + + Two renames naming each other as parent: + Before File rows + COMP-A COMP-A (previous_id COMP-C, parent_id COMP-B) + COMP-B COMP-B (previous_id COMP-D, parent_id COMP-A) + COMP-C + COMP-D + """ + self._seed([TagItem(id="COMP-X", value="Xray"), TagItem(id="COMP-Y", value="Yank")]) + self_parent = [TagItem(id="COMP-X", value="Yank", parent_id="COMP-X", previous_id="COMP-Y")] + for parser_format in self.formats: + with self.subTest(parser_format=parser_format, case="self parent"): + self._assert_rejected( + self_parent, + parser_format, + messages=["Row 1's parent_id (COMP-X) creates a parent cycle."], + error_count=1, + ) + self._seed([ + TagItem(id="COMP-A", value="Alpha"), TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-C", value="Gamma"), TagItem(id="COMP-D", value="Delta"), + ]) + rows = [ + TagItem(id="COMP-A", value="Gamma", parent_id="COMP-B", previous_id="COMP-C"), + TagItem(id="COMP-B", value="Delta", parent_id="COMP-A", previous_id="COMP-D"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order, case="two renames"): + file_rows = [rows[i] for i in order] + self._assert_rejected( + file_rows, + parser_format, + messages=[f"Row 1's parent_id ({file_rows[0].parent_id}) creates a parent cycle."], + error_count=1, + ) + + def test_child_of_deleted_holder_moves_under_tag_renamed_onto_its_id(self) -> None: + """ + The file omits COMP-X, so COMP-X is deleted, and renames COMP-Y onto the id COMP-X. + The child of the deleted tag names COMP-X, so it ends up under the renamed tag, not + as a root, in either row order. + + Before After + COMP-X COMP-X (was COMP-Y) + COMP-C COMP-C + COMP-Y + """ + pks = self._seed([ + TagItem(id="COMP-X", value="Xray"), + TagItem(id="COMP-Y", value="Yank"), + TagItem(id="COMP-C", value="Child", parent_id="COMP-X"), + ]) + object_tags = self._associate(pks, "COMP-C") + rows = [ + TagItem(id="COMP-X", value="Yank", previous_id="COMP-Y"), + TagItem(id="COMP-C", value="Child", parent_id="COMP-X"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order], parser_format) + assert result + assert not Tag.objects.filter(pk=pks["COMP-X"]).exists() + assert Tag.objects.get(pk=pks["COMP-C"]).parent_id == pks["COMP-Y"] + assert Tag.objects.get(pk=pks["COMP-Y"]).external_id == "COMP-X" + self._assert_associations_kept(object_tags) + assert self._exported_parents(parser_format) == {"Yank": None, "Child": "COMP-X"} + + def test_kept_tags_under_deleted_ancestors_become_roots(self) -> None: + """ + A kept tag whose parent the import deletes becomes a root, also when that parent sits + under a deleted ancestor, whose delete would otherwise cascade to the kept tag. + + Before (three cases) After + deleted_root > deleted_child > kept_tag kept_tag + deleted_1 > deleted_2 > kept_upper > deleted_3 kept_upper + > kept_lower kept_lower + deleted_root > kept_child kept_child + """ + cases = { + "grandchild of a deleted chain": ( + [ + TagItem(id="deleted_root", value="Deleted root"), + TagItem(id="deleted_child", value="Deleted child", parent_id="deleted_root"), + TagItem(id="kept_tag", value="Kept tag", parent_id="deleted_child"), + ], + ["kept_tag"], + ), + "kept tags at two depths": ( + [ + TagItem(id="deleted_1", value="Deleted 1"), + TagItem(id="deleted_2", value="Deleted 2", parent_id="deleted_1"), + TagItem(id="kept_upper", value="Kept upper", parent_id="deleted_2"), + TagItem(id="deleted_3", value="Deleted 3", parent_id="kept_upper"), + TagItem(id="kept_lower", value="Kept lower", parent_id="deleted_3"), + ], + ["kept_upper", "kept_lower"], + ), + "child of a deleted root": ( + [ + TagItem(id="deleted_root", value="Deleted root"), + TagItem(id="kept_child", value="Kept child", parent_id="deleted_root"), + ], + ["kept_child"], + ), + } + for name, (before, kept_ids) in cases.items(): + # The file lists only the kept tags, without a parent. + kept = [TagItem(id=tag.id, value=tag.value) for tag in before if tag.id in kept_ids] + for parser_format in self.formats: + with self.subTest(case=name, parser_format=parser_format), self._rolled_back(): + pks = self._seed(before) + result, _log, _plan = self._import(kept, parser_format) + assert result + assert set(self.taxonomy.tag_set.values_list("pk", flat=True)) == {pks[t.id] for t in kept} + assert all(tag.parent_id is None for tag in self.taxonomy.tag_set.all()) + assert self._exported_parents(parser_format) == {t.value: None for t in kept} + + def _assert_tree_after_import( + self, + *, + rows: list[TagItem], + expected_parents: dict[str, str | None], + pks: dict[str, int], + orders: Sequence[tuple[int, ...]], + parser_formats: tuple[ParserFormat, ...], + static_rows: tuple[TagItem, ...] = (), + ) -> None: + """ + Import ``rows`` in each of ``orders`` and each of ``parser_formats``, followed by + ``static_rows`` (the other tags that the replace import has to keep). Check that every + tag in ``expected_parents`` (external_id to parent external_id) ends up under that + parent in the database and the export, and that each tag in ``pks`` (external_id + after the import to pk) keeps its pk. Each import is rolled back. + """ + for parser_format in parser_formats: + for order in orders: + with self.subTest(parser_format=parser_format, order=order), self._rolled_back(): + result, _log, _plan = self._import([rows[i] for i in order] + list(static_rows), parser_format) + assert result + for tag_id, pk in pks.items(): + assert Tag.objects.get(pk=pk).external_id == tag_id + db_parents = { + tag.external_id: tag.parent.external_id if tag.parent else None + for tag in self.taxonomy.tag_set.select_related("parent") + } + assert db_parents == expected_parents + exported = {row["id"]: row["parent_id"] for row in self._exported_rows(parser_format)} + assert exported == expected_parents + + def test_ancestry_inversion_in_any_row_order(self) -> None: + """ + A child of old_root becomes old_root's ancestor, with a new tag between them. That + only works if new_root leaves old_root's subtree before old_root moves under it. + + Before After + old_root new_root + sibling new_middle + new_root old_root + sibling + """ + pks = self._seed([ + TagItem(id="old_root", value="Old root"), + TagItem(id="sibling", value="Sibling", parent_id="old_root"), + TagItem(id="new_root", value="New root", parent_id="old_root"), + ]) + rows = [ + TagItem(id="old_root", value="Old root", parent_id="new_middle"), + TagItem(id="sibling", value="Sibling", parent_id="new_middle"), + TagItem(id="new_root", value="New root"), + TagItem(id="new_middle", value="New middle", parent_id="new_root"), + ] + expected = {"old_root": "new_middle", "sibling": "new_middle", "new_root": None, "new_middle": "new_root"} + orders = list(itertools.permutations(range(4))) + # All 24 row orders in JSON. The CSV parser yields the same rows in the same order, so + # 4 orders in CSV are enough and keep the test fast. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders, parser_formats=(ParserFormat.JSON,), + ) + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders[::6], parser_formats=(ParserFormat.CSV,), + ) + + def test_parent_and_child_swap_places(self) -> None: + """ + A parent and its child swap places, in either row order. + + Before After + old_parent old_child + old_child old_parent + """ + pks = self._seed([ + TagItem(id="old_parent", value="Old parent"), + TagItem(id="old_child", value="Old child", parent_id="old_parent"), + ]) + rows = [ + TagItem(id="old_parent", value="Old parent", parent_id="old_child"), + TagItem(id="old_child", value="Old child"), + ] + self._assert_tree_after_import( + rows=rows, + expected_parents={"old_parent": "old_child", "old_child": None}, + pks=pks, + orders=list(itertools.permutations(range(2))), + parser_formats=self.formats, + ) + + def test_three_level_chain_reversed(self) -> None: + """ + A three-level chain is turned upside down, in all six row orders. + + Before After + top bottom + middle middle + bottom top + """ + pks = self._seed([ + TagItem(id="top", value="Top"), + TagItem(id="middle", value="Middle", parent_id="top"), + TagItem(id="bottom", value="Bottom", parent_id="middle"), + ]) + rows = [ + TagItem(id="bottom", value="Bottom"), + TagItem(id="middle", value="Middle", parent_id="bottom"), + TagItem(id="top", value="Top", parent_id="middle"), + ] + self._assert_tree_after_import( + rows=rows, + expected_parents={"bottom": None, "middle": "bottom", "top": "middle"}, + pks=pks, + orders=list(itertools.permutations(range(3))), + parser_formats=self.formats, + ) + + def test_tag_moved_under_its_former_grandchild(self) -> None: + """ + A root is moved under its former grandchild, which stays under middle, now a root, + in all six row orders. + + Before After + top middle + middle bottom + bottom top + """ + pks = self._seed([ + TagItem(id="top", value="Top"), + TagItem(id="middle", value="Middle", parent_id="top"), + TagItem(id="bottom", value="Bottom", parent_id="middle"), + ]) + rows = [ + TagItem(id="top", value="Top", parent_id="bottom"), + TagItem(id="middle", value="Middle"), + TagItem(id="bottom", value="Bottom", parent_id="middle"), + ] + self._assert_tree_after_import( + rows=rows, + expected_parents={"top": "bottom", "middle": None, "bottom": "middle"}, + pks=pks, + orders=list(itertools.permutations(range(3))), + parser_formats=self.formats, + ) + + # The chain level_1 > level_2 > level_3 > level_4 > deep_tag: deep_tag is at the fifth + # level, so a child of deep_tag is at the sixth level, the deepest the taxonomy allows. + deep_chain = ( + TagItem(id="level_1", value="Level 1"), + TagItem(id="level_2", value="Level 2", parent_id="level_1"), + TagItem(id="level_3", value="Level 3", parent_id="level_2"), + TagItem(id="level_4", value="Level 4", parent_id="level_3"), + ) + deep_tag = TagItem(id="deep_tag", value="Deep tag", parent_id="level_4") + + def test_deep_tag_moved_to_root_with_new_children_in_any_row_order(self) -> None: + """ + The deepest tag of a chain moves to the root while new tags are put under it. + Creating new_grandchild before deep_tag moves would put it at a seventh level, so the + import moves deep_tag to the root first. + + Before After + level_1 > ... > level_4 level_1 > ... > level_4 + deep_tag deep_tag + new_child + new_grandchild + """ + pks = self._seed([*self.deep_chain, self.deep_tag]) + rows = [ + TagItem(id="new_grandchild", value="New grandchild", parent_id="new_child"), + TagItem(id="new_child", value="New child", parent_id="deep_tag"), + TagItem(id="deep_tag", value="Deep tag"), + ] + expected = { + "level_1": None, "level_2": "level_1", "level_3": "level_2", "level_4": "level_3", + "deep_tag": None, "new_child": "deep_tag", "new_grandchild": "new_child", + } + orders = list(itertools.permutations(range(3))) + # All 6 row orders in JSON. The CSV parser yields the same rows in the same order, so + # 3 orders in CSV are enough and keep the test fast. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders, + parser_formats=(ParserFormat.JSON,), static_rows=self.deep_chain, + ) + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders[::2], + parser_formats=(ParserFormat.CSV,), static_rows=self.deep_chain, + ) + + def test_deep_tag_under_renamed_parent_moved_to_root_in_any_row_order(self) -> None: + """ + The same move as above while deep_tag's old parent is renamed in the same import. + deep_tag keeps its pk. + + Before After + level_1 > level_2 > level_3 level_1 > level_2 > level_3 + level_4 renamed_level_4 (was level_4) + deep_tag deep_tag + new_child + new_grandchild + """ + pks = self._seed([*self.deep_chain, self.deep_tag]) + rows = [ + TagItem(id="new_grandchild", value="New grandchild", parent_id="new_child"), + TagItem(id="new_child", value="New child", parent_id="deep_tag"), + TagItem(id="deep_tag", value="Deep tag"), + TagItem(id="renamed_level_4", value="Level 4", parent_id="level_3", previous_id="level_4"), + ] + expected = { + "level_1": None, "level_2": "level_1", "level_3": "level_2", "renamed_level_4": "level_3", + "deep_tag": None, "new_child": "deep_tag", "new_grandchild": "new_child", + } + kept_pks = {tag_id: pk for tag_id, pk in pks.items() if tag_id != "level_4"} | { + "renamed_level_4": pks["level_4"], + } + # All 24 row orders, in JSON only: the test above already runs this kind of move + # through the CSV parser. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=kept_pks, orders=list(itertools.permutations(range(4))), + parser_formats=(ParserFormat.JSON,), static_rows=self.deep_chain[:3], + ) + + def test_subtree_moved_under_deep_tag_before_it_is_shallowed(self) -> None: + """ + A three-level subtree moves under moved_up, which itself moves to the root. Attaching + subtree_root while moved_up is still at the fourth level would put + subtree_grandchild at a seventh level. + + Before After + chain_1 > chain_2 > chain_3 chain_1 > chain_2 > chain_3 + moved_up moved_up + subtree_root subtree_root + subtree_child subtree_child + subtree_grandchild subtree_grandchild + """ + chain = ( + TagItem(id="chain_1", value="Chain 1"), + TagItem(id="chain_2", value="Chain 2", parent_id="chain_1"), + TagItem(id="chain_3", value="Chain 3", parent_id="chain_2"), + ) + pks = self._seed([ + *chain, + TagItem(id="moved_up", value="Moved up", parent_id="chain_3"), + TagItem(id="subtree_root", value="Subtree root"), + TagItem(id="subtree_child", value="Subtree child", parent_id="subtree_root"), + TagItem(id="subtree_grandchild", value="Subtree grandchild", parent_id="subtree_child"), + ]) + rows = [ + TagItem(id="subtree_root", value="Subtree root", parent_id="moved_up"), + TagItem(id="moved_up", value="Moved up"), + TagItem(id="subtree_child", value="Subtree child", parent_id="subtree_root"), + TagItem(id="subtree_grandchild", value="Subtree grandchild", parent_id="subtree_child"), + ] + expected = { + "chain_1": None, "chain_2": "chain_1", "chain_3": "chain_2", "moved_up": None, + "subtree_root": "moved_up", "subtree_child": "subtree_root", "subtree_grandchild": "subtree_child", + } + orders = list(itertools.permutations(range(4))) + # All 24 row orders in JSON. The CSV parser yields the same rows in the same order, so + # 4 orders in CSV are enough and keep the test fast. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders, + parser_formats=(ParserFormat.JSON,), static_rows=chain, + ) + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=orders[::6], + parser_formats=(ParserFormat.CSV,), static_rows=chain, + ) + + def test_three_new_levels_under_moved_deep_tag(self) -> None: + """ + Three new tags are chained under deep_tag, which moves to the root. + + Before After + level_1 > ... > level_4 level_1 > ... > level_4 + deep_tag deep_tag + new_child + new_grandchild + new_great_grandchild + """ + pks = self._seed([*self.deep_chain, self.deep_tag]) + rows = [ + TagItem(id="deep_tag", value="Deep tag"), + TagItem(id="new_child", value="New child", parent_id="deep_tag"), + TagItem(id="new_grandchild", value="New grandchild", parent_id="new_child"), + TagItem(id="new_great_grandchild", value="New great-grandchild", parent_id="new_grandchild"), + ] + expected = { + "level_1": None, "level_2": "level_1", "level_3": "level_2", "level_4": "level_3", "deep_tag": None, + "new_child": "deep_tag", "new_grandchild": "new_child", "new_great_grandchild": "new_grandchild", + } + # All 24 row orders, in JSON only: the moves of a deep tag above already run through + # the CSV parser. + self._assert_tree_after_import( + rows=rows, expected_parents=expected, pks=pks, orders=list(itertools.permutations(range(4))), + parser_formats=(ParserFormat.JSON,), static_rows=self.deep_chain, + ) + + def test_import_that_leaves_tags_too_deep_is_rejected(self) -> None: + """ + An import that would put a tag at the seventh level cannot be applied in any order, + so it is rejected at the Plan step with that one error, and nothing changes. + + Child of the deepest tag: + Before File rows + level_1 > ... > level_6 the same six tags, and new_leaf (parent_id level_6) + + Chain moved under another: + Before File rows + deep_1 > ... > deep_4 the same eight tags, with moved_1 (parent_id deep_4) + moved_1 > ... > moved_4 + """ + def chain(prefix: str, length: int) -> list[TagItem]: + return [ + TagItem(id=f"{prefix}_{i}", value=f"{prefix} {i}", parent_id=f"{prefix}_{i - 1}" if i > 1 else None) + for i in range(1, length + 1) + ] + + six_levels = chain("level", 6) + deep, moved = chain("deep", 4), chain("moved", 4) + cases = { + "child of the deepest tag": ( + six_levels, six_levels + [TagItem(id="new_leaf", value="New leaf", parent_id="level_6")], + ), + "chain moved under another": ( + deep + moved, deep + [TagItem(id="moved_1", value="moved 1", parent_id="deep_4")] + moved[1:], + ), + } + message = "This import would leave tags deeper than 6 levels" + for name, (before, rows) in cases.items(): + self._seed(before) + for parser_format in self.formats: + with self.subTest(case=name, parser_format=parser_format): + log = self._assert_rejected(rows, parser_format, messages=[message], error_count=1) + assert log.count(message) == 1 + self.taxonomy.tag_set.all().delete() + + def test_swap_child_naming_parent_id_in_another_case(self) -> None: + """ + Ids are case-insensitive. In a swap, a child naming its parent's stale id in another + case is rejected once, with its row number and the parent's new id, like the exact + spelling. Children naming the end-state ids in another case are accepted and stay + under their parents. + + Before After (accepted file) + COMP-A COMP-B (was COMP-A) + COMP-AK COMP-AK (parent_id comp-b) + COMP-B COMP-A (was COMP-B) + COMP-BK COMP-BK (parent_id comp-a) + + In the rejected file, COMP-AK says parent_id comp-a, the stale id of its parent. + """ + pks = self._seed([ + TagItem(id="COMP-A", value="Alpha"), + TagItem(id="COMP-AK", value="Alpha kid", parent_id="COMP-A"), + TagItem(id="COMP-B", value="Beta"), + TagItem(id="COMP-BK", value="Beta kid", parent_id="COMP-B"), + ]) + swap = [ + TagItem(id="COMP-B", value="Alpha", previous_id="COMP-A"), + TagItem(id="COMP-A", value="Beta", previous_id="COMP-B"), + ] + stale = swap + [ + TagItem(id="COMP-AK", value="Alpha kid", parent_id="comp-a"), + TagItem(id="COMP-BK", value="Beta kid", parent_id="comp-a"), + ] + accepted = swap + [ + TagItem(id="COMP-AK", value="Alpha kid", parent_id="comp-b"), + TagItem(id="COMP-BK", value="Beta kid", parent_id="comp-a"), + ] + expected = {"COMP-B": None, "COMP-A": None, "COMP-AK": "COMP-B", "COMP-BK": "COMP-A"} + moved_pks = { + "COMP-B": pks["COMP-A"], "COMP-A": pks["COMP-B"], "COMP-AK": pks["COMP-AK"], "COMP-BK": pks["COMP-BK"], + } + # A sample of 4 row orders (renames first, children first, reversed, mixed): the + # tests of the exact spelling already run all 24 orders. + orders = [(0, 1, 2, 3), (2, 3, 0, 1), (3, 2, 1, 0), (2, 0, 3, 1)] + self._assert_tree_after_import( + rows=accepted, expected_parents=expected, pks=moved_pks, orders=orders, parser_formats=self.formats, + ) + for parser_format in self.formats: + for order in orders: + with self.subTest(parser_format=parser_format, order=order, stale=True): + file_rows = [stale[i] for i in order] + position = file_rows.index(stale[2]) + 1 + log = self._assert_rejected( + file_rows, + parser_format, + messages=[ + f"Row {position}'s parent_id (comp-a) names an id this file renames away.", + "Use 'COMP-B' as the parent_id.", + ], + ) + assert log.count("names an id this file renames away") == 1 + + def test_plain_rename_child_naming_parent_id_in_another_case_reported_once(self) -> None: + """ + When the parent is renamed without a swap, a child naming the parent's old id in + another case gets only the parent validation error, in either row order and format. + + Before File rows + COMP-P COMP-P2 (previous_id COMP-P) + COMP-K COMP-K (parent_id comp-p) + """ + self._seed([TagItem(id="COMP-P", value="Parent"), TagItem(id="COMP-K", value="Kid", parent_id="COMP-P")]) + rows = [ + TagItem(id="COMP-P2", value="Parent", previous_id="COMP-P"), + TagItem(id="COMP-K", value="Kid", parent_id="comp-p"), + ] + for parser_format in self.formats: + for order in itertools.permutations(range(2)): + with self.subTest(parser_format=parser_format, order=order): + log = self._assert_rejected( + [rows[i] for i in order], + parser_format, + messages=[ + "Parent tag comp-p is renamed to COMP-P2 in this file; use COMP-P2 as the parent_id.", + ], + ) + assert "names an id this file renames away" not in log diff --git a/tests/openedx_tagging/import_export/test_import_plan.py b/tests/openedx_tagging/import_export/test_import_plan.py index 9fd40b074..b21fd8706 100644 --- a/tests/openedx_tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/import_export/test_import_plan.py @@ -1,12 +1,18 @@ """ Test for import_plan functions """ +import itertools +from unittest import mock + import ddt # type: ignore[import] +from django.db import connection from django.test.testcases import TestCase +from django.test.utils import CaptureQueriesContext -from openedx_tagging.import_export.actions import CreateTag +from openedx_tagging.import_export.actions import CreateTag, DeleteTag, UpdateParentTag from openedx_tagging.import_export.exceptions import TagImportError -from openedx_tagging.import_export.import_plan import TagImportPlan, TagItem +from openedx_tagging.import_export.import_plan import ExistingTag, TagImportPlan, TagItem, _ParentForest +from openedx_tagging.models import Tag, Taxonomy from .test_actions import TestImportActionMixin @@ -17,6 +23,13 @@ class TestTagImportPlan(TestImportActionMixin, TestCase): Test for import plan functions """ + structure_taxonomy: Taxonomy + + @classmethod + def setUpTestData(cls) -> None: + super().setUpTestData() + cls.structure_taxonomy = Taxonomy.objects.create(name="Structure taxonomy") + def setUp(self) -> None: super().setUp() self.import_plan = TagImportPlan(self.taxonomy) @@ -181,6 +194,10 @@ def test_build_delete_actions(self) -> None: True, 0, [ + { + 'name': 'update_parent', + 'id': 'tag_4', + }, { 'name': 'delete', 'id': 'tag_1', @@ -189,10 +206,6 @@ def test_build_delete_actions(self) -> None: 'name': 'delete', 'id': 'tag_2', }, - { - 'name': 'update_parent', - 'id': 'tag_4', - }, { 'name': 'delete', 'id': 'tag_3', @@ -265,7 +278,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "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" + "Add a row for the parent to 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" ), @@ -319,10 +332,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 +632,476 @@ 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_names_renamed_away_parent_rejected(self) -> None: + """ + An existing child that keeps its parent's pre-import id is rejected when the file + renames that parent, and the error names the row and the parent's new id. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_2 (parent_id tag_1) + + The tag_2 row is "no change" for UpdateParentTag, so only the plan-level check can + reject it. + """ + 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(len(self.import_plan.errors), 1) + self.assertIn("Row 2's parent_id (tag_1)", str(self.import_plan.errors[0])) + self.assertIn("Use 'tag_50' as the parent_id.", str(self.import_plan.errors[0])) + + def test_generate_actions_swap_rename_child_stale_parent_id_rejected_once(self) -> None: + """ + In a swap, a child that is itself renamed and names its parent's stale id is reported + exactly once. + + Before File rows + tag_1 tag_3 (previous_id tag_1) + tag_2 tag_1 (previous_id tag_3) + tag_3 tag_20 (previous_id tag_2, parent_id tag_1) + + The rename action's own parent validation accepts "tag_1", because it is also another + rename row's final id, so the plan-level check reports it. + """ + 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_20', value='Tag 2', previous_id='tag_2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn("Row 3's parent_id (tag_1)", str(self.import_plan.errors[0])) + + def test_generate_actions_rename_child_stale_parent_id_without_swap_not_duplicated(self) -> None: + """ + Without a swap, a renamed child that names its parent's stale id is reported only by + the rename action's parent validation, not a second time by the plan-level check. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_20 (previous_id tag_2, parent_id tag_1) + """ + tags = [ + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_20', value='Tag 2', previous_id='tag_2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(self.import_plan.errors), 1) + self.assertIn( + "Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id.", + str(self.import_plan.errors[0]), + ) + + def test_generate_actions_case_only_parent_rename_not_flagged(self) -> None: + """ + A rename that only changes the parent's capitalization keeps the id pointing at the + same tag, so a child that keeps parent_id "tag_1" is accepted. + + Before After + tag_1 TAG_1 + tag_2 tag_2 + """ + tags = [ + TagItem(id='TAG_1', 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(self.import_plan.errors, []) + + def test_generate_actions_swap_children_accepted_in_any_row_order(self) -> None: + """ + Children that name the end-state ids of swapped parents are accepted in every row + order. Each child stays on the same physical parent. + + Before After + tag_1 tag_3 (was tag_1) + tag_2 tag_2 + tag_3 tag_1 (was tag_3) + tag_4 tag_4 + """ + rows = [ + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_3'), + TagItem(id='tag_4', value='Tag 4', parent_id='tag_1'), + ] + # Planning is fast, so every one of the 24 row orders is checked. + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + self.import_plan.generate_actions(tags=[rows[i] for i in order], replace=False) + self.assertEqual(self.import_plan.errors, []) + + def test_generate_actions_swap_stale_ids_rejected_in_any_row_order(self) -> None: + """ + Children that name the stale pre-import ids of swapped parents are rejected in every + row order. Each error names the child's row number and its parent's new id. + + Before File rows + tag_1 tag_1 (previous_id tag_3) + tag_2 tag_3 (previous_id tag_1) + tag_3 tag_2 (parent_id tag_1) + tag_4 tag_4 (parent_id tag_3) + """ + rows = [ + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_3', value='Tag 1', previous_id='tag_1'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + TagItem(id='tag_4', value='Tag 4', parent_id='tag_3'), + ] + # Planning is fast, so every one of the 24 row orders is checked. + for order in itertools.permutations(range(4)): + with self.subTest(order=order): + file_rows = [rows[i] for i in order] + self.import_plan.generate_actions(tags=file_rows, replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 2) + new_ids = {'tag_1': 'tag_3', 'tag_3': 'tag_1'} + for position, row in enumerate(file_rows, start=1): + if row.parent_id: + prefix = f"Row {position}'s parent_id ({row.parent_id})" + matching = [m for m in messages if m.startswith(prefix)] + self.assertEqual(len(matching), 1) + self.assertIn(f"Use '{new_ids[row.parent_id]}' as the parent_id.", matching[0]) + + def test_generate_actions_valid_row_order_keeps_action_order(self) -> None: + """ + A file whose parents already come before their children keeps its row order in the + plan. + + Before After + tag_1 new_parent + new_child + tag_50 (was tag_1) + renamed_tag_child + """ + tags = [ + TagItem(id='new_parent', value='New parent'), + TagItem(id='new_child', value='New child', parent_id='new_parent'), + TagItem(id='tag_50', value='Tag 1', previous_id='tag_1'), + TagItem(id='renamed_tag_child', value='Renamed tag child', parent_id='tag_50'), + ] + 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], + ['new_parent', 'new_child', 'tag_50', 'renamed_tag_child'], + ) + + def test_generate_actions_deep_reversed_chain_terminates(self) -> None: + """ + A 1100-row chain of new tags listed child first is ordered without hitting the + recursion limit, and every row is planned. The chain is far too deep to apply, so the + plan reports it. + + File rows: n1099 (parent_id n1098), ..., n1 (parent_id n0), n0 + """ + 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(len(self.import_plan.errors), 1) + self.assertIn('deeper than 6 levels', str(self.import_plan.errors[0])) + + def test_plan_validations_do_not_query_per_row(self) -> None: + """ + The row ordering and the plan-level validations add no queries, however many rows + have a parent_id: the plan costs the same with them replaced by no-ops. + + File rows: 200 new tags, every second one under tag_1 and the others under the new + tag before them. + """ + tags = [ + TagItem(id=f'n{i}', value=f'N{i}', parent_id='tag_1' if i % 2 == 0 else f'n{i - 1}') + for i in range(200) + ] + with CaptureQueriesContext(connection) as real: + self.import_plan.generate_actions(tags=tags, replace=False) + with ( + mock.patch.object(TagImportPlan, '_validate_parent_id_not_stale'), + mock.patch.object(TagImportPlan, '_validate_no_parent_cycles'), + mock.patch.object( + TagImportPlan, '_order_rows_by_parent_dependency', side_effect=lambda rows, existing: list(rows), + ), + CaptureQueriesContext(connection) as without_validations, + ): + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(len(real), len(without_validations)) + + def test_generate_actions_three_level_chain_defined_later_accepted(self) -> None: + """ + Three new tags listed child first are created parent first. + + File rows After + new_child (parent_id new_parent) new_grandparent + new_parent (parent_id new_grandparent) new_parent + new_grandparent new_child + """ + tags = [ + TagItem(id='new_child', value='New child', parent_id='new_parent'), + TagItem(id='new_parent', value='New parent', parent_id='new_grandparent'), + TagItem(id='new_grandparent', value='New grandparent'), + ] + 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], + ['new_grandparent', 'new_parent', 'new_child'], + ) + + def test_generate_actions_existing_tag_moved_under_later_new_parent_accepted(self) -> None: + """ + An existing tag moved under a new parent whose row comes later in the file is + accepted, and the parent is created first. + + Before After + tag_1 tag_1 + tag_2 new_parent + tag_2 + """ + tags = [ + TagItem(id='tag_2', value='Tag 2', parent_id='new_parent'), + TagItem(id='new_parent', value='New parent'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual(self.import_plan.actions[0].name, 'create') + self.assertEqual(self.import_plan.actions[0].tag.id, 'new_parent') + + def test_generate_actions_mixed_case_parent_id_defined_later_accepted(self) -> None: + """ + A parent_id that differs only in case from the id of a later new row still makes the + child wait for that row, because ids are case-insensitive. + + File rows After + new_child (parent_id NEW_PARENT) New_Parent + New_Parent new_child + """ + tags = [ + TagItem(id='new_child', value='New child', parent_id='NEW_PARENT'), + TagItem(id='New_Parent', value='New parent'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + self.assertEqual(self.import_plan.actions[0].tag.id, 'New_Parent') + + def test_generate_actions_self_parent_of_existing_tag_rejected(self) -> None: + """ + An existing tag that names itself as its parent is a cycle of length one and is + reported once. + + Before File rows + tag_1 tag_2 (parent_id tag_2) + tag_2 + """ + self.import_plan.generate_actions(tags=[TagItem(id='tag_2', value='Tag 2', parent_id='tag_2')], replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(messages, ["Row 1's parent_id (tag_2) creates a parent cycle."]) + + def test_generate_actions_child_of_deleted_holder_follows_rename_onto_its_id(self) -> None: + """ + With replace=True the file omits tag_1, so tag_1 is deleted, and renames tag_3 onto + the id tag_1. The tag_2 row names tag_1, so tag_2 moves under the renamed tag after + the rename runs, instead of being promoted to a root. + + Before After + tag_1 tag_1 (was tag_3) + tag_2 tag_2 + tag_3 + tag_4 + """ + tags = [ + TagItem(id='tag_1', value='Tag 3', previous_id='tag_3'), + TagItem(id='tag_2', value='Tag 2', parent_id='tag_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=True) + self.assertEqual(self.import_plan.errors, []) + row_updates = [ + action for action in self.import_plan.indexed_actions['update_parent'] + if action.tag.id == 'tag_2' and action.tag.parent_id == 'tag_1' + ] + self.assertEqual(len(row_updates), 1) + self.assertGreater( + self.import_plan.actions.index(row_updates[0]), + self.import_plan.actions.index(self.import_plan.indexed_actions['rename_external_id'][0]), + ) + + def _plan_for_new_taxonomy(self, tags: list[TagItem]) -> TagImportPlan: + """ + Create ``tags`` (parents listed before their children) in an empty taxonomy shared by + the class, and return a plan for that taxonomy. A test calls this once, and the tags + are rolled back with the test. + """ + created: dict[str, Tag] = {} + for tag in tags: + created[tag.id] = Tag.objects.create( + taxonomy=self.structure_taxonomy, + value=tag.value, + external_id=tag.id, + parent=created[tag.parent_id] if tag.parent_id else None, + ) + return TagImportPlan(self.structure_taxonomy) + + def test_generate_actions_valid_move_order_keeps_action_order(self) -> None: + """ + A file whose parent moves are already in a valid order keeps its row order in the + plan. + + Before After + old_root new_root + old_child new_middle + new_root old_root + old_child + """ + plan = self._plan_for_new_taxonomy([ + TagItem(id='old_root', value='Old root'), + TagItem(id='old_child', value='Old child', parent_id='old_root'), + TagItem(id='new_root', value='New root', parent_id='old_root'), + ]) + rows = [ + TagItem(id='new_root', value='New root'), + TagItem(id='new_middle', value='New middle', parent_id='new_root'), + TagItem(id='old_root', value='Old root', parent_id='new_middle'), + TagItem(id='old_child', value='Old child', parent_id='new_middle'), + ] + plan.generate_actions(tags=rows, replace=False) + self.assertEqual(plan.errors, []) + self.assertEqual( + [action.tag.id for action in plan.actions], ['new_root', 'new_middle', 'old_root', 'old_child'], + ) + + def test_generate_actions_ordinary_import_has_no_detach_actions(self) -> None: + """ + An import whose steps are all valid keeps its plan: no move to the root is added + before the other actions. + + Before After + level_1 level_1 + level_2 level_2 + level_3 level_3 + new_leaf + """ + chain = [ + TagItem(id='level_1', value='Level 1'), + TagItem(id='level_2', value='Level 2', parent_id='level_1'), + TagItem(id='level_3', value='Level 3', parent_id='level_2'), + ] + plan = self._plan_for_new_taxonomy(chain) + plan.generate_actions( + tags=chain + [TagItem(id='new_leaf', value='New leaf', parent_id='level_3')], + replace=False, + ) + self.assertEqual(plan.errors, []) + self.assertEqual( + [action.name for action in plan.actions], ['without_changes'] * 3 + ['create'], + ) + + def test_parent_dry_run_accepts_valid_and_rejects_invalid_sequences(self) -> None: + """ + ``_ParentForest`` accepts a valid move, and rejects a cycle, a tree that gets too + deep, and a reference to a tag that an earlier delete removed (a delete also removes + the descendants). + + Before + level_1 + level_2 + level_3 + level_4 + level_5 + other_root + other_child + """ + tags = { + 'level_1': ExistingTag(pk=1, parent_pk=None, parent_external_id=None), + 'level_2': ExistingTag(pk=2, parent_pk=1, parent_external_id='level_1'), + 'level_3': ExistingTag(pk=3, parent_pk=2, parent_external_id='level_2'), + 'level_4': ExistingTag(pk=4, parent_pk=3, parent_external_id='level_3'), + 'level_5': ExistingTag(pk=5, parent_pk=4, parent_external_id='level_4'), + 'other_root': ExistingTag(pk=6, parent_pk=None, parent_external_id=None), + 'other_child': ExistingTag(pk=7, parent_pk=6, parent_external_id='other_root'), + } + taxonomy = Taxonomy() + + def move(tag_id: str, parent_id: str | None) -> UpdateParentTag: + return UpdateParentTag(taxonomy, TagItem(id=tag_id, value='x', parent_id=parent_id), 1) + + delete_level_2 = DeleteTag(taxonomy, TagItem(id='level_2', value='x'), 1) + self.assertTrue(_ParentForest(tags).apply(move('other_child', 'level_1'))) + # level_1 under its own descendant is a cycle. + self.assertFalse(_ParentForest(tags).apply(move('level_1', 'level_5'))) + # other_child would end up at a seventh level. + self.assertFalse(_ParentForest(tags).apply(move('other_root', 'level_5'))) + forest = _ParentForest(tags) + self.assertTrue(forest.apply(delete_level_2)) + self.assertFalse(forest.apply(move('other_root', 'level_3'))) + forest = _ParentForest(tags) + self.assertTrue(forest.apply(delete_level_2)) + self.assertTrue(forest.apply(move('other_root', 'level_1'))) + + def test_generate_actions_swap_case_variant_of_stale_parent_id_rejected(self) -> None: + """ + In a swap, a child naming its parent's stale id in another case is rejected like the + exact spelling, with its row number and the parent's new id. + + Before File rows + tag_1 tag_3 (previous_id tag_1) + tag_2 tag_1 (previous_id tag_3) + tag_3 tag_2 (parent_id TAG_1) + """ + 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_2', value='Tag 2', parent_id='TAG_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 1) + self.assertIn("Row 3's parent_id (TAG_1)", messages[0]) + self.assertIn("Use 'tag_3' as the parent_id.", messages[0]) + + def test_generate_actions_swap_case_variant_of_end_state_parent_id_accepted(self) -> None: + """ + In a swap, children naming the end-state ids of their parents in another case are + accepted. + + Before After + tag_1 tag_3 (was tag_1) + tag_2 tag_2 (parent_id TAG_3) + tag_3 tag_1 (was tag_3) + tag_4 tag_4 (parent_id TAG_1) + """ + 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_2', value='Tag 2', parent_id='TAG_3'), + TagItem(id='tag_4', value='Tag 4', parent_id='TAG_1'), + ] + self.import_plan.generate_actions(tags=tags, replace=False) + self.assertEqual(self.import_plan.errors, []) + + def test_generate_actions_plain_rename_case_variant_of_parent_id_reported_once(self) -> None: + """ + When the parent is renamed without a swap, a child naming the parent's old id in + another case gets only the parent validation error, not a second one for the same row. + + Before File rows + tag_1 tag_50 (previous_id tag_1) + tag_2 tag_2 (parent_id TAG_1) + """ + 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) + messages = [str(error) for error in self.import_plan.errors] + self.assertEqual(len(messages), 1) + self.assertIn('Parent tag TAG_1 is renamed to tag_50 in this file', messages[0]) + def test_generate_actions_genuine_collision_not_staged(self) -> None: """ A rename targeting an id held by an unrelated tag that is not