Merge branch 'dev' into tak/re-release-candidate-2.0.1

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
 

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

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

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}
 

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

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

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: {

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

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)

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

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

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
 

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)