From ad49ad52ee8e7cfbf4dc3f1bc34c533e186100b9 Mon Sep 17 00:00:00 2001
From: Teddy Wing
Date: Thu, 3 May 2018 18:21:04 +0200
Subject: TomTom::Matrix#check_for_error_response: Handle HTTP error status
 codes

We might not always get a nicely formatted JSON
`['error']['description']` response body. Sometimes, like for example
when you use an incorrect API key, even with an 'application/json'
content type, TomTom will respond with:

    <h1>Developer Inactive</h1>

What?

In that case, the response has a 403 status. In addition to checking for
an error in the response, should also be checking for the HTTP status
code.

Log the status code in the exception to give us more information about
what went wrong.

Update our existing tests now that `#check_for_error_response` takes a
response object instead of a JSON string.

Refs #6884
---
 lib/tom_tom/matrix.rb           | 19 +++++++-----
 spec/lib/tom_tom/matrix_spec.rb | 64 ++++++++++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 20 deletions(-)

diff --git a/lib/tom_tom/matrix.rb b/lib/tom_tom/matrix.rb
index 40191aa56..d0b476d84 100644
--- a/lib/tom_tom/matrix.rb
+++ b/lib/tom_tom/matrix.rb
@@ -24,14 +24,12 @@ module TomTom
         req.body = build_request_body(points)
       end
 
-      matrix_json = JSON.parse(response.body)
-
-      check_for_error_response(matrix_json)
+      check_for_error_response(response)
 
       extract_costs_to_way_costs!(
         way_costs,
         points_with_ids,
-        matrix_json
+        JSON.parse(response.body)
       )
     end
 
@@ -84,9 +82,16 @@ module TomTom
       })
     end
 
-    def check_for_error_response(matrix_json)
-      if matrix_json.has_key?('error')
-        raise RemoteError, matrix_json['error']['description']
+    def check_for_error_response(response)
+      if response.status != 200
+        raise RemoteError, "status: #{response.status}, body: #{response.body}"
+      end
+
+      json = JSON.parse(response.body)
+
+      if json.has_key?('error')
+        raise RemoteError,
+          "status: #{response.status}, message: #{json['error']['description']}"
       end
     end
 
diff --git a/spec/lib/tom_tom/matrix_spec.rb b/spec/lib/tom_tom/matrix_spec.rb
index 7000b56c8..6564b82fd 100644
--- a/spec/lib/tom_tom/matrix_spec.rb
+++ b/spec/lib/tom_tom/matrix_spec.rb
@@ -151,29 +151,67 @@ RSpec.describe TomTom::Matrix do
 
   describe "#check_for_error_response" do
     it "raises a RemoteError when an 'error' key is present in the response" do
-      response_body = {
-        'formatVersion' => '0.0.1',
-        'error' => {
-          'description' => 'Output format: csv is unsupported.'
-        }
-      }
+      response = double(
+        'response',
+        status: 200,
+        body: JSON.dump({
+          'formatVersion' => '0.0.1',
+          'error' => {
+            'description' => 'Output format: csv is unsupported.'
+          }
+        })
+      )
+
+      expect {
+        matrix.check_for_error_response(response)
+      }.to raise_error(
+        TomTom::Matrix::RemoteError,
+        "status: #{response.status}, message: Output format: csv is unsupported."
+      )
+    end
+
+    it "raises a RemoteError when response status is not 200" do
+      response = double(
+        'response',
+        status: 403,
+        body: '<h1>Developer Inactive</h1>'
+      )
 
       expect {
-        matrix.check_for_error_response(response_body)
+        matrix.check_for_error_response(response)
       }.to raise_error(
         TomTom::Matrix::RemoteError,
-        'Output format: csv is unsupported.'
+        "status: #{response.status}, body: <h1>Developer Inactive</h1>"
+      )
+    end
+
+    it "doesn't raise an error when response status is 200" do
+      response = double(
+        'response',
+        status: 200,
+        body: JSON.dump({
+          'formatVersion' => '0.0.1',
+          'matrix' => []
+        })
       )
+
+      expect {
+        matrix.check_for_error_response(response)
+      }.not_to raise_error
     end
 
     it "doesn't raise errors with a normal response" do
-      response_body = {
-        'formatVersion' => '0.0.1',
-        'matrix' => []
-      }
+      response = double(
+        'response',
+        status: 200,
+        body: JSON.dump({
+          'formatVersion' => '0.0.1',
+          'matrix' => []
+        })
+      )
 
       expect {
-        matrix.check_for_error_response(response_body)
+        matrix.check_for_error_response(response)
       }.to_not raise_error
     end
   end
-- 
cgit v1.2.3