diff --git a/elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb b/elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb index 77f7b969d..e3b9ad687 100644 --- a/elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb +++ b/elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb @@ -389,15 +389,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) diff --git a/elasticgraph-apollo/spec/unit/elastic_graph/apollo/schema_definition_spec.rb b/elasticgraph-apollo/spec/unit/elastic_graph/apollo/schema_definition_spec.rb index dbd4f1ee6..b80a51b24 100644 --- a/elasticgraph-apollo/spec/unit/elastic_graph/apollo/schema_definition_spec.rb +++ b/elasticgraph-apollo/spec/unit/elastic_graph/apollo/schema_definition_spec.rb @@ -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 } diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb index 9be4d045d..4b61944c2 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb @@ -302,6 +302,8 @@ def register_graphql_extension(extension_module, defined_at:, **config) # @param name [Symbol] unique name of the resolver # @param klass [Class] resolver class # @param defined_at [String] the `require` path of the resolver + # @param built_in [bool] Whether this resolver is built-in to ElasticGraph or one of its extensions. + # Built-in resolvers that are unused in a schema will not trigger a warning. # @param resolver_config [Hash] configuration options for the resolver, to support parameterized resolvers # @return [void] # @see Mixins::HasIndices#resolve_fields_with @@ -369,7 +371,7 @@ def register_graphql_extension(extension_module, defined_at:, **config) # end # end # end - def register_graphql_resolver(name, klass, defined_at:, **resolver_config) + 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 = @@ -386,6 +388,11 @@ def register_graphql_resolver(name, klass, defined_at:, **resolver_config) ) @state.graphql_resolvers_by_name[name] = resolver + if built_in + @state.built_in_graphql_resolvers << name + else + @state.built_in_graphql_resolvers.delete(name) + end nil end diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb index 0bd9ad604..40afc05e5 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb @@ -381,16 +381,14 @@ def verify_runtime_metadata(runtime_metadata) end end - 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/") - end.keys + unused_resolvers = registered_resolvers.except(*fields_by_resolvers.keys, *state.built_in_graphql_resolvers).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 diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/built_in_types.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/built_in_types.rb index b040b37fc..99c6e3fb8 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/built_in_types.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/built_in_types.rb @@ -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) diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/state.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/state.rb index b8c73eedc..800d15cbf 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/state.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/state.rb @@ -44,7 +44,7 @@ class State < Struct.new( :json_schema_version_setter_location, :graphql_extension_modules, :graphql_resolvers_by_name, - :resolvers_by_name, + :built_in_graphql_resolvers, :initially_registered_built_in_types, :built_in_types_customization_blocks, :user_definition_complete, @@ -92,6 +92,7 @@ def self.with( json_schema_version: nil, graphql_extension_modules: [], graphql_resolvers_by_name: {}, + built_in_graphql_resolvers: ::Set.new, initially_registered_built_in_types: ::Set.new, built_in_types_customization_blocks: [], user_definition_complete: false, diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs index 310ba70f9..604ed7842 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/api.rbs @@ -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 diff --git a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/state.rbs b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/state.rbs index ee8a043ae..7ea53f98d 100644 --- a/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/state.rbs +++ b/elasticgraph-schema_definition/sig/elastic_graph/schema_definition/state.rbs @@ -20,6 +20,7 @@ module ElasticGraph attr_accessor json_schema_version_setter_location: ::Thread::Backtrace::Location? attr_reader graphql_extension_modules: ::Array[SchemaArtifacts::RuntimeMetadata::GraphQLExtension] attr_reader graphql_resolvers_by_name: ::Hash[::Symbol, SchemaArtifacts::RuntimeMetadata::GraphQLResolver] + attr_reader built_in_graphql_resolvers: ::Set[::Symbol] attr_accessor initially_registered_built_in_types: ::Set[::String] attr_accessor built_in_types_customization_blocks: ::Array[^(SchemaElements::graphQLType) -> void] attr_accessor user_definition_complete: bool @@ -50,8 +51,9 @@ module ElasticGraph deleted_fields_by_type_name_and_old_field_name: ::Hash[::String, ::Hash[::String, SchemaElements::DeprecatedElement]], json_schema_version: Integer?, json_schema_version_setter_location: ::Thread::Backtrace::Location?, - graphql_extension_modules: ::Array[SchemaArtifacts::RuntimeMetadata::Extension], - graphql_resolvers_by_name: ::Hash[::Symbol, SchemaArtifacts::RuntimeMetadata::Extension], + graphql_extension_modules: ::Array[SchemaArtifacts::RuntimeMetadata::GraphQLExtension], + graphql_resolvers_by_name: ::Hash[::Symbol, SchemaArtifacts::RuntimeMetadata::GraphQLResolver], + built_in_graphql_resolvers: ::Set[::Symbol], initially_registered_built_in_types: ::Set[::String], built_in_types_customization_blocks: ::Array[^(SchemaElements::graphQLType) -> void], user_definition_complete: bool, diff --git a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/graphql_resolvers_by_name_spec.rb b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/graphql_resolvers_by_name_spec.rb index 6dfe4cbb1..69cc710be 100644 --- a/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/graphql_resolvers_by_name_spec.rb +++ b/elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/runtime_metadata/graphql_resolvers_by_name_spec.rb @@ -144,7 +144,43 @@ 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 + + it "warns when a built-in resolver name is re-registered as non-built-in and remains unused" 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 + + schema.register_graphql_resolver :resolver1, + GraphQLResolverWithoutLookahead, + defined_at: "some/path" + end + + expect(output.string).to eq(<<~EOS) + WARNING: 1 GraphQL resolver(s) have been registered but are unused: + - resolver1 + 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