From bec860eeff4a7354da08e7cf07a09422ed3b0c88 Mon Sep 17 00:00:00 2001 From: Oryan Date: Wed, 4 Nov 2026 15:23:19 -0500 Subject: [PATCH 1/4] Run the return value check on properties and typed Maps Today's property and Map results go in as known findings, so the check passes and a fix has to delete them. --- .../verify-return-value-unwrapping/SKILL.md | 68 +++++--- src/test/resources/returnvalue/Check.java | 154 ++++++++++++++---- src/test/resources/returnvalue/Fixtures.java | 8 + .../resources/returnvalue/known-findings.txt | 5 + 4 files changed, 183 insertions(+), 52 deletions(-) diff --git a/.claude/skills/verify-return-value-unwrapping/SKILL.md b/.claude/skills/verify-return-value-unwrapping/SKILL.md index c9ed81c9..0b09844a 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 @@ -67,7 +76,7 @@ as `-Dunwrapping.args`, and prints the report. On a failure it also prints the p | 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` | -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 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`. @@ -81,7 +90,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 +106,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,14 +115,23 @@ 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`. +` # `. 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. 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, 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` only at the top or inside that future, and unboxes any +`Optional`. So every nested future, +every generic wrapper, every `DataFetcherResult` graphql-java can't reach, and a `FutureTask` behind a declared `Future` +fail for them. The five lines cover exactly those 2,850 return types. 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. Delete the line then, so that a regression shows up as `FAIL`. @@ -126,10 +146,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 +160,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/src/test/resources/returnvalue/Check.java b/src/test/resources/returnvalue/Check.java index c801ac13..b7d9a5bd 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,7 @@ import java.util.concurrent.FutureTask; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.function.Consumer; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -96,6 +99,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 +134,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 +168,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) + " }"; } } @@ -249,6 +290,7 @@ 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 propertyBudget = intArg(args, "--property-budget", 1); long started = System.nanoTime(); List cases = cases(depth); @@ -260,9 +302,11 @@ public static boolean check(String[] args, String classpath, PrintStream out) th Set accepted = 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 +314,16 @@ 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()); 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,9 +335,14 @@ 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."); } @@ -303,7 +352,7 @@ public static boolean check(String[] args, String classpath, PrintStream out) th known.keySet().forEach(pattern -> knownFailing.put(pattern, new ArrayList<>())); 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); + String pattern = known.keySet().stream().filter(it -> Pattern.compile(it).matcher(testCase.label()).find()).findFirst().orElse(null); if (pattern == null) { unknown.add(list); } else { @@ -344,7 +393,7 @@ private static Map knownFindings(String path) throws Exception { if (!line.isBlank() && !line.startsWith("#")) { int separator = line.indexOf(" # "); if (separator < 0) { - throw new IllegalArgumentException("A known finding is \" # \", not: " + line); + throw new IllegalArgumentException("A known finding is \">> # \", not: " + line); } known.put(line.substring(0, separator).trim(), line.substring(separator + 3).trim()); } @@ -381,9 +430,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 +565,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 +573,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 +607,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 +651,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 +659,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..d38532fc 100644 --- a/src/test/resources/returnvalue/known-findings.txt +++ b/src/test/resources/returnvalue/known-findings.txt @@ -0,0 +1,5 @@ +^(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 +^(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 +^(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 +^(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 +^(property|map) ((\w+<){2,}|(List|Optional|DataFetcherResult)<)DataFetcherResult< # properties and Maps leave a DataFetcherResult below a list, an Optional or another DataFetcherResult to graphql-java, which only reads one at the top or under a top-level future From dd2c07d4ac714ecfc19e71c0eb06ea95e136d09b Mon Sep 17 00:00:00 2001 From: Oryan Date: Wed, 4 Nov 2026 15:23:19 -0500 Subject: [PATCH 2/4] Pin resolver binding and unwrapping in ResolverCompatibilityTest --- .../kickstart/tools/RawGenericFixtures.java | 9 + .../tools/ResolverCompatibilityTest.kt | 425 ++++++++++++++++++ 2 files changed, 434 insertions(+) create mode 100644 src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt 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..a5434df7 --- /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 `property and Map fields return what a method returns, apart from the known findings`() { + 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 type variables as their bound class`() { + 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() + } +} From 594944b530596d4d979812899ebced532fc1dcb4 Mon Sep 17 00:00:00 2001 From: Oryan Date: Wed, 4 Nov 2026 15:23:19 -0500 Subject: [PATCH 3/4] Add the resolver pins to the rule table --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index 4714eb7a..2db66248 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,6 +80,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 today's property and Map gaps as known findings, and fails once one is fixed until it's deleted | | 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 | From b70f073bea9e19483e2716b702189e2720dd826e Mon Sep 17 00:00:00 2001 From: Oryan Date: Wed, 4 Nov 2026 19:55:16 -0500 Subject: [PATCH 4/4] Make each known finding match exactly the return types that fail Each known-findings line now records how many accepted return types it matches. The check fails as STALE when a matched return type passes or the line matches fewer, and as NEW when it matches more, naming what it can. The DataFetcherResult line now matches only the 18 Optional shapes that fail, not the 36 list shapes graphql-java reads itself. Rename two ResolverCompatibilityTest tests whose names overstated what they assert. --- .../verify-return-value-unwrapping/SKILL.md | 34 +++-- .../verify-return-value-unwrapping/check.sh | 2 +- AGENTS.md | 2 +- .../tools/ResolverCompatibilityTest.kt | 4 +- src/test/resources/returnvalue/Check.java | 130 +++++++++++++----- .../resources/returnvalue/known-findings.txt | 10 +- 6 files changed, 125 insertions(+), 57 deletions(-) diff --git a/.claude/skills/verify-return-value-unwrapping/SKILL.md b/.claude/skills/verify-return-value-unwrapping/SKILL.md index 0b09844a..6030c798 100644 --- a/.claude/skills/verify-return-value-unwrapping/SKILL.md +++ b/.claude/skills/verify-return-value-unwrapping/SKILL.md @@ -72,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`, `--property-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 @@ -115,25 +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 -` # `. The regex is matched against the kind and the short return type, such as +` # `. 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. A -new break inside a known pattern is hidden, so after a change to the code a known finding covers, read its -counterexample. +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` only at the top or inside that future, and unboxes any -`Optional`. So every nested future, -every generic wrapper, every `DataFetcherResult` graphql-java can't reach, and a `FutureTask` behind a declared `Future` -fail for them. The five lines cover exactly those 2,850 return types. A String field gets the container's `toString`, such -as `CompletableFuture@5a7cb161[Completed normally]`, with no error. +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. Delete the line then, so that a regression shows up as `FAIL`. +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 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 f3a5b433..4899091f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,7 +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 today's property and Map gaps as known findings, and fails once one is fixed until it's deleted | +| 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/ResolverCompatibilityTest.kt b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt index a5434df7..40b0d1d4 100644 --- a/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt @@ -24,7 +24,7 @@ import java.util.concurrent.FutureTask class ResolverCompatibilityTest { @Test - fun `property and Map fields return what a method returns, apart from the known findings`() { + 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) @@ -97,7 +97,7 @@ class ResolverCompatibilityTest { } @Test - fun `omitted arguments reach primitive parameters as their default and type variables as their bound class`() { + 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) }", diff --git a/src/test/resources/returnvalue/Check.java b/src/test/resources/returnvalue/Check.java index b7d9a5bd..fa7ede26 100644 --- a/src/test/resources/returnvalue/Check.java +++ b/src/test/resources/returnvalue/Check.java @@ -51,6 +51,7 @@ 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; @@ -261,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; @@ -288,9 +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 propertyBudget = intArg(args, "--property-budget", 1); + 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); @@ -300,6 +319,7 @@ 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); @@ -315,6 +335,7 @@ public static boolean check(String[] args, String classpath, PrintStream out) th continue; } 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, testCase.parentType())).build(); Consumer returning = returning(testCase, generated, instance); @@ -347,55 +368,96 @@ public static boolean check(String[] args, String classpath, PrintStream out) th 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.label()).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; + } + if (depth == DEPTH && budget == BUDGET && propertyBudget == PROPERTY_BUDGET) { + passed &= knownFindingsAreExact(known, failing.keySet(), show, out); + } + if (passed) { + out.println(violations.isEmpty() ? "PASS" : "\nPASS apart from the known findings above"); + } + return passed; + } + + 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); } - 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 (!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); } - out.println(violations.isEmpty() ? "PASS" : "\nPASS apart from the known findings above"); - return true; + return added.isEmpty() && stale.isEmpty(); } - private static Map knownFindings(String path) throws Exception { - Map known = new LinkedHashMap<>(); + 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; diff --git a/src/test/resources/returnvalue/known-findings.txt b/src/test/resources/returnvalue/known-findings.txt index d38532fc..ca0a5c60 100644 --- a/src/test/resources/returnvalue/known-findings.txt +++ b/src/test/resources/returnvalue/known-findings.txt @@ -1,5 +1,5 @@ -^(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 -^(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 -^(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 -^(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 -^(property|map) ((\w+<){2,}|(List|Optional|DataFetcherResult)<)DataFetcherResult< # properties and Maps leave a DataFetcherResult below a list, an Optional or another DataFetcherResult to graphql-java, which only reads one at the top or under a top-level future +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