Merge remote-tracking branch 'origin/master' into feature/884-reject-invalid-directives

# Conflicts:
#	src/main/kotlin/graphql/kickstart/tools/SdlDirectives.kt

Author: oryan-block

.github/workflows/pull-request.yml

@@ -24,23 +24,3 @@ jobs:
           restore-keys: ${{ runner.os }}-m2
       - name: Build with Maven
         run: mvn --batch-mode --update-snapshots verify
-
-  # The same jar has to work with the oldest supported graphql-java, so build and test against it too
-  graphql-java-25:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v7
-      - name: Setup java
-        uses: actions/setup-java@v6
-        with:
-          java-version: '11'
-          distribution: 'temurin'
-      - name: Cache Maven packages
-        uses: actions/cache@v6
-        with:
-          path: ~/.m2
-          key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }}
-          restore-keys: ${{ runner.os }}-m2
-      - name: Build with Maven
-        run: mvn --batch-mode --update-snapshots verify -Dgraphql-java.version=25.1

README.md

@@ -26,7 +26,7 @@ kotlin.version=2.3.10
 
 Add the dependency:
 ```groovy
-compile 'io.github.graphql-java-kickstart:graphql-java-tools:14.4.0'
+compile 'io.github.graphql-java-kickstart:graphql-java-tools:14.5.0'
 ```
 
 ### Using Maven
@@ -43,7 +43,7 @@ Add the dependency:
 <dependency>
     <groupId>io.github.graphql-java-kickstart</groupId>
     <artifactId>graphql-java-tools</artifactId>
-    <version>14.4.0</version>
+    <version>14.5.0</version>
 </dependency>
 ```
 

pom.xml

@@ -4,7 +4,7 @@
 
     <groupId>io.github.graphql-java-kickstart</groupId>
     <artifactId>graphql-java-tools</artifactId>
-    <version>14.4.1-SNAPSHOT</version>
+    <version>14.5.1-SNAPSHOT</version>
     <packaging>jar</packaging>
 
     <name>GraphQL Java Tools</name>

src/main/kotlin/graphql/kickstart/tools/GenericTypeResolver.kt

@@ -29,7 +29,7 @@ internal class GenericTypeResolver(private val containingType: JavaType, private
      * - wildcards are replaced by their upper bound, e.g. `? extends Foo` becomes `Foo`
      * - primitives are replaced by their boxed class, e.g. `int` becomes `Integer`
      */
-    fun resolve(javaType: JavaType, onUnwrap: ((wrapped: ParameterizedType) -> Unit)? = null): JavaType {
+    fun resolve(javaType: JavaType, onUnwrap: ((SchemaParserOptions.GenericWrapper) -> Unit)? = null): JavaType {
         return when (val type = resolveTypeVariables(javaType)) {
             is ParameterizedType -> {
                 val rawType = type.rawType
@@ -41,7 +41,7 @@ internal class GenericTypeResolver(private val containingType: JavaType, private
                     throw IndexOutOfBoundsException("Generic type '${TypeUtils.toString(type)}' does not have a type argument at index ${wrapper.index}!")
                 }
 
-                onUnwrap?.invoke(type)
+                onUnwrap?.invoke(wrapper)
                 val unwrapsTo = wrapper.schemaWrapper.invoke(typeArguments[wrapper.index])
                 resolve(unwrapsTo, onUnwrap)
             }

src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt

@@ -6,6 +6,7 @@ import graphql.kickstart.tools.ScanDictionary.InitialDictionaryEntry
 import graphql.kickstart.tools.ScanDictionary.UnionMemberReference
 import graphql.kickstart.tools.ScanDictionary.UnusedTypeReference
 import graphql.kickstart.tools.resolver.FieldResolver
+import graphql.kickstart.tools.resolver.FieldResolverError
 import graphql.kickstart.tools.resolver.FieldResolverScanner
 import graphql.kickstart.tools.resolver.MethodFieldResolver
 import graphql.kickstart.tools.resolver.PropertyFieldResolver
@@ -66,6 +67,7 @@ internal class SchemaClassScanner(
     private val sealedSubclassMatcher = SealedSubclassMatcher(dictionary, typeExtensions)
 
     private val fieldResolversByType = mutableMapOf<ObjectTypeDefinition, MutableMap<FieldDefinition, FieldResolver>>()
+    private val notCallableOnResolver = mutableListOf<NotCallableOnResolver>()
 
     init {
         initialDictionary.forEach { (name, clazz) ->
@@ -145,6 +147,8 @@ internal class SchemaClassScanner(
             }
         }
 
+        throwIfNotCallableOnResolver()
+
         return validateAndCreateResult(rootTypeHolder)
     }
 
@@ -450,19 +454,18 @@ internal class SchemaClassScanner(
     private fun scanResolverInfoForPotentialMatches(type: ObjectTypeDefinition, resolverInfo: ResolverInfo) {
         type.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }.forEach { field ->
             val fieldResolver = fieldResolverScanner.findFieldResolver(field, resolverInfo)
-            warnIfNotCallableOnResolver(type, fieldResolver)
+            collectIfNotCallableOnResolver(type, fieldResolver)
 
             fieldResolversByType.getOrPut(type) { mutableMapOf() }[fieldResolver.field] = fieldResolver
 
             fieldResolver.scanForMatches().forEach { potentialMatch ->
                 dictionary.handleFoundType(typeClassMatcher.match(potentialMatch))
             }
-            warnIfParameterMissing(type, fieldResolver)
+            requireParameterForEachArgument(type, fieldResolver)
         }
     }
 
-    // TODO throw a FieldResolverError instead in 15.0.0 (#868)
-    private fun warnIfNotCallableOnResolver(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
+    private fun collectIfNotCallableOnResolver(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
         val source = fieldResolver.search.source ?: return
         val member: Member = when (fieldResolver) {
             is MethodFieldResolver -> fieldResolver.method
@@ -473,17 +476,26 @@ internal class SchemaClassScanner(
             return
         }
 
-        // class-based proxies don't delegate field reads to the target, so a field needs a getter too
-        val fix = if (member is Field) "Add a getter for the field and use class-based proxies" else "Use class-based proxies instead"
-        log.warn(
+        notCallableOnResolver += NotCallableOnResolver(
             "Field '${type.name}.${fieldResolver.field.name}' resolves to ${member.declaringClass.name}.${member.name}, but the resolver is a " +
-                "${source.javaClass.name}, which isn't a ${member.declaringClass.name}, so every query of the field will fail. This happens " +
-                "with JDK dynamic proxies, which only implement interfaces. $fix, e.g. proxyTargetClass = true in Spring."
+                "${source.javaClass.name}, which isn't a ${member.declaringClass.name}, so every query of the field will fail.",
+            member is Field
+        )
+    }
+
+    private fun throwIfNotCallableOnResolver() {
+        if (notCallableOnResolver.isEmpty()) {
+            return
+        }
+
+        val fix = "Use class-based proxies instead, e.g. proxyTargetClass = true in Spring." +
+            if (notCallableOnResolver.any { it.isField }) " For a public field, also add a getter, since class-based proxies don't pass field reads to the target." else ""
+        throw FieldResolverError(
+            notCallableOnResolver.joinToString("\n", postfix = "\nThis happens with JDK dynamic proxies, which only implement interfaces. $fix") { it.message }
         )
     }
 
-    // TODO throw a FieldResolverError instead in 15.0.0 (#889)
-    private fun warnIfParameterMissing(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
+    private fun requireParameterForEachArgument(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
         if (fieldResolver !is MethodFieldResolver) {
             return
         }
@@ -495,7 +507,7 @@ internal class SchemaClassScanner(
             }
 
             val method = fieldResolver.method
-            log.warn(
+            throw FieldResolverError(
                 "Field '${type.name}.${fieldResolver.field.name}' has no parameter for its argument '${argument.name}: " +
                     "${TypeUtil.simplePrint(argument.type)}' in ${method.declaringClass.name}.${method.name}, so the ${parameter.simpleName} " +
                     "parameter gets the argument instead. Add a parameter for each argument, in order, before the ${parameter.simpleName} parameter."
@@ -528,6 +540,8 @@ internal class SchemaClassScanner(
 
     private data class QueueItem(val type: ObjectTypeDefinition, val clazz: JavaType)
 
+    private data class NotCallableOnResolver(val message: String, val isField: Boolean)
+
     private class RootTypesHolder(
         options: SchemaParserOptions,
         rootInfo: RootTypeInfo,

src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt

@@ -23,7 +23,6 @@ import graphql.language.*
 import graphql.schema.*
 import graphql.schema.idl.RuntimeWiring
 import graphql.schema.idl.ScalarInfo
-import graphql.schema.visibility.BlockedFields
 import org.slf4j.LoggerFactory
 import kotlin.reflect.KClass
 
@@ -79,13 +78,6 @@ class SchemaParser internal constructor(
      * Parses the given schema with respect to the given dictionary and returns GraphQL objects.
      */
     fun parseSchemaObjects(): SchemaObjects {
-        if (!options.introspectionEnabled) {
-            // What the deprecated NoIntrospectionGraphqlFieldVisibility does. Its suggested replacement,
-            // Introspection.enabledJvmWide, would also turn introspection off for every other schema in the JVM.
-            // TODO deprecate introspectionEnabled and remove this in 15.0.0 (#858)
-            codeRegistryBuilder.fieldVisibility(BlockedFields.newBlock().addPattern("__.*").build())
-        }
-        // this overrides the above introspection enabled setting obviously... todo: add documentation
         options.fieldVisibility?.let { codeRegistryBuilder.fieldVisibility(it) }
 
         // Create GraphQL objects
@@ -172,6 +164,8 @@ class SchemaParser internal constructor(
         val name = objectDefinition.name
         val extensions = typeExtensions.extensionsOf<ObjectTypeExtensionDefinition>(name)
         val directives = sdlDirectives.directivesOf(objectDefinition, extensions)
+        val fieldDefinitions = objectDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }
+        rejectRepeated(name, "field", fieldDefinitions)
         val builder = GraphQLObjectType.newObject()
             .name(name)
             .definition(objectDefinition)
@@ -186,7 +180,7 @@ class SchemaParser internal constructor(
                 }
             }
             .apply {
-                objectDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }.forEach { fieldDefinition ->
+                fieldDefinitions.forEach { fieldDefinition ->
                     field { field ->
                         createField(field, fieldDefinition, inputObjects)
                         codeRegistryBuilder.dataFetcher(
@@ -210,6 +204,8 @@ class SchemaParser internal constructor(
                                   inputObjectsInProgress: MutableSet<String>): GraphQLInputObjectType {
         val extensionDefinitions = typeExtensions.extensionsOf<InputObjectTypeExtensionDefinition>(definition.name)
         val directives = sdlDirectives.directivesOf(definition, extensionDefinitions)
+        val inputValueDefinitions = definition.withExtensions(typeExtensions).flatMap { it.inputValueDefinitions }
+        rejectRepeated(definition.name, "field", inputValueDefinitions)
 
         inputObjectsInProgress.add(definition.name)
 
@@ -220,20 +216,18 @@ class SchemaParser internal constructor(
             .description(getDocumentation(definition, options))
             .withSdlDirectives(directives, INPUT_OBJECT)
             .apply {
-                (extensionDefinitions + definition).forEach { typeDefinition ->
-                    typeDefinition.inputValueDefinitions.forEach { fieldDefinition ->
-                        field(
-                            GraphQLInputObjectField.newInputObjectField()
-                                .name(fieldDefinition.name)
-                                .definition(fieldDefinition)
-                                .description(getDocumentation(fieldDefinition, options))
-                                .apply { fieldDefinition.defaultValue?.let { v -> defaultValueLiteral(v) } }
-                                .apply { sdlDirectives.getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } }
-                                .type(determineInputType(fieldDefinition.type, inputObjects, inputObjectsInProgress))
-                                .withSdlDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION)
-                                .build()
-                        )
-                    }
+                inputValueDefinitions.forEach { fieldDefinition ->
+                    field(
+                        GraphQLInputObjectField.newInputObjectField()
+                            .name(fieldDefinition.name)
+                            .definition(fieldDefinition)
+                            .description(getDocumentation(fieldDefinition, options))
+                            .apply { fieldDefinition.defaultValue?.let { v -> defaultValueLiteral(v) } }
+                            .apply { sdlDirectives.getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } }
+                            .type(determineInputType(fieldDefinition.type, inputObjects, inputObjectsInProgress))
+                            .withSdlDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION)
+                            .build()
+                    )
                 }
             }
 
@@ -250,15 +244,15 @@ class SchemaParser internal constructor(
         val extensions = typeExtensions.extensionsOf<EnumTypeExtensionDefinition>(name)
         val directives = sdlDirectives.directivesOf(definition, extensions)
         val values = definition.withExtensions(typeExtensions).flatMap { it.enumValueDefinitions }
-        warnRepeated(name, "value", values.map { it.name })
+        rejectRepeated(name, "value", values)
         val builder = GraphQLEnumType.newEnum()
             .name(name)
             .definition(definition)
             .extensionDefinitions(extensions)
             .description(getDocumentation(definition, options))
             .withSdlDirectives(directives, ENUM)
             .apply {
-                values.distinctBy { it.name }.forEach { valueDefinition ->
+                values.forEach { valueDefinition ->
                     val enumName = valueDefinition.name
                     val enumValue = if (enumClass == null) {
                         enumName
@@ -295,12 +289,7 @@ class SchemaParser internal constructor(
             .specifiedByUrl(getSpecifiedByUrl(directives) ?: provided.specifiedByUrl)
             // like graphql-java, only the schema's directives are applied
             .clearDirectives()
-        try {
-            builder.withSdlDirectives(directives, SCALAR)
-        } catch (e: SchemaError) {
-            // TODO throw instead in 15.0.0 (#893), like for the other types
-            log.warn("Scalar '$name' is built without its directives from the schema: ${e.message}")
-        }
+            .withSdlDirectives(directives, SCALAR)
 
         return builder.build()
     }
@@ -309,15 +298,15 @@ class SchemaParser internal constructor(
         val extensions = typeExtensions.extensionsOf<InterfaceTypeExtensionDefinition>(interfaceDefinition.name)
         val directives = sdlDirectives.directivesOf(interfaceDefinition, extensions)
         val fieldDefinitions = interfaceDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }
-        warnRepeated(interfaceDefinition.name, "field", fieldDefinitions.map { it.name })
+        rejectRepeated(interfaceDefinition.name, "field", fieldDefinitions)
         val builder = GraphQLInterfaceType.newInterface()
             .name(interfaceDefinition.name)
             .definition(interfaceDefinition)
             .extensionDefinitions(extensions)
             .description(getDocumentation(interfaceDefinition, options))
             .withSdlDirectives(directives, INTERFACE)
             .apply {
-                fieldDefinitions.distinctBy { it.name }.forEach { fieldDefinition ->
+                fieldDefinitions.forEach { fieldDefinition ->
                     field { field -> createField(field, fieldDefinition, inputObjects) }
                 }
             }
@@ -334,7 +323,7 @@ class SchemaParser internal constructor(
     private fun createUnionObject(definition: UnionTypeDefinition, types: List<GraphQLObjectType>): GraphQLUnionType {
         val extensions = typeExtensions.extensionsOf<UnionTypeExtensionDefinition>(definition.name)
         val directives = sdlDirectives.directivesOf(definition, extensions)
-        warnRepeated(definition.name, "member", definition.withExtensions(typeExtensions).flatMap { it.memberTypes }.map { (it as TypeName).name })
+        rejectRepeated(definition.name, "member", definition.withExtensions(typeExtensions).flatMap { it.memberTypes }.map { it as TypeName })
         val builder = GraphQLUnionType.newUnionType()
             .name(definition.name)
             .definition(definition)
@@ -376,11 +365,10 @@ class SchemaParser internal constructor(
         return leafObjects
     }
 
-    // TODO throw a SchemaError instead in 15.0.0 (#863), like graphql-java does
-    private fun warnRepeated(typeName: String, kind: String, names: List<String?>) {
-        names.groupingBy { it }.eachCount().filterValues { it > 1 }.keys.forEach {
-            log.warn("Type '$typeName' declares $kind '$it' more than once, only the first one is kept.")
-        }
+    private fun rejectRepeated(typeName: String, kind: String, declarations: List<NamedNode<*>>) {
+        val names = mutableSetOf<String?>()
+        val repeat = declarations.firstOrNull { !names.add(it.name) } ?: return
+        throw SchemaError("Type '$typeName' declares $kind '${repeat.name}' more than once${repeat.sourceLocation.describe()}.")
     }
 
     private fun createField(field: GraphQLFieldDefinition.Builder, fieldDefinition: FieldDefinition, inputObjects: List<GraphQLInputObjectType>): GraphQLFieldDefinition.Builder {

src/main/kotlin/graphql/kickstart/tools/SchemaParserDictionary.kt

@@ -11,16 +11,6 @@ class SchemaParserDictionary {
      */
     fun toMap(): Map<String, Class<*>> = dictionary.toMap()
 
-    // TODO remove in 15.0.0 (#900)
-    /**
-     * Returns a copy of the entries added so far. If several types share a class, the inverse of the copy only maps that class
-     * back to the first of them.
-     */
-    @Deprecated("Use toMap() instead", ReplaceWith("toMap()"))
-    @Suppress("DEPRECATION")
-    fun getDictionary(): graphql.kickstart.tools.util.BiMap<String, Class<*>> =
-        graphql.kickstart.tools.util.BiMap.unmodifiableCopyOf(dictionary)
-
     /**
      * Add arbitrary classes to the parser's dictionary, overriding the generated type name.
      */

src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt

@@ -31,7 +31,6 @@ data class SchemaParserOptions internal constructor(
     val proxyHandlers: List<ProxyHandler>,
     val inputArgumentOptionalDetectOmission: Boolean,
     val preferGraphQLResolver: Boolean,
-    val introspectionEnabled: Boolean,
     val coroutineContextProvider: CoroutineContextProvider,
     val typeDefinitionFactories: List<TypeDefinitionFactory>,
     val fieldVisibility: GraphqlFieldVisibility?,
@@ -62,7 +61,6 @@ data class SchemaParserOptions internal constructor(
         private val proxyHandlers: MutableList<ProxyHandler> = mutableListOf(Spring4AopProxyHandler(), GuiceAopProxyHandler(), JavassistProxyHandler(), WeldProxyHandler())
         private var inputArgumentOptionalDetectOmission = false
         private var preferGraphQLResolver = false
-        private var introspectionEnabled = true
         private var coroutineContextProvider: CoroutineContextProvider? = null
         private var typeDefinitionFactories: MutableList<TypeDefinitionFactory> = mutableListOf(RelayConnectionFactory())
         private var fieldVisibility: GraphqlFieldVisibility? = null
@@ -128,10 +126,6 @@ data class SchemaParserOptions internal constructor(
             this.proxyHandlers.add(proxyHandler)
         }
 
-        fun introspectionEnabled(introspectionEnabled: Boolean) = this.apply {
-            this.introspectionEnabled = introspectionEnabled
-        }
-
         fun coroutineContext(context: CoroutineContext) = this.apply {
             this.coroutineContextProvider = DefaultCoroutineContextProvider(context)
         }
@@ -217,7 +211,6 @@ data class SchemaParserOptions internal constructor(
                 proxyHandlers,
                 inputArgumentOptionalDetectOmission,
                 preferGraphQLResolver,
-                introspectionEnabled,
                 coroutineContextProvider,
                 typeDefinitionFactories,
                 fieldVisibility,