[filiperochalopes]

PR Notes — fix/bulk-import-mappings-url-update-if-exists

TL;DR: Bulk import with update_if_exists=true now updates a mapping that has an explicit ID when its target changes, instead of failing with Mapping ID must be unique within a source.

Closes OpenConceptLab/ocl_issues#3023

---

Root cause

1. MappingImporter.get_queryset (core/importers/models.py) matched an existing mapping on the ID and map_type, from/to concept and to_source_url. When the import changed the target, the existing mapping was not found, the importer took the create path, and Mapping.persist_new rejected the duplicate ID.
2. Once (1) is fixed, a second defect shows up for mappings whose target was resolved: Mapping.populate_fields_from_relations kept the previously linked concept when only to_source_url (or from_source_url) changed, so the new version pointed at a concept of the old repo. The issue lists "resolved targets" as untested.

Main Changes

Area	Files	Description
Importer identity	core/importers/models.py	With an explicit id, get_queryset matches by owning source + ID only. Without an id, the existing relationship-based match is unchanged.
Relation resolution	core/mappings/models.py	A linked concept is dropped (and re-resolved through the new source URL + code) when the data points that end of the mapping to another repo. Applies to to and from.
Tests	core/importers/tests.py	5 regression tests in BulkImportInlineTest.

Key Decisions

Decision	Choice	Rationale
Identity contract	Explicit ID within the owning source identifies the mapping	Proposed in the issue; matches how ConceptImporter identifies concepts. Changing target, map type or from-concept of an ID'd mapping now versions it.
Scope of the relation fix	Both ends (to and from)	Same defect class; one small helper in populate_fields_from_relations.
Shared get_queryset	Also used by DELETE lines	A delete line with an ID now retires the mapping with that ID regardless of the other fields, consistent with the new identity rule.
Alternative rejected	Returning an explicit error when the target changes	The issue allows it only "if target changes are intentionally disallowed"; versioning is what update_if_exists promises.

Verification

docker exec oclapi2-api-1 python manage.py test core.importers core.mappings --keepdb
docker exec oclapi2-api-1 python manage.py test core.integration_tests.tests_mappings core.integration_tests.tests_concepts --keepdb
docker exec oclapi2-api-1 pylint -j2 core/importers/models.py core/importers/tests.py core/mappings/models.py

• RED before the fix: the 3 target-change tests failed with failed=1 (Mapping ID must be unique within a source.); the 2 control tests passed
• Unresolved target (to_source_url Old -> New): 1 updated, 0 failed, one HEAD mapping, 2 versions, previous version keeps the old URL, extras/external_id/to_concept_code preserved
• Resolved target via to_concept_url to another repo: relinked, to_source_id updated
• Resolved target via to_source_url + to_concept_code: relinked (failed before the populate_fields_from_relations change)
• Same ID in another source is not matched (creates a new mapping)
• Without update_if_exists the duplicate error is unchanged
• core.importers + core.mappings: 317 tests, 1 failure (below)
• core.integration_tests.tests_mappings + tests_concepts: 226 tests OK
• pylint 10.00/10, git diff --check clean
• Not tested: HTTP bulk-import route end to end, production, ES indexing (disabled/mocked in tests)

Baseline failure (not caused by this PR): BulkImportLimitsTest.test_grandfathered_npm_url_is_downloaded_and_handed_over_as_upload fails identically on master in the local container (storage backend env), verified by stashing the change and re-running it.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-11T15:06:33.923263Z	3f47a66	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f47a662f5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[filiperochalopes]

Follow-up on the Codex review (head 77d2b618).

Item	Status	Commit
P1 re-resolve a linked target when its code changes	Done, thread resolved	c0b5db6
P2 replace copied source versions when repointing	Done, thread resolved	c0b5db6
P2 refresh the old from concept when a mapping is moved	Done, thread resolved	c0b5db6
Tests failure: test_post_expression_cascade_and_transform_to_extensional_generate_multiple_references	Done	77d2b61

The CI failure was unrelated to the importer: the test sorted references as strings, so it broke when the factory's concept counter crossed 999 -> 1000. The new tests in this PR shifted that counter into the test. It now looks references up by URI.

Each finding got a regression test that failed before the fix. Local run: 711 tests across core.importers, core.mappings and the mappings/concepts/collections integration suites, only BulkImportLimitsTest.test_grandfathered_npm_url... fails, and it fails the same on master locally (storage backend in the container; it passes in CI). CI on 77d2b618: Tests, Pylint and Snyk pass.

Not covered: HTTP bulk-import route end to end, and real Elasticsearch indexing (mocked in the tests).

[snyaggarwal]

Review: correctness and happy-path impact

Verdict: Request changes. The importer identity fix is right and works on real data (one P2 blocks). Changing a target by ID now versions the mapping. CIEL and LOINC replays behave exactly like master. But keep_linked_concept also changes populate_fields_from_relations for everything else that calls it, and it breaks cloned mappings on a normal edit (P2 below).

How I checked it

Real local data (CIEL with 324k mappings, two LOINC repos, consumer sources cloned from CIEL). I replayed the same lines through BulkImportInline(update_if_exists=True) and through PUT on master and on this branch, each inside a rolled-back transaction, and diffed the HEAD mapping fields after every line.

Scenario	Lines	master	this PR
CIEL mappings replayed as-is, to_source_url + to_concept_code	511	511 unchanged	511 unchanged, same fields
Same, to_concept_url form	511	510 unchanged, 1 failed (dup ID)	511 unchanged
CIEL lines with an edited extras	511	484 updated / 27 unchanged	identical, no link changes
LOINC 2.82 mappings.json sample (no ID, relationship path)	400	400 unchanged	400 unchanged
CIEL→LOINC mapping repointed to the other LOINC repo, same code	20	20 failed (dup ID)	20 updated, relinked to the new repo's concept, to_source_id updated ✅
DELETE line with a non-existent ID matching another mapping's target	5	not_found	not_found (sibling not retired) ✅
Cloned mappings, line with to_source_url + to_concept_code and an edited field	75	75 updated, local link kept	75 updated, to_concept_id dropped ❌
Same cloned mappings via PUT (to_source_url + to_concept_code)	10	200, local link kept	200, to_concept_id dropped ❌

---

P2: keep_linked_concept drops deliberate local links on cloned mappings

core/mappings/models.py (keep_linked_concept, used for both ends)

Source.clone_mappings (core/sources/models.py, used by clone/cascade-add) links a cloned mapping's to_concept (and from_concept) to the local copy in the consumer source. It keeps to_source_url pointing at the origin, e.g. /orgs/CIEL/sources/CIEL/. So "linked concept's parent ≠ to_source_url" is a normal, intended state. Locally, 3,849 HEAD mappings are in this state on the to side and 3,277 on the from side, mostly MSF, PIH and Kenya_SMART_Guidelines/CIEL.

The new check compares the linked concept's parent with the URL in the data. Any update that sends the mapping's own to_source_url + to_concept_code (no to_concept_url) therefore throws away the local link and re-resolves against the origin:

• where the origin has the code, the mapping is silently repointed from the local copy to CIEL's concept;
• where it doesn't (as with my local CIEL subset), to_concept_id becomes NULL.

On master both keep the local link. A line with nothing new is safe: the standard checksum ignores to_concept_id, so it comes back unchanged and nothing is saved. Any real edit in the same line (extras, map type, retired, comment) saves the new link. The oclweb3 mapping form isn't affected because it sends to_concept_url. Bulk/CSV-style lines and API clients that send source + code are affected.

Suggested fix: decide "the target moved" by comparing what the request asks for with what the mapping had (previous_to_source_url / previous to_concept_code), not with the linked concept's parent. That keeps clone links when the target is unchanged and still covers the repoint cases in this PR. Rough shape:

def keep_linked_concept(concept, requested_source_url, resolved_source_url, previous_source_url,
                        requested_code, previous_code):
    if not concept:
        return None
    if requested_source_url and separate_version(resolved_source_url)[1] != previous_source_url:
        return None
    if requested_code and requested_code != previous_code:
        return None
    return concept

Please add a regression test that clones a concept + mapping with Source.clone_mappings, then updates the clone (import line with to_source_url + to_concept_code + an extras change, and a PUT) and asserts to_concept_id still points at the local copy.

P3: to_source_version / from_source_version reset compares URL strings, not sources

core/mappings/models.py (if previous_to_source_url and to_source_url != previous_to_source_url)

The stored to_source_url can be a canonical or unresolved spelling (e.g. https://cielterminology.org/ with a pinned version) from when the target couldn't be resolved. Once that same source resolves, get_source rewrites it to the relative URI. The strings then differ and the pinned version is cleared, although the target repo hasn't changed. Locally, a few HEAD mappings store such a canonical URL with a version. I found this by reading the code; it isn't one of the replay scenarios. Suggest resetting only when the resolved source actually changes, e.g. compare to_source_id before and after when both are known, and fall back to the URL only when neither resolves.

P4: a canonical to_source_url now downgrades a resolved, ID'd mapping to unresolved

With the line's to_source_url set to http://loinc.org for 10 resolved CIEL→LOINC mappings: master fails them (dup ID). The PR updates them, and to_concept_id/to_source_id become NULL with to_source_url = http://loinc.org. That's because 10 local repos share that canonical, so it doesn't resolve. This matches what a new mapping would get, so not blocking. But a line that used to fail loudly now quietly unlinks a resolved mapping. Worth a line in the PR notes, or a test that pins the intended behaviour.

P5 / nits

• core/importers/models.py get_queryset: the ID early return sits after the from_concept_url / to_concept_url / to_source_url locals, which it doesn't use. Move it to the top of the method.
• core/importers/models.py process: replaced_from_concept_id is set on every same-source update, even when the from concept doesn't change. It's harmless because the set dedupes. Setting it only when from_concept_id actually changed after create_new_version_for would make the intent clearer.
• The PUT / Concept mappings-payload path (create_new_version_for outside the importer) still doesn't refresh the old from concept's mapped codes when a mapping moves. It wasn't touched here and isn't required, but it's the same class of staleness the importer now handles.

Checks

• pylint -j0 core/ (as in CI): exit 0, 10.00/10.
• core.importers + core.mappings: 320 tests, 1 failure in an unrelated, pre-existing importer limits test (fails locally for environment reasons).
• Integration suites: tests_mappings/tests_concepts/tests_collections/tests_sources ran, but the local Elasticsearch container went down mid-run. The ES-dependent setUpClass errors happen identically on master, so they're inconclusive locally rather than a regression; CI is the reference there.

[filiperochalopes]

Thanks @snyaggarwal, the clone case was a real regression in my keep_linked_concept. Fixed in e1ac55f4.

Item	Status	What changed
P2 cloned mappings lose their local link	Done	A target now counts as moved only when the requested URL or code differs from what the mapping already had (not from the linked concept's parent). I did not compare to_source_id: clone_mappings stores the local source id while to_source_url stays on the origin, so it would flag every clone as moved. Regression tests: test_mapping_import_with_id_keeps_the_local_link_of_a_cloned_mapping (import line with to_source_url + to_concept_code + an extras change) and test_put_200_keeps_the_local_link_of_a_cloned_mapping (PUT). Both fail on the previous commit and pass now. The from end uses the same helper; the tests only exercise the to end.
P3 version reset compares URL strings	Done	Repos are compared by their resolved versionless URL, looked up only when the spelling differs. Regression test test_mapping_import_with_id_keeps_the_pinned_version_when_the_canonical_url_now_resolves stores an unresolved canonical URL with a pinned version, adds the URL registry entry, re-imports, and checks the version is kept. It fails on the previous commit.
P4 canonical to_source_url unlinks a resolved mapping	Left as is, documented	Matches what a new mapping with that URL gets, and the line used to fail loudly. Not pinned with a test, since it is more a limitation than a contract.
P5 early return in get_queryset	Done	Moved to the top of the method.
P5 replaced_from_concept_id on every update	Done	Only recorded when from_concept_id actually changed after create_new_version_for.
P5 PUT/concept-payload path does not refresh the old from concept	Out of scope	Same class of staleness, not touched here. Happy to follow up in a separate issue.

Checks run locally in a throwaway container with the API image (not the long-running API container, which was stopped): 1124 tests across core.importers, core.mappings, core.sources and the mappings/concepts/sources/collections integration suites, 1 failure (BulkImportLimitsTest.test_grandfathered_npm_url_is_downloaded_and_handed_over_as_upload, same as on master locally). pylint on the changed files: 10.00/10. CI on the new head is still pending.