Update json schema validator to return meaningful error messages

Add specs for the change and update string translations

Fix docker executor opts spec with new validation messages

Changelog: changed
MR: !160236

app/validators/json_schema_validator.rb

@@ -30,10 +30,7 @@ def validate_each(record, attribute, value)
 
     if options[:detail_errors]
       validator.validate(value).each do |error|
-        message = format(
-          _("'%{data_pointer}' must be a valid '%{type}'"),
-          data_pointer: error['data_pointer'], type: error['type']
-        )
+        message = format_error_message(error)
         record.errors.add(attribute, message)
       end
     else
@@ -45,6 +42,32 @@ def validate_each(record, attribute, value)
 
   attr_reader :base_directory
 
+  def format_error_message(error)
+    case error['type']
+    when 'oneOf'
+      format_one_of_error(error)
+    else
+      error['error']
+    end
+  end
+
+  def format_one_of_error(error)
+    schema_options = error['schema']['oneOf']
+    required_props = schema_options.flat_map { |option| option['required'] }.uniq
+
+    message = if error['root_schema']['type'] == 'array'
+                _("value at %{data_pointer} should use only one of: %{requirements}")
+              else
+                _("should use only one of: %{requirements}")
+              end
+
+    format(
+      message,
+      requirements: required_props.join(', '),
+      data_pointer: error['data_pointer']
+    )
+  end
+
   def valid_schema?(value)
     validator.valid?(value)
   end

locale/gitlab.pot

@@ -1518,9 +1518,6 @@ msgstr ""
 msgid "%{wildcards_link_start}Wildcards%{wildcards_link_end} such as %{code_tag_start}v*%{code_tag_end} or %{code_tag_start}*-release%{code_tag_end} are supported."
 msgstr ""
-msgid "'%{data_pointer}' must be a valid '%{type}'"
-msgstr ""
-
 msgid "'%{group_name}' has been scheduled for removal on %{removal_time}."
 msgstr ""
@@ -64300,6 +64297,9 @@ msgstr ""
 msgid "should have length between 16 to 24 characters."
 msgstr ""
+msgid "should use only one of: %{requirements}"
+msgstr ""
+
 msgid "show %{count} more"
 msgstr ""
@@ -64479,6 +64479,9 @@ msgstr ""
 msgid "v%{version} published %{timeAgo}"
 msgstr ""
+msgid "value at %{data_pointer} should use only one of: %{requirements}"
+msgstr ""
+
 msgid "value for '%{storage}' must be an integer"
 msgstr ""

spec/lib/gitlab/ci/config/entry/image_spec.rb

@@ -136,7 +136,7 @@
           it 'raises an error' do
             expect(entry).not_to be_valid
             expect(entry.errors.first)
-              .to match %r{image executor opts '/docker/platform' must be a valid 'string'}
+              .to match %r{image executor opts value at `/docker/platform` is not a string}
           end
         end
       end
@@ -184,7 +184,7 @@
           it 'raises an error' do
             expect(entry).not_to be_valid
             expect(entry.errors.first)
-              .to match %r{image executor opts '/docker/user' must be a valid 'string'}
+              .to match %r{image executor opts value at `/docker/user` is not a string}
           end
         end
       end
@@ -194,8 +194,9 @@
 
         it 'is not valid' do
           expect(entry).not_to be_valid
-          expect(entry.errors.first)
-            .to match %r{image executor opts '/docker/unknown_key' must be a valid 'schema'}
+          expect(entry.errors.first).to match(
+            %r{image executor opts object property at `/docker/unknown_key` is a disallowed additional property}
+          )
         end
       end
     end

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -270,14 +270,34 @@
         using RSpec::Parameterized::TableSyntax
 
         where(:case_name, :config, :error) do
-          'when only step is used without name'   | { stage: 'build',
-run: [{ step: 'some reference' }] } | 'job run \'/0\' must be a valid \'required\''
-          'when only script is used without name' | { stage: 'build',
-run: [{ script: 'echo' }] } | 'job run \'/0\' must be a valid \'required\''
-          'when step and script are used together' | { stage: 'build',
-run: [{ name: 'step1', step: 'some reference', script: 'echo' }] } | 'job run \'/0\' must be a valid \'oneof\''
-          'when a subkey does not exist' | { stage: 'build',
-run: [{ name: 'step1', invalid_key: 'some value' }] } | 'job run \'/0\' must be a valid \'required\''
+          'when only step is used without name' | {
+            stage: 'build',
+            run: [{ step: 'some reference' }]
+          } | 'job run object at `/0` is missing required properties: name'
+
+          'when only script is used without name' | {
+            stage: 'build',
+            run: [{ script: 'echo' }]
+          } | 'job run object at `/0` is missing required properties: name'
+
+          'when step and script are used together' | {
+            stage: 'build',
+            run: [{
+              name: 'step1',
+              step: 'some reference',
+              script: 'echo'
+            }]
+          } | 'job run value at /0 should use only one of: step, script'
+
+          'when a required subkey is missing' | {
+            stage: 'build',
+            run: [{ name: 'step1' }]
+          } | 'job run object at `/0` is missing required properties: step'
+
+          'when a subkey is invalid' | {
+            stage: 'build',
+            run: [{ name: 'step1', step: 'some step', invalid_key: 'some value' }]
+          } | 'job run object property at `/0/invalid_key` is a disallowed additional property'
         end
 
         with_them do
@@ -292,7 +312,7 @@
 
           it 'returns error about invalid run' do
             expect(entry).not_to be_valid
-            expect(entry.errors).to include 'job run \'\' must be a valid \'array\''
+            expect(entry.errors).to include 'job run value at root is not an array'
           end
         end
 
@@ -312,7 +332,7 @@
 
           it 'returns error about invalid env' do
             expect(entry).not_to be_valid
-            expect(entry.errors).to include 'job run \'/0/env/my_var\' must be a valid \'string\''
+            expect(entry.errors).to include 'job run value at `/0/env/my_var` is not a string'
           end
         end
 
@@ -321,7 +341,7 @@
 
           it 'returns error about invalid run' do
             expect(entry).not_to be_valid
-            expect(entry.errors).to include 'job run \'/0\' must be a valid \'required\''
+            expect(entry.errors).to include 'job run object at `/0` is missing required properties: step'
           end
         end
       end

spec/lib/gitlab/ci/config/entry/service_spec.rb

@@ -204,7 +204,7 @@
         it 'is not valid' do
           expect(entry).not_to be_valid
           expect(entry.errors.first)
-            .to match %r{service executor opts '/docker/invalid' must be a valid 'schema'}
+            .to match %r{service executor opts object property at `/docker/invalid` is a disallowed additional property}
         end
       end
     end
@@ -216,7 +216,7 @@
         it 'is not valid' do
           expect(entry).not_to be_valid
           expect(entry.errors.first)
-            .to match %r{service executor opts '/docker/platform' must be a valid 'string'}
+            .to match %r{service executor opts value at `/docker/platform` is not a string}
         end
       end
     end

spec/services/ci/create_pipeline_service/run_spec.rb

@@ -137,7 +137,7 @@
     it 'returns errors for invalid configuration' do
       expect(pipeline).not_to be_created_successfully
       expect(pipeline.errors.full_messages).to include(
-        "jobs:job run '/0' must be a valid 'required'"
+        "jobs:job run object at `/0` is missing required properties: name"
       )
     end
   end

spec/validators/json_schema_validator_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe JsonSchemaValidator do
+RSpec.describe JsonSchemaValidator, feature_category: :shared do
   describe '#validates_each' do
     let(:build_report_result) { build(:ci_build_report_result, :with_junit_success) }
 
@@ -78,10 +78,140 @@
 
           expect(build_report_result.errors.size).to eq(1)
           expect(build_report_result.errors.full_messages).to match_array(
-            ["Data '/invalid' must be a valid 'schema'"]
+            ["Data object property at `/invalid` is a disallowed additional property"]
           )
         end
       end
     end
+
+    context 'when validating config with oneOf JSON schema' do
+      let(:config) do
+        {
+          run: [
+            {
+              name: 'hello_steps',
+              step: 'gitlab.com/gitlab-org/ci-cd/runner-tools/echo-step',
+              inputs: {
+                echo: 'hello steps!'
+              }
+            }
+          ]
+        }
+      end
+
+      let(:job) { Gitlab::Ci::Config::Entry::Job.new(config, name: :rspec) }
+      let(:errors) { ActiveModel::Errors.new(job) }
+
+      let(:validator) do
+        described_class.new(
+          attributes: [:run],
+          base_directory: 'app/validators/json_schemas',
+          filename: 'run_steps',
+          hash_conversion: true,
+          detail_errors: true
+        )
+      end
+
+      before do
+        job.compose!
+        allow(job).to receive(:errors).and_return(errors)
+      end
+
+      subject { validator.validate(job) }
+
+      context 'when the value is a valid array of hashes' do
+        before do
+          allow(job).to receive(:read_attribute_for_validation).and_return(config[:run])
+        end
+
+        it 'returns no errors' do
+          subject
+
+          expect(job.errors).to be_empty
+        end
+      end
+
+      context 'when a required property is missing' do
+        before do
+          config[:run] = [{ name: 'hello_steps' }]
+          allow(job).to receive(:read_attribute_for_validation).and_return(config[:run])
+        end
+
+        it 'returns an error message' do
+          subject
+
+          expect(job.errors).not_to be_empty
+          expect("#{job.errors.first.attribute} #{job.errors.first.type}").to eq("run object at `/0` is missing required properties: step")
+        end
+      end
+
+      context 'when oneOf validation fails' do
+        before do
+          config[:run] = [nil]
+          allow(job).to receive(:read_attribute_for_validation).and_return(config[:run])
+        end
+
+        it 'returns an error message' do
+          subject
+
+          expect(job.errors).not_to be_empty
+          expect("#{job.errors.first.attribute} #{job.errors.first.type}").to eq(
+            "run value at /0 should use only one of: step, script"
+          )
+        end
+      end
+
+      context 'when there is a general validation error' do
+        before do
+          config[:run] = 'not an array'
+          allow(job).to receive(:read_attribute_for_validation).and_return(config[:run])
+        end
+
+        it 'returns an error message' do
+          subject
+
+          expect(job.errors).not_to be_empty
+          expect("#{job.errors.first.attribute} #{job.errors.first.type}").to eq("run value at root is not an array")
+        end
+      end
+
+      context 'when a non-array value violates oneOf constraint' do
+        let(:schema) do
+          {
+            "type" => "object",
+            "properties" => {
+              "run" => {
+                "oneOf" => [
+                  { required: ["step"], title: "step" },
+                  { required: ["script"], title: "script" }
+                ]
+              }
+            }
+          }
+        end
+
+        let(:validator) do
+          described_class.new(
+            attributes: [:run],
+            filename: 'test_schema',
+            detail_errors: true
+          )
+        end
+
+        before do
+          config[:run] = 'C'
+          allow(job).to receive(:read_attribute_for_validation).and_return({ run: config[:run] })
+          allow(JSONSchemer).to receive(:schema).and_return(JSONSchemer.schema(schema))
+          allow(File).to receive(:read).with(anything).and_return(schema.to_json)
+        end
+
+        it 'returns an error message for oneOf violation without data pointer' do
+          subject
+
+          expect(job.errors).not_to be_empty
+          expect("#{job.errors.first.attribute} #{job.errors.first.type}").to eq("run should use only one of: step, script")
+        end
+      end
+    end
   end
 end