From 51c776d88ffc2c7f25ed540297d248635fc9a265 Mon Sep 17 00:00:00 2001 From: Mason Ballengee Date: Wed, 20 May 2026 16:00:23 -0400 Subject: [PATCH] Improve error handling for routing errors Related issue: #5037 After a lot of testing, it was determined that error handling for most/all API routes was already fairly robust. The most likely cause of the error reporting issue was users making their request with the wrong HTTP method, e.x. Media Object update is required to be a PUT. Users making the request via PATCH would encounter a 500 error for the incorrect route. This commit adds handling for this situation by creating a new route that catches all requests that don't match an existing route and directing them to specific error handling to provide a 404 page or JSON error, depending on request format. Follow-up work could be to audit our API endpoints and see if there are any it might make sense to add, such as a PATCH option for API updates of Media Objects. --- app/controllers/application_controller.rb | 10 ++++++ config/routes.rb | 7 ++++ .../admin_collections_controller_spec.rb | 9 ++++- .../admin_units_controller_spec.rb | 2 +- .../media_objects_controller_spec.rb | 22 ++++++++++++ spec/requests/routing_error_spec.rb | 35 +++++++++++++++++++ spec/routing/media_objects_routing_spec.rb | 3 ++ 7 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 spec/requests/routing_error_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0b172c792b..f1f6027558 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -229,6 +229,16 @@ def current_ability end end + def handle_routing_error + if request.format == :json + render json: { errors: ["API action does not exist. Check that your URL and HTTP request method are correct."] }, status: :not_found + elsif request.format == :html + render file: Rails.root.join('public', '404.html').to_s, status: :not_found, layout: false + else + head :not_found + end + end + def configure_permitted_parameters devise_parameter_sanitizer.permit(:sign_up) do |user_params| user_params.permit(:username, :email, :password, :password_confirmation) diff --git a/config/routes.rb b/config/routes.rb index 33b2b59afb..4c83b693e1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -274,4 +274,11 @@ end end end + + # Routing errors are triggered in the middleware stack so can't be handled by + # a regular rescue_from block. Constraint is necessary to not break retrieval + # of things from active storage. + match '*unmatched', to: 'application#handle_routing_error', via: :all, constraints: lambda { |req| + req.path.exclude? 'rails/active_storage' + } end diff --git a/spec/controllers/admin_collections_controller_spec.rb b/spec/controllers/admin_collections_controller_spec.rb index 0ac38ebe0c..0c57ce0b53 100644 --- a/spec/controllers/admin_collections_controller_spec.rb +++ b/spec/controllers/admin_collections_controller_spec.rb @@ -417,13 +417,20 @@ collection = Admin::Collection.find(JSON.parse(response.body)['id']) expect(collection.managers).to eq([]) end - it "should return 422 if collection creation failed" do + it "should return 422 if unit not provided" do post 'create', params: { format:'json', admin_collection: { name: collection.name, description: collection.description } } expect(response.status).to eq(422) expect(JSON.parse(response.body)).to include('errors') expect(JSON.parse(response.body)["errors"].class).to eq Array expect(JSON.parse(response.body)["errors"].first.class).to eq String end + it "should return 422 if name not provided" do + post 'create', params: { format:'json', admin_collection: { description: collection.description, unit_id: collection.unit.id } } + expect(response.status).to eq(422) + expect(JSON.parse(response.body)).to include('errors') + expect(JSON.parse(response.body)["errors"].class).to eq Array + expect(JSON.parse(response.body)["errors"].first.class).to eq String + end end context 'when user does not have permissions for unit' do diff --git a/spec/controllers/admin_units_controller_spec.rb b/spec/controllers/admin_units_controller_spec.rb index 3821716d58..7e473941af 100644 --- a/spec/controllers/admin_units_controller_spec.rb +++ b/spec/controllers/admin_units_controller_spec.rb @@ -332,7 +332,7 @@ unit = Admin::Unit.find(JSON.parse(response.body)['id']) expect(unit.unit_admins).to eq([administrator.username]) end - it "should return 422 if unit creation failed" do + it "should return 422 if name not provided" do post 'create', params: { format: 'json', admin_unit: { description: unit.description } } expect(response.status).to eq(422) expect(JSON.parse(response.body)).to include('errors') diff --git a/spec/controllers/media_objects_controller_spec.rb b/spec/controllers/media_objects_controller_spec.rb index fd8b321883..48afc0182f 100644 --- a/spec/controllers/media_objects_controller_spec.rb +++ b/spec/controllers/media_objects_controller_spec.rb @@ -302,6 +302,17 @@ expect(JSON.parse(response.body)["errors"].class).to eq Array expect(JSON.parse(response.body)["errors"].first.class).to eq String end + it "should return 422 if required fields are missing" do + media_object = FactoryBot.create(:published_media_object) + fields = {} + descMetadata_fields.each { |f| fields[f] = media_object.send(f) } + fields[:title] = nil + post 'create', params: { format: 'json', fields: fields, files: [master_file], collection_id: collection.id, publish: true } + expect(response.status).to eq(422) + expect(JSON.parse(response.body)).to include('errors') + expect(JSON.parse(response.body)["errors"].class).to eq Array + expect(JSON.parse(response.body)["errors"].first.class).to eq String + end it "should create a new media_object" do # master_file_obj = FactoryBot.create(:master_file, master_file.slice(:files)) media_object = FactoryBot.create(:media_object)#, sections: [master_file_obj]) @@ -569,6 +580,17 @@ expect(JSON.parse(response.body)["errors"].class).to eq Array expect(JSON.parse(response.body)["errors"].first.class).to eq String end + it "should return 422 if required fields are missing" do + media_object = FactoryBot.create(:published_media_object) + fields = {} + descMetadata_fields.each { |f| fields[f] = media_object.send(f) } + fields[:title] = nil + put 'json_update', params: { format: 'json', id: media_object.id, fields: { title: nil }, files: [master_file], collection_id: collection.id } + expect(response.status).to eq(422) + expect(JSON.parse(response.body)).to include('errors') + expect(JSON.parse(response.body)["errors"].class).to eq Array + expect(JSON.parse(response.body)["errors"].first.class).to eq String + end context "as an authorized non-admin user" do let(:user) { FactoryBot.create(:user) } diff --git a/spec/requests/routing_error_spec.rb b/spec/requests/routing_error_spec.rb new file mode 100644 index 0000000000..8c6ef00559 --- /dev/null +++ b/spec/requests/routing_error_spec.rb @@ -0,0 +1,35 @@ +# Copyright 2011-2026, The Trustees of Indiana University and Northwestern +# University. Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software distributed +# under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR +# CONDITIONS OF ANY KIND, either express or implied. See the License for the +# specific language governing permissions and limitations under the License. +# --- END LICENSE_HEADER BLOCK --- + +require 'rails_helper.rb' + +describe 'Routing Error Handling', type: :request do + it 'responds with 404 page for html request' do + get '/fake/route', params: { format: :html } + expect(response.status).to eq(404) + expect(response.body).to include('The page you were looking for doesn’t exist (404 Not found)') + end + + it 'responds with json error for json request' do + get '/fake/route', params: { format: :json } + expect(response.status).to eq(404) + expect(JSON.parse(response.body)).to eq("errors" => ["API action does not exist. Check that your URL and HTTP request method are correct."]) + end + + it 'responds with 404 status for other request' do + get '/fake/route', params: { format: :atom } + expect(response.status).to eq(404) + expect(response.body).to be_empty + end +end diff --git a/spec/routing/media_objects_routing_spec.rb b/spec/routing/media_objects_routing_spec.rb index 85e74773b2..d0e2d556f3 100644 --- a/spec/routing/media_objects_routing_spec.rb +++ b/spec/routing/media_objects_routing_spec.rb @@ -19,5 +19,8 @@ it "routes to #move_preview.json" do expect(:get => "/media_objects/abc1234/move_preview.json").to route_to("media_objects#move_preview", id: 'abc1234', format: 'json') end + it "handles routing errors" do + expect(post: "/media_objects/abc1234").to route_to("application#handle_routing_error", unmatched: "media_objects/abc1234") + end end end