[Patchew-devel] [PATCH] PUT for result

[Paolo Bonzini]

On 30/07/2018 05:44, Shubham Jain wrote:
> The PUT request is throwing 400 Bad request error right now, when I
> tried a sample dictionary object created manually, it was going into a
> never ending loop. Am I missing something here?

Maybe, see review below.

> Also about the tests, what should be tested the user has the
> permission to PUT or something else too?

Yes, testing for permissions is always a good idea.  After you add the
validation step, you can add tests with invalid data, and check that
they fail.

> 
> +    def get_queryset(self):
> +        queryset = Project.objects.all().order_by('id')
> +        name = self.request.query_params.get('name', None)
> +        if name is not None:
> +            return Project.objects.filter(name=name).order_by('id')
> +        return queryset
> +

Looks good.  There is one alternative that I learnt about, which is to
add a /api/v1/projects/by-name/XXX/ view that redirects to
/api/v1/projects/ID/.

If you want you can look into that too; you can read about it at
http://soabits.blogspot.com/2013/10/url-structures-and-hyper-media-for-web.html
(see under "Natural keys, surrogate keys, URL aliases and resource
duplication").

The page says to use HTTP status "303 See Other", but really "307
Temporary Redirect" is better because 307 guarantees that the HTTP
method is not changed.  This way, you can do a PUT to
/api/v1/projects/by-name/XXX/ and it will be redirected to another PUT.
Instead, 303 tells the client to use GET in the second request.

> +                url =  pr['results'][0]['results']+ test_name + "/"
> +                json_data = {"resource_uri" : url,
> +                             "name": test_name,
> +                             "status": str(passed),

This is true/false, but the right value for status here is one of
"success" or "failure".


> +                             "last_update": "2018-07-27T15:44:42.182352",
> +                             "data": {
> +                                     "head": r['head'],
> +                                     "tester": name,
> +                                     "is_timeout": is_timeout
> +                                     },
> +                             "log_url": self.base_url + "/logs/" + project + "/" + test_name + "/?type=project",
> +                             "log": str(log)
> +                            }
> +                self.rest_api_do(url_cmd=url,
> +                                 request_method='put',
> +                                 content_type='application/json',
> +                                 data=json.dumps(json_data))
>                  logf.close()

You shouldn't need to include the read-only fields, log_url, last_update
and resource_uri.

Regarding the validation, that's handled by a validate() method in
ResultSerializer.  The method needs to check that the name is valid,
using code similar to Result.renderer, and then ask the module object to
actually do the validation.  But the best way to do the latter, in my
opinion, is to also use DRF: add a Serializer in mods/git.py and
mods/testing.py, and point to it with an attribute in the class.

Then you can use the serializer to validate the data like

   module.result_data_serializer_class(data=data). \
       is_valid(raise_exception=True)

The series should be organized as follows:

- add ability to access projects by name (either using ?name= or using
/by-name/XXX)

- add basic infrastructure for result PUT (authentication and validation)

- add support for result PUT to mods/git.py (validation and allowed_groups)

- add support for result PUT to mods/testing.py

- convert patchew-cli applier-report to use result PUT

- convert patchew-cli tester-report to use result PUT

and all the first four patches should add testcases.

Thanks,

Paolo