[Ovirt-devel] [PATCH server 1/2] Generic error handling
David Lutterkort
lutter at redhat.com
Tue May 5 19:03:45 UTC 2009
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
def choose_layout
if(params[:component_layout])
@@ -107,28 +111,38 @@ class ApplicationController < ActionController::Base
def authorize_action(privilege, msg=nil)
msg ||= 'You have insufficient privileges to perform action.'
unless authorized?(privilege)
- handle_auth_error(msg)
+ handle_error(:message => msg,
+ :title => "Access Denied", :status => :forbidden)
false
else
true
end
end
- def handle_auth_error(msg)
+
+ def handle_perm_error(error)
+ handle_error(:error => error, :status => :forbidden,
+ :title => "Access denied")
+ end
+
+ def handle_general_error(error)
+ handle_error(:error => error, :status => :internal_server_error,
+ :title => "Internal Server Error")
+ end
+
+ def handle_error(hash)
+ log_error(hash[:error])
+ msg = hash[:message] || hash[:error].message
+ title = hash[:title] || "Internal Server Error"
+ status = hash[:status] || :internal_server_error
respond_to do |format|
- format.html do
- html_error_page(msg)
- end
- format.json do
- @json_hash ||= {}
- @json_hash[:success] = false
- @json_hash[:alert] = msg
- render :json => @json_hash
- end
- format.xml { head :forbidden }
+ format.html { html_error_page(title, msg) }
+ format.json { render :json => json_error_hash(msg, status) }
+ format.xml { render :xml => xml_errors(msg), :status => status }
end
end
- def html_error_page(msg)
- @title = "Access denied"
+
+ def html_error_page(title, msg)
+ @title = title
@errmsg = msg
@ajax = params[:ajax]
@nolayout = params[:nolayout]
@@ -140,6 +154,7 @@ class ApplicationController < ActionController::Base
render :template => 'layouts/popup-error', :layout => 'popup'
end
end
+
# don't define find_opts for array inputs
def json_hash(full_items, attributes, arg_list=[], find_opts={}, id_method=:id)
page = params[:page].to_i
@@ -175,11 +190,37 @@ class ApplicationController < ActionController::Base
render :json => json_hash(full_items, attributes, arg_list, find_opts, id_method).to_json
end
- def json_error(obj_type, obj, exception)
- json_hash = { :object => obj_type, :success => false}
- json_hash[:errors] = obj.errors.localize_error_messages.to_a if obj
- json_hash[:alert] = exception.message if obj.errors.size == 0
- render :json => json_hash
+ private
+ def json_error_hash(msg, status)
+ json = {}
+ json[:success] = (status == :ok)
+ json.merge!(instance_errors)
+ # There's a potential issue here: if we add :errors for an object
+ # that the view won't generate inline error messages for, the user
+ # won't get any indication what the error is. But if we set :alert
+ # unconditionally, the user will get validation errors twice: once
+ # inline in the form, and once in the flash
+ json[:alert] = msg unless json[:errors]
+ return json
+ end
+
+ def xml_errors(msg)
+ xml = {}
+ xml[:message] = msg
+ xml.merge!(instance_errors)
+ return xml
+ end
+
+ def instance_errors
+ hash = {}
+ instance_variables.each do |ivar|
+ val = instance_variable_get(ivar)
+ if val && val.respond_to?(:errors) && val.errors.size > 0
+ hash[:object] = ivar[1, ivar.size]
+ hash[:errors] ||= []
+ hash[:errors] += val.errors.localize_error_messages.to_a
+ end
+ end
+ return hash
end
-
end
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
@@ -111,14 +111,10 @@ class HardwareController < PoolController
end
def show_storage
- begin
- svc_show(params[:id])
- @storage_tree = @pool.storage_tree(:filter_unavailable => false,
- :include_used => true).to_json
- render_show
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- end
+ svc_show(params[:id])
+ @storage_tree = @pool.storage_tree(:filter_unavailable => false,
+ :include_used => true).to_json
+ render_show
end
def show_tasks
@@ -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 }
- rescue Exception => ex
- render :json => { :success => false, :alert => error.message }
- end
+ 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})}
end
def removestorage
diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb
index 911bc92..6e41ac6 100644
--- a/src/app/controllers/pool_controller.rb
+++ b/src/app/controllers/pool_controller.rb
@@ -37,12 +37,8 @@ class PoolController < ApplicationController
end
def show
- begin
- svc_show(params[:id])
- render_show
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- end
+ svc_show(params[:id])
+ render_show
end
def render_show
@@ -58,12 +54,8 @@ class PoolController < ApplicationController
end
def quick_summary
- begin
- svc_show(params[:id])
- render :layout => 'selection'
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- end
+ svc_show(params[:id])
+ render :layout => 'selection'
end
# resource's users list page
@@ -111,56 +103,36 @@ class PoolController < ApplicationController
def create
# FIXME: REST and browsers send params differently. Should be fixed
# in the views
- begin
- alert = svc_create(params[:pool] ? params[:pool] : params[:hardware_pool],
- additional_create_params)
- respond_to do |format|
- format.json {
- reply = { :object => "pool", :success => true,
- :alert => alert }
- reply[:resource_type] = params[:resource_type] if params[:resource_type]
- render :json => reply
- }
- format.xml {
- render :xml => @pool.to_xml(XML_OPTS),
- :status => :created,
- :location => hardware_pool_url(@pool)
- }
- end
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- rescue Exception => ex
- respond_to do |format|
- format.json { json_error("pool", @pool, ex) }
- format.xml { render :xml => @pool.errors,
- :status => :unprocessable_entity }
- end
+ alert = svc_create(params[:pool] ? params[:pool] : params[:hardware_pool],
+ additional_create_params)
+ respond_to do |format|
+ format.json {
+ reply = { :object => "pool", :success => true,
+ :alert => alert }
+ reply[:resource_type] = params[:resource_type] if params[:resource_type]
+ render :json => reply
+ }
+ format.xml {
+ render :xml => @pool.to_xml(XML_OPTS),
+ :status => :created,
+ :location => hardware_pool_url(@pool)
+ }
end
end
def update
- begin
- alert = svc_update(params[:id], params[:pool] ? params[:pool] :
- params[:hardware_pool])
- respond_to do |format|
- format.json {
- reply = { :object => "pool", :success => true, :alert => alert }
- render :json => reply
- }
- format.xml {
- render :xml => @pool.to_xml(XML_OPTS),
- :status => :created,
- :location => hardware_pool_url(@pool)
- }
- end
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- rescue Exception => ex
- respond_to do |format|
- format.json { json_error("pool", @pool, ex) }
- format.xml { render :xml => @pool.errors,
- :status => :unprocessable_entity }
- end
+ alert = svc_update(params[:id], params[:pool] ? params[:pool] :
+ params[:hardware_pool])
+ respond_to do |format|
+ format.json {
+ reply = { :object => "pool", :success => true, :alert => alert }
+ render :json => reply
+ }
+ format.xml {
+ render :xml => @pool.to_xml(XML_OPTS),
+ :status => :created,
+ :location => hardware_pool_url(@pool)
+ }
end
end
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
diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb
index 993d285..0198d47 100644
--- a/src/app/controllers/smart_pools_controller.rb
+++ b/src/app/controllers/smart_pools_controller.rb
@@ -35,13 +35,9 @@ class SmartPoolsController < PoolController
end
def show_storage
- begin
- svc_show(params[:id])
- @storage_tree = @pool.storage_tree(:filter_unavailable => false, :include_used => true).to_json
- render_show
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- end
+ svc_show(params[:id])
+ @storage_tree = @pool.storage_tree(:filter_unavailable => false, :include_used => true).to_json
+ render_show
end
def additional_create_params
@@ -138,22 +134,9 @@ class SmartPoolsController < PoolController
end
def add_or_remove_items(item_class, item_action)
- begin
- alert = svc_add_remove_items(params[:id], item_class, item_action,
- params[:resource_ids].split(","))
- render :json => { :success => true, :alert => alert}
- rescue
- render :json => { :success => false,
- :alert => "#{item_action.to_s} #{item_class.table_name.humanize} failed." }
- 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 }
- rescue Exception => ex
- render :json => { :success => false, :alert => error.message }
- end
+ alert = svc_add_remove_items(params[:id], item_class, item_action,
+ params[:resource_ids].split(","))
+ render :json => { :success => true, :alert => alert}
end
def add_items
@@ -164,22 +147,8 @@ class SmartPoolsController < PoolController
class_and_id[1] = class_and_id[1].to_a
end
- begin
- alert = svc_add_remove_items(params[:id], nil, :add, class_and_ids)
- render :json => { :success => true, :alert => alert}
- rescue
- render :json => { :success => false,
- :alert => "#{item_action.to_s} failed." }
- 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 }
- rescue Exception => ex
- render :json => { :success => false, :alert => error.message }
- end
-
+ alert = svc_add_remove_items(params[:id], nil, :add, class_and_ids)
+ render :json => { :success => true, :alert => alert}
end
protected
diff --git a/src/app/controllers/storage_volume_controller.rb b/src/app/controllers/storage_volume_controller.rb
index d4a2561..6bdbbdc 100644
--- a/src/app/controllers/storage_volume_controller.rb
+++ b/src/app/controllers/storage_volume_controller.rb
@@ -108,7 +108,10 @@ class StorageVolumeController < ApplicationController
def destroy
unless authorized?(Privilege::MODIFY) and @storage_volume.storage_pool.user_subdividable
- handle_auth_error("You do not have permission to delete this storage volume.")
+ handle_error(:message =>
+ "You do not have permission to delete this storage volume.",
+ :status => :forbidden,
+ :title => "Access Denied")
else
alert, success = delete_volume_internal(@storage_volume)
respond_to do |format|
diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb
index ee6d79e..29c0f16 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -41,13 +41,9 @@ class VmController < ApplicationController
end
def show
- begin
- svc_show(params[:id])
- @actions = @vm.get_action_hash(@user)
- render :layout => 'selection'
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- end
+ svc_show(params[:id])
+ @actions = @vm.get_action_hash(@user)
+ render :layout => 'selection'
end
def add_to_smart_pool
@@ -62,14 +58,8 @@ class VmController < ApplicationController
def create
params[:vm][:forward_vnc] = params[:forward_vnc]
- begin
- alert = svc_create(params[:vm], params[:start_now])
- render :json => { :object => "vm", :success => true, :alert => alert }
- rescue PermissionError => perm_error
- handle_auth_error(perm_error.message)
- rescue Exception => error
- json_error("vm", @vm, error)
- end
+ alert = svc_create(params[:vm], params[:start_now])
+ render :json => { :object => "vm", :success => true, :alert => alert }
end
def edit
@@ -79,14 +69,9 @@ class VmController < ApplicationController
def update
params[:vm][:forward_vnc] = params[:forward_vnc]
- begin
- alert = svc_update(params[:id], params[:vm], params[:start_now],
- params[:restart_now])
- render :json => { :object => "vm", :success => true, :alert => alert }
- rescue Exception => error
- # FIXME: need to distinguish vm vs. task save errors (but should mostly be vm)
- json_error("vm", @vm, error)
- end
+ alert = svc_update(params[:id], params[:vm], params[:start_now],
+ params[:restart_now])
+ render :json => { :object => "vm", :success => true, :alert => alert }
end
#FIXME: we need permissions checks. user must have permission. Also state checks
@@ -123,14 +108,8 @@ class VmController < ApplicationController
end
def destroy
- begin
- alert = svc_destroy(params[:id])
- render :json => { :object => "vm", :success => true, :alert => alert }
- rescue ActionError => error
- json_error("vm", @vm, error)
- rescue Exception => error
- json_error("vm", @vm, error)
- end
+ alert = svc_destroy(params[:id])
+ render :json => { :object => "vm", :success => true, :alert => alert }
end
def storage_volumes_for_vm_json
@@ -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)
- 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
--
1.6.0.6
More information about the ovirt-devel
mailing list