[paynejd]

FHIR R4 terminology service (SVCM transactions)

This series makes oclapi2's FHIR endpoints a FHIR R4 terminology service for the IHE SVCM transactions: CodeSystem, ValueSet and ConceptMap search and read, $lookup, $validate-code, $expand, $translate, and the CapabilityStatement and TerminologyCapabilities. It serves them at the global base (/fhir/) and at owner bases (/orgs/{org}/, /users/{user}/), in JSON and XML. Tracking: OpenConceptLab/ocl_online#451.

How to review it: 17 commits on master addb3d5e: one per work package, each with a message describing its behaviour, then a fix from this PR's review. Only the final tree has been gated (the full test suite, pylint and coverage). Every intermediate tree compiles but hasn't been run on its own.

Commits

1. FHIR requests and errors use native FHIR JSON and XML
2. Canonical lookups preserve registry entries and support FHIR version rules
3. FHIR search and read return the releases a caller may view
4. Missing FHIR operation inputs get consistent, typed error outcomes
5. FHIR lookup and validation answer for the requested version and value set
6. FHIR translation returns the requested map's live mappings and R4 equivalences
7. FHIR expansion pages the requested value set's stored members
8. FHIR metadata describes the operations and code-system versions each base serves
9. ConceptMap system searches use indexed candidates and check each release's mappings
10. CodeSystem and ValueSet titles fall back to the name when no full name is set
11. A named FHIR version resolves to a visible repository that holds it
12. Value set members stored as root concept rows report a holding release
13. FHIR operations reject parameters with competing values and explain ConceptMap URI searches
14. Root concept members report the newest visible release that holds the concept
15. Filtered expansions match members once for both the total and the page
16. Instance operations serve named unreleased versions; capped reads say they're partial
17. Only /fhir/ and owner bases are FHIR paths (review fix)

Outside core/fhir/

These are worth a closer look, because they change code that non-FHIR paths share:

• The shared canonical resolver (ConceptContainerModel.resolve_reference_expression / resolve_repo in core/common/models.py, core/repos/models.py, core/url_registry/models.py, core/common/mixins.py). FHIR resolves through the same resolver as $resolveReference, with flags that only FHIR passes: no HEAD by default, the HEADs a base shows, and a visibility check before any version is read. Registry reads no longer save or reindex entries. With the URL registry on, $resolveReference accepts namespace for expressions that name none; an expression's own namespace wins.
• Middleware and settings: FHIR errors are OperationOutcomes from an outermost middleware, and a server error on a FHIR path becomes a 422. A FHIR path is one under /fhir/ or under an owner base's FHIR routes; other routes keep their own errors. The FHIR validator setting is gone, because XML is converted locally.
• Dependencies: fhir.resources[xml] goes from 7.1.0 to 8.3.0, and the CodeSystem converter used by imports moves to the R4B models.
• Routing: the FHIR routes at the owner bases and the global base.

There are no migrations. The URL registry toggle stays as it is: FHIR works with it on or off.

Testing

• Full suite on the final tree: 3,452 tests OK, pylint 10.00/10, coverage 96% (99% in core/fhir).
• Parity: FHIR answers were compared with FHIRsmith v0.15.0 as an oracle, on a local copy of the corpus and on a staging deployment of this tree's line. Every difference is either fixed or recorded with its reason. The report is on the tracking ticket.

Known limits and follow-ups

• reference searches: OpenConceptLab/ocl_online#547.
• Computed expansions, hierarchy and subsumption, and later FHIR versions: OpenConceptLab/ocl_online#452.
• Expansion member versions for root-row members: OpenConceptLab/ocl_online#549.
• $expand with filter on the largest stored expansions (about 38,000 members) can exceed the database time limit and answer 422: OpenConceptLab/ocl_online#564.

🤖 Generated with Claude Code

https://claude.ai/code/session_01R8hw7w8753DDSCezrmrDA8

[paynejd]

Review loop (Codex pass 1, simplification, Codex check)

• Codex pass 1 (codex-cli 0.163.0, commit 77bd094): no P0 or P1. One P2, fixed in ebe370c4. The FHIR path check treated any path whose third segment is a FHIR resource type as FHIR, such as a current user's source with the id CodeSystem (/user/sources/CodeSystem/), so its errors were turned into OperationOutcomes. It now applies only to /fhir/ and the owner bases /orgs/… and /users/…, the only places FHIR is routed.
• Simplification review: one test trim applied; two small cuts to already-tested code declined.
• Codex check of 77bd094..ebe370c: clean, and the P2 is resolved.
• Tests on the fix: core.fhir, core.middlewares, core.users and core.orgs pass, 843 tests; pylint 10.00.

The full findings are on the tracking ticket.

[paynejd]

Follow-up stacked on this branch: #957 gives the FHIR host its own Swagger page, listing only the operations this PR serves. It goes to master after this one.

[snyaggarwal]

Review focused on: non-FHIR behaviour changes, missing tests, module-function vs OOP design, duplication, complexity and feature envy. Line numbers are from the PR head.

1. Non-FHIR behaviour changes

• ConceptMap import map types (core/concept_maps/serializers.py:46, also used by core/importers/importer.py): R4 equivalences now become OCL map types on import (wider→NARROWER-THAN, narrower→BROADER-THAN). This is the reverse mapping OpenConceptLab/ocl_issues#2924 asks for, so it's intended. Please link that ticket in the description so it closes on merge. The PR also maps unmatched/disjoint→NOT-SAME-AS, which goes beyond the ticket but looks reasonable.
• Synchronous registry relink on every repo/version save. Source.save / Collection.save now call link_url_registry_entries() whenever canonical_url is dirty, which includes every new version row: an extra URLRegistry query plus lookup_entry() per create, inside the request or import, next to the existing async resolve_url_registry_entries task. Two mechanisms now maintain the same link (one sync, one async). The block is also duplicated between core/sources/models.py:702 and core/collections/models.py:170; it could live once on ConceptContainerModel.
• Delete relinks entries in every namespace. relink_url_registry_entries (core/common/models.py:976) re-resolves every active entry with the deleted canonical URL, whatever its namespace, so a widely registered URL means N lookups inside the delete.
• URLRegistry._set_repo (core/url_registry/models.py:140) now clears the link when nothing resolves, and reads no longer write. Intended and tested, but URL-registry API responses change for entries whose URL no longer resolves; worth calling out in the description.
• $resolveReference gains ?namespace= (toggle-gated, tested), and resolve_expression_to_version now applies it to bare-string expressions too.
• Looks fine: middleware reorder, FhirWSGIRequest (FHIR paths only), the BundleMetaSerializer.lastUpdated default fix (datetime.now() → datetime.now, a real bug fix), and the convert_ocl_uri_to_fhir_url segment-only rename.

2. Missing tests

• FhirErrorMiddleware (core/middlewares/middlewares.py:278): no test that an exception on a non-FHIR path is still re-raised (test_answers_left_alone covers responses only).
• FhirWSGIRequest: no test that a malformed Content-Type on a non-FHIR path behaves as before; every wsgi test uses a FHIR path.
• Optional: an export → import round-trip test for map types, the round trip OpenConceptLab/ocl_issues#2924 describes. test_group_equivalence_is_the_map_type_relationship covers the mapping at the serializer level.
• No query-count test for creating a source/version with a canonical URL, now that save does a synchronous registry lookup.
• get_latest_released_version(exclude_head=True) is only tested indirectly.
• convert_ocl_uri_to_fhir_url tests live in core/fhir/tests_capabilities.py:302 rather than with IdentifierSerializerTest in core/common/tests.py.

3. Module-level functions vs OOP

• core/fhir/resolve.py is ~25 free functions threading (namespace, user) / (kwargs, user) through every call (is_shown, is_visible, has_release, may_view_private, owner_access, shown_q, viewable_rows, visible_versions, held_version, ...). That data clump wants to be one per-request access/resolver object.
• Parameter parsing is scattered free functions taking parameters: text, require, coding_of (ops/concepts.py), required_text, boolean, coding_of (ops/translate.py:185,196), whole_number, flag (ops/expand.py:106,118). They belong as typed accessors on the existing FhirParameters (core/fhir/base.py:473).
• Import-time module state: capabilities.STARTED = timezone.now() (capabilities.py:38) and the mutable LOGGED_DROPPED_ELEMENTS = set() (base.py:73).
• REGISTRY_TOGGLE / ServedHead are declared mid-module (resolve.py:388).
• base.py (601 lines) mixes content negotiation, XML conversion, parameter parsing and view base classes; worth splitting.

4. Duplication

• Same parsers, different behaviour:
  • coding_of splits with rpartition('|') in ops/concepts.py:40 but partition('|') in ops/translate.py:196, so the same system|code token can parse differently in $lookup and $translate.
  • CodeableConcept: $translate silently takes the first coding; lookup/validate reject anything but exactly one.
  • flag strips whitespace and boolean doesn't; error messages differ.
• Three too-costly factories: too_costly() in expand.py:101 and translate.py:152, search_too_costly() in search.py:597. One factory taking a message would do.
• Access rules re-implemented: owner_access (resolve.py:320) and may_view_private (resolve.py:283) each re-derive ConceptContainerModel.has_parent_edit_access, differently within the same module; is_public (resolve.py:262) duplicates public_can_view. That's a second permission model that can drift from the main one.
• A 1000 limit is defined five times (CONCEPT_LIMIT, MATCH_LIMIT, MAPPING_LIMIT, REFERENCE_LIMIT, expand.MAX_COUNT), while search.MAX_COUNT reuses the same name for 100. Suggest settings with distinct names.
• Several OCL path ↔ FHIR URL converters: IdentifierSerializer.convert_*, resolve.to_ocl_path, resolve.ocl_path, translate.OCL_PATH + repository_paths.
• CapabilityStatementView / VersionsView routes copy-pasted across orgs/urls.py, users/urls.py and fhir/urls.py.

5. Complex logic

• resolve_reference_expression / resolve_repo (core/common/models.py:1542,1624) gain four FHIR-only parameters (repo_type, allow_head, heads, head_shown) with too-many-arguments,too-many-locals suppressed, and find_repo_by_canonical_url(repo_type=...) switches to different semantics under the flag. A resolution-policy object, or a FHIR subclass of the resolver, would keep the shared $resolveReference path free of FHIR rules.
• shared_resolution's head_shown closure appends to an outer heads list to pass the picked HEAD back out (resolve.py:122).
• resolve() (resolve.py:83) layers a pipe/path version conflict, a newest-public-release fallback, a holder's pick / held version, and cached rights copied between instances. Hard to follow.
• with_side / shown_as in translate.py build **{f'{side}_…'} dynamic Q and annotate expressions that are hard to review.

6. Feature envy

• repository_access, is_visible, has_release, may_view_private, latest_public_release reach into and write to model instances, stamping ad-hoc attributes (repository_public_access, repository_canonical_url, fhir_may_view_private, is_fqdn, resolution_url; plus fhir_concepts, concepts_over_limit in search.py, and core/common/models.py:1637). That logic belongs on ConceptContainerModel or a small wrapper class, not monkey-patched attributes.
• ConceptMapGroupField.to_representation pulls request and view.kwargs from the serializer context and calls with_side, side_system, relationship and namespace_of from the ops/resolve modules.
• Inverted layering: core/common/swagger.py, core/middlewares and the domain serializers now import from core.fhir, which imports them back. cyclic-import is disabled in .pylintrc, so pylint won't flag it.

Other

• FhirOperationView.get_throttles (core/fhir/base.py:593) and CapabilityStatementView.get_throttles (core/fhir/views.py:19) copy a throttling pattern tracked as OpenConceptLab/ocl_online#532 debt. Please use a throttle that's the same for everyone (server protection) or wait for the #532 checkpoint.
• Turning every 5xx on a FHIR path into a 422 hides real server errors from anything that counts 5xx; monitoring/alerting may need adjusting.