Repository navigation
Pin resolver behaviour for methods, properties and Maps - #950
Merged
Merged
Conversation
Today's property and Map results go in as known findings, so the check passes and a fix has to delete them.
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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Why
Agents working in graphql-java-tools keep repeating the same mistakes, and the repo should change so a change that looks right from one file is right for the whole repo. The resolver path is the biggest spot for that. The schema build and the data fetcher each work out the same facts about a resolver, and the next PRs move that into one plan per field. Before they do, this pins what a query returns today, so each of them can show it changed nothing except the property and Map fix.
That fix is needed because property and Map fields don't unwrap their values. The schema build accepts
List<CompletableFuture<String>>on a@JvmFieldand the query returns[java.util.concurrent.CompletableFuture@5a7cb161[Completed normally], ...]with no error, while a method with the same return type returns[v1, v2].Checklist
Description
@JvmFieldcompiles to) and a typedHashMapsubclass. Properties and Maps run with the nullable GraphQL type only, at--property-budget 1and with one pending completion order instead of two, since the only thing they add is how the value is read. That's 226,277 executions, up from 171,693. With both orders it was +10s on the check, which is the whole build-time budget for this PRknown-findings.txtas five lines, one per cause (nested futures with a String leaf, nested futures with an object or union leaf, aFutureTaskbehindFuture, generic wrappers, aDataFetcherResultinside anOptional). They match exactly the 2,850 return types that fail: 448, 896, 234, 1,254 and 18<count> <regex> # <what goes wrong>, and the check fails when a line stops matching exactly the return types that fail. A matched return type that passes isSTALEand gets named, so a partial fix has to narrow the line and a later regression in a fixed type is aFAIL. A line that matches more accepted return types than its count isNEW, since that's a type the build used to reject failing under it. Before, a line only went stale once nothing matched it, and theDataFetcherResultline also covered 36 list shapes that pass, because graphql-java reads aDataFetcherResultthat's a list element itself<kind> <return type>, e.g.property List<CompletableFuture<String>>, so a line without a kind still covers all threeResolverCompatibilityTestpins values by hand for@JvmField, typed Map and untyped Map fields against a method, with its own known findings for the 7 shapes that differ. Also argument order after the source, trailingGraphQLContext/ context class / environment parameters, primitive defaults and a type variable parameter getting its resolved type argument (LongforEcho<Long>), raw and typedOptionalarguments, the List-only concrete scalar bypass, the source check, outermost vs nested wrapper selection, suspend functions and subscription eventsAGENTS.mdrule tableA few of these pin behaviour that looks wrong, on purpose, so a refactor can't change it silently:
Object, so there's nothing to unwrap byCompletableFutures sendsCompletableFuture@...[Completed normally]as the eventcontextClass(DataFetchingEnvironment::class)makes an environment parameter look itself up in theGraphQLContextand fail[Int]argument reaches aList<Long>parameter asIntegers, since onlyListparameters skip conversionThe property and Map fix has to delete both sets of known findings, the lines in
known-findings.txtand theknownFindingsmap inResolverCompatibilityTest. Untyped Maps keep their pins.No release note, nothing changes for library users.
Testing:
mise exec java@temurin-17.0.21+9 -- mvn -B -ntp clean verifypasses at this head, 469 tests, with the API and jar checks fromagent-friendlyagent-friendly, median of 3 interleaved runs eachDataFetcherResultin a property's list fails asFAILon 6 of the 36 list shapes. Before this change they'd have printed asKNOWNSTALEand names the 11 String and 22 object or union return types it fixed (448 -> 437 and 896 -> 874).ResolverCompatibilityTestfails on the 3@JvmFieldshapes it fixedNEW, 235 on the 234-type line isSTALE, andcheck.shexits 1ResolverCompatibilityTestfail: binding the environment in place of the context class value, swapping two argument closures,resolvein place ofresolveTypeVariablesfor parameters, picking the outermost wrapper by declared type, skipping the source check inFieldResolver, treating a rawOptionalparameter as plain, and lettingCollectionparameters skip conversion