From 3f47a662f5a0e687249053e844f01928ec07412f Mon Sep 17 00:00:00 2001 From: Filipe Lopes Date: Wed, 11 Nov 2026 11:58:00 -0300 Subject: [PATCH 1/5] OpenConceptLab/ocl_issues#3023 | Bulk import with update_if_exists identifies a mapping by its ID, so changing the target versions it instead of failing as a duplicate MappingImporter.get_queryset also filtered on map_type, from/to concept and to_source_url. A changed target hid the existing mapping, the import took the create path and failed with "Mapping ID must be unique within a source.". An explicit ID now matches the mapping inside its source only. Mapping.populate_fields_from_relations kept the previously linked concept when only to_source_url/from_source_url changed, leaving a mapping whose concept belonged to the old repo. It now relinks through the new source URL and code. --- core/importers/models.py | 11 +++- core/importers/tests.py | 134 +++++++++++++++++++++++++++++++++++++++ core/mappings/models.py | 20 ++++-- 3 files changed, 158 insertions(+), 7 deletions(-) diff --git a/core/importers/models.py b/core/importers/models.py index 64c06631e..49993a6e2 100644 --- a/core/importers/models.py +++ b/core/importers/models.py @@ -579,6 +579,14 @@ def get_queryset(self): # pylint: disable=too-many-branches from_concept_code = self.get('from_concept_code') to_source_url = self.get('to_source_url') parent_uri = f'/{"users" if self.is_user_owner() else "orgs"}/{self.get("owner")}/sources/{self.get("source")}/' + if self.get('id'): + # An explicit ID identifies the mapping within its source. Matching on its relationship fields too would + # hide the mapping whenever the import changes them (e.g. the target), and the import would then try to + # create a duplicate ID instead of versioning the existing mapping. + self.queryset = Mapping.objects.filter( + parent__uri=parent_uri, id=F('versioned_object_id'), mnemonic=self.get('id')) + return self.queryset + filters = { 'parent__uri': parent_uri, 'id': F('versioned_object_id'), @@ -607,9 +615,6 @@ def get_queryset(self): # pylint: disable=too-many-branches if self.get_cached_source_exists_by_uri(drop_version(to_source_uri)): filters['to_source__uri'] = to_source_uri - if self.get('id'): - filters['mnemonic'] = self.get('id') - if to_source_url: to_source_uri = drop_version(to_source_url) filters['to_source_url'] = to_source_uri diff --git a/core/importers/tests.py b/core/importers/tests.py index 28b070362..d908355f9 100644 --- a/core/importers/tests.py +++ b/core/importers/tests.py @@ -1206,6 +1206,140 @@ def test_mapping_import_cache_stale_after_concept_created_in_same_run(self, batc # BulkImportInline.cache rather than treating the new behavior as a regression. self.assertIsNone(same_as_sugar.to_concept_id) + @staticmethod + def external_target_mapping_line(source, **extra): + """Mapping line with an explicit ID whose target is an external (unresolved) source.""" + return { + 'type': 'Mapping', 'id': 'fixture-map', 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': source.organization.mnemonic, 'source': source.mnemonic, + 'from_concept_url': source.uri + 'concepts/From/', 'to_source_url': '/orgs/External/sources/Old/', + 'to_concept_code': 'A01', **extra + } + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_updates_when_to_source_url_changes(self, batch_index_resources_mock): + """An explicit mapping ID identifies the mapping, so changing its target source versions it (no duplicate).""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='TargetChangeOrg')) + ConceptFactory(parent=source, mnemonic='From') + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.external_target_mapping_line(source, extras={'keep': 'me'}, external_id='ext-1')]).run() + mapping = Mapping.objects.get(parent=source, mnemonic='fixture-map', id=F('versioned_object_id')) + self.assertEqual(mapping.to_source_url, '/orgs/External/sources/Old/') + self.assertEqual(mapping.versions.count(), 1) + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.external_target_mapping_line(source, to_source_url='/orgs/External/sources/New/')]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + self.assertEqual( + Mapping.objects.filter(parent=source, mnemonic='fixture-map', id=F('versioned_object_id')).count(), 1) + mapping.refresh_from_db() + self.assertEqual(mapping.to_source_url, '/orgs/External/sources/New/') + self.assertEqual(mapping.to_concept_code, 'A01') + self.assertEqual(mapping.extras, {'keep': 'me'}) + self.assertEqual(mapping.external_id, 'ext-1') + self.assertEqual(mapping.versions.count(), 2) + latest_version = mapping.get_latest_version() + self.assertEqual(latest_version.to_source_url, '/orgs/External/sources/New/') + self.assertEqual(mapping.versions.exclude(id=latest_version.id).get().to_source_url, + '/orgs/External/sources/Old/') + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_repoints_a_resolved_target_to_another_source(self, batch_index_resources_mock): + """Changing the target to a concept of another repo relinks the mapping instead of failing as a duplicate.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='RepointOrg'), mnemonic='Main') + other_source = OrganizationSourceFactory(organization=source.organization, mnemonic='Other') + ConceptFactory(parent=source, mnemonic='From') + old_target = ConceptFactory(parent=source, mnemonic='Target') + new_target = ConceptFactory(parent=other_source, mnemonic='Target') + line = { + 'type': 'Mapping', 'id': 'repoint-map', 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': source.organization.mnemonic, 'source': source.mnemonic, + 'from_concept_url': source.uri + 'concepts/From/', 'to_concept_url': source.uri + 'concepts/Target/' + } + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[line]).run() + mapping = Mapping.objects.get(parent=source, mnemonic='repoint-map', id=F('versioned_object_id')) + self.assertEqual(mapping.to_concept_id, old_target.versioned_object_id) + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + {**line, 'to_concept_url': other_source.uri + 'concepts/Target/'}]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + mapping.refresh_from_db() + self.assertEqual(mapping.to_concept_id, new_target.versioned_object_id) + self.assertEqual(mapping.to_source_id, other_source.id) + self.assertEqual(mapping.versions.count(), 2) + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_repoints_a_resolved_target_by_source_url_and_code( + self, batch_index_resources_mock): + """A new to_source_url plus to_concept_code must not keep the previously linked target concept.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='RepointCodeOrg'), mnemonic='Main') + other_source = OrganizationSourceFactory(organization=source.organization, mnemonic='Other') + ConceptFactory(parent=source, mnemonic='From') + ConceptFactory(parent=source, mnemonic='Target') + new_target = ConceptFactory(parent=other_source, mnemonic='Target') + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[{ + 'type': 'Mapping', 'id': 'repoint-code-map', 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': source.organization.mnemonic, 'source': source.mnemonic, + 'from_concept_url': source.uri + 'concepts/From/', 'to_concept_url': source.uri + 'concepts/Target/' + }]).run() + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[{ + 'type': 'Mapping', 'id': 'repoint-code-map', 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': source.organization.mnemonic, 'source': source.mnemonic, + 'from_concept_url': source.uri + 'concepts/From/', 'to_source_url': other_source.uri, + 'to_concept_code': 'Target' + }]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + mapping = Mapping.objects.get(parent=source, mnemonic='repoint-code-map', id=F('versioned_object_id')) + self.assertEqual(mapping.to_source_id, other_source.id) + self.assertEqual(mapping.to_concept_id, new_target.versioned_object_id) + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_in_another_source_is_not_matched(self, batch_index_resources_mock): + """The ID identifies a mapping only inside its own source: the same ID elsewhere is a new mapping.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='IdScopeOrg'), mnemonic='One') + other_source = OrganizationSourceFactory(organization=source.organization, mnemonic='Two') + ConceptFactory(parent=source, mnemonic='From') + ConceptFactory(parent=other_source, mnemonic='From') + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.external_target_mapping_line(source)]).run() + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.external_target_mapping_line(other_source, to_source_url='/orgs/External/sources/New/')]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (1, 0, 0)) + self.assertEqual(Mapping.objects.get( + parent=source, mnemonic='fixture-map', id=F('versioned_object_id')).to_source_url, + '/orgs/External/sources/Old/') + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_without_update_if_exists_still_reports_duplicate( + self, batch_index_resources_mock): + """Without update_if_exists the explicit ID keeps failing as already existing.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='NoUpdateOrg')) + ConceptFactory(parent=source, mnemonic='From') + BulkImportInline(content=None, username='ocladmin', update_if_exists=False, input_list=[ + self.external_target_mapping_line(source)]).run() + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=False, input_list=[ + self.external_target_mapping_line(source, to_source_url='/orgs/External/sources/New/')]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 0, 1)) + self.assertEqual(importer.failed[0]['errors'], {'__all__': ['Mapping ID must be unique within a source.']}) + @patch('core.importers.models.batch_index_resources') def test_mapping_import_with_autoid_assignment(self, batch_index_resources_mock): self.assertEqual(Mapping.objects.count(), 0) diff --git a/core/mappings/models.py b/core/mappings/models.py index f96b46a26..d9d3c9229 100644 --- a/core/mappings/models.py +++ b/core/mappings/models.py @@ -378,23 +378,35 @@ def get_source(url): self.from_source, self.from_source_url = get_source(from_source_url) self.to_source, self.to_source_url = get_source(to_source_url) + def keep_linked_concept(concept, requested_source_url, resolved_source_url): + """Already linked concept, dropped when the data points that end of the mapping to another repo.""" + if concept and requested_source_url and get(concept, 'parent.uri') != separate_version( + resolved_source_url)[1]: + return None + return concept + + linked_from_concept = keep_linked_concept( + get(self, 'from_concept'), data.get('from_source_url'), self.from_source_url) + linked_to_concept = keep_linked_concept( + get(self, 'to_concept'), data.get('to_source_url'), self.to_source_url) + if to_concept_code and not is_url_encoded_string(to_concept_code): to_concept_code = encode_string(to_concept_code) if from_concept_code and not is_url_encoded_string(from_concept_code): from_concept_code = encode_string(from_concept_code) - if not to_concept_url and not get(self, 'to_concept') and self.to_source_url and ( + if not to_concept_url and not linked_to_concept and self.to_source_url and ( to_concept_code or self.to_concept_code): to_concept_code = to_concept_code or self.to_concept_code to_concept_url = self.to_source_url + 'concepts/' + to_concept_code + '/' - if not from_concept_url and not get(self, 'from_concept') and self.from_source_url and ( + if not from_concept_url and not linked_from_concept and self.from_source_url and ( from_concept_code or self.from_concept_code): from_concept_code = from_concept_code or self.from_concept_code from_concept_url = self.from_source_url + 'concepts/' + from_concept_code + '/' - from_concept = get_concept(from_concept_url) if from_concept_url else get(self, 'from_concept') - to_concept = get_concept(to_concept_url) if to_concept_url else get(self, 'to_concept') + from_concept = get_concept(from_concept_url) if from_concept_url else linked_from_concept + to_concept = get_concept(to_concept_url) if to_concept_url else linked_to_concept self.from_concept_id = get(from_concept, 'id') self.to_concept_id = get(to_concept, 'id') From c0b5db6e0e99cf58ed7af3f993f0954f3c1e12da Mon Sep 17 00:00:00 2001 From: Filipe Lopes Date: Wed, 11 Nov 2026 12:34:50 -0300 Subject: [PATCH 2/5] OpenConceptLab/ocl_issues#3023 | Repointing a mapping by ID relinks the target on a code change, drops the previous repo's source version and refreshes the old from concept Review findings on the first commit: - changing only to_concept_code (same to_source_url) kept the previously linked concept; the link is now re-resolved when the requested code differs - to_source_version/from_source_version recorded for the previous repo were carried over to the new one; they are now reset when the source changes - a mapping moved to another from concept left the old one's mapped codes stale in the index; the importer now refreshes it too --- core/importers/models.py | 5 +++ core/importers/tests.py | 69 ++++++++++++++++++++++++++++++++++++++++ core/mappings/models.py | 30 ++++++++++------- 3 files changed, 93 insertions(+), 11 deletions(-) diff --git a/core/importers/models.py b/core/importers/models.py index 49993a6e2..94f153043 100644 --- a/core/importers/models.py +++ b/core/importers/models.py @@ -668,6 +668,9 @@ def process(self): self.instance = queryset.first().clone() self.instance._counted = None # pylint: disable=protected-access self.instance._index = False # pylint: disable=protected-access + if self.instance.from_concept_id and self.instance.from_source_id == self.instance.parent_id: + # the update can move the mapping to another from concept, whose mapped codes then need a refresh + self.instance.replaced_from_concept_id = self.instance.from_concept_id errors = Mapping.create_new_version_for(self.instance, self.data, self.user, cache=self.cache) if errors and Mapping.is_standard_checksum_error(errors): return UNCHANGED @@ -826,6 +829,8 @@ def add(self, instance): ])) if resource == 'mapping' and instance.from_concept_id and instance.from_source_id == instance.parent_id: self.mapped_concept_ids.add(instance.from_concept_id) + if resource == 'mapping' and get(instance, 'replaced_from_concept_id'): + self.mapped_concept_ids.add(instance.replaced_from_concept_id) # the mapping no longer starts there def finish(self): # every row first, so that a failure to queue can't leave any deferred diff --git a/core/importers/tests.py b/core/importers/tests.py index d908355f9..1ba858358 100644 --- a/core/importers/tests.py +++ b/core/importers/tests.py @@ -1340,6 +1340,75 @@ def test_mapping_import_with_id_without_update_if_exists_still_reports_duplicate self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 0, 1)) self.assertEqual(importer.failed[0]['errors'], {'__all__': ['Mapping ID must be unique within a source.']}) + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_relinks_the_target_when_only_its_code_changes(self, batch_index_resources_mock): + """Same to_source_url, new to_concept_code: the mapping follows the new code, not the previously linked one.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='CodeChangeOrg')) + ConceptFactory(parent=source, mnemonic='From') + ConceptFactory(parent=source, mnemonic='CodeA') + code_b = ConceptFactory(parent=source, mnemonic='CodeB') + line = { + 'type': 'Mapping', 'id': 'code-map', 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': source.organization.mnemonic, 'source': source.mnemonic, + 'from_concept_url': source.uri + 'concepts/From/', 'to_source_url': source.uri, 'to_concept_code': 'CodeA' + } + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[line]).run() + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + {**line, 'to_concept_code': 'CodeB'}]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + mapping = Mapping.objects.get(parent=source, mnemonic='code-map', id=F('versioned_object_id')) + self.assertEqual(mapping.to_concept_code, 'CodeB') + self.assertEqual(mapping.to_concept_id, code_b.versioned_object_id) + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_replaces_the_source_version_of_the_previous_target( + self, batch_index_resources_mock): + """A source version recorded for the previous target repo is not carried over to the new one.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='VersionChangeOrg')) + ConceptFactory(parent=source, mnemonic='From') + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.external_target_mapping_line(source, to_source_url='/orgs/External/sources/Old/v1/')]).run() + mapping = Mapping.objects.get(parent=source, mnemonic='fixture-map', id=F('versioned_object_id')) + self.assertEqual((mapping.to_source_url, mapping.to_source_version), ('/orgs/External/sources/Old/', 'v1')) + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.external_target_mapping_line(source, to_source_url='/orgs/External/sources/New/v2/')]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + mapping.refresh_from_db() + self.assertEqual((mapping.to_source_url, mapping.to_source_version), ('/orgs/External/sources/New/', 'v2')) + + @patch('core.importers.models.index_concepts_mapped_codes') + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_moved_to_another_from_concept_refreshes_the_previous_one( + self, _, index_concepts_mapped_codes_mock): + """The concept a mapping moved away from stops advertising it, so its mapped codes are refreshed too.""" + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='MovedFromOrg')) + old_from = ConceptFactory(parent=source, mnemonic='OldFrom') + new_from = ConceptFactory(parent=source, mnemonic='NewFrom') + ConceptFactory(parent=source, mnemonic='To') + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.mapping_line(source, 'OldFrom', 'To', id='moved-map')]).run() + index_concepts_mapped_codes_mock.reset_mock() + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + self.mapping_line(source, 'NewFrom', 'To', id='moved-map')]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + mapping = Mapping.objects.get(parent=source, mnemonic='moved-map', id=F('versioned_object_id')) + self.assertEqual(mapping.from_concept_id, new_from.versioned_object_id) + self.assertEqual( + self.queued_mapped_code_ids(index_concepts_mapped_codes_mock) & { + old_from.versioned_object_id, new_from.versioned_object_id}, + {old_from.versioned_object_id, new_from.versioned_object_id}) + @patch('core.importers.models.batch_index_resources') def test_mapping_import_with_autoid_assignment(self, batch_index_resources_mock): self.assertEqual(Mapping.objects.count(), 0) diff --git a/core/mappings/models.py b/core/mappings/models.py index d9d3c9229..9603c906d 100644 --- a/core/mappings/models.py +++ b/core/mappings/models.py @@ -375,25 +375,29 @@ def get_source(url): return source, source.versioned_object_url or source.resolution_url or url return None, url + previous_from_source_url, previous_to_source_url = self.from_source_url, self.to_source_url self.from_source, self.from_source_url = get_source(from_source_url) self.to_source, self.to_source_url = get_source(to_source_url) - def keep_linked_concept(concept, requested_source_url, resolved_source_url): - """Already linked concept, dropped when the data points that end of the mapping to another repo.""" - if concept and requested_source_url and get(concept, 'parent.uri') != separate_version( - resolved_source_url)[1]: + if to_concept_code and not is_url_encoded_string(to_concept_code): + to_concept_code = encode_string(to_concept_code) + if from_concept_code and not is_url_encoded_string(from_concept_code): + from_concept_code = encode_string(from_concept_code) + + def keep_linked_concept(concept, requested_source_url, resolved_source_url, requested_code): + """Already linked concept, dropped when the data points that end of the mapping to another repo or code.""" + if not concept: + return None + if requested_source_url and get(concept, 'parent.uri') != separate_version(resolved_source_url)[1]: + return None + if requested_code and get(concept, 'mnemonic') != requested_code: return None return concept linked_from_concept = keep_linked_concept( - get(self, 'from_concept'), data.get('from_source_url'), self.from_source_url) + get(self, 'from_concept'), data.get('from_source_url'), self.from_source_url, from_concept_code) linked_to_concept = keep_linked_concept( - get(self, 'to_concept'), data.get('to_source_url'), self.to_source_url) - - if to_concept_code and not is_url_encoded_string(to_concept_code): - to_concept_code = encode_string(to_concept_code) - if from_concept_code and not is_url_encoded_string(from_concept_code): - from_concept_code = encode_string(from_concept_code) + get(self, 'to_concept'), data.get('to_source_url'), self.to_source_url, to_concept_code) if not to_concept_url and not linked_to_concept and self.to_source_url and ( to_concept_code or self.to_concept_code): @@ -422,10 +426,14 @@ def keep_linked_concept(concept, requested_source_url, resolved_source_url): self.to_concept_name = data.get('to_concept_name', None) or self.to_concept_name if self.to_source_url: to_source_version, to_source_url = separate_version(self.to_source_url) + if previous_to_source_url and to_source_url != previous_to_source_url: + self.to_source_version = None # a version carried over from the previous repo doesn't apply self.to_source_version = self.to_source_version or to_source_version self.to_source_url = to_source_url if self.from_source_url: from_source_version, from_source_url = separate_version(self.from_source_url) + if previous_from_source_url and from_source_url != previous_from_source_url: + self.from_source_version = None # a version carried over from the previous repo doesn't apply self.from_source_version = self.from_source_version or from_source_version self.from_source_url = from_source_url From 77d2b6189e6a5111d7043a4e16e84fd0cb12ce9f Mon Sep 17 00:00:00 2001 From: Filipe Lopes Date: Wed, 11 Nov 2026 12:34:50 -0300 Subject: [PATCH 3/5] test(collections): make the cascade preview test independent of the factory sequence The test sorted references as strings, so it failed whenever the concept mnemonic counter crossed from 999 to 1000 between its two concepts. Look the expected references up by URI instead. --- core/integration_tests/tests_collections.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/integration_tests/tests_collections.py b/core/integration_tests/tests_collections.py index 3c213875e..d1fd3cc85 100644 --- a/core/integration_tests/tests_collections.py +++ b/core/integration_tests/tests_collections.py @@ -2391,9 +2391,10 @@ def test_post_expression_cascade_and_transform_to_extensional_generate_multiple_ self.assertEqual(response.status_code, 200) self.assertEqual(len(response.data), 3) - results = sorted(response.data, key=lambda ref: ref['reference']) + # keyed by reference: sorting the URIs as strings depends on the factory sequence (concept1000 < concept999) + results = {ref['reference']: ref for ref in response.data} self.assertEqual( - results[0], + results[concept2.uri], { 'reference': concept2.uri, 'concepts': [ConceptListSerializer(concept2).data], @@ -2404,7 +2405,7 @@ def test_post_expression_cascade_and_transform_to_extensional_generate_multiple_ } ) self.assertEqual( - results[1], + results[concept3.uri], { 'reference': concept3.uri, 'concepts': [ConceptListSerializer(concept3).data], @@ -2415,7 +2416,7 @@ def test_post_expression_cascade_and_transform_to_extensional_generate_multiple_ } ) self.assertEqual( - results[2], + results[mapping.uri], { 'reference': mapping.uri, 'concepts': [], From 552fb69266054832fd90240b9d4b58b1593edc9c Mon Sep 17 00:00:00 2001 From: Filipe Lopes Date: Wed, 11 Nov 2026 17:19:12 -0300 Subject: [PATCH 4/5] refactor(importers): drop redundant comment on the explicit-ID mapping lookup --- core/importers/models.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/importers/models.py b/core/importers/models.py index 94f153043..3be1f5ef4 100644 --- a/core/importers/models.py +++ b/core/importers/models.py @@ -580,9 +580,6 @@ def get_queryset(self): # pylint: disable=too-many-branches to_source_url = self.get('to_source_url') parent_uri = f'/{"users" if self.is_user_owner() else "orgs"}/{self.get("owner")}/sources/{self.get("source")}/' if self.get('id'): - # An explicit ID identifies the mapping within its source. Matching on its relationship fields too would - # hide the mapping whenever the import changes them (e.g. the target), and the import would then try to - # create a duplicate ID instead of versioning the existing mapping. self.queryset = Mapping.objects.filter( parent__uri=parent_uri, id=F('versioned_object_id'), mnemonic=self.get('id')) return self.queryset From e1ac55f4a471a1292b1d298be5da65d8b722ef33 Mon Sep 17 00:00:00 2001 From: Filipe Lopes Date: Fri, 13 Nov 2026 22:11:51 -0300 Subject: [PATCH 5/5] OpenConceptLab/ocl_issues#3023 | Keep the local link of cloned mappings when an update repeats their target; compare repos by resolved URL Review findings (snyaggarwal): - P2: the linked concept was dropped whenever its parent differed from the requested to_source_url, which is the normal state of a cloned mapping (the clone links the local concept but keeps the origin URL). A target now counts as moved only when the URL or code differs from what the mapping already had. - P3: the pinned source version was reset by comparing URL spellings. Repos are now compared by their resolved versionless URL, so a canonical URL that starts to resolve to the same repo keeps its version. - P5: the explicit-ID lookup returns before the unused URL locals, and the previous from concept is only recorded when it actually changed. --- core/importers/models.py | 18 +++--- core/importers/tests.py | 76 ++++++++++++++++++++++++ core/integration_tests/tests_mappings.py | 27 +++++++++ core/mappings/models.py | 34 +++++++---- 4 files changed, 134 insertions(+), 21 deletions(-) diff --git a/core/importers/models.py b/core/importers/models.py index 3be1f5ef4..76eda0540 100644 --- a/core/importers/models.py +++ b/core/importers/models.py @@ -573,17 +573,17 @@ def get_queryset(self): # pylint: disable=too-many-branches if self.queryset: return self.queryset - from_concept_url = self.get('from_concept_url') - to_concept_url = self.get('to_concept_url') - to_concept_code = self.get('to_concept_code') - from_concept_code = self.get('from_concept_code') - to_source_url = self.get('to_source_url') parent_uri = f'/{"users" if self.is_user_owner() else "orgs"}/{self.get("owner")}/sources/{self.get("source")}/' if self.get('id'): self.queryset = Mapping.objects.filter( parent__uri=parent_uri, id=F('versioned_object_id'), mnemonic=self.get('id')) return self.queryset + from_concept_url = self.get('from_concept_url') + to_concept_url = self.get('to_concept_url') + to_concept_code = self.get('to_concept_code') + from_concept_code = self.get('from_concept_code') + to_source_url = self.get('to_source_url') filters = { 'parent__uri': parent_uri, 'id': F('versioned_object_id'), @@ -665,10 +665,12 @@ def process(self): self.instance = queryset.first().clone() self.instance._counted = None # pylint: disable=protected-access self.instance._index = False # pylint: disable=protected-access - if self.instance.from_concept_id and self.instance.from_source_id == self.instance.parent_id: - # the update can move the mapping to another from concept, whose mapped codes then need a refresh - self.instance.replaced_from_concept_id = self.instance.from_concept_id + previous_from_concept_id = self.instance.from_concept_id if ( + self.instance.from_source_id == self.instance.parent_id) else None errors = Mapping.create_new_version_for(self.instance, self.data, self.user, cache=self.cache) + if previous_from_concept_id and previous_from_concept_id != self.instance.from_concept_id: + # the mapping moved to another from concept, whose mapped codes then need a refresh + self.instance.replaced_from_concept_id = previous_from_concept_id if errors and Mapping.is_standard_checksum_error(errors): return UNCHANGED return errors or UPDATED diff --git a/core/importers/tests.py b/core/importers/tests.py index 1ba858358..682a17b6e 100644 --- a/core/importers/tests.py +++ b/core/importers/tests.py @@ -50,6 +50,9 @@ from core.sources.constants import AUTO_ID_SEQUENTIAL, AUTO_ID_UUID from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory +from core.toggles.models import Toggle +from core.url_registry.constants import URL_REGISTRY_IN_RESOLVE_REFERENCE_TOGGLE +from core.url_registry.factories import GlobalURLRegistryFactory from core.tasks.models import Task from core.capabilities.constants import IMPORTS_FILE_SIZE_CAPABILITY_ID from core.capabilities.models import UserCapabilityOverride @@ -1409,6 +1412,79 @@ def test_mapping_import_with_id_moved_to_another_from_concept_refreshes_the_prev old_from.versioned_object_id, new_from.versioned_object_id}, {old_from.versioned_object_id, new_from.versioned_object_id}) + def cloned_mapping_in_local_source(self): + """A mapping cloned into a local source: linked to local concepts, with its target URL kept on the origin.""" + organization = OrganizationFactory(mnemonic='CloneOrg') + origin = OrganizationSourceFactory(organization=organization, mnemonic='Origin') + local = OrganizationSourceFactory(organization=organization, mnemonic='Local') + for source in (origin, local): + ConceptFactory(parent=source, mnemonic='From') + origin_to = ConceptFactory(parent=origin, mnemonic='To') + local_to = ConceptFactory(parent=local, mnemonic='To') + user = origin_to.created_by + mapping = MappingFactory.build( + from_concept=ConceptFactory.build(mnemonic='From'), to_concept=ConceptFactory.build(mnemonic='To'), + parent=local, mnemonic='cloned-map', map_type='Same As', to_source_url=origin.uri, + from_source_url=origin.uri) + local.clone_mappings([mapping], user) + mapping = Mapping.objects.get(parent=local, from_concept_code='From', id=F('versioned_object_id')) + self.assertEqual((mapping.to_concept_id, mapping.to_source_url), (local_to.id, origin.uri)) + return origin, local, mapping + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_keeps_the_local_link_of_a_cloned_mapping(self, batch_index_resources_mock): + """Re-sending the unchanged target of a cloned mapping with an edit must not repoint it to the origin.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + origin, local, mapping = self.cloned_mapping_in_local_source() + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[{ + 'type': 'Mapping', 'id': mapping.mnemonic, 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': local.organization.mnemonic, 'source': local.mnemonic, + 'from_concept_url': local.uri + 'concepts/From/', 'to_source_url': origin.uri, 'to_concept_code': 'To', + 'extras': {'edited': 'yes'} + }]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + updated = Mapping.objects.get(parent=local, mnemonic=mapping.mnemonic, id=F('versioned_object_id')) + self.assertEqual(updated.extras, {'edited': 'yes'}) + self.assertEqual(updated.to_concept_id, mapping.to_concept_id) + self.assertEqual(updated.to_source_url, origin.uri) + + @patch('core.importers.models.batch_index_resources') + def test_mapping_import_with_id_keeps_the_pinned_version_when_the_canonical_url_now_resolves( + self, batch_index_resources_mock): + """A source version pinned under a canonical URL survives once that URL resolves to the same repo.""" + batch_index_resources_mock.__name__ = 'batch_index_resources' + Toggle.objects.filter(name=URL_REGISTRY_IN_RESOLVE_REFERENCE_TOGGLE).update( + dev=True, qa=True, demo=True, staging=True, who_staging=True, production=True) + canonical_url = 'https://target.example.test/' + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='CanonicalOrg')) + target = OrganizationSourceFactory( + organization=source.organization, mnemonic='Target', version='HEAD', canonical_url=canonical_url) + ConceptFactory(parent=source, mnemonic='From') + ConceptFactory(parent=target, mnemonic='To') + line = { + 'type': 'Mapping', 'id': 'pinned-map', 'map_type': 'Same As', 'owner_type': 'Organization', + 'owner': source.organization.mnemonic, 'source': source.mnemonic, + 'from_concept_url': source.uri + 'concepts/From/', 'to_source_url': canonical_url, + 'to_concept_code': 'To' + } + BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[line]).run() + Mapping.objects.filter(parent=source, mnemonic='pinned-map').update(to_source_version='v1') + mapping = Mapping.objects.get(parent=source, mnemonic='pinned-map', id=F('versioned_object_id')) + self.assertEqual((mapping.to_source_id, mapping.to_source_url), (None, canonical_url)) + GlobalURLRegistryFactory(url=canonical_url, namespace=target.organization.uri) + + importer = BulkImportInline(content=None, username='ocladmin', update_if_exists=True, input_list=[ + {**line, 'extras': {'edited': 'yes'}}]) + importer.run() + + self.assertEqual((len(importer.created), len(importer.updated), len(importer.failed)), (0, 1, 0)) + mapping.refresh_from_db() + self.assertEqual(mapping.to_source_id, target.id) + self.assertEqual(mapping.to_source_version, 'v1') + @patch('core.importers.models.batch_index_resources') def test_mapping_import_with_autoid_assignment(self, batch_index_resources_mock): self.assertEqual(Mapping.objects.count(), 0) diff --git a/core/integration_tests/tests_mappings.py b/core/integration_tests/tests_mappings.py index 04d9bb849..acfa32f34 100644 --- a/core/integration_tests/tests_mappings.py +++ b/core/integration_tests/tests_mappings.py @@ -1,4 +1,5 @@ from django.core.cache import cache +from django.db.models import F from django.test import override_settings, tag from rest_framework.exceptions import ErrorDetail @@ -702,6 +703,32 @@ def test_put_200_sort_weight(self): self.mapping.refresh_from_db() self.assertEqual(self.mapping.sort_weight, 1.67983) + def test_put_200_keeps_the_local_link_of_a_cloned_mapping(self): + """A cloned mapping keeps its target URL on the origin; sending that URL and code back keeps the local link.""" + origin = OrganizationSourceFactory(mnemonic='CloneOrigin') + ConceptFactory(parent=origin, mnemonic='To') + local_from = ConceptFactory(parent=self.source, mnemonic='From') + local_to = ConceptFactory(parent=self.source, mnemonic='To') + cloned = MappingFactory.build( + from_concept=ConceptFactory.build(mnemonic='From'), to_concept=ConceptFactory.build(mnemonic='To'), + parent=self.source, map_type=SAME_AS, to_source_url=origin.uri) + self.source.clone_mappings([cloned], self.user) + cloned = Mapping.objects.get(parent=self.source, from_concept_code='From', id=F('versioned_object_id')) + self.assertEqual((cloned.from_concept_id, cloned.to_concept_id), (local_from.id, local_to.id)) + + response = self.client.put( + cloned.uri, + {'to_source_url': origin.uri, 'to_concept_code': 'To', 'extras': {'edited': 'yes'}}, + HTTP_AUTHORIZATION='Token ' + self.token, + format='json' + ) + + self.assertEqual(response.status_code, 200) + cloned.refresh_from_db() + self.assertEqual(cloned.extras, {'edited': 'yes'}) + self.assertEqual((cloned.from_concept_id, cloned.to_concept_id), (local_from.id, local_to.id)) + self.assertEqual(cloned.to_source_url, origin.uri) + def test_put_400(self): self.assertEqual(self.mapping.versions.count(), 1) self.assertEqual(self.mapping.get_latest_version().map_type, SAME_AS) diff --git a/core/mappings/models.py b/core/mappings/models.py index 9603c906d..6655d36aa 100644 --- a/core/mappings/models.py +++ b/core/mappings/models.py @@ -376,28 +376,36 @@ def get_source(url): return None, url previous_from_source_url, previous_to_source_url = self.from_source_url, self.to_source_url + previous_from_concept_code, previous_to_concept_code = self.from_concept_code, self.to_concept_code self.from_source, self.from_source_url = get_source(from_source_url) self.to_source, self.to_source_url = get_source(to_source_url) + def has_moved(requested_url, previous_url, resolved_url): + """Whether the data points this end of the mapping to another repo than it already had.""" + if not (requested_url and previous_url): + return False + resolved_url = separate_version(resolved_url)[1] + if resolved_url == previous_url: + return False + # a repo is named by its versionless URL once resolved, so only a differently spelled one is looked up + return resolved_url != separate_version(get_source(previous_url)[1])[1] + + from_moved = has_moved(from_source_url, previous_from_source_url, self.from_source_url) + to_moved = has_moved(to_source_url, previous_to_source_url, self.to_source_url) + if to_concept_code and not is_url_encoded_string(to_concept_code): to_concept_code = encode_string(to_concept_code) if from_concept_code and not is_url_encoded_string(from_concept_code): from_concept_code = encode_string(from_concept_code) - def keep_linked_concept(concept, requested_source_url, resolved_source_url, requested_code): - """Already linked concept, dropped when the data points that end of the mapping to another repo or code.""" - if not concept: - return None - if requested_source_url and get(concept, 'parent.uri') != separate_version(resolved_source_url)[1]: - return None - if requested_code and get(concept, 'mnemonic') != requested_code: - return None - return concept + def keep_linked_concept(concept, moved, requested_code, previous_code): + """The linked concept stays, unless the data moves that end of the mapping to another repo or code.""" + return None if moved or (requested_code and requested_code != previous_code) else concept linked_from_concept = keep_linked_concept( - get(self, 'from_concept'), data.get('from_source_url'), self.from_source_url, from_concept_code) + get(self, 'from_concept'), from_moved, from_concept_code, previous_from_concept_code) linked_to_concept = keep_linked_concept( - get(self, 'to_concept'), data.get('to_source_url'), self.to_source_url, to_concept_code) + get(self, 'to_concept'), to_moved, to_concept_code, previous_to_concept_code) if not to_concept_url and not linked_to_concept and self.to_source_url and ( to_concept_code or self.to_concept_code): @@ -426,13 +434,13 @@ def keep_linked_concept(concept, requested_source_url, resolved_source_url, requ self.to_concept_name = data.get('to_concept_name', None) or self.to_concept_name if self.to_source_url: to_source_version, to_source_url = separate_version(self.to_source_url) - if previous_to_source_url and to_source_url != previous_to_source_url: + if to_moved: self.to_source_version = None # a version carried over from the previous repo doesn't apply self.to_source_version = self.to_source_version or to_source_version self.to_source_url = to_source_url if self.from_source_url: from_source_version, from_source_url = separate_version(self.from_source_url) - if previous_from_source_url and from_source_url != previous_from_source_url: + if from_moved: self.from_source_version = None # a version carried over from the previous repo doesn't apply self.from_source_version = self.from_source_version or from_source_version self.from_source_url = from_source_url