diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 47e5b00..5c38439 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -14,15 +14,6 @@ class BaseController < ActionController::API # cannot read. rescue_from ActionDispatch::Http::Parameters::ParseError, with: :malformed_json - # Anything under /api that does not match a route: a typo, or a client built - # against a newer version of this API. Still answered in JSON. - def unmatched_route - render json: { - error: "not_found", - message: "No route matches #{request.request_method} #{request.path}" - }, status: :not_found - end - private def not_found(error) render json: { error: "not_found", message: error.message }, status: :not_found diff --git a/app/controllers/api/errors_controller.rb b/app/controllers/api/errors_controller.rb new file mode 100644 index 0000000..49542f4 --- /dev/null +++ b/app/controllers/api/errors_controller.rb @@ -0,0 +1,16 @@ +module Api + # Answers the catch-all route under /api. It is the only action that route + # reaches, and it lives here rather than on BaseController so that it is not + # inherited as a public action by every API controller. + class ErrorsController < Api::BaseController + # Anything under /api that does not match a route: a typo, a format the + # API does not speak, or a client built against a newer version of this + # API. Still answered in JSON. + def unmatched_route + render json: { + error: "not_found", + message: "No route matches #{request.request_method} #{request.path}" + }, status: :not_found + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 51533f2..478ee3e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -17,13 +17,19 @@ namespace :v1 do # No new/edit: those serve HTML forms, which this API never renders. They # would also shadow the JSON 404 below with a missing action error. - resources :tasks, except: %i[ new edit ] + # + # The format constraint keeps /tasks and /tasks.json, and sends any other + # extension (/tasks.xml, /tasks/1.xml) to the JSON 404 below instead of + # quietly answering it with JSON as if the extension had been honoured. + resources :tasks, except: %i[ new edit ], constraints: { format: :json } end # Everything under /api answers in JSON, including a wrong path. Without # this, an unknown route falls through to the HTML 404 page and the phone - # gets markup where it expected a body it can parse. - match "*unmatched", to: "base#unmatched_route", via: :all, format: false + # gets markup where it expected a body it can parse. The glob is optional + # because a wildcard needs at least one character: without the parentheses + # the namespace root itself (/api, /api/) would still fall through to HTML. + match "(*unmatched)", to: "errors#unmatched_route", via: :all, format: false end # Reveal health status on /up that returns 200 if the app boots with no exceptions, otherwise 500. diff --git a/test/integration/api_unmatched_route_test.rb b/test/integration/api_unmatched_route_test.rb new file mode 100644 index 0000000..72647e9 --- /dev/null +++ b/test/integration/api_unmatched_route_test.rb @@ -0,0 +1,64 @@ +require "test_helper" + +# Every path under /api must answer in JSON, including the namespace root +# itself and a format the API does not speak: a client that hits the bare +# prefix or asks for .xml should get the same body it can parse for any other +# typo, not the HTML 404 page or a JSON body pretending to be XML. +class ApiUnmatchedRouteTest < ActionDispatch::IntegrationTest + [ "/api", "/api/", "/api/v1", "/api/nope" ].each do |path| + test "GET #{path} returns a json 404" do + get path, as: :json + + assert_json_not_found "GET", path + end + end + + test "POST /api returns a json 404" do + post "/api", as: :json + + assert_json_not_found "POST", "/api" + end + + test "GET /api/v1/tasks.xml returns a json 404" do + get "/api/v1/tasks.xml" + + assert_json_not_found "GET", "/api/v1/tasks.xml" + end + + test "GET /api/v1/tasks/:id.xml returns a json 404 for an existing task" do + path = "/api/v1/tasks/#{tasks(:pending_task).id}.xml" + get path + + assert_json_not_found "GET", path + end + + test "POST /api/v1/tasks.xml does not create a task" do + assert_no_difference "Task.count" do + post "/api/v1/tasks.xml", params: { task: { title: "Ship it" } }, as: :json + end + + assert_json_not_found "POST", "/api/v1/tasks.xml" + end + + test "GET /api/v1/tasks.json still answers" do + get "/api/v1/tasks.json" + + assert_response :success + assert_equal "application/json", response.media_type + end + + test "the catch-all is not an action on every API controller" do + assert_not_includes Api::V1::TasksController.action_methods, "unmatched_route" + assert_includes Api::ErrorsController.action_methods, "unmatched_route" + end + + private + def assert_json_not_found(method, path) + assert_response :not_found + assert_equal "application/json", response.media_type + assert_equal "not_found", response.parsed_body["error"] + # Rails drops a trailing slash from request.path before it reaches the + # controller, so /api/ reports itself as /api. + assert_equal "No route matches #{method} #{path.delete_suffix("/")}", response.parsed_body["message"] + end +end