Merge pull request #4487 from rmosolgo/better-trace-compat

Fix tracer compatibility with inheritance

Author: rmosolgo

lib/graphql/query.rb

@@ -102,16 +102,16 @@ def initialize(schema, query_string = nil, query: nil, document: nil, context: n
 
       # Support `ctx[:backtrace] = true` for wrapping backtraces
       if context && context[:backtrace] && !@tracers.include?(GraphQL::Backtrace::Tracer)
-        if schema.trace_class <= GraphQL::Tracing::LegacyTrace
+        if schema.trace_class <= GraphQL::Tracing::CallLegacyTracers
           context_tracers += [GraphQL::Backtrace::Tracer]
           @tracers << GraphQL::Backtrace::Tracer
         elsif !(current_trace.class <= GraphQL::Backtrace::Trace)
           raise "Invariant: `backtrace: true` should have provided a trace class with Backtrace mixed in, but it didnt. (Found: #{current_trace.class.ancestors}). This is a bug in GraphQL-Ruby, please report it on GitHub."
         end
       end
 
-      if context_tracers.any? && !(schema.trace_class <= GraphQL::Tracing::LegacyTrace)
-        raise ArgumentError, "context[:tracers] are not supported without `trace_class(GraphQL::Tracing::LegacyTrace)` in the schema configuration, please add it."
+      if context_tracers.any? && !(schema.trace_class <= GraphQL::Tracing::CallLegacyTracers)
+        raise ArgumentError, "context[:tracers] are not supported without `trace_with(GraphQL::Tracing::CallLegacyTracers)` in the schema configuration, please add it."
       end
 
 

lib/graphql/schema.rb

@@ -961,10 +961,8 @@ def default_directives
       end
 
       def tracer(new_tracer)
-        if defined?(@trace_modes) && !(trace_class_for(:default) < GraphQL::Tracing::LegacyTrace)
-          raise ArgumentError, "Can't add tracer after configuring a `trace_class`, use GraphQL::Tracing::LegacyTrace to merge legacy tracers into a trace class instead."
-        else
-          trace_mode(:default, Class.new(GraphQL::Tracing::LegacyTrace))
+        if !(trace_class_for(:default) < GraphQL::Tracing::CallLegacyTracers)
+          trace_with(GraphQL::Tracing::CallLegacyTracers)
         end
 
         own_tracers << new_tracer

lib/graphql/tracing/legacy_trace.rb

@@ -4,7 +4,7 @@ module Tracing
     # This trace class calls legacy-style tracer with payload hashes.
     # New-style `trace_with` modules significantly reduce the overhead of tracing,
     # but that advantage is lost when legacy-style tracers are also used (since the payload hashes are still constructed).
-    class LegacyTrace < Trace
+    module CallLegacyTracers
       def lex(query_string:)
         (@multiplex || @query).trace("lex", { query_string: query_string }) { super }
       end
@@ -61,5 +61,9 @@ def resolve_type_lazy(query:, type:, object:)
         query.trace("resolve_type_lazy", { context: query.context, type: type, object: object, path: query.context[:current_path] }) { super }
       end
     end
+
+    class LegacyTrace < Trace
+      include CallLegacyTracers
+    end
   end
 end

spec/graphql/backtrace_spec.rb

@@ -212,7 +212,6 @@ def self.after_query(query)
     it "raises original exception instead of a TracedError when error does not occur during resolving" do
       instrumentation_schema = Class.new(schema) do
         instrument(:query, ErrorInstrumentation)
-        trace_class(GraphQL::Tracing::LegacyTrace)
       end
 
       assert_raises(RuntimeError) {

spec/graphql/schema_spec.rb

@@ -151,9 +151,9 @@ class CustomSubscriptions < GraphQL::Subscriptions::ActionCableSubscriptions
       assert_equal base_schema.multiplex_analyzers + [multiplex_analyzer], schema.multiplex_analyzers
       assert_equal [GraphQL::Backtrace, GraphQL::Subscriptions::ActionCableSubscriptions, CustomSubscriptions], schema.plugins.map(&:first)
       assert_equal [GraphQL::Tracing::DataDogTracing], base_schema.tracers
-      assert_includes base_schema.trace_class.ancestors, GraphQL::Tracing::LegacyTrace
+      assert_includes base_schema.trace_class.ancestors, GraphQL::Tracing::CallLegacyTracers
       assert_equal [GraphQL::Tracing::DataDogTracing, GraphQL::Tracing::NewRelicTracing], schema.tracers
-      assert_includes schema.trace_class.ancestors, GraphQL::Tracing::LegacyTrace
+      assert_includes schema.trace_class.ancestors, GraphQL::Tracing::CallLegacyTracers
 
 
       assert_instance_of CustomSubscriptions, schema.subscriptions

spec/graphql/tracing/legacy_trace_spec.rb

@@ -6,7 +6,7 @@
     custom_tracer = Module.new do
       def self.trace(key, data)
         if key == "execute_query"
-          data[:query].context[:trace_ran] = true
+          data[:query].context[:tracer_ran] = true
         end
         yield
       end
@@ -29,9 +29,53 @@ def int
 
 
     res1 = parent_schema.execute("{ int }")
-    assert_equal true, res1.context[:trace_ran]
+    assert_equal true, res1.context[:tracer_ran]
 
     res2 = child_schema.execute("{ int }")
-    assert_equal true, res2.context[:trace_ran]
+    assert_equal true, res2.context[:tracer_ran]
+  end
+
+  it "Works with new trace modules in the parent class" do
+    custom_tracer = Module.new do
+      def self.trace(key, data)
+        if key == "execute_query"
+          data[:query].context[:tracer_ran] = true
+        end
+        yield
+      end
+    end
+
+    custom_trace_module = Module.new do
+      def execute_query(query:)
+        query.context[:trace_module_ran] = true
+      end
+    end
+
+
+    query_type = Class.new(GraphQL::Schema::Object) do
+      graphql_name("Query")
+      field :int, Integer
+      def int
+        4
+      end
+    end
+
+    parent_schema = Class.new(GraphQL::Schema) do
+      query(query_type)
+      trace_with(custom_trace_module)
+    end
+
+    child_schema = Class.new(parent_schema) do
+      tracer(custom_tracer)
+    end
+
+
+    res1 = parent_schema.execute("{ int }")
+    assert_equal true, res1.context[:trace_module_ran]
+    assert_nil res1.context[:tracer_ran]
+
+    res2 = child_schema.execute("{ int }")
+    assert_equal true, res2.context[:trace_module_ran]
+    assert_equal true, res2.context[:tracer_ran]
   end
 end

spec/support/dummy/schema.rb

@@ -517,7 +517,7 @@ class Schema < GraphQL::Schema
     subscription Subscription
     max_depth 5
     orphan_types Honey, Beverage
-    trace_class GraphQL::Tracing::LegacyTrace
+    trace_with GraphQL::Tracing::CallLegacyTracers
 
     rescue_from(NoSuchDairyError) { |err| raise GraphQL::ExecutionError, err.message  }