Merge branch 'mwaw/210289-fix-dashboard-validation-false-positives-warnings' into 'master'

Metrics dashboard schema validation egde cases

Closes #210289

See merge request gitlab-org/gitlab!34166

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

@@ -27624,6 +27624,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