Resolve elasticgraph-apollo confusing rake warnings#1060
Open
ayousufi wants to merge 2 commits intoblock:mainfrom
Open
Resolve elasticgraph-apollo confusing rake warnings#1060ayousufi wants to merge 2 commits intoblock:mainfrom
ayousufi wants to merge 2 commits intoblock:mainfrom
Conversation
ayousufi
requested review from
BrianSigafoos-SQ,
bsorbo,
jwils,
jwondrusch and
myronmarston
as code owners
myronmarston
requested changes
Mar 1, 2026
...spec/unit/elastic_graph/schema_definition/runtime_metadata/graphql_resolvers_by_name_spec.rb
Show resolved
Hide resolved
elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb
Outdated
Show resolved
Hide resolved
ayousufi
force-pushed
the
abdullah/issue-690/fix-apollo-resolver-warnings
branch
from
8792ab8 to
bbf97e0
Compare
myronmarston
requested changes
Mar 2, 2026
| def to_dumpable_hash | ||
| { | ||
| # Keys here are ordered alphabetically; please keep them that way. | ||
| BUILT_IN => built_in, |
Collaborator
There was a problem hiding this comment.
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_inas an attribute of this type, but ignore it into_dumpable_hash/from_hashso that it isn't persisted inruntime_metadata.yaml. - Rather than putting
built_inon this type, handle that state in another way. For example, you could addbuilt_in_graphql_resolversas a set toElasticGraph::SchemaDefinition::State, add resolvers to that that get registered withbuilt_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.
| # 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. |
Collaborator
There was a problem hiding this comment.
The other params are defined on lines 302-305. We should keep them together. Mind moving this up?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#690
Solution
This change updates the schema validation logic to ignore Apollo's internal resolvers via a
built_inboolean param.built_in: true.How Tested
elasticgraph-apolloschema specs to ensure no warnings (including unused resolvers) are logged during schema definition.built_inresolvers when they are unused.