Merge branch 'revert-ac5e147d' into 'master'

Revert !38925 Improve errors on metrics dashboard config viewer

See merge request gitlab-org/gitlab!39869

app/graphql/resolvers/metrics/dashboard_resolver.rb

@@ -14,8 +14,7 @@ module Resolvers
       def resolve(**args)
         return unless environment
 
-        ::PerformanceMonitoring::PrometheusDashboard
-          .find_for(project: environment.project, user: context[:current_user], path: args[:path], options: { environment: environment })
+        ::PerformanceMonitoring::PrometheusDashboard.find_for(project: environment.project, user: context[:current_user], path: args[:path], options: { environment: environment })
       end
     end
   end

app/graphql/types/metrics/dashboard_type.rb

@@ -16,13 +16,6 @@ module Types
       field :annotations, Types::Metrics::Dashboards::AnnotationType.connection_type, null: true,
             description: 'Annotations added to the dashboard',
             resolver: Resolvers::Metrics::Dashboards::AnnotationResolver
-
-      # In order to maintain backward compatibility we need to return NULL when there are no warnings
-      # and dashboard validation returns an empty array when there are no issues.
-      def schema_validation_warnings
-        warnings = object.schema_validation_warnings
-        warnings unless warnings.empty?
-      end
     end
     # rubocop: enable Graphql/AuthorizeTypes
   end

app/models/blob_viewer/metrics_dashboard_yml.rb

@@ -26,10 +26,19 @@ module BlobViewer
 
     def parse_blob_data
       yaml = ::Gitlab::Config::Loader::Yaml.new(blob.data).load_raw!
-      Gitlab::Metrics::Dashboard::Validator
-        .errors(yaml, dashboard_path: blob.path, project: project)
+
+      ::PerformanceMonitoring::PrometheusDashboard.from_json(yaml)
+      nil
     rescue Gitlab::Config::Loader::FormatError => error
-      [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

@@ -53,18 +53,14 @@ module PerformanceMonitoring
     # This method is planned to be refactored as a part of https://gitlab.com/gitlab-org/gitlab/-/issues/219398
     # implementation. For new existing logic was reused to faster deliver MVC
     def schema_validation_warnings
-      run_custom_validation.map(&:message)
+      self.class.from_json(reload_schema)
+      nil
+    rescue ActiveModel::ValidationError => exception
+      exception.model.errors.map { |attr, error| "#{attr}: #{error}" }
     end
 
     private
 
-    def run_custom_validation
-      Gitlab::Metrics::Dashboard::Validator
-        .errors(reload_schema, dashboard_path: path, project: environment&.project)
-    rescue Gitlab::Config::Loader::FormatError => error
-      [error.message]
-    end
-
     # dashboard finder methods are somehow limited, #find includes checking if
     # user is authorised to view selected dashboard, but modifies schema, which in some cases may
     # cause false positives returned from validation, and #find_raw does not authorise users

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

@@ -5,7 +5,7 @@
   = icon('warning fw')
   = _('Metrics Dashboard YAML definition is invalid:')
   %ul
-    - viewer.errors.each do |error|
-      %li= error
+    - viewer.errors.messages.each do |error|
+      %li= error.join(': ')
 
 = link_to _('Learn more'), help_page_path('operations/metrics/dashboards/index.md')

changelogs/unreleased/mwaw-219398-metrics-extend-dashboard-yaml-definion-validation-when-viewin.yml

----
-title: Change metrics dashboard schema validation messages into exhaustive list of
-  all encountered errors.
-merge_request: 38925
-author:
-type: changed

doc/operations/metrics/dashboards/yaml.md

@@ -156,46 +156,21 @@ and files with invalid syntax display **Metrics Dashboard YAML definition is inv
 
 When **Metrics Dashboard YAML definition is invalid** at least one of the following messages is displayed:
 
-1. `[location] is missing required keys: [list of missing keys]` - The entry at
-   `[location]` is missing a key, or a key has been mistyped. This
-   example returns the error `root is missing required keys: panel_groups`:
-
-   ```yaml
-   dashboard: Important metrics
-   group_panels:
-     - ...
-   ```
-
-1. `[data] at [location] is not of type: [type]` - The entry at `[location]` contains
-   `[data]` which type does not adhere to required types. This example returns the
-   error `'123' at /panel_groups/0/group is not of type: string`:
-
-   ```yaml
-   dashboard: Environment metrics
-   panel_groups:
-   - group: 123
-     panels:
-     ...
-   ```
-
-1. `[data] at [location] is not one of: [types]` - The entry at `[location]` contains
-   `[data]` which is not included in the list of required values. This example returns
-   the error `'scatterplot-chart' at /panel_groups/0/panels/0/type is not one of: ["area-chart", "line-chart", "anomaly-chart", "bar", "column", "stacked-column", "single-stat", "heatmap"]`:
-
-   ```yaml
-   dashboard: Environment metrics
-   panel_groups:
-   - group: Network
-     panels:
-     - title: Throughput
-       type: scatterplot-chart
-       y_label: Requests / Sec
-     ...
-   ```
-
-1. `metric_id must be unique across a project` - At least two metrics entries have
-   the same `id` attribute, which [must be unique](#metrics-metrics-properties).
-1. `The parsed YAML is too big` - The YAML file is larger than 1 MB.
-1. `Invalid configuration format` - The YAML file is empty or does not contain valid YAML.
+1. `dashboard: can't be blank` [learn more](#dashboard-top-level-properties)
+1. `panel_groups: should be an array of panel_groups objects` [learn more](#dashboard-top-level-properties)
+1. `group: can't be blank` [learn more](#panel-group-panel_groups-properties)
+1. `panels: should be an array of panels objects` [learn more](#panel-group-panel_groups-properties)
+1. `title: can't be blank` [learn more](#panel-panels-properties)
+1. `metrics: should be an array of metrics objects` [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)
+1. `YAML syntax: The parsed YAML is too big`
+
+   This is displayed when the YAML file is larger than 1 MB.
+
+1. `YAML syntax: Invalid configuration format`
+
+   This is displayed when the YAML file is empty or does not contain valid YAML.
 
 Metrics Dashboard YAML definition validation information is also available as a [GraphQL API field](../../../api/graphql/reference/index.md#metricsdashboard)

lib/gitlab/metrics/dashboard/validator.rb

@@ -8,18 +8,20 @@ module Gitlab
 
         class << self
           def validate(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
-            errors(content, schema_path, dashboard_path: dashboard_path, project: project).empty?
+            errors = _validate(content, schema_path, dashboard_path: dashboard_path, project: project)
+            errors.empty?
           end
 
           def validate!(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
-            errors = errors(content, schema_path, dashboard_path: dashboard_path, project: project)
+            errors = _validate(content, schema_path, dashboard_path: dashboard_path, project: project)
             errors.empty? || raise(errors.first)
           end
 
-          def errors(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
-            Validator::Client
-              .new(content, schema_path, dashboard_path: dashboard_path, project: project)
-              .execute
+          private
+
+          def _validate(content, schema_path, dashboard_path: nil, project: nil)
+            client = Validator::Client.new(content, schema_path, dashboard_path: dashboard_path, project: project)
+            client.execute
           end
         end
       end

lib/gitlab/metrics/dashboard/validator/client.rb

@@ -46,7 +46,7 @@ module Gitlab
 
           def validate_against_schema
             schemer.validate(content).map do |error|
-              ::Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError.new(error)
+              Errors::SchemaValidationError.new(error)
             end
           end
         end

lib/gitlab/metrics/dashboard/validator/schemas/panel.json

@@ -4,7 +4,7 @@
   "properties": {
     "type": {
       "type": "string",
-      "enum": ["area-chart", "line-chart", "anomaly-chart", "bar", "column", "stacked-column", "single-stat", "heatmap", "gauge"],
+      "enum": ["area-chart", "anomaly-chart", "bar", "column", "stacked-column", "single-stat", "heatmap"],
       "default": "area-chart"
     },
     "title": { "type": "string" },

spec/features/projects/blobs/blob_show_spec.rb

@@ -589,7 +589,7 @@ RSpec.describe 'File blob', :js 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("root is missing required keys: panel_groups")
+          expect(page).to have_content("panel_groups: should be an array of panel_groups objects")
 
           # shows a learn more link
           expect(page).to have_link('Learn more')

spec/lib/gitlab/metrics/dashboard/validator/errors_spec.rb

@@ -34,19 +34,6 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator::Errors do
 
           it { is_expected.to eq 'root is missing required keys: one' }
         end
-
-        context 'when there is type mismatch' do
-          %w(null string boolean integer number array object).each do |expected_type|
-            context "on type: #{expected_type}" do
-              let(:type) { expected_type }
-              let(:details) { nil }
-
-              subject { described_class.new(error_hash).message }
-
-              it { is_expected.to eq "'property_name' at root is not of type: #{expected_type}" }
-            end
-          end
-        end
       end
 
       context 'for nested object' do

spec/lib/gitlab/metrics/dashboard/validator_spec.rb

@@ -143,56 +143,4 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
       end
     end
   end
-
-  describe '#errors' do
-    context 'valid dashboard schema' do
-      it 'returns no errors' do
-        expect(described_class.errors(valid_dashboard)).to eq []
-      end
-
-      context 'with duplicate metric_ids' do
-        it 'returns errors' do
-          expect(described_class.errors(duplicate_id_dashboard)).to eq [Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds.new]
-        end
-      end
-
-      context 'with dashboard_path and project' do
-        subject { described_class.errors(valid_dashboard, dashboard_path: 'test/path.yml', project: project) }
-
-        context 'with no conflicting metric identifiers in db' do
-          it { is_expected.to eq [] }
-        end
-
-        context 'with metric identifier present in current dashboard' do
-          before do
-            create(:prometheus_metric,
-                   identifier:     'metric_a1',
-                   dashboard_path: 'test/path.yml',
-                   project:        project
-                  )
-          end
-
-          it { is_expected.to eq [] }
-        end
-
-        context 'with metric identifier present in another dashboard' do
-          before do
-            create(:prometheus_metric,
-                   identifier:     'metric_a1',
-                   dashboard_path: 'some/other/dashboard/path.yml',
-                   project:        project
-                  )
-          end
-
-          it { is_expected.to eq [Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds.new] }
-        end
-      end
-    end
-
-    context 'invalid dashboard schema' do
-      it 'returns collection of validation errors' do
-        expect(described_class.errors(invalid_dashboard)).to all be_kind_of(Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError)
-      end
-    end
-  end
 end

spec/models/blob_viewer/metrics_dashboard_yml_spec.rb

@@ -13,11 +13,11 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
   subject(:viewer) { described_class.new(blob) }
 
   context 'when the definition is valid' do
-    let(:data) { fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml')}
+    let(:data) { File.read(Rails.root.join('config/prometheus/common_metrics.yml')) }
 
     describe '#valid?' do
       it 'calls prepare! on the viewer' do
-        allow(Gitlab::Metrics::Dashboard::Validator).to receive(:errors)
+        allow(PerformanceMonitoring::PrometheusDashboard).to receive(:from_json)
 
         expect(viewer).to receive(:prepare!)
 
@@ -30,44 +30,46 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
         expect_next_instance_of(::Gitlab::Config::Loader::Yaml, data) do |loader|
           expect(loader).to receive(:load_raw!).and_call_original
         end
-        expect(Gitlab::Metrics::Dashboard::Validator)
-          .to receive(:errors)
-          .with(yml, dashboard_path: '.gitlab/dashboards/custom-dashboard.yml', project: project)
+        expect(PerformanceMonitoring::PrometheusDashboard)
+          .to receive(:from_json)
+          .with(yml)
           .and_call_original
         expect(viewer.valid?).to be_truthy
       end
     end
 
     describe '#errors' do
-      it 'returns empty array' do
-        allow(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).and_return([])
+      it 'returns nil' do
+        allow(PerformanceMonitoring::PrometheusDashboard).to receive(:from_json)
 
-        expect(viewer.errors).to eq []
+        expect(viewer.errors).to be nil
       end
     end
   end
 
   context 'when definition is invalid' do
-    let(:error) { ::Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError.new }
+    let(:error) { ActiveModel::ValidationError.new(PerformanceMonitoring::PrometheusDashboard.new.tap(&:validate)) }
     let(:data) do
       <<~YAML
         dashboard:
       YAML
     end
 
-    before do
-      allow(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).and_return([error])
-    end
-
     describe '#valid?' do
       it 'returns false' do
-        expect(viewer.valid?).to be false
+        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
-        expect(viewer.errors).to eq [error]
+        allow(PerformanceMonitoring::PrometheusDashboard)
+          .to receive(:from_json).and_raise(error)
+
+        expect(viewer.errors).to be error.model.errors
       end
     end
   end
@@ -83,14 +85,17 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
 
     describe '#valid?' do
       it 'returns false' do
-        expect(Gitlab::Metrics::Dashboard::Validator).not_to receive(:errors)
-        expect(viewer.valid?).to be false
+        expect(PerformanceMonitoring::PrometheusDashboard).not_to receive(:from_json)
+        expect(viewer.valid?).to be_falsey
       end
     end
 
     describe '#errors' do
       it 'returns validation errors' do
-        expect(viewer.errors).to all be_kind_of Gitlab::Config::Loader::FormatError
+        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
@@ -108,12 +113,15 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
     end
 
     it 'is invalid' do
-      expect(Gitlab::Metrics::Dashboard::Validator).not_to receive(:errors)
-      expect(viewer.valid?).to be false
+      expect(PerformanceMonitoring::PrometheusDashboard).not_to receive(:from_json)
+      expect(viewer.valid?).to be(false)
     end
 
     it 'returns validation errors' do
-      expect(viewer.errors).to all be_kind_of Gitlab::Config::Loader::FormatError
+      yaml_wrapped_errors = { 'YAML syntax': ["The parsed YAML is too big"] }
+
+      expect(viewer.errors).to be_kind_of(ActiveModel::Errors)
+      expect(viewer.errors.messages).to eq(yaml_wrapped_errors)
     end
   end
 end

spec/models/performance_monitoring/prometheus_dashboard_spec.rb

@@ -219,31 +219,20 @@ RSpec.describe PerformanceMonitoring::PrometheusDashboard do
   end
 
   describe '#schema_validation_warnings' do
-    let_it_be(:project) { create(:project) }
-    let_it_be(:environment) { create(:environment, project: project) }
-    let(:path) { '.gitlab/dashboards/test.yml' }
-
-    subject(:schema_validation_warnings) { described_class.new(json_content.merge(path: path, environment: environment)).schema_validation_warnings }
-
-    before do
-      allow(Gitlab::Metrics::Dashboard::Finder).to receive(:find_raw).with(project, dashboard_path: path).and_return(json_content)
-    end
-
     context 'when schema is valid' do
       it 'returns nil' do
-        expect(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).with(json_content, dashboard_path: path, project: project).and_return([])
-
-        expect(schema_validation_warnings).to eq []
+        expect(described_class).to receive(:from_json)
+        expect(described_class.new.schema_validation_warnings).to be_nil
       end
     end
 
     context 'when schema is invalid' do
       it 'returns array with errors messages' do
-        error = ::Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError.new
-
-        expect(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).with(json_content, dashboard_path: path, project: project).and_return([error])
+        instance = described_class.new
+        instance.errors.add(:test, 'test error')
 
-        expect(schema_validation_warnings).to eq [error.message]
+        expect(described_class).to receive(:from_json).and_raise(ActiveModel::ValidationError.new(instance))
+        expect(described_class.new.schema_validation_warnings).to eq ['test: test error']
       end
     end
   end

spec/requests/api/graphql/metrics/dashboard_query_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe 'Getting Metrics Dashboard' do
 
   let_it_be(:current_user) { create(:user) }
   let(:project) { create(:project) }
-  let(:environment) { create(:environment, project: project) }
+  let!(:environment) { create(:environment, project: project) }
 
   let(:query) do
     graphql_query_for(
@@ -67,7 +67,7 @@ RSpec.describe 'Getting Metrics Dashboard' do
         it 'returns metrics dashboard' do
           dashboard = graphql_data.dig('project', 'environments', 'nodes', 0, 'metricsDashboard')
 
-          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["root is missing required keys: panel_groups"])
+          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["panel_groups: should be an array of panel_groups objects"])
         end
       end
 
@@ -78,7 +78,7 @@ RSpec.describe 'Getting Metrics Dashboard' do
         it 'returns metrics dashboard' do
           dashboard = graphql_data.dig('project', 'environments', 'nodes', 0, 'metricsDashboard')
 
-          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["root is missing required keys: dashboard, panel_groups"])
+          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["dashboard: can't be blank", "panel_groups: should be an array of panel_groups objects"])
         end
       end
     end