[Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects

Paolo Bonzini posted 9 patches 6 years, 8 months ago
[Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects
Posted by Paolo Bonzini 6 years, 8 months ago
From: Shubham Jain <shubhamjain7495@gmail.com>

This lets one use the REST API for the functionality of the old testing-report
and applier-report endpoints.

Signed-off-by: Shubham Jain <shubhamjain7495@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 api/rest.py     | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 mods/git.py     | 12 +++++++++++-
 mods/testing.py | 10 +++++++++-
 3 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/api/rest.py b/api/rest.py
index df0640a..22d7c98 100644
--- a/api/rest.py
+++ b/api/rest.py
@@ -25,6 +25,9 @@ from rest_framework.response import Response
 import rest_framework
 from mbox import addr_db_to_rest, MboxMessage
 from rest_framework.parsers import JSONParser, BaseParser
+from rest_framework.exceptions import ValidationError
+import re
+import mod
 
 SEARCH_PARAM = 'q'
 
@@ -54,6 +57,12 @@ class PatchewPermission(permissions.BasePermission):
                 return True
         return False
 
+    def has_result_group_permission(self, request, view, result_renderer):
+        for grp in request.user.groups.all():
+            if grp.name in result_renderer.allowed_groups:
+                return True
+        return False
+
     def has_generic_permission(self, request, view):
         return (request.method in permissions.SAFE_METHODS) or \
                self.is_superuser(request) or \
@@ -62,7 +71,9 @@ class PatchewPermission(permissions.BasePermission):
     def has_permission(self, request, view):
         return self.has_generic_permission(request, view) or \
                (hasattr(view, 'project') and view.project and \
-                self.has_project_permission(request, view, view.project))
+                self.has_project_permission(request, view, view.project)) or \
+               (hasattr(view, 'result_renderer') and view.result_renderer and \
+                self.has_result_group_permission(request, view, view.result_renderer))
 
     def has_object_permission(self, request, view, obj):
         # For non-project objects, has_project_permission has been evaluated
@@ -478,6 +489,7 @@ class ResultSerializer(serializers.ModelSerializer):
     class Meta:
         model = Result
         fields = ('resource_uri', 'name', 'status', 'last_update', 'data', 'log_url')
+        read_only_fields = ('name', 'last_update', )
 
     resource_uri = HyperlinkedResultField(view_name='results-detail')
     log_url = SerializerMethodField(required=False)
@@ -487,21 +499,54 @@ class ResultSerializer(serializers.ModelSerializer):
         request = self.context['request']
         return obj.get_log_url(request)
 
+    def validate(self, data):
+        data_serializer_class = self.context['renderer'].result_data_serializer_class
+        data_serializer_class(data=data['data'],
+                              context=self.context).is_valid(raise_exception=True)
+        return data
+
 class ResultSerializerFull(ResultSerializer):
     class Meta:
         model = Result
         fields = ResultSerializer.Meta.fields + ('log',)
+        read_only_fields = ResultSerializer.Meta.read_only_fields
 
     # The database field is log_xz, so this is needed here
     log = CharField(required=False)
 
 class ResultsViewSet(mixins.ListModelMixin, mixins.RetrieveModelMixin,
-                         viewsets.GenericViewSet):
+                     mixins.UpdateModelMixin, viewsets.GenericViewSet):
     lookup_field = 'name'
     lookup_value_regex = '[^/]+'
     filter_backends = (filters.OrderingFilter,)
     ordering_fields = ('name',)
     ordering = ('name',)
+    permission_classes = (PatchewPermission, )
+
+    # for permissions
+    @property
+    def project(self):
+        if hasattr(self, '__project'):
+            return self.__project
+        try:
+            self.__project = Project.objects.get(id=self.kwargs['projects_pk'])
+        except:
+            self.__project = None
+        return self.__project
+
+    @property
+    def result_renderer(self):
+        try:
+            found = re.match("^[^.]*", self.kwargs['name'])
+        except:
+            return None
+        return mod.get_module(found.group(0)) if found else None
+
+    def get_serializer_context(self):
+        context = super(ResultsViewSet, self).get_serializer_context()
+        if 'name' in self.kwargs:
+            context['renderer'] = self.result_renderer
+        return context
 
     def get_serializer_class(self, *args, **kwargs):
         if self.lookup_field in self.kwargs:
diff --git a/mods/git.py b/mods/git.py
index 97db081..267546a 100644
--- a/mods/git.py
+++ b/mods/git.py
@@ -23,6 +23,8 @@ from api.rest import PluginMethodField, reverse_detail
 from api.views import APILoginRequiredView, prepare_series
 from patchew.logviewer import LogView
 from schema import *
+from rest_framework import serializers
+from rest_framework.fields import CharField
 
 _instance = None
 
@@ -42,11 +44,19 @@ class GitLogViewer(LogView):
             raise Http404("Object not found: " + series)
         return obj.git_result
 
+class DataSerializer(serializers.Serializer):
+    base = CharField()
+
+    # TODO: should be present iff the result is a success
+    repo = CharField(required=False)
+    url = CharField(required=False)
+    tag = CharField(required=False)
 
 class GitModule(PatchewModule):
     """Git module"""
     name = "git"
-
+    allowed_groups = ('importers', )
+    result_data_serializer_class = DataSerializer
     project_property_schema = \
         ArraySchema("git", desc="Configuration for git module",
                     members=[
diff --git a/mods/testing.py b/mods/testing.py
index ecd63d1..98216d4 100644
--- a/mods/testing.py
+++ b/mods/testing.py
@@ -25,6 +25,8 @@ from api.search import SearchEngine
 from event import emit_event, declare_event, register_handler
 from patchew.logviewer import LogView
 from schema import *
+from rest_framework import serializers
+from rest_framework.fields import CharField, BooleanField
 
 _instance = None
 
@@ -47,12 +49,18 @@ class TestingLogViewer(LogView):
             raise Http404("Object not found: " + project_or_series)
         return _instance.get_testing_result(obj, testing_name)
 
+class DataSerializer(serializers.Serializer):
+    # TODO: is_timeout should be present iff the result is a failure
+    is_timeout = BooleanField(required=False)
+    head = CharField()
+    tester = CharField(default=serializers.CurrentUserDefault())
 
 class TestingModule(PatchewModule):
     """Testing module"""
 
     name = "testing"
-
+    allowed_groups = ('testers', )
+    result_data_serializer_class = DataSerializer
     test_schema = \
         ArraySchema("{name}", "Test", desc="Test spec",
                     members=[
-- 
2.17.1


_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects
Posted by Fam Zheng 6 years, 8 months ago
On Sat, 08/18 16:10, Paolo Bonzini wrote:
> From: Shubham Jain <shubhamjain7495@gmail.com>
> 
> This lets one use the REST API for the functionality of the old testing-report
> and applier-report endpoints.
> 
> Signed-off-by: Shubham Jain <shubhamjain7495@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  api/rest.py     | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
>  mods/git.py     | 12 +++++++++++-
>  mods/testing.py | 10 +++++++++-
>  3 files changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/api/rest.py b/api/rest.py
> index df0640a..22d7c98 100644
> --- a/api/rest.py
> +++ b/api/rest.py
> @@ -25,6 +25,9 @@ from rest_framework.response import Response
>  import rest_framework
>  from mbox import addr_db_to_rest, MboxMessage
>  from rest_framework.parsers import JSONParser, BaseParser
> +from rest_framework.exceptions import ValidationError
> +import re
> +import mod
>  
>  SEARCH_PARAM = 'q'
>  
> @@ -54,6 +57,12 @@ class PatchewPermission(permissions.BasePermission):
>                  return True
>          return False
>  
> +    def has_result_group_permission(self, request, view, result_renderer):
> +        for grp in request.user.groups.all():
> +            if grp.name in result_renderer.allowed_groups:
> +                return True
> +        return False
> +
>      def has_generic_permission(self, request, view):
>          return (request.method in permissions.SAFE_METHODS) or \
>                 self.is_superuser(request) or \
> @@ -62,7 +71,9 @@ class PatchewPermission(permissions.BasePermission):
>      def has_permission(self, request, view):
>          return self.has_generic_permission(request, view) or \
>                 (hasattr(view, 'project') and view.project and \
> -                self.has_project_permission(request, view, view.project))
> +                self.has_project_permission(request, view, view.project)) or \
> +               (hasattr(view, 'result_renderer') and view.result_renderer and \
> +                self.has_result_group_permission(request, view, view.result_renderer))

This is barely readable now...

>  
>      def has_object_permission(self, request, view, obj):
>          # For non-project objects, has_project_permission has been evaluated
> @@ -478,6 +489,7 @@ class ResultSerializer(serializers.ModelSerializer):
>      class Meta:
>          model = Result
>          fields = ('resource_uri', 'name', 'status', 'last_update', 'data', 'log_url')
> +        read_only_fields = ('name', 'last_update', )
>  
>      resource_uri = HyperlinkedResultField(view_name='results-detail')
>      log_url = SerializerMethodField(required=False)
> @@ -487,21 +499,54 @@ class ResultSerializer(serializers.ModelSerializer):
>          request = self.context['request']
>          return obj.get_log_url(request)
>  
> +    def validate(self, data):
> +        data_serializer_class = self.context['renderer'].result_data_serializer_class
> +        data_serializer_class(data=data['data'],
> +                              context=self.context).is_valid(raise_exception=True)
> +        return data
> +
>  class ResultSerializerFull(ResultSerializer):
>      class Meta:
>          model = Result
>          fields = ResultSerializer.Meta.fields + ('log',)
> +        read_only_fields = ResultSerializer.Meta.read_only_fields
>  
>      # The database field is log_xz, so this is needed here
>      log = CharField(required=False)
>  
>  class ResultsViewSet(mixins.ListModelMixin, mixins.RetrieveModelMixin,
> -                         viewsets.GenericViewSet):
> +                     mixins.UpdateModelMixin, viewsets.GenericViewSet):
>      lookup_field = 'name'
>      lookup_value_regex = '[^/]+'
>      filter_backends = (filters.OrderingFilter,)
>      ordering_fields = ('name',)
>      ordering = ('name',)
> +    permission_classes = (PatchewPermission, )
> +
> +    # for permissions
> +    @property
> +    def project(self):
> +        if hasattr(self, '__project'):
> +            return self.__project
> +        try:
> +            self.__project = Project.objects.get(id=self.kwargs['projects_pk'])
> +        except:
> +            self.__project = None
> +        return self.__project
> +
> +    @property
> +    def result_renderer(self):
> +        try:
> +            found = re.match("^[^.]*", self.kwargs['name'])

Maybe create a Result.mod field so this magic is not necessary? Otherwise the
convention should be documented somewhere.

> +        except:
> +            return None
> +        return mod.get_module(found.group(0)) if found else None
> +
> +    def get_serializer_context(self):
> +        context = super(ResultsViewSet, self).get_serializer_context()
> +        if 'name' in self.kwargs:
> +            context['renderer'] = self.result_renderer
> +        return context
>  
>      def get_serializer_class(self, *args, **kwargs):
>          if self.lookup_field in self.kwargs:
> diff --git a/mods/git.py b/mods/git.py
> index 97db081..267546a 100644
> --- a/mods/git.py
> +++ b/mods/git.py
> @@ -23,6 +23,8 @@ from api.rest import PluginMethodField, reverse_detail
>  from api.views import APILoginRequiredView, prepare_series
>  from patchew.logviewer import LogView
>  from schema import *
> +from rest_framework import serializers
> +from rest_framework.fields import CharField
>  
>  _instance = None
>  
> @@ -42,11 +44,19 @@ class GitLogViewer(LogView):
>              raise Http404("Object not found: " + series)
>          return obj.git_result
>  
> +class DataSerializer(serializers.Serializer):
> +    base = CharField()
> +
> +    # TODO: should be present iff the result is a success
> +    repo = CharField(required=False)
> +    url = CharField(required=False)
> +    tag = CharField(required=False)
>  
>  class GitModule(PatchewModule):
>      """Git module"""
>      name = "git"
> -
> +    allowed_groups = ('importers', )
> +    result_data_serializer_class = DataSerializer
>      project_property_schema = \
>          ArraySchema("git", desc="Configuration for git module",
>                      members=[
> diff --git a/mods/testing.py b/mods/testing.py
> index ecd63d1..98216d4 100644
> --- a/mods/testing.py
> +++ b/mods/testing.py
> @@ -25,6 +25,8 @@ from api.search import SearchEngine
>  from event import emit_event, declare_event, register_handler
>  from patchew.logviewer import LogView
>  from schema import *
> +from rest_framework import serializers
> +from rest_framework.fields import CharField, BooleanField
>  
>  _instance = None
>  
> @@ -47,12 +49,18 @@ class TestingLogViewer(LogView):
>              raise Http404("Object not found: " + project_or_series)
>          return _instance.get_testing_result(obj, testing_name)
>  
> +class DataSerializer(serializers.Serializer):
> +    # TODO: is_timeout should be present iff the result is a failure
> +    is_timeout = BooleanField(required=False)
> +    head = CharField()
> +    tester = CharField(default=serializers.CurrentUserDefault())
>  
>  class TestingModule(PatchewModule):
>      """Testing module"""
>  
>      name = "testing"
> -
> +    allowed_groups = ('testers', )
> +    result_data_serializer_class = DataSerializer
>      test_schema = \
>          ArraySchema("{name}", "Test", desc="Test spec",
>                      members=[
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel

Fam

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects
Posted by Paolo Bonzini 6 years, 8 months ago
On 22/08/2018 08:14, Fam Zheng wrote:
>> +    @property
>> +    def result_renderer(self):
>> +        try:
>> +            found = re.match("^[^.]*", self.kwargs['name'])
> Maybe create a Result.mod field so this magic is not necessary? Otherwise the
> convention should be documented somewhere.
> 

Do you mean something like Result.renderer_from_name (a staticmethod)?

Thanks,

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects
Posted by Fam Zheng 6 years, 8 months ago
On Wed, 08/22 10:35, Paolo Bonzini wrote:
> On 22/08/2018 08:14, Fam Zheng wrote:
> >> +    @property
> >> +    def result_renderer(self):
> >> +        try:
> >> +            found = re.match("^[^.]*", self.kwargs['name'])
> > Maybe create a Result.mod field so this magic is not necessary? Otherwise the
> > convention should be documented somewhere.
> > 
> 
> Do you mean something like Result.renderer_from_name (a staticmethod)?

I mean can we somehow match the self.kwargs['name'] string back to DB record(s),
where we can create a new field that "duplicates" the first part of the name
(the part before the first "." as matched in this line)?

Fam

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects
Posted by Paolo Bonzini 6 years, 8 months ago
On 22/08/2018 10:52, Fam Zheng wrote:
> On Wed, 08/22 10:35, Paolo Bonzini wrote:
>> On 22/08/2018 08:14, Fam Zheng wrote:
>>>> +    @property
>>>> +    def result_renderer(self):
>>>> +        try:
>>>> +            found = re.match("^[^.]*", self.kwargs['name'])
>>> Maybe create a Result.mod field so this magic is not necessary? Otherwise the
>>> convention should be documented somewhere.
>>>
>>
>> Do you mean something like Result.renderer_from_name (a staticmethod)?
> 
> I mean can we somehow match the self.kwargs['name'] string back to DB record(s),
> where we can create a new field that "duplicates" the first part of the name
> (the part before the first "." as matched in this line)?

I think you don't have the Result object yet, but we can do something 
like this to avoid the code duplication:

diff --git a/api/models.py b/api/models.py
index eea924f..32e8b41 100644
--- a/api/models.py
+++ b/api/models.py
@@ -84,10 +84,14 @@ class Result(models.Model):
         emit_event("ResultUpdate", obj=self.obj,
                    old_status=old_status, result=self)
 
+    @staticmethod
+    def renderer_from_name(name):
+        found = re.match("^[^.]*", name)
+        return mod.get_module(found.group(0)) if found else None
+
     @property
     def renderer(self):
-        found = re.match("^[^.]*", self.name)
-        return mod.get_module(found.group(0)) if found else None
+        return Result.renderer_from_name(self.name)
 
     @property
     def obj(self):
diff --git a/api/rest.py b/api/rest.py
index 22d7c98..cc881af 100644
--- a/api/rest.py
+++ b/api/rest.py
@@ -536,11 +536,9 @@ class ResultsViewSet(mixins.ListModelMixin, mixins.RetrieveModelMixin,
 
     @property
     def result_renderer(self):
-        try:
-            found = re.match("^[^.]*", self.kwargs['name'])
-        except:
-            return None
-        return mod.get_module(found.group(0)) if found else None
+        if 'name' in self.kwargs:
+            return Result.renderer_from_name(self.kwargs['name'])
+        return None
 
     def get_serializer_context(self):
         context = super(ResultsViewSet, self).get_serializer_context()

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH 7/9] rest: add support for PUT of result objects
Posted by Fam Zheng 6 years, 8 months ago
On Wed, 08/22 11:38, Paolo Bonzini wrote:
> On 22/08/2018 10:52, Fam Zheng wrote:
> > On Wed, 08/22 10:35, Paolo Bonzini wrote:
> >> On 22/08/2018 08:14, Fam Zheng wrote:
> >>>> +    @property
> >>>> +    def result_renderer(self):
> >>>> +        try:
> >>>> +            found = re.match("^[^.]*", self.kwargs['name'])
> >>> Maybe create a Result.mod field so this magic is not necessary? Otherwise the
> >>> convention should be documented somewhere.
> >>>
> >>
> >> Do you mean something like Result.renderer_from_name (a staticmethod)?
> > 
> > I mean can we somehow match the self.kwargs['name'] string back to DB record(s),
> > where we can create a new field that "duplicates" the first part of the name
> > (the part before the first "." as matched in this line)?
> 
> I think you don't have the Result object yet, but we can do something 
> like this to avoid the code duplication:
> 
> diff --git a/api/models.py b/api/models.py
> index eea924f..32e8b41 100644
> --- a/api/models.py
> +++ b/api/models.py
> @@ -84,10 +84,14 @@ class Result(models.Model):
>          emit_event("ResultUpdate", obj=self.obj,
>                     old_status=old_status, result=self)
>  
> +    @staticmethod
> +    def renderer_from_name(name):
> +        found = re.match("^[^.]*", name)
> +        return mod.get_module(found.group(0)) if found else None
> +
>      @property
>      def renderer(self):
> -        found = re.match("^[^.]*", self.name)
> -        return mod.get_module(found.group(0)) if found else None
> +        return Result.renderer_from_name(self.name)
>  
>      @property
>      def obj(self):
> diff --git a/api/rest.py b/api/rest.py
> index 22d7c98..cc881af 100644
> --- a/api/rest.py
> +++ b/api/rest.py
> @@ -536,11 +536,9 @@ class ResultsViewSet(mixins.ListModelMixin, mixins.RetrieveModelMixin,
>  
>      @property
>      def result_renderer(self):
> -        try:
> -            found = re.match("^[^.]*", self.kwargs['name'])
> -        except:
> -            return None
> -        return mod.get_module(found.group(0)) if found else None
> +        if 'name' in self.kwargs:
> +            return Result.renderer_from_name(self.kwargs['name'])
> +        return None
>  
>      def get_serializer_context(self):
>          context = super(ResultsViewSet, self).get_serializer_context()

Yes, that is fine then.

Fam

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel