Skip to content

Commit

Permalink
Fixes "TypeError" for all http 404 responses
Browse files Browse the repository at this point in the history
Prior to this PR, any `404` response from mailgun would cause a
`TypeError: exception class/object expected` to be thrown from the
mailgun gem.

What was happening is that the `#submit` method in `Mailgun::Base` would
translate any `ClientError` to a `Mailgun::Error` (which does not
inherit from `Exception`, and thus is not re-raisable) and then either
re-raise that `Mailgun::Error`'s `#handle` or the error itself, based on
whether `handle` returned an instance of `Mailgun::ErrorBase` or not.

The slightly confusing thing here is that re-raising the
`Mailgun::Error` class would always cause a `TypeError: exception
class/object expected` exception, so that code path is never desired.

There is either a bug in the inheritance of `Mailgun::Error`, and it
**should** inherit from `Exception` (or, preferably, `StandardError`),
which would allow it to be thrown directly, or we should remove the code
path that throws this unthrowable class.

Since `Mailgun::NotFound` was the only error class that seemed to take
advantage of this option, and that in the case of a 404 response a
`Mailgun::NotFound` exception seems a better error to throw, I propose
in this PR to remove the option alltogether, and always throw the result
of `Mailgun::Error#handle`.
  • Loading branch information
melcher committed Dec 22, 2016
1 parent 5476618 commit 966d46a
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 8 deletions.
6 changes: 1 addition & 5 deletions lib/mailgun/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,7 @@ def self.submit(method, url, parameters={})
:code => error_code || nil,
:message => error_message || nil
)
if error.handle.kind_of? Mailgun::ErrorBase
raise error.handle
else
raise error
end
raise error.handle
end
end

Expand Down
3 changes: 0 additions & 3 deletions lib/mailgun/mailgun_error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ def handle
end

class NotFound < ErrorBase
def handle
return nil
end
end

class BadRequest < ErrorBase
Expand Down
31 changes: 31 additions & 0 deletions spec/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,37 @@

Mailgun.submit :test_method, '/', :arg1=>"val1"
end

context "when the client raises a 400 http error" do
it "should re-raise a bad request mailgun error" do
client_error = Mailgun::ClientError.new
client_error.http_code = 400
client_error.http_body = { "message" => "Expression is missing" }.to_json
expect(Mailgun::Client).to receive(:new)
.with('/')
.and_return(client_double)
expect(client_double).to receive(:test_method)
.with({:arg1=>"val1"})
.and_raise(client_error)

expect { Mailgun.submit :test_method, '/', :arg1=>"val1" }.to raise_exception Mailgun::BadRequest
end
end
context "when the client raises a 404 http error" do
it "should re-raise a not found mailgun error" do
client_error = Mailgun::ClientError.new
client_error.http_code = 404
client_error.http_body = { "message" => "Not Found" }.to_json
expect(Mailgun::Client).to receive(:new)
.with('/')
.and_return(client_double)
expect(client_double).to receive(:test_method)
.with({:arg1=>"val1"})
.and_raise(client_error)

expect { Mailgun.submit :test_method, '/', :arg1=>"val1" }.to raise_exception Mailgun::NotFound
end
end
end
end

Expand Down

0 comments on commit 966d46a

Please sign in to comment.