diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c84fe1f..fd267c1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changes to the UKHPI app by version and date +## Unreleased + +## 2.0.1 - 2025-01 + +- (Jon) Refactored UI code to use if/elsif/else in error page responses to + ensure a message is displayed at all times no matter what status is passed in + as per best practices +- (Jon) Updated the `ApplicationController` to handle each error status + appropriately as well as ensure that the `instrument_internal_error` method is + called when an internal error is raised +- (Jon) Updated the `instrument_internal_error` method to replace the + `maybe_report_to_sentry` method while reporting internal errors to the + Prometheus metrics only when necessary +- (Jon) Removed the custom error handling by deleting the `ExceptionsController` + and the `exceptions` initialiser +- (Jon) Fix for casting long strings to `Date`, `Time` or `DateTime` in Ruby + 3.1.0 +- (Jon) Improves error metrics reporting to ensure that logging always happens + with the appropriate severity depending on the exception status while reducing + the types of errors that can trigger a an error metric and therefore a + notification in slack + [GH-149](https://github.com/epimorphics/hmlr-linked-data/issues/149) + ## 2.0.0 - 2024-11 - (Bogdan) Updated all gems by regenerating `Gemfile.lock` @@ -10,7 +33,8 @@ ## 1.8.0 - 2024-10 -- (Dan) Updates ruby version to 2.7.8 and alpine version to 3.16 [GH-455](https://github.com/epimorphics/ukhpi/issues/455) +- (Dan) Updates ruby version to 2.7.8 and alpine version to 3.16 + [GH-455](https://github.com/epimorphics/ukhpi/issues/455) ## 1.7.6 - 2024-10 diff --git a/Gemfile b/Gemfile index 267fdf37..f678e3ba 100644 --- a/Gemfile +++ b/Gemfile @@ -51,7 +51,8 @@ end group :development do # Access an IRB console on exception pages or by using <%= console %> in views gem 'web-console' - + # Rails Panel is a Chrome extension for Rails development that will end your tailing of development.log. + gem 'meta_request' # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' end diff --git a/Gemfile.lock b/Gemfile.lock index 2ea53fd2..554d0e3b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -210,6 +210,9 @@ GEM net-smtp marcel (1.0.4) matrix (0.4.2) + meta_request (0.8.5) + rack-contrib (>= 1.1, < 3) + railties (>= 3.0.0, < 9) method_source (1.1.0) mini_mime (1.1.5) minitest (5.25.1) @@ -266,6 +269,8 @@ GEM puma (>= 6.0) racc (1.8.1) rack (3.1.8) + rack-contrib (2.5.0) + rack (< 4) rack-proxy (0.7.7) rack rack-session (2.0.0) @@ -475,6 +480,7 @@ DEPENDENCIES json_expressions json_rails_logger! m + meta_request minitest-rails minitest-reporters mocha diff --git a/Makefile b/Makefile index 3934ec9a..f4cbec73 100644 --- a/Makefile +++ b/Makefile @@ -89,7 +89,7 @@ run: start @if docker network inspect dnet > /dev/null 2>&1; then echo "Using docker network dnet"; else echo "Create docker network dnet"; docker network create dnet; sleep 2; fi @docker run -p ${PORT}:3000 -e API_SERVICE_URL=${API_SERVICE_URL} --network dnet --rm --name ${SHORTNAME} ${NAME}:${TAG} -server: assets start +server: @export SECRET_KEY_BASE=$(./bin/rails secret) @API_SERVICE_URL=${API_SERVICE_URL} ./bin/rails server -p ${PORT} diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b00bb450..2a9e6072 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true # :nodoc: -class ApplicationController < ActionController::Base +class ApplicationController < ActionController::Base # rubocop:disable Metrics/ClassLength + include Rails.application.routes.url_helpers + include ActionView::Helpers::TranslationHelper # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. protect_from_forgery with: :exception @@ -37,9 +39,79 @@ def log_request_result detailed_request_log(duration) end + # Handle specific types of exceptions and render the appropriate error page + # or attempt to render a generic error page if no specific error page exists + unless Rails.application.config.consider_all_requests_local + rescue_from StandardError do |e| + # Trigger the appropriate error handling method based on the exception + case e.class + when ActionController::RoutingError, ActionView::MissingTemplate + :render404 + when ActionController::InvalidCrossOriginRequest + :render403 + when ActionController::BadRequest, ActionController::ParameterMissing + :render400 + else + :handle_internal_error + end + end + end + + # Render the appropriate error page based on the exception + def handle_internal_error(exception) + if exception.instance_of? ArgumentError + render_error(400) + else + Rails.logger.warn "No explicit error page for exception #{exception} - #{exception.class}" + # Instrument ActiveSupport::Notifications for internal server errors only: + sentry_code = instrument_internal_error(exception) + render_error(500, sentry_code) + end + end + + def render_400(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(400) + end + + def render_403(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(403) + end + + def render_404(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(404) + end + + def render_500(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(500) + end + + def render_error(status, sentry_code = nil) # rubocop:disable Metrics/AbcSize + reset_response + + status = Rack::Utils::SYMBOL_TO_STATUS_CODE[status] if status.is_a?(Symbol) + @view_state ||= LandingState.new(UserLanguageSelection.new(params)) + + respond_to do |format| + format.html { render_html_error_page(status, sentry_code) } + # Anything else returns the status as human readable plain string + format.all { render plain: Rack::Utils::HTTP_STATUS_CODES[status].to_s, status: status } + end + end + + def render_html_error_page(status, sentry_code) # rubocop:disable Metrics/MethodLength + render 'exceptions/error_page', + locals: { status: status, sentry_code: sentry_code }, + layout: true, + status: status + end + + def reset_response + self.response_body = nil + end + private - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def detailed_request_log(duration) env = request.env @@ -63,7 +135,7 @@ def detailed_request_log(duration) log_response(response.status, log_fields) end - # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength # Log the error with the appropriate log level based on the status code def log_response(status, error_log) @@ -79,10 +151,29 @@ def log_response(status, error_log) # Notify subscriber(s) of an internal error event with the payload of the # exception once done - # @param [Exception] exp the exception that caused the error + # @param [exc] exp the exception that caused the error # @return [ActiveSupport::Notifications::Event] provides an object-oriented # interface to the event - def instrument_internal_error(exception) - ActiveSupport::Notifications.instrument('internal_error.application', exception: exception) + # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength + def instrument_internal_error(exc, status = nil) + err = { + message: exc&.message || exc, + status: Rack::Utils::SYMBOL_TO_STATUS_CODE[exc] + } + err[:status] = status if status + err[:type] = exc.class&.name if exc&.class + err[:cause] = exc&.cause if exc&.cause + err[:backtrace] = exc&.backtrace if exc&.backtrace && Rails.env.development? + # Log the exception to the Rails logger with the appropriate severity + Rails.logger.send(err[:status] < 500 ? :warn : :error, JSON.generate(err)) + # Return unless the status code is 500 or greater to ensure subscribers are NOT notified + return nil unless err[:status] >= 500 + + sevent = Sentry.capture_exception(exc) unless Rails.env.development? + # Instrument the internal error event to notify subscribers of the error + ActiveSupport::Notifications.instrument('internal_error.application', exception: err) + # Return the event id for the internal error event + sevent&.event_id end + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength end diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index 8b7f2a3e..f3ff76a2 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -3,14 +3,14 @@ # Controller for the main user experience of browsing the UKHPI statistics. # Usually the primary interaction will be via JavaScript and XHR, but we also # support non-JS access by setting browse preferences in the `edit` action. -class BrowseController < ApplicationController +class BrowseController < ApplicationController # rubocop:disable Metrics/ClassLength layout 'webpack_application' def show user_selections = UserSelections.new(params) if !user_selections.valid? - render_request_error(user_selections, 400) + render_request_error(user_selections, :bad_request) elsif explain_non_json?(user_selections) redirect_to_html_view(user_selections) else @@ -128,19 +128,20 @@ def view_result(view_state) }.merge(new_params)) end - def render_request_error(user_selections, status) + def render_request_error(user_selections, status_code) # rubocop:disable Metrics/MethodLength + # Convert status code to integer if it is a symbol + status_code = Rack::Utils::SYMBOL_TO_STATUS_CODE[status_code] if status_code.is_a?(Symbol) respond_to do |format| - @view_state = { user_selections: user_selections } - request_status = status == 400 ? :bad_request : :internal_server_error + @view_state ||= { user_selections: user_selections } format.html do render 'exceptions/error_page', - locals: { status: status, sentry_code: nil }, + locals: { status: status_code, sentry_code: nil }, layout: true, - status: request_status + status: status_code end format.json do - render(json: { status: 'request error' }, status: request_status) + render(json: { status: 'request error' }, status: status_code) end end end diff --git a/app/controllers/compare_controller.rb b/app/controllers/compare_controller.rb index 4998baf9..a66da0ca 100644 --- a/app/controllers/compare_controller.rb +++ b/app/controllers/compare_controller.rb @@ -31,7 +31,7 @@ def render_print render 'compare/print', layout: 'print' end - def perform_query(user_compare_selections) + def perform_query(user_compare_selections) # rubocop:disable Metrics/MethodLength query_results = {} base_selection = UserSelections.new( __safe_params: { diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb deleted file mode 100644 index f11d6c1d..00000000 --- a/app/controllers/exceptions_controller.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -# Controller to handle dynamically displaying error messages -class ExceptionsController < ApplicationController - layout 'application' - - def render_error - env = request.env - exception = env['action_dispatch.exception'] - status_code = ActionDispatch::ExceptionWrapper.new(env, exception).status_code - - sentry_code = maybe_report_to_sentry(exception, status_code) - # add the exception to the prometheus metrics - instrument_internal_error(exception) - - # add the exception to the prometheus metrics but only on errors that are 404 - instrument_internal_error(exception) unless status_code == 404 - - render :error_page, - locals: { status: status_code, sentry_code: sentry_code }, - layout: true, - status: status_code - end - - private - - def maybe_report_to_sentry(exception, status_code) - return nil if Rails.env.development? # Why are we reporting to Sentry in dev? - return nil unless status_code >= 500 - - sevent = Sentry.capture_exception(exception) - sevent&.event_id - end -end diff --git a/app/lib/version.rb b/app/lib/version.rb index da348ade..4d205e68 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -3,7 +3,7 @@ module Version MAJOR = 2 MINOR = 0 - PATCH = 0 + PATCH = 1 SUFFIX = nil VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" end diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index 7130e764..4bbdc3cf 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -54,15 +54,23 @@ def alternative_key(key) end # Recognise a date. Accepts ISO-8601 strings or Date objects. - # Dates that match YYYY-MM will be transformed to YYYY-MM-01. # @param date Either an ISO0-8601 date string, or a date object def parse_date(date) if date.is_a?(Date) date else - date_str = date.to_s.match?(/\d\d\d\d-(1[012]|0[1-9])/) ? "#{date}-01" : date.to_s + # We need to truncate the date to at most the final 10 characters expected + # to avoid potential errors from maliciously long strings. + potential_date = date.to_s.first(10) + # strings that match YYYY-MM will be transformed to YYYY-MM-01 (i.e. 10 chars) + date_str = potential_date.match?(/\d\d\d\d-(1[012]|0[1-9])/) ? "#{potential_date}-01" : potential_date # rubocop:disable Layout/LineLength + # We can now parse the date string and fail if it is not a valid date. Date.parse(date_str) end + rescue ArgumentError => e + # We use truncate here to show the original date value in the logs with an + # ellipsis but with a maximum length of 128 characters to clarify the error. + Rails.logger.error("Failed to parse date '#{date.to_s.truncate(128)}': #{e.message || e}") end def array_valued?(param) diff --git a/app/models/data_view.rb b/app/models/data_view.rb index f2844e46..52e24037 100644 --- a/app/models/data_view.rb +++ b/app/models/data_view.rb @@ -7,7 +7,7 @@ # a view of the `averagePrice` indicator, together with the relevant dates, # location and other options, and access to the underlying data, to enable # the renderer to draw the display -class DataView +class DataView # rubocop:disable Metrics/ClassLength include Rails.application.routes.url_helpers attr_reader :user_selections, :query_result, :indicator, :theme diff --git a/app/models/ukhpi_indicator.rb b/app/models/ukhpi_indicator.rb index 72560cbf..02864d86 100644 --- a/app/models/ukhpi_indicator.rb +++ b/app/models/ukhpi_indicator.rb @@ -17,7 +17,7 @@ def initialize(slug, root_name, label_key) # @return True if this indicator denotes sales volume def volume? - slug == 'vol' + slug && slug == 'vol' end # @return The label for this indicator diff --git a/app/views/browse/_data_view.html.haml b/app/views/browse/_data_view.html.haml index 8b8ffda4..3450a198 100644 --- a/app/views/browse/_data_view.html.haml +++ b/app/views/browse/_data_view.html.haml @@ -26,9 +26,10 @@ %br %span.o-data-view__options-statistics - data_view.each_statistic do |stat| + - x = x ? x + 1 : 0 %label - checked = data_view.statistic_selected?(stat) - %input{ type: 'checkbox', value: stat.slug, checked: checked } + %input{ id: "checkbox-#{node_id}-#{stat.slug}-#{x}", name: "checkbox-#{node_id}-#{stat.slug}", type: 'checkbox', value: stat.slug, checked: checked } %a{ href: data_view.add_remove_statistic(!checked, stat) } = stat.label diff --git a/app/views/common/_header.html.haml b/app/views/common/_header.html.haml index a5256a23..0a14e1a9 100644 --- a/app/views/common/_header.html.haml +++ b/app/views/common/_header.html.haml @@ -32,12 +32,13 @@ .o-secondary-banner - if Rails.application.config.welsh_language_enabled %nav.o-secondary-banner__lang-select{ 'aria-label' => 'language selector' } - - if @view_state.user_selections.english? + - user_selections = @view_state.respond_to?(:[]) ? @view_state[:user_selections] : @view_state.user_selections + - if user_selections.english? English - else - = link_to('English', @view_state.user_selections.alternative_language_params.params) + = link_to('English', user_selections.alternative_language_params.params) | - - if @view_state.user_selections.welsh? + - if user_selections.welsh? Cymraeg - else - = link_to('Cymraeg', @view_state.user_selections.alternative_language_params.params) + = link_to('Cymraeg', user_selections.alternative_language_params.params) diff --git a/app/views/exceptions/error_page.html.haml b/app/views/exceptions/error_page.html.haml index e27f6e64..cfca09b2 100644 --- a/app/views/exceptions/error_page.html.haml +++ b/app/views/exceptions/error_page.html.haml @@ -1,3 +1,6 @@ +- puts "Rendering error page with status: #{status.inspect}" if Rails.env.development? +- puts "View state: #{@view_state.inspect}" if Rails.env.development? + %article.u-clear - if status == 400 %h1.o-heading--1 Request not understood @@ -7,12 +10,12 @@ message as a result of using the HPI or PPD applications, please let us know so that we can correct the problem. - - if @view_state.user_selections&.errors + - if @view_state && @view_state[:user_selections].errors.any? %p The following issues were identified in your request: %ul.list.list-bullet - - @view_state.user_selections.errors.each do |err_msg| + - @view_state[:user_selections].errors.each do |err_msg| %li = err_msg @@ -42,16 +45,14 @@ = sentry_code %h2.o-heading--2 Who to contact - %p - If you are unable to access the data, please - %a{ href: 'http://site.landregistry.gov.uk/contact-us/form' } - fill in our contact form. - - %p - For general transaction data enquiries, email - %a{ href: 'mailto:DRO@landregistry.gov.uk' } - DRO@landregistry.gov.uk - - %p - For general price paid data enquiries, contact - = mail_to(Rails.application.config.contact_email_address) + %ul.list.list-bullet + %li + If you are unable to access the data, please + %a{ href: '//site.landregistry.gov.uk/contact-us/form' } + fill in our contact form. + %li + For general transaction data enquiries, email + = mail_to('DRO@landregistry.gov.uk') + %li + For general price paid data enquiries, contact + = mail_to(Rails.application.config.contact_email_address) diff --git a/config/initializers/exceptions.rb b/config/initializers/exceptions.rb deleted file mode 100644 index b0de5388..00000000 --- a/config/initializers/exceptions.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -# Custom error handling via a Rack middleware action -Rails.application.config.exceptions_app = - lambda do |env| - ExceptionsController.action(:render_error).call(env) - end diff --git a/config/routes.rb b/config/routes.rb index adf861e7..d679ca1f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -15,4 +15,6 @@ get 'doc/ukhpi', to: 'doc#ukhpi', as: 'ukhpi_doc' get 'doc/ukhpi-dsd', to: 'doc#ukhpi_dsd' get 'doc/ukhpi-user-guide', to: 'doc#ukhpi_user_guide' + + get '*unmatched_route', to: 'application#render_404' end diff --git a/test/models/ukhpi_indicator_test.rb b/test/models/ukhpi_indicator_test.rb index 5f26a7da..27bb2cec 100644 --- a/test/models/ukhpi_indicator_test.rb +++ b/test/models/ukhpi_indicator_test.rb @@ -4,7 +4,7 @@ # Unit tests on the UkhpiIndicator class class UkhpiIndicatorTest < ActiveSupport::TestCase - describe 'UkhpiIndicator' do + describe 'UkhpiIndicator' do # rubocop:disable Metrics/BlockLength describe '#initialize' do it 'should provide access to the initialised values' do ind = UkhpiIndicator.new('foo', 'foo_root', 'average_price') diff --git a/test/models/ukhpi_statistic_test.rb b/test/models/ukhpi_statistic_test.rb index 1d8c3201..6b7593b7 100644 --- a/test/models/ukhpi_statistic_test.rb +++ b/test/models/ukhpi_statistic_test.rb @@ -4,7 +4,7 @@ # Unit tests on the UkhpiStatistic class class UkhpiStatisticTest < ActiveSupport::TestCase - describe 'UkhpiStatistic' do + describe 'UkhpiStatistic' do # rubocop:disable Metrics/BlockLength describe '#initialize' do it 'should provide accessors to initialization state' do stat = UkhpiStatistic.new('foo', 'foo_r', 'all_property_types', true) diff --git a/test/models/ukhpi_theme_test.rb b/test/models/ukhpi_theme_test.rb index 7ee869a3..b4d791ba 100644 --- a/test/models/ukhpi_theme_test.rb +++ b/test/models/ukhpi_theme_test.rb @@ -4,7 +4,7 @@ # Unit tests on the UkhpiTheme class class UkhpiThemeTest < ActiveSupport::TestCase - describe 'UkhpiTheme' do + describe 'UkhpiTheme' do # rubocop:disable Metrics/BlockLength describe '#initialize' do it 'should provide accessors to initialization state' do stat = stub