diff --git a/lib/zammad_api.rb b/lib/zammad_api.rb index b49f445..608477d 100644 --- a/lib/zammad_api.rb +++ b/lib/zammad_api.rb @@ -1,4 +1,5 @@ require 'zammad_api/version' +require 'zammad_api/errors' require 'zammad_api/client' module ZammadAPI diff --git a/lib/zammad_api/client.rb b/lib/zammad_api/client.rb index a2aba34..b1d6c85 100644 --- a/lib/zammad_api/client.rb +++ b/lib/zammad_api/client.rb @@ -31,7 +31,7 @@ def method_missing(method, *_args) begin class_object = Kernel.const_get(class_name) rescue - raise "Resource for #{method} does not exist" + raise ResourceNotFoundError, "Resource for #{method} does not exist" end ZammadAPI::Dispatcher.new(@transport, class_object) end @@ -39,20 +39,20 @@ def method_missing(method, *_args) private def check_config - raise 'missing url in config' if !@config[:url] - raise 'config url needs to start with http:// or https://' if !%r{^(http|https)://}.match?(@config[:url]) + raise ConfigurationError, 'missing url in config' if !@config[:url] + raise ConfigurationError, 'config url needs to start with http:// or https://' if !%r{^(http|https)://}.match?(@config[:url]) # check for token auth return if @config[:http_token] && !@config[:http_token].empty? return if @config[:oauth2_token] && !@config[:oauth2_token].empty? if !@config[:user] || @config[:user].empty? - raise 'missing user in config' + raise ConfigurationError, 'missing user in config' end return if @config[:password] && !@config[:password].empty? - raise 'missing password in config' + raise ConfigurationError, 'missing password in config' end def modulize(string) diff --git a/lib/zammad_api/errors.rb b/lib/zammad_api/errors.rb new file mode 100644 index 0000000..4e07e1d --- /dev/null +++ b/lib/zammad_api/errors.rb @@ -0,0 +1,62 @@ +require 'json' + +module ZammadAPI + class Error < RuntimeError; end + + class ConfigurationError < Error; end + + class ResourceNotFoundError < Error; end + + class ResponseError < Error + attr_reader :response, :operation, :resource_class + + def initialize(operation:, response:, resource_class: nil) + @operation = operation + @response = response + @resource_class = resource_class + super(default_message) + end + + def status + response&.status + end + + def body + response&.body + end + + def self.from(response, **kwargs) + klass = if response.nil? + self + elsif response.status >= 500 + ServerError + else + ClientError + end + klass.new(response: response, **kwargs) + end + + private + + def default_message + subject = resource_class ? "#{operation} (#{resource_class.name})" : operation + "Can't #{subject}: #{detail}" + end + + def detail + body_error || "HTTP #{status}" + end + + def body_error + return nil if body.to_s.strip.empty? + + parsed = JSON.parse(body) + parsed.is_a?(Hash) ? parsed['error'] : nil + rescue JSON::ParserError + nil + end + end + + class ClientError < ResponseError; end + class ServerError < ResponseError; end +end diff --git a/lib/zammad_api/list_base.rb b/lib/zammad_api/list_base.rb index 6b59114..c8b692c 100644 --- a/lib/zammad_api/list_base.rb +++ b/lib/zammad_api/list_base.rb @@ -62,11 +62,12 @@ def request(request, url, parameter) }.join('&') response = @transport.get(url: url) - data = safe_json_parse(response.body) if response.status != 200 - raise "Can't get .#{request} of object (#{@resource.class.name}): #{data['error']}" + raise ResponseError.from(response, operation: "get .#{request} of object", resource_class: @resource.class) end + data = safe_json_parse(response.body) + list = [] data.each do |local_data| item = @resource.new(@transport, local_data) @@ -77,7 +78,7 @@ def request(request, url, parameter) end def perform_request(_parameter) - raise "no perform_request implementation for #{self.class.name} found" + raise Error, "no perform_request implementation for #{self.class.name} found" end end end diff --git a/lib/zammad_api/resources/base.rb b/lib/zammad_api/resources/base.rb index c894401..b3a6dde 100644 --- a/lib/zammad_api/resources/base.rb +++ b/lib/zammad_api/resources/base.rb @@ -43,12 +43,9 @@ def changed? def destroy response = @transport.delete(url: "#{@url}/#{@attributes[:id]}") - if response.body.to_s != '' && response.body.to_s != ' ' - data = safe_json_parse(response.body) - end return true if response.status == 200 - raise "Can't destroy object (#{self.class.name}): #{data['error']}" + raise ResponseError.from(response, operation: 'destroy object', resource_class: self.class) end def save @@ -79,11 +76,11 @@ def self.search(transport, parameter) def self.find(transport, id) response = transport.get(url: "#{@url}/#{id}?expand=true") - data = safe_json_parse(response.body) if response.status != 200 - raise "Can't find object (#{self.class.name}): #{data['error']}" + raise ResponseError.from(response, operation: 'find object', resource_class: self) end + data = safe_json_parse(response.body) item = new(transport, data) item.new_instance = false item @@ -110,11 +107,10 @@ def saved_attributes end def save_new - response = @transport.post(url: "#{@url}?expand=true", params: @attributes) - attributes = safe_json_parse(response.body) - return attributes if response.status == 201 + response = @transport.post(url: "#{@url}?expand=true", params: @attributes) + return safe_json_parse(response.body) if response.status == 201 - save_error(attributes) + save_error(response) end def save_existing @@ -122,16 +118,14 @@ def save_existing @changes.each do |name, values| attributes_to_post[name] = values[1] end - response = @transport.put(url: "#{@url}/#{@attributes[:id]}?expand=true", params: attributes_to_post) - attributes = safe_json_parse(response.body) - - return attributes if response.status == 200 + response = @transport.put(url: "#{@url}/#{@attributes[:id]}?expand=true", params: attributes_to_post) + return safe_json_parse(response.body) if response.status == 200 - save_error(attributes) + save_error(response) end - def save_error(attributes) - raise "Can't save object (#{self.class.name}): #{attributes['error']}" + def save_error(response) + raise ResponseError.from(response, operation: 'save object', resource_class: self.class) end def symbolize_keys_deep!(hash) diff --git a/lib/zammad_api/resources/ticket.rb b/lib/zammad_api/resources/ticket.rb index dfb9c12..448209b 100644 --- a/lib/zammad_api/resources/ticket.rb +++ b/lib/zammad_api/resources/ticket.rb @@ -3,11 +3,12 @@ class ZammadAPI::Resources::Ticket < ZammadAPI::Resources::Base def articles response = @transport.get(url: "/api/v1/ticket_articles/by_ticket/#{id}?expand=true") - data = safe_json_parse(response.body) if response.status != 200 - raise "Can't get articles (#{self.class.name}): #{data['error']}" + raise ZammadAPI::ResponseError.from(response, operation: 'get articles', resource_class: self.class) end + data = safe_json_parse(response.body) + data.collect do |raw| item = ZammadAPI::Resources::TicketArticle.new(@transport, raw) item.new_instance = false diff --git a/lib/zammad_api/resources/ticket_article_attachment.rb b/lib/zammad_api/resources/ticket_article_attachment.rb index a445f60..c3e06d9 100644 --- a/lib/zammad_api/resources/ticket_article_attachment.rb +++ b/lib/zammad_api/resources/ticket_article_attachment.rb @@ -13,7 +13,6 @@ def download response = @transport.get(url: "/api/v1/ticket_attachment/#{ticket_id}/#{article_id}/#{id}") return response.body if response.status == 200 - data = safe_json_parse(response.body) - raise "Can't get articles (#{self.class.name}): #{data['error']}" + raise ZammadAPI::ResponseError.from(response, operation: 'get articles', resource_class: self.class) end end diff --git a/spec/zammad_api/client_spec.rb b/spec/zammad_api/client_spec.rb index d213b1e..2b67d00 100644 --- a/spec/zammad_api/client_spec.rb +++ b/spec/zammad_api/client_spec.rb @@ -17,6 +17,48 @@ let(:config) { Helper.config } let(:instance) { described_class.new(config) } + describe '.new' do + it 'raises ConfigurationError when url is missing' do + expect { described_class.new(config.merge(url: nil)) } + .to raise_error(ZammadAPI::ConfigurationError, 'missing url in config') + end + + it 'raises ConfigurationError when url scheme is unsupported' do + expect { described_class.new(config.merge(url: 'ftp://example.com')) } + .to raise_error(ZammadAPI::ConfigurationError, 'config url needs to start with http:// or https://') + end + + it 'raises ConfigurationError when user is missing' do + expect { described_class.new(config.merge(user: nil)) } + .to raise_error(ZammadAPI::ConfigurationError, 'missing user in config') + end + + it 'raises ConfigurationError when password is missing' do + expect { described_class.new(config.merge(password: nil)) } + .to raise_error(ZammadAPI::ConfigurationError, 'missing password in config') + end + + it 'does not require user/password when http_token is supplied' do + expect { described_class.new(url: config[:url], http_token: 'token') }.not_to raise_error + end + + it 'does not require user/password when oauth2_token is supplied' do + expect { described_class.new(url: config[:url], oauth2_token: 'token') }.not_to raise_error + end + end + + describe '#method_missing' do + it 'raises ResourceNotFoundError for unknown resources' do + expect { instance.does_not_exist }.to raise_error(ZammadAPI::ResourceNotFoundError, /Resource for DoesNotExist does not exist/) + end + + it 'attaches the underlying NameError as #cause' do + instance.does_not_exist + rescue ZammadAPI::ResourceNotFoundError => e + expect(e.cause).to be_a(NameError) + end + end + describe '#perform_on_behalf_of' do it 'performs a given block on behalft of a given user' do on_behalf_of_identifier = 'some_login' diff --git a/spec/zammad_api/errors_spec.rb b/spec/zammad_api/errors_spec.rb new file mode 100644 index 0000000..b335088 --- /dev/null +++ b/spec/zammad_api/errors_spec.rb @@ -0,0 +1,164 @@ +require 'spec_helper' + +describe ZammadAPI do + describe ZammadAPI::Error do + it 'descends from RuntimeError' do + expect(described_class.ancestors).to include(RuntimeError) + end + end + + describe ZammadAPI::ConfigurationError do + it 'descends from ZammadAPI::Error' do + expect(described_class.ancestors).to include(ZammadAPI::Error) + end + + it 'descends from RuntimeError' do + expect(described_class.ancestors).to include(RuntimeError) + end + end + + describe ZammadAPI::ResourceNotFoundError do + it 'descends from ZammadAPI::Error' do + expect(described_class.ancestors).to include(ZammadAPI::Error) + end + + it 'descends from RuntimeError' do + expect(described_class.ancestors).to include(RuntimeError) + end + end + + describe ZammadAPI::ResponseError do + let(:fake_response) { Struct.new(:status, :body) } + + describe '.from' do + it 'returns a ClientError for 4xx responses' do + response = fake_response.new(404, '{}') + expect(described_class.from(response, operation: 'find object')).to be_a(ZammadAPI::ClientError) + end + + it 'returns a ClientError for 408 (Request Timeout)' do + response = fake_response.new(408, '{}') + expect(described_class.from(response, operation: 'find object')).to be_a(ZammadAPI::ClientError) + end + + it 'returns a ClientError for 429 (Too Many Requests)' do + response = fake_response.new(429, '{}') + expect(described_class.from(response, operation: 'find object')).to be_a(ZammadAPI::ClientError) + end + + it 'returns a ServerError for 5xx responses' do + response = fake_response.new(500, '{}') + expect(described_class.from(response, operation: 'find object')).to be_a(ZammadAPI::ServerError) + end + + it 'returns a ServerError for 502 (Bad Gateway)' do + response = fake_response.new(502, 'Bad Gateway') + expect(described_class.from(response, operation: 'find object')).to be_a(ZammadAPI::ServerError) + end + + it 'returns a base ResponseError when no response is supplied' do + result = described_class.from(nil, operation: 'find object') + expect(result.class).to eq(described_class) + end + end + + describe 'subclasses' do + it 'ClientError descends from ResponseError' do + expect(ZammadAPI::ClientError.ancestors).to include(described_class) + end + + it 'ServerError descends from ResponseError' do + expect(ZammadAPI::ServerError.ancestors).to include(described_class) + end + + it 'descends from ZammadAPI::Error' do + expect(described_class.ancestors).to include(ZammadAPI::Error) + end + + it 'descends from RuntimeError' do + expect(described_class.ancestors).to include(RuntimeError) + end + end + + describe '#message' do + it "uses the JSON body's 'error' key when present" do + response = fake_response.new(404, '{"error":"User not found"}') + error = described_class.from(response, operation: 'find object', resource_class: ZammadAPI::Resources::User) + expect(error.message).to eq("Can't find object (ZammadAPI::Resources::User): User not found") + end + + it 'preserves the original message format for the JSON-with-error case' do + response = fake_response.new(422, '{"error":"name can\'t be blank"}') + error = described_class.from(response, operation: 'save object', resource_class: ZammadAPI::Resources::Group) + expect(error.message).to eq("Can't save object (ZammadAPI::Resources::Group): name can't be blank") + end + + it 'falls back to HTTP status when the body is not JSON (issue #29 scenario)' do + response = fake_response.new(502, 'Bad Gateway') + error = described_class.from(response, operation: 'find object', resource_class: ZammadAPI::Resources::User) + expect(error.message).to eq("Can't find object (ZammadAPI::Resources::User): HTTP 502") + end + + it 'falls back to HTTP status when the body is empty' do + response = fake_response.new(500, '') + error = described_class.from(response, operation: 'destroy object', resource_class: ZammadAPI::Resources::User) + expect(error.message).to eq("Can't destroy object (ZammadAPI::Resources::User): HTTP 500") + end + + it "falls back to HTTP status when the JSON body has no 'error' key" do + response = fake_response.new(400, '{"foo":"bar"}') + error = described_class.from(response, operation: 'find object', resource_class: ZammadAPI::Resources::User) + expect(error.message).to eq("Can't find object (ZammadAPI::Resources::User): HTTP 400") + end + + it 'omits the resource_class segment when none is supplied' do + response = fake_response.new(404, '{"error":"nope"}') + error = described_class.from(response, operation: 'find object') + expect(error.message).to eq("Can't find object: nope") + end + + it 'does not attach the JSON parser error as cause (it is a symptom, not the cause)' do + response = fake_response.new(502, 'Bad Gateway') + error = described_class.from(response, operation: 'find object') + expect(error.cause).to be_nil + end + end + + describe 'accessors' do + let(:response) { fake_response.new(404, '{"error":"nope"}') } + let(:error) do + described_class.from(response, operation: 'find object', resource_class: ZammadAPI::Resources::User) + end + + it 'exposes the underlying response' do + expect(error.response).to be(response) + end + + it 'exposes the response status' do + expect(error.status).to eq(404) + end + + it 'exposes the response body' do + expect(error.body).to eq('{"error":"nope"}') + end + + it 'exposes the operation' do + expect(error.operation).to eq('find object') + end + + it 'exposes the resource_class' do + expect(error.resource_class).to eq(ZammadAPI::Resources::User) + end + + it 'returns nil for status when no response is supplied' do + error = described_class.from(nil, operation: 'find object') + expect(error.status).to be_nil + end + + it 'returns nil for body when no response is supplied' do + error = described_class.from(nil, operation: 'find object') + expect(error.body).to be_nil + end + end + end +end diff --git a/spec/zammad_api/resources/group_spec.rb b/spec/zammad_api/resources/group_spec.rb index 0b96426..6caf0d3 100644 --- a/spec/zammad_api/resources/group_spec.rb +++ b/spec/zammad_api/resources/group_spec.rb @@ -12,7 +12,7 @@ expect(group_invalid.class).to eq(ZammadAPI::Resources::Group) expect(group_invalid.new_record?).to be(true) - expect { group_invalid.save }.to raise_error(RuntimeError) + expect { group_invalid.save }.to raise_error(ZammadAPI::ClientError) end it 'new with valid attributes' do diff --git a/spec/zammad_api/resources/organization_spec.rb b/spec/zammad_api/resources/organization_spec.rb index 85af4e9..7851181 100644 --- a/spec/zammad_api/resources/organization_spec.rb +++ b/spec/zammad_api/resources/organization_spec.rb @@ -12,7 +12,7 @@ expect(organization_invalid.class).to eq(ZammadAPI::Resources::Organization) expect(organization_invalid.new_record?).to be(true) - expect { organization_invalid.save }.to raise_error(RuntimeError) + expect { organization_invalid.save }.to raise_error(ZammadAPI::ClientError) end it 'new with valid attributes' do diff --git a/spec/zammad_api/resources/ticket_priority_spec.rb b/spec/zammad_api/resources/ticket_priority_spec.rb index d84a717..45f41a8 100644 --- a/spec/zammad_api/resources/ticket_priority_spec.rb +++ b/spec/zammad_api/resources/ticket_priority_spec.rb @@ -12,7 +12,7 @@ expect(ticket_priority_invalid.class).to eq(ZammadAPI::Resources::TicketPriority) expect(ticket_priority_invalid.new_record?).to be(true) - expect { ticket_priority_invalid.save }.to raise_error(RuntimeError) + expect { ticket_priority_invalid.save }.to raise_error(ZammadAPI::ClientError) end it 'new with valid attributes' do diff --git a/spec/zammad_api/resources/ticket_spec.rb b/spec/zammad_api/resources/ticket_spec.rb index d0fbb59..1c95705 100644 --- a/spec/zammad_api/resources/ticket_spec.rb +++ b/spec/zammad_api/resources/ticket_spec.rb @@ -12,7 +12,7 @@ expect(ticket_invalid.class).to eq(ZammadAPI::Resources::Ticket) expect(ticket_invalid.new_record?).to be(true) - expect { ticket_invalid.save }.to raise_error(RuntimeError) + expect { ticket_invalid.save }.to raise_error(ZammadAPI::ClientError) end it 'new with valid attributes' do diff --git a/spec/zammad_api/resources/ticket_state_spec.rb b/spec/zammad_api/resources/ticket_state_spec.rb index f605b35..d672398 100644 --- a/spec/zammad_api/resources/ticket_state_spec.rb +++ b/spec/zammad_api/resources/ticket_state_spec.rb @@ -12,7 +12,7 @@ expect(ticket_state_invalid.class).to eq(ZammadAPI::Resources::TicketState) expect(ticket_state_invalid.new_record?).to be(true) - expect { ticket_state_invalid.save }.to raise_error(RuntimeError) + expect { ticket_state_invalid.save }.to raise_error(ZammadAPI::ClientError) end it 'new with valid attributes' do diff --git a/spec/zammad_api/resources/user_spec.rb b/spec/zammad_api/resources/user_spec.rb index b96f154..a82b94b 100644 --- a/spec/zammad_api/resources/user_spec.rb +++ b/spec/zammad_api/resources/user_spec.rb @@ -15,7 +15,7 @@ expect(user_invalid.class).to eq(ZammadAPI::Resources::User) expect(user_invalid.new_record?).to be(true) - expect { user_invalid.save }.to raise_error(RuntimeError) + expect { user_invalid.save }.to raise_error(ZammadAPI::ClientError) end it 'new with valid attributes' do @@ -268,7 +268,7 @@ # to have some references and not allow users to delete sleep 12 - expect { user.destroy }.to raise_error(RuntimeError) + expect { user.destroy }.to raise_error(ZammadAPI::ClientError) #result = user.destroy #expect(result).to eq(true) end diff --git a/spec/zammad_api_spec.rb b/spec/zammad_api_spec.rb index 07aee5f..6616717 100644 --- a/spec/zammad_api_spec.rb +++ b/spec/zammad_api_spec.rb @@ -10,23 +10,27 @@ client = Helper.client(user: 'not_existing', password: 'not_existing') it 'user' do - expect { client.user.find(1) }.to raise_error(RuntimeError) + expect { client.user.find(1) }.to raise_error(ZammadAPI::ClientError) do |error| + expect(error.status).to eq(401) + expect(error.operation).to eq('find object') + expect(error.resource_class).to eq(ZammadAPI::Resources::User) + end end it 'organization' do - expect { client.organization.find(1) }.to raise_error(RuntimeError) + expect { client.organization.find(1) }.to raise_error(ZammadAPI::ClientError) end it 'group' do - expect { client.group.find(1) }.to raise_error(RuntimeError) + expect { client.group.find(1) }.to raise_error(ZammadAPI::ClientError) end it 'ticket_priority' do - expect { client.ticket_priority.find(1) }.to raise_error(RuntimeError) + expect { client.ticket_priority.find(1) }.to raise_error(ZammadAPI::ClientError) end it 'ticket_state' do - expect { client.ticket_state.find(1) }.to raise_error(RuntimeError) + expect { client.ticket_state.find(1) }.to raise_error(ZammadAPI::ClientError) end end end