[Follow-up BE] Scope the criteria-groups query by course instead of loading every group

Abierto
  • ¿Algo no está bien?
#919 2 comentarios 0 reacciones 1 asignado Ver en GitHub

Los mantenedores suelen responder en 1 día

@ufedaseyeuconsultant ya está trabajando en esto.

Desde el 30/10/2026.

Evaluación

Este issue todavía no se ha evaluado.

Descripción

Context

get_competency_criteria_tree() (#681, with course-run scoping from #682) answers a read scoped to some course runs. To do that it loads every non-archived CompetencyCriteriaGroup of the competency and walks the tree from the root, so it can learn which course each group belongs to. The read then returns only the groups above any course plus the groups of the requested runs, and it fetches criteria only for those groups. The number of group rows grows with the number of criteria, because each criterion gets its own leaf group. So a read scoped to one course run still loads the group rows of every course run the competency has been used in.

The groups query could instead filter with a predicate such as course__isnull=True OR course_id IN (...), but only if every group below a course-level group recorded that course. Today none of them do.

What ADR 0002 says today

ADR 0002, Decision 2, column 4, as amended for #872 on 2026-10-22:

course_id: Nullable foreign key for the course that scopes a course-scoped subtree. Only the course-level group carries it; the groups below it leave course_id null and belong to the course through that ancestor. Immutable after creation. Root groups always have course_id null.

The same amendment's changelog entry says it supersedes the 2026-09-04 entry, which had said that every group in a course-scoped subtree carries course_id. The code matches the current wording: _create_leaf_group() creates each leaf with course null, and only the course-level group has a course.

Decision 2 also states the retrieval scope a read must satisfy: "A read always includes the root group, plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated." It also expects each course-scoped subtree to stay small, realistically 500 nodes or fewer.

Why a leaf cannot simply carry course_id

ADR 0002 Decision 5, index 10, is the unique constraint oel_cbe_criteria_group_one_course_group_per_tag_course on (oel_tagging_tag_id, course_id, parent_id). It allows at most one course-level group per competency and course. The constraint has no condition, because MySQL has no partial indexes. Every leaf of a course hangs off the same course-level group. If leaves carried course_id = X, a second leaf would have the same (tag, X, parent) as the first and would violate the constraint. So putting the course on every group through course_id also means replacing that constraint. The repo's pattern for a uniqueness rule that needs a condition is an application-computed column that is set only on the rows the rule covers and is unique, as scope_code already is on CompetencyCriteriaGroup for roots and on CompetencyRuleProfile.

Options

Option What changes Cost
A. Keep the current model Nothing. A scoped read keeps loading every group of the competency and walking the tree in Python. No migration and no ADR change. The extra rows are the groups of other course runs. Their number is bounded by the competency's total number of criteria.
B. Put course_id on every group in a course-scoped subtree Replace the index 10 constraint with a computed unique column set only on course-level groups. Set course on every group created below a course-level group. Backfill existing groups. Reverse the 2026-10-22 correction in ADR 0002. A migration with a backfill, an ADR amendment, and updates to the code that treats a null course as "leaf": the leaf check in resolve_supplied_leaf_group(), get_courses_with_relative_criteria() (which joins through parent__course), and the course lookup in update_competency_criteria_group() (#760).
C. Add a separate, non-unique column for the subtree's course Add a new nullable, indexed foreign key, for example subtree_course_id, set on the course-level group and on every group below it. course_id and its constraint stay as the ADR describes them. Backfill existing groups. A migration with a backfill and an ADR amendment that adds a column. course_id keeps its meaning, so the call sites listed under B do not change. The cost is one more column to keep in step with course_id on create.

With B or C, the groups query in get_competency_criteria_tree() becomes a predicate on the course column, and a test pins that a read scoped to one course run loads only the groups above any course and that run's groups.

Decision needed

  1. Choose A, B or C.
  2. If B or C is chosen, schedule the work. @mgwozdz-unicon owns that prioritization.

Acceptance criteria (for B or C)

  • Every group in a course-scoped subtree records the course-level group's course, including groups created before the migration.
  • Two leaves under the same course-level group can be created, and a second course-level group for the same competency and course is still rejected by the database.
  • A read scoped to one course run loads only the groups above any course and that run's groups, which a query-count or row-count test pins.
  • The existing criteria-tree, create, bulk-update and group-update tests pass with unchanged behavior.
  • ADR 0002 is amended to describe the chosen column.
Lenguaje dominante
Python
Estrellas
10
Forks
33
Merge medio
3 d 4 h
PR fusionados (30 d)
33

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de openedx/openedx-core

Todos los issues de openedx/openedx-core

Issues similares

Más issues de Python

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.