docs: add ADR for non-nullable tag external_id#658
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @mgwozdz-unicon! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Generally this makes sense to me.
I'm just wondering about the algorithm for deriving the default external_id. I think I would prefer just using value directly or using the uppercase version of value (perhaps with spaces replaced with underscores) as the default for backfilling (with a number added if needed, but it almost never will be since value is also unique per taxonomy and most taxonomies either have external IDs for all values or none).
The reason is that I expect we'll be displaying these values in the UI, and in the taxonomies I've looked at there are often a lot of tags that start with similar prefixes. So I think things like ADMINISTRATIVE_FUNCTIONS, ADMINISTRATION, ADMINISTRATIVE_SOFTWARE are better external_ids than Adminis, Adminis2, Adminis3, and will be less confusing to users when they're seen.
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
Looks great to me, just have a look at my comment about the 7-character cutoff.
| 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. |
There was a problem hiding this comment.
Nit. For clarification: this just means that if we don't have nullable external_id, then every external_id in the same taxonomy must be unique and we can't use a workaround like a string to avoid external_id if it's not available, correct?
And this is does not pose a problem for us because the "value" field already is unique per taxonomy, so just backfilling to external_id will have external_id be unique.
| 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 |
There was a problem hiding this comment.
Nit. Sounds fine, but curious why not two migrations - a data migration for backfill followed by a migration with this constraint?
| 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 |
There was a problem hiding this comment.
Why the first 7 characters, not all of the value?
Taking the first 7 characters seems to create unnecessary collisions because we have skills like
"data" and "data science" and such that share the same initial characters. And having external ids like "data1" and "data2" for "data" and "data science" could be misleading.
Values are already used as unique labels / identifiers. Wouldn't copying the whole value string (if necessary adding some affixes) be cleaner? Is there any drawback to it?
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.
|
@bradenmacdonald Agreed, and it turns out to simplify things really nicely. Because @jesperhodge, addressed your three comments:
|
| 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. |
There was a problem hiding this comment.
It is possible I think for some weird taxonomy to exist where only some tags have external IDs specified and one of those existing external IDs is the same as the value of a different tag that doesn't yet have an external ID. This should be an extremely rare case, maybe never encountered at all, but it is possible, and if it happened, then some sort of conflict handling (append a number etc.) would be required.
There was a problem hiding this comment.
You're right. I committed an update to address this.
github-project-automation
Bot
moved this from Ready for Review
to Done
in Contributions
6 participants
Why
CBE needs every tag to have an
external_idso it can be associated with objects as part of competency criteria. Non-CBE taxonomies are expected to want a mandatory, institution-assigned identifier too, particularly once there's a need to convert an existing regular taxonomy into a Competency Taxonomy.Summary
Tag.external_idNOT NULLat the database level, for every taxonomy.openedx/openedx-core's ADR 0010 (mutableexternal_id): backfills every existing NULL row with a value derived from the tag'svalue(first ~7 characters, with a deterministic numeric-counter fallback on collision), then adds the constraint in the same migration.external_id(REST API, import, UI) get one auto-generated the same way if not provided, rather than the request being rejected; existing integrations that omitexternal_idtoday don't need to change.