From 758e7d68e632bb27eb16ffcd12a35c1bc016ceb7 Mon Sep 17 00:00:00 2001
From: Paul Chavard <pro@paul.chavard.net>
Date: Thu, 11 Nov 2021 10:31:01 +0100
Subject: [PATCH] fix(graphql): fix and improuve query parsing for logs

---
 app/controllers/api/v2/graphql_controller.rb  | 37 +----------
 .../concerns/graphql_operation_log_concern.rb | 62 +++++++++++++++++++
 .../graphql_operation_log_concern_spec.rb     | 59 ++++++++++++++++++
 3 files changed, 124 insertions(+), 34 deletions(-)
 create mode 100644 app/controllers/concerns/graphql_operation_log_concern.rb
 create mode 100644 spec/controllers/concerns/graphql_operation_log_concern_spec.rb

diff --git a/app/controllers/api/v2/graphql_controller.rb b/app/controllers/api/v2/graphql_controller.rb
index 4e9802b3a..11a5ce654 100644
--- a/app/controllers/api/v2/graphql_controller.rb
+++ b/app/controllers/api/v2/graphql_controller.rb
@@ -1,4 +1,6 @@
 class API::V2::GraphqlController < API::V2::BaseController
+  include GraphqlOperationLogConcern
+
   def execute
     variables = ensure_hash(params[:variables])
 
@@ -24,43 +26,10 @@ class API::V2::GraphqlController < API::V2::BaseController
     super
 
     payload.merge!({
-      graphql_operation: operation_log(params[:query], params[:operationName], params[:variables])
+      graphql_operation: operation_log(params[:query], params[:operationName], params[:variables]&.to_unsafe_h)
     })
   end
 
-  def operation_log(query, operation_name, variables)
-    return "NoQuery" if query.nil?
-
-    operation = GraphQL.parse(query).children.find do |node|
-      if node.is_a?(GraphQL::Language::Nodes::OperationDefinition)
-        node.name == operation_name
-      end
-    end
-
-    return "InvalidQuery" if operation.nil?
-    return "IntrospectionQuery" if operation.name == "IntrospectionQuery"
-
-    message = operation.operation_type
-    if operation.name
-      message += ": #{operation.name} { "
-    end
-    message += operation.selections.map(&:name).join(', ')
-    message += " }"
-    if variables.present?
-      message += " "
-      message += variables.to_unsafe_h.flat_map do |(name, value)|
-        if name == "input"
-          value.map do |(name, value)|
-            "#{name}: \"#{value.to_s.truncate(10)}\""
-          end
-        else
-          "#{name}: \"#{value.to_s.truncate(10)}\""
-        end
-      end.join(', ')
-    end
-    message
-  end
-
   def process_action(*args)
     super
   rescue ActionDispatch::Http::Parameters::ParseError => exception
diff --git a/app/controllers/concerns/graphql_operation_log_concern.rb b/app/controllers/concerns/graphql_operation_log_concern.rb
new file mode 100644
index 000000000..86650bfa0
--- /dev/null
+++ b/app/controllers/concerns/graphql_operation_log_concern.rb
@@ -0,0 +1,62 @@
+module GraphqlOperationLogConcern
+  extend ActiveSupport::Concern
+
+  # This method parses GraphQL query and creates a short description of the query. It is useful for logging.
+  def operation_log(query, operation_name, variables)
+    return "NoQuery" if query.nil?
+
+    operation = parse_graphql_query(query, operation_name)
+
+    return "InvalidQuery" if operation.nil?
+    return "IntrospectionQuery" if operation.name == "IntrospectionQuery"
+
+    message = "#{operation.operation_type}: "
+    message += if operation.name.present?
+      "#{operation.name} { "
+    else
+      "{ "
+    end
+    message += operation.selections.map(&:name).join(', ')
+    message += " } "
+    message += if variables.present?
+      variables.flat_map do |(name, value)|
+        format_graphql_variable(name, value)
+      end
+    else
+      operation.selections.flat_map(&:arguments).flat_map do |argument|
+        format_graphql_variable(argument.name, argument.value)
+      end
+    end.join(', ')
+
+    message.strip
+  end
+
+  private
+
+  def parse_graphql_query(query, operation_name)
+    operations = GraphQL.parse(query).children.filter do |node|
+      node.is_a?(GraphQL::Language::Nodes::OperationDefinition)
+    end
+    if operations.size == 1
+      operations.first
+    else
+      operations.find { |node| node.name == operation_name }
+    end
+  rescue
+    nil
+  end
+
+  def format_graphql_variable(name, value)
+    if value.is_a?(Hash)
+      value.map do |(name, value)|
+        format_graphql_variable(name, value)
+      end
+    elsif value.is_a?(GraphQL::Language::Nodes::InputObject)
+      value.arguments.map do |argument|
+        format_graphql_variable(argument.name, argument.value)
+      end
+    else
+      "#{name}: \"#{value.to_s.truncate(10)}\""
+    end
+  end
+end
diff --git a/spec/controllers/concerns/graphql_operation_log_concern_spec.rb b/spec/controllers/concerns/graphql_operation_log_concern_spec.rb
new file mode 100644
index 000000000..074a6eaf6
--- /dev/null
+++ b/spec/controllers/concerns/graphql_operation_log_concern_spec.rb
@@ -0,0 +1,59 @@
+RSpec.describe GraphqlOperationLogConcern, type: :controller do
+  class TestController < ActionController::Base
+    include GraphqlOperationLogConcern
+  end
+
+  controller TestController do
+  end
+
+  describe '#operation_log' do
+    let(:query) { nil }
+    let(:variables) { nil }
+    let(:operation_name) { nil }
+
+    subject { controller.operation_log(query, operation_name, variables) }
+
+    context 'with no query' do
+      it { expect(subject).to eq('NoQuery') }
+    end
+
+    context 'with invalid query' do
+      let(:query) { 'query { demarche {} }' }
+
+      it { expect(subject).to eq('InvalidQuery') }
+    end
+
+    context 'with two queries' do
+      let(:query) { 'query demarche { demarche } query dossier { dossier }' }
+      let(:operation_name) { 'dossier' }
+
+      it { expect(subject).to eq('query: dossier { dossier }') }
+    end
+
+    context 'with arguments' do
+      let(:query) { 'query demarche { demarche(number: 123) { id } }' }
+
+      it { expect(subject).to eq('query: demarche { demarche } number: "123"') }
+    end
+
+    context 'with variables' do
+      let(:query) { 'query { demarche(number: 123) { id } }' }
+      let(:variables) { { number: 124 } }
+
+      it { expect(subject).to eq('query: { demarche } number: "124"') }
+    end
+
+    context 'with mutation and arguments' do
+      let(:query) { 'mutation { passerDossierEnInstruction(input: { number: 123 }) { id } }' }
+
+      it { expect(subject).to eq('mutation: { passerDossierEnInstruction } number: "123"') }
+    end
+
+    context 'with mutation and variables' do
+      let(:query) { 'mutation { passerDossierEnInstruction(input: { number: 123 }) { id } }' }
+      let(:variables) { { input: { number: 124 } } }
+
+      it { expect(subject).to eq('mutation: { passerDossierEnInstruction } number: "124"') }
+    end
+  end
+end