Speed up submission result construction

Gemfile

@@ -37,7 +37,7 @@ gem 'image_processing', '~> 1.12.2'
 gem 'bootsnap', '~> 1.16.0', require: false
 
 # used to validate container responses
-gem 'json-schema', '~> 3.0.0'
+gem 'json_schemer', '~> 0.2.24'
 
 # delayed jobs
 gem 'delayed_job_active_record', '~> 4.1.7'

Gemfile.lock

@@ -160,6 +160,8 @@ GEM
     docker-api (2.2.0)
       excon (>= 0.47.0)
       multi_json
+    ecma-re-validator (0.4.0)
+      regexp_parser (~> 2.2)
     ed25519 (1.3.0)
     erubi (1.12.0)
     exception_notification (4.5.0)
@@ -185,6 +187,7 @@ GEM
     glob (0.4.0)
     globalid (1.1.0)
       activesupport (>= 5.0)
+    hana (1.3.7)
     has_scope (0.8.1)
       actionpack (>= 5.2)
       activesupport (>= 5.2)
@@ -220,8 +223,11 @@ GEM
       bindata
       faraday (~> 2.0)
       faraday-follow_redirects
-    json-schema (3.0.0)
-      addressable (>= 2.8)
+    json_schemer (0.2.24)
+      ecma-re-validator (~> 0.3)
+      hana (~> 1.3)
+      regexp_parser (~> 2.0)
+      uri_template (~> 0.7)
     jwt (2.7.0)
     kramdown (2.4.0)
       rexml
@@ -466,6 +472,7 @@ GEM
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
     unicode-display_width (2.4.2)
+    uri_template (0.7.0)
     validate_email (0.1.6)
       activemodel (>= 3.0)
       mail (>= 2.2.5)
@@ -536,7 +543,7 @@ DEPENDENCIES
   image_processing (~> 1.12.2)
   jbuilder (~> 2.11.5)
   jsbundling-rails (~> 1.1.1)
-  json-schema (~> 3.0.0)
+  json_schemer (~> 0.2.24)
   jwt (~> 2.7.0)
   kramdown (~> 2.4.0)
   kramdown-parser-gfm (~> 1.1.0)

app/runners/result_constructor.rb

@@ -1,5 +1,5 @@
 require 'json' # JSON support
-require 'json-schema' # json schema validation, from json-schema gem
+require 'json_schemer' # json schema validation
 
 class ResultConstructorError < StandardError
   attr_accessor :title, :description
@@ -12,32 +12,56 @@ def initialize(title, description = nil)
 end
 
 class ResultConstructor
-  FULL_SCHEMA = JSON.parse(Rails.public_path.join('schemas/judge_output.json').read)
-  PART_SCHEMA = JSON.parse(Rails.public_path.join('schemas/partial_output.json').read)
+  FULL_SCHEMER = JSONSchemer.schema(Rails.public_path.join('schemas/judge_output.json'))
+  PART_SCHEMER = JSONSchemer.schema(Rails.public_path.join('schemas/partial_output.json'))
 
   LEVELSA = %i[judgement tab context testcase test].freeze
   LEVELSH = { judgement: 0, tab: 1, context: 2, testcase: 3, test: 4 }.freeze
   GATHER = { tab: :groups, context: :groups, testcase: :groups, test: :tests }.freeze
 
-  def initialize(locale)
+  def initialize(locale, full_check: false)
     @locale = locale
     @level = nil
     @result = {}
+    @full_check = full_check || Rails.configuration.slow_judge_results_validation
   end
 
   def feed(judge_output)
     raise ResultConstructorError, 'No judge output' if judge_output.empty?
 
-    split_jsons(judge_output).each do |json|
-      if JSON::Validator.validate(PART_SCHEMA, json)
-        update(json)
-      elsif JSON::Validator.validate(FULL_SCHEMA, json)
-        @result = json
-      else
-        raise ResultConstructorError.new(
-          'Judge output is not a valid json',
-          json.to_s
-        )
+    if @full_check
+      # In development, we validate each update object.
+      split_jsons(judge_output).each do |json|
+        if PART_SCHEMER.valid?(json.deep_stringify_keys)
+          update(json)
+        elsif FULL_SCHEMER.valid?(json.deep_stringify_keys)
+          @result = json
+        else
+          raise ResultConstructorError.new(
+            'Judge output is not a valid json',
+            json.to_s
+          )
+        end
+      end
+    else
+      # In production, we do one validation at the end (in result)
+      split_jsons(judge_output).each do |json|
+        if json.key?(:command)
+          begin
+            # Clone the object to have better errors, since the update method
+            # may modify the json hash.
+            update(json.clone)
+          rescue StandardError
+            # We rescue all errors, since the json may have invalid data, resulting
+            # in stuff like TypeErrors, NoMethodErrors or KeyErrors
+            raise ResultConstructorError.new(
+              'Judge output is not a valid json',
+              json.to_s
+            )
+          end
+        else
+          @result = json
+        end
       end
     end
   end
@@ -56,6 +80,14 @@ def result(timeout)
     close_tab(badgeCount: @tab[:badgeCount] || 1) if @level == :tab
     close_judgement(accepted: false, status: status) if @level == :judgement
 
+    # Before finalizing the result, check if it is valid.
+    unless FULL_SCHEMER.valid?(@result.deep_stringify_keys)
+      raise ResultConstructorError.new(
+        'Judge output is not a valid json',
+        @result.to_s
+      )
+    end
+
     @result
   end
 

config/application.rb

@@ -47,5 +47,8 @@ class Application < Rails::Application
     config.active_storage.queues.purge    = :default
 
     config.action_view.default_form_builder = "StandardFormBuilder"
+
+    # Should the slower but fuller validation of judge results happen
+    config.slow_judge_results_validation = true
   end
 end

config/environments/production.rb

@@ -145,4 +145,7 @@
   config.action_mailer.deliver_later_queue_name = 'default'
 
   config.submissions_storage_path = Rails.root.join('data', 'storage', 'submissions')
+
+  # Should the slower but fuller validation of judge results happen
+  config.slow_judge_results_validation = false
 end

public/schemas/judge_output.json

@@ -88,7 +88,7 @@
         "type":    { "$ref": "#/definitions/severity" },
         "rows":    { "$ref": "#/definitions/index" },
         "columns": { "$ref": "#/definitions/index" },
-        "externalUrl":    { "type": "string" }
+        "externalUrl":    { "type": ["string", "null"] }
       }
     },
     "test-format": {

test/runners/result_constructor_test.rb

@@ -3,13 +3,11 @@
 require 'result_constructor'
 
 class ResultConstructorTest < ActiveSupport::TestCase
-  MINIMAL_FULL_S =
-
-    test 'empty output should fail' do
-      assert_raises ResultConstructorError do
-        construct_result([''])
-      end
+  test 'empty output should fail' do
+    assert_raises ResultConstructorError do
+      construct_result([''])
     end
+  end
 
   test 'empty json should fail' do
     assert_raises ResultConstructorError do
@@ -46,6 +44,41 @@ class ResultConstructorTest < ActiveSupport::TestCase
     end
   end
 
+  test 'intentionally annoyingly wrong json should fail' do
+    assert_raises ResultConstructorError do
+      construct_result([
+        # This command is invalid
+        '{ "command": "start-nothing" }',
+        '{ "title": "Test", "command": "start-tab" }',
+        '{ "description": { "format": "code", "description": "..." }, "command": "start-context" }',
+        '{ "description": { "format": "plain", "description": "" }, "command": "start-testcase" }',
+        '{ "expected": "70", "command": "start-test" }'
+      ])
+    end
+
+    assert_raises ResultConstructorError do
+      construct_result([
+        # There is a type in "command"
+        '{ "commund": "start-judgement" }',
+        '{ "title": "Test", "command": "start-tab" }',
+        '{ "description": { "format": "code", "description": "..." }, "command": "start-context" }',
+        '{ "description": { "format": "plain", "description": "" }, "command": "start-testcase" }',
+        '{ "expected": "70", "command": "start-test" }'
+      ])
+    end
+
+    assert_raises ResultConstructorError do
+      construct_result([
+        '{ "command": "start-judgement" }',
+        '{ "title": "Test", "command": "start-tab" }',
+        # A nested object is invalid: a description is a list
+        '{ "description": ["yes"], "command": "start-context" }',
+        '{ "description": { "format": "plain", "description": "" }, "command": "start-testcase" }',
+        '{ "expected": "70", "command": "start-test" }'
+      ])
+    end
+  end
+
   test 'partial output should accumulated status' do
     assert_equal({
       accepted: false,
@@ -339,11 +372,23 @@ class ResultConstructorTest < ActiveSupport::TestCase
     end
   end
 
+  protected
+
+  def create_rc(locale: 'en')
+    ResultConstructor.new(locale)
+  end
+
   private
 
   def construct_result(food, locale: 'en', timeout: false)
-    rc = ResultConstructor.new(locale)
+    rc = create_rc(locale: locale)
     food.each { |f| rc.feed f }
     rc.result(timeout)
   end
 end
+
+class CheckingResultConstructorTest < ResultConstructorTest
+  def create_rc(locale: 'en')
+    ResultConstructor.new(locale, full_check: true)
+  end
+end