4 files changed, 72 insertions, 23 deletions

diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb
index ef258bf07cb..0a1cf169aca 100644
--- a/app/controllers/graphql_controller.rb
+++ b/app/controllers/graphql_controller.rb
@@ -5,7 +5,7 @@ class GraphqlController < ApplicationController
   before_action :check_graphql_feature_flag!
 
   def execute
-    variables = ensure_hash(params[:variables])
+    variables = Gitlab::Graphql::Variables.new(params[:variables]).to_h
     query = params[:query]
     operation_name = params[:operationName]
     context = {
@@ -15,35 +15,31 @@ class GraphqlController < ApplicationController
     render json: result
   end
 
+  rescue_from StandardError do |exception|
+    log_exception(exception)
+
+    render_error("Internal server error")
+  end
+
+  rescue_from Gitlab::Graphql::Variables::Invalid do |exception|
+    render_error(exception.message, status: :unprocessable_entity)
+  end
+
   private
 
   # Overridden from the ApplicationController to make the response look like
   # a GraphQL response. That is nicely picked up in Graphiql.
   def render_404
-    error = { errors: [ message: "Not found" ] }
+    render_error("Not found!", status: :not_found)
+  end
+
+  def render_error(message, status: 500)
+    error = { errors: [message: message] }
 
-    render json: error, status: :not_found
+    render json: error, status: status
   end
 
   def check_graphql_feature_flag!
     render_404 unless Feature.enabled?(:graphql)
   end
-
-  # Handle form data, JSON body, or a blank value
-  def ensure_hash(ambiguous_param)
-    case ambiguous_param
-    when String
-      if ambiguous_param.present?
-        ensure_hash(JSON.parse(ambiguous_param))
-      else
-        {}
-      end
-    when Hash, ActionController::Parameters
-      ambiguous_param
-    when nil
-      {}
-    else
-      raise ArgumentError, "Unexpected parameter: #{ambiguous_param}"
-    end
-  end
 end
diff --git a/lib/gitlab/graphql.rb b/lib/gitlab/graphql.rb
new file mode 100644
index 00000000000..04a89432230
--- /dev/null
+++ b/lib/gitlab/graphql.rb
@@ -0,0 +1,5 @@
+module Gitlab
+  module Graphql
+    StandardGraphqlError = Class.new(StandardError)
+  end
+end
diff --git a/lib/gitlab/graphql/variables.rb b/lib/gitlab/graphql/variables.rb
new file mode 100644
index 00000000000..ffbaf65b512
--- /dev/null
+++ b/lib/gitlab/graphql/variables.rb
@@ -0,0 +1,37 @@
+module Gitlab
+  module Graphql
+    class Variables
+      Invalid = Class.new(Gitlab::Graphql::StandardGraphqlError)
+
+      def initialize(param)
+        @param = param
+      end
+
+      def to_h
+        ensure_hash(@param)
+      end
+
+      private
+
+      # Handle form data, JSON body, or a blank value
+      def ensure_hash(ambiguous_param)
+        case ambiguous_param
+        when String
+          if ambiguous_param.present?
+            ensure_hash(JSON.parse(ambiguous_param))
+          else
+            {}
+          end
+        when Hash, ActionController::Parameters
+          ambiguous_param
+        when nil
+          {}
+        else
+          raise Invalid, "Unexpected parameter: #{ambiguous_param}"
+        end
+      rescue JSON::ParserError => e
+        raise Invalid.new(e)
+      end
+    end
+  end
+end
diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb
index d6689dbc3c6..1449036e148 100644
--- a/spec/controllers/graphql_controller_spec.rb
+++ b/spec/controllers/graphql_controller_spec.rb
@@ -2,6 +2,8 @@ require 'spec_helper'
 
 describe GraphqlController do
   describe 'execute' do
+    let(:user) { nil }
+
     before do
       sign_in(user) if user
 
@@ -39,17 +41,26 @@ describe GraphqlController do
         is_expected.to eq('echo' => '"Simon" says: test success')
       end
     end
+
+    context 'invalid variables' do
+      it 'returns an error' do
+        run_test_query!(variables: "This is not JSON")
+
+        expect(response).to have_gitlab_http_status(422)
+        expect(json_response['errors'].first['message']).not_to be_nil
+      end
+    end
   end
 
   # Chosen to exercise all the moving parts in GraphqlController#execute
-  def run_test_query!
+  def run_test_query!(variables: { 'text' => 'test success' })
     query = <<~QUERY
       query Echo($text: String) {
         echo(text: $text)
       }
     QUERY
 
-    post :execute, query: query, operationName: 'Echo', variables: { 'text' => 'test success' }
+    post :execute, query: query, operationName: 'Echo', variables: variables
   end
 
   def query_response