Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions core/concepts/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,71 @@ def create_new_versions_for_removed_parents(self, uris):
child = Concept.objects.filter(uri=uri).first().get_latest_version()
child.parent_concepts.add(new_latest_version)

def create_mappings(self, mappings):
from core.mappings.serializers import MappingDetailSerializer
results = []
any_with_errors = False
if mappings and self.id:
user = self.created_by
for mapping_data in mappings:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The absence of specific mapping validation here can trigger a 500 error with a TypeError, preventing the rollback and persisting the concept even in the event of a payload failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_create_mapping_from_self is under try/catch block.
this calls Mapping.persist_new which runs validation based on repo validation schema.
if there are any errors in mapping creation they populate errors here in catch block or down below when serializing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, _create_mapping_from_self is within a try-catch block, but the error in for mapping_data in mappings is not caught if it's not an expected list. However, the initial validation that was performed already resolves this by returning a 400 error beforehand. Thanks

errors = {}
try:
created = self._create_mapping_from_self(mapping_data.copy(), user)
except Exception as ex:
error_dict = get(ex, 'message_dict') or get(ex, 'error_dict')
if error_dict:
errors = error_dict
else:
errors['__all__'] = [str(ex)]
any_with_errors = True
created = False
serializer = None
if created:
serializer = MappingDetailSerializer(created)
errors = created.errors
if not errors and not created.id:
errors['__all__'] = ['Something bad happened while creating the mapping.']
if errors:
serializer._errors = errors # pylint: disable=protected-access
any_with_errors = True

results.append({
'mapping': mapping_data,
'instance': created,
'serializer': serializer,
'errors': errors
})

return results, any_with_errors

def _create_mapping_from_self(self, mapping_data, user):
from core.mappings.models import Mapping
data = {key: value for key, value in mapping_data.items() if not key.startswith('from_')}
data['from_concept_url'] = drop_version(self.uri)
if data.get('to_concept') == '__parent_concept':
data['to_concept_url'] = self.uri
data.pop('to_concept')

return Mapping.persist_new({**data, 'parent_id': self.parent_id}, user)

@staticmethod
def _remove_mappings_just_created(mappings_results):
for mapping in mappings_results:
if get(mapping, 'instance.id', None):
try:
mapping['instance'].delete()
except IntegrityError:
pass

@staticmethod
def _get_errors_from_mappings(mappings_result):
return [
{
**mapping['mapping'],
'errors': get(mapping, 'serializer.errors') or mapping.get('errors')
} for mapping in mappings_result if mapping.get('errors')
]

def set_locales(self, locales, locale_klass):
if not self.id:
return # pragma: no cover
Expand Down Expand Up @@ -783,6 +848,10 @@ def persist_new(cls, data, user=None, create_initial_version=True, create_parent
names = data.pop('names', []) or []
descriptions = data.pop('descriptions', []) or []
parent_concept_uris = data.pop('parent_concept_urls', None)
mappings_payload = data.pop('mappings_payload', None) or data.pop('mappings', None) or []
mappings_result = []
has_mapping_errors = False
initial_version = None
concept = Concept(**data)
temp_version = generate_temp_version()
concept.version = temp_version
Expand Down Expand Up @@ -832,6 +901,12 @@ def persist_new(cls, data, user=None, create_initial_version=True, create_parent
initial_version.sources.set([parent])

concept.sources.set([parent])

if mappings_payload:
mappings_result, has_mapping_errors = concept.create_mappings(mappings_payload)
if has_mapping_errors:
raise ValidationError('Error(s) occurred while creating mappings.')

if get(settings, 'TEST_MODE', False):
update_mappings_concept(concept.id)
else:
Expand All @@ -850,13 +925,25 @@ def persist_new(cls, data, user=None, create_initial_version=True, create_parent
parent.update_concepts_count()
concept.set_checksums(sync=sync_checksum)
except ValidationError as ex:
Comment thread
filiperochalopes marked this conversation as resolved.
if mappings_result:
concept._remove_mappings_just_created(mappings_result)
if get(initial_version, 'id'):
initial_version.delete()
if concept.id:
concept.delete()
concept.errors.update(get(ex, 'message_dict', {}) or get(ex, 'error_dict', {}))
if has_mapping_errors:
concept.errors['mappings'] = concept._get_errors_from_mappings(mappings_result)
except (IntegrityError, ValueError) as ex:
if mappings_result:
concept._remove_mappings_just_created(mappings_result)
if get(initial_version, 'id'):
initial_version.delete()
if concept.id:
concept.delete()
concept.errors.update({'__all__': ex.args})
if has_mapping_errors:
concept.errors['mappings'] = concept._get_errors_from_mappings(mappings_result)

return concept

Expand Down
3 changes: 2 additions & 1 deletion core/concepts/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ def to_representation(self, instance): # used to be to_native
class ConceptAbstractSerializer(AbstractResourceSerializer):
uuid = CharField(source='id', read_only=True)
mappings = SerializerMethodField()
mappings_payload = ListField(child=JSONField(), write_only=True, required=False, allow_empty=True)
parent_concepts = SerializerMethodField()
child_concepts = SerializerMethodField()
hierarchy_path = SerializerMethodField()
Expand All @@ -126,7 +127,7 @@ class Meta:
abstract = True
fields = AbstractResourceSerializer.Meta.fields + (
'uuid', 'parent_concept_urls', 'child_concept_urls', 'parent_concepts', 'child_concepts', 'hierarchy_path',
'mappings', 'extras', 'summary', 'references', 'has_children', 'checksums'
'mappings', 'extras', 'summary', 'references', 'has_children', 'checksums', 'mappings_payload'
)

def __init__(self, *args, **kwargs): # pylint: disable=too-many-branches
Expand Down
6 changes: 3 additions & 3 deletions core/concepts/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,9 @@ def post(self, request, **_):
if not self.parent_resource or isinstance(request.data, list):
raise Http404()
concept_id = get(request.data, 'id') or generate_temp_version()
serializer = self.get_serializer(
data={**request.data, 'parent_id': self.parent_resource.id, 'id': concept_id, 'name': concept_id}
)
data = {**request.data, 'parent_id': self.parent_resource.id, 'id': concept_id, 'name': concept_id}
data['mappings_payload'] = data.pop('mappings', [])
serializer = self.get_serializer(data=data)
if serializer.is_valid():
serializer.save()
if serializer.is_valid():
Expand Down
169 changes: 168 additions & 1 deletion core/integration_tests/tests_concepts.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from core.concepts.documents import ConceptDocument
from core.concepts.models import Concept
from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory
from core.mappings.models import Mapping
from core.mappings.tests.factories import MappingFactory
from core.orgs.models import Organization
from core.sources.tests.factories import OrganizationSourceFactory, UserSourceFactory
Expand Down Expand Up @@ -143,6 +144,173 @@ def test_post_201(self):
self.assertEqual(response.status_code, 400)
self.assertEqual(response.data, {'__all__': ['Concept ID must be unique within a source.']})

def test_post_201_with_mappings(self):
concepts_url = f"/orgs/{self.organization.mnemonic}/sources/{self.source.mnemonic}/concepts/"
random_concept = ConceptFactory()

mappings = [
# 1 to target concept that doesnt exists with __parent_concept as substitution
{
'from_concept': '__parent_concept',
'to_concept_url': '/orgs/random-org/sources/random-source/concepts/target-concept/',
'map_type': 'Same As'
},
# 2 to target concept that doesnt exists with parent concept as direct url
{
'from_concept_url': concepts_url + 'c1/',
'to_concept_url': '/orgs/random-org/sources/random-source/concepts/target-concept/',
'map_type': 'BROADER-THAN'
},
# 3 to target concept that exists
{
'from_concept_url': concepts_url + 'c1/',
'to_concept_url': random_concept.url,
'map_type': 'NARROWER-THAN'
},
# 4 self mapping without from_concept
{
'to_concept_url': concepts_url + 'c1/',
'map_type': 'Same As'
},
]

response = self.client.post(
concepts_url,
{**self.concept_payload, 'mappings': mappings},
HTTP_AUTHORIZATION='Token ' + self.token,
format='json'
)

self.assertEqual(response.status_code, 201)
self.assertListEqual(
sorted(list(response.data.keys())),
sorted([
'uuid',
'id',
'external_id',
'concept_class',
'datatype',
'url',
'retired',
'source',
'owner',
'owner_type',
'owner_url',
'display_name',
'display_locale',
'names',
'descriptions',
'created_on',
'updated_on',
'versions_url',
'version',
'extras',
'type',
'update_comment',
'version_url',
'updated_by',
'created_by',
'public_can_view',
'checksums',
'property',
'versioned_object_id',
'latest_source_version'
])
)

concept = Concept.objects.filter(mnemonic='c1').first().versioned_object
latest_version = concept.get_latest_version()

self.assertFalse(latest_version.is_versioned_object)
self.assertTrue(latest_version.is_latest_version)

self.assertTrue(concept.is_versioned_object)
self.assertFalse(concept.is_latest_version)

self.assertEqual(concept.versions.count(), 1)
self.assertEqual(response.data['uuid'], str(concept.id))
self.assertEqual(response.data['datatype'], 'Coded')
self.assertEqual(response.data['concept_class'], 'Procedure')
self.assertEqual(response.data['url'], concept.uri)
self.assertFalse(response.data['retired'])
self.assertEqual(response.data['source'], self.source.mnemonic)
self.assertEqual(response.data['owner'], self.organization.mnemonic)
self.assertEqual(response.data['owner_type'], "Organization")
self.assertEqual(response.data['owner_url'], self.organization.uri)
self.assertEqual(response.data['display_name'], 'c1 name')
self.assertEqual(response.data['display_locale'], 'en')
self.assertEqual(response.data['versions_url'], concept.uri + 'versions/')
self.assertEqual(response.data['version'], str(concept.id))
self.assertEqual(response.data['extras'], {'foo': 'bar'})
self.assertEqual(response.data['type'], 'Concept')
self.assertEqual(response.data['version_url'], latest_version.uri)
self.assertEqual(latest_version.get_bidirectional_mappings().count(), 4)
self.assertEqual(concept.get_bidirectional_mappings().count(), 4)
self.assertEqual(concept.parent.get_mappings_queryset().count(), 4)
self.assertEqual(self.source.get_mappings_queryset().count(), 4)

def test_post_400_with_mappings_everything_or_nothing(self):
concepts_url = f"/orgs/{self.organization.mnemonic}/sources/{self.source.mnemonic}/concepts/"
random_concept = ConceptFactory()

mappings = [
# 1 to target concept that doesnt exists with __parent_concept as substitution
{
'to_concept_url': '/orgs/random-org/sources/random-source/concepts/target-concept/',
'map_type': 'Same As'
},
# 2 to target concept that doesnt exists with parent concept as direct url -- must fail for duplicate
{
'from_concept_url': concepts_url + 'c1/',
'to_concept_url': '/orgs/random-org/sources/random-source/concepts/target-concept/',
'map_type': 'Same As'
},
# 3 to target concept that exists
{
'from_concept_url': concepts_url + 'c1/',
'to_concept_url': random_concept.url,
'map_type': 'NARROWER-THAN'
},
# 4 parent concept not involved - must pass and from_concept_url is ignored and set to parent concept url
{
'from_concept_url': concepts_url + 'c2/',
'to_concept_url': random_concept.url,
'map_type': 'Same-As'
},
# 5 parent concept not involved - must fail for parent concept not from concept
{
'from_concept_url': random_concept.url,
'to_concept_url': concepts_url + 'c1/',
'map_type': 'Same-As'
},
]

response = self.client.post(
concepts_url,
{**self.concept_payload, 'mappings': mappings},
HTTP_AUTHORIZATION='Token ' + self.token,
format='json'
)

self.assertEqual(response.status_code, 400)
self.assertEqual(
response.data,
{
'mappings': [
{
**mappings[1],
'errors': {
'__all__': ['Parent, map_type, from_concept, to_source, to_concept_code must be unique.']
}
}
]
}
)
self.assertFalse(Concept.objects.filter(mnemonic='c1').exists())
self.assertFalse(self.source.get_concepts_queryset().exists())
self.assertFalse(self.source.get_mappings_queryset().exists())
self.assertEqual(Mapping.objects.count(), 0)

def test_post_400(self):
concepts_url = f"/orgs/{self.organization.mnemonic}/sources/{self.source.mnemonic}/concepts/"

Expand Down Expand Up @@ -319,7 +487,6 @@ def test_put_200(self): # pylint: disable=too-many-statements
self.assertEqual(response.data['display_name'], prev_version.display_name)
self.assertEqual(concept.datatype, "N/A")


def test_put_200_openmrs_schema(self): # pylint: disable=too-many-statements
self.create_lookup_concept_classes()
source = OrganizationSourceFactory(custom_validation_schema=OPENMRS_VALIDATION_SCHEMA)
Expand Down