[Ovirt-devel] [PATCH server 1/2] Generic error handling

[Scott Seago]

David Lutterkort wrote:
> Instead of explicit rescue blocks in actions, we now register error
> handlers for each exception (with a catch-all handler for Exception) The
> handlers generate an appropriate response for each exception and output
> method.
>
> Remove handle_auth_error and json_error, since their functionality is now
> covered by handle_perm_error and handle_general_error
> ---
>  src/app/controllers/application.rb               |   81 +++++++++++++++-----
>  src/app/controllers/hardware_controller.rb       |   37 +++-------
>  src/app/controllers/pool_controller.rb           |   88 ++++++++--------------
>  src/app/controllers/resources_controller.rb      |    4 +-
>  src/app/controllers/smart_pools_controller.rb    |   47 ++----------
>  src/app/controllers/storage_volume_controller.rb |    5 +-
>  src/app/controllers/vm_controller.rb             |   61 ++++-----------
>  7 files changed, 131 insertions(+), 192 deletions(-)
>
> diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb
> index bdd5b64..6932a1f 100644
> --- a/src/app/controllers/application.rb
> +++ b/src/app/controllers/application.rb
> @@ -46,6 +46,10 @@ class ApplicationController < ActionController::Base
>    before_filter :tmp_authorize_admin, :only => [:create, :update, :destroy]
>    before_filter :is_logged_in, :get_help_section
>  
> +  # General error handlers, must be in order from least specific
> +  # to most specific
> +  rescue_from Exception, :with => :handle_general_error
> +  rescue_from PermissionError, :with => :handle_perm_error
>   
We might want to include a PartialSuccessError handler too, although 
that could be a separate patch.

> diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
> index 56a2c47..0b6cf9b 100644
> --- a/src/app/controllers/hardware_controller.rb
> +++ b/src/app/controllers/hardware_controller.rb
> @@ -219,24 +215,13 @@ class HardwareController < PoolController
>    end
>  
>    def edit_items(svc_method, target_pool_id, item_action)
> -    begin
> -      alert = send(svc_method, params[:id], params[:resource_ids].split(","),
> -                   target_pool_id)
> -      render :json => { :success => true, :alert => alert,
> -                        :storage => @pool.storage_tree({:filter_unavailable =>
> -                                                        false,
> -                                                        :include_used => true,
> -                                                        :state =>
> -                                                        item_action.to_s})}
> -    rescue PermissionError => perm_error
> -      handle_auth_error(perm_error.message)
> -      # If we need to give more details as to which hosts/storage succeeded,
> -      # they're in the exception
> -    rescue PartialSuccessError => error
> -      render :json => { :success => false, :alert => error.message }
>   
Right now it's functionally equivalent to remove the PartialSuccessError 
handling and use the generic error handler -- although if we're trying 
to phase out rescuing of Exception we should probably handle these 
specific cases explicitly when we expect them.

I guess the question is should PartialSuccessError be handled inline -- 
since it might have action-specific behavior (i.e. which layout to use 
for html, etc.) -- or since most of these are ajax/json responses, 
should we handle it globally by default -- since actions are always free 
to rescue directly if they have custom behavior.
> diff --git a/src/app/controllers/resources_controller.rb b/src/app/controllers/resources_controller.rb
> index 7ad79ca..edee59d 100644
> --- a/src/app/controllers/resources_controller.rb
> +++ b/src/app/controllers/resources_controller.rb
> @@ -113,8 +113,8 @@ class ResourcesController < PoolController
>        @success_list = @vms
>        @failures = {}
>        render :layout => 'confirmation'
> -    rescue PermissionError => perm_error
> -      handle_auth_error(perm_error.message)
> +    rescue PermissionError
> +      raise
>      rescue PartialSuccessError => error
>        @success_list = error.successes
>        @failures = error.failures
>   
So here's an example of custom PartialSuccessError handling -- We might 
be able to make this work generically - if we always include @successes 
and @failures in the respond.html block -- except I'm not sure how to 
handle the layout bit, unless we set a @layout variable at the beginning 
of the action -- to use for the format.html in the error handler. Again 
something for another patch though.
> @@ -144,24 +123,14 @@ class VmController < ApplicationController
>    end
>  
>    def vm_action
> -    begin
> -      alert = svc_vm_action(params[:id], params[:vm_action],
> -                            params[:vm_action_data])
> -      render :json => { :object => "vm", :success => true, :alert => alert  }
> -    rescue ActionError => error
> -      json_error("vm", @vm, error)
>   
Another one that could use an explicit rescue_from handler.

> -    rescue Exception => error
> -      json_error("vm", @vm, error)
> -    end
> +    alert = svc_vm_action(params[:id], params[:vm_action],
> +                          params[:vm_action_data])
> +    render :json => { :object => "vm", :success => true, :alert => alert  }
>    end
>  
>    def cancel_queued_tasks
> -    begin
> -      alert = svc_cancel_queued_tasks(params[:id])
> -      render :json => { :object => "vm", :success => true, :alert => alert  }
> -    rescue Exception => error
> -      json_error("vm", @vm, error)
> -    end
> +    alert = svc_cancel_queued_tasks(params[:id])
> +    render :json => { :object => "vm", :success => true, :alert => alert  }
>    end
>  
>    def migrate
>   

I think all of the above comments could be addressed separately, so I'd 
say ACK even with the comments.

Basically we need to
1) explicitly add a rescue_from handler for each explicit exception type 
we care about (among other things we might eventually add different 
icons to the error message depending on the exception type, etc)
2) we should try to come up with a way to handle PartialSuccess, etc. 
with the rescue_from handler even when we have an explicit layout, we 
need to render -- with setting a @layout method, etc.


Scott