Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jkmarx/remove unused igv stuff #2844

Merged
merged 17 commits into from
Jul 9, 2018
116 changes: 23 additions & 93 deletions refinery/core/serializers.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import logging

import celery
from django.conf import settings
from rest_framework import serializers
from rest_framework.validators import UniqueValidator

from data_set_manager.models import Node
from file_store.models import FileStoreItem

from .models import DataSet, Event, UserProfile, Workflow

Expand Down Expand Up @@ -109,100 +107,32 @@ class Meta:
model = Workflow


class NodeSerializer(serializers.HyperlinkedModelSerializer):
assay = serializers.StringRelatedField()
study = serializers.StringRelatedField()
child_nodes = serializers.SerializerMethodField('_get_children')
parent_nodes = serializers.SerializerMethodField('_get_parents')
auxiliary_nodes = serializers.SerializerMethodField('_get_aux_nodes')
auxiliary_file_generation_task_state = serializers.SerializerMethodField(
'_get_aux_node_task_state'
)
file_extension = serializers.SerializerMethodField(
'_get_file_extension')
relative_file_store_item_url = serializers.SerializerMethodField(
'_get_relative_url')
ready_for_igv_detail_view = serializers.SerializerMethodField(
'_ready_for_igv_detail_view')
is_auxiliary_node = serializers.SerializerMethodField(
'_is_auxiliary_node')

def _get_children(self, obj):
return obj.get_children()

def _get_parents(self, obj):
return obj.get_parents()

def _get_aux_nodes(self, obj):
aux_nodes = obj.get_auxiliary_nodes()
urls = []
for uuid in aux_nodes:
try:
node = Node.objects.get(uuid=uuid)
urls.append(node.get_relative_file_store_item_url())
except (Node.DoesNotExist,
Node.MultipleObjectsReturned) as e:
logger.debug(e)
return urls

def _get_aux_node_task_state(self, obj):
return obj.get_auxiliary_file_generation_task_state()

def _get_file_extension(self, obj):
try:
file_store_item = FileStoreItem.objects.get(uuid=obj.file_uuid)
except (FileStoreItem.DoesNotExist,
FileStoreItem.MultipleObjectsReturned) as exc:
logger.debug(exc)
return None
return file_store_item.get_extension()

def _get_relative_url(self, obj):
return obj.get_relative_file_store_item_url() or None

def _ready_for_igv_detail_view(self, obj):
if not obj.is_auxiliary_node:
ready_for_igv_detail_view = True
for item in obj.get_auxiliary_nodes():
try:
node = Node.objects.get(uuid=item)
except (Node.DoesNotExist,
Node.MultipleObjectsReturned) \
as e:
logger.error(e)
return False

state = node.get_auxiliary_file_generation_task_state()
if not state == celery.states.SUCCESS:
ready_for_igv_detail_view = False

return ready_for_igv_detail_view
else:
return None

def _is_auxiliary_node(self, obj):
return obj.is_auxiliary_node
class NodeSerializer(serializers.ModelSerializer):
file_uuid = serializers.CharField(max_length=36,
required=False,
allow_null=True)

class Meta:
model = Node
fields = [
'uuid',
'name',
'type',
'analysis_uuid',
'subanalysis',
'assay',
'study',
'relative_file_store_item_url',
'parent_nodes',
'child_nodes',
'auxiliary_nodes',
'is_auxiliary_node',
'file_extension',
'auxiliary_file_generation_task_state',
'ready_for_igv_detail_view',
'file_uuid'
]
fields = ['uuid', 'file_uuid']
Copy link
Member

Choose a reason for hiding this comment

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

('uuid, 'file_uuid')

example


def validate_file_uuid(self, file_uuid):
if file_uuid is not None:
raise serializers.ValidationError(
'API does not support adding file store uuids.'
)
pass

def partial_update(self, instance, validated_data):
"""
Update and return an existing `Node` instance, given the
validated data.
"""
instance.file_uuid = validated_data.get('file_uuid',
instance.file_uuid)

instance.save()
return instance


class EventSerializer(serializers.ModelSerializer):
Expand Down
3 changes: 2 additions & 1 deletion refinery/core/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@

# DRF url routing
core_router = DefaultRouter()
core_router.register(r'nodes', NodeViewSet)
core_router.register(r'workflows', WorkflowViewSet)
core_router.register(r'events', EventViewSet)
core_router.urls.extend([
Expand All @@ -85,6 +84,8 @@
DataSetsViewSet.as_view()),
url(r'^analyses/(?P<uuid>' + UUID_RE + r')/$',
AnalysesViewSet.as_view()),
url(r'^nodes/(?P<uuid>' + UUID_RE + r')/$',
NodeViewSet.as_view()),
url(r'^openid_token/$',
OpenIDToken.as_view(), name="openid-token")
])
92 changes: 80 additions & 12 deletions refinery/core/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from django.core.mail import EmailMessage
from django.core.urlresolvers import reverse
from django.db import transaction
from django.http import (HttpResponse, HttpResponseBadRequest,
from django.http import (Http404, HttpResponse, HttpResponseBadRequest,
HttpResponseForbidden, HttpResponseNotFound,
HttpResponseRedirect, HttpResponseServerError,
JsonResponse)
Expand All @@ -32,13 +32,16 @@
from requests.exceptions import HTTPError
from rest_framework import authentication, status, viewsets
from rest_framework.decorators import detail_route
from rest_framework.exceptions import APIException
from rest_framework.permissions import IsAuthenticated
from rest_framework.renderers import JSONRenderer
from rest_framework.response import Response
from rest_framework.views import APIView
import xmltodict

from data_set_manager.models import Node
from data_set_manager.search_indexes import NodeIndex
from file_store.models import FileStoreItem

from .forms import ProjectForm, UserForm, UserProfileForm, WorkflowForm
from .models import (Analysis, CustomRegistrationProfile, DataSet, Event,
Expand Down Expand Up @@ -693,13 +696,80 @@ def graph(self, request, *args, **kwargs):
)


class NodeViewSet(viewsets.ModelViewSet):
Copy link
Member

Choose a reason for hiding this comment

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

What was the need for the ModelViewSet to APIView change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I was more so curious about the underlying need to switch from one to the other

"""API endpoint that allows Nodes to be viewed"""
queryset = Node.objects.all()
serializer_class = NodeSerializer
lookup_field = 'uuid'
http_method_names = ['get']
# permission_classes = (IsAuthenticated,)
class NodeViewSet(APIView):
Copy link
Member

Choose a reason for hiding this comment

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

Missing authentication_classes and permission_classes attributes.

"""API endpoint that allows Nodes to be viewed".
---
#YAML

PATCH:
parameters_strategy:
form: replace
query: merge

parameters:
- name: uuid
description: User profile uuid used as an identifier
type: string
paramType: path
required: true
- name: file_uuid
description: uuid for file store item
type: string
paramType: form
required: false
...
"""
http_method_names = ['get', 'patch']
Copy link
Member

Choose a reason for hiding this comment

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

The recommended approach to restrict API methods is to inherit from GenericViewSet and add appropriate mixins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a viewset.


def update_file_store(self, old_uuid, new_uuid):
Copy link
Member

Choose a reason for hiding this comment

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

This method contains very simple logic and is used only once, so does not need to be factored out.

if new_uuid is None:
Copy link
Member

Choose a reason for hiding this comment

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

This check should not be needed since validate_file_uuid() in serializer already handles validation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though the api is only supporting file_uuid deletion, this is needed to avoid future issues when the file_uuid is not changed and other fields are.

try:
FileStoreItem.objects.get(uuid=old_uuid).delete_datafile()
Copy link
Member

Choose a reason for hiding this comment

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

Try block should be as simple as possible, so delete_datafile() should be moved out.

except (FileStoreItem.DoesNotExist,
FileStoreItem.MultipleObjectsReturned) as e:
logger.error(e)

def get_object(self, uuid):
try:
return Node.objects.get(uuid=uuid)
except Node.DoesNotExist as e:
logger.error(e)
raise Http404
except Node.MultipleObjectsReturned as e:
logger.error(e)
raise APIException("Multiple objects returned.")

def get(self, request, uuid):
node = self.get_object(uuid)
data_set = node.study.get_dataset()
public_group = ExtendedGroup.objects.public_group()

if request.user.has_perm('core.read_dataset', data_set) or \
'read_dataset' in get_perms(public_group, data_set):
Copy link
Member

Choose a reason for hiding this comment

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

DRF docs recommend using a custom permission class

serializer = NodeSerializer(node)
return Response(serializer.data)

return Response(node, status=status.HTTP_401_UNAUTHORIZED)
Copy link
Member

Choose a reason for hiding this comment

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

Returning a Node instance contradicts HTTP 401 status code.


def patch(self, request, uuid):
node = self.get_object(uuid)
old_file_uuid = node.file_uuid
data_set_owner = node.study.get_dataset().get_owner()

if data_set_owner == request.user:
Copy link
Member

Choose a reason for hiding this comment

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

DRF docs recommend using a custom permission class

serializer = NodeSerializer(node, data=request.data, partial=True)
if serializer.is_valid():
serializer.save()
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be the last action contingent on success of data file deletion and Solr index updates.

self.update_file_store(old_file_uuid, node.file_uuid)
NodeIndex().update_object(node, using="data_set_manager")
return Response(
serializer.data, status=status.HTTP_202_ACCEPTED
)
return Response(
serializer.errors, status=status.HTTP_400_BAD_REQUEST
)

return Response(uuid, status=status.HTTP_401_UNAUTHORIZED)


class EventViewSet(viewsets.ModelViewSet):
Expand Down Expand Up @@ -800,12 +870,10 @@ def get_object(self, uuid):
return DataSet.objects.get(uuid=uuid)
except DataSet.DoesNotExist as e:
logger.error(e)
return Response(uuid, status=status.HTTP_404_NOT_FOUND)
raise Http404
except DataSet.MultipleObjectsReturned as e:
logger.error(e)
return Response(
uuid, status=status.HTTP_500_INTERNAL_SERVER_ERROR
)
raise APIException("Multiple objects returned.")

def is_user_authorized(self, user, data_set):
if (not user.is_authenticated() or
Expand Down