From 7822f986681c932d11563e1d111f0c13120ff662 Mon Sep 17 00:00:00 2001 From: Mike Heft Date: Mon, 8 Jul 2024 13:13:49 -0600 Subject: [PATCH] 26 error handling (#39) * Work on adding custom error handling * Add error handling to middleware stack * Work on dynamic error definition * fix error generation * Work on ci using middleware * try different path to get to load in gh ci * Try adding require to rails helper * move pre loading * revert addition to require middleware in rails helper * Add debuggin steps * fix typo in debugger action * try loading manually in application.rb * try loading in different dir * fix typo * fix rubocop errors * remove debugger actions * Add spec for different error * update test title * Add script to start redis in background and start rails server * fix rubocop --- .rubocop.yml | 6 +++ .../drivers/selectable_rides_controller.rb | 4 +- app/serializers/api_exception_serializer.rb | 11 ++++ app_start.sh | 27 ++++++++++ config/application.rb | 5 ++ lib/api_exception/base_exception.rb | 22 ++++++++ lib/api_exception/error_mapper.rb | 31 +++++++++++ lib/client/helpers.rb | 21 ++++++++ lib/middleware/error_handler.rb | 33 ++++++++++++ lib/rides/commands/get_routes_data.rb | 53 +++++++++++++++---- lib/rides/commands/rank_rides.rb | 6 ++- spec/errors/error_spec.rb | 20 +++++++ spec/rails_helper.rb | 2 + .../requests/drivers/selectable_rides_spec.rb | 11 ++++ 14 files changed, 237 insertions(+), 15 deletions(-) create mode 100644 app/serializers/api_exception_serializer.rb create mode 100755 app_start.sh create mode 100644 lib/api_exception/base_exception.rb create mode 100644 lib/api_exception/error_mapper.rb create mode 100644 lib/client/helpers.rb create mode 100644 lib/middleware/error_handler.rb create mode 100644 spec/errors/error_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 397c700..fb716ce 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -37,19 +37,25 @@ Layout/SpaceInsideBlockBraces: Lint/UnusedMethodArgument: AutoCorrect: false Metrics/AbcSize: + CountRepeatedAttributes: false + Max: 20 Exclude: - "app/models/application_record.rb" Metrics/BlockLength: Exclude: - "spec/**/*.rb" - "lib/rubocop/cop/custom/*.rb" +Metrics/ClassLength: + Enabled: false Metrics/CyclomaticComplexity: Exclude: - "lib/rubocop/cop/custom/*.rb" Metrics/MethodLength: + Max: 20 Exclude: - "lib/rubocop/cop/custom/*.rb" - "app/models/application_record.rb" + - "app/client/helpers.rb" Metrics/PerceivedComplexity: Exclude: - "lib/rubocop/cop/custom/*.rb" diff --git a/app/controllers/drivers/selectable_rides_controller.rb b/app/controllers/drivers/selectable_rides_controller.rb index 623b89a..dd3aad7 100644 --- a/app/controllers/drivers/selectable_rides_controller.rb +++ b/app/controllers/drivers/selectable_rides_controller.rb @@ -4,8 +4,8 @@ module Drivers class SelectableRidesController < ApplicationController def index rides = Rides::Commands::RankRides.call(driver:)[offset, limit] - opts = { include: %i[from_address to_address] } - render json: RidePojoSerializer.new(rides, opts) + + render json: RidePojoSerializer.new(rides, { include: %i[from_address to_address] }) end private def driver diff --git a/app/serializers/api_exception_serializer.rb b/app/serializers/api_exception_serializer.rb new file mode 100644 index 0000000..4d45b1e --- /dev/null +++ b/app/serializers/api_exception_serializer.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class ApiExceptionSerializer + include JSONAPI::Serializer + set_type :error + set_id do |error, _params| + error.status + end + + attributes :status, :code, :message +end diff --git a/app_start.sh b/app_start.sh new file mode 100755 index 0000000..35612f4 --- /dev/null +++ b/app_start.sh @@ -0,0 +1,27 @@ +#!/bin/bash + +port=3000 # Default port + +# Parse command line options +while getopts ":p:" opt; do + case ${opt} in + p) + port=${OPTARG} + ;; + \?) + echo "Invalid option: -$OPTARG" >&2 + exit 1 + ;; + :) + echo "Option -$OPTARG requires an argument." >&2 + exit 1 + ;; + esac +done + +# Start Redis in daemon mode +redis-server --daemonize yes + +# Start Rails server on specified port +echo "Starting Rails server on port $port" +bundle exec rails server -p $port diff --git a/config/application.rb b/config/application.rb index d3c1b33..f109a58 100644 --- a/config/application.rb +++ b/config/application.rb @@ -18,6 +18,8 @@ # you've limited to :test, :development, or :production. Bundler.require(*Rails.groups) +require_relative "../lib/middleware/error_handler" + module RouteRater class Application < Rails::Application # Initialize configuration defaults for originally generated Rails version. @@ -28,6 +30,9 @@ class Application < Rails::Application # Common ones are `templates`, `generators`, or `middleware`, for example. config.autoload_lib(ignore: %w(assets tasks)) config.autoload_paths << Rails.root.join('lib') + config.autoload_paths += %W(#{config.root}/lib) + + config.middleware.use Middleware::ErrorHandler # Configuration for the application, engines, and railties goes here. # diff --git a/lib/api_exception/base_exception.rb b/lib/api_exception/base_exception.rb new file mode 100644 index 0000000..7707887 --- /dev/null +++ b/lib/api_exception/base_exception.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module ApiException + class BaseException < StandardError + include ActiveModel::Serialization + attr_reader :status, :code, :message + + def initialize(message, code = 500, status = :internal_error) + super(message) + @status = status + @code = code + @message = message + end + end + + class HTTPRequestError < BaseException; end + class JSONParserError < BaseException; end + class NoBlockGivenError < BaseException; end + class RecordNotFound < BaseException; end + class RetryError < BaseException; end + class RideCountMismatchError < BaseException; end +end diff --git a/lib/api_exception/error_mapper.rb b/lib/api_exception/error_mapper.rb new file mode 100644 index 0000000..8832fa1 --- /dev/null +++ b/lib/api_exception/error_mapper.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module ApiException + module ErrorMapper + def map_error(exception) + custom_error_class_name = "ApiException::#{exception.class.name.split('::').last}" + if Object.const_defined?(custom_error_class_name) + custom_error_class_name.constantize + else + define_error_class(custom_error_class_name) + end + end + + private def define_error_class(class_name) + clean_class_name = class_name.split("::").last.gsub(/[^\w]/, "") # Clean up class name + + # Define the error class if it doesn't exist + unless Object.const_defined?(clean_class_name) + error_class = Class.new(ApiException::BaseException) do + def initialize(msg = nil, code = 500, status = :internal_error) + super(msg, code, status) + end + end + + ApiException.const_set(clean_class_name, error_class) + end + + ApiException.const_get(clean_class_name) + end + end +end diff --git a/lib/client/helpers.rb b/lib/client/helpers.rb new file mode 100644 index 0000000..37b8ce5 --- /dev/null +++ b/lib/client/helpers.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Client + module Helpers + def with_retries(max_retries: 10, &blk) + raise ApiException::NoBlockGivenError, "Must provide a block" if blk.blank? + + retries = 0 + begin + yield + rescue StandardError => _e + raise ApiException::RetryError unless retries <= max_retries + + retries += 1 + max_sleep_seconds = Float(2**retries) + sleep rand(0..max_sleep_seconds) + retry + end + end + end +end diff --git a/lib/middleware/error_handler.rb b/lib/middleware/error_handler.rb new file mode 100644 index 0000000..dc7a1d3 --- /dev/null +++ b/lib/middleware/error_handler.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "./lib/api_exception/error_mapper" + +module Middleware + class ErrorHandler + include ApiException::ErrorMapper # Include the module where map_error and define_error_class are defined + + def initialize(app) + @app = app + end + + def call(env) + @app.call(env) + rescue StandardError => e + handle_exception(e) + end + + private def handle_exception(exception) + error_class = map_error(exception) + raise error_class, exception.message + rescue ApiException::RecordNotFound => e + render_error_response(e, 404, :not_found) + rescue StandardError => e + render_error_response(e, 500, e.status || :internal_error) + end + + private def render_error_response(error, status, code) + [status, { "Content-Type" => "application/json" }, + [{ error: { status:, code:, message: error.message } }.to_json]] + end + end +end diff --git a/lib/rides/commands/get_routes_data.rb b/lib/rides/commands/get_routes_data.rb index 659147c..696575a 100644 --- a/lib/rides/commands/get_routes_data.rb +++ b/lib/rides/commands/get_routes_data.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "./lib/api_exception/base_exception" + module Rides module Commands # Makes a request to the Google API to obtain the route information for all the @@ -8,6 +10,8 @@ module Commands # Even though we only care about one combo per route, it is still more efficient to do it in this manner. # We then compare the indexes and get the ones that match, which gives us our original desired routes. class GetRoutesData < BaseCommand + include Client::Helpers + CACHE = Cache::Store DIRECTIONS_API_URL = "https://routes.googleapis.com/distanceMatrix/v2:computeRouteMatrix" DEFAULT_HEADERS = { @@ -16,6 +20,7 @@ class GetRoutesData < BaseCommand "Content-Type" => "application/json" }.freeze DEFAULT_REQUEST_PARAMS = { routingPreference: "TRAFFIC_AWARE", travelMode: "DRIVE" }.freeze + MAX_NUM_ELEMENTS = 25 def call(rides:) data = get_route_data_for_rides(rides) @@ -30,7 +35,14 @@ def call(rides:) # The response keeps the array positioning on the return. Since we're getting a matrix # of routes back, we only want the ones where we explicitly have a 'Ride'. This means that # we want the computations where the indicies match. - data = data.select { _1[:originIndex] == _1[:destinationIndex] } + data = data.flatten.select { _1[:originIndex] == _1[:destinationIndex] } + if data.length != rides.length + raise ApiException::RideCountMismatchError, + "The number of routes does not match the number of rides.", + 500, + :internal_error + end + data = transform_keys!(data) combine_routes_data!(data, rides) @@ -56,9 +68,12 @@ def call(rides:) end private def get_route_data_for_rides(rides) - body = build_request_body(rides) - response_body = routes_data(body) - JSON.parse(response_body, symbolize_names: true) + batches = rides.each_slice(MAX_NUM_ELEMENTS).to_a + batches.map do |batch| + body = build_request_body(batch) + response_body = routes_data(body) + JSON.parse(response_body, symbolize_names: true) + end end private def routes_data(body) @@ -85,16 +100,32 @@ def call(rides:) end private def fetch_routes_data(key, body) - response = connection.post( - DIRECTIONS_API_URL, - body.merge(DEFAULT_REQUEST_PARAMS) - ) - body = response.body - cache_response!(key, body) + response = with_retries do + connection.post( + DIRECTIONS_API_URL, + body.merge(DEFAULT_REQUEST_PARAMS) + ) + end - body + handle_response(response, key) end + private def handle_response(response, key) + if response.status != 200 + result = JSON.parse(response.body, symbolize_names: true) + error = result.first[:error] + raise ApiException::HTTPRequestError.new(error[:message], error[:status].downcase.to_sym, error[:code]) + else + body = response.body + cache_response!(key, body) + + body + end + rescue JSON::ParserError => e + message = e.message + Rails.logger.warn "Attemped to parse invalid JSON: #{message}" + raise ApiException::JSONParserError, message + end private def cache_response!(key, value) CACHE.set(key, value) end diff --git a/lib/rides/commands/rank_rides.rb b/lib/rides/commands/rank_rides.rb index e125ef8..5f7a3ef 100644 --- a/lib/rides/commands/rank_rides.rb +++ b/lib/rides/commands/rank_rides.rb @@ -42,9 +42,11 @@ def call(driver:) rides = route_data(driver) if commutes.count != rides.count - raise RideCountMismatchError, + raise ApiException::RideCountMismatchError, "The number of rides doesn't match the number of commute rides." \ - "Please check the ride(s) configuration and try again." + "Please check the ride(s) configuration and try again.", + 500, + :internal_error end combine_rides!(rides, commutes) diff --git a/spec/errors/error_spec.rb b/spec/errors/error_spec.rb new file mode 100644 index 0000000..e72175b --- /dev/null +++ b/spec/errors/error_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "Errors", :skip_geocode, type: :request do + it "raises JSONParserError and responds with correct error shape" do + expect(Rides::Commands::GetRoutesData).to receive(:call).and_raise( + ApiException::JSONParserError.new("Attemped to parse invalid JSON:"), 500 + ) + driver = create(:driver) + get "/drivers/#{driver.id}/selectable_rides" + + expect(response).to have_http_status(500) + result = JSON.parse(response.body, symbolize_names: true) + + expect(result[:error].keys).to include(:status, :code, :message) + expect(result.dig(:error, :message)).to eq("Attemped to parse invalid JSON:") + expect(result.dig(:error, :code)).to eq("internal_error") + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index ef9b708..815115d 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -2,9 +2,11 @@ # This file is copied to spec/ when you run 'rails generate rspec:install' ENV["RAILS_ENV"] ||= "test" + require_relative "../config/environment" # Prevent database truncation if the environment is production abort("The Rails environment is running in production mode!") if Rails.env.production? + require "rspec/rails" require "spec_helper" require "webmock/rspec" diff --git a/spec/requests/drivers/selectable_rides_spec.rb b/spec/requests/drivers/selectable_rides_spec.rb index 260d22d..cf94cdf 100644 --- a/spec/requests/drivers/selectable_rides_spec.rb +++ b/spec/requests/drivers/selectable_rides_spec.rb @@ -3,6 +3,17 @@ require "rails_helper" RSpec.describe "Drivers::Rides", type: :request do + describe "errors" do + it "raises NotFoundError when unable to find driver" do + get "/drivers/1/selectable_rides" + expect(response).to have_http_status(:not_found) + + result = JSON.parse(response.body, symbolize_names: true) + expect(result[:error].keys).to include(:status, :code, :message) + expect(result.dig(:error, :message)).to eq("Couldn't find Driver with 'id'=1") + end + end + describe "GET /drivers/:driver_id/rides" do it "returns ranked rides" do VCR.use_cassette("ranked_rides") do