Tag import: parent_id isn't re-validated when it matches the pre-import parent

Summary

A tag import (CSV or JSON, with the optional previous_id column from #673) must reject a row whose parent_id is its current parent's pre-import id when the same file renames that parent. It must keep accepting a child that names the parent's id after the import (the end-state id), also when two parents swap their ids.

Fixing this showed that several neighboring cases went wrong in the same code: parent references depended on the order of the rows, kept children could be lost to the delete cascade, parent cycles and imports deeper than the depth limit were not rejected at the Plan step, and some valid imports failed partway through. Each of those is described below with its own acceptance criteria, because the fix for this issue delivers them too.

In the error messages below, "Row N" 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. In a JSON file it is the N-th entry of tags.

Problem A: a stale parent_id of an existing child is accepted

UpdateParentTag.applies_for decides whether a row changes its tag's parent by comparing the row's parent_id with the external_id of the tag's parent before the import. When the two match, the row is "no change", so _validate_parent never runs for it.

Take an existing child tag_2 under tag_1. The file renames tag_1 to tag_50 and keeps the tag_2 row with parent_id: tag_1. The tag_2 row matches the pre-import parent, so it is "no change", and the import accepts it silently. After the import no tag has the id tag_1, so the row's parent_id names nothing. #673's scenario "a child row referencing its parent's pre-import external_id is rejected" requires rejecting exactly this file.

The existing regression tests only used a brand-new child. A new child is created through CreateTag, which runs _validate_parent, so those tests passed while the existing-child case was broken.

Problem B: the first fix rejected valid swaps

A first attempt at fixing Problem A (reverted) rejected every parent_id that named an id which one rename row gives up and another rename row takes, as happens in a swap. That also rejected a child that names the correct end-state id. For example, when tag_1 and tag_3 swap ids, tag_4 (a child of the old tag_3) correctly says parent_id: tag_1, and the first fix rejected it. As a result, two swapped parents with children could not be imported at all.

Root cause and proposed fix

Both problems come from keying the validation on the wrong condition. The only ambiguous reading is a row whose own parent_id equals the pre-import id of its own current parent while the same file renames that parent to another id.

The fix adds a plan-level check for exactly that condition. The check runs whether or not the row is "no change" for UpdateParentTag. Its error names the row and the parent's new id, for example: Row 2's parent_id (tag_1) names an id this file renames away. Use 'tag_50' as the parent_id. A child that names the end-state id of its parent is accepted.

The same pass also corrects a code comment that claimed _validate_parent covers the plain-rename case. That comment was wrong, because a "no change" row never calls _validate_parent.

Acceptance criteria

Every criterion in this issue must hold whatever the order of the rows in the file.

1. #673's scenario "a child row referencing its parent's pre-import external_id is rejected" is rejected:
   • Given a taxonomy where tag_2 is an existing child of tag_1
   • When the admin imports a file that renames tag_1 to tag_50 and keeps the tag_2 row with parent_id: tag_1
   • Then the Plan step rejects the import with an error that names the tag_2 row and says to use tag_50 as the parent_id
   • And nothing in the taxonomy changes.
2. A swap of two parents with children succeeds:
   • Given tag_2 under tag_1 and tag_4 under tag_3
   • When the admin imports a full replace file (replace=True) that renames tag_3 to tag_1 and tag_1 to tag_3, with tag_2 saying parent_id: tag_3 and tag_4 saying parent_id: tag_1
   • Then the import succeeds
   • And each child stays on the same physical parent: tag_2 is under the tag now called tag_3, and tag_4 is under the tag now called tag_1.
3. An ambiguous swap reference is still rejected:
   • Given tag_2 under tag_1
   • When the file swaps the ids of tag_1 and tag_3, and the tag_2 row still says parent_id: tag_1
   • Then the Plan step rejects the import, because after the import tag_1 names the old tag_3, so the row could mean "stay with my parent" or "move under the tag now called tag_1"
   • And the error names the tag_2 row and says to use tag_3 as the parent_id.
4. test_generate_actions_parent_id_new_id_after_earlier_rename_accepted and the test_generate_actions_rename_external_id_replace_skips_delete* tests continue to pass unchanged.

Additional fix 1: parent references do not depend on row order

The import builds, validates and runs one action per row in file order. A row whose parent_id names a tag that a later row creates or renames was therefore rejected with "Unknown parent tag", and the message told the admin to move the parent's row first. #673 requires that the outcome does not depend on the order of the rows.

• Given a taxonomy without a tag new_parent

• When the admin imports a file whose row for new_child (with parent_id: new_parent) comes before the row that creates new_parent

• Then the import succeeds and new_child is under new_parent.

• Given tag_2 under tag_1

• When the file renames tag_1 to tag_50, and a row naming parent_id: tag_50 comes before the rename row

• Then the import succeeds and that row's tag is under the renamed tag.

• Given a parent with a child

• When the file makes the child a root and moves the parent under it, in either row order

• Then the import succeeds with the child as the root and the parent under it.

• Given old_root with the children sibling and new_root

• When the file makes new_root a root, creates new_middle under new_root, and moves old_root and sibling under new_middle, in any row order

• Then the import succeeds with that tree, because new_root leaves the subtree of old_root before old_root moves under it.

• Given a file that already lists every parent before its children

• When the admin imports it

• Then the Plan lists the actions in the same order as before this fix.

Additional fix 2: kept children of a deleted parent survive the delete cascade

With replace=True, a tag that the file omits is deleted, and deleting a tag also deletes all its descendants. A kept tag whose parent is deleted is promoted to a root first. That promotion used to run after some of the deletes, so a kept tag whose deleted parent sat under another deleted tag was removed by the cascade, and the import failed when it ran.

• Given a chain deleted_root > deleted_child > kept_tag

• When the admin imports a full replace file that lists only kept_tag

• Then the import succeeds

• And kept_tag is a root with the same database row (same pk, content associations intact), and deleted_root and deleted_child are gone.

• Given a full replace import that deletes the parent of a kept tag

• When the admin looks at the Plan

• Then every promotion of a kept tag to a root comes before the first delete.

Additional fix 3: parent cycles and self-parents are rejected at the Plan step

No plan-level check looked at the parents the tags will have after the import, so a parent_id that makes a tag its own ancestor passed the Plan step.

• Given tag_2 under tag_1

• When the admin imports a file whose tag_1 row says parent_id: tag_2

• Then the Plan step rejects the import with Row 1's parent_id (tag_2) creates a parent cycle.

• And nothing in the taxonomy changes.

• Given an existing tag tag_2

• When the file's tag_2 row says parent_id: tag_2

• Then the Plan step rejects the import with a parent cycle error that names that row.

• Given a file in which a rename row claims an id that the same full replace import frees, and the renamed tag names that id as its own parent, or two such renamed tags name each other as parent

• When the admin imports it

• Then the Plan step rejects the import with exactly one error, the parent cycle error on the cycle's first row in the file.

• Given a file in which new tags name each other as parent, so that none of their ids exists before the import

• When the admin imports it

• Then the Plan step rejects the import with the existing "Unknown parent tag" error, once, and does not report the same cycle a second time.

Additional fix 4: an import deeper than the depth limit is rejected at the Plan step

A taxonomy allows at most 6 levels. The Plan step did not check the depth of the resulting tree, so such an import either failed when it ran or, through the descendant update in Tag.save, stored tags deeper than the limit.

• Given a chain of six levels
• When the admin imports a file that adds a child under the sixth-level tag, or that moves a four-level chain under the fourth level of another chain
• Then the Plan step rejects the import with one error: This import would leave tags deeper than 6 levels, or build an invalid parent hierarchy, so it cannot be applied.
• And nothing in the taxonomy changes.

Additional fix 5: imports whose intermediate steps would be invalid are applied by moving the affected tags to the root first

The import runs its actions one at a time, and Tag.save checks the depth at each step. A file whose end state is valid could therefore fail partway through, even with the row ordering of additional fix 1. For example, deep_tag is at the fifth level and moves to the root, while the same file creates new_child under deep_tag and new_grandchild under new_child. If the new tags are created before deep_tag moves, new_grandchild is at the seventh level and the step fails.

The fix simulates the parent changes of the planned actions in memory at the Plan step. If a step would be invalid, the plan is rebuilt once: every existing tag whose parent changes is first moved to the root, and then the rows are applied with each parent before its children. Only plans that need it change.

• Given a chain level_1 > level_2 > level_3 > level_4 > deep_tag

• When the admin imports a file that moves deep_tag to the root and creates new_child under deep_tag and new_grandchild under new_child, in any row order

• Then the import succeeds with that tree, and deep_tag keeps its database row.

• Given a chain chain_1 > chain_2 > chain_3 > moved_up and a separate chain subtree_root > subtree_child > subtree_grandchild

• When the file moves moved_up to the root and subtree_root under moved_up, in any row order

• Then the import succeeds with that tree, although attaching subtree_root while moved_up is still at the fourth level would put subtree_grandchild at the seventh level.

• Given an import in which every step is valid in the order the plan builds it

• When the admin looks at the Plan

• Then the Plan has no extra actions that move tags to the root.

Additional fix 6: a case variant of the stale id is treated like the exact spelling

External ids are case-insensitive. A child that names its parent's stale id with different capitalization (TAG_1 for tag_1) is a parent change for UpdateParentTag, so it reaches _validate_parent. In a swap, _validate_parent accepts that id, because another rename row claims it, so the ambiguous reference from criterion 3 got through when it was spelled in another case.

• Given tag_2 under tag_1

• When the file swaps the ids of tag_1 and tag_3, and the tag_2 row says parent_id: TAG_1

• Then the Plan step rejects the import with one error that names the tag_2 row and says to use tag_3 as the parent_id.

• Given the same swap

• When the children name the end-state ids in another case (TAG_3 for tag_2, TAG_1 for tag_4)

• Then the import succeeds and each child stays on the same physical parent.

• Given tag_2 under tag_1

• When the file only changes the capitalization of the parent's id (tag_1 to TAG_1), and the tag_2 row names the parent as tag_1, TAG_1 or Tag_1

• Then the import succeeds and tag_2 stays under its parent.

• Given tag_2 under tag_1

• When the file renames tag_1 to tag_50, and the tag_2 row says parent_id: TAG_1

• Then the Plan step rejects the import with the existing parent validation error only, once.

References

• #673, the ticket that introduced previous_id, and its acceptance scenarios.
• ADR 0010: docs/openedx_tagging/decisions/0010-mutable-tag-external-id.rst.
• Review comments on #804 that led to this issue:
  • https://github.com/openedx/openedx-core/pull/804#discussion_r4213665719
  • https://github.com/openedx/openedx-core/pull/804#discussion_r4198381829