diff --git a/docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst b/docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst index 6826a4161..cf811bbcd 100644 --- a/docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst +++ b/docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst @@ -68,6 +68,30 @@ no existing caller has to change what it sends. The response side narrows from "nullable string" to "always a string," which is the safe direction for consumers: code written to expect a possible ``null`` simply never takes that branch anymore. +Known limitations +~~~~~~~~~~~~~~~~~ + +Importing a file exported before ``0022_tag_external_id_not_null.py`` ran, for a tag that +had no ``external_id`` at export time, references that tag by its database id instead of a +real ``external_id`` (that export's own stand-in for one). Re-importing such a file +unchanged is safe even if that stand-in id happens to equal a different tag's real, +current ``external_id``: the row's ``value`` still belongs to the original tag, which +this kind of re-import leaves untouched, so ``Tag.value``'s own per-taxonomy uniqueness +guarantees the wrongly-matched tag's value differs from it. That mismatch makes a rename +action apply to the wrongly-matched tag too, and its value-duplicate check rejects the +whole import before anything executes. + +That protection depends on the row's ``value`` still matching an existing tag. If the +re-imported file also assigns that row a value nothing else in the taxonomy currently +has, meaning it isn't simply "re-import unchanged" but also intends a genuine rename, +and the row's id still happens to collide with a different, unrelated tag's real +``external_id``, that combination isn't guarded against: the rename applies to the +wrongly-matched tag instead of the one the file's own history means it should apply to. +Closing this would require changes to the import matching logic itself, weighed against a +real risk of rejecting legitimate imports that use a plain numeric ``external_id`` (a +common style for CBE competency identifiers) that happens to equal an unrelated tag's +database id. That tradeoff is not addressed by this decision. + Future considerations ~~~~~~~~~~~~~~~~~~~~~~ @@ -114,3 +138,12 @@ Changelog the backfill algorithm: it isn't injective, so it can map two distinct tag values onto the same ``external_id``, undermining the collision-free argument this decision relies on. + +2026-09-25: + +* Added "Known limitations", documenting why re-importing a + pre-``0022_tag_external_id_not_null.py`` export unchanged stays safe even if its + database-id stand-in collides with another tag's real ``external_id`` (an existing + value-duplicate check on a co-triggered rename action catches it), and the narrower + residual risk when such a file is also given a genuinely new value on top of that + collision. diff --git a/pyproject.toml b/pyproject.toml index 91b16ee9a..c594a2255 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -102,6 +102,7 @@ test-base = [ "ddt", "django-debug-toolbar", "django-stubs", + "django-test-migrations", "djangorestframework-stubs", "freezegun", "import-linter", diff --git a/src/openedx_tagging/api.py b/src/openedx_tagging/api.py index 54addd7d8..005600d63 100644 --- a/src/openedx_tagging/api.py +++ b/src/openedx_tagging/api.py @@ -446,7 +446,9 @@ def add_tag_to_taxonomy( """ Adds a new Tag to provided Taxonomy. If a Tag already exists in the Taxonomy, an exception is raised, otherwise the newly created - Tag is returned + Tag is returned. + + If `external_id` is omitted, one is generated from the tag's name. """ new_tag = taxonomy.add_tag(tag, parent_tag_value, external_id) diff --git a/src/openedx_tagging/import_export/actions.py b/src/openedx_tagging/import_export/actions.py index a501c9e30..a22d1d13a 100644 --- a/src/openedx_tagging/import_export/actions.py +++ b/src/openedx_tagging/import_export/actions.py @@ -72,12 +72,7 @@ def _get_tag(self) -> Tag: """ Returns the respective tag of this actions """ - if self.tag.id: - try: - return self.taxonomy.tag_set.get(external_id=self.tag.id) - except Tag.DoesNotExist: - pass - return self.taxonomy.tag_set.get(value=self.tag.value, external_id=None) + return self.taxonomy.tag_set.get(external_id=self.tag.id) def _search_action( self, diff --git a/src/openedx_tagging/import_export/import_plan.py b/src/openedx_tagging/import_export/import_plan.py index 6502c2c1b..9312823da 100644 --- a/src/openedx_tagging/import_export/import_plan.py +++ b/src/openedx_tagging/import_export/import_plan.py @@ -96,13 +96,9 @@ def _get_tag_id(self, tag: Tag) -> str: """ Get the id used on the Tag model. - By default, the external_id is used for import and export, - but there are cases where taxonomies are created without external_id. - In those cases the tag id is used + The external_id is used for import and export. """ - if tag.external_id: - return tag.external_id - return str(tag.id) + return tag.external_id def _build_delete_actions(self, tags: dict): """ diff --git a/src/openedx_tagging/migrations/0022_tag_external_id_not_null.py b/src/openedx_tagging/migrations/0022_tag_external_id_not_null.py new file mode 100644 index 000000000..9073880b4 --- /dev/null +++ b/src/openedx_tagging/migrations/0022_tag_external_id_not_null.py @@ -0,0 +1,109 @@ +""" +Make Tag.external_id non-nullable. + +Backfills a generated external_id (derived from the tag's value) for every +existing tag that doesn't already have one, then enforces NOT NULL on the +column. + +See docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst for +the rationale. + +Note that we copy the candidate-generation logic here instead of importing +`tag_external_id_candidate` from `openedx_tagging.models.utils`, so that this +migration keeps producing the exact same values it did when it first ran, +regardless of any future changes to that app code (matching the precedent in +`openedx_content/backcompat/publishing/migrations/0010_backfill_dependencies.py`). +""" +from __future__ import annotations + +from django.db import migrations +from django.db.models import Q + +import openedx_django_lib.fields + +# Frozen copy of TAG_EXTERNAL_ID_MAX_LENGTH / tag_external_id_candidate from +# openedx_tagging.models.utils, as of when this migration was written. See the +# module docstring for why this isn't just imported. +_TAG_EXTERNAL_ID_MAX_LENGTH = 255 + +# Caps both the SQL batch size of each bulk_update() and how many Tag instances +# are held in memory at once before being flushed. +_BATCH_SIZE = 500 + + +def _tag_external_id_candidate(value: str, attempt: int = 1) -> str: + if attempt == 1: + return value.strip()[:_TAG_EXTERNAL_ID_MAX_LENGTH].strip() + suffix = f"-{attempt}" + return value.strip()[: _TAG_EXTERNAL_ID_MAX_LENGTH - len(suffix)].strip() + suffix + + +def backfill(apps, _schema_editor): + """ + Generate and persist an external_id for every tag that doesn't have one. + + "Doesn't have one" covers both NULL and an empty string: the field has always + been blank=True, and this migration runs unmodified against arbitrary + downstream data, so a row written by code outside this repo could plausibly + hold "" rather than NULL for "no external_id". + """ + Tag = apps.get_model("oel_tagging", "Tag") + + is_missing = Q(external_id__isnull=True) | Q(external_id="") + has_value = ~is_missing + + taxonomy_ids = Tag.objects.filter(is_missing).values_list("taxonomy_id", flat=True).distinct() + for taxonomy_id in taxonomy_ids: + existing = { + eid.casefold() for eid in + Tag.objects.filter(has_value, taxonomy_id=taxonomy_id).values_list("external_id", flat=True) + } + batch = [] + for tag in Tag.objects.filter(is_missing, taxonomy_id=taxonomy_id).order_by("id").iterator(): + attempt = 1 + candidate = _tag_external_id_candidate(tag.value, attempt) + while candidate.casefold() in existing: + attempt += 1 + candidate = _tag_external_id_candidate(tag.value, attempt) + existing.add(candidate.casefold()) + tag.external_id = candidate + batch.append(tag) + + if len(batch) >= _BATCH_SIZE: + Tag.objects.bulk_update(batch, ["external_id"], batch_size=_BATCH_SIZE) + batch.clear() + + if batch: + Tag.objects.bulk_update(batch, ["external_id"], batch_size=_BATCH_SIZE) + + +def reverse_backfill(_apps, _schema_editor): + """ + No-op. + + Rollback only loosens the NOT NULL constraint. The generated + external_id values are intentionally left in place. + """ + + +class Migration(migrations.Migration): + + atomic = False + + dependencies = [ + ("oel_tagging", "0021_remove_system_defined_add_read_only"), + ] + + operations = [ + migrations.RunPython(backfill, reverse_backfill), + migrations.AlterField( + model_name="tag", + name="external_id", + field=openedx_django_lib.fields.MultiCollationCharField( + blank=True, + db_collations={"mysql": "utf8mb4_unicode_ci", "sqlite": "NOCASE"}, + help_text="Used to link an Open edX Tag with a tag in an externally-defined taxonomy.", + max_length=255, + ), + ), + ] diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index f2cc41b4f..f725ec778 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -17,7 +17,7 @@ from openedx_django_lib.fields import MultiCollationTextField, case_insensitive_char_field, case_sensitive_char_field from ..data import TagDataQuerySet -from .utils import RESERVED_TAG_CHARS +from .utils import RESERVED_TAG_CHARS, tag_external_id_candidate # Maximum depth of tags that can be created. Internally, the system has no depth limits, but for reasonable performance # guarantees we enforce this depth. Note: depth is zero-indexed so "5" means 6 levels of depth are allowed. @@ -63,7 +63,6 @@ class Tag(models.Model): ) external_id = case_insensitive_char_field( max_length=255, - null=True, # To allow multiple values with our UNIQUE constraint, we need to use NULL values here instead of "" blank=True, help_text=_( "Used to link an Open edX Tag with a tag in an externally-defined taxonomy." @@ -135,9 +134,7 @@ def display_str(self): """ String representation of a Tag used on user logs. """ - if self.external_id: - return f"<{self.__class__.__name__}> ({self.external_id} / {self.value})" - return f"<{self.__class__.__name__}> ({self.value})" + return f"<{self.__class__.__name__}> ({self.external_id} / {self.value})" def get_lineage(self) -> Lineage: """ @@ -174,6 +171,22 @@ def save(self, *args, **kwargs) -> None: Compute and persist depth and lineage before saving, then cascade any changes to descendants. """ self.clean() + if not self.external_id: + assert self.taxonomy is not None + candidate = tag_external_id_candidate(self.value, 1) + if self.taxonomy.tag_set.filter(external_id__iexact=candidate).exists(): + # Collision on the first attempt: load all other external_ids in this taxonomy once, + # and generate further candidates in memory rather than re-querying per attempt. + existing = { + eid.casefold() for eid in + self.taxonomy.tag_set.exclude(pk=self.pk).values_list("external_id", flat=True) + } + attempt = 2 + candidate = tag_external_id_candidate(self.value, attempt) + while candidate.casefold() in existing: + attempt += 1 + candidate = tag_external_id_candidate(self.value, attempt) + self.external_id = candidate old_values = ( Tag.objects.filter(pk=self.pk).values("depth", "lineage").first() if self.pk else None @@ -525,6 +538,9 @@ def add_tag( if self.tag_set.filter(value__iexact=tag_value).exists(): raise ValueError(f"Tag with value '{tag_value}' already exists for taxonomy.") + if external_id and self.validate_external_id(external_id): + raise ValueError(f"Tag with external_id '{external_id}' already exists for taxonomy.") + parent = None if parent_tag_value: # Get parent tag from taxonomy, raises Tag.DoesNotExist if doesn't diff --git a/src/openedx_tagging/models/utils.py b/src/openedx_tagging/models/utils.py index a2c07ce6b..1fee03e33 100644 --- a/src/openedx_tagging/models/utils.py +++ b/src/openedx_tagging/models/utils.py @@ -11,3 +11,21 @@ # e.g. languages-v1: en;es;fr ] TAGS_CSV_SEPARATOR = RESERVED_TAG_CHARS[2] + +TAG_EXTERNAL_ID_MAX_LENGTH = 255 + + +def tag_external_id_candidate(value: str, attempt: int = 1) -> str: + """ + Generate a candidate ``external_id`` for a tag from its ``value``. + + ``attempt`` 1 returns ``value`` alone, capped at ``TAG_EXTERNAL_ID_MAX_LENGTH``. + Each later attempt appends a ``-{attempt}`` suffix (``-2``, ``-3``, ...) and + truncates ``value`` further so the suffixed result still fits within that cap. + Callers use successive attempts to find an ``external_id`` that doesn't collide + with one already used in the same taxonomy. + """ + if attempt == 1: + return value.strip()[:TAG_EXTERNAL_ID_MAX_LENGTH].strip() + suffix = f"-{attempt}" + return value.strip()[: TAG_EXTERNAL_ID_MAX_LENGTH - len(suffix)].strip() + suffix diff --git a/tests/openedx_tagging/fixtures/tagging.yaml b/tests/openedx_tagging/fixtures/tagging.yaml index 9c458aeca..aff7db627 100644 --- a/tests/openedx_tagging/fixtures/tagging.yaml +++ b/tests/openedx_tagging/fixtures/tagging.yaml @@ -25,7 +25,7 @@ taxonomy: 1 parent: null value: Bacteria - external_id: null + external_id: Bacteria depth: 0 lineage: "Bacteria\t" - model: oel_tagging.tag @@ -34,7 +34,7 @@ taxonomy: 1 parent: null value: Archaea - external_id: null + external_id: Archaea depth: 0 lineage: "Archaea\t" - model: oel_tagging.tag @@ -43,7 +43,7 @@ taxonomy: 1 parent: null value: Eukaryota - external_id: null + external_id: Eukaryota depth: 0 lineage: "Eukaryota\t" - model: oel_tagging.tag @@ -52,7 +52,7 @@ taxonomy: 1 parent: 1 value: Eubacteria - external_id: null + external_id: Eubacteria depth: 1 lineage: "Bacteria\tEubacteria\t" - model: oel_tagging.tag @@ -61,7 +61,7 @@ taxonomy: 1 parent: 1 value: Archaebacteria - external_id: null + external_id: Archaebacteria depth: 1 lineage: "Bacteria\tArchaebacteria\t" - model: oel_tagging.tag @@ -70,7 +70,7 @@ taxonomy: 1 parent: 2 value: DPANN - external_id: null + external_id: DPANN depth: 1 lineage: "Archaea\tDPANN\t" - model: oel_tagging.tag @@ -79,7 +79,7 @@ taxonomy: 1 parent: 2 value: Euryarchaeida - external_id: null + external_id: Euryarchaeida depth: 1 lineage: "Archaea\tEuryarchaeida\t" - model: oel_tagging.tag @@ -88,7 +88,7 @@ taxonomy: 1 parent: 2 value: Proteoarchaeota - external_id: null + external_id: Proteoarchaeota depth: 1 lineage: "Archaea\tProteoarchaeota\t" - model: oel_tagging.tag @@ -97,7 +97,7 @@ taxonomy: 1 parent: 3 value: Animalia - external_id: null + external_id: Animalia depth: 1 lineage: "Eukaryota\tAnimalia\t" - model: oel_tagging.tag @@ -106,7 +106,7 @@ taxonomy: 1 parent: 3 value: Plantae - external_id: null + external_id: Plantae depth: 1 lineage: "Eukaryota\tPlantae\t" - model: oel_tagging.tag @@ -115,7 +115,7 @@ taxonomy: 1 parent: 3 value: Fungi - external_id: null + external_id: Fungi depth: 1 lineage: "Eukaryota\tFungi\t" - model: oel_tagging.tag @@ -124,7 +124,7 @@ taxonomy: 1 parent: 3 value: Protista - external_id: null + external_id: Protista depth: 1 lineage: "Eukaryota\tProtista\t" - model: oel_tagging.tag @@ -133,7 +133,7 @@ taxonomy: 1 parent: 3 value: Monera - external_id: null + external_id: Monera depth: 1 lineage: "Eukaryota\tMonera\t" - model: oel_tagging.tag @@ -142,7 +142,7 @@ taxonomy: 1 parent: 9 value: Arthropoda - external_id: null + external_id: Arthropoda depth: 2 lineage: "Eukaryota\tAnimalia\tArthropoda\t" - model: oel_tagging.tag @@ -151,7 +151,7 @@ taxonomy: 1 parent: 9 value: Chordata - external_id: null + external_id: Chordata depth: 2 lineage: "Eukaryota\tAnimalia\tChordata\t" - model: oel_tagging.tag @@ -160,7 +160,7 @@ taxonomy: 1 parent: 9 value: Gastrotrich - external_id: null + external_id: Gastrotrich depth: 2 lineage: "Eukaryota\tAnimalia\tGastrotrich\t" - model: oel_tagging.tag @@ -169,7 +169,7 @@ taxonomy: 1 parent: 9 value: Cnidaria - external_id: null + external_id: Cnidaria depth: 2 lineage: "Eukaryota\tAnimalia\tCnidaria\t" - model: oel_tagging.tag @@ -178,7 +178,7 @@ taxonomy: 1 parent: 9 value: Ctenophora - external_id: null + external_id: Ctenophora depth: 2 lineage: "Eukaryota\tAnimalia\tCtenophora\t" - model: oel_tagging.tag @@ -187,7 +187,7 @@ taxonomy: 1 parent: 9 value: Placozoa - external_id: null + external_id: Placozoa depth: 2 lineage: "Eukaryota\tAnimalia\tPlacozoa\t" - model: oel_tagging.tag @@ -196,7 +196,7 @@ taxonomy: 1 parent: 9 value: Porifera - external_id: null + external_id: Porifera depth: 2 lineage: "Eukaryota\tAnimalia\tPorifera\t" - model: oel_tagging.tag @@ -205,7 +205,7 @@ taxonomy: 1 parent: 15 value: Mammalia - external_id: null + external_id: Mammalia depth: 3 lineage: "Eukaryota\tAnimalia\tChordata\tMammalia\t" - model: oel_tagging.tag diff --git a/tests/openedx_tagging/import_export/test_actions.py b/tests/openedx_tagging/import_export/test_actions.py index 71e76a48c..78b43d130 100644 --- a/tests/openedx_tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/import_export/test_actions.py @@ -339,34 +339,6 @@ def test_str(self, tag_id: str, parent_id: str, expected: str): ) assert str(action) == expected - def test_str_no_external_id(self): - tag_1 = Tag( - value="Tag 5", - taxonomy=self.taxonomy, - ) - tag_1.save() - tag_2 = Tag( - value="Tag 6", - taxonomy=self.taxonomy, - parent=tag_1, - ) - tag_2.save() - tag_item = TagItem( - id=None, - value='Tag 6', - parent_id='tag_3', - ) - action = UpdateParentTag( - taxonomy=self.taxonomy, - tag=tag_item, - index=100, - ) - expected = ( - "Update the parent of (Tag 6) " - "from parent (Tag 5) to tag_3" - ) - assert str(action) == expected - @ddt.data( ('tag_100', None, False), # Tag doesn't exist on database ('tag_2', 'tag_1', False), # Parent don't change diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index bdb04a86a..924b90296 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -214,96 +214,120 @@ def test_import_with_export_output(self) -> None: assert new_tag.parent assert tag.parent.external_id == new_tag.parent.external_id - def test_import_removing_no_external_id(self) -> None: - new_taxonomy = Taxonomy(name="New taxonomy") - new_taxonomy.save() - tag1 = Tag.objects.create( - id=1000, - value="Tag 1", - taxonomy=new_taxonomy, - ) - tag2 = Tag.objects.create( - id=1001, - value="Tag 2", - taxonomy=new_taxonomy, - ) - tag3 = Tag.objects.create( - id=1002, - value="Tag 3", - taxonomy=new_taxonomy, - ) - tag1.save() - tag2.save() - tag3.save() - # Import with empty tags, to remove all tags - importFile = BytesIO(json.dumps({"tags": []}).encode()) - result, _tasks, _plan = import_export_api.import_tags( - new_taxonomy, - importFile, - ParserFormat.JSON, + def test_reimport_after_generated_external_ids_is_a_no_op(self) -> None: + """ + Export a taxonomy whose tags have auto-generated external_ids, then re-import + the exact same file. The plan should report no changes at all. + """ + self.taxonomy.add_tag("Generated Tag One") + self.taxonomy.add_tag("Generated Tag Two", parent_tag_value="Generated Tag One") + + output = import_export_api.export_tags(self.taxonomy, self.parser_format) + file = BytesIO(output.encode()) + + result, _task, plan = import_export_api.import_tags( + self.taxonomy, + file, + self.parser_format, replace=True, + plan_only=True, ) assert result + assert plan is not None + assert not plan.errors + assert all(action.name == "without_changes" for action in plan.actions) - def test_import_removing_with_childs(self) -> None: + def test_reimport_pre_upgrade_export_file_fails_safely(self) -> None: """ - Test import need to remove childs with parents that will also be removed + A file exported before this ticket's upgrade has each tag's old integer database id + in the `id` column instead of a real external_id. Re-importing it afterward (in ADD + mode, not replace) must fail safely: validation errors identifying the rows it can't + match, and no tag created, renamed, re-parented, or deleted. """ - new_taxonomy = Taxonomy(name="New taxonomy") - new_taxonomy.save() - level2 = Tag.objects.create( - id=1000, - external_id="tag_2", - value="Tag 2", - taxonomy=new_taxonomy, - ) - level1 = Tag.objects.create( - id=1001, - external_id="tag_1", - value="Tag 1", - taxonomy=new_taxonomy, - ) - level3 = Tag.objects.create( - id=1002, - external_id="tag_3", - value="Tag 3", - taxonomy=new_taxonomy, + tag = self.taxonomy.add_tag("Existing Tag") + tag_count_before = self.taxonomy.tag_set.count() + + # Simulate a pre-upgrade export: `id` is an old integer PK, not `tag.external_id`. + stale_pk = str(tag.pk) + assert stale_pk != tag.external_id # sanity check the simulation is realistic + import_file = BytesIO(json.dumps({"tags": [{"id": stale_pk, "value": tag.value}]}).encode()) + + result, _task, plan = import_export_api.import_tags( + self.taxonomy, + import_file, + self.parser_format, + replace=False, + plan_only=True, ) - level2.parent = level1 - level2.save() + assert not result + assert plan is not None + assert plan.errors + assert any("Duplicated tag value" in str(error) for error in plan.errors) + assert self.taxonomy.tag_set.count() == tag_count_before + # Confirm nothing was renamed either: + tag.refresh_from_db() + assert tag.value == "Existing Tag" + + def test_reimport_pre_upgrade_export_file_fails_safely_on_id_collision(self) -> None: + """ + Same stale-database-id scenario as above, but the id also happens to equal a + different tag's real, current external_id, so the row resolves to that other + tag instead of failing to match anyone. The import still fails safely: that + tag's value differs from the row's (Tag.value is unique per taxonomy, and the + original tag still holds it), so a rename action also applies to it and its + duplicate-value check rejects the whole plan. + """ + tag = self.taxonomy.add_tag("Existing Tag") + stale_pk = str(tag.pk) + assert stale_pk != tag.external_id # sanity check the simulation is realistic - level3.parent = level3 - level3.save() + # A different, unrelated tag whose real external_id happens to equal that stale id. + other_tag = self.taxonomy.add_tag("Unrelated Tag", external_id=stale_pk) + tag_count_before = self.taxonomy.tag_set.count() - # Import with empty tags, to remove all tags - importFile = BytesIO(json.dumps({"tags": []}).encode()) - result, _tasks, _plan = import_export_api.import_tags( - new_taxonomy, - importFile, - ParserFormat.JSON, - replace=True, + import_file = BytesIO(json.dumps({"tags": [{"id": stale_pk, "value": tag.value}]}).encode()) + + result, _task, plan = import_export_api.import_tags( + self.taxonomy, + import_file, + self.parser_format, + replace=False, + plan_only=True, ) - assert result + assert not result + assert plan is not None + assert plan.errors + assert any("Duplicated tag value" in str(error) for error in plan.errors) + assert self.taxonomy.tag_set.count() == tag_count_before + # Confirm the unrelated tag was not renamed or re-parented: + other_tag.refresh_from_db() + assert other_tag.value == "Unrelated Tag" + assert other_tag.parent is None + # And the original tag is also untouched: + tag.refresh_from_db() + assert tag.value == "Existing Tag" - def test_import_removing_with_childs_no_external_id(self) -> None: + def test_import_removing_with_childs(self) -> None: """ - Test import need to remove childs with parents that will also be removed, - using tags without external_id + Test import need to remove childs with parents that will also be removed """ new_taxonomy = Taxonomy(name="New taxonomy") new_taxonomy.save() level2 = Tag.objects.create( id=1000, + external_id="tag_2", value="Tag 2", taxonomy=new_taxonomy, ) level1 = Tag.objects.create( id=1001, + external_id="tag_1", value="Tag 1", taxonomy=new_taxonomy, ) level3 = Tag.objects.create( id=1002, + external_id="tag_3", value="Tag 3", taxonomy=new_taxonomy, ) @@ -315,28 +339,6 @@ def test_import_removing_with_childs_no_external_id(self) -> None: # Import with empty tags, to remove all tags importFile = BytesIO(json.dumps({"tags": []}).encode()) - - result, _tasks, _plan = import_export_api.import_tags( - new_taxonomy, - importFile, - ParserFormat.JSON, - replace=True, - ) - assert result - - def test_import_same_value_without_external_id(self) -> None: - new_taxonomy = Taxonomy(name="New taxonomy") - new_taxonomy.save() - - # Tag with no external_id - Tag.objects.create( - value="same_value", - taxonomy=new_taxonomy, - ) - - # Import with one tag with the same value - importFile = BytesIO(json.dumps({"tags": [{"id": "imported_tag", "value": "same_value"}]}).encode()) - result, _tasks, _plan = import_export_api.import_tags( new_taxonomy, importFile, diff --git a/tests/openedx_tagging/test_migrations.py b/tests/openedx_tagging/test_migrations.py new file mode 100644 index 000000000..ca1c5c906 --- /dev/null +++ b/tests/openedx_tagging/test_migrations.py @@ -0,0 +1,100 @@ +""" +Tests for the 0022_tag_external_id_not_null migration's backfill logic. + +Uses the `migrator` pytest fixture from django_test_migrations to run the actual +migration against historical (frozen) model states, the same way it will run against +real downstream data. +""" +from __future__ import annotations + +import pytest + +MIGRATE_FROM = ("oel_tagging", "0021_remove_system_defined_add_read_only") +MIGRATE_TO = ("oel_tagging", "0022_tag_external_id_not_null") + + +@pytest.mark.django_db +def test_backfill_generates_missing_external_ids(migrator) -> None: + """ + Tags with a NULL external_id get one generated from their value; tags that already + have one are left untouched; a collision between a generated value and a pre-existing, + hand-assigned external_id is resolved to two distinct identifiers. + """ + old_state = migrator.apply_initial_migration(MIGRATE_FROM) + Taxonomy = old_state.apps.get_model("oel_tagging", "Taxonomy") + Tag = old_state.apps.get_model("oel_tagging", "Tag") + + taxonomy = Taxonomy.objects.create(name="Migration Test", export_id="migration_test") + + plain_tag = Tag.objects.create( + taxonomy=taxonomy, value="Plain Tag", external_id=None, depth=0, lineage="Plain Tag\t", + ) + long_value = "L" * 300 + long_tag = Tag.objects.create( + taxonomy=taxonomy, value=long_value, external_id=None, depth=0, lineage=long_value + "\t", + ) + # An institution-assigned external_id that happens to match another tag's value exactly: + institution_tag = Tag.objects.create( + taxonomy=taxonomy, value="Institution Value", external_id="Colliding Value", + depth=0, lineage="Institution Value\t", + ) + colliding_tag = Tag.objects.create( + taxonomy=taxonomy, value="Colliding Value", external_id=None, depth=0, lineage="Colliding Value\t", + ) + # An empty string is also "no external_id": the field has always been blank=True, + # and downstream data outside this repo's own code paths could hold "" instead of NULL. + empty_string_tag = Tag.objects.create( + taxonomy=taxonomy, value="Empty String Tag", external_id="", depth=0, lineage="Empty String Tag\t", + ) + + new_state = migrator.apply_tested_migration(MIGRATE_TO) + NewTag = new_state.apps.get_model("oel_tagging", "Tag") + + new_plain_tag = NewTag.objects.get(pk=plain_tag.pk) + new_long_tag = NewTag.objects.get(pk=long_tag.pk) + new_institution_tag = NewTag.objects.get(pk=institution_tag.pk) + new_colliding_tag = NewTag.objects.get(pk=colliding_tag.pk) + new_empty_string_tag = NewTag.objects.get(pk=empty_string_tag.pk) + + assert new_plain_tag.external_id == "Plain Tag" + assert new_empty_string_tag.external_id == "Empty String Tag" + + # Truncated, non-empty, and unique in its taxonomy: + assert new_long_tag.external_id + assert len(new_long_tag.external_id) <= 255 + + # The institution-assigned value must not be overwritten by the collision... + assert new_institution_tag.external_id == "Colliding Value" + # ...and the colliding tag must get a different, generated identifier instead: + assert new_colliding_tag.external_id is not None + assert new_colliding_tag.external_id != new_institution_tag.external_id + + all_external_ids = list( + NewTag.objects.filter(taxonomy_id=taxonomy.pk).values_list("external_id", flat=True) + ) + assert None not in all_external_ids + assert "" not in all_external_ids + assert len(all_external_ids) == len({eid.casefold() for eid in all_external_ids}) + + +@pytest.mark.django_db +def test_reverse_migration_is_a_noop_that_keeps_data(migrator) -> None: + """ + Migrating back down only loosens the NOT NULL constraint; it doesn't delete the + external_id values that were generated going forward. + """ + old_state = migrator.apply_initial_migration(MIGRATE_FROM) + Taxonomy = old_state.apps.get_model("oel_tagging", "Taxonomy") + Tag = old_state.apps.get_model("oel_tagging", "Tag") + + taxonomy = Taxonomy.objects.create(name="Rollback Test", export_id="rollback_test") + tag = Tag.objects.create( + taxonomy=taxonomy, value="Rollback Tag", external_id=None, depth=0, lineage="Rollback Tag\t", + ) + + migrator.apply_tested_migration(MIGRATE_TO) + reverted_state = migrator.apply_tested_migration(MIGRATE_FROM) + RevertedTag = reverted_state.apps.get_model("oel_tagging", "Tag") + + reverted_tag = RevertedTag.objects.get(pk=tag.pk) + assert reverted_tag.external_id == "Rollback Tag" diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 06ebf7763..abcdc9728 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -17,7 +17,7 @@ from openedx_tagging import api from openedx_tagging.models import ObjectTag, Tag, Taxonomy -from openedx_tagging.models.utils import RESERVED_TAG_CHARS +from openedx_tagging.models.utils import RESERVED_TAG_CHARS, TAG_EXTERNAL_ID_MAX_LENGTH, tag_external_id_candidate from openedx_tagging.signal_handlers import _is_explicit_tag_delete from openedx_tagging.tasks import ( emit_content_object_associations_changed_for_object_ids_task, @@ -284,14 +284,15 @@ def test_get_root(self) -> None: get_filtered_tags(). """ result = list(self.taxonomy.get_filtered_tags(depth=1)) - common_fields = {"depth": 0, "parent_value": None, "external_id": None} + common_fields = {"depth": 0, "parent_value": None} for r in result: del r["_id"] # Remove the internal database IDs; they aren't interesting here and a other tests check them assert result == [ - # These are the root tags, in alphabetical order: - {"value": "Archaea", "child_count": 3, **common_fields}, - {"value": "Bacteria", "child_count": 2, **common_fields}, - {"value": "Eukaryota", "child_count": 5, **common_fields}, + # These are the root tags, in alphabetical order. The fixture gives each tag an + # external_id equal to its own value. + {"value": "Archaea", "child_count": 3, "external_id": "Archaea", **common_fields}, + {"value": "Bacteria", "child_count": 2, "external_id": "Bacteria", **common_fields}, + {"value": "Eukaryota", "child_count": 5, "external_id": "Eukaryota", **common_fields}, ] def test_get_child_tags_one_level(self) -> None: @@ -300,16 +301,17 @@ def test_get_child_tags_one_level(self) -> None: the closed taxonomy, using get_filtered_tags(). With counts included. """ result = list(self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Eukaryota")) - common_fields = {"depth": 1, "parent_value": "Eukaryota", "external_id": None} + common_fields = {"depth": 1, "parent_value": "Eukaryota"} for r in result: del r["_id"] # Remove the internal database IDs; they aren't interesting here and a other tests check them assert result == [ - # These are the Eukaryota tags, in alphabetical order: - {"value": "Animalia", "child_count": 7, **common_fields}, - {"value": "Fungi", "child_count": 0, **common_fields}, - {"value": "Monera", "child_count": 0, **common_fields}, - {"value": "Plantae", "child_count": 0, **common_fields}, - {"value": "Protista", "child_count": 0, **common_fields}, + # These are the Eukaryota tags, in alphabetical order. The fixture gives each tag an + # external_id equal to its own value. + {"value": "Animalia", "child_count": 7, "external_id": "Animalia", **common_fields}, + {"value": "Fungi", "child_count": 0, "external_id": "Fungi", **common_fields}, + {"value": "Monera", "child_count": 0, "external_id": "Monera", **common_fields}, + {"value": "Plantae", "child_count": 0, "external_id": "Plantae", **common_fields}, + {"value": "Protista", "child_count": 0, "external_id": "Protista", **common_fields}, ] def test_get_grandchild_tags_one_level(self) -> None: @@ -318,18 +320,19 @@ def test_get_grandchild_tags_one_level(self) -> None: "Eukaryota" root tag in the closed taxonomy, using get_filtered_tags(). """ result = list(self.taxonomy.get_filtered_tags(depth=1, parent_tag_value="Animalia")) - common_fields = {"depth": 2, "parent_value": "Animalia", "external_id": None} + common_fields = {"depth": 2, "parent_value": "Animalia"} for r in result: del r["_id"] # Remove the internal database IDs; they aren't interesting here and a other tests check them assert result == [ - # These are the Eukaryota tags, in alphabetical order: - {"value": "Arthropoda", "child_count": 0, **common_fields}, - {"value": "Chordata", "child_count": 1, **common_fields}, - {"value": "Cnidaria", "child_count": 0, **common_fields}, - {"value": "Ctenophora", "child_count": 0, **common_fields}, - {"value": "Gastrotrich", "child_count": 0, **common_fields}, - {"value": "Placozoa", "child_count": 0, **common_fields}, - {"value": "Porifera", "child_count": 0, **common_fields}, + # These are the Eukaryota tags, in alphabetical order. The fixture gives each tag an + # external_id equal to its own value. + {"value": "Arthropoda", "child_count": 0, "external_id": "Arthropoda", **common_fields}, + {"value": "Chordata", "child_count": 1, "external_id": "Chordata", **common_fields}, + {"value": "Cnidaria", "child_count": 0, "external_id": "Cnidaria", **common_fields}, + {"value": "Ctenophora", "child_count": 0, "external_id": "Ctenophora", **common_fields}, + {"value": "Gastrotrich", "child_count": 0, "external_id": "Gastrotrich", **common_fields}, + {"value": "Placozoa", "child_count": 0, "external_id": "Placozoa", **common_fields}, + {"value": "Porifera", "child_count": 0, "external_id": "Porifera", **common_fields}, ] def test_get_depth_1_search_term(self) -> None: @@ -343,7 +346,7 @@ def test_get_depth_1_search_term(self) -> None: "child_count": 3, "depth": 0, "parent_value": None, - "external_id": None, + "external_id": "Archaea", # The fixture gives this tag an external_id equal to its own value "_id": 2, # These IDs are hard-coded in the test fixture file }, ] @@ -360,7 +363,7 @@ def test_get_depth_1_child_search_term(self) -> None: "child_count": 0, "depth": 1, "parent_value": "Bacteria", - "external_id": None, + "external_id": "Archaebacteria", # The fixture gives this tag an external_id equal to its own value "_id": 5, # These IDs are hard-coded in the test fixture file }, ] @@ -467,7 +470,7 @@ def test_tags_deep(self) -> None: "parent_value": "Chordata", "depth": 3, "child_count": 0, - "external_id": None, + "external_id": "Mammalia", # The fixture gives this tag an external_id equal to its own value "_id": 21, # These IDs are hard-coded in the test fixture file } ] @@ -1203,3 +1206,100 @@ def test_emit_content_object_associations_changed_for_tag_task(self, mock_signal for call in mock_signal.send_event.call_args_list } assert emitted_object_ids == {first_object_id, second_object_id} + + +class TestTagExternalIdCandidate(TestCase): + """ + Direct unit tests for the tag_external_id_candidate() helper. + """ + + def test_bare_value_round_trips(self) -> None: + assert tag_external_id_candidate("Bacteria", 1) == "Bacteria" + + def test_long_value_truncates(self) -> None: + value = "x" * (TAG_EXTERNAL_ID_MAX_LENGTH + 50) + candidate = tag_external_id_candidate(value, 1) + assert candidate == value[:TAG_EXTERNAL_ID_MAX_LENGTH] + assert len(candidate) == TAG_EXTERNAL_ID_MAX_LENGTH + + def test_whitespace_at_truncation_boundary_is_stripped(self) -> None: + # Construct a value where the character landing exactly at the truncation + # boundary is whitespace, so the naive slice would leave a trailing space. + value = ("a" * (TAG_EXTERNAL_ID_MAX_LENGTH - 1)) + " " + "bbbbb" + candidate = tag_external_id_candidate(value, 1) + assert candidate == "a" * (TAG_EXTERNAL_ID_MAX_LENGTH - 1) + assert not candidate.endswith(" ") + + def test_different_attempts_differ(self) -> None: + value = "Some Tag" + assert tag_external_id_candidate(value, 1) != tag_external_id_candidate(value, 2) + + +class TestTagExternalIdGeneration(TestTagTaxonomyMixin, TestCase): + """ + Tests for the three entry points that generate a Tag's external_id. + """ + + def test_tag_save_generates_external_id_from_value(self) -> None: + tag = Tag(taxonomy=self.taxonomy, value="Generated From Save") + tag.save() + assert tag.external_id == "Generated From Save" + + def test_bare_tag_create_generates_external_id(self) -> None: + # Proves the generation logic lives in Tag.save(), not just in Taxonomy.add_tag(). + tag = Tag.objects.create(taxonomy=self.taxonomy, value="Bare Create Tag") + assert tag.external_id == "Bare Create Tag" + + def test_add_tag_generates_external_id(self) -> None: + tag = self.taxonomy.add_tag("Added Via Taxonomy") + assert tag.external_id == "Added Via Taxonomy" + + def test_add_tag_duplicate_external_id_raises(self) -> None: + self.taxonomy.add_tag("First Tag", external_id="dup-id") + with pytest.raises(ValueError): + self.taxonomy.add_tag("Second Tag", external_id="dup-id") + # Also raises on a case-different duplicate: + with pytest.raises(ValueError): + self.taxonomy.add_tag("Third Tag", external_id="DUP-ID") + + def test_rename_preserves_external_id(self) -> None: + original_external_id = self.bacteria.external_id + self.bacteria.value = "Renamed Bacteria" + self.bacteria.save() + assert self.bacteria.external_id == original_external_id + + def test_save_resolves_generated_external_id_collision(self) -> None: + """ + Two tags whose values share the same first 255 characters would otherwise + generate the same attempt-1 candidate; Tag.save()'s own collision branch (not + the migration's separate copy of the same logic) must resolve that here. + """ + value_one = "X" * (TAG_EXTERNAL_ID_MAX_LENGTH + 45) + value_two = ("X" * TAG_EXTERNAL_ID_MAX_LENGTH) + ("Y" * 50) + + first = Tag.objects.create(taxonomy=self.taxonomy, value=value_one) + second = Tag.objects.create(taxonomy=self.taxonomy, value=value_two) + + assert first.external_id == "X" * TAG_EXTERNAL_ID_MAX_LENGTH + assert second.external_id != first.external_id + assert second.external_id + assert second.external_id == tag_external_id_candidate(value_two, 2) + assert second.external_id.endswith("-2") + + def test_read_only_taxonomy_tags_get_external_id_and_stay_read_only(self) -> None: + """ + A tag created directly on a read-only taxonomy (bypassing add_tag()'s own + read_only check, e.g. as a plugin writing directly to the model might) still + gets a generated external_id, and the taxonomy's read_only enforcement in + add_tag()/update_tag()/delete_tags() is unaffected by that. + """ + read_only_taxonomy = Taxonomy.objects.create(name="RO Test", read_only=True) + tag = Tag.objects.create(taxonomy=read_only_taxonomy, value="RO Tag") + assert tag.external_id == "RO Tag" + + with pytest.raises(ValueError): + read_only_taxonomy.add_tag("Another Tag") + with pytest.raises(ValueError): + read_only_taxonomy.update_tag("RO Tag", "Renamed") + with pytest.raises(ValueError): + read_only_taxonomy.delete_tags(["RO Tag"]) diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index da933f7c0..8848f513c 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -1969,7 +1969,8 @@ def test_create_tag_in_taxonomy(self): self.assertIsNotNone(data.get("_id")) self.assertEqual(data.get("value"), new_tag_value) self.assertIsNone(data.get("parent_value")) - self.assertIsNone(data.get("external_id")) + # No external_id was supplied, so one is generated from the tag's name. + self.assertEqual(data.get("external_id"), new_tag_value) self.assertIsNone(data.get("sub_tags_link")) self.assertEqual(data.get("child_count"), 0) @@ -2000,6 +2001,42 @@ def test_create_tag_in_taxonomy_with_parent(self): self.assertIsNone(data.get("sub_tags_link")) self.assertEqual(data.get("child_count"), 0) + def test_create_tag_in_taxonomy_with_duplicate_external_id(self): + self.client.force_authenticate(user=self.staff) + existing_external_id = "dup-ext-id" + + create_data = { + "tag": "First Tag With External Id", + "external_id": existing_external_id, + } + response = self.client.post( + self.small_taxonomy_url, create_data, format="json" + ) + assert response.status_code == status.HTTP_201_CREATED + + tag_count_before = self.small_taxonomy.tag_set.count() + + # Exact duplicate external_id: + response = self.client.post( + self.small_taxonomy_url, + {"tag": "Second Tag", "external_id": existing_external_id}, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert existing_external_id in str(response.data) + + # Duplicate that only differs in case: + response = self.client.post( + self.small_taxonomy_url, + {"tag": "Third Tag", "external_id": existing_external_id.upper()}, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert existing_external_id.upper() in str(response.data) + + # Neither rejected request should have created a new tag: + assert self.small_taxonomy.tag_set.count() == tag_count_before + def test_create_tag_in_invalid_taxonomy(self): self.client.force_authenticate(user=self.staff) new_tag_value = "New Tag" diff --git a/uv.lock b/uv.lock index bab5c92ca..1f4ef3255 100644 --- a/uv.lock +++ b/uv.lock @@ -711,6 +711,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/83/65/4d73fce956b5ebf26449259664360e539fbe95f0e86be396c28b636ba72a/django_stubs_ext-6.0.7-py3-none-any.whl", hash = "sha256:53a9c7c5a7c7e718cc6308cfce1e7470f2cac0b9d38dbcd60fbfa82704f1d592", size = 10362, upload-time = "2026-07-14T10:07:55.653Z" }, ] +[[package]] +name = "django-test-migrations" +version = "1.7.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/0f/26/6518a878dcdf5de8c20eee254a8dc942d8e907edbc5e131c1c3057837705/django_test_migrations-1.7.0.tar.gz", hash = "sha256:9b199fa3817b28846455d8a13434eed44d15cd1bd89a5144902f8e5212bc1199", size = 20635, upload-time = "2026-09-23T09:32:41.042Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/8c/3f/32851cdb5850136349ba5d7f5e2536e9f3460452b20508460f91218b4b6d/django_test_migrations-1.7.0-py3-none-any.whl", hash = "sha256:409d5437f7a2a9a47c4c9384902c8fa6d072c142f73b1d75a6df310831f9edea", size = 25542, upload-time = "2026-09-23T09:32:42.105Z" }, +] + [[package]] name = "django-waffle" version = "5.0.0" @@ -1587,6 +1596,7 @@ dev = [ { name = "django" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "doc8" }, { name = "edx-i18n-tools" }, @@ -1617,6 +1627,7 @@ doc = [ { name = "django" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "doc8" }, { name = "freezegun" }, @@ -1640,6 +1651,7 @@ quality = [ { name = "django" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "edx-lint" }, { name = "freezegun" }, @@ -1662,6 +1674,7 @@ test = [ { name = "django" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "freezegun" }, { name = "import-linter" }, @@ -1678,6 +1691,7 @@ test-base = [ { name = "ddt" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "freezegun" }, { name = "import-linter" }, @@ -1716,6 +1730,7 @@ dev = [ { name = "django", specifier = ">=5.2,<6.0" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "doc8" }, { name = "edx-i18n-tools" }, @@ -1746,6 +1761,7 @@ doc = [ { name = "django", specifier = ">=5.2,<6.0" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "doc8" }, { name = "freezegun" }, @@ -1769,6 +1785,7 @@ quality = [ { name = "django", specifier = ">=5.2,<6.0" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "edx-lint" }, { name = "freezegun" }, @@ -1791,6 +1808,7 @@ test = [ { name = "django", specifier = ">=5.2,<6.0" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "freezegun" }, { name = "import-linter" }, @@ -1807,6 +1825,7 @@ test-base = [ { name = "ddt" }, { name = "django-debug-toolbar" }, { name = "django-stubs" }, + { name = "django-test-migrations" }, { name = "djangorestframework-stubs" }, { name = "freezegun" }, { name = "import-linter" },