docs: add ADR for competency taxonomy detection#662
Conversation
openedx_tagging needs a layering-safe way to report whether a Taxonomy is competency-enabled (openedx#618), without gaining any knowledge of the CompetencyTaxonomy subclass CBE owns. Adds a taxonomy_type field to the base Taxonomy model set by the caller that creates a CompetencyTaxonomy row, and documents why two prior approaches on openedx#618 (an openedx_tagging-side hasattr check, an overridable get_type() method) were unsound. Amends ADR 0002 to note the corresponding creation-time invariant and that its rejection of a flat taxonomy_type column still stands, unaffected by this narrower field.
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. |
| whether a ``CompetencyTaxonomy`` row actually exists for that taxonomy. This is accepted | ||
| because it is enforced at a single transactional chokepoint (the creation path for a | ||
| ``CompetencyTaxonomy``), not as an ongoing invariant that application code must maintain | ||
| across multiple call sites. |
There was a problem hiding this comment.
I'm not sure about this.
Add a
taxonomy_typefield directly toTaxonomy: ATaxonomyType(models.TextChoices)enum with valuesTAGS(default) andCOMPETENCY
So we're still hard-coding a reference to COMPETENCY in the tagging app, and we now have to ensure that we keep the field's value in sync with whether or not a CompetencyTaxonomy actually exists.
Plus, per the ADR 614, we have to update a whole bunch of other tagging REST APIs to support the taxonomy_type field.
If we're going to hard-code some reference to Competencies in the tagging app, I think what I'm leaning toward is something radically simpler:
Put this in the taxonomy list/get serializer, and that's it - no other changes needed to the taxonomy app.
# Technically this is a layering violation as our tagging/taxonomy app is not supposed
# to be aware of competencies, but it simplifies a lot for us and it doesn't require
# importing any competency code.
# We can revisit this in the future if we introduce other taxonomy types.
taxonomy_type = "competency" if hasattr(instance, "competencytaxonomy") else "tags"REST APIs: I don't think there's any need to change the tagging REST APIs. To create a competency taxonomy from the frontend, you could either (A) call the "create taxonomy" REST API and on succes, call the "create competency metadata for taxonomy" REST API (from the cbe app), or (B) call a competency REST API that creates the Taxonomy and the CompetencyTaxonomy object together in a single request.
Export: You could add the taxonomy_type field to the JSON export data if that seems useful. It's not possible to include on CSVs.
Import: If the user chooses to import something as a Competency taxonomy (in the frontend UI), it's the same thing: import the taxonomy then call a second API to convert it to Competency.
There was a problem hiding this comment.
Going through in order:
On the sync-drift point: yes, I agree that this tradeoff is the risk, but hopefully it can be resolved at implementation time.
On the REST API surface per #614: I don't think ADR 13 is adding work there. Those endpoints need to accept taxonomy_type on create/import regardless of how the Get side reports it, so that's #614's scope either way, not something this ADR introduces on top.
On the hasattr suggestion: I hear the appeal, it's less code today, and I get that you're framing it as a deliberate, called-out layering violation rather than an accidental one. Thinking it through more, I don't think either approach avoids touching openedx_tagging when a new type shows up eventually, the field approach still means adding a value to the shared enum. Where I think they differ is what that touch requires: with the field, adding a type is adding a string to a catalog, the read path (TaxonomySerializer) doesn't change, it already just returns whatever's stored. With hasattr, adding a type means adding another branch to the serializer's logic that names a specific downstream app's MTI relation, so the read path itself keeps growing a fact about another app's model internals with every flavor. That feels like the thing the ADR is trying to keep out of a generic library, holding the type as a piece of data the caller owns, versus holding it as code that inspects other apps' structure. Also worth flagging: this looks close to option 2 from your comment on #618 (Adding a tiny bit of Competency awareness to the base tagging app), where you said you leaned toward 1+3 over it. Did something change your thinking there, or is this meant as a variant of option 1 rather than 2?
On separate Competency REST APIs: Since the Taxonomies landing page is meant to serve both flows, I don't think it makes sense to make the frontend juggle two calls, especially when this could become even more calls than that if the number of taxonomy types increases.
On export/import: agreed, that's already settled in #614 and not really in scope here.
There was a problem hiding this comment.
Yeah, sorry, I'm still not happy with any of the approaches here so I'm just throwing out ideas to see if there's anything that you like. I'm not so unhappy that I'm going to block anything though. I'm reluctantly fine with any of these approaches, but I want to see if we can come up with something clean that doesn't have any layering violations.
Thinking about this more, what if we kept the taxonomy code in openedx-core / oel_tagging completely free of competency awareness, but use the platform's override of the REST API at openedx/core/djangoapps/content_tagging/rest_api/v1/ to inject the competency pieces you need in the REST API? Then you could add taxonomy_type = "competency" if hasattr(instance, "competencytaxonomy") else "tags" to the serializer and also wire through the "competency creation" changes to the REST API specified in #614 while still keeping some semblance of layer separation, because the core tagging API is unchanged.
So the layers would be:
oel_taggingpython API & REST APIs in this repo - pure tagging/taxonomy only ✅content_taggingpython API in openedx-platform - pure tagging/taxonomy only ✅ (as specified on [BE] Update taxonomy Create & Import endpoints to accept Taxonomy Type #614 , "Layering constraint. openedx_tagging/api.py::create_taxonomy() creates only a plain Taxonomy. It must not import or call anything from openedx_learning. The CompetencyTaxonomy creation belongs exclusively in the CBE applet.")- CBE django app python API - can convert taxonomies to competencies, provide other competency features ✅ respects layering
content_taggingREST API in openedx-platform (openedx/core/djangoapps/content_tagging/rest_api/v1/) - injects and handles thetaxonomy_typefield (not persisted in the database), which gets routed to the competency python API as needed.⚠️ breaks layering but this is a fairly minimal "wrapper"/"router" type code anyways so it seems like a logical place to do this- Studio MFE frontend: combined handling of taxonomies & competencies as per UX mockups.
⚠️ breaks layering but the product folks think it's better UX than separating them into two separate UIs.
At least in that case there are clean boundaries drawn in several places that completely respect the layering.
There was a problem hiding this comment.
I had Claude create the following comparison table and diagrams to help me think through our options more clearly, with your most recent proposal added as option 4.
I think Option 1 (currently in this ADR) and your option 4 (the router in openedx-platform) are the two strongest contenders at this point. I'm not opposed to going with option 4 if you feel it fits Open edX's existing patterns better and is worth it to avoid an explicit COMPETENCY value in the tagging app's enum.
I think there are two bigger-picture questions underneath this:
-
If someone runs Open edX without using any CBE features (as many people probably will), is this okay? Jenna and I have already been marketing this CBE initiative as "built into the core, not an add-on," so I think it's completely fair to accept a small trade-off where every Open edX instance carries a tiny bit of CBE awareness even if that instance never uses the feature. On this question, I'd put options 1 and 4 on relatively equal ground, both are small, harmless trade-offs for non-CBE users, though option 1's is arguably a bit lighter since it's inert stored data, while option 4 runs a small check on every request.
-
If someone wants to add another taxonomy type, is this okay? This situation might actually be pretty unlikely, especially given our recent conversation about getting rid of system-level taxonomies, but given how pluggable Open edX is meant to be, I still think it's a fair question to want a good answer for. Here the two options aren't equal: option 1 is a one-line addition to the model, a new enum value, and the serializer never changes. Option 4 is a new branch added directly into openedx-platform's shared serializer, plus a matching
select_relatedaddition to that view's queryset.
Given how close options 1 and 4 are, I think I'd like to bring this to Tuesday's architecture meeting rather than trying to settle it here in the thread. Let me know if that works for you, or if you'd rather wait for Dave, or if one option is starting to stand out to you as more of a clear winner. Also let me know if you have different thinking around the comparison table and what's been proposed than what is outlined below.
Here's the comparison, for reference:
Comparison
| # | Proposal | DB schema | Layering | Pros | Cons |
|---|---|---|---|---|---|
| 1 | ADR: taxonomy_type enum field |
Taxonomy gets new column |
No import; enum value name is a soft reference | - Filterable/queryable with a plain WHERE- Serializer just returns stored data, no cross-app knowledge - Already drafted, one migration - Extensible: a future third taxonomy type is just a new enum value; the read path never changes again |
- Generic app's enum now names a specific downstream concept - Needs to be set correctly at creation time |
| 2 | hasattr check in serializer |
Unchanged | Yes, flagged as a layering violation in review | - No migration - Always accurate, can't drift out of sync |
- Generic app's serializer names a sibling app's model relation directly - Needs a join/exists-subquery to filter by type, no plain indexed WHERE- N+1 risk: one extra query per row when listing taxonomies unless select_related('competencytaxonomy') is added, and since the check is on the shared base serializer, oel_tagging's own REST API (used by consumers other than openedx-platform) inherits the risk with no equivalent fix- Not extensible: a future third taxonomy type needs another hardcoded branch added directly to oel_tagging's shared serializer- Already deprioritized once on #618 (1+3 was favored then); revived without stated reasoning |
| 3a | Two-call frontend create flow | Unchanged | N/A, this is a call-shape question | - No new REST surface | - Frontend must orchestrate two calls - Partial-failure handling (taxonomy created, competency conversion fails) - Already rejected for UX reasons |
| 3b | New combined competency REST API | Unchanged | N/A | - Atomic from the frontend's point of view | - New endpoint to design, build, maintain - Frontend still needs both the plain "create taxonomy" call and this combined call, and has to pick between them since nothing tells it upfront whether a Tags or Competency taxonomy is being created - That branching arguably relocates the type-awareness into the frontend rather than removing it, though it wouldn't formally be called a layering violation there |
| 4 | Router: check + create-flow wiring moved into platform's REST view | Unchanged | Claimed to be none; violation (if any) contained to the integration layer, but it's relocated, not removed | - oel_tagging and CBE app both stay fully pure- One existing endpoint, no new REST surface - Matches this repo's real rule: openedx-core must never depend on openedx-platform, not the reverse - Verified against the actual code: TaxonomyOrgView already fully overrides get_queryset() and TaxonomyOrgSerializer already extends the base fields, so both the check and its select_related fix stay entirely in openedx-platform, zero footprint on oel_tagging or its other consumers- A deployment that never installs the CBE app is completely unaffected, since oel_tagging itself never mentions competencies |
- Same query-filtering cost as option 2, no schema change - Same N+1 risk, but confined to openedx-platform's own endpoint, and fixable there without touching oel_tagging- Every openedx-platform deployment carries this hardcoded check permanently in its shared content_tagging layer, whether or not that operator ever enables CBE, harmless at runtime, but not absent- Not extensible: same as option 2, a future third taxonomy type needs another hardcoded branch, just added to openedx-platform's shared serializer instead of oel_tagging's |
Legend: 🟩 pure/unchanged · 🟧 competency-specific · 🟦 integration layer · 🟥 violation/hardcoding · 🟨 schema change
Option 1 — persisted enum field
flowchart TD
FE[Studio MFE]
subgraph PLAT[openedx-platform]
P1[REST API]
end
subgraph CORE[openedx-core: oel_tagging]
C1[Serializer]
end
subgraph CBEAPP[openedx-core: CBE app]
B1[CBE API]
end
subgraph DB[Database]
T[("Taxonomy<br/>+ taxonomy_type")]
CT[(CompetencyTaxonomy)]
end
FE --> P1 --> C1
C1 -->|reads column| T
B1 -->|writes on create| CT
Only option where the Taxonomy table's schema changes. Serializer just reads a plain column, no cross-app lookup at read time.
Option 2 — hasattr in oel_tagging's serializer
flowchart TD
FE[Studio MFE]
subgraph PLAT[openedx-platform]
P1[REST API]
end
subgraph CORE[openedx-core: oel_tagging]
C1["Serializer<br/>hasattr check"]
end
subgraph CBEAPP[openedx-core: CBE app]
B1[CBE API]
end
subgraph DB[Database]
T[(Taxonomy<br/>unchanged)]
CT[(CompetencyTaxonomy)]
end
FE --> P1 --> C1
C1 -->|reads| T
C1 -.->|"exists? (per request)"| CT
B1 --> CT
Taxonomy table is untouched. The generic serializer instead reaches across to check for CompetencyTaxonomy's existence on every read, that cross-app relation name is the violation.
Option 3a — two-call frontend create flow
flowchart TD
FE[Studio MFE]
subgraph PLAT[openedx-platform]
P1[REST API]
end
subgraph CORE[openedx-core: oel_tagging]
C1[create_taxonomy]
end
subgraph CBEAPP[openedx-core: CBE app]
B0[existing conversion call]
end
subgraph DB[Database]
T[(Taxonomy<br/>unchanged)]
CT[(CompetencyTaxonomy)]
end
FE -->|"call 1: create taxonomy"| P1 --> C1 --> T
FE -->|"call 2: convert to competency"| B0 --> CT
Schema is unchanged. The frontend makes two sequential calls and has to handle the case where the first succeeds but the second fails.
Option 3b — new combined competency REST API
flowchart TD
FE["Studio MFE<br/>picks a call based on taxonomy type"]
subgraph PLAT[openedx-platform]
P1[REST API: create taxonomy]
end
subgraph CORE[openedx-core: oel_tagging]
C1[create_taxonomy]
end
subgraph CBEAPP[openedx-core: CBE app]
B0N[new combined endpoint]
end
subgraph DB[Database]
T[(Taxonomy<br/>unchanged)]
CT[(CompetencyTaxonomy)]
end
FE -->|"Tags: plain call"| P1 --> C1 --> T
FE -->|"Competency: combined call"| B0N
B0N --> T
B0N --> CT
Schema is unchanged. One new endpoint handles both tables in one request, but the frontend still needs both this call and the plain create-taxonomy call, and has to choose between them since nothing upstream tells it whether a Tags or Competency taxonomy is being created. That's the type-awareness moving into the frontend rather than disappearing.
Option 4 — router in openedx-platform's REST view
flowchart TD
FE[Studio MFE]
subgraph PLAT["openedx-platform (router lives here)"]
P1["Serializer: adds taxonomy_type<br/>via hasattr, on the same instance"]
end
subgraph CORE[openedx-core: oel_tagging]
C1[Base Taxonomy query/serializer, stays pure]
end
subgraph DB[Database]
T[(Taxonomy<br/>unchanged)]
CT[(CompetencyTaxonomy)]
end
FE --> P1
P1 -->|delegates for the base object| C1
C1 --> T
P1 -.->|"hasattr check on that<br/>same object, no new API call"| CT
oel_tagging stays untouched and fetches plain Taxonomy rows as it always has, same schema as options 2/3. openedx-platform's serializer runs one extra check on that same already-fetched instance, no second endpoint or separate CBE call needed for GET, since the two tables are already joinable at the database/ORM level. Query-filtering cost is identical to option 2. The already-decided #614 create-flow wiring is left out of this diagram since it applies the same way regardless of which GET option is chosen.
The dotted line from openedx-platform's serializer to CompetencyTaxonomy isn't a network call, a Python import, or a call into any CBE API. It's Django exposing an existing database join: because CompetencyTaxonomy is defined as a subclass of Taxonomy, the two tables are already linked at the schema level, and any code in the same process, including openedx-platform's serializer, can ask "does a matching row exist in the other table?" without ever referencing CBE's code.
There was a problem hiding this comment.
Thanks, this is super helpful! Let's discuss on Tuesday.
If someone runs Open edX without using any CBE features (as many people probably will), is this okay?
I think we both agree that any of these approaches are all OK in this case, as the CBE feature has a pretty small footprint on the overall tagging/taxonomy code and UI.
If someone wants to add another taxonomy type, is this okay?
I think this is a really important question and it's what I'm most concerned about. If we set a precedent that taxonomies are a foundational layer and other things can be built on top of them without modifying the taxonomy code or UI, I think there's no problem at all. People can build whatever they imagine on top of tagging and taxonomies. But in the case of something like CBE, I would prefer to see such features built as separate apps, with their own UI, and not "built in" to the tagging/taxonomy code.
We're instead setting the opposite precedent: integrating a new feature with taxonomies involves adding some code directly to the taxonomies backend and integrating custom UI components into the tagging frontend as well.
So we're neither building a generic plugin API nor maintaining a clean abstraction/layering, but instead introducing a bit of coupling of competency + taxonomy app in a few places. (Exactly which places depends on which approach from the options above, but it seems like product folks want the UI integrated in any case.)
So: I think this is reasonable if competencies is the only special "type" of taxonomy that we or anyone else will have in the platform for the foreseeable future. But if we expect other use cases, then we should set a cleaner architectural precedent that builds a proper plugin API for integration (e.g. allows you to inject components into the tagging/taxonomy UI, and register new taxonomy types dynamically) or just builds on top of taxonomies without modifying them at all (e.g. fully separate frontend, and REST APIs, layering respected).
ADR 0013 no longer decides to add a taxonomy_type field to Taxonomy: an architecture-meeting review moved that decision to detecting competency taxonomies entirely within openedx-platform instead. The lifecycle-rule note and Rejected Alternatives cross-reference this amendment added to describe that field's creation-time invariant no longer describe anything that happens, so revert ADR 0002 to its state from before the amendment.
The next commit changes this ADR's decision away from a taxonomy_type field, so the filename no longer describes it. Renamed on its own, ahead of that content change, so the rename and the content diff stay independently reviewable.
Reversed at the 2026-07-21 architecture meeting: instead of a taxonomy_type field on the base Taxonomy model, report a taxonomy's type entirely within openedx-platform (a hasattr check on the same already-fetched instance, plus a matching select_related), so openedx_tagging and the CBE app stay fully free of any competency-specific reference. The field-based approach is demoted to Rejected Alternatives, alongside two more precise variants (two-call create flow, combined REST API) that were previously folded into a single vaguer entry. Adds a diagram of the chosen approach.
Replace references to openedx-platform's specific serializer/view class and method names, and the literal hasattr()/select_related() calls, with a description of the mechanism itself. Those identifiers belong to a repo this ADR doesn't govern, so naming them risked the ADR reading as stale if openedx-platform's internal structure changes, even though the decision itself wouldn't have.
Drop the TaxonomySerializer parenthetical (an openedx_tagging class name that isn't needed to state the requirement), the redundant CompetencyTaxonomy(Taxonomy) Python-syntax notation (already said in prose just before it), and the parenthetical GitHub issue references, which read oddly in an ADR meant to stand on its own.
|
We discussed this at today's architecture meeting and decided to go with option 4: keep the type-detection check, and its I've updated ADR 0013 to reflect that: status is now Accepted, the Decision section describes the openedx-platform-side approach with a diagram, and the original field-based approach is now the first Rejected Alternative with the reasoning above. I also split the "frontend-only" alternative into the two more precise variants we actually compared in the table above (the two-call create flow and the combined REST API), and reverted the ADR 0002 amendment since it no longer describes anything that happens now that there's no new field. The changes are spread across five commits so the diff stays easy to review: a revert of the ADR 0002 amendment, a pure rename of the ADR file with zero content diff, the actual decision rewrite, and two small follow-up commits trimming implementation detail (specific class/method names, and parenthetical issue references) out of the Context and Decision sections. @bradenmacdonald, could you take another look when you get a chance? Let me know if I've mischaracterized anything about option 4 above. |
mgwozdz-unicon
changed the title
4 participants
Summary
openedx_tagging's taxonomy Get endpoints need to report whether a taxonomy is a Competency Taxonomy (#618), symmetrically with the Create/Import endpoint'staxonomy_typefield (#614).This PR adds ADR 0013 (
openedx_tagging), deciding to report a taxonomy's type entirely within openedx-platform: its serializer and view, which already override the base ones, check whether a relatedCompetencyTaxonomyrow exists for a givenTaxonomyand fix the resulting query cost, with no new field, method, or enum value added toopenedx_taggingor the CBE app.An earlier draft of this ADR instead proposed a
taxonomy_typefield directly onTaxonomy. Following architecture-team review, that approach and four others considered along the way are documented as Rejected Alternatives in the ADR itself.Context
taxonomy_typetaxonomy_typefield (this ADR's read-side value mirrors it)get_type()method)openedx_learning) —CompetencyTaxonomymulti-table-inheritance model; the DB relation this decision relies onopenedx_tagging) — prior art foropenedx_tagging-owned ADRsdriven by CBE needs