Add thread-safe clear_cache suppressor and extract_cot_id_from_model_name

[bctiemann]

Summary

Separates two infrastructural utilities from PR #442 (polymorphic object fields) into a standalone, reviewable PR.

Changes

utilities.py

extract_cot_id_from_model_name(model_name)
Replaces the fragile chained .replace("table", "").replace("model", "") pattern used throughout the codebase to extract a COT primary key from an internal model name (e.g. table3model → "3"). The old approach silently corrupts names that happen to contain the substrings "table" or "model". The new function uses a strict regex (^table(\d+)model$) and returns None for non-matching names.

Thread-safe apps.clear_cache suppressor
Django's apps.register_model() calls apps.clear_cache(), which triggers our get_models() override → get_model() for every known COT → potential infinite recursion and spurious cache invalidations during dynamic model registration.

The old fix was a non-thread-safe module-level monkey-patch (apps.clear_cache = lambda: None) with a broken restore (apps.clear_cache = apps.clear_cache). The new approach:

• install_clear_cache_suppressor() — called once from AppConfig.ready(), installs a single wrapper on apps.clear_cache (idempotent).
• _suppress_clear_cache() — private context manager using a thread-local depth counter. Only the calling thread sees suppression; all other threads continue to receive real cache invalidation.
• generate_model() — updated to use _suppress_clear_cache() instead of the lambda patch.

__init__.py

• Calls install_clear_cache_suppressor() at the top of ready().
• Replaces the chained .replace() calls in get_model() with extract_cot_id_from_model_name().

Relationship to PR #442

PR #442 (polymorphic object fields) uses both of these utilities extensively. Once this PR merges, #442 should be rebased so it picks them up from feature rather than defining them itself. Note: models.py in #442 imports suppress_clear_cache directly — that import should be updated to use the private _suppress_clear_cache after rebase.

Migrations

This PR also fixes the migration numbering conflict present in feature.

Generated with Claude Code

[bctiemann]

@claude /review

[claude]

Claude finished @bctiemann's task in 1m 40s —— View job

---

PR Review: Add thread-safe clear_cache suppressor and extract_cot_id_from_model_name

• Read changed files
• Analyze code quality and correctness
• Post review feedback

---

Overview

This PR cleanly extracts two focused utilities from the larger PR #442. The design is sound — replacing the non-thread-safe monkey-patch with a thread-local depth counter is the right approach, and the regex-based extraction is strictly better than chained .replace().

However, there are a couple of bugs introduced in the callers that need addressing before merge.

---

Bugs

1. TypeError when extract_cot_id_from_model_name returns None — __init__.py:238

custom_object_type_id = int(extract_cot_id_from_model_name(model_name))

If model_name passes the pre-guard at line 225 (which only checks for the substrings "table" and "model") but fails the strict regex (e.g. "sometablemodel", "table_model"), extract_cot_id_from_model_name returns None and int(None) raises TypeError — not LookupError. Callers expecting a LookupError will see an uncaught exception instead.

Fix: check the return value before calling int(). Fix this →

The pre-guard at line 225 is also inconsistent with the regex — it should either use extract_cot_id_from_model_name itself for the early-out check, or be documented as a fast-path that is not exhaustive.

2. AttributeError on None.isdigit() — forms.py:111

custom_object_type_id = extract_cot_id_from_model_name(obj.model)
if custom_object_type_id.isdigit():

Previously the chained .replace() calls always returned a string (possibly ""), so .isdigit() was safe. Now extract_cot_id_from_model_name can return None, making .isdigit() an AttributeError. Since the function only matches the strict ^table(\d+)model$ pattern, the returned string is always all-digits when non-None, so the .isdigit() check is also now redundant. Fix this →

---

Minor Issues

models.py imports private _suppress_clear_cache directly

models.py:57 imports and uses _suppress_clear_cache at line 588, rather than going through generate_model(). The PR description acknowledges the analogous issue in #442, but this file is changed in the current PR diff. The usage pattern is different (calling apps.register_model() directly, not type()), so direct use of the context manager is arguably correct — but it would be worth a brief inline comment explaining why generate_model() isn't used here.

views.py:532 and field_types.py — no None guard before CustomObjectType.objects.get(pk=...)

Many call sites pass the return value of extract_cot_id_from_model_name directly to a DB query without checking for None. In practice these code paths are only reached when content_type.app_label == APP_LABEL, which means the model name should always match the pattern — but this is an implicit invariant. A defensive check or an assertion would make the assumption explicit.

install_clear_cache_suppressor has a minor TOCTOU race on the idempotency check

if apps.clear_cache is _wrapped_clear_cache:
    return
_real_clear_cache = apps.clear_cache
apps.clear_cache = _wrapped_clear_cache

Two concurrent callers could both pass the is _wrapped_clear_cache check and then both attempt to assign _real_clear_cache. The second assignment would store _wrapped_clear_cache itself as _real_clear_cache, effectively breaking suppression. Since this is called from AppConfig.ready() (single-threaded startup), this is not a real-world issue, but a comment noting the assumption would be helpful.

---

Positives

• Thread-local depth counter design is clean and reentrant — the finally block ensures decrement even on exception.
• The regex ^table(\d+)model$ is strict and self-documenting; the compiled module-level _COT_MODEL_RE is the right approach.
• install_clear_cache_suppressor idempotency check is good.
• Suppression happening at the type() call in generate_model() and around apps.register_model() in models.py covers both registration paths.
• The __all__ export list correctly keeps _suppress_clear_cache private.

---

Tests

No new unit tests for extract_cot_id_from_model_name (e.g. valid inputs, None-returning edge cases, empty string, names with embedded "table"/"model" substrings). A few parametrized tests would be valuable given this function replaces a pattern used in ~10 places. Same for the thread-safety behavior of the suppressor.

[bctiemann]

Addressed all surfaced issues. PR #442 will need to rebased on feature once this is merged.

[arthanson]

Just a question on if assert is correct of if we want to Raise the error on these checks.

[bctiemann]

Changed the asserts to ValueErrors.