Defend against dashboard errors, rework sequence

9 files changed, 102 insertions, 10 deletions

diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb
index 29aab7baa60..04f9782b158 100644
--- a/app/controllers/projects/environments_controller.rb
+++ b/app/controllers/projects/environments_controller.rb
@@ -158,7 +158,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   end
 
   def metrics_dashboard
-    render_403 && return unless Feature.enabled?(:environment_metrics_use_prometheus_endpoint, @project)
+    return render_403 unless Feature.enabled?(:environment_metrics_use_prometheus_endpoint, @project)
     result = Gitlab::MetricsDashboard::Service.new(@project, @current_user, environment: environment).get_dashboard
 
     respond_to do |format|
diff --git a/lib/gitlab/metrics_dashboard/processor.rb b/lib/gitlab/metrics_dashboard/processor.rb
index ef9d75947f0..8fa95f1ee2d 100644
--- a/lib/gitlab/metrics_dashboard/processor.rb
+++ b/lib/gitlab/metrics_dashboard/processor.rb
@@ -3,23 +3,21 @@
 module Gitlab
   module MetricsDashboard
     # Responsible for processesing a dashboard hash, inserting
-    # relevantDB records & sorting for proper rendering in
+    # relevant DB records & sorting for proper rendering in
     # the UI. These includes shared metric info, custom metrics
     # info, and alerts (only in EE).
     class Processor
+      SEQUENCE = [
+        Stages::CommonMetricsInserter,
+        Stages::ProjectMetricsInserter,
+        Stages::Sorter
+      ].freeze
+
       def initialize(project, environment)
         @project = project
         @environment = environment
       end
 
-      def sequence
-        [
-          Stages::CommonMetricsInserter,
-          Stages::ProjectMetricsInserter,
-          Stages::Sorter
-        ]
-      end
-
       # Returns a new dashboard hash with the results of
       # running transforms on the dashboard.
       def process(dashboard)
@@ -30,6 +28,12 @@ module Gitlab
 
         dashboard
       end
+
+      private
+
+      def sequence
+        SEQUENCE
+      end
     end
   end
 end
diff --git a/lib/gitlab/metrics_dashboard/service.rb b/lib/gitlab/metrics_dashboard/service.rb
index a65f01ca54e..84f174c7d1b 100644
--- a/lib/gitlab/metrics_dashboard/service.rb
+++ b/lib/gitlab/metrics_dashboard/service.rb
@@ -14,6 +14,8 @@ module Gitlab
         dashboard = process_dashboard(dashboard_string)
 
         success(dashboard: dashboard)
+      rescue Gitlab::MetricsDashboard::Stages::BaseStage::DashboardLayoutError => e
+        error(e.message, :unprocessable_entity)
       end
 
       private
@@ -27,6 +29,7 @@ module Gitlab
         "metrics_dashboard_#{SYSTEM_DASHBOARD_NAME}"
       end
 
+      # Returns a new dashboard Hash, supplemented with DB info
       def process_dashboard(dashboard)
         Processor.new(project, params[:environment]).process(dashboard)
       end
diff --git a/lib/gitlab/metrics_dashboard/stages/base_stage.rb b/lib/gitlab/metrics_dashboard/stages/base_stage.rb
index bdbf0c196cc..df49126a07b 100644
--- a/lib/gitlab/metrics_dashboard/stages/base_stage.rb
+++ b/lib/gitlab/metrics_dashboard/stages/base_stage.rb
@@ -4,6 +4,8 @@ module Gitlab
   module MetricsDashboard
     module Stages
       class BaseStage
+        DashboardLayoutError = Class.new(StandardError)
+
         DEFAULT_PANEL_TYPE = 'area-chart'
 
         attr_reader :project, :environment
@@ -23,9 +25,27 @@ module Gitlab
 
         protected
 
+        def missing_panel_groups!
+          raise DashboardLayoutError.new('Top-level key :panel_groups must be an array')
+        end
+
+        def missing_panels!
+          raise DashboardLayoutError.new('Each "panel_group" must define an array :panels')
+        end
+
+        def missing_metrics!
+          raise DashboardLayoutError.new('Each "panel" must define an array :metrics')
+        end
+
         def for_metrics(dashboard)
+          missing_panel_groups! unless dashboard[:panel_groups].is_a?(Array)
+
           dashboard[:panel_groups].each do |panel_group|
+            missing_panels! unless panel_group[:panels].is_a?(Array)
+
             panel_group[:panels].each do |panel|
+              missing_metrics! unless panel[:metrics].is_a?(Array)
+
               panel[:metrics].each do |metric|
                 yield metric
               end
diff --git a/lib/gitlab/metrics_dashboard/stages/project_metrics_inserter.rb b/lib/gitlab/metrics_dashboard/stages/project_metrics_inserter.rb
index 8edb21c89c1..ab33ee75cce 100644
--- a/lib/gitlab/metrics_dashboard/stages/project_metrics_inserter.rb
+++ b/lib/gitlab/metrics_dashboard/stages/project_metrics_inserter.rb
@@ -60,15 +60,21 @@ module Gitlab
         end
 
         def find_panel_group(panel_groups, metric)
+          return unless panel_groups
+
           panel_groups.find { |group| group[:group] == metric.group_title }
         end
 
         def find_panel(panels, metric)
+          return unless panels
+
           panel_identifiers = [DEFAULT_PANEL_TYPE, metric.title, metric.y_label]
           panels.find { |panel| panel.values_at(:type, :title, :y_label) == panel_identifiers }
         end
 
         def find_metric(metrics, metric)
+          return unless metrics
+
           metrics.find { |m| m[:id] == metric.identifier }
         end
 
diff --git a/lib/gitlab/metrics_dashboard/stages/sorter.rb b/lib/gitlab/metrics_dashboard/stages/sorter.rb
index a2429e65efa..ca310c9637a 100644
--- a/lib/gitlab/metrics_dashboard/stages/sorter.rb
+++ b/lib/gitlab/metrics_dashboard/stages/sorter.rb
@@ -5,6 +5,8 @@ module Gitlab
     module Stages
       class Sorter < BaseStage
         def transform!(dashboard)
+          missing_panel_groups! unless dashboard[:panel_groups].is_a? Array
+
           sort_groups!(dashboard)
           sort_panels!(dashboard)
         end
@@ -19,6 +21,8 @@ module Gitlab
         # Sorts the panels in the dashboard by the :weight key
         def sort_panels!(dashboard)
           dashboard[:panel_groups].each do |group|
+            missing_panels! unless group[:panels].is_a? Array
+
             group[:panels] = group[:panels].sort_by { |panel| -panel[:weight].to_i }
           end
         end
diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb
index 6c3aad55622..b43698a6ef7 100644
--- a/spec/controllers/projects/environments_controller_spec.rb
+++ b/spec/controllers/projects/environments_controller_spec.rb
@@ -482,6 +482,19 @@ describe Projects::EnvironmentsController do
         expect(json_response.keys).to contain_exactly('dashboard', 'status')
         expect(json_response['dashboard']).to be_an_instance_of(Hash)
       end
+
+      context 'when the dashboard could not be provided' do
+        before do
+          allow(YAML).to receive(:load_file).and_return({})
+        end
+
+        it 'returns an error response' do
+          get :metrics_dashboard, params: environment_params(format: :json)
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response.keys).to contain_exactly('message', 'status', 'http_status')
+        end
+      end
     end
   end
 
diff --git a/spec/lib/gitlab/metrics_dashboard/processor_spec.rb b/spec/lib/gitlab/metrics_dashboard/processor_spec.rb
index 1bd905989fe..9a4897944c6 100644
--- a/spec/lib/gitlab/metrics_dashboard/processor_spec.rb
+++ b/spec/lib/gitlab/metrics_dashboard/processor_spec.rb
@@ -47,6 +47,32 @@ describe Gitlab::MetricsDashboard::Processor do
         expect(actual_metrics_order).to eq expected_metrics_order
       end
     end
+
+    shared_examples_for 'errors with message' do |expected_message|
+      it 'raises a DashboardLayoutError' do
+        error_class = Gitlab::MetricsDashboard::Stages::BaseStage::DashboardLayoutError
+
+        expect { dashboard }.to raise_error(error_class, expected_message)
+      end
+    end
+
+    context 'when the dashboard is missing panel_groups' do
+      let(:dashboard_yml) { {} }
+
+      it_behaves_like 'errors with message', 'Top-level key :panel_groups must be an array'
+    end
+
+    context 'when the dashboard contains a panel_group which is missing panels' do
+      let(:dashboard_yml) { { panel_groups: [{}] } }
+
+      it_behaves_like 'errors with message', 'Each "panel_group" must define an array :panels'
+    end
+
+    context 'when the dashboard contains a panel which is missing metrics' do
+      let(:dashboard_yml) { { panel_groups: [{ panels: [{}] }] } }
+
+      it_behaves_like 'errors with message', 'Each "panel" must define an array :metrics'
+    end
   end
 
   private
diff --git a/spec/lib/gitlab/metrics_dashboard/service_spec.rb b/spec/lib/gitlab/metrics_dashboard/service_spec.rb
index 710c71fd6bd..38c7942f250 100644
--- a/spec/lib/gitlab/metrics_dashboard/service_spec.rb
+++ b/spec/lib/gitlab/metrics_dashboard/service_spec.rb
@@ -24,5 +24,21 @@ describe Gitlab::MetricsDashboard::Service, :use_clean_rails_memory_store_cachin
       described_class.new(project, environment).get_dashboard
       described_class.new(project, environment).get_dashboard
     end
+
+    context 'when the dashboard is configured incorrectly' do
+      let(:bad_dashboard) { {} }
+
+      before do
+        allow(described_class).to receive(:system_dashboard).and_return(bad_dashboard)
+      end
+
+      it 'returns an appropriate message and status code' do
+        result = described_class.new(project, environment).get_dashboard
+
+        expect(result.keys).to contain_exactly(:message, :http_status, :status)
+        expect(result[:status]).to eq(:error)
+        expect(result[:status]).to eq(:unprocessable_entity)
+      end
+    end
   end
 end