Skip to content

Commit fa9e368

Browse files
paynejdclaude
paynejd
and
claude
authored
OpenConceptLab/ocl_online#451 | FHIR $validate-code and $translate need a code and its system, and read only that code (#947)
* OpenConceptLab/ocl_online#451 | FHIR $validate-code and $translate need a code and its system, and read only that code - A POSTed Parameters resource is read with the value types R4 gives each input, and a code or URI sent as valueString is read too. When both are sent, the declared type wins. Before, a code sent as valueString was dropped, and the request went on as if it had no code. - CodeSystem $validate-code without a code, or with a code but no url, is a 400 OperationOutcome naming the missing parameter. Before, it validated against every concept on the server, and a display check read all of them. A url that doesn't resolve, or that the requester can't view, still answers result=false, with or without a code (OpenConceptLab/ocl_online#224). - ValueSet $validate-code without a code, or with a code but no system, is a 400. Before, it answered for the first concept in the value set, or on the server. Without url, the global route still checks the code against the code system alone. - ConceptMap $translate without a code, or with a code but no system, is a 400. It no longer tests whether the ConceptMap, the code or the system match anything before narrowing further: each test read every matching mapping. - The 400 is a DRF ValidationError carrying an OperationOutcome (issue code invalid, expression naming the parameter), shaped like $lookup's not-found. - core/code_systems/tests_operation_bounds.py checks every concept and mapping read these operations make, for GET and POST, with and without code, url, system and display: each must name its rows by code or id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wdpwbn9tU8NmVL8oW2A75d * OpenConceptLab/ocl_online#451 | FHIR $translate answers only through its own checks, never the mapping list's search, facets or CSV export - $translate inherited the mapping list view's shortcuts. A q, sort or faceted filter parameter sent it to Elasticsearch, facetsOnly and searchStatsOnly answered with facets or search stats, the IncludeFacets header wrapped the answer in facets, and csv=true answered with a CSV export of the matching mappings instead of the translation. The search ones skipped apply_filters, so the request needed no code or system, and the translation's url, code and system filters weren't applied. - Each shortcut is off for $translate. Their parameters are ignored, like any other parameter the operation doesn't define. - The two $validate-code views and $lookup read through get_queryset directly, so they never took these shortcuts. tests_operation_bounds.py now checks all four operations with each shortcut's parameter or header: the answer is the same as without it, a request without a code is still a 400, and no search, facet, search-stats or export call is made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wdpwbn9tU8NmVL8oW2A75d * OpenConceptLab/ocl_online#451 | FHIR $translate reads a POSTed Parameters resource - POST $translate failed with a TypeError before reading its inputs: it passed instance twice through the view's get_serializer, which renders the answer. The Parameters resource is now read by the serializer class itself, as a GET's query is. - tests_operation_bounds.py checks $translate by POST, with declared value types and with valueString, as it checks GET: a missing code or system is a 400, and every mapping read names its rows by code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RMvtGDtJmnhfUDvHQAw4HF --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f9489ca commit fa9e368

5 files changed

Lines changed: 345 additions & 35 deletions

File tree

‎core/code_systems/tests_operation_bounds.py‎

Lines changed: 253 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,253 @@
1+
"""
2+
FHIR operations read only the concepts and mappings their input names (OpenConceptLab/ocl_online#451).
3+
4+
A $validate-code or $translate without a code, or without the system the code is in, is a 400 OperationOutcome,
5+
never a read of every concept or mapping. A POSTed Parameters resource is read with the value types R4 gives each
6+
input, so a code sent as valueString is still the code.
7+
"""
8+
import json
9+
import re
10+
from itertools import product
11+
from urllib.parse import urlencode
12+
13+
from django.db import connection
14+
from django.test.utils import CaptureQueriesContext
15+
from mock.mock import patch, Mock
16+
from rest_framework.response import Response
17+
from rest_framework.test import APIClient
18+
19+
from core.code_systems.serializers import ValidateCodeParametersSerializer
20+
from core.collections.models import CollectionReference
21+
from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory
22+
from core.common.mixins import ListWithHeadersMixin
23+
from core.common.tests import OCLTestCase
24+
from core.common.views import BaseAPIView
25+
from core.concepts.tests.factories import ConceptFactory
26+
from core.mappings.models import Mapping
27+
from core.mappings.tests.factories import MappingFactory
28+
from core.orgs.tests.factories import OrganizationFactory
29+
from core.sources.models import Source
30+
from core.sources.tests.factories import OrganizationSourceFactory
31+
32+
SYSTEM = 'http://example.org/fhir/CodeSystem/bounds'
33+
VALUE_SET = 'http://example.org/fhir/ValueSet/bounds'
34+
DISPLAY = 'Malaria'
35+
WRONG_DISPLAY = 'Not the display of c1'
36+
VALUE_TYPES = {'url': 'valueUri', 'system': 'valueUri', 'code': 'valueCode', 'display': 'valueString',
37+
'systemVersion': 'valueString'}
38+
# A statement that reads concept or mapping rows: that table is its own, not a joined one
39+
ROW_READ = re.compile(r'^SELECT .+? FROM "(concepts|mappings)"(?: |$)')
40+
# A literal equality, or a literal IN list, on a column that names rows: a code, an id, or the concept whose
41+
# parents or children are read. A LIMIT alone doesn't count: `.first()` of every concept is still every concept.
42+
NAMED_ROWS = re.compile(
43+
r'"((concepts|mappings)"\."(mnemonic|from_concept_code|id|versioned_object_id)'
44+
r'|concepts_hierarchicalconcepts"\."(child_id|parent_id))" (= (\'|\d)|IN \((?!SELECT))')
45+
# The list views' own query parameters and headers, each of which would take a list shortcut (a search, a sort,
46+
# facets, search stats or a CSV export) past an operation's checks if the operation inherited it
47+
LIST_SHORTCUTS = [
48+
({'q': 'c1'}, {}),
49+
({'sortAsc': 'id'}, {}),
50+
({'sortDesc': 'id'}, {}),
51+
({'facetsOnly': 'true'}, {}),
52+
({'searchStatsOnly': 'true', 'q': 'c1'}, {}),
53+
({'csv': 'true'}, {}),
54+
({}, {'HTTP_INCLUDEFACETS': 'true'}),
55+
({'q': 'c1'}, {'HTTP_INCLUDESEARCHSTATS': 'true'}),
56+
]
57+
58+
59+
def row_reads(queries):
60+
return [query['sql'] for query in queries if ROW_READ.match(query['sql'])]
61+
62+
63+
def unbounded_reads(queries):
64+
"""The statements that read concept or mapping rows without naming them by code or id."""
65+
return [sql for sql in row_reads(queries) if not NAMED_ROWS.search(sql)]
66+
67+
68+
class FhirOperationBoundsTest(OCLTestCase):
69+
@patch('core.sources.models.index_source_concepts', Mock(__name__='index_source_concepts'))
70+
@patch('core.sources.models.index_source_mappings', Mock(__name__='index_source_mappings'))
71+
def setUp(self):
72+
super().setUp()
73+
self.org = OrganizationFactory(mnemonic='BoundsOrg')
74+
self.source = OrganizationSourceFactory(organization=self.org, mnemonic='bounds', canonical_url=SYSTEM)
75+
self.concept = ConceptFactory(parent=self.source, mnemonic='c1', names=1, names__name=DISPLAY)
76+
other = ConceptFactory(parent=self.source, mnemonic='c2')
77+
MappingFactory(parent=self.source, from_concept=self.concept, to_concept=other, from_source=self.source,
78+
to_source=self.source, from_concept_code='c1', to_concept_code='c2')
79+
self.source_v1 = OrganizationSourceFactory.build(
80+
version='v1', mnemonic='bounds', organization=self.org, canonical_url=SYSTEM, released=True)
81+
Source.persist_new_version(self.source_v1, self.source.created_by)
82+
self.collection = OrganizationCollectionFactory(
83+
organization=self.org, mnemonic='boundsvs', canonical_url=VALUE_SET, version='HEAD')
84+
self.collection_v1 = OrganizationCollectionFactory(
85+
organization=self.org, mnemonic='boundsvs', canonical_url=VALUE_SET, version='v1', released=True,
86+
is_latest_version=True)
87+
for collection, name in [(self.collection, 'e1'), (self.collection_v1, 'e2')]:
88+
collection.expansion_uri = ExpansionFactory(mnemonic=name, collection_version=collection).uri
89+
collection.save()
90+
self.collection.add_references([CollectionReference(
91+
expression=self.concept.uri, collection=self.collection, code='c1', system=self.source.uri, version='v1')])
92+
self.collection_v1.seed_references()
93+
self.client = APIClient()
94+
95+
def request(self, path, method, headers=None, **inputs):
96+
"""(response, the SQL it ran) for a GET with the inputs as a query, or a POST of them as Parameters. A POST
97+
as 'POST valueString' sends every input as valueString, as some clients do."""
98+
inputs = {name: value for name, value in inputs.items() if value is not None}
99+
with CaptureQueriesContext(connection) as context:
100+
if method == 'GET':
101+
response = self.client.get(f'{path}?{urlencode(inputs)}' if inputs else path, **(headers or {}))
102+
else:
103+
parameters = [{'name': name, ('valueString' if 'valueString' in method else VALUE_TYPES[name]): value}
104+
for name, value in inputs.items()]
105+
response = self.client.post(path, json.dumps({'resourceType': 'Parameters', 'parameter': parameters}),
106+
content_type='application/json')
107+
return response, context.captured_queries
108+
109+
def assert_required(self, response, name, msg):
110+
self.assertEqual(response.status_code, 400, msg)
111+
outcome = json.loads(response.content)
112+
self.assertEqual(outcome['resourceType'], 'OperationOutcome', msg)
113+
self.assertEqual(outcome['issue'][0]['code'], 'invalid', msg)
114+
self.assertEqual(outcome['issue'][0]['expression'], [name], msg)
115+
116+
def assert_result(self, response, expected, msg):
117+
self.assertEqual(response.status_code, 200, msg)
118+
self.assertEqual(response.data['parameter'][0], {'name': 'result', 'valueBoolean': expected}, msg)
119+
120+
def test_code_system_validate_code_reads_only_the_named_code(self):
121+
cases = product(('GET', 'POST', 'POST valueString'), (None, SYSTEM), (None, 'c1'),
122+
(None, DISPLAY, WRONG_DISPLAY))
123+
for method, url, code, display in cases:
124+
msg = (method, url, code, display)
125+
with self.subTest(msg):
126+
response, queries = self.request(
127+
'/fhir/CodeSystem/$validate-code', method, url=url, code=code, display=display)
128+
129+
self.assertEqual(unbounded_reads(queries), [], msg)
130+
if not code:
131+
self.assert_required(response, 'code', msg)
132+
elif not url:
133+
self.assert_required(response, 'url', msg)
134+
else:
135+
self.assert_result(response, display != WRONG_DISPLAY, msg)
136+
self.assertTrue([sql for sql in row_reads(queries) if '"concepts"."mnemonic" = \'c1\'' in sql], msg)
137+
138+
def test_code_system_validate_code_with_a_misnamed_code_is_a_400(self):
139+
response, queries = self.request(
140+
'/fhir/CodeSystem/$validate-code', 'GET', url=SYSTEM, Code='c1', display=WRONG_DISPLAY)
141+
142+
self.assertEqual(unbounded_reads(queries), [])
143+
self.assert_required(response, 'code', 'Code')
144+
145+
def test_code_system_validate_code_for_an_unknown_system_is_false_with_or_without_a_code(self):
146+
for code, display in product((None, 'c1'), (None, WRONG_DISPLAY)):
147+
with self.subTest((code, display)):
148+
response, queries = self.request(
149+
'/fhir/CodeSystem/$validate-code', 'GET', url='http://example.org/unknown', code=code,
150+
display=display)
151+
152+
self.assertEqual(unbounded_reads(queries), [], (code, display))
153+
self.assert_result(response, False, (code, display))
154+
155+
def test_value_set_validate_code_reads_only_the_named_code(self):
156+
paths = [('/fhir/ValueSet/$validate-code', None), ('/fhir/ValueSet/$validate-code', VALUE_SET),
157+
(f'/orgs/{self.org.mnemonic}/ValueSet/boundsvs/$validate-code', None)]
158+
for method, (path, url), code, system, display in product(
159+
('GET', 'POST valueString'), paths, (None, 'c1'), (None, SYSTEM), (None, WRONG_DISPLAY)):
160+
msg = (method, path, url, code, system, display)
161+
with self.subTest(msg):
162+
response, queries = self.request(
163+
path, method, url=url, code=code, system=system, systemVersion='v1', display=display)
164+
165+
self.assertEqual(unbounded_reads(queries), [], msg)
166+
if not code:
167+
self.assert_required(response, 'code', msg)
168+
elif not system:
169+
self.assert_required(response, 'system', msg)
170+
else:
171+
self.assert_result(response, display is None, msg)
172+
173+
def test_translate_reads_only_the_named_code(self):
174+
for method, url, code, system in product(
175+
('GET', 'POST', 'POST valueString'), (None, SYSTEM), (None, 'c1'), (None, SYSTEM)):
176+
msg = (method, url, code, system)
177+
with self.subTest(msg):
178+
response, queries = self.request(
179+
'/fhir/ConceptMap/$translate', method, url=url, code=code, system=system)
180+
181+
self.assertEqual(unbounded_reads(queries), [], msg)
182+
if not code:
183+
self.assert_required(response, 'code', msg)
184+
elif not system:
185+
self.assert_required(response, 'system', msg)
186+
else:
187+
self.assert_result(response, True, msg)
188+
self.assertEqual(response.data['parameter'][1]['part'][1]['valueCoding']['code'], 'c2', msg)
189+
190+
def test_lookup_reads_only_the_named_code(self):
191+
for system, code in product((None, SYSTEM), (None, 'c1')):
192+
with self.subTest((system, code)):
193+
response, queries = self.request('/fhir/CodeSystem/$lookup', 'GET', system=system, code=code)
194+
195+
self.assertEqual(unbounded_reads(queries), [], (system, code))
196+
self.assertEqual(response.status_code, 200 if system and code else 400, (system, code))
197+
198+
def test_operations_take_no_list_shortcut(self):
199+
"""A search, sort, facets, search stats or CSV export never stands in for an operation's own checks."""
200+
operations = [
201+
('/fhir/CodeSystem/$validate-code', {'url': SYSTEM, 'code': 'c1'}),
202+
('/fhir/ValueSet/$validate-code',
203+
{'url': VALUE_SET, 'system': SYSTEM, 'systemVersion': 'v1', 'code': 'c1'}),
204+
('/fhir/ConceptMap/$translate', {'url': SYSTEM, 'system': SYSTEM, 'code': 'c1'}),
205+
('/fhir/CodeSystem/$lookup', {'system': SYSTEM, 'code': 'c1'}),
206+
]
207+
shortcuts = {
208+
(BaseAPIView, 'get_search_results_qs'): (Mapping.objects.none(), {}, None, {}),
209+
(BaseAPIView, 'get_fuzzy_search_results_qs'): (Mapping.objects.none(), {}, None, {}),
210+
(BaseAPIView, 'get_facets'): {},
211+
(BaseAPIView, 'get_search_stats'): {},
212+
(ListWithHeadersMixin, 'get_csv'): Response({'url': 'https://example.org/export.csv'}),
213+
}
214+
mocks = {name: Mock(return_value=value) for (_, name), value in shortcuts.items()}
215+
patches = [patch.object(klass, name, mocks[name]) for klass, name in shortcuts]
216+
for patcher in patches:
217+
patcher.start()
218+
self.addCleanup(patcher.stop)
219+
220+
for (path, inputs), (params, headers) in product(operations, LIST_SHORTCUTS):
221+
msg = (path, params, headers)
222+
with self.subTest(msg):
223+
expected, _ = self.request(path, 'GET', **inputs)
224+
response, queries = self.request(path, 'GET', headers=headers, **inputs, **params)
225+
226+
self.assertEqual(unbounded_reads(queries), [], msg)
227+
self.assertEqual(
228+
(response.status_code, json.loads(response.content)),
229+
(expected.status_code, json.loads(expected.content)), msg)
230+
231+
without_code = {name: value for name, value in inputs.items() if name != 'code'}
232+
response, queries = self.request(path, 'GET', headers=headers, **without_code, **params)
233+
234+
self.assertEqual(unbounded_reads(queries), [], msg)
235+
self.assertEqual(response.status_code, 400, msg)
236+
self.assertEqual({name: mock.call_count for name, mock in mocks.items()}, dict.fromkeys(mocks, 0))
237+
238+
def test_parameters_are_read_with_the_value_types_r4_allows(self):
239+
serializer = ValidateCodeParametersSerializer(data={'resourceType': 'Parameters', 'parameter': [
240+
{'name': 'url', 'valueString': SYSTEM},
241+
{'name': 'code', 'valueString': 'c1'},
242+
{'name': 'display', 'valueString': DISPLAY},
243+
{'name': 'version', 'valueCode': 'v1'},
244+
{'name': 'system', 'valueBoolean': True},
245+
]})
246+
247+
self.assertTrue(serializer.is_valid())
248+
self.assertEqual(serializer.validated_data['parameters'], {'url': SYSTEM, 'code': 'c1', 'display': DISPLAY})
249+
250+
serializer = ValidateCodeParametersSerializer(data={'resourceType': 'Parameters', 'parameter': [
251+
{'name': 'code', 'valueCode': 'c1', 'valueString': 'c2'}]})
252+
self.assertTrue(serializer.is_valid())
253+
self.assertEqual(serializer.validated_data['parameters'], {'code': 'c1'})

‎core/code_systems/views.py‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,29 @@ def __init__(self, code: str = None):
8181
)
8282

8383

84+
class FhirParameterRequiredError(ValidationError):
85+
"""A 400 OperationOutcome for an operation input that is required, naming it in `expression`."""
86+
def __init__(self, message: str, expression: str):
87+
super().__init__(
88+
detail={
89+
"resourceType": "OperationOutcome",
90+
"id": "exception",
91+
"text": {
92+
"status": "additional",
93+
"div": f'<div xmlns="http://www.w3.org/1999/xhtml">{message}</div>',
94+
},
95+
"issue": [
96+
{
97+
"severity": "error",
98+
"code": "invalid",
99+
"details": {"text": message},
100+
"expression": [expression],
101+
}
102+
],
103+
}
104+
)
105+
106+
84107
class CodeSystemListLookupView(ConceptRetrieveUpdateDestroyView):
85108
serializer_class = ParametersSerializer
86109

@@ -151,6 +174,8 @@ def get_permissions(self):
151174
return [CanViewParentDictionaryAsGuest(), ]
152175

153176
def get_queryset(self):
177+
"""The concept with the code in the code system `url` names. A code system that doesn't resolve, or that the
178+
requester can't view, reads nothing; without a code, or with a code but no url, the request is a 400."""
154179
queryset = super().get_queryset()
155180

156181
parameters = self.get_parameters()
@@ -173,15 +198,17 @@ def get_queryset(self):
173198
if not source:
174199
return queryset.none()
175200

176-
if code:
177-
if not source:
178-
return queryset.none()
201+
if not code:
202+
raise FhirParameterRequiredError('A code to validate is required.', expression='code')
203+
if not source:
204+
raise FhirParameterRequiredError(
205+
'url is required with a code: the code system to validate it in.', expression='url')
179206

180-
queryset = queryset.filter(sources=source, mnemonic=code)
207+
queryset = queryset.filter(sources=source, mnemonic=code)
181208

182-
if display and queryset:
209+
if display:
183210
instance = queryset.first()
184-
if display not in (instance.name, instance.display_name):
211+
if not instance or display not in (instance.name, instance.display_name):
185212
return queryset.none()
186213

187214
return queryset

‎core/concept_maps/views.py‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from pydash import get
77

88
from core.bundles.serializers import FHIRBundleSerializer
9+
from core.code_systems.views import FhirParameterRequiredError
910
from core.common.constants import HEAD
1011
from core.common.fhir_helpers import translate_fhir_query
1112
from core.common.permissions import CanViewConceptDictionary
@@ -81,15 +82,34 @@ def get_permissions(self):
8182
def get_serializer_class(self):
8283
return ConceptMapParametersSerializer
8384

85+
# $translate answers with the translation of the mappings apply_filters names, and nothing else. The mapping
86+
# list's shortcuts (a search, a sort or a faceted filter, facets, search stats, a CSV export) would answer in its
87+
# place, and the search ones skip apply_filters, so each is off. Their parameters are ignored like any other this
88+
# operation doesn't define.
89+
allow_csv_export = False
90+
91+
def should_perform_es_search(self):
92+
return False
93+
94+
def only_facets(self):
95+
return False
96+
97+
def only_search_stats(self):
98+
return False
99+
100+
def should_include_facets(self):
101+
return False
102+
84103
def verify_scope(self):
85104
"""
86105
Override to not verify scope and allow POSTing for everyone
87106
:return: None
88107
"""
89108

90109
def apply_filters(self, queryset):
110+
# get_serializer renders the answer, so a POSTed Parameters resource is read by the serializer class itself
91111
if self.request.method in ['POST', 'PUT']:
92-
parameters = self.get_serializer(data=self.request.data, instance=None)
112+
parameters = self.get_serializer_class()(data=self.request.data)
93113
else:
94114
parameters = self.get_serializer_class().parse_query_params(self.request.query_params)
95115

@@ -104,22 +124,19 @@ def apply_filters(self, queryset):
104124
system = params.get('system')
105125
targetsystem = params.get('targetsystem')
106126
# TODO: implement 'source' and 'target'
127+
if not code:
128+
raise FhirParameterRequiredError('A code to translate is required.', expression='code')
129+
if not system:
130+
raise FhirParameterRequiredError(
131+
'system is required with a code: the code system the code is from.', expression='system')
132+
107133
if url:
108134
queryset = queryset.filter(parent__canonical_url=url)
109-
if not queryset:
110-
return queryset
111-
112-
if code:
113-
queryset = queryset.filter(from_concept_code=code)
114-
if not queryset:
115-
return queryset
116-
if system:
117-
system_url = IdentifierSerializer.convert_fhir_url_to_ocl_uri(system, 'sources')
118-
queryset = queryset.filter(Q(from_source__canonical_url=system) |
119-
Q(from_source_url=system_url) |
120-
Q(from_source__uri=system_url))
121-
if not queryset:
122-
return queryset
135+
queryset = queryset.filter(from_concept_code=code)
136+
system_url = IdentifierSerializer.convert_fhir_url_to_ocl_uri(system, 'sources')
137+
queryset = queryset.filter(Q(from_source__canonical_url=system) |
138+
Q(from_source_url=system_url) |
139+
Q(from_source__uri=system_url))
123140
if targetsystem:
124141
target_url = IdentifierSerializer.convert_fhir_url_to_ocl_uri(targetsystem, 'sources')
125142
queryset = queryset.filter(Q(to_source__canonical_url=targetsystem) |

0 commit comments

Comments
 (0)