Skip to content

fix: reject stale parent ids on tag import and make parent changes order-independent #871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ormsbee wants to merge 3 commits into from
Closed

fix: reject stale parent ids on tag import and make parent changes order-independent #871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9780796
fix: reject a child row that keeps its renamed parent's stale externa…
ormsbee Oct 14, 2026
66d5fb1
fix: make tag import parent changes order-independent and reject bad …
ormsbee Oct 14, 2026
8a7b691
refactor: list the detach step in the plan as a DetachTag action
ormsbee Oct 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 125 additions & 55 deletions src/openedx_tagging/import_export/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,51 +105,54 @@ def _search_action(

def _validate_parent(self, indexed_actions) -> ImportActionError | None:
"""
Validate that `parent_id` names a tag that will exist once this import finishes.

A match against a tag being renamed away in this same import (see
`_vacated_pks`) doesn't count as the real parent: it falls through to the
same "created or renamed-in this import" check as a brand-new parent. If
nothing lands on that vacated id either, the error names the id it was
renamed to, instead of the generic "unknown parent" message for a parent
never mentioned at all.
Validate that `parent_id` names the tag that will hold that id once this import finishes.

That is the tag currently holding `parent_id`, unless this import renames it
away (see `_vacated_pks`). A renamed-away id, like a brand-new one, must be
given to a tag by a row of this file: a rename onto it (`_vacated_pks` maps
every renamed tag to its new id, whatever the row order) or a create action
built before this one (TagImportPlan builds a new parent before its
children). Even then it must not be this row's own current parent: a row
that names its parent's pre-import id while another row takes that id (a
swap) could mean "stay with my parent" or "move under the tag now called
`parent_id`", so it is rejected. The error for a renamed-away id names the
id it was renamed to, instead of the generic "unknown parent" message for
an id never mentioned at all.
"""
vacated_pks: dict[int, str] = indexed_actions.get("_vacated_pks", {})
parent_tag = None
is_vacated = False
try:
# Validates that the parent exists on the taxonomy
parent_tag = self.taxonomy.tag_set.get(external_id=self.tag.parent_id)
if parent_tag.pk in vacated_pks:
is_vacated = True
raise Tag.DoesNotExist
except Tag.DoesNotExist:
# Or if the parent is created or renamed-in on previous actions
found = self._search_action(
indexed_actions, CreateTag.name, "id", self.tag.parent_id, casefold=True
) or self._search_action(
indexed_actions, RenameTagExternalId.name, "id", self.tag.parent_id, casefold=True
parent_tag = self.taxonomy.tag_set.filter(external_id=self.tag.parent_id).first()
new_id = vacated_pks.get(parent_tag.pk) if parent_tag is not None else None
if parent_tag is not None and new_id is None:
return None # The parent exists and keeps its id.

found = (self.tag.parent_id or "").casefold() in {
renamed_to.casefold() for renamed_to in vacated_pks.values()
} or self._search_action(indexed_actions, CreateTag.name, "id", self.tag.parent_id, casefold=True)
if found and not (parent_tag is not None and self._is_current_parent(parent_tag)):
Comment on lines +128 to +131

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: these four lines took me a few reads. As far as I can tell, found means "some row of this file gives the id parent_id to a tag, by a rename onto it or by a create action built earlier", and line 131 means "accept that, unless the id currently belongs to this row's own parent (the ambiguous swap)". Naming the pieces would make both lines say that directly, something like:

renamed_onto_id = (self.tag.parent_id or "").casefold() in {
    renamed_to.casefold() for renamed_to in vacated_pks.values()
}
created_with_id = self._search_action(indexed_actions, CreateTag.name, "id", self.tag.parent_id, casefold=True)
names_own_parents_old_id = parent_tag is not None and self._is_current_parent(parent_tag)
if (renamed_onto_id or created_with_id) and not names_own_parents_old_id:
    return None

return None
if new_id is not None:
return ImportActionError(
action=self,
message=_(
"Parent tag {parent_id} is renamed to {new_id} in this file; "

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion, within the current requirements: for the ambiguous swap case, the error tells the admin to use the parent's new id, which assumes they meant "stay with my parent". An admin who meant "move under the tag now called tag_1" gets steered the wrong way. Could the message also say how to do that, for example by doing the swap and the move in two separate imports?

"use {new_id} as the parent_id."
).format(parent_id=self.tag.parent_id, new_id=new_id),
)
if not found:
if is_vacated:
# is_vacated is only set after parent_tag was assigned above.
assert parent_tag is not None
new_id = vacated_pks[parent_tag.pk]
return ImportActionError(
action=self,
message=_(
"Parent tag {parent_id} is renamed to {new_id} in this file; "
"use {new_id} as the parent_id."
).format(parent_id=self.tag.parent_id, new_id=new_id),
)
return ImportActionError(
action=self,
message=_(
"Unknown parent tag ({parent_id}). "
"You need to add parent before the child in your file."
).format(parent_id=self.tag.parent_id),
)
return None
return ImportActionError(
action=self,
message=_(
"Unknown parent tag ({parent_id}). "
"Add a row for the parent to your file."
).format(parent_id=self.tag.parent_id),
)

def _is_current_parent(self, parent_tag: Tag) -> bool:

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I found the name and docstring of _is_current_parent hard to follow, mostly because "this row's tag" could be a tag that doesn't exist yet. If I read it right, it answers "before the import, was parent_tag the parent of the tag this row describes?", looking that tag up by previous_id when the row renames it and by id otherwise. Something like _was_parent_before_import, with that lookup rule in the docstring, would be clearer to me.

"""
Whether `parent_tag` is the parent that this row's tag has before the import.
"""
return self.taxonomy.tag_set.filter(
external_id=self.tag.previous_id or self.tag.id, parent=parent_tag,
).exists()

def _validate_value(self, indexed_actions) -> ImportActionError | None:
"""
Expand Down Expand Up @@ -326,33 +329,49 @@ class UpdateParentTag(ImportAction):
"""

name = "update_parent"
_description: str | None = None

def __str__(self) -> str:
taxonomy_tag = self._get_tag()

description_str = _("Update the parent of {tag} from parent {old_parent} to {new_parent}").format(
tag=taxonomy_tag.display_str(),
old_parent=taxonomy_tag.parent.display_str() if taxonomy_tag.parent else None,
new_parent=self.tag.parent_id,
)

return str(description_str)
# Describe the change as of the plan: the plan is logged before it runs, and the
# execution detaches every moved tag first, which would then read as "from None".
if self._description is None:
taxonomy_tag = self._get_tag()
self._description = str(
_("Update the parent of {tag} from parent {old_parent} to {new_parent}").format(
tag=taxonomy_tag.display_str(),
old_parent=taxonomy_tag.parent.display_str() if taxonomy_tag.parent else None,
new_parent=self.tag.parent_id,
)
)
return self._description
Comment on lines 334 to +346

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion from Claude: the cached description only reads correctly if __str__ runs before the detach step. TagImportPlan.execute() calls str(action) right before each action runs (import_plan.py line 554), and by then the detach action has already moved the tag to the root. import_tags logs the plan first (api.py line 128), so the import path is fine, but any other caller that calls generate_actions() and then execute(task) would log "from parent None". Building the description when the action is created would remove that ordering dependency.


@classmethod
def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> bool:
"""
This action applies whenever there is a change on the parent.

A row that names its parent's current id is still a parent change when

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I had trouble telling whether "current id" meant the id before or after the import. In this PR "current" always means before the import, but the new docstrings and comments use several terms for the same two ideas: "current id" here, "currently holding" (line 110), "own current parent" (line 115) and "pre-import id" (line 116) for the id before the import, and "final id" and "end state" in import_plan.py for the id after it. Could we settle on "pre-import id" and "final id" throughout?

this import gives that id to another tag: by renaming the parent away
from it, or by renaming another tag onto it once the parent is deleted.
Such a row used to count as "no change", so a stale id got through
`_validate_parent` and a reclaimed id lost the child to the delete
phase's promotion.
Comment on lines +356 to +358

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these lines describe how the code used to behave, which belongs in the commit history rather than the docstring. I'd delete them, or keep only the reason in present tense, for example "Otherwise a stale id would skip parent validation, and a child of a reclaimed id would be lost to the delete phase's promotion."


See ImportAction._resolve_update_target for when this doesn't apply
regardless of the parent change (a rename_external_id row).
"""
taxonomy_tag = cls._resolve_update_target(taxonomy, tag)
if taxonomy_tag is None:
return False
return (
taxonomy_tag.parent is not None
and taxonomy_tag.parent.external_id != tag.parent_id
) or (taxonomy_tag.parent is None and tag.parent_id is not None)
parent = taxonomy_tag.parent

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: old_parent would make it clearer that this is the parent before the import, and it matches the old_parent label __str__ already uses on line 342.

if parent is None:
return tag.parent_id is not None
if parent.external_id != tag.parent_id:
return True
vacated_pks: dict[int, str] = indexed_actions.get("_vacated_pks", {}) if indexed_actions else {}
return parent.pk in vacated_pks or parent.external_id.casefold() in {

@mgwozdz-unicon mgwozdz-unicon Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion from Claude: this line, and line 128 in _validate_parent, rebuild a casefolded set of every renamed-to id for every row, so the cost is about rows times renames. I haven't measured it, but for a large file that renames every tag that is a few million casefolds, twice. Building the set once in _build_staging_actions, next to _vacated_pks, would avoid it.

new_id.casefold() for new_id in vacated_pks.values()
}

def validate(self, indexed_actions) -> list[ImportActionError]:
"""
Expand Down Expand Up @@ -641,6 +660,56 @@ def execute(self) -> None:
self.taxonomy.tag_set.filter(pk=self.target_pk).update(external_id=placeholder)


class DetachTag(ImportAction):
"""
Action to move a tag to the root before the actions that change parents run.

Synthesized by TagImportPlan._build_detach_actions for every existing tag
whose parent changes, not read from a file row. Tag.save checks the depth
of each step, and a cycle corrupts the tree silently, so a restructure that
is valid as a whole can still go wrong part-way through, depending on the
order of the actions. Once every moved tag is at the root, each tag is at
or above its final depth and none is an ancestor of its new parent, so the
actions that follow can attach tags in any order.
"""

name = "detach"

def __str__(self) -> str:
return str(
_(
"Move tag {tag} to the root first, so that the parent changes "
"in this file can run in any order."
).format(tag=self.tag)
)

@classmethod
def applies_for(cls, taxonomy: Taxonomy, tag: TagItem, indexed_actions=None) -> bool:
"""
This action is an exception: synthesized in TagImportPlan.generate_actions.
"""
return False

def validate(self, indexed_actions) -> list[ImportActionError]:
"""
No validations necessary
"""
return []

def execute(self) -> None:
"""
Moves the tag to the root, with its descendants.

Looks the tag up by pk: a StageTagExternalIdForSwap action may have
moved it onto a placeholder id by now.
"""
# _build_detach_actions only builds this action with a target_pk.
assert self.target_pk is not None
tag = self.taxonomy.tag_set.get(pk=self.target_pk)
tag.parent = None
tag.save()


class DeleteTag(ImportAction):
"""
Action for delete a Tag
Expand Down Expand Up @@ -718,6 +787,7 @@ def execute(self) -> None:
RenameTagExternalId,
CreateTag,
StageTagExternalIdForSwap,
DetachTag,
DeleteTag,
WithoutChanges,
]
27 changes: 27 additions & 0 deletions src/openedx_tagging/import_export/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,33 @@ def __init__(self, tag_id: str, plain_row: int, rename_row: int, **kargs):
).format(tag_id=tag_id, plain_row=plain_row, rename_row=rename_row)


class ParentCycleError(TagImportError):
"""
Exception raised when a row's parent_id would make its tag its own ancestor.

``row`` is the 1-based position of the row among the file's data rows, as
in DuplicateFinalIdError.
"""

def __init__(self, row: int, parent_id: str, **kargs):
super().__init__(**kargs)
self.message = _(
"Row {row}'s parent_id ({parent_id}) creates a parent cycle."
).format(row=row, parent_id=parent_id)


class TagTooDeepError(TagImportError):
"""
Exception raised when the import would leave a tag deeper than the taxonomy allows.
"""

def __init__(self, tag_id: str, max_levels: int, **kargs):
super().__init__(**kargs)
self.message = _(
"This import would leave tag {tag_id} more than {max_levels} levels deep."
).format(tag_id=tag_id, max_levels=max_levels)


class ImportActionConflict(ImportActionError):
"""
Exception used when exists a conflict between actions
Expand Down
Loading
Loading