OpenConceptLab/ocl_issues#2480 | clone operation everything or nothing

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)

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')

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.']