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. In the import log, an error raised by a row's action starts with that action, for example Action error in 'update_parent' (#6): Row 2: .... The #6 is the action's number in the plan, which can differ from the row number because the plan adds its own steps and builds a parent's row before its children's rows. The row is the Row N part.

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. A row that names its parent's pre-import id is only a problem when the same file gives that id away: the file renames the parent to another id, or, in a full replace import, the file deletes the parent and renames another tag onto its id.

The fix treats such a row as a parent change, so the per-row parent validation runs for it even though its parent_id matches the parent's pre-import id. When the file renames the parent away, the validation rejects the row. The only ambiguous reading is a row whose parent_id equals the pre-import id of its own current parent while another row of the same file takes that id, as in a swap. That row is rejected too. The error names the row and the parent's new id, for example: Row 2: Parent tag tag_1 is renamed to tag_50 in this file; use tag_50 as the parent_id. When another row takes the id, the error adds a second sentence for the other possible meaning: To move under the tag that takes the id tag_1 in this file instead, import the id changes and the move as two separate files. A child that names the end-state id of its parent is accepted. When the file deletes the parent and renames another tag onto its id, a child that names that id moves under the renamed tag.

A comment in the import plan says that _validate_parent reports a reference to an id the import renames away. Before this fix the comment was wrong for a "no change" row, which never called _validate_parent. With the fix the comment holds, because such a row is now a parent change and reaches _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. With the tag_2 row as the second row, the error is Row 2: Parent tag tag_1 is renamed to tag_50 in this file; 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, says to use tag_3 as the parent_id, and says how to make the other move. With the tag_2 row as the third row, the error is Row 3: Parent tag tag_1 is renamed to tag_3 in this file; use tag_3 as the parent_id. To move under the tag that takes the id tag_1 in this file instead, import the id changes and the move as two separate files.
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.

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