jgorset / facebook-messenger

Definitely the best way to make Bots on Facebook Messenger with Ruby
MIT License
962 stars 211 forks source link

Fix: Subscriptions with empty subscribed_fields results in 500 #244

Closed lluft closed 5 years ago

lluft commented 5 years ago

Facebooks Api can't handle an empty array in the query params resulting in a 500:

19:53:50 web.1  | <- "POST /v3.2/me/subscribed_apps?access_token=XXX&subscribed_fields[]= HTTP/1.1\r\nAccept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3\r\nAccept: */*\r\nUser-Agent: Ruby\r\nConnection: close\r\nHost: grap
h.facebook.com\r\nContent-Length: 0\r\nContent-Type: application/x-www-form-urlencoded\r\n\r\n"
19:53:50 web.1  | <- ""
19:53:50 web.1  | -> "HTTP/1.1 500 Internal Server Error\r\n"
19:53:50 web.1  | -> "x-page-usage: {\"call_count\":2,\"total_cputime\":1,\"total_time\":1}\r\n"
19:53:50 web.1  | -> "x-app-usage: {\"call_count\":0,\"total_cputime\":0,\"total_time\":0}\r\n"
19:53:50 web.1  | -> "WWW-Authenticate: OAuth \"Facebook Platform\" \"unknown_error\" \"An unknown error has occurred.\"\r\n"
19:53:50 web.1  | -> "Content-Type: application/json; charset=UTF-8\r\n"
19:53:50 web.1  | -> "facebook-api-version: v3.2\r\n"
19:53:50 web.1  | -> "Strict-Transport-Security: max-age=15552000; preload\r\n"
19:53:50 web.1  | -> "Pragma: no-cache\r\n"
19:53:50 web.1  | -> "x-fb-rev: 1000627010\r\n"
19:53:50 web.1  | -> "Access-Control-Allow-Origin: *\r\n"
19:53:50 web.1  | -> "Cache-Control: no-store\r\n"
19:53:50 web.1  | -> "x-fb-trace-id: XXX\r\n"
19:53:50 web.1  | -> "x-fb-request-id: XXX\r\n"
19:53:50 web.1  | -> "Expires: Sat, 01 Jan 2000 00:00:00 GMT\r\n"
19:53:50 web.1  | -> "X-FB-Debug: XXX==\r\n"
19:53:50 web.1  | -> "Date: Sat, 20 Apr 2019 17:53:50 GMT\r\n"
19:53:50 web.1  | -> "Connection: close\r\n"
19:53:50 web.1  | -> "Content-Length: 114\r\n"
19:53:50 web.1  | -> "\r\n"
19:53:50 web.1  | reading 114 bytes...
19:53:50 web.1  | -> "{\"error\":{\"message\":\"An unknown error has occurred.\",\"type\":\"OAuthException\",\"code\":1,\"fbtrace_id\":\"XXX\"}}"
19:53:50 web.1  | read 114 bytes
19:53:50 web.1  | Conn close

Changing to sending the payload in the body fixes this and returns the correct error message:

20:01:56 web.1  | -> "HTTP/1.1 400 Bad Request\r\n"
20:01:56 web.1  | -> "x-page-usage: {\"call_count\":2,\"total_cputime\":1,\"total_time\":1}\r\n"
20:01:56 web.1  | -> "x-app-usage: {\"call_count\":0,\"total_cputime\":0,\"total_time\":0}\r\n"
20:01:56 web.1  | -> "WWW-Authenticate: OAuth \"Facebook Platform\" \"invalid_request\" \"(#100) The parameter subscribed_fields is required.\"\r\n"
20:01:56 web.1  | -> "Content-Type: text/javascript; charset=UTF-8\r\n"
20:01:56 web.1  | -> "facebook-api-version: v3.2\r\n"
20:01:56 web.1  | -> "Strict-Transport-Security: max-age=15552000; preload\r\n"
20:01:56 web.1  | -> "Pragma: no-cache\r\n"
20:01:56 web.1  | -> "x-fb-rev: 1000627010\r\n"
20:01:56 web.1  | -> "Access-Control-Allow-Origin: *\r\n"
20:01:56 web.1  | -> "Cache-Control: no-store\r\n"
20:01:56 web.1  | -> "x-fb-trace-id: XXX\r\n"
20:01:56 web.1  | -> "x-fb-request-id: XXX\r\n"
20:01:56 web.1  | -> "Expires: Sat, 01 Jan 2000 00:00:00 GMT\r\n"
20:01:56 web.1  | -> "X-FB-Debug:XXX==\r\n"
20:01:56 web.1  | -> "Date: Sat, 20 Apr 2019 18:01:56 GMT\r\n"
20:01:56 web.1  | -> "Connection: close\r\n"
20:01:56 web.1  | -> "Content-Length: 137\r\n"
20:01:56 web.1  | -> "\r\n"
20:01:56 web.1  | reading 137 bytes...
20:01:56 web.1  | -> "{\"error\":{\"message\":\"(#100) The parameter subscribed_fields is required.\",\"type\":\"OAuthException\",\"code\":100,\"fbtrace_id\":\"XXX\"}}"
20:01:56 web.1  | read 137 bytes
20:01:56 web.1  | Conn close
jgorset commented 5 years ago

Thanks @lluft! I have just two questions about Facebook's behaviour for the tests, but otherwise this looks good to merge.

lluft commented 5 years ago

@jgorset addressed your comments. Should be ready to merge, right?

jgorset commented 5 years ago

Thanks @lluft! This is a good change, and I've merged it.

I double checked against Facebook's API just to make sure that our testing stubs reflect what the API actually returns, and it looks like Facebook actually returns HTTP 200 either way, so I updated them accordingly.