Vonage / vonage-ruby-sdk

Vonage REST API client for Ruby. API support for SMS, Voice, Text-to-Speech, Numbers, Verify (2FA) and more.
https://developer.vonage.com
Apache License 2.0
217 stars 108 forks source link

⚠️ URGENT: NoMethodError (undefined method `messages' for #<Vonage::Response:0x000055dbca9bb328>) #216

Closed jarthod closed 2 years ago

jarthod commented 2 years ago

:warning: Since 11:53 UTC today I am receiving this exception at every send (despite the SMS being sent properly). Which is causing some very unfortunately duplicate SMS because of retries. Example:

client.sms.send text: "test #1", from: "xxxxxxxxxx", to: "yyyyyyyyyy"
Traceback (most recent call last):
        1: from (irb):3
NoMethodError (undefined method `messages' for #<Vonage::Response:0x000055dbca9bb328>)

I suppose this is caused by a change in the API which breaks the gem error handling. I started debugging this for you:

client.sms.method(:request).call("/sms/json", params: {text: "updown test #1", from: "xxxxx", to: "yyyyy"}, type: Vonage::SMS::Post)
=> #<Vonage::Response:0x000055dbcac41cc8 @entity=nil, @http_response=#<Net::HTTPOK 200 OK readbody=true>>
irb(main):009:0> _.messages
Traceback (most recent call last):
        1: from (irb):9
NoMethodError (undefined method `messages' for #<Vonage::Response:0x000055dbcac41cc8>)

It looks like @entity is nil, which means that the ContentType was likely not as expected: https://github.com/Vonage/vonage-ruby-sdk/blob/ac7568a332c8c5c5c3b46c545544de1ec9f6e91f/lib/vonage/namespace.rb#L204-L210

Could you please revert this API change quickly in order to resume normal operation ? (I'll send this to support too) and then issue a fix in the error handling of the gem too to prevent this from happening further. Ideally you should also fix the nexmo gem or communicate to your users that it's not maintained more visibly, because I just discovered it today because of this bug, and thought: oh shit maybe this is fixed in vonage gem then, no luck ☺


Edit: quick test with curl shows sometimes you're returning "text/plain" content type:

> curl -iX "POST" "https://rest.nexmo.com/sms/json" -d "from=test" -d "text=test" -d "to=test" -d "api_key=xxxxx" -d "api_secret=yyyyy"
HTTP/2 200 
date: Fri, 08 Apr 2022 14:15:26 GMT
content-type: text/plain
content-length: 90
server: envoy
x-nexmo-statuscode: 3
x-envoy-upstream-service-time: 21
x-frame-options: deny
x-xss-protection: 1; mode=block
strict-transport-security: max-age=31536000; includeSubdomains
x-content-type-options: nosniff

{"messages":[{"status":"3","error-text":"to address is not numeric"}],"message-count":"1"}

This is probably the cause as discussed above. Also a 200OK for an error is not great ;)

jarthod commented 2 years ago

Here is the quick monkey patch I wrote to prevent this error from causing too much damage:

# Quick monkey patch to fix Vonage regression:
# https://github.com/Vonage/vonage-ruby-sdk/issues/216

# Consider body starting with "{" as JSON too

module Vonage
  class Namespace
    def parse(response, response_class)
      case response
      when Net::HTTPNoContent
        response_class.new(nil, response)
      when Net::HTTPSuccess
        if response['Content-Type'].split(';').first == 'application/json' || response.body[0] == "{"
          entity = ::JSON.parse(response.body, object_class: Vonage::Entity)

          response_class.new(entity, response)
        else
          response_class.new(nil, response)
        end
      else
        raise Errors.parse(response)
      end
    end
  end
end

It just considers body starting with { as json too even if content type is wrong, probably does not work for all calls but at least for SMS sending it's good enough.

just-the-v commented 2 years ago

We have the same problem here!

jarthod commented 2 years ago

They responded to my support message and gave the incident update: https://api.vonagestatus.com/incidents/wlr2264d67g7 It should be fixed now (I haven't verified yet).

AlexMuir commented 2 years ago

Thanks guys. Status dashboard about as useful as a chocolate teapot today.

superchilled commented 2 years ago

Looks like this was due to an issue which occurred last Friday, where the SMS API was sending responses in "text/plain". This should be resolved now. Are any of you still experiencing this issue.

jarthod commented 2 years ago

Yes that is what I explained, wouldn't it be a good idea to improve the reliability of the gem in this case? at best by parsing the json without relying on the content-type and at least by returning an exception which tells the actual problem? Currently this code path generates an object which is incompatible with the next lines.

superchilled commented 2 years ago

@jarthod yes, absolutely. I'm planning on making some improvements to the gem to try and avoid similar issues in the future.

ccyrille commented 2 years ago

@superchilled beyond a local fix regarding how the content-type is handled, it would seem like a good idea to have integration / end-to-end tests for this SDK. Given the maturity of the product, I am quite surprised it isn't already in place. Is it something that you intend to put in place in a near future ?

superchilled commented 2 years ago

Underlying cause of the SMS API issue addressed in this release