Skip to content

Commit 5aa2c60

Browse files
authored
Merge pull request #869 from graphql-java-kickstart/cleanup/422-comments
Cut comments from #857 that repeat the code or explain its history
2 parents b8a33b6 + 973a96f commit 5aa2c60

16 files changed

Lines changed: 8 additions & 116 deletions

‎pom.xml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,6 @@
373373

374374
<profiles>
375375
<profile>
376-
<!-- Tests of Java sealed classes, which need Java 17 to compile -->
377376
<id>java17-tests</id>
378377
<activation>
379378
<jdk>[17,)</jdk>

‎src/main/kotlin/graphql/kickstart/tools/ScannedSchemaObjects.kt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,5 +26,4 @@ internal data class ScannedSchemaObjects(
2626

2727
internal typealias TypeClassDictionary = BiMap<TypeDefinition<*>, JavaType>
2828
internal typealias CustomScalarMap = Map<String, GraphQLScalarType>
29-
// the extensions of each type by type name, in the order they're declared
3029
internal typealias TypeExtensions = Map<String, List<TypeDefinition<*>>>

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

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,11 @@ internal class SchemaClassScanner(
4545
private val typeDefinitions = allDefinitions.filterIsInstance<TypeDefinition<*>>()
4646
private val baseTypeNames = typeDefinitions.filter { it !is SDLExtensionDefinition }.map { it.name }.toSet()
4747

48-
// the first extension of an undefined union, enum, interface or scalar stands in for its definition
4948
private val loneExtensions = typeDefinitions
5049
.filter { it is SDLExtensionDefinition && it !is ObjectTypeDefinition && it !is InputObjectTypeDefinition && it.name !in baseTypeNames }
5150
.distinctBy { it.name }
5251

5352
private val definitionsByName = typeDefinitions.filter { it !is SDLExtensionDefinition || it in loneExtensions }.associateBy { it.name }
54-
// without extensions of a different kind than their type, which are ignored with a warning, see init
5553
private val typeExtensions: TypeExtensions = typeDefinitions
5654
.filter { it is SDLExtensionDefinition && it !in loneExtensions && !extendsOtherKind(it) }
5755
.groupBy { it.name }
@@ -80,8 +78,6 @@ internal class SchemaClassScanner(
8078
log.warn("Type '${extension.name}' is defined as ${definition.kind()}, but extended as ${extension.kind()}. The extension is ignored.")
8179
}
8280

83-
// Input object types never go into the type => class bimap that interface and union type resolution use, so several of
84-
// them can share a class, e.g. ObjectNode
8581
initialDictionary
8682
.filterKeys { definitionsByName[it] !is InputObjectTypeDefinition }
8783
.entries.groupBy({ it.value }, { it.key })
@@ -397,8 +393,6 @@ internal class SchemaClassScanner(
397393
}
398394
}
399395

400-
// Scanning a sealed subclass can find the classes of the other types, so only fail when no type has exactly one sealed
401-
// subclass named after it. Types still without a class fail on the next pass.
402396
val sealedMatches = typesWithoutClass.filterIsInstance<ObjectTypeDefinition>().mapNotNull { sealedSubclassMatches(it).singleOrNull() }
403397
if (sealedMatches.isEmpty()) {
404398
typesWithoutClass.firstOrNull()?.let { throw SchemaClassScannerError(failureMessage(it) + sealedSubclassHint(it)) }
@@ -407,9 +401,6 @@ internal class SchemaClassScanner(
407401
sealedMatches.forEach { handleFoundType(it) }
408402
}
409403

410-
/**
411-
* Explains why no sealed subclass was used for the type, if any of its unions or interfaces has a sealed class.
412-
*/
413404
private fun sealedSubclassHint(type: TypeDefinition<*>): String {
414405
if (type !is ObjectTypeDefinition) {
415406
return ""
@@ -429,25 +420,15 @@ internal class SchemaClassScanner(
429420
}
430421
}
431422

432-
/**
433-
* A member of a union or an implementor of an interface may never be returned directly, e.g. `NormalPet` when only
434-
* `pets(): List<Pet>` returns `union Pet`. If the class found for the union or interface is sealed, its subclass named
435-
* after the object type can be used instead.
436-
*/
437423
private fun sealedSubclassMatches(type: ObjectTypeDefinition): List<TypeClassMatcher.ValidMatch> =
438424
sealedTypesOf(type)
439425
.flatMap { sealedType ->
440426
sealedSubclasses(sealedType.unwrap())
441427
.filter { it.simpleName == type.name }
442-
.map { TypeClassMatcher.ValidMatch(type, parameterize(it, sealedType), SealedSubclassReference(sealedType)) }
428+
.map { TypeClassMatcher.ValidMatch(type, withSealedTypeArguments(it, sealedType), SealedSubclassReference(sealedType)) }
443429
}
444-
// a subclass is found twice when it's under two of these sealed types, or extends both a sealed type and one of its
445-
// sealed subclasses
446430
.distinctBy { it.javaType.unwrap() }
447431

448-
/**
449-
* The sealed classes found for the unions the type is a member of and the interfaces it implements.
450-
*/
451432
private fun sealedTypesOf(type: ObjectTypeDefinition): List<JavaType> =
452433
dictionary
453434
.filterKeys { definition ->
@@ -461,18 +442,11 @@ internal class SchemaClassScanner(
461442
.distinct()
462443
.filter { it.unwrap().kotlin.isSealed }
463444

464-
/**
465-
* All subclasses of a sealed class, including those of its sealed subclasses. kotlin-reflect also returns the permitted
466-
* subclasses of Java sealed classes.
467-
*/
445+
// kotlin-reflect also returns the permitted subclasses of Java sealed classes
468446
private fun sealedSubclasses(clazz: Class<*>): List<Class<*>> =
469447
clazz.kotlin.sealedSubclasses.flatMap { listOf(it.java) + sealedSubclasses(it.java) }
470448

471-
/**
472-
* Passes the type arguments of a parameterized sealed class on to its generic subclass, e.g. `Success<Pet>` for
473-
* `Result<Pet>`, so the types of the subclass's fields can be resolved.
474-
*/
475-
private fun parameterize(subclass: Class<*>, sealedType: JavaType): JavaType {
449+
private fun withSealedTypeArguments(subclass: Class<*>, sealedType: JavaType): JavaType {
476450
if (sealedType !is ParameterizedType || subclass.typeParameters.isEmpty()) {
477451
return subclass
478452
}
@@ -538,7 +512,6 @@ internal class SchemaClassScanner(
538512
}
539513
}
540514

541-
// a JDK dynamic proxy only implements its target's interfaces, so the target's own methods and fields can't be called on it
542515
// TODO throw a FieldResolverError instead in 15.0.0 (#868)
543516
private fun warnIfNotCallableOnResolver(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
544517
val source = fieldResolver.search.source ?: return
@@ -693,7 +666,6 @@ internal class SchemaClassScanner(
693666
}
694667
}
695668

696-
// e.g. `extend union Foo` of `type Foo`
697669
private fun extendsOtherKind(definition: TypeDefinition<*>): Boolean {
698670
val type = definitionsByName[definition.name]
699671
return definition is SDLExtensionDefinition && type != null && type.kind() != definition.kind()

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,6 @@ class SchemaParser internal constructor(
187187
.withAppliedDirectives(*buildAppliedDirectives(directives))
188188
.withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.INPUT_OBJECT))
189189
.apply {
190-
// unlike graphql-java the fields of the extensions come first, which is kept so the order of the fields doesn't change
191190
(extensionDefinitions + definition).forEach { typeDefinition ->
192191
typeDefinition.inputValueDefinitions.forEach { fieldDefinition ->
193192
field(
@@ -326,9 +325,6 @@ class SchemaParser internal constructor(
326325
}
327326
}
328327

329-
// The directives of the extensions follow those of the type, like in graphql-java. A non-repeatable directive that an extension
330-
// applies again is dropped, where graphql-java rejects the schema, so those schemas keep building and the applied directives
331-
// match the legacy ones (see buildDirectives). Undeclared directives may be repeatable, so they're all kept.
332328
private fun directivesOf(definition: TypeDefinition<*>, extensions: List<TypeDefinition<*>>): List<Directive> {
333329
val applied = definition.directives.mapTo(mutableSetOf()) { it.name }
334330
return definition.directives + extensions.flatMap { extension ->

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import kotlin.reflect.KClass
55

66
class SchemaParserDictionary {
77

8-
// Not a BiMap, since several input types can share a class, e.g. ObjectNode. The scanner checks that other types don't.
98
private val dictionary = linkedMapOf<String, Class<*>>()
109

1110
/**

‎src/main/kotlin/graphql/kickstart/tools/UnmappedInputFieldChecker.kt‎

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,14 @@ import graphql.schema.GraphQLType
2121
import graphql.schema.GraphQLTypeUtil
2222
import org.slf4j.LoggerFactory
2323

24-
/**
25-
* Checks that the classes of resolver arguments have a property for each field of their input objects, see [UnmappedInputFields].
26-
*
27-
* The fields are looked up in the deserializer that the resolver's ObjectMapper converts the argument with, not in the class
28-
* itself, so that a field counts as mapped whenever Jackson maps it. An input type can be used by fields whose mappers are
29-
* configured differently, so it's checked once for each mapper.
30-
*/
3124
internal class UnmappedInputFieldChecker(
3225
private val inputObjects: Map<String, GraphQLInputObjectType>,
3326
private val unmappedInputFields: UnmappedInputFields
3427
) {
3528
private val log = LoggerFactory.getLogger(javaClass)
3629

37-
// a deserializer is cached by its mapper, so this is per mapper, class and input type, and ends recursive input types
3830
private val checked = mutableSetOf<Pair<JsonDeserializer<*>, String>>()
3931

40-
// reported once for each consequence, with the first argument found using it, since mappers differ in ignoring unknown properties
4132
private val messages = linkedMapOf<Unmapped, String>()
4233

4334
fun check(objectType: String, resolver: MethodFieldResolver) {
@@ -50,14 +41,11 @@ internal class UnmappedInputFieldChecker(
5041
val mapper = resolver.objectMapper
5142
val context = (mapper.deserializationContext as? DefaultDeserializationContext)
5243
?.createDummyInstance(mapper.deserializationConfig) ?: return@forEachIndexed
53-
// the same lookup as convertValue's when it converts the argument
5444
val deserializer = context.findRootValueDeserializer(mapper.typeFactory.constructType(parameterType))
5545
checkFields(mapper, context, deserializer, null, inputObject, usage)
5646
} catch (e: VirtualMachineError) {
5747
throw e
5848
} catch (e: Throwable) {
59-
// e.g. a class Jackson can't deserialize, or one that refers to a missing class, which only fails once the argument is
60-
// converted, as it did before this check. Errors are caught too, since the lookup used to run on the first query
6149
log.debug("Can't check the input fields of $usage with Jackson", e)
6250
}
6351
}
@@ -101,12 +89,9 @@ internal class UnmappedInputFieldChecker(
10189
}
10290
}
10391

104-
// the deserializer of the class an input object is converted to, past the lists, arrays and Optionals around it
10592
private tailrec fun beanDeserializer(context: DeserializationContext, deserializer: JsonDeserializer<*>?, property: BeanProperty?): BeanDeserializerBase? =
10693
when (deserializer) {
107-
// a delegating creator gets the whole input object, e.g. as a Map
10894
is BeanDeserializerBase -> deserializer.takeUnless { it.valueInstantiator.canCreateUsingDelegate() }
109-
// a Map gets the fields as its keys
11095
is ContainerDeserializerBase<*> ->
11196
if (deserializer.valueType.isMapLikeType || isPolymorphic(context, deserializer.contentType)) null
11297
else beanDeserializer(context, deserializer.contentDeserializer, property)
@@ -120,20 +105,17 @@ internal class UnmappedInputFieldChecker(
120105
}
121106

122107
/**
123-
* Whether the elements of a list, an array or an Optional are read with type info, so that their class is only known once their
124-
* type id is read. Their deserializer is the base class's, which doesn't know the subclasses' properties. The same lookup as
125-
* Jackson's when it creates the deserializer: type info from a property's annotation, or else from the class or default typing.
108+
* The deserializer of elements read with type info is the base class's, which doesn't know the subclasses' properties. The
109+
* same lookup as Jackson's when it creates the deserializer: type info from a property's annotation, or else from the class
110+
* or default typing.
126111
*/
127112
private fun isPolymorphic(context: DeserializationContext, contentType: JavaType): Boolean =
128113
contentType.getTypeHandler<Any?>() != null || context.factory.findTypeDeserializer(context.config, contentType) != null
129114

130115
/**
131-
* Returns the properties Jackson knows if it fails on the field, or null if it maps or ignores it.
132-
*
133116
* `{"<field>": null}` is read into a placeholder object rather than an instance of the class, so neither its constructors nor
134117
* its setters are called, but the field still goes through Jackson's handling of a property the class doesn't have: ignored
135-
* names, any setters, ignoreUnknown and problem handlers. Unknown properties are made to fail even if the mapper ignores them.
136-
* Any other exception, e.g. from a deserializer that can't read into an existing object, means the field can't be checked.
118+
* names, any setters, ignoreUnknown and problem handlers.
137119
*/
138120
private fun knownPropertiesIfUnmapped(
139121
mapper: ObjectMapper,

‎src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,10 @@ import java.io.IOException
1818
import java.lang.reflect.Type
1919

2020
/**
21-
* Converts an argument value that graphql-java already coerced to the type of the resolver method's parameter.
22-
*
2321
* ObjectMapper.convertValue serializes a value and reads it back, which the coerced values of many custom scalars don't
2422
* survive, e.g. an upload, or a LocalDate without the jsr310 module. Those values are written as embedded objects
2523
* instead, which Jackson's deserializers return as they are if they have the type being read, and convert with
2624
* toString() if a String is read.
27-
*
28-
* An untyped parameter, e.g. a Map<String, Any?>, gets every coerced value as it is, apart from strings, numbers,
29-
* booleans and enums, which are converted as before.
3025
*/
3126
internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) {
3227

@@ -39,16 +34,12 @@ internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) {
3934
}
4035

4136
if (isUntyped) {
42-
// nothing reads an untyped value through its serialized form, so every coerced value is kept
4337
return read(embed(value) { true } ?: value)
4438
}
4539

46-
// a deserializer may read the serialized form, so values are only embedded if converting fails
4740
return try {
4841
mapper.convertValue<Any?>(value, javaType)
4942
} catch (e: IllegalArgumentException) {
50-
// a class whose value failed to convert isn't tried again in this value, since that can be expensive, e.g. for
51-
// a list of uploads. It isn't remembered across calls, so the same input always gives the same result
5243
val nonConvertingClasses = HashSet<Class<*>>()
5344
val embedded = embed(value) { scalar ->
5445
when {
@@ -63,16 +54,11 @@ internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) {
6354
try {
6455
read(embedded)
6556
} catch (retryError: IllegalArgumentException) {
66-
// the values that failed to convert likely hid the actual problem
6757
throw retryError.apply { addSuppressed(e) }
6858
}
6959
}
7060
}
7161

72-
/**
73-
* Returns a copy of the value in which the scalar values that [shouldEmbed] accepts are wrapped in [EmbeddedValue],
74-
* or null if it accepts none.
75-
*/
7662
private fun embed(value: Any, shouldEmbed: (Any) -> Boolean): Any? {
7763
var embedsAny = false
7864
fun copy(value: Any?): Any? = when (value) {
@@ -131,7 +117,6 @@ internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) {
131117
.withHandler(embeddedValueHandler)
132118
}
133119

134-
// written to the buffer as an embedded object, which it keeps as it is
135120
private class EmbeddedValue(private val value: Any) : JsonSerializable {
136121
override fun serialize(gen: JsonGenerator, serializers: SerializerProvider) = gen.writeEmbeddedObject(value)
137122

‎src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ internal class MethodFieldResolver(
4949
private val numberOfParameters = method.parameterCountWithoutContinuation()
5050
private val hasAdditionalParameter = numberOfParameters == (field.inputValueDefinitions.size + getIndexOffset() + 1)
5151

52-
// the arguments are converted with the same ObjectMapper that the UnmappedInputFieldChecker checks their classes with
5352
internal val objectMapper: ObjectMapper by lazy { options.objectMapperProvider.provide(field) }
5453

5554
override fun createDataFetcher(): DataFetcher<*> {
@@ -202,7 +201,6 @@ internal class MethodFieldResolver(
202201

203202
private fun getJavaMethodParameterIndex(index: Int) = index + getIndexOffset()
204203

205-
// the type that the value of the argument at the index is converted to
206204
internal fun getMethodParameterType(index: Int): JavaType? {
207205
val methodIndex = getJavaMethodParameterIndex(index)
208206
val parameters = method.parameterTypes

‎src/main/kotlin/graphql/kickstart/tools/util/Utils.kt‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@ internal fun Type<*>.unwrap(): Type<*> = when (this) {
3838
internal inline fun <reified T : TypeDefinition<*>> TypeExtensions.extensionsOf(name: String): List<T> =
3939
this[name].orEmpty().filterIsInstance<T>()
4040

41-
// The type followed by its extensions, e.g. `union.withExtensions(typeExtensions).flatMap { it.memberTypes }`. Every extension class
42-
// is a subclass of its type's class, e.g. UnionTypeExtensionDefinition of UnionTypeDefinition.
41+
// Every extension class is a subclass of its type's class, e.g. UnionTypeExtensionDefinition of UnionTypeDefinition.
4342
internal inline fun <reified T : TypeDefinition<*>> T.withExtensions(typeExtensions: TypeExtensions): List<T> =
4443
listOf(this) + typeExtensions.extensionsOf<T>(name)
4544

‎src/test/java17/graphql/kickstart/tools/SchemaClassScannerSealedJavaTypesTest.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import static org.junit.Assert.assertEquals;
1313
import static org.junit.Assert.assertTrue;
1414

15-
// Sealed classes need Java 17, so the java17-tests profile only compiles and runs this on Java 17 and later.
1615
public class SchemaClassScannerSealedJavaTypesTest {
1716

1817
@Test

0 commit comments

Comments
 (0)