Re: [Kimchi-devel] [PATCHv7 1/8] Support params for GET method

On 2014年01月18日 02:01, Aline Manera wrote:
> On 01/14/2014 01:47 PM, lvroyce0210@gmail.com wrote:
>> From: Royce Lv <lvroyce@linux.vnet.ibm.com>
>>
>> Add support for passing params for GET method,
>> This include add param to get_list method,
>> and filter the result with given params.
>> we will call it like:
>> GET /collection?filter_field=value
>> With current implementation with a wrapper,
>> we can easily support any collection query with param.
>>
>> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com>
>> ---
>> src/kimchi/control/base.py | 28 ++++++++++++++++++++--------
>> src/kimchi/control/storagepools.py | 4 ++--
>> src/kimchi/control/storagevolumes.py | 2 +-
>> src/kimchi/control/utils.py | 2 ++
>> 4 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py
>> index 185c8d8..a477abb 100644
>> --- a/src/kimchi/control/base.py
>> +++ b/src/kimchi/control/base.py
>> @@ -28,7 +28,7 @@ import urllib2
>>
>> import kimchi.template
>> from kimchi.control.utils import get_class_name, internal_redirect, 
>> model_fn
>> -from kimchi.control.utils import parse_request, validate_method
>> +from kimchi.control.utils import parse_request, parse_param, 
>> validate_method
>> from kimchi.control.utils import validate_params
>> from kimchi.exception import InvalidOperation, InvalidParameter
>> from kimchi.exception import MissingParameter, NotFoundError, 
>> OperationFailed
>> @@ -212,10 +212,10 @@ class Collection(object):
>>
>> return res.get()
>>
>> - def _get_resources(self):
>> + def _get_resources(self, filter_params):
>> try:
>> get_list = getattr(self.model, model_fn(self, 'get_list'))
>> - idents = get_list(*self.model_args)
>> + idents = get_list(*self.model_args, **filter_params)
>
> If the filter will be done in filter_data() below we don't need to 
> pass filter_params to get_list
My plan is to make it effective in either of these two places:
reasons are according to http://libvirt.org/html/libvirt-libvirt.html
1. Many list API takes flags to indicate what kinds of resource they 
will query, this act as a filter, is supported by libvirt itself, we 
don't need to filter on our own.
2. Some full resource query is quite time consuming and not needed:
such as: user want to query 'nfs' targets on a server, if without 
get_list filter, we will do:
1> query all type of targets: nfs+ iscsi+ gluster, etc
2> filter out iscsi + gluster
which is quite waste of effort.
>
>> res_list = []
>> for ident in idents:
>> # internal text, get_list changes ident to unicode for sorted
>> @@ -234,19 +234,31 @@ class Collection(object):
>> args = self.resource_args + [ident.decode("utf-8")]
>> return self.resource(self.model, *args)
>>
>> - def get(self):
>> - resources = self._get_resources()
>> + def filter_data(self, resources, filter_params):
>> data = []
>> for res in resources:
>> - data.append(res.data)
>> + valid = True
>> + for key, val in filter_params.items():
>> + if key in res.data and res.data[key] != val:
>> + valid = False
>> + break
>
> You should continue the loop even when one resource doesn't have the 
> filter parameter.
yeah, my condition is "if key in res.data and res.data[key] != val" it 
means if resource doesn't have filter parameter, it will ignore this key 
and iterate next filter key.
> For example, some storagepools have "task_id" parameter and other ones 
> don't have.
>
>> + if valid:
>> + data.append(res.data)
>> + return data
>> +
>> + def get(self, filter_params):
>> + resources = self._get_resources(filter_params)
>> + data = self.filter_data(resources, filter_params)
>> return kimchi.template.render(get_class_name(self), data)
>>
>> @cherrypy.expose
>> - def index(self, *args):
>> + def index(self, *args, **kwargs):
>> method = validate_method(('GET', 'POST'))
>> if method == 'GET':
>> try:
>> - return self.get()
>> + filter_params = parse_param()
>> + validate_params(filter_params, self, 'get_list')
>> + return self.get(filter_params)
>> except InvalidOperation, param:
>> error = "Invalid operation: '%s'" % param
>> raise cherrypy.HTTPError(400, error)
>> diff --git a/src/kimchi/control/storagepools.py 
>> b/src/kimchi/control/storagepools.py
>> index 782f5a6..e3236a7 100644
>> --- a/src/kimchi/control/storagepools.py
>> +++ b/src/kimchi/control/storagepools.py
>> @@ -63,9 +63,9 @@ class StoragePools(Collection):
>>
>> return resp
>>
>> - def _get_resources(self):
>> + def _get_resources(self, filter_params):
>> try:
>> - res_list = super(StoragePools, self)._get_resources()
>> + res_list = super(StoragePools, self)._get_resources(filter_params)
>> # Append reserved pools
>> isos = getattr(self, ISO_POOL_NAME)
>> isos.lookup()
>> diff --git a/src/kimchi/control/storagevolumes.py 
>> b/src/kimchi/control/storagevolumes.py
>> index d541807..cd15bcc 100644
>> --- a/src/kimchi/control/storagevolumes.py
>> +++ b/src/kimchi/control/storagevolumes.py
>> @@ -70,7 +70,7 @@ class IsoVolumes(Collection):
>> super(IsoVolumes, self).__init__(model)
>> self.pool = pool
>>
>> - def get(self):
>> + def get(self, filter_params):
>> res_list = []
>> try:
>> get_list = getattr(self.model, model_fn(self, 'get_list'))
>> diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py
>> index c3c5f8e..4b3c4b0 100644
>> --- a/src/kimchi/control/utils.py
>> +++ b/src/kimchi/control/utils.py
>> @@ -81,6 +81,8 @@ def parse_request():
>> raise cherrypy.HTTPError(415, "This API only supports"
>> " 'application/json'")
>
>>
>> +def parse_param():
>> + return cherrypy.request.params
>
> You don't need a function for it.
> It is just one line.
> Use cherrypy.request.params directly
ACK
>
>>
>> def internal_redirect(url):
>> raise cherrypy.InternalRedirect(url.encode("utf-8"))
>