use Exception class when MessageBird replies with an unexpected status code

[elhu]

Currently, if the API responds with a status code other than 200, 201, 204, 401, 404, 405, 422, the gem tries to raise an error. However, HTTPServerError is not an exception class, which in turns causes the following exception:

TypeError: exception class/object expected

I believe we should use HttpServerException instead: https://github.com/ruby/ruby/blob/v2_0_0_0/lib/net/http/exceptions.rb.

WDYT?

[samwierema]

Hi @elhu,

Thanks for the pull request. We'll be discussing this internally and get back to you soon.

At first glance: I think this breaks any backwards compatibility and would require a major version upgrade. However, I'll let the Ruby-ists in our team have a better look. And, of course, if you have some input on that, I'd love to hear it.

[elhu]

Hi @samwierema ,

I realise this is a breaking change, but the current behaviour is broken in my opinion:

2.2.2 :001 > client = ::MessageBird::Client.new("REDACTED")
 => #<MessageBird::Client:0x0000000578f578 @access_key="REDACTED">
2.2.2 :002 > client.lookup("+310123456789")
TypeError: exception class/object expected
  from .../gems/messagebird-rest-1.3.2/lib/messagebird/client.rb:54:in `raise'

Now, my solution isn't perfect either. According to your documentation, the server responds with a specific status code (400) if the phone number looked up is invalid. I think a better approach would either be to return something indicating the lookup failed (nil perhaps?) or to raise an exception more specific than "something failed on the server side"... but at least my changes allow the gem user to be reasonably sure that they're rescuing the right thing.

[samwierema]

Hi @elhu,

After some discussion we've decided not to merge this pull request. Not for not being a valid issue, but we'd like to resolve this in a different manner, mostly to avoid the major version bump at this time (we have some other changes we'd want to do in that case).

One thing we should definitely implement is support for the 400 HTTP status code returned by the Lookup service. That would mitigate at least that the error is triggered in this specific case.

The other thing we're looking into, but haven't decided upon, is to add a use_exception flag in the client. E.g.:

def initialize(access_key = nil, use_exceptions = false)

And then raise a valid exception when it's true. E.g.:

case response.code.to_i
when 200, 201, 204, 401, 404, 405, 422
    json = JSON.parse(response.body)
else
    when @use_exceptions
        raise Net::HTTPServerException.new 'Unknown response from server', response
    else
        raise Net::HTTPServerError.new response.http_version, 'Unknown response from server', response
    end
end

What do you think? If you have another alternative here, we'd love to hear it and discuss some more.

[elhu]

@samwierema I'm 100% in support of adding a specific error for the 400 HTTP status case, but that is also a breaking change.

People who are currently using the lookup method and are relying on rescuing a (albeit weird) TypeError when the phone number is invalid will still have to change some code.

The @use_exceptions trick will work, but it's really a hack around the actual issue.

I can't make the decision for you guys, but if it were my gem I would probably introduce this fix (or better, a specific error for invalid phone numbers) in a new major version quickly, even if I had changes requiring another major version bump planned for a few weeks from now.

Basically, if there's no hard constraints (like the client version having to match the API version for example), I don't necessarily see releasing two majors close together as a big issue.

[elhu]

@samwierema any news?

I was thinking about something. In some way sending an invalid phone number over to the API could be considered a kind of TypeError (in the sense that the string sent to the API does not match the "phone number" type).

In that case, maybe we could have something like:

class InvalidPhoneNumber < TypeError; end

and raise that error.

It's not a breaking change since any one currently rescuing TypeError will still be able to rescue the exception thanks to inheritance.

It's not elegant, but it does the job until the next major release and in my opinion it doesn't feel as hack-ish as the use_exceptions solution.

WDYT?

[elhu]

Did you have time to look at my latest comment :)?

[elhu]

Hi! Is this client still maintained?

[cooldarkdryplace]

Hi @elhu, Sorry for a delay. Your solution based on class InvalidPhoneNumber < TypeError; end looks good to me. I think it should better be introduced as a new PR, so I am going to close this one. Thanks for your contribution and for being proactive in solving this.