diff --git a/.claude/skills/verify-return-value-unwrapping/SKILL.md b/.claude/skills/verify-return-value-unwrapping/SKILL.md index c9ed81c9..6030c798 100644 --- a/.claude/skills/verify-return-value-unwrapping/SKILL.md +++ b/.claude/skills/verify-return-value-unwrapping/SKILL.md @@ -1,15 +1,15 @@ --- name: verify-return-value-unwrapping -description: Exhaustively check, up to a bound, that a resolver method's return value reaches graphql-java fully unwrapped. Futures are awaited, generic wrappers are transformed, Optional and DataFetcherResult are handled, and list order, nulls and failures are kept, for every return type nested up to 3 containers deep. Use it before pushing any change to ReturnValueUnwrapper, TypeClassMatcher's unwrapping, GenericTypeResolver.resolve, the data fetchers in MethodFieldResolver, or the GenericWrapper defaults, and whenever a bug report says graphql-java got a CompletableFuture, Optional or wrapper object as a field value. +description: Exhaustively check, up to a bound, that the value of a resolver method, public field or typed Map reaches graphql-java fully unwrapped. Futures are awaited, generic wrappers are transformed, Optional and DataFetcherResult are handled, and list order, nulls and failures are kept, for every return type nested up to 3 containers deep. Use it before pushing any change to ReturnValueUnwrapper, TypeClassMatcher's unwrapping, GenericTypeResolver.resolve, the data fetchers in MethodFieldResolver, PropertyFieldResolver or MapFieldResolver, or the GenericWrapper defaults, and whenever a bug report says graphql-java got a CompletableFuture, Optional or wrapper object as a field value. --- # Verify return value unwrapping ## The property -A user writes a resolver method whose return type nests containers, such as -`CompletableFuture>>`. If the schema build accepts that method, then graphql-java must get -exactly the value the method returned, with every container removed. Futures are waited on and generic wrappers go +A user writes a resolver method, a public field or a `Map` whose type nests containers, such as +`CompletableFuture>>`. If the schema build accepts it, then graphql-java must get +exactly the value it returned, with every container removed. Futures are waited on and generic wrappers go through their transformers. List order and nulls are kept. A failed future or a throwing transformer fails the field. A `Future` that isn't a `CompletionStage` fails the field with the "can't wait on" error. @@ -36,10 +36,19 @@ The leaf is `Payload` (an object type), `String`, or `Object` for a union `Item implements `Future`, so only the declared type tells a leaf from a future. Each return type runs with an all-nullable and an all-non-null GraphQL type. -For each combination the check compiles a Java resolver class with that return type at runtime. It builds the schema -through the public `SchemaParser` API, which rejects about a quarter of the combinations, such as `Optional` below the top -level. For every accepted one it executes the query once for each value that makes at most `--budget` (default 2) -non-default choices. When the return type holds futures, it runs each value three ways: futures already completed, +Each return type runs as three kinds of resolver, each compiled as a Java class at runtime: + +| Kind | Generated resolver | Data fetcher | +|---|---|---| +| `method` | `public field()` on the query resolver | `MethodFieldResolver` | +| `property` | `public field;` on the query resolver, which is also what a Kotlin `@JvmField` compiles to | `PropertyFieldResolver` | +| `map` | `extends HashMap>`, bound to `type Holder` and returned by `Query.holder` | `MapFieldResolver` | + +The check builds each schema through the public `SchemaParser` API, which rejects about a fifth of the combinations, such as +`Optional` below the top level. For every accepted one it executes the query once for each value that makes at most +`--budget` (default 2) non-default choices. Properties and Maps use `--property-budget` (default 1), only the nullable +GraphQL type and only the first-created-first completion order, because what they add is how the value is read, and the +methods already cover the rest. When the return type holds futures, it runs each value three ways: futures already completed, and pending futures completed after the data fetcher returns, first-created first and last-created first. The expected result is what graphql-java returns when a plain data fetcher on the same schema returns the plain value, @@ -47,7 +56,7 @@ or throws the failure. So graphql-java's own null handling and error format appl holds several failures, any one of them failing the field counts as correct. `ReturnValueUnwrappingTest` compiles `Check.java` and `Fixtures.java` at test time and runs the check, so `mvn verify` -runs it at the default bounds on every build. About 172,000 executions run in about 15 seconds. +runs it at the default bounds on every build. About 226,000 executions run in about 18 seconds on JDK 17. ## Run @@ -63,13 +72,14 @@ as `-Dunwrapping.args`, and prints the report. On a failure it also prints the p | Exit | Meaning | |---|---| | 0 | `PASS`, or `PASS apart from the known findings above` | -| 1 | `FAIL`: a return type that isn't a known finding breaks the property | +| 1 | `FAIL`: a return type that isn't a known finding breaks the property. `NEW`: a known finding matches more accepted return types than its line records | | 2 | `MODEL ERROR`: the checkout or the check didn't compile, or the build rejected a return type the check must exercise, so a pass would prove nothing | -| 3 | `STALE`: a known finding no longer fails. Delete its line from `known-findings.txt` | +| 3 | `STALE`: a known finding matches a return type that no longer fails, or fewer accepted return types than its line records | -Options: `--depth N`, `--budget N`, `--show N` (counterexamples printed, default 15), and `--rejected`, which lists +Options: `--depth N`, `--budget N`, `--property-budget N`, `--show N` (counterexamples and stale return types printed, default 15), and `--rejected`, which lists every combination the schema build rejected, with the build's error. A depth below 3 can't build the `mustAccept` -return types, so it ends in `MODEL ERROR`. +return types, so it ends in `MODEL ERROR`. `NEW` and `STALE` apply only at the default depth and budgets, because the +counts in `known-findings.txt` are for those. ## Reading a counterexample @@ -81,7 +91,8 @@ public List> field() for field: [Payload] (21 failing execut got: {errors=[{message=Exception while fetching data (/field[1]/data) : Expected source object to be an instance of 'unwrapcheck.Fixtures$Payload' but instead got 'java.util.concurrent.FutureTask', ... ``` -The first line is the resolver method and the field's GraphQL type. `returns` is the value the method returned. +The first line is the resolver and the field's GraphQL type. A property prints as `public List> field;` +and a Map as `extends HashMap>> for Holder.field: [Payload]`. `returns` is the value the method returned. `Later(CompletableFuture(x))` means a `Later` whose transformer returns a `CompletableFuture` of `x`. Only the first failing value per return type is shown. Rerun with a higher `--show` to see more return types. @@ -96,7 +107,8 @@ A real bug versus a modeling error: - **Modeling error.** The check's own idea of the value is wrong. For example, a new container variant builds a value that its declared type doesn't allow, or the error message the check expects for a `FutureTask` (in `build`, case `FUTURE_TASK`) differs from a message the library changed on purpose. Confirm against the library's own tests in - `src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt` before you edit the check. + `src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt` before you edit the check. Properties and Maps + are expected to fail a `FutureTask` with the method's wording, naming the generated class and `field`. - **`MODEL ERROR` about rejected return types.** A change made the build reject a return type in `mustAccept` in `Check.java`. That's either a regression in the build, which you report, or an intended change, after which you swap in another accepted return type of the same kind. @@ -104,16 +116,30 @@ A real bug versus a modeling error: ## Known findings `src/test/resources/returnvalue/known-findings.txt` lists the return types that break the property at the sync commit, one per line as -` # `. They still -print as `KNOWN` with a counterexample on every run, and the check passes apart from them. A new break inside a known -pattern is hidden, so after a change to the code a known finding covers, read its counterexample. - -At the sync commit there are none. The check found two bugs when it was written, and both are fixed. One was a -`DataFetcherResult` inside an `Optional` (#939). The other was a wrapper whose transformer returns a future, directly around -a declared future (#938). A `withTransformer` registration keeps that #938 behavior, which is why `Later` is registered -through `withCompletionStageTransformer`. - -When a fix lands, the check exits 3. Delete the line then, so that a regression shows up as `FAIL`. +` # `. The regex is matched against the kind and the short return type, such as +`property List>`, so a pattern without a kind covers all three. The first matching line wins. +Matching return types still print as `KNOWN` with a counterexample on every run, and the check passes apart from them. + +Each line must match only return types that fail, and exactly `` of the return types the build accepts. A +matched return type that passes makes the check `STALE` and names it, so a partial fix has to narrow the line, and a later +regression in a fixed return type is a `FAIL`. A count that grows makes it `NEW`, because a return type the build used to +reject now fails under the line. What stays hidden is a known return type that starts failing in a different way, so +after a change to the code a known finding covers, read its counterexample. + +At the sync commit, methods have none. Properties and Maps hand their value to graphql-java as it is. graphql-java awaits +only a top-level `CompletionStage`, reads a `DataFetcherResult` at the top, inside that future and as a list element, and +unboxes any `Optional`, but not a `DataFetcherResult` inside one. So every nested future, every generic wrapper, a +`DataFetcherResult` inside an `Optional`, and a `FutureTask` behind a declared `Future` fail for them. The five lines +match exactly those 2,850 return types, and their counts add up to it. A String field gets the container's `toString`, +such as `CompletableFuture@5a7cb161[Completed normally]`, with no error. + +The check found two method bugs when it was written, and both are fixed. One was a `DataFetcherResult` inside an +`Optional` (#939). The other was a wrapper whose transformer returns a future, directly around a declared future (#938). A +`withTransformer` registration keeps that #938 behavior, which is why `Later` is registered through +`withCompletionStageTransformer`. + +When a fix lands, the check exits 3 and names the return types that pass. Narrow the line to the rest and set its count, +or delete it when none fail, so that a regression shows up as `FAIL`. ## What this does not cover @@ -126,10 +152,12 @@ When a fix lands, the check exits 3. Delete the line then, so that a regression - Lists of more than 2 elements, more than 2 non-default choices per value, more than 3 containers, and completion orders other than the two listed. A pass says nothing beyond those bounds. - Which return types the schema build should reject. The check only exercises what the build accepts. +- Untyped Maps, whose value type is `Object`, properties with a getter (those run as methods), and properties and Maps on + a type below the root other than `Holder`. `ResolverCompatibilityTest` pins untyped Maps and Kotlin `@JvmField`s. ## Coverage and sync -Last confirmed to describe the code at commit `cd3233ca` (master, 2026-10-30). It covers: +Last confirmed to describe the code at commit `af9ba4ad` (agent-friendly, 2026-11-04). It covers: | File | What | |---|---| @@ -138,11 +166,13 @@ Last confirmed to describe the code at commit `cd3233ca` (master, 2026-10-30). I | `src/main/kotlin/graphql/kickstart/tools/GenericTypeResolver.kt` | `resolve`, `resolveTypeVariables` | | `src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt` | `createDataFetcher` (`valueClass`, the unwrapper), `returnType`, `returnValueMatch`, `LightMethodFieldResolverDataFetcher` | | `src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt` | `GenericWrapper` and the default wrappers in `build()` | +| `src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt` | all of it | +| `src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt` | all of it, for Map subclasses with a typed value | To see whether a change needs another look: ```bash -git log --oneline cd3233ca..HEAD -- src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt src/main/kotlin/graphql/kickstart/tools/GenericTypeResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt +git log --oneline af9ba4ad..HEAD -- src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt src/main/kotlin/graphql/kickstart/tools/GenericTypeResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt ``` Move the sync commit forward only after you've read those changes and confirmed that the containers, variants and diff --git a/.claude/skills/verify-return-value-unwrapping/check.sh b/.claude/skills/verify-return-value-unwrapping/check.sh index 6f57e1b0..144ad2a3 100755 --- a/.claude/skills/verify-return-value-unwrapping/check.sh +++ b/.claude/skills/verify-return-value-unwrapping/check.sh @@ -15,7 +15,7 @@ fi sed -n '/AssertionError: /,/^\tat /p' "$log" | sed -e '1s/.*AssertionError: //' -e '$d' echo "Maven log: $log" -grep -q '^FAIL:' "$log" && exit 1 +grep -q -E '^(FAIL|NEW):' "$log" && exit 1 grep -q 'MODEL ERROR:' "$log" && exit 2 grep -q '^STALE:' "$log" && exit 3 tail -40 "$log" diff --git a/AGENTS.md b/AGENTS.md index f68d6de1..4899091f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,6 +82,7 @@ happening. "docs only" means nothing does yet, so the rule depends on you readin |---|---| | A resolver method's return value is unwrapped by one rule that the schema build and the data fetcher share | docs only | | A resolver method's parameter layout, type variables and source are decided once, at schema build | docs only | +| A change to how resolvers are found, bound or unwrapped keeps what a method, a property and a Map field return | `ResolverCompatibilityTest` and the bounded check in `ReturnValueUnwrappingTest`. Each lists exactly today's property and Map gaps as known findings, and fails when one is fixed until it's removed, or when a new failure would hide under one | | One output has one builder. Don't add a second path that builds the same thing | docs only | | Read an SDL type's members merged with its extensions, never from the raw definition | docs only | | Reuse an existing structure before you add a parallel one | docs only | diff --git a/src/test/kotlin/graphql/kickstart/tools/RawGenericFixtures.java b/src/test/kotlin/graphql/kickstart/tools/RawGenericFixtures.java index d01b3076..37d23d98 100644 --- a/src/test/kotlin/graphql/kickstart/tools/RawGenericFixtures.java +++ b/src/test/kotlin/graphql/kickstart/tools/RawGenericFixtures.java @@ -1,6 +1,7 @@ package graphql.kickstart.tools; import java.util.List; +import java.util.Optional; import java.util.concurrent.CompletableFuture; // Raw types can't be expressed in Kotlin, so these fixtures must stay in Java. @@ -14,6 +15,14 @@ public Tree tree() { } } + public static class RawOptionalResolver implements GraphQLQueryResolver { + + @SuppressWarnings({"rawtypes", "unused"}) + public String raw(Optional value) { + return String.valueOf(value); + } + } + public static class Tree { private final T value; diff --git a/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt new file mode 100644 index 00000000..40b0d1d4 --- /dev/null +++ b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt @@ -0,0 +1,425 @@ +package graphql.kickstart.tools + +import graphql.ExecutionInput +import graphql.ExecutionResult +import graphql.GraphQL +import graphql.GraphQLContext +import graphql.execution.DataFetcherResult +import graphql.kickstart.tools.SchemaParserOptions.GenericWrapper +import graphql.schema.DataFetchingEnvironment +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.reactive.asFlow +import kotlinx.coroutines.reactive.publish +import kotlinx.coroutines.runBlocking +import org.junit.Assert.fail +import org.junit.Test +import org.reactivestreams.Publisher +import java.util.* +import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletableFuture.completedFuture +import java.util.concurrent.CompletableFuture.supplyAsync +import java.util.concurrent.Future +import java.util.concurrent.FutureTask + +class ResolverCompatibilityTest { + + @Test + fun `@JvmField and typed Map fields return what a method returns apart from the known findings, and untyped Map fields hand graphql-java the value as it is`() { + val wrong = Kind.entries.flatMap { kind -> + shapes.mapNotNull { shape -> + val got = holderField(kind, shape) + val expected = if (kind == Kind.UNTYPED_MAP) rawValueResults.getValue(shape.name) else shape.expected(kind) + val known = knownFindings[kind to shape.name] + when { + got == (known ?: expected) -> null + known != null && got == expected -> "$kind ${shape.name} now returns what a method returns. Delete its known finding." + else -> "$kind ${shape.name}\n expected: ${known ?: expected}\n got: $got" + } + } + } + if (wrong.isNotEmpty()) { + fail(wrong.joinToString("\n")) + } + } + + @Test + fun `arguments follow the source parameter in declaration order`() { + val result = execute( + """ + type Query { foo: Foo } + type Foo { name: String joined(first: String!, second: String!, count: Int!): String } + """, + "{ foo { joined(first: \"x\", second: \"y\", count: 3) } }", + object : GraphQLQueryResolver { + fun foo() = Foo("a") + }, + object : GraphQLResolver { + fun joined(foo: Foo, first: String, second: String, count: Int, env: DataFetchingEnvironment) = + "${foo.name} $first $second $count ${env.field.name}" + } + ) + + assertEquals(result, "{foo={joined=a x y 3 joined}}") + } + + @Test + fun `trailing parameters bind the GraphQLContext, the context class and the environment, repeated and in any order`() { + val schema = "type Query { extras(first: String!): String }" + val options = SchemaParserOptions.newOptions().contextClass(AppContext::class).build() + + assertEquals( + execute(schema, "{ extras(first: \"x\") }", ContextResolver(), options = options, context = mapOf(AppContext::class.java to AppContext("ann"), "key" to "k")), + "{extras=x extras ann k true}" + ) + assertEquals( + execute(schema, "{ extras(first: \"x\") }", ContextResolver(), options = options), + "{extras=null}\nerror: Exception while fetching data (/extras) : Resolver method 'ContextResolver.extras' has a parameter of the context class " + + "'AppContext', but the GraphQLContext has no value for it. Put it in the GraphQLContext under the class key when building the " + + "ExecutionInput, for example graphQLContext(mapOf(AppContext::class.java to context))." + ) + } + + @Test + fun `a context class of DataFetchingEnvironment is looked up in the GraphQLContext`() { + val result = execute( + "type Query { environment: String }", + "{ environment }", + EnvironmentResolver(), + options = SchemaParserOptions.newOptions().contextClass(DataFetchingEnvironment::class).build() + ) + + assertEquals( + result, + "{environment=null}\nerror: Exception while fetching data (/environment) : Resolver method 'EnvironmentResolver.environment' has a parameter " + + "of the context class 'graphql.schema.DataFetchingEnvironment', but the GraphQLContext has no value for it. Put it in the GraphQLContext " + + "under the class key when building the ExecutionInput, for example graphQLContext(mapOf(DataFetchingEnvironment::class.java to context))." + ) + } + + @Test + fun `omitted arguments reach primitive parameters as their default, and a type variable parameter gets a value of its resolved type argument`() { + val result = execute( + "type Query { primitives(i: Int, b: Boolean, d: Float, boxed: Int): String echo(value: Int!): String }", + "{ primitives echo(value: 3) }", + PrimitivesResolver() + ) + + assertEquals(result, "{primitives=0 false 0.0 null, echo=3 Long}") + } + + @Test + fun `Optional arguments are empty when null, and null when omitted with omission detection`() { + val schema = "type Query { raw(value: String): String typed(value: String): String }" + val query = "{ givenRaw: raw(value: \"x\") nullRaw: raw(value: null) omittedRaw: raw given: typed(value: \"x\") null: typed(value: null) omitted: typed }" + val typed = object : GraphQLQueryResolver { + fun typed(value: Optional?) = value.toString() + } + val detecting = SchemaParserOptions.newOptions().inputArgumentOptionalDetectOmission(true).build() + + assertEquals( + execute(schema, query, RawGenericFixtures.RawOptionalResolver(), typed), + "{givenRaw=Optional[x], nullRaw=Optional.empty, omittedRaw=Optional.empty, given=Optional[x], null=Optional.empty, omitted=Optional.empty}" + ) + assertEquals( + execute(schema, query, RawGenericFixtures.RawOptionalResolver(), typed, options = detecting), + "{givenRaw=Optional[x], nullRaw=Optional.empty, omittedRaw=null, given=Optional[x], null=Optional.empty, omitted=null}" + ) + } + + @Test + fun `lists of concrete scalars skip conversion only for a List parameter`() { + val result = execute( + "type Query { numbers(list: [Int!]!, collection: [Int!]!, set: [Int!]!): String }", + "{ numbers(list: [1], collection: [2], set: [3]) }", + object : GraphQLQueryResolver { + fun numbers(list: List, collection: Collection, set: Set) = + listOf>(list, collection, set).joinToString(" ") { it.first()!!.javaClass.simpleName } + } + ) + + assertEquals(result, "{numbers=Integer Long Long}") + } + + @Test + fun `a source of another class fails the field`() { + val result = execute( + "type Query { holder: Holder } type Holder { name: String tag: String extra: String }", + "{ holder { name tag extra } }", + object : GraphQLQueryResolver { + fun holder(): Any = Impostor() + }, + object : GraphQLResolver { + fun extra(holder: Holder) = holder.name + }, + dictionary = listOf(Holder::class.java) + ) + + assertEquals( + result, + """ + {holder={name=null, tag=null, extra=null}} + error: Exception while fetching data (/holder/name) : Expected source object to be an instance of 'Holder' but instead got 'Impostor' + error: Exception while fetching data (/holder/tag) : Expected source object to be an instance of 'Holder' but instead got 'Impostor' + error: Exception while fetching data (/holder/extra) : Source type (Impostor) is not expected type (Holder)! + """.trimIndent() + ) + } + + @Test + fun `the outermost wrapper is picked by the value's class and a nested one by the declared type`() { + val result = execute( + "type Query { top: String nested: [String] }", + "{ top nested }", + object : GraphQLQueryResolver { + fun top(): Box = SpecialBox("a") + fun nested(): List> = listOf(SpecialBox("b")) + }, + options = SchemaParserOptions.newOptions().genericWrappers(boxWrapper, specialBoxWrapper).build() + ) + + assertEquals(result, "{top=a special, nested=[b]}") + } + + @Test + fun `a value of two unrelated wrapper types gets the transformer registered last, whatever the declared type`() { + val result = execute( + "type Query { left: String right: String }", + "{ left right }", + object : GraphQLQueryResolver { + fun left(): Left = Both("l", "r") + fun right(): Right = Both("l", "r") + }, + options = SchemaParserOptions.newOptions().genericWrappers(leftWrapper, rightWrapper).build() + ) + + assertEquals(result, "{left=r, right=r}") + } + + @Test + fun `suspend functions await nested futures, and fail on a future at the top`() { + val result = execute("type Query { futures: [String] future: String }", "{ futures future }", SuspendResolver()) + + assertEquals( + result, + "{futures=[futures, b], future=null}\nerror: Exception while fetching data (/future) : Suspend function 'SuspendResolver.future' resolved to " + + "a CompletableFuture, which graphql-java can't wait on. Return its value instead." + ) + } + + @Test + fun `subscription events reach graphql-java as they are, so a future event prints as a string`() { + val schema = SchemaParser.newParser() + .schemaString("type Query { unused: String } type Subscription { events: String futureEvents: String }") + .resolvers( + object : GraphQLQueryResolver { + fun unused(): String? = null + }, + object : GraphQLSubscriptionResolver { + fun events(): Publisher = publish { + send("a") + send("b") + } + + fun futureEvents(): Publisher> = publish { + send(completedFuture("a")) + } + }) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + fun events(query: String) = runBlocking { gql.execute(query).getData>()!!.asFlow().toList() }.map { it.render() } + + assertEquals(events("subscription { events }"), listOf("{events=a}", "{events=b}")) + assertEquals(events("subscription { futureEvents }"), listOf("{futureEvents=CompletableFuture@...[Completed normally]}")) + } + + private fun execute( + schema: String, + query: String, + vararg resolvers: GraphQLResolver<*>, + options: SchemaParserOptions = SchemaParserOptions.newOptions().build(), + dictionary: List> = listOf(), + context: Map = mapOf() + ): String = GraphQL.newGraphQL( + SchemaParser.newParser().schemaString(schema).resolvers(*resolvers).options(options).dictionary(dictionary).build().makeExecutableSchema() + ).build().execute(ExecutionInput.newExecutionInput(query).graphQLContext(context)).render() + + private fun holderField(kind: Kind, shape: Shape): String { + val holder = kind.holder(shape) + val parser = SchemaParser.newParser() + .schemaString("type Query { holder: Holder } type Holder { ${shape.name}: ${shape.type} }" + if (shape.selection.isEmpty()) "" else " type Foo { name: String }") + .resolvers(object : GraphQLQueryResolver { + fun holder(): Any = holder + }) + .dictionary("Holder", holder.javaClass) + .options(SchemaParserOptions.newOptions().genericWrappers(boxWrapper).build()) + if (kind == Kind.UNTYPED_MAP && shape.selection.isNotEmpty()) { + parser.dictionary(Foo::class) + } + val schema = try { + parser.build().makeExecutableSchema() + } catch (e: RuntimeException) { + return "rejected: ${e.message!!.lines().first().substringAfterLast("): ")}" + } + val result = GraphQL.newGraphQL(schema).build().execute("{ holder { ${shape.name}${shape.selection} } }") + val value = result.getData?>>()!!["holder"]?.get(shape.name) + return (listOf("$value") + result.errors.map { "error: ${it.message}" }).joinToString("\n").normalized() + } + + private fun ExecutionResult.render() = (listOf("${getData()}") + errors.map { "error: ${it.message}" }).joinToString("\n").normalized() + + private fun String.normalized() = replace(Regex("@\\p{XDigit}+"), "@...") + .replace("${ResolverCompatibilityTest::class.java.name}$", "") + .replace("java.util.concurrent.", "") + + enum class Kind(val holder: (Shape) -> Any) { + METHOD({ MethodHolder() }), + JVM_FIELD({ JvmFieldHolder() }), + TYPED_MAP({ shape -> shape.typedMap(JvmFieldHolder()) }), + UNTYPED_MAP({ shape -> hashMapOf(shape.name to shape.value()) }) + } + + class Shape(val name: String, val type: String, private val expected: String, val typedMap: (JvmFieldHolder) -> Map) { + val selection = if (type.contains("Foo")) " { name }" else "" + + fun value(): Any? = JvmFieldHolder::class.java.getField(name).get(JvmFieldHolder()) + + fun expected(kind: Kind) = expected.replace("HOLDER", kind.holder(this).javaClass.simpleName) + } + + class Foo(val name: String) + + class JvmFieldHolder { + @JvmField val future: CompletableFuture = completedFuture(Foo("a")) + @JvmField val futureOfListOfFutures: CompletableFuture>> = completedFuture(listOf(completedFuture(Foo("a")), completedFuture(Foo("b")))) + @JvmField val optional: Optional = Optional.of(Foo("a")) + @JvmField val result: DataFetcherResult = DataFetcherResult.newResult().data(Foo("a")).build() + @JvmField val plainFuture: Future = FutureTask { Foo("a") }.apply { run() } + @JvmField val box: Box = Box(Foo("a")) + @JvmField val optionalResult: Optional> = Optional.of(DataFetcherResult.newResult().data(Foo("a")).build()) + @JvmField val listOfFutures: List> = listOf(completedFuture(Foo("a")), completedFuture(Foo("b"))) + @JvmField val listOfStringFutures: List> = listOf(completedFuture("a"), completedFuture("b")) + @JvmField val boxOfString: Box = Box("a") + @JvmField val listOfOptionals: List> = listOf(Optional.of("a"), Optional.empty()) + } + + class MethodHolder { + private val values = JvmFieldHolder() + + fun future() = values.future + fun futureOfListOfFutures() = values.futureOfListOfFutures + fun optional() = values.optional + fun result() = values.result + fun plainFuture() = values.plainFuture + fun box() = values.box + fun optionalResult() = values.optionalResult + fun listOfFutures() = values.listOfFutures + fun listOfStringFutures() = values.listOfStringFutures + fun boxOfString() = values.boxOfString + fun listOfOptionals() = values.listOfOptionals + } + + class FutureMap : HashMap>() + class FutureOfListOfFuturesMap : HashMap>>>() + class OptionalMap : HashMap>() + class ResultMap : HashMap>() + class PlainFutureMap : HashMap>() + class BoxMap : HashMap>() + class OptionalResultMap : HashMap>>() + class ListOfFuturesMap : HashMap>>() + class ListOfStringFuturesMap : HashMap>>() + class BoxOfStringMap : HashMap>() + class ListOfOptionalsMap : HashMap>>() + + open class Box(val content: T) + + class SpecialBox(content: T) : Box(content) + + interface Left { + val left: T + } + + interface Right { + val right: T + } + + class Both(override val left: T, override val right: T) : Left, Right + + class AppContext(val user: String) + + class ContextResolver : GraphQLQueryResolver { + fun extras(first: String, env: DataFetchingEnvironment, context: AppContext, graphQLContext: GraphQLContext, again: DataFetchingEnvironment) = + "$first ${env.field.name} ${context.user} ${graphQLContext.get("key")} ${again === env}" + } + + class EnvironmentResolver : GraphQLQueryResolver { + fun environment(env: DataFetchingEnvironment): String = env.field.name + } + + abstract class Echo { + fun echo(value: T) = "$value ${value!!::class.java.simpleName}" + } + + class PrimitivesResolver : Echo(), GraphQLQueryResolver { + fun primitives(i: Int, b: Boolean, d: Double, boxed: Int?) = "$i $b $d $boxed" + } + + class SuspendResolver : GraphQLQueryResolver { + suspend fun futures(env: DataFetchingEnvironment): List> = listOf(completedFuture(env.field.name), supplyAsync { "b" }) + suspend fun future(): CompletableFuture = completedFuture("a") + } + + class Holder(val name: String) { + @JvmField val tag = "t" + } + + class Impostor + + private companion object { + val boxWrapper = GenericWrapper.withTransformer(Box::class, 0, { box -> box.content }) + val specialBoxWrapper = GenericWrapper.withTransformer(SpecialBox::class, 0, { box -> "${box.content} special" }) + val leftWrapper = GenericWrapper.withTransformer(Left::class, 0, { wrapper -> wrapper.left }) + val rightWrapper = GenericWrapper.withTransformer(Right::class, 0, { wrapper -> wrapper.right }) + + val shapes = listOf( + Shape("future", "Foo", "{name=a}") { FutureMap().apply { put("future", it.future) } }, + Shape("futureOfListOfFutures", "[Foo]", "[{name=a}, {name=b}]") { FutureOfListOfFuturesMap().apply { put("futureOfListOfFutures", it.futureOfListOfFutures) } }, + Shape("optional", "Foo", "{name=a}") { OptionalMap().apply { put("optional", it.optional) } }, + Shape("result", "Foo", "{name=a}") { ResultMap().apply { put("result", it.result) } }, + Shape( + "plainFuture", "Foo", + "null\nerror: Exception while fetching data (/holder/plainFuture) : Resolver method 'HOLDER.plainFuture' resolved to a FutureTask, " + + "which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead." + ) { PlainFutureMap().apply { put("plainFuture", it.plainFuture) } }, + Shape("box", "Foo", "{name=a}") { BoxMap().apply { put("box", it.box) } }, + Shape("optionalResult", "Foo", "{name=a}") { OptionalResultMap().apply { put("optionalResult", it.optionalResult) } }, + Shape("listOfFutures", "[Foo]", "[{name=a}, {name=b}]") { ListOfFuturesMap().apply { put("listOfFutures", it.listOfFutures) } }, + Shape("listOfStringFutures", "[String]", "[a, b]") { ListOfStringFuturesMap().apply { put("listOfStringFutures", it.listOfStringFutures) } }, + Shape("boxOfString", "String", "a") { BoxOfStringMap().apply { put("boxOfString", it.boxOfString) } }, + Shape("listOfOptionals", "[String]", "rejected: java.util.Optional can only be used at the top level of a return type") { ListOfOptionalsMap().apply { put("listOfOptionals", it.listOfOptionals) } } + ) + + private fun notFoo(path: String, got: String) = + "error: Exception while fetching data (/holder/$path/name) : Expected source object to be an instance of 'Foo' but instead got '$got'" + + val rawValueResults = mapOf( + "future" to "{name=a}", + "futureOfListOfFutures" to "[{name=null}, {name=null}]\n${notFoo("futureOfListOfFutures[0]", "CompletableFuture")}\n${notFoo("futureOfListOfFutures[1]", "CompletableFuture")}", + "optional" to "{name=a}", + "result" to "{name=a}", + "plainFuture" to "{name=null}\n${notFoo("plainFuture", "FutureTask")}", + "box" to "{name=null}\n${notFoo("box", "Box")}", + "optionalResult" to "{name=null}\n${notFoo("optionalResult", "graphql.execution.DataFetcherResult")}", + "listOfFutures" to "[{name=null}, {name=null}]\n${notFoo("listOfFutures[0]", "CompletableFuture")}\n${notFoo("listOfFutures[1]", "CompletableFuture")}", + "listOfStringFutures" to "[CompletableFuture@...[Completed normally], CompletableFuture@...[Completed normally]]", + "boxOfString" to "Box@...", + "listOfOptionals" to "[a, null]" + ) + + val knownFindings = listOf(Kind.JVM_FIELD, Kind.TYPED_MAP).flatMap { kind -> + listOf("futureOfListOfFutures", "plainFuture", "box", "optionalResult", "listOfFutures", "listOfStringFutures", "boxOfString") + .map { (kind to it) to rawValueResults.getValue(it) } + }.toMap() + } +} diff --git a/src/test/resources/returnvalue/Check.java b/src/test/resources/returnvalue/Check.java index c801ac13..fa7ede26 100644 --- a/src/test/resources/returnvalue/Check.java +++ b/src/test/resources/returnvalue/Check.java @@ -30,10 +30,12 @@ import java.io.ByteArrayOutputStream; import java.io.OutputStream; import java.io.PrintStream; +import java.lang.reflect.Field; import java.net.URI; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; @@ -48,6 +50,8 @@ import java.util.concurrent.FutureTask; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.function.Consumer; +import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -96,6 +100,22 @@ enum Leaf { } } + enum Kind { + METHOD("method", List.of(Nullability.values()), List.of(Timing.values())), + PROPERTY("property", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)), + MAP("map", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)); + + final String label; + final List nullabilities; + final List timings; + + Kind(String label, List nullabilities, List timings) { + this.label = label; + this.nullabilities = nullabilities; + this.timings = timings; + } + } + enum Nullability {NULLABLE, NON_NULL} enum Variant {VALUE, NULL, EMPTY, FAILED, FUTURE_TASK, WITH_ERROR, FUTURE_ITEM} @@ -115,12 +135,14 @@ enum Timing { static final class Case { final List shape; final Leaf leaf; + final Kind kind; final Nullability nullability; final String className; - Case(List shape, Leaf leaf, Nullability nullability, String className) { + Case(List shape, Leaf leaf, Kind kind, Nullability nullability, String className) { this.shape = shape; this.leaf = leaf; + this.kind = kind; this.nullability = nullability; this.className = className; } @@ -147,8 +169,28 @@ String shortJavaType() { return javaType().replace("unwrapcheck.Fixtures.", "").replaceAll("[a-z.]+\\.(?=[A-Z])", ""); } + String label() { + return kind.label + " " + shortJavaType(); + } + String describe() { - return "public " + shortJavaType() + " field() for field: " + graphQLType(); + switch (kind) { + case METHOD: + return "public " + shortJavaType() + " field() for field: " + graphQLType(); + case PROPERTY: + return "public " + shortJavaType() + " field; for field: " + graphQLType(); + default: + return "extends HashMap for Holder.field: " + graphQLType(); + } + } + + String parentType() { + return kind == Kind.MAP ? "Holder" : "Query"; + } + + String query() { + String field = "field" + leaf.selection; + return "query { " + (kind == Kind.MAP ? "holder { " + field + " }" : field) + " }"; } } @@ -220,6 +262,23 @@ static final class Violation { } } + static final class KnownFinding { + final int returnTypes; + final Pattern pattern; + final String description; + final Set covered = new LinkedHashSet<>(); + + KnownFinding(int returnTypes, Pattern pattern, String description) { + this.returnTypes = returnTypes; + this.pattern = pattern; + this.description = description; + } + } + + private static final int DEPTH = 3; + private static final int BUDGET = 2; + private static final int PROPERTY_BUDGET = 1; + private static final String KNOWN_FILE = "src/test/resources/returnvalue/known-findings.txt"; private static final String BLOCKED = "the data fetcher blocked until they completed"; private static volatile Object oracleValue; @@ -247,8 +306,10 @@ static final class Violation { ); public static boolean check(String[] args, String classpath, PrintStream out) throws Exception { - int depth = intArg(args, "--depth", 3); - int budget = intArg(args, "--budget", 2); + int depth = intArg(args, "--depth", DEPTH); + int budget = intArg(args, "--budget", BUDGET); + int propertyBudget = intArg(args, "--property-budget", PROPERTY_BUDGET); + int show = intArg(args, "--show", 15); long started = System.nanoTime(); List cases = cases(depth); @@ -258,11 +319,14 @@ public static boolean check(String[] args, String classpath, PrintStream out) th int runs = 0; boolean blocked = false; Set accepted = new LinkedHashSet<>(); + Set acceptedReturnTypes = new LinkedHashSet<>(); Map> violations = new LinkedHashMap<>(); for (Case testCase : cases) { + Class generated = resolvers.get(testCase.className); + Object instance = generated.getDeclaredConstructor().newInstance(); GraphQLSchema schema; try { - schema = schema(testCase, resolvers.get(testCase.className).getDeclaredConstructor().newInstance()); + schema = schema(testCase, instance); } catch (RuntimeException e) { rejected++; if (flag(args, "--rejected")) { @@ -270,16 +334,17 @@ public static boolean check(String[] args, String classpath, PrintStream out) th } continue; } - accepted.add(testCase.javaType() + " " + testCase.graphQLType()); + accepted.add(testCase.kind.label + " " + testCase.javaType() + " " + testCase.graphQLType()); + acceptedReturnTypes.add(testCase.label()); GraphQL actual = GraphQL.newGraphQL(schema).build(); - GraphQL oracle = GraphQL.newGraphQL(oracleSchema(schema)).build(); - String query = "query { field" + testCase.leaf.selection + " }"; - List timings = testCase.shape.stream().anyMatch(Container::isFuture) && !blocked ? List.of(Timing.values()) : List.of(Timing.DONE); + GraphQL oracle = GraphQL.newGraphQL(oracleSchema(schema, testCase.parentType())).build(); + Consumer returning = returning(testCase, generated, instance); + List timings = testCase.shape.stream().anyMatch(Container::isFuture) && !blocked ? testCase.kind.timings : List.of(Timing.DONE); plans: - for (Plan plan : plansWithinBudget(testCase, 0, budget)) { + for (Plan plan : plansWithinBudget(testCase, 0, testCase.kind == Kind.METHOD ? budget : propertyBudget)) { for (Timing timing : timings) { runs++; - Violation violation = run(testCase, plan, timing, actual, oracle, query); + Violation violation = run(testCase, plan, timing, actual, oracle, returning); if (violation != null) { violations.computeIfAbsent(testCase, c -> new ArrayList<>()).add(violation); if (violation.actual.equals(BLOCKED)) { @@ -291,62 +356,108 @@ public static boolean check(String[] args, String classpath, PrintStream out) th } } - List missing = mustAccept.stream().filter(it -> !accepted.contains(it)).collect(Collectors.toList()); - out.printf("depth %d, budget %d: %d return types, %d rejected by the schema build, %d accepted, %d executions, %.1fs%n", - depth, budget, cases.size(), rejected, accepted.size(), runs, (System.nanoTime() - started) / 1e9); + List missing = Arrays.stream(Kind.values()) + .flatMap(kind -> mustAccept.stream() + .filter(it -> kind.nullabilities.contains(Nullability.NON_NULL) || !it.endsWith("!")) + .map(it -> kind.label + " " + it)) + .filter(it -> !accepted.contains(it)) + .collect(Collectors.toList()); + out.printf("depth %d, budget %d (%d for properties and Maps): %d return types, %d rejected by the schema build, %d accepted, %d executions, %.1fs%n", + depth, budget, propertyBudget, cases.size(), rejected, accepted.size(), runs, (System.nanoTime() - started) / 1e9); if (blocked) { out.println("A data fetcher blocked on a pending future, so later return types ran with completed futures only."); } - Map known = knownFindings(stringArg(args, "--known")); - Map> knownFailing = new LinkedHashMap<>(); - known.keySet().forEach(pattern -> knownFailing.put(pattern, new ArrayList<>())); + Map> failing = new LinkedHashMap<>(); + violations.forEach((testCase, list) -> failing.computeIfAbsent(testCase.label(), it -> new ArrayList<>()).addAll(list)); + List known = knownFindings(stringArg(args, "--known")); List> unknown = new ArrayList<>(); - violations.forEach((testCase, list) -> { - String pattern = known.keySet().stream().filter(it -> Pattern.compile(it).matcher(testCase.shortJavaType()).find()).findFirst().orElse(null); - if (pattern == null) { - unknown.add(list); - } else { - knownFailing.get(pattern).add(testCase); + for (String returnType : acceptedReturnTypes) { + Optional finding = known.stream().filter(it -> it.pattern.matcher(returnType).find()).findFirst(); + if (finding.isPresent()) { + finding.get().covered.add(returnType); + } else if (failing.containsKey(returnType)) { + unknown.add(failing.get(returnType)); } - }); + } - knownFailing.forEach((pattern, failing) -> { - if (!failing.isEmpty()) { - out.printf("%nKNOWN (%s): %d return types still fail, e.g.%n", known.get(pattern), failing.size()); - print(out, violations.get(failing.get(0)).get(0), violations.get(failing.get(0)).size()); + for (KnownFinding finding : known) { + List stillFailing = finding.covered.stream().filter(failing::containsKey).collect(Collectors.toList()); + if (!stillFailing.isEmpty()) { + out.printf("%nKNOWN (%s): %d return types still fail, e.g.%n", finding.description, stillFailing.size()); + List first = failing.get(stillFailing.get(0)); + print(out, first.get(0), first.size()); } - }); + } + boolean passed = true; if (!unknown.isEmpty()) { int total = unknown.stream().mapToInt(List::size).sum(); out.printf("%nFAIL: %d executions in %d return types gave graphql-java something other than the value the resolver returned.%n", total, unknown.size()); - unknown.stream().limit(intArg(args, "--show", 15)).forEach(list -> print(out, list.get(0), list.size())); - return false; + unknown.stream().limit(show).forEach(list -> print(out, list.get(0), list.size())); + passed = false; } if (!missing.isEmpty() || runs == 0) { out.println("\nMODEL ERROR: the schema build rejected return types the check must exercise, so a pass would prove nothing:"); missing.forEach(it -> out.println(" " + it)); - return false; + passed = false; } - List fixed = knownFailing.entrySet().stream().filter(it -> it.getValue().isEmpty()).map(it -> known.get(it.getKey())).collect(Collectors.toList()); - if (!fixed.isEmpty()) { - out.println("\nSTALE: these known findings no longer fail. Delete their lines from src/test/resources/returnvalue/known-findings.txt so a regression shows up as FAIL:"); - fixed.forEach(it -> out.println(" " + it)); - return false; + if (depth == DEPTH && budget == BUDGET && propertyBudget == PROPERTY_BUDGET) { + passed &= knownFindingsAreExact(known, failing.keySet(), show, out); } - out.println(violations.isEmpty() ? "PASS" : "\nPASS apart from the known findings above"); - return true; + if (passed) { + out.println(violations.isEmpty() ? "PASS" : "\nPASS apart from the known findings above"); + } + return passed; } - private static Map knownFindings(String path) throws Exception { - Map known = new LinkedHashMap<>(); + private static boolean knownFindingsAreExact(List known, Set failing, int show, PrintStream out) { + List stale = new ArrayList<>(); + List added = new ArrayList<>(); + for (KnownFinding finding : known) { + int covered = finding.covered.size(); + List passing = finding.covered.stream().filter(it -> !failing.contains(it)).collect(Collectors.toList()); + if (passing.size() == covered) { + stale.add(" " + finding.description + ": none of the " + covered + " return types it matches fail. Delete the line."); + } else if (!passing.isEmpty()) { + stale.add(" " + finding.description + ": " + passing.size() + " of the " + covered + " return types it matches pass. Narrow the line to the " + + (covered - passing.size()) + " that fail and set its count to that:"); + passing.stream().limit(show).forEach(it -> stale.add(" " + it)); + if (passing.size() > show) { + stale.add(" and " + (passing.size() - show) + " more, rerun with --show " + passing.size()); + } + } + String counts = " " + finding.description + ": the line records " + finding.returnTypes + " return types and matches " + covered + " that the schema build accepts."; + if (covered > finding.returnTypes) { + added.add(counts); + } else if (covered < finding.returnTypes) { + stale.add(counts + " Unless you just narrowed the line, the build now rejects a return type it used to accept. " + + "Find it by diffing --rejected output against the last commit, then set the count to " + covered + "."); + } + } + if (!added.isEmpty()) { + out.println("\nNEW: known findings in " + KNOWN_FILE + " match more return types than they record, so a new failure would print as KNOWN, not FAIL. " + + "Unless you just widened a line, the build now accepts a return type it used to reject, and that return type fails. " + + "Find it by diffing --rejected output against the last commit, and treat it as a FAIL:"); + added.forEach(out::println); + } + if (!stale.isEmpty()) { + out.println("\nSTALE: known findings in " + KNOWN_FILE + " no longer match exactly the return types that fail, so a regression in a fixed one would print as KNOWN, not FAIL:"); + stale.forEach(out::println); + } + return added.isEmpty() && stale.isEmpty(); + } + + private static List knownFindings(String path) throws Exception { + Pattern format = Pattern.compile("(\\d+) (.+?) # (.+)"); + List known = new ArrayList<>(); for (String line : Files.readAllLines(Path.of(path))) { if (!line.isBlank() && !line.startsWith("#")) { - int separator = line.indexOf(" # "); - if (separator < 0) { - throw new IllegalArgumentException("A known finding is \" # \", not: " + line); + Matcher parts = format.matcher(line.trim()); + if (!parts.matches()) { + throw new IllegalArgumentException("A known finding is \" >> # \", not: " + line); } - known.put(line.substring(0, separator).trim(), line.substring(separator + 3).trim()); + known.add(new KnownFinding(Integer.parseInt(parts.group(1)), Pattern.compile(parts.group(2)), parts.group(3))); } } return known; @@ -381,9 +492,11 @@ static List cases(int depth) { List cases = new ArrayList<>(); for (int i = 0; i < shapes.size(); i++) { for (Leaf leaf : Leaf.values()) { - String className = "unwrapcheck.gen.R" + i + "_" + leaf.ordinal(); - for (Nullability nullability : Nullability.values()) { - cases.add(new Case(shapes.get(i), leaf, nullability, className)); + for (Kind kind : Kind.values()) { + String className = "unwrapcheck.gen.R" + i + "_" + leaf.ordinal() + "_" + kind.ordinal(); + for (Nullability nullability : kind.nullabilities) { + cases.add(new Case(shapes.get(i), leaf, kind, nullability, className)); + } } } } @@ -514,7 +627,7 @@ private static DataFetcherResult result(Object data, String error) { return builder.build(); } - static Violation run(Case testCase, Plan plan, Timing timing, GraphQL actual, GraphQL oracle, String query) { + static Violation run(Case testCase, Plan plan, Timing timing, GraphQL actual, GraphQL oracle, Consumer returning) { Context context = new Context(timing, testCase.className + ".field"); Built built = build(testCase, plan, 0, context); if (timing == Timing.PENDING_REVERSED) { @@ -522,7 +635,8 @@ static Violation run(Case testCase, Plan plan, Timing timing, GraphQL actual, Gr } String got; - Fixtures.nextReturnValue = built.returned; + String query = testCase.query(); + returning.accept(built.returned); java.util.concurrent.Future> fetch = fetching.submit(() -> actual.executeAsync(ExecutionInput.newExecutionInput().query(query).build())); CompletableFuture result; try { @@ -555,9 +669,40 @@ static Violation run(Case testCase, Plan plan, Timing timing, GraphQL actual, Gr return acceptable.contains(got) ? null : new Violation(testCase, built.text, timing, String.join("\n or ", acceptable), got); } - static GraphQLSchema schema(Case testCase, Object resolver) { - String sdl = "type Query { field: " + testCase.graphQLType() + " }\n"; - SchemaParserBuilder parser = SchemaParser.newParser().resolvers((GraphQLResolver) resolver).options(options); + static Consumer returning(Case testCase, Class generated, Object instance) throws Exception { + switch (testCase.kind) { + case METHOD: + return value -> Fixtures.nextReturnValue = value; + case PROPERTY: { + Field field = generated.getField("field"); + return value -> { + try { + field.set(instance, value); + } catch (IllegalAccessException e) { + throw new IllegalStateException(e); + } + }; + } + default: + return value -> { + @SuppressWarnings("unchecked") + Map holder = (Map) instance; + holder.put("field", value); + Fixtures.nextReturnValue = holder; + }; + } + } + + static GraphQLSchema schema(Case testCase, Object generated) { + SchemaParserBuilder parser = SchemaParser.newParser().options(options); + String sdl; + if (testCase.kind == Kind.MAP) { + sdl = "type Query { holder: Holder }\ntype Holder { field: " + testCase.graphQLType() + " }\n"; + parser.resolvers(new Fixtures.HolderQuery()).dictionary("Holder", generated.getClass()); + } else { + sdl = "type Query { field: " + testCase.graphQLType() + " }\n"; + parser.resolvers((GraphQLResolver) generated); + } if (testCase.leaf != Leaf.STRING) { sdl += "type Payload { data: String }\n"; } @@ -568,7 +713,7 @@ static GraphQLSchema schema(Case testCase, Object resolver) { return parser.schemaString(sdl).build().makeExecutableSchema(); } - static GraphQLSchema oracleSchema(GraphQLSchema schema) { + static GraphQLSchema oracleSchema(GraphQLSchema schema, String parentType) { DataFetcher plain = environment -> { if (oracleFailure != null) { throw new IllegalStateException(oracleFailure); @@ -576,19 +721,30 @@ static GraphQLSchema oracleSchema(GraphQLSchema schema) { return oracleValue; }; return schema.transform(builder -> builder.codeRegistry( - schema.getCodeRegistry().transform(registry -> registry.dataFetcher(FieldCoordinates.coordinates("Query", "field"), plain)))); + schema.getCodeRegistry().transform(registry -> registry.dataFetcher(FieldCoordinates.coordinates(parentType, "field"), plain)))); } static Map> compile(List cases, String classpath) throws Exception { Map sources = new LinkedHashMap<>(); for (Case testCase : cases) { String simpleName = testCase.className.substring(testCase.className.lastIndexOf('.') + 1); - sources.putIfAbsent(testCase.className, - "package unwrapcheck.gen;\n" - + "public class " + simpleName + " implements graphql.kickstart.tools.GraphQLQueryResolver {\n" - + " @SuppressWarnings(\"unchecked\")\n" - + " public " + testCase.javaType() + " field() { return (" + testCase.javaType() + ") unwrapcheck.Fixtures.nextReturnValue; }\n" - + "}\n"); + String type = testCase.javaType(); + String body; + switch (testCase.kind) { + case METHOD: + body = " implements graphql.kickstart.tools.GraphQLQueryResolver {\n" + + " @SuppressWarnings(\"unchecked\")\n" + + " public " + type + " field() { return (" + type + ") unwrapcheck.Fixtures.nextReturnValue; }\n"; + break; + case PROPERTY: + body = " implements graphql.kickstart.tools.GraphQLQueryResolver {\n" + + " public " + type + " field;\n"; + break; + default: + body = " extends java.util.HashMap {\n"; + break; + } + sources.putIfAbsent(testCase.className, "package unwrapcheck.gen;\npublic class " + simpleName + body + "}\n"); } JavaCompiler javac = ToolProvider.getSystemJavaCompiler(); diff --git a/src/test/resources/returnvalue/Fixtures.java b/src/test/resources/returnvalue/Fixtures.java index aea82b7d..2584abde 100644 --- a/src/test/resources/returnvalue/Fixtures.java +++ b/src/test/resources/returnvalue/Fixtures.java @@ -1,5 +1,7 @@ package unwrapcheck; +import graphql.kickstart.tools.GraphQLQueryResolver; + import java.util.List; import java.util.Objects; import java.util.concurrent.CompletableFuture; @@ -13,6 +15,12 @@ public final class Fixtures { private Fixtures() { } + public static final class HolderQuery implements GraphQLQueryResolver { + public Object holder() { + return nextReturnValue; + } + } + public static final class Payload { private final String data; diff --git a/src/test/resources/returnvalue/known-findings.txt b/src/test/resources/returnvalue/known-findings.txt index e69de29b..ca0a5c60 100644 --- a/src/test/resources/returnvalue/known-findings.txt +++ b/src/test/resources/returnvalue/known-findings.txt @@ -0,0 +1,5 @@ +448 ^(property|map) \w+<.*Future<.*String>+$ # properties and Maps don't await a future below the top level, so a String field returns the future's toString with no error +896 ^(property|map) \w+<.*Future<.*(Payload|Object)>+$ # properties and Maps don't await a future below the top level, so an object or union field fails +234 ^(property|map) Future< # properties and Maps don't check a declared Future, so a FutureTask fails with "Expected source object" or returns its toString, not the "can't wait on" error +1254 ^(property|map) .*\b(Page|Later|Try)< # properties and Maps don't run generic wrapper transformers, so the field fails or a String field returns the wrapper's toString +18 ^(property|map) (CompletableFuture<)?Optional