From 547a1c09539b9af381988b4abb458052c639e9d9 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Sat, 8 Feb 2025 15:47:26 -0800 Subject: [PATCH] Refactor: move where we call `field.coerce_result`. Previously, we called it from `GarphQLAdapter#call`, applying it to all resolved fields. However, we only need to use it in the aggregation `GroupedBy` resolver, where we get `DayOfWeek` enum values that we may need to coerce. There are no other cases where it is needed. This allows us to simplify `GarphQLAdapter#call` so that it now only does two things: * Identifies the resolver to dispatch to. * Dispatches to that resolver. This will allow us to further optimize by leveraging built-in functionality of the GraphQL gem to have it dispatch to the appropriate resolver in a more performant manner. --- .../aggregation/resolvers/grouped_by.rb | 8 ++++- .../graphql/resolvers/graphql_adapter.rb | 31 +++---------------- .../graphql/query_executor_spec.rb | 4 +-- 3 files changed, 14 insertions(+), 29 deletions(-) diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/grouped_by.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/grouped_by.rb index 3325ba8f..eae16515 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/grouped_by.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/grouped_by.rb @@ -24,7 +24,13 @@ def call(parent_type, graphql_field, object, args, context) return with(field_path: new_field_path) if field.type.object? bucket_entry = Support::HashUtil.verbose_fetch(bucket, "key") - Support::HashUtil.verbose_fetch(bucket_entry, FieldPathEncoder.encode(new_field_path.map(&:name_in_graphql_query))) + result = Support::HashUtil.verbose_fetch(bucket_entry, FieldPathEncoder.encode(new_field_path.map(&:name_in_graphql_query))) + + # Give the field a chance to coerce the result before returning it. Initially, this is only used to deal with + # enum value overrides (e.g. so that if `DayOfWeek.MONDAY` has been overridden to `DayOfWeek.MON`, we can coerce + # a `MONDAY` value being returned by a painless script to `MON`), but this is designed to be general purpose + # and we may use it for other coercions in the future. + field.coerce_result(result) end end end diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/graphql_adapter.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/graphql_adapter.rb index 27fd7313..e3d1c64c 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/graphql_adapter.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/graphql_adapter.rb @@ -28,33 +28,11 @@ def initialize(schema:, runtime_metadata:, resolvers:) # See https://graphql-ruby.org/api-doc/1.9.6/GraphQL/Schema.html#from_definition-class_method # (specifically, the `default_resolve` argument) for the API documentation. def call(parent_type, field, object, args, context) - schema_field = @schema.field_named(parent_type.graphql_name, field.name) - - resolver = resolver_for(schema_field, object) do - raise <<~ERROR - No resolver yet implemented for this case. - - parent_type: #{schema_field.parent_type} - - field: #{schema_field} - - obj: #{object.inspect} - - args: #{args.inspect} - - ctx: #{context.inspect} - ERROR + resolver = resolver_for(parent_type, field, object) do + raise "No resolver yet implemented for `#{parent_type.graphql_name}.#{field.name}`." end - result = resolver.call(parent_type, field, object, args, context) - - # Give the field a chance to coerce the result before returning it. Initially, this is only used to deal with - # enum value overrides (e.g. so that if `DayOfWeek.MONDAY` has been overridden to `DayOfWeek.MON`, we can coerce - # a `MONDAY` value being returned by a painless script to `MON`), but this is designed to be general purpose - # and we may use it for other coercions in the future. - # - # Note that coercion of scalar values is handled by the `coerce_result` callback below. - schema_field.coerce_result(result) + resolver.call(parent_type, field, object, args, context) end # In order to support unions and interfaces, we must implement `resolve_type`. @@ -85,7 +63,8 @@ def scalar_coercion_adapter_for(type) @coercion_adapters_by_scalar_type_name[type.graphql_name] end - def resolver_for(field, object) + def resolver_for(parent_type, graphql_field, object) + field = @schema.field_named(parent_type.graphql_name, graphql_field.name) return object if object.respond_to?(:can_resolve?) && object.can_resolve?(field: field, object: object) resolver = @resolvers.find { |r| r.can_resolve?(field: field, object: object) } resolver || yield diff --git a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb index dcf69a08..8a378390 100644 --- a/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb +++ b/elasticgraph-graphql/spec/unit/elastic_graph/graphql/query_executor_spec.rb @@ -189,11 +189,11 @@ class GraphQL expect { execute_expecting_no_errors(query_string) - }.to raise_error(a_string_including("No resolver yet implemented for this case")) + }.to raise_error(a_string_including("No resolver yet implemented for `Query.foo`.")) .and log a_string_including( "Query Foo[1] for client (anonymous) failed with an exception[2]", query_string.to_s, - "RuntimeError: No resolver yet implemented for this case" + "RuntimeError: No resolver yet implemented for `Query.foo`." ) end