You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by mf...@redhat.com on 2012/02/27 16:48:19 UTC
[PATCH core 2/3] Client: Revamped client/server error handling
From: Michal Fojtik <mf...@redhat.com>
This patch will various HTTP error exceptions to
client library. The exception classes are then used
to properly report server/client exceptions and they
should help handle various HTTP errors.
Signed-off-by: Michal fojtik <mf...@redhat.com>
---
client/lib/deltacloud.rb | 72 ++++++++------------
client/lib/errors.rb | 140 ++++++++++++++++++++++++++++++++++++++
client/specs/errors_spec.rb | 59 ++++++++++++++++
server/views/errors/500.xml.haml | 2 +
server/views/errors/501.xml.haml | 13 +---
server/views/errors/502.xml.haml | 13 +---
server/views/errors/504.xml.haml | 13 +---
7 files changed, 232 insertions(+), 80 deletions(-)
create mode 100644 client/lib/errors.rb
create mode 100644 client/specs/errors_spec.rb
diff --git a/client/lib/deltacloud.rb b/client/lib/deltacloud.rb
index 614eab2..a52af1f 100644
--- a/client/lib/deltacloud.rb
+++ b/client/lib/deltacloud.rb
@@ -21,6 +21,7 @@ require 'hwp_properties'
require 'instance_state'
require 'documentation'
require 'base_object'
+require 'errors'
require 'client_bucket_methods'
module DeltaCloud
@@ -314,9 +315,7 @@ module DeltaCloud
request(:post, entry_points[:"#{$1}s"], {}, params) do |response|
obj = base_object(:"#{$1}", response)
- # All create calls must respond 201 HTTP code
- # to indicate that resource was created.
- handle_backend_error(response) if response.code!=201
+ response_error(response) unless response_successfull?(response.code)
yield obj if block_given?
end
return obj
@@ -349,6 +348,28 @@ module DeltaCloud
headers
end
+ def response_successfull?(code)
+ return true if code.to_s =~ /^2(\d{2})$/
+ return true if code.to_s =~ /^3(\d{2})$/
+ return false
+ end
+
+ def response_error(response)
+ if response.code.to_s =~ /4(\d{2})/
+ DeltaCloud::HTTPError.client_error(response.code)
+ else
+ xml = Nokogiri::XML(response.to_s)
+ opts = {
+ :driver => (xml/'backend').first[:driver],
+ :provider => (xml/'backend').first[:provider],
+ :params => (xml/'request/param').inject({}) { |r,p| r[:"#{p[:name]}"] = p.text; r }
+ }
+ backtrace = (xml/'backtrace').empty? ? nil : (xml/'backtrace').first.text.split("\n")[1..10].map { |l| l.strip }
+ DeltaCloud::HTTPError.server_error(xml.root[:status] || response.code,
+ (xml/'message').first.text, opts, backtrace)
+ end
+ end
+
# Basic request method
#
def request(*args, &block)
@@ -367,55 +388,18 @@ module DeltaCloud
if conf[:method].eql?(:post)
resource = RestClient::Resource.new(conf[:path], :open_timeout => conf[:open_timeout], :timeout => conf[:timeout])
resource.send(:post, conf[:form_data], default_headers.merge(extended_headers)) do |response, request, block|
- handle_backend_error(response) if [500, 502, 501, 401, 504].include? response.code
- if response.respond_to?('body')
- yield response.body if block_given?
- else
- yield response.to_s if block_given?
- end
+ response_error(response) unless response_successfull? response.code
+ yield response.to_s
end
else
resource = RestClient::Resource.new(conf[:path], :open_timeout => conf[:open_timeout], :timeout => conf[:timeout])
resource.send(conf[:method], default_headers.merge(extended_headers)) do |response, request, block|
- handle_backend_error(response) if [500, 502, 501, 504, 401].include? response.code
- if conf[:method].eql?(:get) and [301, 302, 307].include? response.code
- response.follow_redirection(request) do |response, request, block|
- if response.respond_to?('body')
- yield response.body if block_given?
- else
- yield response.to_s if block_given?
- end
- end
- else
- if response.respond_to?('body')
- yield response.body if block_given?
- else
- yield response.to_s if block_given?
- end
- end
+ response_error(response) unless response_successfull? response.code
+ yield response.to_s
end
end
end
- # Re-raise backend errors as on exception in client with message from
- # backend
- class BackendError < StandardError
-
- def initialize(opts={})
- opts[:message] = "Not authorized / Invalid credentials" if opts[:code] == 401
- super("#{opts[:code]} : #{opts[:message]}")
- set_backtrace(opts[:backtrace].split("\n").map { |l| l.strip }[0..10]) if opts[:backtrace]
- end
-
- end
-
- def handle_backend_error(response)
- response_xml = Nokogiri::XML(response)
- backtrace = (response_xml/'error/backtrace').empty? ? nil : (response_xml/'error/backtrace').text
- raise BackendError.new(:message => (response_xml/'error/message').text,
- :code => response.code,
- :backtrace => backtrace)
- end
# Check if specified collection have wanted feature
def feature?(collection, name)
diff --git a/client/lib/errors.rb b/client/lib/errors.rb
new file mode 100644
index 0000000..d0eff44
--- /dev/null
+++ b/client/lib/errors.rb
@@ -0,0 +1,140 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+module DeltaCloud
+ module HTTPError
+
+ class ClientError < StandardError
+
+ attr_reader :params, :driver, :provider
+
+ def initialize(code, message, opts={}, backtrace=nil)
+ @params, @driver, @provider = opts[:params], opts[:driver], opts[:provider]
+ if code.to_s =~ /^5(\d{2})/
+ message += "\nParameters: #{@params.inspect}\n"
+ message += "Driver: #{@driver}@#{@provider}"
+ end
+ super("#{code}\n\n#{self.class.superclass}: #{message}\n\n")
+ # If server provided us the backtrace, then replace client backtrace
+ # with the server one.
+ set_backtrace(backtrace) unless backtrace.nil?
+ end
+ end
+
+ class ServerError < ClientError; end
+ class UknownError < ClientError; end
+
+ # For sake of consistent documentation we need to create
+ # this exceptions manually, instead of using some meta-programming.
+ # Client will really appreciate this it will try to catch some
+ # specific exception.
+
+ # Client errors (4xx)
+ class BadRequest < ClientError; end
+ class Unauthorized < ClientError; end
+ class Forbidden < ClientError; end
+ class NotFound < ClientError; end
+ class MethodNotAllowed < ClientError; end
+ class NotAcceptable < ClientError; end
+ class RequestTimeout < ClientError; end
+ class Gone < ClientError; end
+ class ExpectationFailed < ClientError; end
+ class UnsupportedMediaType < ClientError; end
+
+ # Server errors (5xx)
+ class DeltacloudError < ServerError; end
+ class ProviderError < ServerError; end
+ class ProviderTimeout < ServerError; end
+ class ServiceUnavailable < ServerError; end
+ class NotImplemented < ServerError; end
+
+ class ExceptionHandler
+
+ attr_reader :http_status_code, :message, :trace
+
+ def initialize(status_code, message=nil, opts={}, backtrace=nil, &block)
+ @http_status_code = status_code.to_i
+ @trace = backtrace
+ @message = message || client_error_messages[status_code] || 'No error message received'
+ @options = opts
+ instance_eval(&block) if block_given?
+ end
+
+ def on(code, exception_class)
+ if code == @http_status_code
+ raise exception_class.new(code, @message, @options, @trace)
+ end
+ end
+
+ private
+
+ def client_error_messages
+ {
+ 400 => 'The request could not be understood by the server due to malformed syntax.',
+ 401 => 'Authentication required for this request or invalid credentials provided.',
+ 403 => 'Requested operation is not allowed for this resource.',
+ 404 => 'Not Found',
+ 405 => 'Method not allowed for this resource.',
+ 406 => 'Requested media type is not supported by server.',
+ 408 => 'The client did not produce a request within the time that the server was prepared to wait.',
+ 410 => 'The resource is no longer available'
+ }
+ end
+
+ end
+
+ def self.parse_response_error(response)
+
+ end
+
+ def self.client_error(code)
+ ExceptionHandler.new(code) do
+ # Client errors
+ on 400, BadRequest
+ on 401, Unauthorized
+ on 403, Forbidden
+ on 404, NotFound
+ on 405, MethodNotAllowed
+ on 406, NotAcceptable
+ on 408, RequestTimeout
+ on 410, Gone
+ end
+ end
+
+ def self.server_error(code, message, opts={}, backtrace=nil)
+ ExceptionHandler.new(code, message, opts, backtrace) do
+ # Client errors
+ on 400, BadRequest
+ on 401, Unauthorized
+ on 403, Forbidden
+ on 404, NotFound
+ on 405, MethodNotAllowed
+ on 406, NotAcceptable
+ on 408, RequestTimeout
+ on 410, Gone
+ on 415, UnsupportedMediaType
+ on 417, ExpectationFailed
+ # Server errors
+ on 500, DeltacloudError
+ on 501, NotImplemented
+ on 502, ProviderError
+ on 503, ServiceUnavailable
+ on 504, ProviderTimeout
+ end
+ raise Deltacloud::HTTPError::UnknownError.new(code, message, opts, backtrace)
+ end
+
+ end
+end
diff --git a/client/specs/errors_spec.rb b/client/specs/errors_spec.rb
new file mode 100644
index 0000000..031e3b7
--- /dev/null
+++ b/client/specs/errors_spec.rb
@@ -0,0 +1,59 @@
+#
+# Copyright (C) 2009-2011 Red Hat, Inc.
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+require 'specs/spec_helper'
+
+describe "server error handler" do
+
+ it_should_behave_like "all resources"
+
+ it 'should capture HTTP 500 error as DeltacloudError' do
+ DeltaCloud.new( API_NAME, API_PASSWORD, API_URL ) do |client|
+ expect { client.realm('500') }.should raise_error(DeltaCloud::HTTPError::DeltacloudError)
+ end
+ end
+
+ it 'should capture HTTP 502 error as ProviderError' do
+ DeltaCloud.new( API_NAME, API_PASSWORD, API_URL ) do |client|
+ expect { client.realm('502') }.should raise_error(DeltaCloud::HTTPError::ProviderError)
+ end
+ end
+
+ it 'should capture HTTP 501 error as NotImplemented' do
+ DeltaCloud.new( API_NAME, API_PASSWORD, API_URL ) do |client|
+ expect { client.realm('501') }.should raise_error(DeltaCloud::HTTPError::NotImplemented)
+ end
+ end
+
+ it 'should capture HTTP 504 error as ProviderTimeout' do
+ DeltaCloud.new( API_NAME, API_PASSWORD, API_URL ) do |client|
+ expect { client.realm('504') }.should raise_error(DeltaCloud::HTTPError::ProviderTimeout)
+ end
+ end
+
+end
+
+describe "client error handler" do
+
+ it 'should capture HTTP 404 error as NotFound' do
+ DeltaCloud.new( API_NAME, API_PASSWORD, API_URL ) do |client|
+ expect { client.realm('non-existing-realm') }.should raise_error(DeltaCloud::HTTPError::NotFound)
+ end
+ end
+
+end
diff --git a/server/views/errors/500.xml.haml b/server/views/errors/500.xml.haml
index 5c111e8..0b2d14a 100644
--- a/server/views/errors/500.xml.haml
+++ b/server/views/errors/500.xml.haml
@@ -1,3 +1,5 @@
+- unless defined?(partial)
+ !!! XML
%error{:url => "#{request.env['REQUEST_URI']}", :status => "#{response.status}"}
%kind backend_error
%backend{ :driver => driver_symbol, :provider => "#{Thread::current[:provider] || ENV['API_PROVIDER'] || 'default'}" }
diff --git a/server/views/errors/501.xml.haml b/server/views/errors/501.xml.haml
index 788fe4b..a4015a4 100644
--- a/server/views/errors/501.xml.haml
+++ b/server/views/errors/501.xml.haml
@@ -1,12 +1 @@
-%error{:url => "#{request.env['REQUEST_URI']}", :status => "#{response.status}"}
- %kind backend_error
- %backend{ :driver => driver_symbol, :provider => "#{Thread::current[:provider] || ENV['API_PROVIDER'] || 'default'}" }
- - if @error.respond_to?(:details) && @error.details
- %details< #{cdata @error.details.join("\n")}
- %message< #{cdata @error.message}
- - if @error.respond_to? :backtrace
- %backtrace=cdata @error.backtrace.join("\n")
- - if params
- %request
- - params.each do |k, v|
- %param{ :name => k}=v
+= haml :'errors/500', :locals => { :@error => @error, :partial => true }
diff --git a/server/views/errors/502.xml.haml b/server/views/errors/502.xml.haml
index 788fe4b..a4015a4 100644
--- a/server/views/errors/502.xml.haml
+++ b/server/views/errors/502.xml.haml
@@ -1,12 +1 @@
-%error{:url => "#{request.env['REQUEST_URI']}", :status => "#{response.status}"}
- %kind backend_error
- %backend{ :driver => driver_symbol, :provider => "#{Thread::current[:provider] || ENV['API_PROVIDER'] || 'default'}" }
- - if @error.respond_to?(:details) && @error.details
- %details< #{cdata @error.details.join("\n")}
- %message< #{cdata @error.message}
- - if @error.respond_to? :backtrace
- %backtrace=cdata @error.backtrace.join("\n")
- - if params
- %request
- - params.each do |k, v|
- %param{ :name => k}=v
+= haml :'errors/500', :locals => { :@error => @error, :partial => true }
diff --git a/server/views/errors/504.xml.haml b/server/views/errors/504.xml.haml
index 788fe4b..a4015a4 100644
--- a/server/views/errors/504.xml.haml
+++ b/server/views/errors/504.xml.haml
@@ -1,12 +1 @@
-%error{:url => "#{request.env['REQUEST_URI']}", :status => "#{response.status}"}
- %kind backend_error
- %backend{ :driver => driver_symbol, :provider => "#{Thread::current[:provider] || ENV['API_PROVIDER'] || 'default'}" }
- - if @error.respond_to?(:details) && @error.details
- %details< #{cdata @error.details.join("\n")}
- %message< #{cdata @error.message}
- - if @error.respond_to? :backtrace
- %backtrace=cdata @error.backtrace.join("\n")
- - if params
- %request
- - params.each do |k, v|
- %param{ :name => k}=v
+= haml :'errors/500', :locals => { :@error => @error, :partial => true }
--
1.7.9.1
Re: [PATCH core 2/3] Client: Revamped client/server error handling
Posted by David Lutterkort <lu...@redhat.com>.
On Mon, 2012-02-27 at 16:48 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mf...@redhat.com>
>
> This patch will various HTTP error exceptions to
> client library. The exception classes are then used
> to properly report server/client exceptions and they
> should help handle various HTTP errors.
ACK, with a couple nits:
> Signed-off-by: Michal fojtik <mf...@redhat.com>
> ---
> client/lib/deltacloud.rb | 72 ++++++++------------
> client/lib/errors.rb | 140 ++++++++++++++++++++++++++++++++++++++
This is more of a longstanding problem, than something about htis patch:
we are very unclean with the namespace of our client gem; we should
really have everything under lib/deltacloud/ for the client to comply
with Ruby conventions.
> client/specs/errors_spec.rb | 59 ++++++++++++++++
> server/views/errors/500.xml.haml | 2 +
> server/views/errors/501.xml.haml | 13 +---
> server/views/errors/502.xml.haml | 13 +---
> server/views/errors/504.xml.haml | 13 +---
The changes to the error templates for 502 and 504 should really be
folded into patch 1/3; that probably means that the changes to 500 and
501 also need to go into that patch, or into a separate one preceding
1/3.
> diff --git a/client/lib/deltacloud.rb b/client/lib/deltacloud.rb
> index 614eab2..a52af1f 100644
> --- a/client/lib/deltacloud.rb
> +++ b/client/lib/deltacloud.rb
> @@ -349,6 +348,28 @@ module DeltaCloud
> headers
> end
>
> + def response_successfull?(code)
There's a typo in successful: it should only have one l.
David