OpenConceptLab/ocl_online#454 | Load list-row lookups once per page for concepts and mappings

[paynejd]

Relates to OpenConceptLab/ocl_online#454 (partial: the ticket tracks two more causes, listed at the end).

Summary

Concept and mapping lists made several queries per row while serializing a page. For each row they looked up:

• the latest version's uri (version_url);
• latest_source_version;
• the from/to concepts' sources and the sources' owners (mappings);
• created_by/updated_by.

A 1,000-row page of mappings ran about 8,500 queries.

This change loads those once per page, after pagination:

• SourceChildMixin.load_list_fields(instances) prefetches each model's LIST_PREFETCHES. It then runs one DISTINCT ON query for the HEAD rows' latest version uris and one for each row's latest released source version (plus an in_bulk for those sources).
• SourceChildPageSerializer, a ListSerializer, calls it for the page. It's set as list_serializer_class on Concept{List,VersionList,Detail}Serializer and Mapping{List,VersionList,Detail}Serializer, so search results get it too.
• version_url and latest_source_version use the page-loaded values when they're there. Serializing a single object runs the same code as before.

It loads after pagination rather than annotating the page query, because a subquery annotation is evaluated for every row an OFFSET skips. On a deep page of a large source, EXPLAIN showed both subqueries running for the ~300k skipped rows.

Collection-scoped lists (from review)

• latest_source_version is back in collection scope, as the source version the row's references in that collection resolved to (newest first; HEAD gives null). That's what TBv3 computes from includeReferences today; the plain latest release was the ocl_issues#2770 bug. It's loaded once per page.
• Lookups only run for fields the response includes.
• Collection pages are flat too. They no longer query per row for previous_version_url, references, names, descriptions, source_versions or collection_versions.
• Three outputs now have a fixed order: source_versions by created_at, collection_versions as a sorted list (was a set), and reference uris by id.

Query counts

Local test stack, CIEL-shaped fixture, 1,000-row page of HEAD mappings:

	Queries	Time (local DB)
master	8,505	5.4 s
this branch	14	1.3 s

The local DB has no network round trip, so production saves more per query than this shows. Timings from before the change are recorded on the ticket.

Tests

• MappingListQueryCountTest and ConceptListQueryCountTest (new, sharing SourceChildListQueryCountMixin in core/integration_tests/test_utils.py) cover HEAD, repo-version, collection and global lists, both default and verbose=true. Each checks three things:
  • a page of 4 runs the same number of queries as a page of 2;
  • the response equals the rows serialized one at a time;
  • latest_source_version and version_url have the expected values.
• ConceptListQueryCountTest.test_revisions_in_no_released_version covers search-result rows saved since the last release.
• The new tests fail on master. A page of 2 vs 4 takes 22 → 39 queries (mappings HEAD), 22 → 37 (mappings version), 9 → 13 (concepts HEAD), 11 → 17 (concepts version) and 12 → 24 (concept revisions). They pass on this branch.
• Locally, the full suite (2,539 tests) passes and pylint -j0 core/ is clean.

Review

• Codex pass 1: one P1 (F1.1), fixed in 61274f6. A concept revision in no released version still made one query per row through get_source_version_before_creation().
• Codex pass 2: clean.
• Simplification review: 4 cuts, applied in 1744f47. One shared latest_source_version on the mixin, two redundant guards dropped, shared test mixin.
• Codex check of the cuts: clean.

Details are in the comments below.

Not in this PR (tracked on OpenConceptLab/ocl_online#454)

• Deep pages of a HEAD list. The page query joins the from/to concepts for every row the OFFSET skips. The repo-version path already selects the page's ids first and doesn't have this cost. That's OpenConceptLab/ocl_online#454 | Page HEAD concept and mapping lists by ids first #937.
• Rows without stored checksums. These still calculate and save them during a GET (backfill: OpenConceptLab/ocl_online#475).

🤖 Generated with Claude Code

[paynejd]

Codex review, pass 1 (codex-cli 0.162.1, commit 4cb6f4e)

Verdict: 1 finding (P1).

• F1.1 (P1) core/concepts/models.py:986: concept revisions in no released version (e.g. search results created since the last release) still make one query per row: summary_properties → get_parent_source_version() falls through to get_source_version_before_creation(), although the page load already found no released version for the row.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "full",
    "commit": "4cb6f4e1",
    "verdict": "findings",
    "prior": [],
    "findings": [
        {
            "id": "F1.1",
            "severity": "P1",
            "tag": null,
            "file": "core/concepts/models.py",
            "line": 986,
            "claim": "Concept search pages still perform one source query per revision without a non-HEAD source association.",
            "scenario": "Search results can contain concept revisions created since the last source snapshot. The batch loader caches latest_source_version=None, but serializing their property field calls get_parent_source_version(), which falls through to get_source_version_before_creation() and queries sources again. Pages containing 25 or 1,000 such revisions therefore incur 25 or 1,000 additional queries. The new tests miss this because every revision belongs to v1.",
            "action": "Make parent-source resolution honor the cached absence and return the prefetched parent without another query. Add query-count coverage for search results containing revisions with no non-HEAD source membership.",
            "lines_delta": null,
            "confidence": "high"
        }
    ]
}

[paynejd]

Response to Codex pass 1:

• F1.1 (P1): fixed in 61274f6. get_source_version_before_creation() returns None when load_list_fields found no released version for the row: it only searches those versions. New test ConceptListQueryCountTest.test_revisions_in_no_released_version serializes revisions as search results arrive (by id, names prefetched). Master runs 12 → 24 queries for 2 → 4 rows; this branch stays flat.

[paynejd]

Codex review, pass 2 (codex-cli 0.162.1, commit 61274f6)

Verdict: clean. F1.1 resolved; no new findings.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "verify",
    "commit": "61274f65",
    "verdict": "clean",
    "prior": [
        {
            "id": "F1.1",
            "status": "resolved"
        }
    ],
    "findings": []
}

[paynejd]

Simplification review (fresh Claude subagent, commit 61274f6)

Verdict: 4 cuts proposed, all applied in 1744f47.

• S1 (shrink, tests): the concept and mapping query-count tests duplicated each other. Applied: they share SourceChildListQueryCountMixin in core/integration_tests/test_utils.py.
• S2 (shrink): Concept and Mapping had identical latest_source_version overrides, which left the mixin's own version unused (only these two models use SourceChildMixin). Applied: the shared body is on the mixin, next to load_list_fields, and the overrides are gone.
• S3 (delete): the empty-page and no-heads guards; Django runs no SQL for an empty __in, in_bulk(set()) or an empty prefetch. Applied.
• S4 (shrink): always set _latest_version_uri, since version_url falls back on None. Applied.

After the cuts, the new tests pass on this branch and fail on master: all 5, with 22 → 39, 22 → 37, 9 → 13, 11 → 17 and 12 → 24 queries for a page of 2 vs 4. pylint -j0 core/ is clean. The full suite is rerunning.

simplify.json

{
    "reviewer": "claude subagent",
    "perspective": "simplify",
    "mode": "full",
    "commit": "61274f65",
    "verdict": "findings",
    "prior": [],
    "findings": [
        {
            "id": "S1",
            "severity": null,
            "tag": "shrink",
            "file": "core/integration_tests/tests_mappings.py",
            "line": 1088,
            "claim": "MappingListQueryCountTest duplicates ConceptListQueryCountTest almost line for line (get_page, assert_page, set_released_versions, test_head_list, test_version_list differ only in model, M2M name, url attribute and serializers).",
            "scenario": null,
            "action": "Move the shared methods into one SourceChildListQueryCountMixin parameterized by model, relation, list_serializer, detail_serializer; each test class keeps its setUp and attributes.",
            "lines_delta": -40,
            "confidence": "medium"
        },
        {
            "id": "S2",
            "severity": null,
            "tag": "shrink",
            "file": "core/concepts/models.py",
            "line": 984,
            "claim": "Concept.latest_source_version and Mapping.latest_source_version are identical overrides with the same new guard; the mixin's own latest_source_version is dead code.",
            "scenario": null,
            "action": "Put the shared body (with the _latest_source_version guard) on SourceChildMixin and delete both overrides.",
            "lines_delta": -10,
            "confidence": "medium"
        },
        {
            "id": "S3",
            "severity": null,
            "tag": "delete",
            "file": "core/common/mixins.py",
            "line": 650,
            "claim": "The early `if not instances: return` and `if heads else {}` guard cases Django already short-circuits without SQL.",
            "scenario": null,
            "action": "Nothing replaces it.",
            "lines_delta": -2,
            "confidence": "medium"
        },
        {
            "id": "S4",
            "severity": null,
            "tag": "shrink",
            "file": "core/common/mixins.py",
            "line": 671,
            "claim": "The `if instance.id in latest_version_uris` branch is unnecessary; version_url falls back when the value is None.",
            "scenario": null,
            "action": "instance._latest_version_uri = latest_version_uris.get(instance.id)",
            "lines_delta": -1,
            "confidence": "high"
        }
    ]
}

[paynejd]

Codex review, check of cuts (codex-cli 0.162.1, commit 1744f47)

Verdict: clean. The simplification commit broke nothing.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "cuts",
    "commit": "1744f476",
    "verdict": "clean",
    "prior": [],
    "findings": []
}

[snyaggarwal]

Issue complete: no.

Findings:

1. Collection-scoped concept/mapping list responses still omit latest_source_version, but oclweb3 uses that value in listing rows to render the source repo version. In oclweb3, ResultConstants.js renders the concept Repo column via sourceVersionOf(item), and sourceVersionOf() falls back to resource.latest_source_version when references are not present. Because the API omits the field for collection-scoped serializers, oclweb3 has to request includeReferences=true&includeResolvedRepoVersions=true for collection concept/mapping pages just to reconstruct the source version. That is a much heavier path, and in local testing even limit=1 collection requests with references were slow enough to abandon. Please keep/provide latest_source_version for collection-scoped concept and mapping list rows instead of relying on the references fallback.

2. load_list_fields() always performs the latest-source-version through-table lookup even when the active serializer removed latest_source_version from the response. On local LOINC/CIEL data this added ~13 ms / ~5k buffer hits for a 1,000-row collection concept page and ~20 ms / ~4k buffer hits for a collection mapping page, despite the value not being serialized. If the field remains conditional, this work should be gated by the active serializer fields; if we restore the field for collections, this should be measured as intentional response data rather than wasted work.

Additional query-plan notes from local oclapi2-db-1:

• LOINC HEAD source had ~535k concept members and ~537k mapping members; LOINC version 1507 had ~267k concept and ~268k mapping members.
• Source-version latest-source lookups for 1,000 LOINC concept/mapping IDs were about 10-11 ms hot.
• Global latest-source lookups for 1,000 global concept/mapping rows were about 12-13 ms hot.
• The version_url latest-version lookup was ~15-18 ms hot after warming, but cold/mixed-cache runs were materially higher (~150 ms for mappings and one ~485 ms concept run).

Security blockers: none.
Testing gap: the new tests cover source HEAD/version pages, but not global list pages or collection-scoped concept/mapping list responses consumed by oclweb3.

[paynejd]

Codex review, pass 3 (codex-cli 0.162.1, commit 11caddf)

Verdict: 1 finding (P1). F1.1 still resolved.

• F3.1 (P1) core/common/mixins.py: resolving a reference's source version is memoized per (namespace, system, version), so a collection page referencing many different source versions still runs resolve_reference_expression once per distinct version.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "verify",
    "commit": "11caddf1",
    "verdict": "findings",
    "prior": [
        {
            "id": "F1.1",
            "status": "resolved"
        }
    ],
    "findings": [
        {
            "id": "F3.1",
            "severity": "P1",
            "tag": null,
            "file": "core/common/mixins.py",
            "line": 788,
            "claim": "Collection source-version resolution still queries per row when references span distinct sources or versions.",
            "scenario": "For a collection containing rows from different source/version tuples, every tuple misses the memoization cache. Each call to Source.resolve_reference_expression performs repository and version queries, so database work grows with page size. The new collection test only references self.v1, which hides this remaining N+1 path.",
            "action": "Batch resolution of the page's distinct source/version keys, preserving reference-resolution semantics, and add query-count coverage with collection rows spanning multiple sources and versions.",
            "lines_delta": null,
            "confidence": "high"
        }
    ]
}

[paynejd]

Response to Codex pass 3:

• F3.1: accepted as a P2.
  • The lookup scales with the number of distinct source versions on a page, about 2–4 queries each, not with rows. Collections usually pin one or a few sources.
  • It's the same per-version memoization includeResolvedRepoVersions uses today, so nothing regresses.
  • Batching resolve_reference_expression faithfully would reimplement canonical URLs, owner and global URL registries, namespaces and the toggle. That risks changing the shown version, the #2770 bug.
  • If this matters for a real collection, it can be a follow-up.

[paynejd]

Simplification review of the review response (fresh Claude subagent, commit 11caddf)

Verdict: 6 cuts proposed: S2–S5 applied in ba8e086, S1 and S6 declined.

• S1 (delete): declined. The duplicate pop in ConceptVersionListSerializer is on master already. The parent's pops sit in a try/except, where an earlier pop that raises skips the rest, so this one isn't provably dead.
• S2 (shrink): applied. load_list_fields loads collection references once, for either field that reads them.
• S3 (delete): applied. Reference ids are a plain loaded attribute.
• S4 (yagni): applied. FIELD_PREFETCHES is a tuple of relation names.
• S5 (shrink): applied. A shorter docstring.
• S6 (delete, low confidence): declined. The 'collection' in view.kwargs check keeps the collection scope explicit.

After the cuts: pylint -j0 core/ is clean, and the concepts, mappings and collections integration tests pass (323).

simplify.json

{
    "reviewer": "claude subagent",
    "perspective": "simplify",
    "mode": "full",
    "commit": "11caddf1",
    "verdict": "findings",
    "prior": [],
    "findings": [
        {
            "id": "S1",
            "severity": null,
            "tag": "delete",
            "file": "core/concepts/serializers.py",
            "line": 353,
            "claim": "ConceptVersionListSerializer repeats the parent's latest_source_version pop.",
            "scenario": null,
            "action": "delete the subclass's view_kwargs line and pop",
            "lines_delta": -3,
            "confidence": "high"
        },
        {
            "id": "S2",
            "severity": null,
            "tag": "shrink",
            "file": "core/common/mixins.py",
            "line": 778,
            "claim": "load_collection_source_versions pre-loads references that load_list_fields could load once.",
            "scenario": null,
            "action": "load references in load_list_fields for references or latest_source_version; drop the pre-load",
            "lines_delta": -3,
            "confidence": "medium"
        },
        {
            "id": "S3",
            "severity": null,
            "tag": "delete",
            "file": "core/common/mixins.py",
            "line": 764,
            "claim": "The (collection id, ids) tuple guards a collection mismatch that can't happen within a request.",
            "scenario": null,
            "action": "plain _collection_reference_ids with a hasattr check",
            "lines_delta": -1,
            "confidence": "medium"
        },
        {
            "id": "S4",
            "severity": null,
            "tag": "yagni",
            "file": "core/concepts/models.py",
            "line": 224,
            "claim": "FIELD_PREFETCHES is a dict with one entry whose field equals its lookup.",
            "scenario": null,
            "action": "a tuple of relation names; prefetch fields.intersection(FIELD_PREFETCHES)",
            "lines_delta": -1,
            "confidence": "medium"
        },
        {
            "id": "S5",
            "severity": null,
            "tag": "shrink",
            "file": "core/common/mixins.py",
            "line": 659,
            "claim": "load_list_fields' docstring restates its loaders.",
            "scenario": null,
            "action": "keep the first sentence",
            "lines_delta": -3,
            "confidence": "medium"
        },
        {
            "id": "S6",
            "severity": null,
            "tag": "delete",
            "file": "core/common/serializers.py",
            "line": 231,
            "claim": "get_collection_in_scope's view.kwargs check repeats what request.instance establishes.",
            "scenario": null,
            "action": "check request.instance only",
            "lines_delta": -2,
            "confidence": "low"
        }
    ]
}

[paynejd]

Codex review, check of cuts (codex-cli 0.162.1, commit ba8e086)

Verdict: clean. The simplification commit broke nothing.

findings.json

{
    "reviewer": "codex-cli 0.162.1",
    "perspective": "defects",
    "mode": "cuts",
    "commit": "ba8e0860",
    "verdict": "clean",
    "prior": [],
    "findings": []
}

[paynejd]

@snyaggarwal thanks, all three are addressed in 11caddf and ba8e086 (master merged in 04576dc, no conflicts).

1. latest_source_version in collection-scoped lists. It's back, but as the version the collection resolved to, not the latest release. You hid it in ocl_issues#2770 because the plain latest release was wrong there, so returning that again would bring the bug back.

• In collection scope, the field is now the newest source version the row's references in that collection resolved to. HEAD gives null.
• That's the value oclweb3's sourceVersionOf() computes from references[].resolved_repo_versions today. So collection list pages can drop includeReferences=true&includeResolvedRepoVersions=true (Search.jsx) once this deploys, and show the same version.
• One difference: it counts source (system) versions only. oclweb3 also counts valueset (collection) versions, which isn't a source version anyway.
• It's loaded once per page. A single object in collection scope (e.g. concept detail under a collection) now returns it too, instead of hiding it.
• If a view has collection in its kwargs but hasn't loaded the collection into request.instance, the field stays hidden, as before.

2. Work for fields the response drops. load_list_fields now runs only the lookups the serialized fields read. Collection mapping lists skip the latest-release lookup; collection concept lists still need it for property (through get_parent_source_version).

3. Testing gap. SourceChildListQueryCountMixin now covers collection lists (default and verbose) and global /concepts/ and /mappings/. Each checks that a page of 4 runs the same queries as a page of 2 and matches row-by-row output. The collection test also checks that rows show the pinned v1, not the latest release v2. All 9 tests fail on master: collection mappings 29 → 52, collection concepts 24 → 42.

Also changed while making collection pages flat:

• Collection pages no longer query per row for previous_version_url, references, names, descriptions, source_versions or collection_versions.
• Three outputs now have a fixed order where it was arbitrary before:
  • source_versions is ordered by source version created_at;
  • collection_versions is a sorted list (was a set);
  • reference uris are ordered by id.

Review loop:

• Codex pass 3 found F3.1, which I accepted as a P2 (reason above). Resolving reference versions is memoized per distinct source version, about 2–4 queries each. That's the same as includeResolvedRepoVersions today.
• The simplification review applied 4 of 6 cuts. Codex's check of the cuts was clean.
• Locally, the full suite passes (2,554 tests at 11caddf; the concepts, mappings and collections modules again after the cuts), and pylint is clean.