From f36017c9177ef7b57a6700584361fce1e869d0b9 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 12 Oct 2023 15:47:08 -0400 Subject: [PATCH 1/4] Add a test for getting legacy tracers from the base class --- spec/graphql/tracing/trace_modes_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/graphql/tracing/trace_modes_spec.rb b/spec/graphql/tracing/trace_modes_spec.rb index 27f0f2a98a..906d01db07 100644 --- a/spec/graphql/tracing/trace_modes_spec.rb +++ b/spec/graphql/tracing/trace_modes_spec.rb @@ -110,6 +110,23 @@ def execute_query(query:) assert_equal :was_configured, res.context[:configured_option] end + describe "inheriting from GraphQL::Schema" do + it "gets CallLegacyTracers" do + base_class = Class.new(GraphQL::Schema) + child_class = Class.new(base_class) + + # Initialize the trace classes, make sure no legacy tracers are present at this point: + refute_includes base_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + refute_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + + # add a legacy tracer + base_class.tracer(Class.new) + assert_includes base_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + # The child class will also run inherited legacy tracers: + assert_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + end + end + describe "custom default trace mode" do class CustomDefaultSchema < TraceModesTest::ParentSchema From 9e80a48fca77f2fb00c10b9ce5d7293dc85c36cc Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 12 Oct 2023 15:55:10 -0400 Subject: [PATCH 2/4] Update spec to use GraphQL::Schema directly --- spec/graphql/tracing/trace_modes_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/graphql/tracing/trace_modes_spec.rb b/spec/graphql/tracing/trace_modes_spec.rb index 906d01db07..cfa89bb596 100644 --- a/spec/graphql/tracing/trace_modes_spec.rb +++ b/spec/graphql/tracing/trace_modes_spec.rb @@ -112,17 +112,17 @@ def execute_query(query:) describe "inheriting from GraphQL::Schema" do it "gets CallLegacyTracers" do - base_class = Class.new(GraphQL::Schema) - child_class = Class.new(base_class) + child_class = Class.new(GraphQL::Schema) - # Initialize the trace classes, make sure no legacy tracers are present at this point: - refute_includes base_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + # Initialize the trace class, make sure no legacy tracers are present at this point: refute_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers # add a legacy tracer - base_class.tracer(Class.new) - assert_includes base_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers - # The child class will also run inherited legacy tracers: + GraphQL::Schema.tracer(Class.new) + # A newly created child class gets the right setup: + new_child_class = Class.new(GraphQL::Schema) + assert_includes new_child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + # But what about an already-created child class? assert_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers end end From ef53f448d49d693e05fda801645370e14adc6d5a Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 12 Oct 2023 16:07:52 -0400 Subject: [PATCH 3/4] Build default trace modes as late as possible --- lib/graphql/schema.rb | 5 ++--- spec/graphql/tracing/trace_modes_spec.rb | 7 +++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/graphql/schema.rb b/lib/graphql/schema.rb index 0a56d9e373..c1fa498506 100644 --- a/lib/graphql/schema.rb +++ b/lib/graphql/schema.rb @@ -170,9 +170,9 @@ def trace_class(new_class = nil) end # @return [Class] Return the trace class to use for this mode, looking one up on the superclass if this Schema doesn't have one defined. - def trace_class_for(mode, build: false) + def trace_class_for(mode) own_trace_modes[mode] || - (superclass.respond_to?(:trace_class_for) ? superclass.trace_class_for(mode) : nil) + (superclass.respond_to?(:trace_class_for) ? superclass.trace_class_for(mode) : (own_trace_modes[mode] = build_trace_mode(mode))) end # Configure `trace_class` to be used whenever `context: { trace_mode: mode_name }` is requested. @@ -921,7 +921,6 @@ def resolve_type(type, obj, ctx) def inherited(child_class) if self == GraphQL::Schema - child_class.own_trace_modes[:default] = child_class.build_trace_mode(:default) child_class.directives(default_directives.values) end # Make sure the child class has these built out, so that diff --git a/spec/graphql/tracing/trace_modes_spec.rb b/spec/graphql/tracing/trace_modes_spec.rb index cfa89bb596..912c24b366 100644 --- a/spec/graphql/tracing/trace_modes_spec.rb +++ b/spec/graphql/tracing/trace_modes_spec.rb @@ -116,14 +116,17 @@ def execute_query(query:) # Initialize the trace class, make sure no legacy tracers are present at this point: refute_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers - + tracer_class = Class.new # add a legacy tracer - GraphQL::Schema.tracer(Class.new) + GraphQL::Schema.tracer(tracer_class) # A newly created child class gets the right setup: new_child_class = Class.new(GraphQL::Schema) assert_includes new_child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers # But what about an already-created child class? assert_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers + ensure + GraphQL::Schema.send(:own_tracers).delete(tracer_class) + GraphQL::Schema.own_trace_modes.delete(:default) end end From 1065dc89a4e7f6107d74103a42aa0070cfb9e110 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 12 Oct 2023 16:34:09 -0400 Subject: [PATCH 4/4] Fix test clean-up code --- spec/graphql/tracing/legacy_trace_spec.rb | 6 +++--- spec/graphql/tracing/trace_modes_spec.rb | 12 ++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/spec/graphql/tracing/legacy_trace_spec.rb b/spec/graphql/tracing/legacy_trace_spec.rb index 874f18feea..6295efe3ba 100644 --- a/spec/graphql/tracing/legacy_trace_spec.rb +++ b/spec/graphql/tracing/legacy_trace_spec.rb @@ -48,6 +48,7 @@ def self.trace(key, data) custom_trace_module = Module.new do def execute_query(query:) query.context[:trace_module_ran] = true + super end end @@ -69,13 +70,12 @@ def int 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] + assert_equal true, res2.context[:trace_module_ran], "New Trace Ran" + assert_equal true, res2.context[:tracer_ran], "Legacy Trace Ran" end end diff --git a/spec/graphql/tracing/trace_modes_spec.rb b/spec/graphql/tracing/trace_modes_spec.rb index 912c24b366..3c774824f5 100644 --- a/spec/graphql/tracing/trace_modes_spec.rb +++ b/spec/graphql/tracing/trace_modes_spec.rb @@ -112,11 +112,17 @@ def execute_query(query:) describe "inheriting from GraphQL::Schema" do it "gets CallLegacyTracers" do + # Use a new base trace mode class to avoid polluting the base class + # which already-initialized schemas have in their inheritance chain + # (It causes `CallLegacyTracers` to end up in the chain twice otherwise) + GraphQL::Schema.own_trace_modes[:default] = GraphQL::Schema.build_trace_mode(:default) + child_class = Class.new(GraphQL::Schema) # Initialize the trace class, make sure no legacy tracers are present at this point: refute_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers tracer_class = Class.new + # add a legacy tracer GraphQL::Schema.tracer(tracer_class) # A newly created child class gets the right setup: @@ -124,9 +130,11 @@ def execute_query(query:) assert_includes new_child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers # But what about an already-created child class? assert_includes child_class.trace_class_for(:default).ancestors, GraphQL::Tracing::CallLegacyTracers - ensure + + # Reset GraphQL::Schema tracer state: GraphQL::Schema.send(:own_tracers).delete(tracer_class) - GraphQL::Schema.own_trace_modes.delete(:default) + GraphQL::Schema.own_trace_modes[:default] = GraphQL::Schema.build_trace_mode(:default) + refute_includes GraphQL::Schema.new_trace.class.ancestors, GraphQL::Tracing::CallLegacyTracers end end