feat: make Tag.external_id non-nullable, auto-generated when omitted

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.

pyproject.toml

@@ -102,6 +102,7 @@ test-base = [
     "ddt",
     "django-debug-toolbar",
     "django-stubs",
+    "django-test-migrations",
     "djangorestframework-stubs",
     "freezegun",
     "import-linter",

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)
 

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,

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):
         """

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,
+            ),
+        ),
+    ]

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

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