Fix errors in metrics dashboard validation

When dashboard schema used wrong object types for nested objects
(eg: string insted of an array) validation raised 500 errors.
Also when metrics dashboard raised errors while beening processed
by dashboard finder, validation reported false positve warning,
due finder response which does not contain full schema.

app/models/performance_monitoring/prometheus_dashboard.rb

@@ -7,7 +7,7 @@ module PerformanceMonitoring
     attr_accessor :dashboard, :panel_groups, :path, :environment, :priority, :templating, :links
 
     validates :dashboard, presence: true
-    validates :panel_groups, presence: true
+    validates :panel_groups, array_members: { member_class: PerformanceMonitoring::PrometheusPanelGroup }
 
     class << self
       def from_json(json_content)
@@ -35,9 +35,15 @@ module PerformanceMonitoring
 
         new(
           dashboard: attributes['dashboard'],
-          panel_groups: attributes['panel_groups']&.map { |group| PrometheusPanelGroup.from_json(group) }
+          panel_groups: initialize_children_collection(attributes['panel_groups'])
         )
       end
+
+      def initialize_children_collection(children)
+        return unless children.is_a?(Array)
+
+        children.map { |group| PerformanceMonitoring::PrometheusPanelGroup.from_json(group) }
+      end
     end
 
     def to_yaml
@@ -47,7 +53,7 @@ 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
-      self.class.from_json(self.as_json)
+      self.class.from_json(reload_schema)
       nil
     rescue ActiveModel::ValidationError => exception
       exception.model.errors.map { |attr, error| "#{attr}: #{error}" }
@@ -55,6 +61,14 @@ module PerformanceMonitoring
 
     private
 
+    # 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
+    def reload_schema
+      project = environment&.project
+      project.nil? ? self.as_json : Gitlab::Metrics::Dashboard::Finder.find_raw(project, dashboard_path: path)
+    end
+
     def yaml_valid_attributes
       %w(panel_groups panels metrics group priority type title y_label weight id unit label query query_range dashboard)
     end

app/models/performance_monitoring/prometheus_panel.rb

@@ -7,7 +7,8 @@ module PerformanceMonitoring
     attr_accessor :type, :title, :y_label, :weight, :metrics, :y_axis, :max_value
 
     validates :title, presence: true
-    validates :metrics, presence: true
+    validates :metrics, array_members: { member_class: PerformanceMonitoring::PrometheusMetric }
+
     class << self
       def from_json(json_content)
         build_from_hash(json_content).tap(&:validate!)
@@ -23,9 +24,15 @@ module PerformanceMonitoring
           title: attributes['title'],
           y_label: attributes['y_label'],
           weight: attributes['weight'],
-          metrics: attributes['metrics']&.map { |metric| PrometheusMetric.from_json(metric) }
+          metrics: initialize_children_collection(attributes['metrics'])
         )
       end
+
+      def initialize_children_collection(children)
+        return unless children.is_a?(Array)
+
+        children.map { |metrics| PerformanceMonitoring::PrometheusMetric.from_json(metrics) }
+      end
     end
 
     def id(group_title)

app/models/performance_monitoring/prometheus_panel_group.rb

@@ -7,7 +7,8 @@ module PerformanceMonitoring
     attr_accessor :group, :priority, :panels
 
     validates :group, presence: true
-    validates :panels, presence: true
+    validates :panels, array_members: { member_class: PerformanceMonitoring::PrometheusPanel }
+
     class << self
       def from_json(json_content)
         build_from_hash(json_content).tap(&:validate!)
@@ -21,9 +22,15 @@ module PerformanceMonitoring
         new(
           group: attributes['group'],
           priority: attributes['priority'],
-          panels: attributes['panels']&.map { |panel| PrometheusPanel.from_json(panel) }
+          panels: initialize_children_collection(attributes['panels'])
         )
       end
+
+      def initialize_children_collection(children)
+        return unless children.is_a?(Array)
+
+        children.map { |panels| PerformanceMonitoring::PrometheusPanel.from_json(panels) }
+      end
     end
   end
 end

app/validators/array_members_validator.rb

+# frozen_string_literal: true
+
+# ArrayMembersValidator
+#
+# Custom validator that checks if validated
+# attribute contains non empty array, which every
+# element is an instances of :member_class
+#
+# Example:
+#
+#   class Config::Root < ActiveRecord::Base
+#     validates :nodes, member_class: Config::Node
+#   end
+#
+class ArrayMembersValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    if !value.is_a?(Array) || value.empty? || value.any? { |child| !child.instance_of?(options[:member_class]) }
+      record.errors.add(attribute, _("should be an array of %{object_name} objects") % { object_name: options.fetch(:object_name, attribute) })
+    end
+  end
+end

changelogs/unreleased/mwaw-210289-fix-dashboard-validation-false-positives-warnings.yml

+---
+title: Fix 500 errors and false positive warnings during metrics dashboard validation.
+merge_request: 34166
+author:
+type: fixed

doc/user/project/integrations/prometheus.md

@@ -353,11 +353,11 @@ 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. `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. `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: can't be blank` [learn more](#panel-group-panel_groups-properties)
-1. `metrics: can't be blank` [learn more](#panel-panels-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)

locale/gitlab.pot

@@ -27618,6 +27618,9 @@ msgstr ""
 msgid "severity|Unknown"
 msgstr ""
 
+msgid "should be an array of %{object_name} objects"
+msgstr ""
+
 msgid "should be greater than or equal to %{access} inherited membership from group %{group_name}"
 msgstr ""
 

spec/features/projects/blobs/blob_show_spec.rb

@@ -593,7 +593,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("panel_groups: can't be blank")
+          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/models/performance_monitoring/prometheus_dashboard_spec.rb

@@ -50,19 +50,19 @@ describe PerformanceMonitoring::PrometheusDashboard do
       context 'dashboard content is missing' do
         let(:json_content) { nil }
 
-        it_behaves_like 'validation failed', panel_groups: ["can't be blank"], dashboard: ["can't be blank"]
+        it_behaves_like 'validation failed', panel_groups: ["should be an array of panel_groups objects"], dashboard: ["can't be blank"]
       end
 
       context 'dashboard content is NOT a hash' do
         let(:json_content) { YAML.safe_load("'test'") }
 
-        it_behaves_like 'validation failed', panel_groups: ["can't be blank"], dashboard: ["can't be blank"]
+        it_behaves_like 'validation failed', panel_groups: ["should be an array of panel_groups objects"], dashboard: ["can't be blank"]
       end
 
       context 'content is an array' do
         let(:json_content) { [{ "dashboard" => "Dashboard Title" }] }
 
-        it_behaves_like 'validation failed', panel_groups: ["can't be blank"], dashboard: ["can't be blank"]
+        it_behaves_like 'validation failed', panel_groups: ["should be an array of panel_groups objects"], dashboard: ["can't be blank"]
       end
 
       context 'dashboard definition is missing panels_groups and dashboard keys' do
@@ -72,7 +72,7 @@ describe PerformanceMonitoring::PrometheusDashboard do
           }
         end
 
-        it_behaves_like 'validation failed', panel_groups: ["can't be blank"], dashboard: ["can't be blank"]
+        it_behaves_like 'validation failed', panel_groups: ["should be an array of panel_groups objects"], dashboard: ["can't be blank"]
       end
 
       context 'group definition is missing panels and group keys' do
@@ -88,7 +88,7 @@ describe PerformanceMonitoring::PrometheusDashboard do
           }
         end
 
-        it_behaves_like 'validation failed', panels: ["can't be blank"], group: ["can't be blank"]
+        it_behaves_like 'validation failed', panels: ["should be an array of panels objects"], group: ["can't be blank"]
       end
 
       context 'panel definition is missing metrics and title keys' do
@@ -110,7 +110,7 @@ describe PerformanceMonitoring::PrometheusDashboard do
           }
         end
 
-        it_behaves_like 'validation failed', metrics: ["can't be blank"], title: ["can't be blank"]
+        it_behaves_like 'validation failed', metrics: ["should be an array of metrics objects"], title: ["can't be blank"]
       end
 
       context 'metrics definition is missing unit, query and query_range keys' do
@@ -180,7 +180,7 @@ describe PerformanceMonitoring::PrometheusDashboard do
   describe '.find_for' do
     let(:project) { build_stubbed(:project) }
     let(:user) { build_stubbed(:user) }
-    let(:environment) { build_stubbed(:environment) }
+    let(:environment) { build_stubbed(:environment, project: project) }
     let(:path) { ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH }
 
     context 'dashboard has been found' do

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

@@ -62,12 +62,12 @@ describe 'Getting Metrics Dashboard' do
 
       context 'invalid dashboard' do
         let(:path) { '.gitlab/dashboards/metrics.yml' }
-        let(:project) { create(:project, :repository, :custom_repo, namespace: current_user.namespace, files: { path => "---\ndasboard: ''" }) }
+        let(:project) { create(:project, :repository, :custom_repo, namespace: current_user.namespace, files: { path => "---\ndashboard: 'test'" }) }
 
         it 'returns metrics dashboard' do
           dashboard = graphql_data.dig('project', 'environments', 'nodes', 0, 'metricsDashboard')
 
-          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["dashboard: can't be blank", "panel_groups: can't be blank"])
+          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["panel_groups: should be an array of panel_groups objects"])
         end
       end
 
@@ -78,7 +78,7 @@ 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" => ["dashboard: can't be blank", "panel_groups: can't be blank"])
+          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

spec/validators/array_members_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ArrayMembersValidator do
+  using RSpec::Parameterized::TableSyntax
+
+  child_class = Class.new
+
+  subject(:test_class) do
+    Class.new do
+      include ActiveModel::Model
+      include ActiveModel::Validations
+      attr_accessor :children
+      validates :children, array_members: { member_class: child_class }
+    end
+  end
+
+  where(:children, :is_valid) do
+    [child_class.new]                | true
+    [Class.new.new]                  | false
+    [child_class.new, Class.new.new] | false
+    []                               | false
+    child_class.new                  | false
+    [Class.new(child_class).new]     | false
+  end
+
+  with_them do
+    it 'only accepts valid children nodes' do
+      expect(test_class.new(children: children).valid?).to eq(is_valid)
+    end
+  end
+
+  context 'validation message' do
+    subject(:test_class) do
+      Class.new do
+        include ActiveModel::Model
+        include ActiveModel::Validations
+        attr_accessor :children
+      end
+    end
+
+    context 'with default object name' do
+      it 'uses attribute name', :aggregate_failures do
+        test_class.class_eval do
+          validates :children, array_members: { member_class: child_class }
+        end
+
+        object = test_class.new(children: [])
+
+        expect(object.valid?).to be_falsey
+        expect(object.errors.messages).to eql(children: ['should be an array of children objects'])
+      end
+    end
+
+    context 'with custom object name' do
+      it 'uses that name', :aggregate_failures do
+        test_class.class_eval do
+          validates :children, array_members: { member_class: child_class, object_name: 'test' }
+        end
+
+        object = test_class.new(children: [])
+
+        expect(object.valid?).to be_falsey
+        expect(object.errors.messages).to eql(children: ['should be an array of test objects'])
+      end
+    end
+  end
+end