From ae9f0cb6251846450f22ed8411b216757cb8bffc Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Fri, 10 Jul 2026 15:13:44 -0700 Subject: [PATCH 1/3] docs: add ADR for non-nullable tag external_id --- .../0012-non-nullable-tag-external-id.rst | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst 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 new file mode 100644 index 000000000..73d294bc6 --- /dev/null +++ b/docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst @@ -0,0 +1,101 @@ +.. _openedx-tagging-adr-0012: + +12. Non-nullable tag ``external_id`` +===================================== + +Status +------ + +Proposed + +Context +------- + +`ADR 0010 <0010-mutable-tag-external-id.rst>`_ made ``Tag.external_id`` mutable through an +import-only rename pathway, to serve as the institution-editable "Competency ID" for +Competency-Based Education (CBE) tags. That ADR left ``external_id`` nullable, scoped to +competency taxonomies. + +Building the CBE competency workflow surfaced a stronger requirement: every tag needs a +value for ``external_id`` in order to be able to associate it with objects as part of +competency criteria. It's also anticipated that non-CBE taxonomies will want a mandatory, +institution-assigned identifier in the future, particularly when there becomes a desire to +convert regular taxonomies into Competency Taxonomies. + +Today, ``external_id`` is nullable specifically to work around its own +``unique_together(taxonomy, external_id)`` constraint: NULL is the only value Postgres, +MySQL, and SQLite all treat as exempt from a uniqueness constraint, so it's the +mechanism that lets multiple tags in the same taxonomy have "no external_id" at once. +Making the field mandatory means every existing NULL row in every current Open edX +instance needs a real value before the constraint can be added, since this is a +published library whose migrations run unmodified against arbitrary downstream data. + +Decision +-------- + +Make ``Tag.external_id`` ``NOT NULL`` at the database level, for every taxonomy, in one +migration. + +- **Backfill for existing rows.** A ``RunPython`` step generates a value for every tag + whose ``external_id`` is currently NULL, following the same + backfill-then-constrain shape as the ``depth``/``lineage`` migration + (``0020_tag_depth_and_lineage.py``): populate real values first, then add the + constraint in the same migration once every row satisfies it. +- **Backfill algorithm.** Take the first ~7 characters of the tag's ``value`` (matching + ``external_id``'s existing case-insensitive collation). If that prefix collides with + another tag's ``external_id`` already in the same taxonomy, append a numeric counter + to the prefix and keep incrementing it until it's unique. This is deterministic and + bounded: it never needs an unbounded retry loop, since appending an incrementing + counter to an already-short prefix is guaranteed to terminate within the taxonomy's + tag count. +- **New tags going forward.** The REST API and import format keep treating + ``external_id`` as optional for the caller, unchanged from today. When a tag is + created without one, the same prefix-plus-collision-fallback algorithm generates one + automatically, rather than rejecting the request. No existing integration that omits + ``external_id`` today has to change. +- **Institutions can replace an auto-generated value.** If an institution doesn't want + the auto-generated ``external_id``, they can change it to their own value through + ADR 0010's rename pathway. + +This is not treated as a breaking change to the public REST API, and doesn't go through +the DEPR process. The request side (``external_id`` on tag creation) stays optional, so +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. + +Future considerations +~~~~~~~~~~~~~~~~~~~~~~ + +If an institution converts an existing regular taxonomy to a CBE taxonomy, its tags +will already have a non-null ``external_id`` (auto-generated or backfilled) from this +decision, with no separate migration needed for that conversion. + +Rejected Alternatives +---------------------- + +Derive backfilled values from the tag's primary key +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Still needs a collision fallback: a tag could already have a hand-assigned +``external_id`` that happens to match another tag's ``id``. Also rejected because +``id`` is meant to stay internal (per the README's model conventions, ``uuid`` is the +documented identifier for external reference), and because it produces a value with no +relationship to the tag (e.g. ``482913``), which reads as meaningless to institutions. + +Require callers to supply ``external_id`` on tag creation +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Simpler mechanically than auto-generation, but rejected because it's a breaking change +for the REST API and any import/UI flow that omits ``external_id`` today. + +Scope this decision to CBE/competency taxonomies only, for now +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Rejected because taxonomies later converted to Competency Taxonomies will want this too. + +Changelog +--------- + +2026-07-10: + +* Proposed. From f348601ba9c3c9f9e846b5c014d7880915dea5d3 Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Fri, 10 Jul 2026 16:57:19 -0700 Subject: [PATCH 2/3] docs: derive backfilled external_id from tag value Set external_id to the tag's existing value instead of a fixed-length prefix, per review feedback. Since (taxonomy, value) already carries the same unique constraint being added to external_id, this is guaranteed collision-free with no fallback logic needed. --- .../0012-non-nullable-tag-external-id.rst | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) 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 73d294bc6..0237a279d 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 @@ -29,6 +29,9 @@ mechanism that lets multiple tags in the same taxonomy have "no external_id" at Making the field mandatory means every existing NULL row in every current Open edX instance needs a real value before the constraint can be added, since this is a published library whose migrations run unmodified against arbitrary downstream data. +``Tag.value`` already carries the same per-taxonomy, case-insensitive uniqueness +constraint (``unique_together(taxonomy, value)``), so backfilling ``external_id`` from +``value`` cannot introduce a new collision under the constraint being added here. Decision -------- @@ -41,18 +44,17 @@ migration. backfill-then-constrain shape as the ``depth``/``lineage`` migration (``0020_tag_depth_and_lineage.py``): populate real values first, then add the constraint in the same migration once every row satisfies it. -- **Backfill algorithm.** Take the first ~7 characters of the tag's ``value`` (matching - ``external_id``'s existing case-insensitive collation). If that prefix collides with - another tag's ``external_id`` already in the same taxonomy, append a numeric counter - to the prefix and keep incrementing it until it's unique. This is deterministic and - bounded: it never needs an unbounded retry loop, since appending an incrementing - counter to an already-short prefix is guaranteed to terminate within the taxonomy's - tag count. +- **Backfill algorithm.** Set ``external_id`` to the tag's existing ``value``. Since + ``(taxonomy, value)`` is already a unique, case-insensitive constraint matching + ``external_id``'s own, this is guaranteed unique with no collision handling needed. + Implementations may transform the copied value for UI legibility, for example + uppercasing it and replacing spaces with underscores, but no particular format is + required by this decision. - **New tags going forward.** The REST API and import format keep treating ``external_id`` as optional for the caller, unchanged from today. When a tag is - created without one, the same prefix-plus-collision-fallback algorithm generates one - automatically, rather than rejecting the request. No existing integration that omits - ``external_id`` today has to change. + created without one, the same backfill algorithm generates one automatically, rather + than rejecting the request. No existing integration that omits ``external_id`` today + has to change. - **Institutions can replace an auto-generated value.** If an institution doesn't want the auto-generated ``external_id``, they can change it to their own value through ADR 0010's rename pathway. From 6916b5d8149d0c55f67b6fed26980c7f41facf0c Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Mon, 13 Jul 2026 06:39:27 -0600 Subject: [PATCH 3/3] docs: handle rare external_id backfill collision with existing value --- .../0012-non-nullable-tag-external-id.rst | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) 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 0237a279d..26732a0af 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 @@ -31,7 +31,9 @@ instance needs a real value before the constraint can be added, since this is a published library whose migrations run unmodified against arbitrary downstream data. ``Tag.value`` already carries the same per-taxonomy, case-insensitive uniqueness constraint (``unique_together(taxonomy, value)``), so backfilling ``external_id`` from -``value`` cannot introduce a new collision under the constraint being added here. +``value`` is collision-free in virtually every case. The one edge case is a tag whose +``value`` matches a different tag's pre-existing, hand-assigned ``external_id`` in the +same taxonomy, which the backfill algorithm below handles. Decision -------- @@ -44,12 +46,14 @@ migration. backfill-then-constrain shape as the ``depth``/``lineage`` migration (``0020_tag_depth_and_lineage.py``): populate real values first, then add the constraint in the same migration once every row satisfies it. -- **Backfill algorithm.** Set ``external_id`` to the tag's existing ``value``. Since - ``(taxonomy, value)`` is already a unique, case-insensitive constraint matching - ``external_id``'s own, this is guaranteed unique with no collision handling needed. - Implementations may transform the copied value for UI legibility, for example - uppercasing it and replacing spaces with underscores, but no particular format is - required by this decision. +- **Backfill algorithm.** Set ``external_id`` to the tag's existing ``value``. In the + rare case where that collides with a different tag's pre-existing, hand-assigned + ``external_id`` in the same taxonomy, the collision must be resolved deterministically + without an unbounded retry loop, for example by appending an incrementing numeric + counter to the copied value until it's unique. Implementations may also transform the + copied value for UI legibility, for example uppercasing it and replacing spaces with + underscores, but no particular format or collision-resolution scheme is required by + this decision. - **New tags going forward.** The REST API and import format keep treating ``external_id`` as optional for the caller, unchanged from today. When a tag is created without one, the same backfill algorithm generates one automatically, rather @@ -78,11 +82,12 @@ Rejected Alternatives Derive backfilled values from the tag's primary key ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Still needs a collision fallback: a tag could already have a hand-assigned -``external_id`` that happens to match another tag's ``id``. Also rejected because -``id`` is meant to stay internal (per the README's model conventions, ``uuid`` is the -documented identifier for external reference), and because it produces a value with no -relationship to the tag (e.g. ``482913``), which reads as meaningless to institutions. +Needs the same kind of collision fallback as the ``value``-derived approach: a tag could +already have a hand-assigned ``external_id`` that happens to match another tag's ``id``. +Also rejected because ``id`` is meant to stay internal (per the README's model +conventions, ``uuid`` is the documented identifier for external reference), and because +it produces a value with no relationship to the tag (e.g. ``482913``), which reads as +meaningless to institutions. Require callers to supply ``external_id`` on tag creation ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~