-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add RuntimeJob model #1177
Add RuntimeJob model #1177
Conversation
Signed-off-by: Akihiko Kuroda <[email protected]>
couple of nits on the API:
|
@psschwei Thanks for review. I'll update them to
add: add runtime job id to the middleware job. |
Thinking about this in the context of #1164 : would these APIs make sense as a subfunction of jobs, i.e. something like WDYT? I don't have strong opinions here, just know that changing an API once it's out there is not the most fun thing in the world 😄 |
gateway/api/v1/views.py
Outdated
|
||
queryset = RuntimeJob.objects.all() | ||
serializer_class = v1_serializers.RuntimeJobSerializer | ||
permission_classes = [permissions.IsAuthenticated, IsOwner] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsOwner
might not work here because it is checking owner
or author
field of object. But RuntimeJob
does not have that.
We can modify IsOwner
permission class a little bit to act differently based on object type. Something like
# Instaed of this
class IsOwner(permissions.BasePermission):
"""
Custom permission to only allow owners of an object to edit it.
"""
def has_object_permission(self, request, view, obj):
return obj.author == request.user
# do something like
class IsOwner(permissions.BasePermission):
"""
Custom permission to only allow owners of an object to edit it.
"""
def has_object_permission(self, request, view, obj):
if isinstance(obj, RuntimeJob):
return obj.job.author == request.user
return obj.author == request.user
gateway/api/views.py
Outdated
@@ -468,3 +472,41 @@ def upload(self, request): # pylint: disable=invalid-name | |||
destination.write(chunk) | |||
return Response({"message": file_path}) | |||
return Response("server error", status=status.HTTP_500_INTERNAL_SERVER_ERROR) | |||
|
|||
|
|||
class RuntimeJobViewSet(viewsets.ModelViewSet): # pylint: disable=too-many-ancestors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use here standart django model viewset and it will give us CRUD on RuntimeJob
object.
Or alternatevely, if we want to keep this view, we should use RuntimeJobSerializer
to validate requests.
serializer = RuntimeJobSerializer(data=request.data)
serializer.is_valid(raise_exception=True) # or better handle exception and return response with explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IceKhan13 I thought these APIs are convenient but both APIs can be replaces by one CRUD API call. I'll take it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IceKhan13 On the second thought, the API getting the list of runtime jobs for the middleware job may may be useful when the RuntimeJob table gets huge. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was about using default django api view. For RuntimeJob
model, which gives all crud with validation out of the box.
https://www.django-rest-framework.org/api-guide/viewsets/#modelviewset
Basically
class RuntimeJobViewSet(viewsets.ModelViewSet):
serializer_class = RuntimeJobSerializer
permission_classes = [...]
def get_queryset(self):
return RuntimeJob.object.filter(job__author=self.request.user)
... # add here insert if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But let's merge this PR and add API view in separate one.
I look out APIs. This PR basically has model change only. |
gateway/api/permissions.py
Outdated
if isinstance(obj, RuntimeJob): | ||
return obj.job.author == request.user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this left over from the API part that was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I take out. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this PR and add API view for RuntimeJob
in separate PR
Thank you, Aki!
Summary
Fix #1163
Details and comments
This PR has