Skip to content

Resolve elasticgraph-apollo confusing rake warnings #1060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ayousufi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Resolve elasticgraph-apollo confusing rake warnings #1060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c89c0d7
Resolve elasticgraph-apollo confusing rake warnings
ayousufi Mar 1, 2026
bbf97e0
Support built_in resolver param
ayousufi Mar 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,15 +382,15 @@ def define_apollo_schema_elements
register_graphql_extension GraphQL::EngineExtension, defined_at: require_path

require(require_path = "elastic_graph/apollo/graphql/entities_field_resolver")
register_graphql_resolver :apollo_entities, GraphQL::EntitiesFieldResolver, defined_at: require_path
register_graphql_resolver :apollo_entities, GraphQL::EntitiesFieldResolver, defined_at: require_path, built_in: true

require(require_path = "elastic_graph/apollo/graphql/service_field_resolver")
register_graphql_resolver :apollo_service, GraphQL::ServiceFieldResolver, defined_at: require_path
register_graphql_resolver :apollo_service, GraphQL::ServiceFieldResolver, defined_at: require_path, built_in: true

require(require_path = "elastic_graph/apollo/graphql/apollo_entity_ref_resolver")
register_graphql_resolver :apollo_entity_ref, GraphQL::ApolloEntityRefResolver::ForSingleId, defined_at: require_path
register_graphql_resolver :apollo_entity_ref_list, GraphQL::ApolloEntityRefResolver::ForIdList, defined_at: require_path
register_graphql_resolver :apollo_entity_ref_paginated, GraphQL::ApolloEntityRefResolver::ForPaginatedList, defined_at: require_path
register_graphql_resolver :apollo_entity_ref, GraphQL::ApolloEntityRefResolver::ForSingleId, defined_at: require_path, built_in: true
register_graphql_resolver :apollo_entity_ref_list, GraphQL::ApolloEntityRefResolver::ForIdList, defined_at: require_path, built_in: true
register_graphql_resolver :apollo_entity_ref_paginated, GraphQL::ApolloEntityRefResolver::ForPaginatedList, defined_at: require_path, built_in: true
end

def apollo_object_type(name, &block)
Expand Down
4 changes: 4 additions & 0 deletions elasticgraph-apollo/spec/unit/elastic_graph/apollo/schema_definition_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ module Apollo

include_context "SchemaDefinitionHelpers"

after do
expect(logged_output).to exclude(/warn/i)
end

def self.with_both_casing_forms(&block)
context "with schema elements configured to use camelCase" do
let(:schema_element_name_form) { :camelCase }
Expand Down
7 changes: 5 additions & 2 deletions ...-schema_artifacts/lib/elastic_graph/schema_artifacts/runtime_metadata/graphql_resolver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module ElasticGraph
module SchemaArtifacts
module RuntimeMetadata
# @private
class GraphQLResolver < ::Data.define(:needs_lookahead, :resolver_ref)
class GraphQLResolver < ::Data.define(:needs_lookahead, :resolver_ref, :built_in)
def self.with_lookahead_loader
@with_lookahead_loader ||= ExtensionLoader.new(InterfaceWithLookahead)
end
Expand All @@ -21,6 +21,7 @@ def self.without_lookahead_loader
@without_lookahead_loader ||= ExtensionLoader.new(InterfaceWithoutLookahead)
end

BUILT_IN = "built_in"
NEEDS_LOOKAHEAD = "needs_lookahead"
RESOLVER_REF = "resolver_ref"

Expand All @@ -32,13 +33,15 @@ def load_resolver
def self.from_hash(hash)
new(
needs_lookahead: hash.fetch(NEEDS_LOOKAHEAD),
resolver_ref: hash.fetch(RESOLVER_REF)
resolver_ref: hash.fetch(RESOLVER_REF),
built_in: hash.fetch(BUILT_IN) { false }
)
end

def to_dumpable_hash
{
# Keys here are ordered alphabetically; please keep them that way.
BUILT_IN => built_in,
Copy link
Collaborator

@myronmarston myronmarston Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The built_in metadata is only used when dumping schema artifacts, right? I don't think we need to bloat runtime_metadata.yaml with it since there's not a later runtime when we need it.

Two potential ideas to solve it:

  • Keep built_in as an attribute of this type, but ignore it in to_dumpable_hash/from_hash so that it isn't persisted in runtime_metadata.yaml.
  • Rather than putting built_in on this type, handle that state in another way. For example, you could add built_in_graphql_resolvers as a set to ElasticGraph::SchemaDefinition::State, add resolvers to that that get registered with built_in: true, and then check membership in that set when generating the warning.

I think I lean towards the latter as it would be kinda weird to have an attribute of a runtime metadata class that isn't persisted to runtime metadata.

NEEDS_LOOKAHEAD => needs_lookahead,
RESOLVER_REF => resolver_ref
}
Expand Down
12 changes: 8 additions & 4 deletions ...schema_artifacts/sig/elastic_graph/schema_artifacts/runtime_metadata/graphql_resolver.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,23 @@ module ElasticGraph
class GraphQLResolverSupertype
attr_reader needs_lookahead: bool
attr_reader resolver_ref: ::Hash[::String, ::String]
attr_reader built_in: bool

def initialize: (
needs_lookahead: bool,
resolver_ref: ::Hash[::String, ::String]
resolver_ref: ::Hash[::String, ::String],
built_in: bool
) -> void

def with: (
?needs_lookahead: bool,
?resolver_ref: ::Hash[::String, ::String]
?resolver_ref: ::Hash[::String, ::String],
?built_in: bool
) -> instance

def self.new:
(needs_lookahead: bool, resolver_ref: ::Hash[::String, ::String]) -> instance
| (bool, ::Hash[::String, ::String]) -> instance
(needs_lookahead: bool, resolver_ref: ::Hash[::String, ::String], built_in: bool) -> instance
| (bool, ::Hash[::String, ::String], bool) -> instance
end

class GraphQLResolver < GraphQLResolverSupertype
Expand All @@ -27,6 +30,7 @@ module ElasticGraph
self.@without_lookahead_loader: ExtensionLoader?
def self.without_lookahead_loader: () -> ExtensionLoader

BUILT_IN: "built_in"
NEEDS_LOOKAHEAD: "needs_lookahead"
RESOLVER_REF: "resolver_ref"

Expand Down
12 changes: 8 additions & 4 deletions ...ifacts/spec/unit/elastic_graph/schema_artifacts/runtime_metadata/graphql_resolver_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ module RuntimeMetadata
resolver_ref: {
"name" => "ElasticGraph::GraphQLResolverWithLookahead",
"require_path" => "elastic_graph/spec_support/example_extensions/graphql_resolvers"
}
},
built_in: false
)

expect(resolver.load_resolver.extension_class).to be GraphQLResolverWithLookahead
Expand All @@ -30,7 +31,8 @@ module RuntimeMetadata
resolver_ref: {
"name" => "ElasticGraph::GraphQLResolverWithoutLookahead",
"require_path" => "elastic_graph/spec_support/example_extensions/graphql_resolvers"
}
},
built_in: false
)

expect(resolver.load_resolver.extension_class).to be GraphQLResolverWithoutLookahead
Expand All @@ -42,7 +44,8 @@ module RuntimeMetadata
resolver_ref: {
"name" => "ElasticGraph::GraphQLResolverWithoutLookahead",
"require_path" => "elastic_graph/spec_support/example_extensions/graphql_resolvers"
}
},
built_in: false
)

expect {
Expand All @@ -56,7 +59,8 @@ module RuntimeMetadata
resolver_ref: {
"name" => "ElasticGraph::GraphQLResolverWithLookahead",
"require_path" => "elastic_graph/spec_support/example_extensions/graphql_resolvers"
}
},
built_in: false
)

expect {
Expand Down
7 changes: 5 additions & 2 deletions elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,9 @@ def register_graphql_extension(extension_module, defined_at:, **config)
# end
# end
# end
def register_graphql_resolver(name, klass, defined_at:, **resolver_config)
# @param built_in [bool] Whether this resolver is built-in to ElasticGraph or one of its extensions.
Copy link
Collaborator

@myronmarston myronmarston Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other params are defined on lines 302-305. We should keep them together. Mind moving this up?

# Built-in resolvers that are unused in a schema will not trigger a warning.
def register_graphql_resolver(name, klass, defined_at:, built_in: false, **resolver_config)
extension = SchemaArtifacts::RuntimeMetadata::Extension.new(klass, defined_at, resolver_config)

needs_lookahead =
Expand All @@ -382,7 +384,8 @@ def register_graphql_resolver(name, klass, defined_at:, **resolver_config)

resolver = SchemaArtifacts::RuntimeMetadata::GraphQLResolver.new(
needs_lookahead: needs_lookahead,
resolver_ref: extension.to_dumpable_hash
resolver_ref: extension.to_dumpable_hash,
built_in: built_in
)

@state.graphql_resolvers_by_name[name] = resolver
Expand Down
5 changes: 3 additions & 2 deletions elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,14 +383,15 @@ def verify_runtime_metadata(runtime_metadata)

unused_resolvers = registered_resolvers.except(*fields_by_resolvers.keys).reject do |name, res|
# Ignore our built-in resolvers.
res.resolver_ref.fetch("require_path").start_with?("elastic_graph/graphql/resolvers/")
res.built_in
end.keys

unless unused_resolvers.empty?
state.output.puts <<~EOS
WARNING: #{unused_resolvers.size} GraphQL resolver(s) have been registered but are unused:
- #{unused_resolvers.sort.join("\n - ")}
These resolvers can be removed.
These resolvers can be removed. If you intended for them to be available as built-in/internal
resolvers, pass `built_in: true` when registering them.
EOS
end

Expand Down
15 changes: 10 additions & 5 deletions ...h-schema_definition/lib/elastic_graph/schema_definition/schema_elements/built_in_types.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,25 +1425,30 @@ def register_standard_graphql_resolvers
require(require_path = "elastic_graph/graphql/resolvers/get_record_field_value")
schema_def_api.register_graphql_resolver :get_record_field_value,
GraphQL::Resolvers::GetRecordFieldValue,
defined_at: require_path
defined_at: require_path,
built_in: true

require(require_path = "elastic_graph/graphql/resolvers/list_records")
schema_def_api.register_graphql_resolver :list_records,
GraphQL::Resolvers::ListRecords,
defined_at: require_path
defined_at: require_path,
built_in: true

require(require_path = "elastic_graph/graphql/resolvers/nested_relationships")
schema_def_api.register_graphql_resolver :nested_relationships,
GraphQL::Resolvers::NestedRelationships,
defined_at: require_path
defined_at: require_path,
built_in: true

require(require_path = "elastic_graph/graphql/resolvers/object")
schema_def_api.register_graphql_resolver :object_with_lookahead,
GraphQL::Resolvers::Object::WithLookahead,
defined_at: require_path
defined_at: require_path,
built_in: true
schema_def_api.register_graphql_resolver :object_without_lookahead,
GraphQL::Resolvers::Object::WithoutLookahead,
defined_at: require_path
defined_at: require_path,
built_in: true
end

def define_date_grouping_arguments(grouping_field, omit_timezone: false)
Expand Down
2 changes: 1 addition & 1 deletion elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ module ElasticGraph
def results: () -> Results
def json_schema_version: (::Integer) -> void
def register_graphql_extension: (::Module, defined_at: ::String, **untyped) -> void
def register_graphql_resolver: (::Symbol, ::Class, defined_at: ::String, **untyped) -> void
def register_graphql_resolver: (::Symbol, ::Class, defined_at: ::String, ?built_in: bool, **untyped) -> void
def on_built_in_types: () { (SchemaElements::graphQLType) -> void } -> void
def on_root_query_type: () { (SchemaElements::ObjectType) -> void } -> void
end
Expand Down
16 changes: 15 additions & 1 deletion ...c/unit/elastic_graph/schema_definition/runtime_metadata/graphql_resolvers_by_name_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,24 @@ module SchemaDefinition
WARNING: 2 GraphQL resolver(s) have been registered but are unused:
- resolver1
- resolver2
These resolvers can be removed.
These resolvers can be removed. If you intended for them to be available as built-in/internal
resolvers, pass `built_in: true` when registering them.
EOS
end

it "does not warn when a resolver is registered with `built_in: true` but never used" do
output = StringIO.new

graphql_resolvers_by_name(output: output) do |schema|
schema.register_graphql_resolver :resolver1,
GraphQLResolverWithoutLookahead,
defined_at: "some/path",
built_in: true
end

expect(output.string).to eq("")
end

def graphql_resolvers_by_name(...)
define_schema(...).runtime_metadata.graphql_resolvers_by_name
end
Expand Down
5 changes: 3 additions & 2 deletions spec_support/lib/elastic_graph/spec_support/runtime_metadata_support.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,11 @@ def configured_graphql_resolver(name, **config)
"require_path" => "elastic_graph/graphql/resolvers/get_record_field_value"
}

def graphql_resolver_with(needs_lookahead: false, resolver_ref: DEFAULT_RESOLVER_REF)
def graphql_resolver_with(needs_lookahead: false, resolver_ref: DEFAULT_RESOLVER_REF, built_in: false)
GraphQLResolver.new(
needs_lookahead: needs_lookahead,
resolver_ref: resolver_ref
resolver_ref: resolver_ref,
built_in: built_in
)
end

Expand Down
Loading