From 54fd63d3b568843a50c55a976bc07474d33534d1 Mon Sep 17 00:00:00 2001 From: Trey Pendragon Date: Thu, 7 Feb 2019 12:43:49 -0800 Subject: [PATCH] Missing resources should redirect and error. This should prevent a whole suite of Honeybadger errors, and avoid displaying 500 errors to users when they can't do anything about it. Closes #2534 --- app/controllers/application_controller.rb | 10 ++++++++++ .../ephemera_box/ephemera_boxes_controller_spec.rb | 6 ++++-- .../ephemera_fields_controller_spec.rb | 6 ++++-- .../ephemera_folders_controller_spec.rb | 6 ++++-- .../ephemera_projects_controller_spec.rb | 6 ++++-- .../ephemera_terms_controller_spec.rb | 6 ++++-- .../ephemera_vocabularies_controller_spec.rb | 6 ++++-- .../file_set/file_sets_controller_spec.rb | 3 ++- .../raster_resources_controller_spec.rb | 6 ++++-- .../scanned_map/scanned_maps_controller_spec.rb | 9 ++++++--- .../scanned_resources_controller_spec.rb | 3 ++- .../vector_resources_controller_spec.rb | 6 ++++-- .../controllers/base_resource_controller.rb | 14 ++++++++++---- spec/support/matchers/redirect_to_not_found.rb | 6 ++++++ 14 files changed, 68 insertions(+), 25 deletions(-) create mode 100644 spec/support/matchers/redirect_to_not_found.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 488c2eab66..fe206bc744 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -32,6 +32,16 @@ def deny_resource_access(exception) end end + rescue_from Valkyrie::Persistence::ObjectNotFoundError, with: :resource_not_found + def resource_not_found(_exception) + respond_to do |format| + format.json { head :not_found } + format.html do + redirect_to root_url, alert: "The requested resource does not exist." + end + end + end + # Figgy has no use cases for having unique shared searches, and this prevents # the user list from growing out of control. def guest_user diff --git a/spec/resources/ephemera_box/ephemera_boxes_controller_spec.rb b/spec/resources/ephemera_box/ephemera_boxes_controller_spec.rb index 64191d9424..8c18590650 100644 --- a/spec/resources/ephemera_box/ephemera_boxes_controller_spec.rb +++ b/spec/resources/ephemera_box/ephemera_boxes_controller_spec.rb @@ -154,7 +154,8 @@ end context "when a ephemera box doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -193,7 +194,8 @@ end context "when a ephemera box doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end it_behaves_like "a workflow controller", :ephemera_box diff --git a/spec/resources/ephemera_field/ephemera_fields_controller_spec.rb b/spec/resources/ephemera_field/ephemera_fields_controller_spec.rb index 188b200010..c001a5a50c 100644 --- a/spec/resources/ephemera_field/ephemera_fields_controller_spec.rb +++ b/spec/resources/ephemera_field/ephemera_fields_controller_spec.rb @@ -111,7 +111,8 @@ end context "when a ephemera field doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -151,7 +152,8 @@ end context "when a ephemera field doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/ephemera_folder/ephemera_folders_controller_spec.rb b/spec/resources/ephemera_folder/ephemera_folders_controller_spec.rb index df2ed8ec07..9a9364ca11 100644 --- a/spec/resources/ephemera_folder/ephemera_folders_controller_spec.rb +++ b/spec/resources/ephemera_folder/ephemera_folders_controller_spec.rb @@ -254,7 +254,8 @@ end context "when a ephemera folder doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -382,7 +383,8 @@ end context "when a ephemera folder doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/ephemera_project/ephemera_projects_controller_spec.rb b/spec/resources/ephemera_project/ephemera_projects_controller_spec.rb index fcbb9eaf2f..f4c3b1396f 100644 --- a/spec/resources/ephemera_project/ephemera_projects_controller_spec.rb +++ b/spec/resources/ephemera_project/ephemera_projects_controller_spec.rb @@ -148,7 +148,8 @@ end context "when a ephemera project doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -184,7 +185,8 @@ end context "when a ephemera project doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/ephemera_term/ephemera_terms_controller_spec.rb b/spec/resources/ephemera_term/ephemera_terms_controller_spec.rb index ef6d2864d6..69f86bf0c4 100644 --- a/spec/resources/ephemera_term/ephemera_terms_controller_spec.rb +++ b/spec/resources/ephemera_term/ephemera_terms_controller_spec.rb @@ -108,7 +108,8 @@ end context "when a ephemera term doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -132,7 +133,8 @@ end context "when a ephemera term doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/ephemera_vocabulary/ephemera_vocabularies_controller_spec.rb b/spec/resources/ephemera_vocabulary/ephemera_vocabularies_controller_spec.rb index a49bb5159a..3f3df6e0b4 100644 --- a/spec/resources/ephemera_vocabulary/ephemera_vocabularies_controller_spec.rb +++ b/spec/resources/ephemera_vocabulary/ephemera_vocabularies_controller_spec.rb @@ -125,7 +125,8 @@ end context "when a ephemera vocabulary doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -149,7 +150,8 @@ end context "when a ephemera vocabulary doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/file_set/file_sets_controller_spec.rb b/spec/resources/file_set/file_sets_controller_spec.rb index 44f74d62fe..64d9489da1 100644 --- a/spec/resources/file_set/file_sets_controller_spec.rb +++ b/spec/resources/file_set/file_sets_controller_spec.rb @@ -26,7 +26,8 @@ context "with an invalid FileSet ID" do it "displays an error" do - expect { patch :update, params: { id: "no-exist", file_set: { title: ["Second"] } } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "no-exist", file_set: { title: ["Second"] } } + expect(response).to redirect_to_not_found end end diff --git a/spec/resources/raster_resource/raster_resources_controller_spec.rb b/spec/resources/raster_resource/raster_resources_controller_spec.rb index a72208d7c3..6b30b90bd7 100644 --- a/spec/resources/raster_resource/raster_resources_controller_spec.rb +++ b/spec/resources/raster_resource/raster_resources_controller_spec.rb @@ -118,7 +118,8 @@ end context "when a raster resource doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -156,7 +157,8 @@ end context "when a raster resource doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/scanned_map/scanned_maps_controller_spec.rb b/spec/resources/scanned_map/scanned_maps_controller_spec.rb index 0f035e67f0..36a203174e 100644 --- a/spec/resources/scanned_map/scanned_maps_controller_spec.rb +++ b/spec/resources/scanned_map/scanned_maps_controller_spec.rb @@ -177,7 +177,8 @@ end context "when a map image doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -201,7 +202,8 @@ end context "when a map image doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -239,7 +241,8 @@ end context "when a map image doesn't exist" do it "raises an error" do - expect { get :structure, params: { id: "banana" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :structure, params: { id: "banana" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/scanned_resource/scanned_resources_controller_spec.rb b/spec/resources/scanned_resource/scanned_resources_controller_spec.rb index 73997ca427..c86f70891d 100644 --- a/spec/resources/scanned_resource/scanned_resources_controller_spec.rb +++ b/spec/resources/scanned_resource/scanned_resources_controller_spec.rb @@ -203,7 +203,8 @@ end context "when a scanned resource doesn't exist" do it "raises an error" do - expect { get :structure, params: { id: "banana" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :structure, params: { id: "banana" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/resources/vector_resource/vector_resources_controller_spec.rb b/spec/resources/vector_resource/vector_resources_controller_spec.rb index ac2f9d026c..307e20a618 100644 --- a/spec/resources/vector_resource/vector_resources_controller_spec.rb +++ b/spec/resources/vector_resource/vector_resources_controller_spec.rb @@ -118,7 +118,8 @@ end context "when a vector resource doesn't exist" do it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -155,7 +156,8 @@ end context "when a vector resource doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/shared_specs/controllers/base_resource_controller.rb b/spec/shared_specs/controllers/base_resource_controller.rb index 0b99f434a7..a9dc99b449 100644 --- a/spec/shared_specs/controllers/base_resource_controller.rb +++ b/spec/shared_specs/controllers/base_resource_controller.rb @@ -4,7 +4,6 @@ RSpec.shared_examples "a BaseResourceController" do |*flags| include ActionDispatch::Routing::PolymorphicRoutes include Rails.application.routes.url_helpers - with_queue_adapter :inline let(:user) { nil } let(:adapter) { Valkyrie::MetadataAdapter.find(:indexing_persister) } @@ -157,6 +156,11 @@ expect(response).to redirect_to root_path expect { query_service.find_by(id: resource.id) }.to raise_error ::Valkyrie::Persistence::ObjectNotFoundError end + it "returns a 404 when given a bad resource ID" do + delete :destroy, params: { id: SecureRandom.uuid } + + expect(response).to redirect_to_not_found + end end describe "edit" do @@ -168,8 +172,9 @@ it_behaves_like "an access controlled edit request" end context "when a resource doesn't exist" do - it "raises an error" do - expect { get :edit, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + it "redirects" do + get :edit, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do @@ -194,7 +199,8 @@ end context "when a resource doesn't exist" do it "raises an error" do - expect { patch :update, params: { id: "test" } }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError) + patch :update, params: { id: "test" } + expect(response).to redirect_to_not_found end end context "when it does exist" do diff --git a/spec/support/matchers/redirect_to_not_found.rb b/spec/support/matchers/redirect_to_not_found.rb new file mode 100644 index 0000000000..770b54288a --- /dev/null +++ b/spec/support/matchers/redirect_to_not_found.rb @@ -0,0 +1,6 @@ +RSpec::Matchers.define :redirect_to_not_found do + match do |actual| + expect(actual).to redirect_to '/' + expect(actual.flash["alert"]).to eq "The requested resource does not exist." + end +end