From a6998de41703e05c7906f5650534b4cd51700cab Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 1 Jul 2025 12:00:34 +0100 Subject: [PATCH] 404 if project not found in Api::SchoolProjectsController Previously when a request was made to either of the actions in `Api::SchoolProjectsController` for a project that does not exist, the following exception was raised: NoMethodError: undefined method `school_project' for nil:NilClass Unhandled exceptions are rescued by the default `ActionDispatch::ShowExceptions` middleware [1]. In turn this uses the default exceptions app, `ActionDispatch::PublicExceptions` [2] to render a static HTML error page from `public/500.html` in this case. Since this is an API endpoint, ideally it should always respond with JSON in the response body, otherwise it makes writing client code a lot harder. Indeed my actual motivation for fixing this is, because we've been seeing JSON:ParseError exceptions raised in experience-cs [3]. I've fixed the problem by using `Project.find_by!` instead of `Project.find_by`. This means that if the project is not found, a `ActiveRecord::RecordNotFound` exception is raised. This in turn is rescued by the `rescue_from` block in `ApiController` [4] and the action responds with a 404 Not Found head response, i.e. with no body. This head response seems to be handled OK by the HTTParty code in experience-cs [5] and thus it fixes the problem I was trying to address. The equivalent client-side code in editor-standalone [6] was already throwing an AxisError exception due to the 500 response code (not being a 2XX code [7]) and it will continue to throw the same exception, albeit with a payload reflecting the response code being 404 instead of 500 and the body being empty instead of being the Rails 500 HTML error page. There is no specific error-handling code in editor-standalone catching this exception, so I assume it's just relying on the generic error handling for the React app. Thus I think the user-facing behaviour should be unchanged. [1]: https://api.rubyonrails.org/v7.1.3.4/classes/ActionDispatch/ShowExceptions.html [2]: https://api.rubyonrails.org/v7.1.3.4/classes/ActionDispatch/PublicExceptions.html [3]: https://raspberrypifoundation.sentry.io/issues/6708738500/ [4]: https://github.com/RaspberryPiFoundation/editor-api/blob/c37ab30714edeb08beaa1929970772545f38e93d/app/controllers/api_controller.rb#L9 [5]: https://github.com/RaspberryPiFoundation/experience-cs/blob/7b559bef916e0a32b3174f2391dcc587a8c4fe3b/lib/editor_api/client.rb [6]: https://github.com/RaspberryPiFoundation/editor-standalone/blob/f1286121460e79da7ffa7431487a2fb0872f2ae5/src/utils/apiCallHandler/projects.js#L71-L91 [7]: https://axios-http.com/docs/handling_errors --- app/controllers/api/school_projects_controller.rb | 4 ++-- spec/requests/school_projects/set_finished_spec.rb | 10 ++++++++++ spec/requests/school_projects/show_finished_spec.rb | 10 ++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/school_projects_controller.rb b/app/controllers/api/school_projects_controller.rb index edaf3121d..0dc92553d 100644 --- a/app/controllers/api/school_projects_controller.rb +++ b/app/controllers/api/school_projects_controller.rb @@ -6,13 +6,13 @@ class SchoolProjectsController < ApiController load_and_authorize_resource :project def show_finished - @school_project = Project.find_by(identifier: params[:id]).school_project + @school_project = Project.find_by!(identifier: params[:id]).school_project authorize! :show_finished, @school_project render :finished, formats: [:json], status: :ok end def set_finished - project = Project.find_by(identifier: params[:id]) + project = Project.find_by!(identifier: params[:id]) @school_project = project.school_project authorize! :set_finished, @school_project result = SchoolProject::SetFinished.call(school_project: @school_project, finished: params[:finished]) diff --git a/spec/requests/school_projects/set_finished_spec.rb b/spec/requests/school_projects/set_finished_spec.rb index f30f52fc7..169ca0a54 100644 --- a/spec/requests/school_projects/set_finished_spec.rb +++ b/spec/requests/school_projects/set_finished_spec.rb @@ -80,4 +80,14 @@ expect(teacher_project.school_project.finished).to be_falsey end end + + context 'when project does not exist' do + before do + put('/api/projects/does-not-exist/finished', headers:, params: { finished: false }) + end + + it 'returns not found response' do + expect(response).to have_http_status(:not_found) + end + end end diff --git a/spec/requests/school_projects/show_finished_spec.rb b/spec/requests/school_projects/show_finished_spec.rb index 9f187c486..966d0189d 100644 --- a/spec/requests/school_projects/show_finished_spec.rb +++ b/spec/requests/school_projects/show_finished_spec.rb @@ -52,5 +52,15 @@ expect(response).to have_http_status(:forbidden) end end + + context 'when project does not exist' do + before do + get('/api/projects/does-not-exist/finished', headers:) + end + + it 'returns not found response' do + expect(response).to have_http_status(:not_found) + end + end end end