Merge branch 'mwaw/219398-improve-validation-errors-for-metrics-dashboard-yaml' into 'master'

Improve metrics dashboard validation error messages

See merge request gitlab-org/gitlab!39123

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

@@ -34,7 +34,7 @@ module Gitlab
           end
 
           def schemer
-            @schemer ||= JSONSchemer.schema(Pathname.new(schema_path), formats: custom_formats.format_handlers)
+            @schemer ||= ::JSONSchemer.schema(Pathname.new(schema_path), formats: custom_formats.format_handlers)
           end
 
           def validate_against_schema

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

@@ -9,19 +9,42 @@ module Gitlab
 
           class SchemaValidationError < InvalidDashboardError
             def initialize(error = {})
-              if error.is_a?(Hash) && error.present?
-                data            = error["data"]
-                data_pointer    = error["data_pointer"]
-                schema          = error["schema"]
-                schema_pointer  = error["schema_pointer"]
+              super(error_message(error))
+            end
 
-                msg = _("'%{data}' is invalid at '%{data_pointer}'. Should be '%{schema}' due to schema definition at '%{schema_pointer}'") %
-                  { data: data, data_pointer: data_pointer, schema: schema, schema_pointer: schema_pointer }
+            private
+
+            def error_message(error)
+              if error.is_a?(Hash) && error.present?
+                pretty(error)
               else
-                msg = "Dashboard failed schema validation"
+                "Dashboard failed schema validation"
               end
+            end
+
+            # based on https://github.com/davishmcclurg/json_schemer/blob/master/lib/json_schemer/errors.rb
+            # with addition ability to translate error messages
+            def pretty(error)
+              data, data_pointer, type, schema = error.values_at('data', 'data_pointer', 'type', 'schema')
+              location = data_pointer.empty? ? 'root' : data_pointer
 
-              super(msg)
+              case type
+              when 'required'
+                keys = error.fetch('details').fetch('missing_keys').join(', ')
+                _("%{location} is missing required keys: %{keys}") % { location: location, keys: keys }
+              when 'null', 'string', 'boolean', 'integer', 'number', 'array', 'object'
+                _("'%{data}' at %{location} is not of type: %{type}") % { data: data, location: location, type: type }
+              when 'pattern'
+                _("'%{data}' at %{location} does not match pattern: %{pattern}") % { data: data, location: location, pattern: schema.fetch('pattern') }
+              when 'format'
+                _("'%{data}' at %{location} does not match format: %{format}") % { data: data, location: location, format: schema.fetch('format') }
+              when 'const'
+                _("'%{data}' at %{location} is not: %{const}") % { data: data, location: location, const: schema.fetch('const').inspect }
+              when 'enum'
+                _("'%{data}' at %{location} is not one of: %{enum}") % { data: data, location: location, enum: schema.fetch('enum') }
+              else
+                _("'%{data}' at %{location} is invalid: error_type=%{type}") % { data: data, location: location, type: type }
+              end
             end
           end
 

locale/gitlab.pot

@@ -527,6 +527,9 @@ msgstr ""
 msgid "%{loadingIcon} Started"
 msgstr ""
 
+msgid "%{location} is missing required keys: %{keys}"
+msgstr ""
+
 msgid "%{lock_path} is locked by GitLab User %{lock_user_id}"
 msgstr ""
 
@@ -806,7 +809,22 @@ msgstr ""
 msgid "<project name>"
 msgstr ""
 
-msgid "'%{data}' is invalid at '%{data_pointer}'. Should be '%{schema}' due to schema definition at '%{schema_pointer}'"
+msgid "'%{data}' at %{location} does not match format: %{format}"
+msgstr ""
+
+msgid "'%{data}' at %{location} does not match pattern: %{pattern}"
+msgstr ""
+
+msgid "'%{data}' at %{location} is invalid: error_type=%{type}"
+msgstr ""
+
+msgid "'%{data}' at %{location} is not of type: %{type}"
+msgstr ""
+
+msgid "'%{data}' at %{location} is not one of: %{enum}"
+msgstr ""
+
+msgid "'%{data}' at %{location} is not: %{const}"
 msgstr ""
 
 msgid "'%{level}' is not a valid visibility level"

spec/fixtures/lib/gitlab/metrics/dashboard/dashboard_groups_missing_panels_and_group.yml

+dashboard: 'Test Dashboard'
+panel_groups:
+- panels:
+  - title: "Super Chart A1"
+    type: "area-chart"
+    y_label: "y_label"
+    weight: 1
+    max_value: 1
+    metrics:
+    - id: metric_a1
+      query_range: |+
+        avg(
+          sum(
+            container_memory_usage_bytes{
+              container_name!="POD",
+              pod_name=~"^{{ci_environment_slug}}-(.*)",
+              namespace="{{kube_namespace}}"
+            }
+          ) by (job)
+        ) without (job)
+        /1024/1024/1024
+      unit: unit
+      label: Legend Label
+  - title: "Super Chart A2"
+    type: "area-chart"
+    y_label: "y_label"
+    weight: 2
+    metrics:
+    - id: metric_a2
+      query_range: 'query'
+      label: Legend Label
+      unit: unit
+- group: Group B

spec/fixtures/lib/gitlab/metrics/dashboard/dashboard_is_an_array.yml

+---
+- dashboard: 'Test Dashboard'
+  panel_groups:
+    - group: Group A
+      panels:
+      - title: "Super Chart A2"
+        type: "area-chart"
+        y_label: "y_label"
+        weight: 2
+        metrics:
+          - id: metric_a2
+            query_range: 'query'
+            label: Legend Label
+            unit: unit
+- dashboard: 'second entry'

spec/fixtures/lib/gitlab/metrics/dashboard/dashboard_missing_panel_groups.yml

+dashboard: 'Test Dashboard'
+priority: 1
+links:
+  - title: Link 1
+    url: https://gitlab.com
+  - title: Link 2
+    url: https://docs.gitlab.com
+templating:
+  variables:
+    text_variable_full_syntax:
+      label: 'Variable 1'
+      type: text
+      options:
+        default_value: 'default'
+    text_variable_simple_syntax: 'default value'
+    custom_variable_simple_syntax: ['value1', 'value2', 'value3']
+    custom_variable_full_syntax:
+      label: 'Variable 2'
+      type: custom
+      options:
+        values:
+          - value: 'value option 1'
+            text: 'Option 1'
+          - value: 'value_option_2'
+            text: 'Option 2'
+            default: true
+    metric_label_values_variable:
+      label: 'Variable 3'
+      type: metric_label_values
+      options:
+        series_selector: 'backend:haproxy_backend_availability:ratio{env="{{env}}"}'
+        label: 'backend'

spec/fixtures/lib/gitlab/metrics/dashboard/dashboard_panel_is_missing_metrics.yml

+dashboard: 'Test Dashboard'
+panel_groups:
+- group: Group A
+  priority: 1
+  panels:
+  - title: "Super Chart A1"
+    type: "area-chart"
+    y_label: "y_label"
+    weight: 1
+    max_value: 1
+  - title: "Super Chart A2"
+    type: "area-chart"
+    y_label: "y_label"
+    weight: 2
+    metrics:

spec/fixtures/lib/gitlab/metrics/dashboard/dashboard_panle_groups_wrong_content_type.yml

+dashboard: 'Test Dashboard'
+priority: 1
+links:
+- title: Link 1
+  url: https://gitlab.com
+- title: Link 2
+  url: https://docs.gitlab.com
+templating:
+  variables:
+    text_variable_full_syntax:
+      label: 'Variable 1'
+      type: text
+      options:
+        default_value: 'default'
+    text_variable_simple_syntax: 'default value'
+    custom_variable_simple_syntax: ['value1', 'value2', 'value3']
+    custom_variable_full_syntax:
+      label: 'Variable 2'
+      type: custom
+      options:
+        values:
+        - value: 'value option 1'
+          text: 'Option 1'
+        - value: 'value_option_2'
+          text: 'Option 2'
+          default: true
+    metric_label_values_variable:
+      label: 'Variable 3'
+      type: metric_label_values
+      options:
+        series_selector: 'backend:haproxy_backend_availability:ratio{env="{{env}}"}'
+        label: 'backend'
+panel_groups: this should be an array

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

@@ -4,28 +4,130 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Metrics::Dashboard::Validator::Errors do
   describe Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError do
-    context 'valid error hash from jsonschemer' do
+    context 'empty error hash' do
+      let(:error_hash) { {} }
+
+      it 'uses default error message' do
+        expect(described_class.new(error_hash).message).to eq('Dashboard failed schema validation')
+      end
+    end
+
+    context 'formatted message' do
+      subject { described_class.new(error_hash).message }
+
       let(:error_hash) do
         {
-          'data'            => 'data',
-          'data_pointer'    => 'data_pointer',
+          'data'         => 'property_name',
+          'data_pointer' => pointer,
+          'type'         => type,
           'schema'       => 'schema',
-          'schema_pointer'  => 'schema_pointer'
+          'details'      => details
         }
       end
 
-      it 'formats message' do
-        expect(described_class.new(error_hash).message).to eq(
-          "'data' is invalid at 'data_pointer'. Should be 'schema' due to schema definition at 'schema_pointer'"
-        )
+      context 'for root object' do
+        let(:pointer) { '' }
+
+        context 'when required keys are missing' do
+          let(:type) { 'required' }
+          let(:details) { { 'missing_keys' => ['one'] } }
+
+          it { is_expected.to eq 'root is missing required keys: one' }
         end
       end
 
-    context 'empty error hash' do
-      let(:error_hash) { {} }
+      context 'for nested object' do
+        let(:pointer) { '/nested_objects/0' }
 
-      it 'uses default error message' do
-        expect(described_class.new(error_hash).message).to eq('Dashboard failed schema validation')
+        context 'when required keys are missing' do
+          let(:type) { 'required' }
+          let(:details) { { 'missing_keys' => ['two'] } }
+
+          it { is_expected.to eq '/nested_objects/0 is missing required keys: two' }
+        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 /nested_objects/0 is not of type: #{expected_type}" }
+            end
+          end
+        end
+
+        context 'when data does not match pattern' do
+          let(:type) { 'pattern' }
+          let(:error_hash) do
+            {
+              'data'         => 'property_name',
+              'data_pointer' => pointer,
+              'type'         => type,
+              'schema'       => { 'pattern' => 'aa.*' }
+            }
+          end
+
+          it { is_expected.to eq "'property_name' at /nested_objects/0 does not match pattern: aa.*" }
+        end
+
+        context 'when data does not match format' do
+          let(:type) { 'format' }
+          let(:error_hash) do
+            {
+              'data'         => 'property_name',
+              'data_pointer' => pointer,
+              'type'         => type,
+              'schema'       => { 'format' => 'date-time' }
+            }
+          end
+
+          it { is_expected.to eq "'property_name' at /nested_objects/0 does not match format: date-time" }
+        end
+
+        context 'when data is not const' do
+          let(:type) { 'const' }
+          let(:error_hash) do
+            {
+              'data'         => 'property_name',
+              'data_pointer' => pointer,
+              'type'         => type,
+              'schema'       => { 'const' => 'one' }
+            }
+          end
+
+          it { is_expected.to eq "'property_name' at /nested_objects/0 is not: \"one\"" }
+        end
+
+        context 'when data is not included in enum' do
+          let(:type) { 'enum' }
+          let(:error_hash) do
+            {
+              'data'         => 'property_name',
+              'data_pointer' => pointer,
+              'type'         => type,
+              'schema'       => { 'enum' => %w(one two) }
+            }
+          end
+
+          it { is_expected.to eq "'property_name' at /nested_objects/0 is not one of: [\"one\", \"two\"]" }
+        end
+
+        context 'when data is not included in enum' do
+          let(:type) { 'unknown' }
+          let(:error_hash) do
+            {
+              'data'         => 'property_name',
+              'data_pointer' => pointer,
+              'type'         => type,
+              'schema'       => 'schema'
+            }
+          end
+
+          it { is_expected.to eq "'property_name' at /nested_objects/0 is invalid: error_type=unknown" }
+        end
       end
     end
   end

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

@@ -34,6 +34,15 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
   end
 
   describe '#validate!' do
+    shared_examples 'validation failed' do |errors_message|
+      it 'raises error with corresponding messages', :aggregate_failures do
+        expect { subject }.to raise_error do |error|
+          expect(error).to be_kind_of(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError)
+          expect(error.message).to eq(errors_message)
+        end
+      end
+    end
+
     context 'valid dashboard' do
       it 'returns true' do
         expect(described_class.validate!(valid_dashboard)).to be true
@@ -41,22 +50,37 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
     end
 
     context 'invalid dashboard' do
+      subject { described_class.validate!(invalid_dashboard) }
+
       context 'invalid schema' do
-        it 'raises error' do
-          expect { described_class.validate!(invalid_dashboard) }
-            .to raise_error(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError,
-              "'this_should_be_a_int' is invalid at '/panel_groups/0/panels/0/weight'."\
-              " Should be '{\"type\"=>\"number\"}' due to schema definition at '/properties/weight'")
+        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
+
+        context 'panel groups missing' do
+          let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_missing_panel_groups.yml')) }
+
+          it_behaves_like 'validation failed', 'root is missing required keys: panel_groups'
+        end
+
+        context 'groups are missing panels and group keys' do
+          let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_groups_missing_panels_and_group.yml')) }
+
+          it_behaves_like 'validation failed', '/panel_groups/0 is missing required keys: group'
+        end
+
+        context 'panel is missing metrics key' do
+          let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_panel_is_missing_metrics.yml')) }
+
+          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
-          it 'checks against given dashboard and returns false' do
-            expect { described_class.validate!(duplicate_id_dashboard) }
-              .to raise_error(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError,
-                "metric_id must be unique across a project")
-          end
+          subject { described_class.validate!(duplicate_id_dashboard) }
+
+          it_behaves_like 'validation failed', 'metric_id must be unique across a project'
         end
       end
     end