dealing with query param issues and class structure

Author: clewellyn-nava

apps/fhir/bluebutton/views/generic.py

@@ -51,7 +51,8 @@ class FhirDataView(APIView):
         ApplicationActivePermission,
         HasCrosswalk,
         ResourcePermission,
-        DataAccessGrantPermission]
+        DataAccessGrantPermission
+    ]
 
     def __init__(self, version=1):
         self.version = version

apps/fhir/bluebutton/views/insurancecard.py

@@ -41,11 +41,8 @@ def has_permission(self, request, view):
         return request.user.is_authenticated and hasattr(request.user, 'crosswalk')
 
     def build_parameters(self, request):
-        patient_id = request.query_params.get('patient', None)
-        if not patient_id:
-            patient_id = request.user.crosswalk.fhir_id
         return {
-            '_format': 'application/json+fhir'
+            '_format': 'application/json'
         }
 
     def build_url(self, fhir_settings, resource_type, resource_id=None, *args, **kwargs):

apps/fhir/bluebutton/views/insurancecard_viewset.py

@@ -1,6 +1,4 @@
 from rest_framework import viewsets, permissions
-from rest_framework.response import Response
-
 from apps.fhir.bluebutton.views.generic import FhirDataView
 from apps.authorization.permissions import DataAccessGrantPermission
 from apps.capabilities.permissions import TokenHasProtectedCapability
@@ -47,17 +45,6 @@ def __init__(self, version=1, **kwargs):
     def initial(self, request, *args, **kwargs):
         return super().initial(request, self.resource_type, *args, **kwargs)
 
-    def list(self, request, *args, **kwargs):
-        '''Equivalent to get() in FhirDataView'''
-        out = self.fetch_data(request, self.resource_type, *args, **kwargs)
-        return Response(out)
-
-    def build_parameters(self, request):
-        patient_id = request.query_params.get('patient', None)
-        if not patient_id:
-            patient_id = request.user.crosswalk.fhir_id
-        return {'_format': 'application/json+fhir'}
-
     def build_url(self, fhir_settings, resource_type, resource_id=None, *args, **kwargs):
         if fhir_settings.fhir_url.endswith('v1/fhir/'):
             # only if called by tests

apps/fhir/bluebutton/views/patient_viewset.py

@@ -1,7 +1,4 @@
-from rest_framework import viewsets, permissions
-from rest_framework.response import Response
-
-from apps.fhir.bluebutton.views.generic import FhirDataView
+from apps.fhir.bluebutton.views.viewsets_base import ResourceViewSet
 from apps.fhir.bluebutton.views.search import HasSearchScope, SearchView
 from apps.authorization.permissions import DataAccessGrantPermission
 from apps.capabilities.permissions import TokenHasProtectedCapability
@@ -11,9 +8,10 @@
     ResourcePermission,
     ApplicationActivePermission,
 )
+from rest_framework import permissions
 
 
-class PatientViewSet(FhirDataView, viewsets.ViewSet):
+class PatientViewSet(ResourceViewSet):
     """Patient django-rest-framework ViewSet experiment
 
     Args:
@@ -24,51 +22,31 @@ class PatientViewSet(FhirDataView, viewsets.ViewSet):
     version = 1
     resource_type = 'Patient'
 
-    QUERY_TRANSFORMS = getattr(SearchView, 'QUERY_TRANSFORMS', {})
-    QUERY_SCHEMA = {**getattr(SearchView, 'QUERY_SCHEMA', {}), '_id': str, 'identifier': str}
-
-    required_scopes = ['patient/Patient.read', 'patient/Patient.rs', 'patient/Patient.s']
-
-    def __init__(self, version=1, **kwargs):
-        super().__init__(version)
+    # TODO - I don't love the separation here, could be indicative that we don't want to move to resource based ViewSets, or that
+    # we need a better base class, or these differences should be defined in PatientViewSet.
+    SEARCH_QUERY_TRANSFORMS = getattr(SearchView, 'QUERY_TRANSFORMS', {})
+    SEARCH_QUERY_SCHEMA = {**getattr(SearchView, 'QUERY_SCHEMA', {}), '_id': str, 'identifier': str}
 
-    def initial(self, request, *args, **kwargs):
-        return super().initial(request, self.resource_type, *args, **kwargs)
+    SEARCH_PERMISSION_CLASSES = (
+        permissions.IsAuthenticated,
+        ApplicationActivePermission,
+        ResourcePermission,
+        SearchCrosswalkPermission,
+        DataAccessGrantPermission,
+        TokenHasProtectedCapability,
+        HasSearchScope,
+    )
 
-    def get_permissions(self):
-        action = getattr(self, 'action', None)
-        if action == 'list':
-            perm_classes = [
-                permissions.IsAuthenticated,
-                ApplicationActivePermission,
-                ResourcePermission,
-                SearchCrosswalkPermission,
-                DataAccessGrantPermission,
-                TokenHasProtectedCapability,
-                HasSearchScope,
-            ]
-        else:
-            perm_classes = [
-                permissions.IsAuthenticated,
-                ApplicationActivePermission,
-                ResourcePermission,
-                ReadCrosswalkPermission,
-                DataAccessGrantPermission,
-                TokenHasProtectedCapability,
-            ]
-        return [p() for p in perm_classes]
-
-    def list(self, request, *args, **kwargs):
-        '''Equivalent to get()/search in FhirDataView'''
-        out = self.fetch_data(request, self.resource_type, *args, **kwargs)
-        return Response(out)
+    READ_PERMISSION_CLASSES = (
+        permissions.IsAuthenticated,
+        ApplicationActivePermission,
+        ResourcePermission,
+        ReadCrosswalkPermission,
+        DataAccessGrantPermission,
+        TokenHasProtectedCapability,
+    )
 
-    def retrieve(self, request, resource_id=None, *args, **kwargs):
-        out = self.fetch_data(request, self.resource_type, resource_id=resource_id, *args, **kwargs)
-        return Response(out)
-
-    def build_parameters(self, request):
-        return {'_format': 'application/json+fhir'}
+    required_scopes = ['patient/Patient.read', 'patient/Patient.rs', 'patient/Patient.s']
 
     def build_url(self, fhir_settings, resource_type, resource_id=None, *args, **kwargs):
         if fhir_settings.fhir_url.endswith('v1/fhir/'):

apps/fhir/bluebutton/views/viewsets_base.py

@@ -0,0 +1,73 @@
+from rest_framework import viewsets, permissions
+from rest_framework.response import Response
+from voluptuous import Schema, REMOVE_EXTRA
+
+from apps.fhir.bluebutton.views.generic import FhirDataView
+
+
+class ResourceViewSet(FhirDataView, viewsets.ViewSet):
+    """Base FHIR resource ViewSet, would replace FhirDataView if we decide to go in that direction
+
+    Args:
+        FhirDataView: Base mixin, unchanged
+        viewsets: django-rest-framework ViewSet base class
+    """
+
+    resource_type = None
+
+    SEARCH_PERMISSION_CLASSES = (permissions.IsAuthenticated,)
+    READ_PERMISSION_CLASSES = (permissions.IsAuthenticated,)
+
+    def initial(self, request, *args, **kwargs):
+        return super().initial(request, self.resource_type, *args, **kwargs)
+
+    def get_permissions(self):
+        action = getattr(self, 'action', None)
+        if action == 'list':
+            permission_classes = getattr(self, 'SEARCH_PERMISSION_CLASSES', self.SEARCH_PERMISSION_CLASSES)
+        elif action == 'retrieve':
+            permission_classes = getattr(self, 'READ_PERMISSION_CLASSES', self.READ_PERMISSION_CLASSES)
+        else:
+            permission_classes = (permissions.IsAuthenticated,)
+        return [permission_class() for permission_class in permission_classes]
+
+    def list(self, request, *args, **kwargs):
+        out = self.fetch_data(request, self.resource_type, *args, **kwargs)
+        return Response(out)
+
+    def retrieve(self, request, resource_id=None, *args, **kwargs):
+        out = self.fetch_data(request, self.resource_type, resource_id=resource_id, *args, **kwargs)
+        return Response(out)
+
+    # A lot of this is copied (haphazardly) from generic, and the names and handling of the schema could be cleaned up
+    def get_query_schema(self):
+        if getattr(self, 'action', None) == 'list':
+            return getattr(self, 'SEARCH_QUERY_SCHEMA', getattr(self, 'QUERY_SCHEMA', {}))
+        return {}
+
+    def get_query_transforms(self):
+        if getattr(self, 'action', None) == 'list':
+            return getattr(self, 'SEARCH_QUERY_TRANSFORMS', getattr(self, 'QUERY_TRANSFORMS', {}))
+        return getattr(self, 'QUERY_TRANSFORMS', {})
+
+    def map_parameters(self, params):
+        transforms = self.get_query_transforms()
+        for key, correct in transforms.items():
+            val = params.pop(key, None)
+            if val is not None:
+                params[correct] = val
+        return params
+
+    def filter_parameters(self, request):
+        if getattr(self, 'action', None) != 'list':
+            return {}
+
+        params = self.map_parameters(request.query_params.dict())
+        params['_lastUpdated'] = request.query_params.getlist('_lastUpdated')
+
+        schema = Schema(self.get_query_schema(), extra=REMOVE_EXTRA)
+        return schema(params)
+
+    # TODO - investigate if this is needed, or if we can assume application/json+fhir everywhere
+    def build_parameters(self, request):
+        return {'_format': 'application/json+fhir'}