[tbain]

Description

Adds three public functions to openedx_tagging.api. Together they let another app mark a tag association as depended upon, and let the tagging app's delete paths ask whether a tag or a taxonomy has a locked association beneath it.

• lock_object_tags_for_deletion(object_tag_ids) sets ObjectTag.deletion_locked=True on one association or several. It reads the current flags first and raises ObjectTagDoesNotExist before writing if any id does not exist, then runs one UPDATE for the ids still unlocked. An already-locked association is read, not written, so the common case of a second learner graded on the same criterion takes no row lock. If a row is deleted between the read and the write, the setter detects the shortfall and raises for that id.
• get_tags_locked_for_deletion(tag_ids) returns the subset of the given tag ids that have at least one locked association directly on them. It runs one query no matter how many ids it receives.
• is_taxonomy_locked_for_deletion(taxonomy_id) returns whether any association in the taxonomy is locked, as a single existence check.

Both readers count archived associations. An association that was locked and later archived still anchors a learner's mastery history, so the tag and taxonomy above it must still read as in use.

openedx#782 is the first caller of the setter. openedx#779 and openedx#780 call the readers to decide between archiving and hard delete. As the ticket specifies, this PR adds no REST endpoint and no unlock function. The functions do no permission checks; the caller authorizes, as the api.py module docstring states.

Impacted roles: Developer only. Nothing calls these functions yet.

Choices that differ from the ticket

• The setter is named lock_object_tags_for_deletion, plural. The ticket calls it lock_object_tag_for_deletion. The function accepts one id or many, and @ormsbee's approval on [BE] Add deletion-lock functions to openedx_tagging openedx/openedx-core#777 left room to adjust the names if they read awkwardly. The two readers keep the ticket's names.
• The readers see archived rows through the default manager. The ticket expects [BE] Exclude archived tagging records from read paths and block edits to them openedx/openedx-core#778 to hide archived rows by default and to provide an opt-out. [BE] Exclude archived tagging records from read paths and block edits to them openedx/openedx-core#778 (feat: add logic to exclude archived/retired tags and tag objects from API read/write #1) works the other way round: ObjectTag.objects returns every row, and hiding archived rows is an opt-in .unarchived() call. The readers therefore use plain ObjectTag.objects. test_archived_locked_association_still_counts fails if .unarchived() is added to either reader, which is the guard the ticket asks for.
• 'is_taxonomy_locked_for_deletion()' looks for locked associations by the association’s own taxonomy column. Every ObjectTag records which taxonomy it belongs to directly, so the query does not need to look up the taxonomy through the tag. The direct column gives the same answer whenever a tag is present, because validation rejects an association whose tag comes from a different taxonomy. It also finds associations that have no tag at all, such as free-text values, and it is already covered by a database index.
• Locking an archived association is allowed. [BE] Exclude archived tagging records from read paths and block edits to them openedx/openedx-core#778 refuses edits to archived records, but a lock only protects data and never makes a row visible again. An association that [BE] Branch tag association deletes between archiving and hard delete openedx/openedx-core#779 archives is already locked, so locking it again is a no-op instead of an error that [BE] Lock tagging records against deletion when a learner competency status is written openedx/openedx-core#782 would have to handle.

Supporting information

• Closes [BE] Add deletion-lock functions to openedx_tagging openedx/openedx-core#777
• Design: the push-based lock approach approved on [Arch] Implementation approach for competency data delete/edit guardrails openedx/openedx-core#655
• This branch is stacked on the [BE] Exclude archived tagging records from read paths and block edits to them openedx/openedx-core#778 branch (feat: add logic to exclude archived/retired tags and tag objects from API read/write #1), which is itself stacked on [BE] Add an archived field to Tag, Taxonomy, and ObjectTag, and a deletion_locked field to ObjectTag openedx/openedx-core#776 (feat: Adding archived and deletion-locked flags to tagging models openedx/openedx-core#856). The change under review is the diff against the [BE] Exclude archived tagging records from read paths and block edits to them openedx/openedx-core#778 branch.
• The setter is consumed by [BE] Lock tagging records against deletion when a learner competency status is written openedx/openedx-core#782, and the readers by [BE] Branch tag association deletes between archiving and hard delete openedx/openedx-core#779 and [BE] Branch tag and taxonomy deletes between archiving and hard delete openedx/openedx-core#780.

Testing instructions

1. Run the new tests, then the rest of the tagging suite to confirm nothing else changed:
   • pytest --no-cov tests/openedx_tagging/test_api.py::TestDeletionLock (17 tests)
   • pytest --no-cov tests/openedx_tagging/
2. Check the behavior by hand in a Django shell (python manage.py shell). These steps create rows in your dev database.

   from openedx_tagging import api
   from openedx_tagging.models import ObjectTag

   taxonomy = api.create_taxonomy("Lock demo")
   locked_tag = api.add_tag_to_taxonomy(taxonomy, "Locked tag")
   other_tag = api.add_tag_to_taxonomy(taxonomy, "Unlocked tag")
   api.tag_object("demo-object-1", taxonomy, ["Locked tag"])
   api.tag_object("demo-object-2", taxonomy, ["Unlocked tag"])
   object_tag = ObjectTag.objects.get(object_id="demo-object-1", taxonomy=taxonomy)

   api.is_taxonomy_locked_for_deletion(taxonomy.id)                      # False
   api.lock_object_tags_for_deletion(object_tag.id)
   api.get_tags_locked_for_deletion([locked_tag.id, other_tag.id])       # {locked_tag.id}
   api.is_taxonomy_locked_for_deletion(taxonomy.id)                      # True
   api.lock_object_tags_for_deletion([object_tag.id, 999999])            # raises ObjectTag.DoesNotExist: [999999]

   ObjectTag.objects.filter(pk=object_tag.pk).update(archived=True)
   api.get_tags_locked_for_deletion([locked_tag.id])                     # still {locked_tag.id}
   api.is_taxonomy_locked_for_deletion(taxonomy.id)                      # still True

3. Confirm the layering: lint-imports keeps both contracts.

Other information

• There is no model change and no migration. The deletion_locked field comes from [BE] Add an archived field to Tag, Taxonomy, and ObjectTag, and a deletion_locked field to ObjectTag openedx/openedx-core#776.
• pylint, mypy, pycodestyle, pydocstyle, isort, and lint-imports are clean on the changed files.
• AI usage: this PR was developed with Claude (Anthropic) in Cowork, with Sonnet sub-agents doing the implementation and an independent review pass. I reviewed the change and ran the tests and checks locally.

---

🤖 Generated with Claude Code

[jesperhodge]

@tbain I got stuck on the third point of "Choices that differ from the ticket", which I didn't understand ("The taxonomy check matches..." . I had Fable explain it to me and rewrite it:

"
is_taxonomy_locked_for_deletion() looks for locked associations by the association's own taxonomy column. Every ObjectTag records which taxonomy it belongs to directly, so the query does not need to look up the taxonomy through the tag. The direct column gives the same answer whenever a tag is present, because validation rejects an association whose tag comes from a different taxonomy. It also finds associations that have no tag at all, such as free-text values, and it is already covered by a database index.
"

[jesperhodge]

Hey great work! Only some minor details that AI pointed out to me.

AI Code Review

1. The setter takes a database row lock even when the row is already deletion-locked. openedx#782 will call it in every grade transaction, and every learner graded on the same criterion shares one association row, so their grades would queue behind each other on that row lock. The approved design (openedx#655) says to read first and update only rows not yet deletion-locked, and ADR 0004 allows no such lock.

Fix steps and sources

lock_object_tags_for_deletion runs filter(pk__in=ids).update(deletion_locked=True) even when the row is already locked.

Fix, inside the setter:

1. Read (pk, deletion_locked) for the given ids with a plain, non-locking query.
2. Raise DoesNotExist for any missing id before writing.
3. Only if some ids are still unlocked, run filter(pk__in=still_unlocked, deletion_locked=False).update(deletion_locked=True), keeping atomic() around that write.
4. Edit openedx#782's sentence "Do not add a read to avoid a write here", which contradicts the design.

If you and the ticket authors instead accept the contention, record it in the PR and the ticket, because it reverses the approved design.

Sources:

• openedx#782 says the lock is set "in the same transaction as the learner status write", which by ADR 0004 Decision 1 is the grade transaction.
• mgwozdz-unicon's openedx#655 proposal of 2026-08-27, which ormsbee approved, reads: "The lock write reads deletion_locked first and only writes when it's still False. ObjectTag is one row shared by every learner graded against that criterion, so an unconditional write on every grade would take a write lock on that row inside every single grade transaction, serializing every other learner's grade behind whichever one currently holds it, even though only the very first write ever actually changes anything." The same author restated it on 2026-08-28.
• ADR 0004 Decision 6 says no path other than a staff correction takes a lock.
• Which text wins is a decision for the ticket authors. The ADR is accepted and the openedx#655 text is the approved design, so the review treats openedx#782's sentence as the one that drifted.

Inferred: InnoDB, MySQL's default storage engine, should lock every row an UPDATE matches until the outer transaction commits, even when the value does not change. The review did not run it on MySQL, and the SQLite test suite cannot show it, which ADR 0004 itself notes.

2. Fix the PR description: it says the setter checks ids before writing, but the code writes, then rolls back.

Details

The outcome is the same. Item 1's fix makes the description true; otherwise, correct it. Captured SQL on failure: SAVEPOINT, UPDATE, SELECT, ROLLBACK TO SAVEPOINT, RELEASE SAVEPOINT. The outcome is still all-or-nothing and the docstring is correct, but a reviewer checking criterion 4 ("writes nothing") from the description would review the wrong mechanism. If you skip item 1's fix, reword the description to "writes inside a transaction and rolls back if any id did not match".

Decisions to make or write down

3. Agree on the setter's name in the PR thread, then update openedx#777 and openedx#782 to match.

Details

The PR ships the plural lock_object_tags_for_deletion; openedx#777 and openedx#782 use the singular lock_object_tag_for_deletion. The plural matches the module's get_object_tags and delete_object_tags, and ormsbee's openedx#777 approval left room to workshop names, but until the rename is confirmed, openedx#782 names a function that does not exist.

4. Decide what is_taxonomy_locked_for_deletion(None) returns. It likely says "locked" because of locked rows left over from deleted taxonomies.

Details

Return False early, or document that None is unsupported. Deleting a taxonomy sets its associations' taxonomy_id to NULL, and filter(taxonomy_id=None) becomes IS NULL, so None matches those rows (inferred, not reproduced). The signature says int, so this is a decision rather than a bug.

Improvements worth doing

5. Reject string, bytes, and boolean ids. "12" locks rows 1 and 2, True locks row 1, and locks cannot be undone through the API.

Details

True passes the isinstance(..., int) check and locks primary key 1, and "12" is iterated as characters and locks primary keys 1 and 2. A mixed list like [1, "1"] raises a false DoesNotExist naming '1', because the set holds two members and the UPDATE matches one row. A list of two missing ids of mixed types crashes in sorted() with a TypeError. The review reproduced every case in this item in a scratch script.

Follow tag_object, which raises ValueError for a non-list, or normalize with {int(i) for i in ...}, and add one parameterized test.

6. Return early on empty input, so it issues no lock query, as openedx#782 requires.

Details

openedx#782's no-criteria path must "issue no lock query at all"; atomic() emits SAVEPOINT and RELEASE even for empty input. Add the guard and a test for it.

7. Complete the docstrings: say that locks are permanent and that the readers don't lock rows.

Details

• The setter's docstring should say that the lock is permanent with no unlock API, that it nests as a savepoint and is not durable, and that it holds row locks until the caller commits.
• The readers' docstrings should say they are non-locking point-in-time reads, so openedx#779 and openedx#780 must re-check deletion_locked in their DELETE's WHERE clause.
• Move the "Archived associations count..." sentence above the :param lines, out from after :return:.

8. Test the readers' edge cases: unknown tag ids and empty input.

Details

get_tags_locked_for_deletion's docstring promises to omit unknown tag ids, and nothing tests it. Test empty input on both readers.

9. In the multi-id test, assert one UPDATE instead of three queries, so item 1's fix doesn't break it.

Details

test_lock_multiple_ids_in_one_call uses assertNumQueries(3), which counts atomic()'s SAVEPOINT and RELEASE, so any transaction change breaks it, including item 1's fix, even with correct behavior. Use CaptureQueriesContext and assert exactly one UPDATE.

Tests ran on SQLite only, so CI gives the two MySQL-sensitive tests, this one and the idempotency test, their first MySQL run.

Small things

• Consider capping the list of missing ids in the DoesNotExist message.
• Export ObjectTagDoesNotExist beside TagDoesNotExist.
• Comment the MySQL FOUND_ROWS dependency at the count check.

Details

• The DoesNotExist message lists every missing id, with no cap.
• Export ObjectTagDoesNotExist = ObjectTag.DoesNotExist, so callers need not import models.
• On MySQL, idempotent re-locking relies on Django's FOUND_ROWS flag. The comment is unnecessary if item 1's fix lands, because that fix removes the dependency.

[jesperhodge]

Suggested change

	"""
	An archived but locked association still counts for both readers.
	This guards against adding ``.unarchived()`` to the readers, which would hide associations that anchor
	learner history.
	"""
	"""
	get_tags_locked_for_deletion() and is_taxonomy_locked_for_deletion() still report
	a locked association after it has been archived.
	Removing a locked tag from content archives the association instead of deleting
	it. If these functions skipped archived rows, deleting that tag or its taxonomy
	afterward would really delete it, and the learner mastery status pointing at the
	association would lose its tag. Adding .unarchived() to either function makes
	this test fail.
	"""

[tbain]

For this one, I asked Claude to make the comments in general less verbose, so while it applied this comment it then edited it for verbosity. I think the new comment still serves its purpose to get this message across, albeit it might be a little terse. Let me know if you would prefer to keep the longer comment, though.

[jesperhodge]

AI likes to specify what a function does not do. I suggest we remove this from the docstring.

[tbain]

Addressed, I think.

[tbain]

@jesperhodge I think all of your comments are addressed;
AI generated summary edited by me:

1. The setter now reads deletion_locked first and only UPDATEs rows still unlocked, per the [Arch] Implementation approach for competency data delete/edit guardrails openedx/openedx-core#655 design. An already-locked association takes no row lock; test_lock_is_idempotent asserts no UPDATE is issued. I kept a rowcount check on the write path so a row deleted between the read and the write still raises rather than returning success; it only runs on a shortfall, so the hot path is unaffected. [BE] Lock tagging records against deletion when a learner competency status is written openedx/openedx-core#782's "do not add a read to avoid a write" sentence needs a ticket edit, which I'll raise there.
2. Keeping the plural lock_object_tags_for_deletion; it matches get_object_tags and delete_object_tags. I'll update [BE] Add deletion-lock functions to openedx_tagging openedx/openedx-core#777 and [BE] Lock tagging records against deletion when a learner competency status is written openedx/openedx-core#782 to the plural once that's settled here.
3. None returns False early, with a test that deletes a taxonomy and checks the leftover rows don't count.
4. str, bytes, bool, float, None, and arrays containing those types raise TypeError before any query (9 parameterized cases).
5. 6, 7, 8, 9. Done as suggested. ObjectTagDoesNotExist is exported.
6. I skipped capping the missing-id list; if a caller is passing many bad ids I feel like we'd want the verbose, complete list rather than a capped number, though if we do feel like we want this cap what do we feel is appropriate? 10, 20, 100?

[jesperhodge]

Hi Tyler,
this looks really good, only one thing.

In lock_object_tags_for_deletion, you fixed the issue I found with the db lock correctly and removed the db lock. That's good.
However you also removed atomic(), which creates another small issue.
AI and me both think that it would be best to keep atomic() without the db lock - so NO db lock, but atomic() yes. Because that will fix this race condition you identified earlier.

Something like this, according to Claude:

if unlocked:
    with transaction.atomic():
        updated = ObjectTag.objects.filter(pk__in=unlocked, deletion_locked=False).update(deletion_locked=True)
        if updated != len(unlocked):
            surviving = set(ObjectTag.objects.filter(pk__in=unlocked).values_list("pk", flat=True))
            deleted = set(unlocked) - surviving
            if deleted:
                raise ObjectTag.DoesNotExist(...)