[Kimchi-devel] [PATCH 01/16] Move generate_action_handler() function to Resource() class

[Mark Wu]

On 12/24/2013 10:26 AM, Sheldon wrote:
> On 12/24/2013 02:41 AM, Aline Manera wrote:
>> From: Aline Manera <alinefm at br.ibm.com>
>>
>> generate_action_handler() function is only used by Resources 
>> instances so move
>> it to Resource()
>>
>> Signed-off-by: Aline Manera <alinefm at br.ibm.com>
>> ---
>>   src/kimchi/controller.py |   75 
>> +++++++++++++++++++++++-----------------------
>>   1 file changed, 37 insertions(+), 38 deletions(-)
>>
>> diff --git a/src/kimchi/controller.py b/src/kimchi/controller.py
>> index 2940278..ba8b53d 100644
>> --- a/src/kimchi/controller.py
>> +++ b/src/kimchi/controller.py
>> @@ -103,33 +103,6 @@ def validate_params(params, instance, action):
>>               e.message for e in validator.iter_errors(request)))
>>
>>
>> -def generate_action_handler(instance, action_name, action_args=None):
>> -    def wrapper(*args, **kwargs):
>> -        validate_method(('POST'))
>> -        try:
>> -            model_args = list(instance.model_args)
>> -            if action_args is not None:
>> -                model_args.extend(parse_request()[key] for key in 
>> action_args)
>> -            fn = getattr(instance.model, model_fn(instance, 
>> action_name))
>> -            fn(*model_args)
>> -            raise internal_redirect(instance.uri_fmt %
>> -                                    tuple(instance.model_args))
>> -        except MissingParameter, param:
>> -            raise cherrypy.HTTPError(400, "Missing parameter: '%s'" 
>> % param)
>> -        except InvalidParameter, param:
>> -            raise cherrypy.HTTPError(400, "Invalid parameter: '%s'" 
>> % param)
>> -        except InvalidOperation, msg:
>> -            raise cherrypy.HTTPError(400, "Invalid operation: '%s'" 
>> % msg)
>> -        except OperationFailed, msg:
>> -            raise cherrypy.HTTPError(500, "Operation Failed: '%s'" % 
>> msg)
>> -        except NotFoundError, msg:
>> -            raise cherrypy.HTTPError(404, "Not found: '%s'" % msg)
>> -
>> -    wrapper.__name__ = action_name
>> -    wrapper.exposed = True
>> -    return wrapper
>> -
>> -
>>   class Resource(object):
>>       """
>>       A Resource represents a single entity in the API (such as a 
>> Virtual Machine)
>> @@ -153,6 +126,32 @@ class Resource(object):
>>           self.model_args = (ident,)
>>           self.update_params = []
>>
>> +    def generate_action_handler(self, instance, action_name, 
>> action_args=None):
> do we still need instance? Here instance is same with self.
>
> I think it is OK.
It's good to move it to Resource class,  but you need remove the 
redundant parameter 'instance' as
Sheldon suggested.

>
> + def generate_action_handler(self, action_name, action_args=None):
>> +        def wrapper(*args, **kwargs):
>> +            validate_method(('POST'))
>> +            try:
>> +                model_args = list(instance.model_args)
>> +                if action_args is not None:
>> +                    model_args.extend(parse_request()[key] for key 
>> in action_args)
>> +                fn = getattr(instance.model, model_fn(instance, 
>> action_name))
>> +                fn(*model_args)
>> +                raise internal_redirect(instance.uri_fmt %
>> + tuple(instance.model_args))
>> +            except MissingParameter, param:
>> +                raise cherrypy.HTTPError(400, "Missing parameter: 
>> '%s'" % param)
>> +            except InvalidParameter, param:
>> +                raise cherrypy.HTTPError(400, "Invalid parameter: 
>> '%s'" % param)
>> +            except InvalidOperation, msg:
>> +                raise cherrypy.HTTPError(400, "Invalid operation: 
>> '%s'" % msg)
>> +            except OperationFailed, msg:
>> +                raise cherrypy.HTTPError(500, "Operation Failed: 
>> '%s'" % msg)
>> +            except NotFoundError, msg:
>> +                raise cherrypy.HTTPError(404, "Not found: '%s'" % msg)
>> +
>> +        wrapper.__name__ = action_name
>> +        wrapper.exposed = True
>> +        return wrapper
>> +
>>       def lookup(self):
>>           try:
>>               lookup = getattr(self.model, model_fn(self, 'lookup'))
>> @@ -369,9 +368,9 @@ class VM(Resource):
>>           self.update_params = ["name"]
>>           self.screenshot = VMScreenShot(model, ident)
>>           self.uri_fmt = '/vms/%s'
>> -        self.start = generate_action_handler(self, 'start')
>> -        self.stop = generate_action_handler(self, 'stop')
>> -        self.connect = generate_action_handler(self, 'connect')
>> +        self.start = self.generate_action_handler(self, 'start')
> then you can call
> + self.start = self.generate_action_handler('start')
>> +        self.stop = self.generate_action_handler(self, 'stop')
>> +        self.connect = self.generate_action_handler(self, 'connect')
>>
>>       @property
>>       def data(self):
>> @@ -453,8 +452,8 @@ class Network(Resource):
>>       def __init__(self, model, ident):
>>           super(Network, self).__init__(model, ident)
>>           self.uri_fmt = "/networks/%s"
>> -        self.activate = generate_action_handler(self, 'activate')
>> -        self.deactivate = generate_action_handler(self, 'deactivate')
>> +        self.activate = self.generate_action_handler(self, 'activate')
>> +        self.deactivate = self.generate_action_handler(self, 
>> 'deactivate')
>>
>>       @property
>>       def data(self):
>> @@ -475,8 +474,8 @@ class StorageVolume(Resource):
>>           self.info = {}
>>           self.model_args = [self.pool, self.ident]
>>           self.uri_fmt = '/storagepools/%s/storagevolumes/%s'
>> -        self.resize = generate_action_handler(self, 'resize', ['size'])
>> -        self.wipe = generate_action_handler(self, 'wipe')
>> +        self.resize = self.generate_action_handler(self, 'resize', 
>> ['size'])
>> +        self.wipe = self.generate_action_handler(self, 'wipe')
>>
>>       @property
>>       def data(self):
>> @@ -522,8 +521,8 @@ class StoragePool(Resource):
>>           super(StoragePool, self).__init__(model, ident)
>>           self.update_params = ["autostart"]
>>           self.uri_fmt = "/storagepools/%s"
>> -        self.activate = generate_action_handler(self, 'activate')
>> -        self.deactivate = generate_action_handler(self, 'deactivate')
>> +        self.activate = self.generate_action_handler(self, 'activate')
>> +        self.deactivate = self.generate_action_handler(self, 
>> 'deactivate')
>>
>>       @property
>>       def data(self):
>> @@ -689,8 +688,8 @@ class Host(Resource):
>>           self.stats = HostStats(self.model)
>>           self.stats.exposed = True
>>           self.uri_fmt = '/host/%s'
>> -        self.reboot = generate_action_handler(self, 'reboot')
>> -        self.shutdown = generate_action_handler(self, 'shutdown')
>> +        self.reboot = self.generate_action_handler(self, 'reboot')
>> +        self.shutdown = self.generate_action_handler(self, 'shutdown')
>>           self.partitions = Partitions(self.model)
>>           self.partitions.exposed = True
>>
>
>