Merge branch 'rc/update_validator_dashboard_path' into 'master'

Update metric dashboard validator

See merge request gitlab-org/gitlab!38847

lib/gitlab/metrics/dashboard/validator.rb

@@ -7,20 +7,20 @@ module Gitlab
         DASHBOARD_SCHEMA_PATH = 'lib/gitlab/metrics/dashboard/validator/schemas/dashboard.json'.freeze
 
         class << self
-          def validate(content, schema_path = DASHBOARD_SCHEMA_PATH, project: nil)
-            errors = _validate(content, schema_path, project: project)
+          def validate(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
+            errors = _validate(content, schema_path, dashboard_path: dashboard_path, project: project)
             errors.empty?
           end
 
-          def validate!(content, schema_path = DASHBOARD_SCHEMA_PATH, project: nil)
-            errors = _validate(content, schema_path, project: project)
+          def validate!(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
+            errors = _validate(content, schema_path, dashboard_path: dashboard_path, project: project)
             errors.empty? || raise(errors.first)
           end
 
           private
 
-          def _validate(content, schema_path, project)
-            client = Client.new(content, schema_path, project: project)
+          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

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

@@ -8,9 +8,12 @@ module Gitlab
           # @param content [Hash] Representing a raw, unprocessed
           # dashboard object
           # @param schema_path [String] Representing path to dashboard schema file
-          def initialize(content, schema_path, project: nil)
+          # @param dashboard_path[String] Representing path to dashboard content file
+          # @param project [Project] Project to validate dashboard against
+          def initialize(content, schema_path, dashboard_path: nil, project: nil)
             @content = content
             @schema_path = schema_path
+            @dashboard_path = dashboard_path
             @project = project
           end
 
@@ -23,14 +26,18 @@ module Gitlab
 
           private
 
-          attr_reader :content, :schema_path, :project
+          attr_reader :content, :schema_path, :project, :dashboard_path
 
           def custom_formats
             @custom_formats ||= CustomFormats.new
           end
 
           def post_schema_validator
-            @post_schema_validator ||= PostSchemaValidator.new(project: project, metric_ids: custom_formats.metric_ids_cache)
+            PostSchemaValidator.new(
+              project:        project,
+              metric_ids:     custom_formats.metric_ids_cache,
+              dashboard_path: dashboard_path
+            )
           end
 
           def schemer

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

@@ -5,25 +5,46 @@ module Gitlab
     module Dashboard
       module Validator
         class PostSchemaValidator
-          def initialize(project: nil, metric_ids: [])
-            @project = project
+          def initialize(metric_ids:, project: nil, dashboard_path: nil)
             @metric_ids = metric_ids
+            @project = project
+            @dashboard_path = dashboard_path
           end
 
           def validate
-            [uniq_metric_ids_across_project].compact
+            errors = []
+            errors << uniq_metric_ids
+            errors.compact
           end
 
           private
 
-          attr_reader :project, :metric_ids
+          attr_reader :project, :metric_ids, :dashboard_path
+
+          def uniq_metric_ids
+            return Validator::Errors::DuplicateMetricIds.new if metric_ids.uniq!
 
+            uniq_metric_ids_across_project if project.present? || dashboard_path.present?
+          end
+
+          # rubocop: disable CodeReuse/ActiveRecord
           def uniq_metric_ids_across_project
-            # TODO: modify this method to check metric identifier uniqueness across project once we start
-            # recording dashboard_path https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38237
+            return ArgumentError.new(_('Both project and dashboard_path are required')) unless
+              dashboard_path.present? && project.present?
+
+            # If PrometheusMetric identifier is not unique across project and dashboard_path,
+            # we need to error because we don't know if the user is trying to create a new metric
+            # or update an existing one.
+            identifier_on_other_dashboard = PrometheusMetric.where(
+              project: project,
+              identifier: metric_ids
+            ).where.not(
+              dashboard_path: dashboard_path
+            ).exists?
 
-            Validator::Errors::DuplicateMetricIds.new if metric_ids.uniq!
+            Validator::Errors::DuplicateMetricIds.new if identifier_on_other_dashboard
           end
+          # rubocop: enable CodeReuse/ActiveRecord
         end
       end
     end

locale/gitlab.pot

@@ -3932,6 +3932,9 @@ msgstr ""
 msgid "Boards|View scope"
 msgstr ""
 
+msgid "Both project and dashboard_path are required"
+msgstr ""
+
 msgid "Branch"
 msgstr ""
 

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

@@ -22,7 +22,7 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator::Client do
       let(:dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/invalid_dashboard.yml')) }
 
       it 'returns array of error objects' do
-        expect(subject.execute).to all(be_a(Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError))
+        expect(subject.execute).to include(Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError)
       end
     end
   end

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

@@ -4,17 +4,75 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Metrics::Dashboard::Validator::PostSchemaValidator do
   describe '#validate' do
-    context 'unique metric ids' do
-      it 'returns blank array' do
+    context 'with no project and dashboard_path provided' do
+      context 'unique local metric_ids' do
+        it 'returns empty array' do
           expect(described_class.new(metric_ids: [1, 2, 3]).validate).to eq([])
         end
       end
 
-    context 'duplicate metric ids' do
-      it 'raises error' do
+      context 'duplicate local metrics_ids' do
+        it 'returns error' do
           expect(described_class.new(metric_ids: [1, 1]).validate)
             .to eq([Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds])
         end
       end
     end
+
+    context 'with project and dashboard_path' do
+      let(:project) { create(:project) }
+
+      subject do
+        described_class.new(
+          project: project,
+          metric_ids: ['some_identifier'],
+          dashboard_path: 'test/path.yml'
+        ).validate
+      end
+
+      context 'with unique metric identifiers' do
+        before do
+          create(:prometheus_metric,
+            project: project,
+            identifier: 'some_other_identifier',
+            dashboard_path: 'test/path.yml'
+          )
+        end
+
+        it 'returns empty array' do
+          expect(subject).to eq([])
+        end
+      end
+
+      context 'duplicate metric identifiers in database' do
+        context 'with different dashboard_path' do
+          before do
+            create(:prometheus_metric,
+              project: project,
+              identifier: 'some_identifier',
+              dashboard_path: 'some/other/path.yml'
+            )
+          end
+
+          it 'returns error' do
+            expect(subject).to include(Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds)
+          end
+        end
+
+        context 'with same dashboard_path' do
+          before do
+            create(:prometheus_metric,
+              project: project,
+              identifier: 'some_identifier',
+              dashboard_path: 'test/path.yml'
+            )
+          end
+
+          it 'returns empty array' do
+            expect(subject).to eq([])
+          end
+        end
+      end
+    end
+  end
 end

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

@@ -9,27 +9,57 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
   let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/invalid_dashboard.yml')) }
   let_it_be(:duplicate_id_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/duplicate_id_dashboard.yml')) }
 
+  let_it_be(:project) { create(:project) }
+
   describe '#validate' do
-    context 'valid dashboard' do
+    context 'valid dashboard schema' do
       it 'returns true' do
         expect(described_class.validate(valid_dashboard)).to be true
       end
-    end
 
-    context 'invalid dashboard' do
-      context 'invalid schema' do
+      context 'with duplicate metric_ids' do
         it 'returns false' do
-          expect(described_class.validate(invalid_dashboard)).to be false
+          expect(described_class.validate(duplicate_id_dashboard)).to be false
         end
       end
 
-      context 'duplicate metric ids' do
-        context 'with no project given' do
-          it 'checks against given dashboard and returns false' do
-            expect(described_class.validate(duplicate_id_dashboard)).to be false
+      context 'with dashboard_path and project' do
+        subject { described_class.validate(valid_dashboard, dashboard_path: 'test/path.yml', project: project) }
+
+        context 'with no conflicting metric identifiers in db' do
+          it { is_expected.to be true }
+        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 be true }
+        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 be false }
         end
       end
     end
+
+    context 'invalid dashboard schema' do
+      it 'returns false' do
+        expect(described_class.validate(invalid_dashboard)).to be false
+      end
     end
   end
 
@@ -43,16 +73,53 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
       end
     end
 
-    context 'valid dashboard' do
+    context 'valid dashboard schema' do
       it 'returns true' do
         expect(described_class.validate!(valid_dashboard)).to be true
       end
+
+      context 'with duplicate metric_ids' do
+        subject { described_class.validate!(duplicate_id_dashboard) }
+
+        it_behaves_like 'validation failed', 'metric_id must be unique across a project'
       end
 
-    context 'invalid dashboard' do
+      context 'with dashboard_path and project' do
+        subject { described_class.validate!(valid_dashboard, dashboard_path: 'test/path.yml', project: project) }
+
+        context 'with no conflicting metric identifiers in db' do
+          it { is_expected.to be true }
+        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 be true }
+        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_behaves_like 'validation failed', 'metric_id must be unique across a project'
+        end
+      end
+    end
+
+    context 'invalid dashboard schema' do
       subject { described_class.validate!(invalid_dashboard) }
 
-      context 'invalid schema' do
       context 'wrong property type' do
         it_behaves_like 'validation failed', "'this_should_be_a_int' at /panel_groups/0/panels/0/weight is not of type: number"
       end
@@ -75,14 +142,5 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
         it_behaves_like 'validation failed', '/panel_groups/0/panels/0 is missing required keys: metrics'
       end
     end
-
-      context 'duplicate metric ids' do
-        context 'with no project given' do
-          subject { described_class.validate!(duplicate_id_dashboard) }
-
-          it_behaves_like 'validation failed', 'metric_id must be unique across a project'
-        end
-      end
-    end
   end
 end