Merge branch 'mwaw/210289-metrics-dashboard-file-validation-mvc-1' into 'master'

Metrics Dashboard YAML validation - add auxiliary blob view for dashboard files.

See merge request gitlab-org/gitlab!33202

app/models/blob.rb

@@ -50,6 +50,7 @@ class Blob < SimpleDelegator
     BlobViewer::License,
     BlobViewer::Contributing,
     BlobViewer::Changelog,
+    BlobViewer::MetricsDashboardYml,
 
     BlobViewer::CargoToml,
     BlobViewer::Cartfile,

app/models/blob_viewer/metrics_dashboard_yml.rb

+# frozen_string_literal: true
+
+module BlobViewer
+  class MetricsDashboardYml < Base
+    include ServerSide
+    include Gitlab::Utils::StrongMemoize
+    include Auxiliary
+
+    self.partial_name = 'metrics_dashboard_yml'
+    self.loading_partial_name = 'metrics_dashboard_yml_loading'
+    self.file_types = %i(metrics_dashboard)
+    self.binary = false
+
+    def valid?
+      errors.blank?
+    end
+
+    def errors
+      strong_memoize(:errors) do
+        prepare!
+        parse_blob_data
+      end
+    end
+
+    private
+
+    def parse_blob_data
+      ::PerformanceMonitoring::PrometheusDashboard.from_json(YAML.safe_load(blob.data))
+      nil
+    rescue Psych::SyntaxError => error
+      wrap_yml_syntax_error(error)
+    rescue ActiveModel::ValidationError => invalid
+      invalid.model.errors
+    end
+
+    def wrap_yml_syntax_error(error)
+      ::PerformanceMonitoring::PrometheusDashboard.new.errors.tap do |errors|
+        errors.add(:'YAML syntax', error.message)
+      end
+    end
+  end
+end

app/models/performance_monitoring/prometheus_dashboard.rb

@@ -13,7 +13,7 @@ module PerformanceMonitoring
       def from_json(json_content)
         dashboard = new(
           dashboard: json_content['dashboard'],
-          panel_groups: json_content['panel_groups'].map { |group| PrometheusPanelGroup.from_json(group) }
+          panel_groups: json_content['panel_groups']&.map { |group| PrometheusPanelGroup.from_json(group) }
         )
 
         dashboard.tap(&:validate!)

app/models/performance_monitoring/prometheus_panel.rb

@@ -15,7 +15,7 @@ module PerformanceMonitoring
         title: json_content['title'],
         y_label: json_content['y_label'],
         weight: json_content['weight'],
-        metrics: json_content['metrics'].map { |metric| PrometheusMetric.from_json(metric) }
+        metrics: json_content['metrics']&.map { |metric| PrometheusMetric.from_json(metric) }
       )
 
       panel.tap(&:validate!)

app/models/performance_monitoring/prometheus_panel_group.rb

@@ -13,7 +13,7 @@ module PerformanceMonitoring
       panel_group = new(
         group: json_content['group'],
         priority: json_content['priority'],
-        panels: json_content['panels'].map { |panel| PrometheusPanel.from_json(panel) }
+        panels: json_content['panels']&.map { |panel| PrometheusPanel.from_json(panel) }
       )
 
       panel_group.tap(&:validate!)

app/views/projects/blob/viewers/_metrics_dashboard_yml.html.haml

+- if viewer.valid?
+  = icon('check fw')
+  = _('Metrics Dashboard YAML definition is valid.')
+- else
+  = icon('warning fw')
+  = _('Metrics Dashboard YAML definition is invalid:')
+  %ul
+    - viewer.errors.messages.each do |error|
+      %li= error.join(': ')
+
+= link_to _('Learn more'), help_page_path('user/project/integrations/prometheus.md', anchor: 'defining-custom-dashboards-per-project')

app/views/projects/blob/viewers/_metrics_dashboard_yml_loading.html.haml

+= icon('spinner spin fw')
+= _('Metrics Dashboard YAML definition') + '…'
+
+= link_to _('Learn more'), help_page_path('user/project/integrations/prometheus.md')

changelogs/unreleased/mwaw-210289-metrics-dashboard-file-validation-mvc-1.yml

+---
+title: Added validation for YAML files with metrics dashboard definitions.
+merge_request: 33202
+author:
+type: added

doc/user/project/integrations/prometheus.md

@@ -326,6 +326,33 @@ new branch.
 If you select your **default** branch, the new dashboard becomes immediately available.
 If you select another branch, this branch should be merged to your **default** branch first.
 
+#### Dashboard YAML syntax validation
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33202) in GitLab 13.1.
+
+To confirm your dashboard definition contains valid YAML syntax:
+
+1. Navigate to **{doc-text}** **Repository > Files**.
+1. Navigate to your dashboard file in your repository.
+1. Review the information pane about the file, displayed above the file contents.
+
+Files with valid syntax display **Metrics Dashboard YAML definition is valid**,
+and files with invalid syntax display **Metrics Dashboard YAML definition is invalid**.
+
+![Metrics Dashboard_YAML_syntax_validation](img/prometheus_dashboard_yaml_validation_v13_1.png)
+
+When **Metrics Dashboard YAML definition is invalid** at least one of the following messages is displayed:
+
+1. `dashboard: can't be blank` [learn more](#dashboard-top-level-properties)
+1. `panel_groups: can't be blank` [learn more](#dashboard-top-level-properties)
+1. `group: can't be blank` [learn more](#panel-group-panel_groups-properties)
+1. `panels: can't be blank` [learn more](#panel-group-panel_groups-properties)
+1. `metrics: can't be blank` [learn more](#panel-panels-properties)
+1. `title: can't be blank` [learn more](#panel-panels-properties)
+1. `query: can't be blank` [learn more](#metrics-metrics-properties)
+1. `query_range: can't be blank` [learn more](#metrics-metrics-properties)
+1. `unit: can't be blank` [learn more](#metrics-metrics-properties)
+
 #### Dashboard YAML properties
 
 Dashboards have several components:

locale/gitlab.pot

@@ -13756,6 +13756,15 @@ msgstr ""
 msgid "Metrics Dashboard"
 msgstr ""
 
+msgid "Metrics Dashboard YAML definition"
+msgstr ""
+
+msgid "Metrics Dashboard YAML definition is invalid:"
+msgstr ""
+
+msgid "Metrics Dashboard YAML definition is valid."
+msgstr ""
+
 msgid "Metrics and profiling"
 msgstr ""
 

spec/features/projects/blobs/blob_show_spec.rb

@@ -555,6 +555,53 @@ describe 'File blob', :js do
     end
   end
 
+  describe '.gitlab/dashboards/custom-dashboard.yml' do
+    before do
+      project.add_maintainer(project.creator)
+
+      Files::CreateService.new(
+        project,
+        project.creator,
+        start_branch: 'master',
+        branch_name: 'master',
+        commit_message: "Add .gitlab/dashboards/custom-dashboard.yml",
+        file_path: '.gitlab/dashboards/custom-dashboard.yml',
+        file_content: file_content
+      ).execute
+
+      visit_blob('.gitlab/dashboards/custom-dashboard.yml')
+    end
+
+    context 'valid dashboard file' do
+      let(:file_content) { File.read(Rails.root.join('config/prometheus/common_metrics.yml')) }
+
+      it 'displays an auxiliary viewer' do
+        aggregate_failures do
+          # shows that dashboard yaml is valid
+          expect(page).to have_content('Metrics Dashboard YAML definition is valid.')
+
+          # shows a learn more link
+          expect(page).to have_link('Learn more')
+        end
+      end
+    end
+
+    context 'invalid dashboard file' do
+      let(:file_content) { "dashboard: 'invalid'" }
+
+      it 'displays an auxiliary viewer' do
+        aggregate_failures do
+          # shows that dashboard yaml is invalid
+          expect(page).to have_content('Metrics Dashboard YAML definition is invalid:')
+          expect(page).to have_content("panel_groups: can't be blank")
+
+          # shows a learn more link
+          expect(page).to have_link('Learn more')
+        end
+      end
+    end
+  end
+
   context 'LICENSE' do
     before do
       visit_blob('LICENSE')

spec/models/blob_viewer/metrics_dashboard_yml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe BlobViewer::MetricsDashboardYml do
+  include FakeBlobHelpers
+  include RepoHelpers
+
+  let_it_be(:project) { create(:project, :repository) }
+  let(:blob) { fake_blob(path: '.gitlab/dashboards/custom-dashboard.yml', data: data) }
+  let(:sha) { sample_commit.id }
+
+  subject(:viewer) { described_class.new(blob) }
+
+  context 'when the definition is valid' do
+    let(:data) { File.read(Rails.root.join('config/prometheus/common_metrics.yml')) }
+
+    describe '#valid?' do
+      it 'calls prepare! on the viewer' do
+        allow(PerformanceMonitoring::PrometheusDashboard).to receive(:from_json)
+
+        expect(viewer).to receive(:prepare!)
+
+        viewer.valid?
+      end
+
+      it 'returns true' do
+        expect(PerformanceMonitoring::PrometheusDashboard)
+          .to receive(:from_json).with(YAML.safe_load(data))
+        expect(viewer.valid?).to be_truthy
+      end
+    end
+
+    describe '#errors' do
+      it 'returns nil' do
+        allow(PerformanceMonitoring::PrometheusDashboard).to receive(:from_json)
+
+        expect(viewer.errors).to be nil
+      end
+    end
+  end
+
+  context 'when definition is invalid' do
+    let(:error) { ActiveModel::ValidationError.new(PerformanceMonitoring::PrometheusDashboard.new.tap(&:validate)) }
+    let(:data) do
+      <<~YAML
+        dashboard:
+      YAML
+    end
+
+    describe '#valid?' do
+      it 'returns false' do
+        expect(PerformanceMonitoring::PrometheusDashboard)
+          .to receive(:from_json).and_raise(error)
+
+        expect(viewer.valid?).to be_falsey
+      end
+    end
+
+    describe '#errors' do
+      it 'returns validation errors' do
+        allow(PerformanceMonitoring::PrometheusDashboard)
+          .to receive(:from_json).and_raise(error)
+
+        expect(viewer.errors).to be error.model.errors
+      end
+    end
+  end
+
+  context 'when YAML syntax is invalid' do
+    let(:data) do
+      <<~YAML
+        dashboard: 'empty metrics'
+         panel_groups:
+        - group: 'Group Title'
+      YAML
+    end
+
+    describe '#valid?' do
+      it 'returns false' do
+        expect(PerformanceMonitoring::PrometheusDashboard).not_to receive(:from_json)
+        expect(viewer.valid?).to be_falsey
+      end
+    end
+
+    describe '#errors' do
+      it 'returns validation errors' do
+        yaml_wrapped_errors = { 'YAML syntax': ["(<unknown>): did not find expected key while parsing a block mapping at line 1 column 1"] }
+
+        expect(viewer.errors).to be_kind_of ActiveModel::Errors
+        expect(viewer.errors.messages).to eql(yaml_wrapped_errors)
+      end
+    end
+  end
+end

spec/models/performance_monitoring/prometheus_dashboard_spec.rb

@@ -38,24 +38,123 @@ describe PerformanceMonitoring::PrometheusDashboard do
     end
 
     describe 'validations' do
-      context 'when dashboard is missing' do
-        before do
-          json_content['dashboard'] = nil
+      shared_examples 'validation failed' do |errors_messages|
+        it 'raises error with corresponding messages', :aggregate_failures do
+          expect { subject }.to raise_error do |error|
+            expect(error).to be_kind_of(ActiveModel::ValidationError)
+            expect(error.model.errors.messages).to eql(errors_messages)
+          end
         end
+      end
 
-        subject { described_class.from_json(json_content) }
+      context 'dashboard definition is missing panels_groups and dashboard keys' do
+        let(:json_content) do
+          {
+            "dashboard" => nil
+          }
+        end
 
-        it { expect { subject }.to raise_error(ActiveModel::ValidationError) }
+        it_behaves_like 'validation failed', panel_groups: ["can't be blank"], dashboard: ["can't be blank"]
       end
 
-      context 'when panel groups are missing' do
-        before do
-          json_content['panel_groups'] = []
+      context 'group definition is missing panels and group keys' do
+        let(:json_content) do
+          {
+            "dashboard" => "Dashboard Title",
+            "templating" => {
+              "variables" => {
+                "variable1" => %w(value1 value2 value3)
+              }
+            },
+            "panel_groups" => [{ "group" => nil }]
+          }
         end
 
-        subject { described_class.from_json(json_content) }
+        it_behaves_like 'validation failed', panels: ["can't be blank"], group: ["can't be blank"]
+      end
+
+      context 'panel definition is missing metrics and title keys' do
+        let(:json_content) do
+          {
+            "dashboard" => "Dashboard Title",
+            "templating" => {
+              "variables" => {
+                "variable1" => %w(value1 value2 value3)
+              }
+            },
+            "panel_groups" => [{
+              "group" => "Group Title",
+              "panels" => [{
+                "type" => "area-chart",
+                "y_label" => "Y-Axis"
+              }]
+            }]
+          }
+        end
+
+        it_behaves_like 'validation failed', metrics: ["can't be blank"], title: ["can't be blank"]
+      end
+
+      context 'metrics definition is missing unit, query and query_range keys' do
+        let(:json_content) do
+          {
+            "dashboard" => "Dashboard Title",
+            "templating" => {
+              "variables" => {
+                "variable1" => %w(value1 value2 value3)
+              }
+            },
+            "panel_groups" => [{
+              "group" => "Group Title",
+              "panels" => [{
+                "type" => "area-chart",
+                "title" => "Chart Title",
+                "y_label" => "Y-Axis",
+                "metrics" => [{
+                  "id" => "metric_of_ages",
+                  "label" => "Metric of Ages",
+                  "query_range" => nil
+                }]
+              }]
+            }]
+          }
+        end
+
+        it_behaves_like 'validation failed', unit: ["can't be blank"], query_range: ["can't be blank"], query: ["can't be blank"]
+      end
+
+      # for each parent entry validation first is done to its children,
+      # whole execution is stopped on first encountered error
+      # which is the one that is reported
+      context 'multiple offences on different levels' do
+        let(:json_content) do
+          {
+            "dashboard" => nil,
+            "panel_groups" => [{
+              "group" => nil,
+              "panels" => [{
+                "type" => "area-chart",
+                "title" => nil,
+                "y_label" => "Y-Axis",
+                "metrics" => [{
+                  "id" => "metric_of_ages",
+                  "label" => "Metric of Ages",
+                  "query_range" => 'query'
+                }, {
+                  "id" => "metric_of_ages",
+                  "unit" => "count",
+                  "label" => "Metric of Ages",
+                  "query_range" => nil
+                }]
+              }]
+            }, {
+              "group" => 'group',
+              "panels" => nil
+            }]
+          }
+        end
 
-        it { expect { subject }.to raise_error(ActiveModel::ValidationError) }
+        it_behaves_like 'validation failed', unit: ["can't be blank"]
       end
     end
   end

spec/models/performance_monitoring/prometheus_panel_group_spec.rb

@@ -32,7 +32,7 @@ describe PerformanceMonitoring::PrometheusPanelGroup do
     describe 'validations' do
       context 'when group is missing' do
         before do
-          json_content['group'] = nil
+          json_content.delete('group')
         end
 
         subject { described_class.from_json(json_content) }

spec/models/performance_monitoring/prometheus_panel_spec.rb

@@ -54,7 +54,7 @@ describe PerformanceMonitoring::PrometheusPanel do
 
       context 'when metrics are missing' do
         before do
-          json_content['metrics'] = []
+          json_content.delete('metrics')
         end
 
         subject { described_class.from_json(json_content) }