From b6c0477552576e7f8575bf2dca6e7899b640c012 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 30 Apr 2018 18:33:49 +0200 Subject: TomTom::Matrix: Handle error responses from TomTom Occasionally, the following error would appear in our logs: NoMethodError RouteWayCostWorker/perform Error message NoMethodError: undefined method `each_with_index' for nil:NilClass Stack trace (show Rails) /app/lib/tom_tom/matrix.rb: 83:in `extract_costs_to_way_costs!' /app/lib/tom_tom/matrix.rb: 23:in `matrix' /app/lib/tom_tom.rb: 24:in `matrix' /app/app/services/route_way_cost_calculator.rb: 8:in `calculate!' /app/app/workers/route_way_cost_worker.rb: 12:in `perform' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 167:in `execute_job' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 139:in `block (5 levels) in process' /var/lib/gems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq.rb: 36:in `block in ' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 135:in `block (4 levels) in process' ….0/gems/sidekiq-4.2.10/lib/sidekiq/middleware/chain.rb: 128:in `block in invoke' ….0/gems/sidekiq-4.2.10/lib/sidekiq/middleware/chain.rb: 130:in `block in invoke' …-4.2.10/lib/sidekiq/middleware/server/active_record.rb: 6:in `call' ….0/gems/sidekiq-4.2.10/lib/sidekiq/middleware/chain.rb: 130:in `block in invoke' …idekiq-4.2.10/lib/sidekiq/middleware/server/logging.rb: 10:in `call' ….0/gems/sidekiq-4.2.10/lib/sidekiq/middleware/chain.rb: 130:in `block in invoke' …kiq-4.2.10/lib/sidekiq/middleware/server/retry_jobs.rb: 74:in `call' ….0/gems/sidekiq-4.2.10/lib/sidekiq/middleware/chain.rb: 130:in `block in invoke' ….0/gems/sidekiq-4.2.10/lib/sidekiq/middleware/chain.rb: 133:in `invoke' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 134:in `block (3 levels) in process' …/gems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/logging.rb: 32:in `with_context' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 132:in `block (2 levels) in process' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 183:in `stats' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 131:in `block in process' /var/lib/gems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq.rb: 35:in `block in ' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 126:in `process' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 82:in `process_one' …ems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/processor.rb: 70:in `run' …lib/gems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/util.rb: 17:in `watchdog' …lib/gems/2.3.0/gems/sidekiq-4.2.10/lib/sidekiq/util.rb: 26:in `block in safe_thread' My best guess is that this was caused by TomTom responding with an error, which we weren't handling previously. In that case, the response would be a JSON string, but include an `'error'` field instead of `'matrix'` and `'summary'`. Thus, when we'd try to `matrix_json['matrix']`, it would fail. Add a new method that checks for errors before we try to parse `WayCost`s. If a server error is detected, we log the message to the Rails log and return an empty array. Refs #6884 --- spec/lib/tom_tom/matrix_spec.rb | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'spec') diff --git a/spec/lib/tom_tom/matrix_spec.rb b/spec/lib/tom_tom/matrix_spec.rb index 605f1d254..be63447d4 100644 --- a/spec/lib/tom_tom/matrix_spec.rb +++ b/spec/lib/tom_tom/matrix_spec.rb @@ -149,6 +149,35 @@ RSpec.describe TomTom::Matrix do end end + describe "#check_for_error_response" do + it "raises an RemoteError when an 'error' key is present in the response" do + response_body = { + 'formatVersion' => '0.0.1', + 'error' => { + 'description' => 'Output format: csv is unsupported.' + } + } + + expect { + matrix.check_for_error_response(response_body) + }.to raise_error( + TomTom::Matrix::RemoteError, + 'Output format: csv is unsupported.' + ) + end + + it "doesn't raise errors with a normal response" do + response_body = { + 'formatVersion' => '0.0.1', + 'matrix' => [] + } + + expect { + matrix.check_for_error_response(response_body) + }.to_not raise_error + end + end + describe "#extract_costs_to_way_costs!" do it "puts distance & time costs in way_costs" do way_costs = [ -- cgit v1.2.3 From d53978539c71d598d7cb5f47ad9e3f30a83eb06a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 2 May 2018 19:27:39 +0200 Subject: RouteWayCostCalculator: Don't update `costs` if response errors If there's an API error, we shouldn't update the route's `costs` field. Let's say we've already calculated some costs for a route A. We then edit and re-save A, which triggers a recalculation of the costs. Now the TomTom API responds with an error. We don't want to overwrite our existing costs with an empty array because they could still be useful. In this case, we should instead keep the existing costs we already had. To achieve this, move the `RemoteError` rescue into `RouteWayCostCalculator`, leaving the error unhandled in `TomTom.matrix`. Refs #6884 --- spec/services/route_way_cost_calculator_spec.rb | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) (limited to 'spec') diff --git a/spec/services/route_way_cost_calculator_spec.rb b/spec/services/route_way_cost_calculator_spec.rb index 79b81e34d..593ba29ef 100644 --- a/spec/services/route_way_cost_calculator_spec.rb +++ b/spec/services/route_way_cost_calculator_spec.rb @@ -29,5 +29,30 @@ RSpec.describe RouteWayCostCalculator do expect(route.costs).not_to be_nil expect { JSON.parse(JSON.dump(route.costs)) }.not_to raise_error end + + it "doesn't update route costs when there is a server error" do + route = create(:route) + + stub_request( + :post, + "https://api.tomtom.com/routing/1/matrix/json?key&routeType=shortest&travelMode=bus" + ) + .with( + headers: { + 'Accept'=>'*/*', + 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', + 'Content-Type'=>'application/json', + 'User-Agent'=>'Faraday v0.9.2' + }) + .to_return( + status: 200, + body: "{\"formatVersion\":\"0.0.1\",\"error\":{\"description\":\"Outputformat:csvisunsupported.\"}}", + headers: {} + ) + + RouteWayCostCalculator.new(route).calculate! + + expect(route.costs).to be_nil + end end end -- cgit v1.2.3 From bfc4fab8943a28d2f30084a38d88678d7bfbbac4 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 3 May 2018 17:15:37 +0200 Subject: TomTom::Matrix spec: Fix typo 'an' -> 'a' Refs #6884 --- spec/lib/tom_tom/matrix_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/lib/tom_tom/matrix_spec.rb b/spec/lib/tom_tom/matrix_spec.rb index be63447d4..7000b56c8 100644 --- a/spec/lib/tom_tom/matrix_spec.rb +++ b/spec/lib/tom_tom/matrix_spec.rb @@ -150,7 +150,7 @@ RSpec.describe TomTom::Matrix do end describe "#check_for_error_response" do - it "raises an RemoteError when an 'error' key is present in the response" do + it "raises a RemoteError when an 'error' key is present in the response" do response_body = { 'formatVersion' => '0.0.1', 'error' => { -- cgit v1.2.3 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:

Developer Inactive

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 --- spec/lib/tom_tom/matrix_spec.rb | 64 ++++++++++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 13 deletions(-) (limited to 'spec') 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: '

Developer Inactive

' + ) 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:

Developer Inactive

" + ) + 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 From 9c442cd66c7d12266ba53e85949f775f6b842d2d Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 4 May 2018 12:49:06 +0200 Subject: Move `TomTom::Matrix::RemoteError` to`TomTom::Errors::MatrixRemoteError` I previously tried to correct a circular dependency problem in a057276129b1f62b811743db3b8f867a05241ed3, but that didn't fix it (it was intermittent, and came back). After some wrangling, I've now deduced with some confidence that the problem comes from `RouteWayCostCalculator`, which used `TomTom::Matrix::RemoteError`. From the way it looks, this seems to mess up the Rails autoloader since `tom_tom.rb` will try to load the `Matrix` class from the `TomTom.matrix` call above. Or something. In an attempt to fix the circular dependency error for real this time, move the error class to a completely separate module from `Matrix`, and refer to this when we need to use the error class. Refs #6884 --- spec/lib/tom_tom/matrix_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/lib/tom_tom/matrix_spec.rb b/spec/lib/tom_tom/matrix_spec.rb index 6564b82fd..f914cf7ff 100644 --- a/spec/lib/tom_tom/matrix_spec.rb +++ b/spec/lib/tom_tom/matrix_spec.rb @@ -150,7 +150,7 @@ RSpec.describe TomTom::Matrix do end describe "#check_for_error_response" do - it "raises a RemoteError when an 'error' key is present in the response" do + it "raises a MatrixRemoteError when an 'error' key is present in the response" do response = double( 'response', status: 200, @@ -165,12 +165,12 @@ RSpec.describe TomTom::Matrix do expect { matrix.check_for_error_response(response) }.to raise_error( - TomTom::Matrix::RemoteError, + TomTom::Errors::MatrixRemoteError, "status: #{response.status}, message: Output format: csv is unsupported." ) end - it "raises a RemoteError when response status is not 200" do + it "raises a MatrixRemoteError when response status is not 200" do response = double( 'response', status: 403, @@ -180,7 +180,7 @@ RSpec.describe TomTom::Matrix do expect { matrix.check_for_error_response(response) }.to raise_error( - TomTom::Matrix::RemoteError, + TomTom::Errors::MatrixRemoteError, "status: #{response.status}, body:

Developer Inactive

" ) end -- cgit v1.2.3