From b5b40c45c5a1162fb53d34a5fc856b1e599d8936 Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Mon, 20 Apr 2026 12:21:43 +0530 Subject: [PATCH] OpenConceptLab/ocl_issues#2480 | clone operation everything or nothing --- core/sources/models.py | 101 ++++++++++++++++++++------- core/sources/tests/tests.py | 132 +++++++++++++++++++++++++++++++++++- core/sources/views.py | 18 +++-- 3 files changed, 217 insertions(+), 34 deletions(-) diff --git a/core/sources/models.py b/core/sources/models.py index d8f4384a..6c8e1c2f 100644 --- a/core/sources/models.py +++ b/core/sources/models.py @@ -6,7 +6,7 @@ from django.conf import settings from django.contrib.postgres.fields import ArrayField from django.core.exceptions import ValidationError -from django.db import models +from django.db import models, transaction from django.db.models import UniqueConstraint, F, Max, Count from django.db.models.functions import Cast from pydash import get, compact @@ -25,6 +25,12 @@ from core.tasks.models import Task +class CloneError(Exception): + def __init__(self, errors): + super().__init__('Clone failed.') + self.errors = errors + + class Source(DirtyFieldsMixin, ConceptContainerModel): DEFAULT_AUTO_ID_START_FROM = 1 TOKEN_MATCH_ALGORITHM = 'es' @@ -689,38 +695,83 @@ def get_mapped_sources(self, exclude_self=True): queryset = queryset.exclude(id=self.id) return queryset - def clone_resources(self, user, concepts, mappings, **kwargs): + def clone_resources(self, user, concepts, mappings, **kwargs): # pylint: disable=too-many-locals,too-many-branches from core.mappings.models import Mapping - added_concepts, added_mappings = [], [] - equivalency_map_types = (kwargs.get('equivalency_map_types') or '').split(',') - _concepts_to_add_mappings_for = [] - for concept in concepts: - if not self.get_equivalent_concept(concept, equivalency_map_types): + with transaction.atomic(): + added_concepts, added_mappings = [], [] + concept_errors, mapping_errors = [], [] + equivalency_map_types = (kwargs.get('equivalency_map_types') or '').split(',') + _concepts_to_add_mappings_for = [] + for concept in concepts: + if self.get_equivalent_concept(concept, equivalency_map_types): + continue + cloned_concept = concept.versioned_object.clone() - added_concepts += self.clone_concepts([cloned_concept], user, False) - if not cloned_concept.id: + self.clone_concepts([cloned_concept], user, False) + if cloned_concept.id: + added_concepts.append(cloned_concept) + else: + concept_errors.append(self.get_clone_concept_error(cloned_concept, concept)) continue + if equivalency_map_types: - added_mappings += self.clone_mappings( - [Mapping.build( - map_type=equivalency_map_types[0], from_concept=cloned_concept, to_concept=concept, - parent=self - )], - user, - False + equivalency_mapping = Mapping.build( + map_type=equivalency_map_types[0], from_concept=cloned_concept, to_concept=concept, + parent=self ) + self.clone_mappings([equivalency_mapping], user, False) + if equivalency_mapping.id: + added_mappings.append(equivalency_mapping) + else: + mapping_errors.append(self.get_clone_mapping_error(equivalency_mapping)) _concepts_to_add_mappings_for.append([concept, cloned_concept]) - for concept_pair in _concepts_to_add_mappings_for: - concept, cloned_concept = concept_pair - for mapping in mappings.filter(from_concept__versioned_object_id=concept.versioned_object_id): - existing_to_concept = self.get_equivalent_concept(mapping.to_concept, equivalency_map_types) - added_mappings += self.clone_mappings( - [mapping.clone(user, cloned_concept, existing_to_concept)], user, False) - if added_concepts or added_mappings: - self.update_children_counts() + for concept_pair in _concepts_to_add_mappings_for: + concept, cloned_concept = concept_pair + for mapping in mappings.filter(from_concept__versioned_object_id=concept.versioned_object_id): + existing_to_concept = self.get_equivalent_concept(mapping.to_concept, equivalency_map_types) + cloned_mapping = mapping.clone(user, cloned_concept, existing_to_concept) + self.clone_mappings([cloned_mapping], user, False) + if cloned_mapping.id: + added_mappings.append(cloned_mapping) + else: + mapping_errors.append(self.get_clone_mapping_error(cloned_mapping, mapping)) + + errors = {} + if concept_errors: + errors['concepts'] = concept_errors + if mapping_errors: + errors['mappings'] = mapping_errors + if errors: + raise CloneError(errors) + + if added_concepts or added_mappings: + self.update_children_counts() + + return added_concepts, added_mappings - return added_concepts, added_mappings + @staticmethod + def get_clone_concept_error(cloned_concept, original_concept=None): + source_concept = original_concept or cloned_concept + return { + 'mnemonic': get(source_concept, 'mnemonic'), + 'source_url': get(source_concept, 'uri'), + 'errors': get(cloned_concept, 'errors') or {'__all__': ['Failed to clone concept.']} + } + + @staticmethod + def get_clone_mapping_error(cloned_mapping, original_mapping=None): + source_mapping = original_mapping or cloned_mapping + return { + 'map_type': get(source_mapping, 'map_type'), + 'from_concept_code': ( + get(source_mapping, 'from_concept_code') or get(source_mapping, 'from_concept.mnemonic') + ), + 'to_concept_code': get(source_mapping, 'to_concept_code') or get(source_mapping, 'to_concept.mnemonic'), + 'from_source_url': get(source_mapping, 'from_source_url') or get(source_mapping, 'from_source.uri'), + 'to_source_url': get(source_mapping, 'to_source_url') or get(source_mapping, 'to_source.uri'), + 'errors': get(cloned_mapping, 'errors') or {'__all__': ['Failed to clone mapping.']} + } def get_equivalent_concept(self, concept, equivalency_map_type): equivalent_mapping = self.get_equivalent_mapping(concept, equivalency_map_type) diff --git a/core/sources/tests/tests.py b/core/sources/tests/tests.py index e62fd3a6..a6abd723 100644 --- a/core/sources/tests/tests.py +++ b/core/sources/tests/tests.py @@ -12,7 +12,7 @@ from core.common.tasks import update_source_active_concepts_count from core.common.tasks import update_source_active_mappings_count from core.common.tasks import update_validation_schema -from core.common.tests import OCLTestCase +from core.common.tests import OCLTestCase, OCLAPITestCase from core.concepts.documents import ConceptDocument from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory @@ -21,7 +21,7 @@ from core.orgs.tests.factories import OrganizationFactory from core.services.storages.postgres import PostgresQL from core.sources.documents import SourceDocument -from core.sources.models import Source +from core.sources.models import Source, CloneError from core.sources.tests.factories import OrganizationSourceFactory, UserSourceFactory from core.url_registry.factories import OrganizationURLRegistryFactory, GlobalURLRegistryFactory from core.users.models import UserProfile @@ -1232,6 +1232,133 @@ def test_clone_with_cascade_with_autoid_sequence_manual_set(self): # pylint: di self.assertEqual(result.cascaded_entries['concepts'].count(), 0) self.assertEqual(result.cascaded_entries['mappings'].count(), 0) + def test_clone_with_cascade_rolls_back_when_mapping_clone_fails(self): + source1 = OrganizationSourceFactory(mnemonic='source1') + source1_concept1 = ConceptFactory( + mnemonic='concept1', parent=source1, names=[ConceptNameFactory.build(name='concept1')]) + source1_concept2 = ConceptFactory( + mnemonic='concept2', parent=source1, names=[ConceptNameFactory.build(name='concept2')]) + MappingFactory( + from_concept=source1_concept2, to_concept=source1_concept1, parent=source1, map_type='Q-AND-A') + + source2 = OrganizationSourceFactory(mnemonic='source2') + + with patch('core.mappings.models.Mapping.save_cloned', autospec=True) as save_cloned_mock: + def fail_save_cloned(mapping): + mapping.errors = {'__all__': ['mapping clone failed']} + + save_cloned_mock.side_effect = fail_save_cloned + + with self.assertRaisesMessage(CloneError, 'Clone failed.'): + source2.clone_with_cascade( + concept_to_clone=source1_concept2, + user=source1_concept2.created_by, + map_types='Q-AND-A', + equivalency_map_types='SAME-AS' + ) + + self.assertEqual(source2.get_active_concepts().count(), 0) + self.assertEqual(source2.get_active_mappings().count(), 0) + + def test_clone_with_cascade_returns_concept_object_errors_in_clone_error(self): + source1 = OrganizationSourceFactory(mnemonic='source1') + source1_concept = ConceptFactory( + mnemonic='concept1', parent=source1, names=[ConceptNameFactory.build(name='concept1')]) + source2 = OrganizationSourceFactory(mnemonic='source2') + + with patch('core.concepts.models.Concept.save_cloned', autospec=True) as save_cloned_mock: + def fail_save_cloned(concept): + concept.errors = {'external_id': ['duplicate external id']} + + save_cloned_mock.side_effect = fail_save_cloned + + with self.assertRaises(CloneError) as raised: + source2.clone_with_cascade( + concept_to_clone=source1_concept, + user=source1_concept.created_by, + equivalency_map_types='SAME-AS' + ) + + self.assertEqual( + raised.exception.errors['concepts'], + [{ + 'mnemonic': source1_concept.mnemonic, + 'source_url': source1_concept.uri, + 'errors': {'external_id': ['duplicate external id']} + }] + ) + self.assertEqual(source2.get_active_concepts().count(), 0) + + def test_clone_with_cascade_returns_mapping_object_errors_in_clone_error(self): + source1 = OrganizationSourceFactory(mnemonic='source1') + source1_concept1 = ConceptFactory( + mnemonic='concept1', parent=source1, names=[ConceptNameFactory.build(name='concept1')]) + source1_concept2 = ConceptFactory( + mnemonic='concept2', parent=source1, names=[ConceptNameFactory.build(name='concept2')]) + failed_mapping = MappingFactory( + from_concept=source1_concept2, to_concept=source1_concept1, parent=source1, map_type='Q-AND-A', + from_source=source1, to_source=source1 + ) + source2 = OrganizationSourceFactory(mnemonic='source2') + + with patch('core.mappings.models.Mapping.save_cloned', autospec=True) as save_cloned_mock: + def fail_save_cloned(mapping): + mapping.errors = {'map_type': ['unsupported in target source']} + + save_cloned_mock.side_effect = fail_save_cloned + + with self.assertRaises(CloneError) as raised: + source2.clone_with_cascade( + concept_to_clone=source1_concept2, + user=source1_concept2.created_by, + map_types='Q-AND-A' + ) + + self.assertIn( + { + 'map_type': failed_mapping.map_type, + 'from_concept_code': failed_mapping.from_concept.mnemonic, + 'to_concept_code': failed_mapping.to_concept.mnemonic, + 'from_source_url': failed_mapping.from_source.uri, + 'to_source_url': failed_mapping.to_source.uri, + 'errors': {'map_type': ['unsupported in target source']} + }, + raised.exception.errors['mappings'] + ) + self.assertEqual(source2.get_active_concepts().count(), 0) + self.assertEqual(source2.get_active_mappings().count(), 0) + + +class SourceCloneAPITest(OCLAPITestCase): + def test_clone_api_returns_structured_errors_and_rolls_back(self): + source1 = OrganizationSourceFactory(mnemonic='source1') + concept_to_clone = ConceptFactory( + mnemonic='concept1', parent=source1, names=[ConceptNameFactory.build(name='concept1')]) + source2 = OrganizationSourceFactory(mnemonic='source2') + user = source2.created_by + source2.organization.members.add(user) + self.client.force_authenticate(user=user) + + url = f'/orgs/{source2.organization.mnemonic}/sources/{source2.mnemonic}/concepts/$clone/' + + with patch('core.concepts.models.Concept.save_cloned', autospec=True) as save_cloned_mock: + def fail_save_cloned(concept): + concept.errors = {'__all__': ['concept clone failed']} + + save_cloned_mock.side_effect = fail_save_cloned + response = self.client.post(url, {'expressions': [concept_to_clone.uri]}, format='json') + + self.assertEqual(response.status_code, 200) + payload = response.data[concept_to_clone.uri] + self.assertEqual(payload['status'], 400) + self.assertEqual(payload['errors']['concepts'][0]['mnemonic'], concept_to_clone.mnemonic) + self.assertEqual(payload['errors']['concepts'][0]['source_url'], concept_to_clone.uri) + self.assertEqual(payload['errors']['concepts'][0]['errors'], {'__all__': ['concept clone failed']}) + self.assertEqual(source2.get_active_concepts().count(), 0) + self.assertEqual(source2.get_active_mappings().count(), 0) + + +class SourceValidationTest(OCLTestCase): def test_clean_properties_valid(self): source = Source(properties=[ {'code': 'height', 'type': 'integer'}, @@ -1288,6 +1415,7 @@ def test_clean_filters_extra_keys(self): with self.assertRaises(ValidationError): source.clean_filters() + class TasksTest(OCLTestCase): @patch('core.sources.models.Source.index_children') @patch('core.common.tasks.export_source') diff --git a/core/sources/views.py b/core/sources/views.py index 53c6c084..65a29d08 100644 --- a/core/sources/views.py +++ b/core/sources/views.py @@ -34,7 +34,7 @@ from core.sources.constants import DELETE_FAILURE, DELETE_SUCCESS, VERSION_ALREADY_EXISTS from core.sources.documents import SourceDocument from core.sources.mixins import SummaryMixin -from core.sources.models import Source +from core.sources.models import Source, CloneError from core.sources.search import SourceFacetedSearch from core.sources.serializers import ( SourceDetailSerializer, SourceListSerializer, SourceCreateSerializer, SourceVersionDetailSerializer, @@ -390,12 +390,16 @@ def post(self, request, **kwargs): # pylint: disable=unused-argument, too-many- parent_resources[parent_uri] = Source.objects.filter(uri=parent_uri).first() parent_resource = parent_resources[parent_uri] from core.bundles.models import Bundle - bundle = Bundle.clone( - concept_to_clone, parent_resource, instance, request.user, - self.request.get_full_path(), is_verbose, **parameters - ) - result['status'] = status.HTTP_200_OK - result['bundle'] = BundleSerializer(bundle, context={'request': request}).data + try: + bundle = Bundle.clone( + concept_to_clone, parent_resource, instance, request.user, + self.request.get_full_path(), is_verbose, **parameters + ) + result['status'] = status.HTTP_200_OK + result['bundle'] = BundleSerializer(bundle, context={'request': request}).data + except CloneError as ex: + result['status'] = status.HTTP_400_BAD_REQUEST + result['errors'] = ex.errors else: result['status'] = status.HTTP_404_NOT_FOUND result['errors'] = [f'Concept to clone with expression {expression} not found.']