feat: enforce competency-hierarchy dominance for criteria by alezconsultant · Pull Request #875 · openedx/openedx-core

alezconsultant · 2026-10-16T17:52:57Z

Description

Implements #666: before a CompetencyCriterion is created, reject it with
a 400 if any taxonomy ancestor or descendant of its competency tag already has a live
CompetencyCriterion in the same course. This is the containment check
create_competency_criterion() reserved a seam for in #665. Siblings, and a repeat criterion
for the same tag in the same course, stay allowed, per the ticket's acceptance criteria.

Archived rows do not count. An archived criterion, or a criterion under an archived leaf,
course-level, or root group, no longer blocks a related tag from getting a criterion in the same
course, following ADR-0002 and ADR-0003 Decision 3, which treat an archived association as gone
for authoring. That also keeps this check consistent with #717 scoping the duplicate check to
archived=False.

Changes

src/openedx_tagging/api.py: adds get_ancestor_tags(tag) and get_descendant_tags(tag),
both -> QuerySet[Tag], matching the ticket's approved technical notes (name, file, and
signature). Mirrors Tag.descendant_count's lineage-prefix mechanism and its
free-text-taxonomy guard.
src/openedx_learning/applets/cbe/api.py:
- adds get_courses_with_relative_criteria(tag_ids, course=None) (public, listed in
  __all__). It maps each course already blocked by one of the given tags to the lowest-id
  conflicting tag, and adds parent__course=course to the filter when a course is given, so
  a create only loads the criteria in its own course. Leaving course off returns the full
  map, which the blocked-courses endpoint ([BE] Get Competency Dominance Courses #796) uses. The filter also requires
  parent__course__isnull=False, so a criterion misplaced on a non-leaf group is skipped
  rather than failing every create for that tag.
- adds _validate_containment(group, course) (private) and calls it from
  create_competency_criterion() before the object is tagged.
- create_competency_criterion() now takes course_run from
  associate_competency_criterion() instead of re-parsing object_id and fetching the course
  run a second time. Its rule fields are keyword-only, and it runs inside the caller's
  transaction and locks rather than opening its own.
- associate_competency_criterion() takes a per-course lock right after the existing Tag
  lock (always tag first, then course run), so two concurrent creates in the same course
  cannot both pass the containment check.
src/openedx_learning/applets/cbe/models/course_run.py and migration
0016_add_competency_course_run: adds CompetencyCourseRun, a CBE-owned row whose primary
key is a OneToOneField(CourseRun, on_delete=CASCADE). It is created with get_or_create() on
a course's first criterion and locked with select_for_update() on every create. Locking this
row instead of the catalog's CourseRun keeps the lock from blocking other apps' inserts that
reference the course run (InnoDB takes a shared lock on the parent row for every foreign-key
check), and keeps openedx_learning from querying openedx_catalog tables directly, per
catalog ADR-0001 Decision 4. The model has no other fields yet; it is the per-course CBE row
that [FE] Let the operator choose to replace Course Grade Slot with CBE Progress #839 can extend. Annotated .. no_pii:.
Tests:
- tests/openedx_tagging/test_api.py: unit tests for the two new tagging functions, including
  the root, leaf, and free-text-taxonomy edges.
- tests/openedx_learning/applets/cbe/test_api.py: get_courses_with_relative_criteria() in
  isolation (single match, lowest-id tie-break, empty input, narrowing to the given course, a
  criterion outside a leaf, and one test per archived case: criterion, leaf group,
  course-level group, root group, and a course with one live and one archived criterion);
  _validate_containment() for the ancestor, descendant, sibling, other-course, and
  no-relatives cases; create_competency_criterion() using the caller's course_run;
  associate_competency_criterion() creating and then reusing the CompetencyCourseRun row;
  a test that fails if anything calls CourseRun.objects.select_for_update() during a create;
  and cascade deletion of CompetencyCourseRun with its course run.
- tests/openedx_learning/applets/cbe/test_views.py: one endpoint test per
  acceptance-criteria scenario in the ticket (all six), plus one for an archived ancestor
  criterion no longer blocking its descendant. Each uses its own uniquely-prefixed tag family so
  scenarios cannot interfere with each other. The reject-scenario tests assert the response body
  names the conflicting tag and course, not just that an error key is present.

Manual Testing

Run against a Tutor devstack (MySQL 8.4, READ COMMITTED, the isolation level Django uses for
MySQL), with this checkout mounted into the CMS container and migration
0016_add_competency_course_run applied.

Dominance scenarios, through associate_competency_criterion() in a manage.py cms shell, each
with its own tag family under one competency taxonomy:

A criterion on a parent tag in course X, then a criterion on its child tag in course X:
ValidationError keyed tag_id, naming the parent tag and course X.
The reverse order, child first, then parent, in the same course: rejected, naming the child.
A three-level chain, criterion on the root, then on the grandchild: rejected, naming the root.
Two sibling tags in the same course: both created.
A parent tag in course X and its child in course Y: both created.
The same tag twice in the same course, on two different blocks: both created.
Scenario 1 again, with the parent-tag criterion archived first: the child's criterion is
created.

Lock behavior, with two real MySQL connections:

While one create holds the course lock, a conflicting create in the same course waits, then
gets the dominance ValidationError once the first commits.
While the CompetencyCourseRun lock is held, another transaction inserting a row with a
foreign key to the same CourseRun finishes immediately. With a CourseRun lock held in its
place, that same insert hits Lock wait timeout exceeded.
Two simultaneous "first criterion in this course" creates, repeated 15 times: no deadlocks
and no IntegrityErrors.

Related to #666

🤖 Generated with Claude Code

openedx-webhooks · 2026-10-16T17:53:04Z

Thanks for the pull request, @alezconsultant!

This repository is currently maintained by @axim-engineering.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

tbain

This PR was approved with a Unicon code review performed against this original PR: alezconsultant#2

Light follow-up human code review of this PR and also a Codex evaluation as a fresh code review substantiate that there are no substantive changes after the prior review from changing the merge target, and no new issues or concerns were introduced as part of that process.

ormsbee

Three requests before merging, all inline in api.py:

Please pass course_run from associate_competency_criterion() into create_competency_criterion(), rather than re-parsing object_id and re-fetching the course run inside a savepoint.
Please let _validate_containment() narrow get_courses_with_relative_criteria() to the one course it's checking, so a create under a widely used competency doesn't pull every criterion in that subtree across every course.
Please move the lock off the CourseRun row and onto a 1:1 row that CBE owns, since an exclusive lock on the catalog row blocks every foreign-key insert that points at the course run, and openedx_learning shouldn't be querying catalog tables directly.

There's also a question about whether archived criteria should block (right now they do), which I think belongs in #717 if it isn't addressed here, and a nit.

Thank you.

Claude drafted the first pass of this review and ran the CBE and tagging suites (293 tests) on this branch, plus a probe confirming that an archived criterion with its whole group chain archived still blocks, and counting queries on a derive-path create (39 on main, 44 here for a tag with no relatives).

ormsbee · 2026-10-16T19:37:47Z

+    Maps each conflicting CourseRun to the lowest-id tag (of ``tag_ids``) with a criterion there.
+    """
+    groups: QuerySet[CompetencyCriteriaGroup, CompetencyCriteriaGroup] = (
+        CompetencyCriteriaGroup.objects.filter(tag_id__in=tag_ids, criteria__isnull=False)


This returns one row per criterion for every relative tag in every course (Claude's probe: three child tags with criteria in five other courses is 15 rows, each joined to Tag and CourseRun), and _validate_containment() then looks up exactly one of those courses. For a competency near the top of an institution-wide taxonomy, that's every criterion under the subtree across every course run, on every create. Could this take an optional course and add parent__course=course to the filter when it's given? #796 still gets the whole map by leaving it off, and both callers keep sharing the one query, which is what the ticket wanted. The ticket's caution against pre-filtering on course was written when the course was going to be parsed out of object_id; now that it comes from parent__course, which _create_leaf_group() always sets on the course-level group, the filter is exact rather than an approximation.

ormsbee · 2026-10-16T19:37:47Z

+    result: dict[CourseRun, Tag] = {}
+    for group in groups:
+        # criteria__isnull=False guarantees every row here is a leaf with a course-level parent.
+        assert group.parent is not None


Nit: nothing at the database level keeps a criterion off a root or course-level group (that check lives in resolve_supplied_leaf_group()), so one row that got there through the admin or a script turns into an AssertionError for every create touching this tag, with nothing in the message to find it by. Adding parent__course__isnull=False to the filter would make the comment above true by construction; if the asserts stay for mypy, please put group.id in their messages.

ormsbee · 2026-10-16T19:37:47Z

+        # tag.taxonomy_id is nullable at the model level; already guaranteed set by the caller.
+        assert tag.taxonomy_id is not None
+        try:
+            object_course = get_course_run(UsageKey.from_string(object_id).course_key)


Please have associate_competency_criterion() pass course_run in, rather than parsing object_id and calling get_course_run() a second time: the caller did both right before calling this, and #666 asked for the already-parsed key specifically so this wouldn't happen. With course_run as a parameter, the two asserts and the course-mismatch check below can go as well (for a supplied group resolve_supplied_leaf_group() already checks it, and for a derived one it's true by construction), and the lock below can key off course_run directly. Claude counted 39 queries for a derive-path create on main and 44 here for a tag with no relatives; the savepoint pair and the second get_course_run() are three of the five.

ormsbee · 2026-10-16T19:37:47Z

+        except CourseRun.DoesNotExist as exc:
+            raise ValidationError({"object_id": _("No course run matches object_id's course.")}) from exc
+        # Without this lock, two concurrent creates could both pass the containment check below.
+        CourseRun.objects.select_for_update().get(pk=object_course.id)


I'm concerned about this lock on the CourseRun row for two reasons:

InnoDB takes a shared lock on the parent row for every foreign key check and holds it until commit (locks set by statements), so an exclusive lock here blocks every insert that points at this course run, from any app, for the length of this transaction, and this create waits behind any uncommitted transaction that inserted a child row for the course. Today that's only the two CBE tables, but catalog ADR-0001 Decision 3 says every course-referencing model here and in openedx-platform should be a foreign key to CourseRun, so that set is going to grow.

openedx_learning shouldn't query openedx_catalog's tables directly: nothing outside the catalog app does today, and this module goes through get_course_run() everywhere else.

I'm still fine with a lock, since this invariant spans rows and no constraint can express it. Please put it on a row that CBE owns: a model with a OneToOneField(CourseRun, primary_key=True, on_delete=CASCADE), created with get_or_create() when a course gets its first criterion and locked with select_for_update() on that table, so the only thing it blocks is another criterion create in the same course. That's the pattern ADR-0001 Decision 4 lays out for per-course data in other apps. It can be the per-course CBE row that #839 is going to need anyway. Please also take it right after the Tag lock in associate_competency_criterion(), with a comment that the order is tag first, then course, so nobody later takes them the other way around.

FYI to @kdmccormick, @bradenmacdonald, @Agrendalath, since we might want to formalize this as a general practice for the CourseRun model.

alezconsultant · 2026-10-20T13:34:56Z

@ormsbee

Branch updated and rebased.

I pass cource_run into create_competency_criterion now.
get_courses_with_relative_criteria(tag_ids, course=None) now adds parent__course=course when a course is given, and _validate_containment() passes it, so a create only loads the criteria in its own course.
Add model CompetencyCourseRun in openedx-core-666/src/openedx_learning/applets/cbe/models/course_run.py with OneToOneField to avoid locking parent rows.
The filter now includes parent__course__isnull=False, so a criterion on anything other than a leaf is skipped.
Archived rows no longer count: get_courses_with_relative_criteria() skips an archived criterion and any criterion under an archived leaf, course-level, or root group.

ormsbee

All three requests are addressed, and I'm glad you went ahead and ignored archived criteria and groups here, with tests, rather than leaving it to #717. I have a tiny request to fix a comment.

Please squash to one commit, with a body that describes what's here now (the current feat commit body still says it locks the CourseRun row and rejects a course mismatch), and bring the PR description up to date (the branched-from and manual-testing paragraphs are stale, and the Changes section doesn't mention CompetencyCourseRun or the archived filtering). Then I'll merge.

Thank you!

Claude drafted the first pass of this review and ran the CBE and tagging suites (343 tests) and makemigrations --check on this branch, confirmed that the rebased feat commit differs from the one I reviewed only in an import line, and re-counted queries on a derive-path create (42 for a second create in a course, against 44 last round and 39 on main).

ormsbee · 2026-10-20T16:11:12Z

        # Serializes the duplicate check and root creation; MySQL has no partial unique index for one root per tag.
        Tag.objects.select_for_update().get(pk=tag.pk)
+        # Serializes the per-course dominance check on CBE's own row, not the catalog's CourseRun.
+        # Created without FOR UPDATE first: locking a missing row lets two first creates deadlock on the gap.


This comment describes REPEATABLE READ. Under READ COMMITTED, which is what Django runs MySQL at, a locking read locks only index records, not the gaps before them, so FOR UPDATE on a missing row locks nothing at all, and the reason to create first is just that there has to be a row to lock. Maybe something like:

Suggested change

# Created without FOR UPDATE first: locking a missing row lets two first creates deadlock on the gap.

# Created first, since a locking read of a row that doesn't exist yet locks nothing; get_or_create()

# resolves two first creates racing on the primary key.

Reject a CompetencyCriterion at creation time if any taxonomy ancestor or descendant of its competency tag already has a live CompetencyCriterion in the same course. Siblings, and a repeat criterion for the same tag in the same course, stay allowed. Archived criteria, and criteria under an archived leaf, course-level, or root group, do not count, since ADR 0002 hides archived rows from authoring and new associations. Add to openedx_tagging/api.py: get_ancestor_tags(tag) and get_descendant_tags(tag), both returning a QuerySet[Tag], mirroring Tag.descendant_count's own lineage-prefix mechanism and its free-text-taxonomy guard. Add to applets/cbe/api.py: get_courses_with_relative_criteria(tag_ids, course=None), a public function mapping each course already blocked by one of the given tags to the lowest-id conflicting tag, narrowed to one course when course is given. It only counts criteria on a leaf group, whose parent carries the course, so a criterion misplaced on another group is skipped rather than failing every create for the tag. Add _validate_containment(group, course), called from create_competency_criterion() before the object is tagged. create_competency_criterion() takes the caller's course_run instead of re-parsing object_id and fetching the course run a second time, and its rule fields are keyword-only. It runs inside the transaction and locks that associate_competency_criterion() holds. Add CompetencyCourseRun, a CBE-owned row whose primary key is a OneToOneField to the catalog's CourseRun (migration 0016). associate_competency_criterion() creates it with get_or_create() on a course's first criterion and locks it with select_for_update() right after the Tag lock, so two concurrent creates in the same course cannot both pass the containment check. Locking this row instead of CourseRun keeps the lock from blocking other apps' inserts that reference the course run, and keeps openedx_learning from querying catalog tables directly. Related to openedx#666 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

alezconsultant · 2026-10-28T11:04:13Z

@ormsbee All done.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 16, 2026

openedx-webhooks added this to Contributions Oct 16, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Oct 16, 2026

tbain approved these changes Oct 19, 2026

View reviewed changes

ormsbee requested changes Oct 19, 2026

View reviewed changes

alezconsultant force-pushed the alezconsultant/666-competency-hierarchy-dominance branch from 88460a5 to 0d8f6a9 Compare October 20, 2026 13:15

alezconsultant requested a review from ormsbee October 20, 2026 13:35

alezconsultant mentioned this pull request Oct 20, 2026

feat: add blocked-courses endpoint for a competency alezconsultant/openedx-core#7

Closed

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Oct 22, 2026

alezconsultant force-pushed the alezconsultant/666-competency-hierarchy-dominance branch from 4957939 to d5091d0 Compare October 26, 2026 13:01

thelmick-unicon mentioned this pull request Oct 27, 2026

[Follow-up - BE] Block dominance conflicts on taxonomy re-import #902

Open

ormsbee requested changes Oct 28, 2026

View reviewed changes

alezconsultant force-pushed the alezconsultant/666-competency-hierarchy-dominance branch from d5091d0 to a8114ea Compare October 28, 2026 10:54

alezconsultant force-pushed the alezconsultant/666-competency-hierarchy-dominance branch from a8114ea to 7033c48 Compare October 28, 2026 10:58

alezconsultant requested a review from ormsbee October 28, 2026 11:04

ormsbee approved these changes Oct 28, 2026

View reviewed changes

ormsbee merged commit 9247ee4 into openedx:main Oct 28, 2026
7 checks passed

github-project-automation Bot moved this from In Eng Review to Done in Contributions Oct 28, 2026

ormsbee mentioned this pull request Oct 28, 2026

feat: enforce one root criteria group per competency in the database #900

Merged

jesperhodge mentioned this pull request Oct 28, 2026

fix: drop row locks from bulk criterion updates #914

Draft

alezconsultant mentioned this pull request Oct 28, 2026

Alezconsultant/796 blocked courses #915

Open

jesperhodge mentioned this pull request Oct 28, 2026

Remove row locks in the CBE applet that protect no invariant #916

Open

4 tasks

dvarenikqaconsultant mentioned this pull request Oct 30, 2026

[BE] Add Competency hierarchy dominance validation to Competency Criteria Creation #666

Closed

Navigation Menu

Repository navigation

feat: enforce competency-hierarchy dominance for criteria - #875

feat: enforce competency-hierarchy dominance for criteria#875
ormsbee merged 1 commit into
openedx:mainfrom
alezconsultant:alezconsultant/666-competency-hierarchy-dominance

alezconsultant commented Oct 16, 2026 •

edited

Loading

Uh oh!

openedx-webhooks commented Oct 16, 2026

Uh oh!

tbain left a comment

Uh oh!

ormsbee left a comment

Uh oh!

Uh oh!

ormsbee Oct 16, 2026

Uh oh!

ormsbee Oct 16, 2026

Uh oh!

ormsbee Oct 16, 2026

Uh oh!

ormsbee Oct 16, 2026

Uh oh!

alezconsultant commented Oct 20, 2026

Uh oh!

ormsbee left a comment

Uh oh!

ormsbee Oct 20, 2026

Uh oh!

alezconsultant commented Oct 28, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	# Created without FOR UPDATE first: locking a missing row lets two first creates deadlock on the gap.
	# Created first, since a locking read of a row that doesn't exist yet locks nothing; get_or_create()
	# resolves two first creates racing on the primary key.

Conversation

alezconsultant commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Manual Testing

Uh oh!

openedx-webhooks commented Oct 16, 2026

Uh oh!

tbain left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ormsbee Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

ormsbee Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

ormsbee Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

ormsbee Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

alezconsultant commented Oct 20, 2026

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

alezconsultant commented Oct 28, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alezconsultant commented Oct 16, 2026 •

edited

Loading