Skip to content

Pin resolver behaviour for methods, properties and Maps - #950

Merged
oryan-block merged 5 commits into
agent-friendlyfrom
agent-friendly-p9-resolver-pins
Nov 5, 2026
Merged

oryan-block merged 5 commits into
agent-friendlyfrom
agent-friendly-p9-resolver-pins

Conversation

@oryan-block

@oryan-block oryan-block commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

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 @JvmField and 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

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Description

  • the bounded return value check now runs every return type as a method, a public field (what a @JvmField compiles to) and a typed HashMap subclass. Properties and Maps run with the nullable GraphQL type only, at --property-budget 1 and 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 PR
  • today's property and Map results go into known-findings.txt as five lines, one per cause (nested futures with a String leaf, nested futures with an object or union leaf, a FutureTask behind Future, generic wrappers, a DataFetcherResult inside an Optional). They match exactly the 2,850 return types that fail: 448, 896, 234, 1,254 and 18
  • each line is now <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 is STALE and gets named, so a partial fix has to narrow the line and a later regression in a fixed type is a FAIL. A line that matches more accepted return types than its count is NEW, since that's a type the build used to reject failing under it. Before, a line only went stale once nothing matched it, and the DataFetcherResult line also covered 36 list shapes that pass, because graphql-java reads a DataFetcherResult that's a list element itself
  • known findings match <kind> <return type>, e.g. property List<CompletableFuture<String>>, so a line without a kind still covers all three
  • new ResolverCompatibilityTest pins 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, trailing GraphQLContext / context class / environment parameters, primitive defaults and a type variable parameter getting its resolved type argument (Long for Echo<Long>), raw and typed Optional arguments, the List-only concrete scalar bypass, the source check, outermost vs nested wrapper selection, suspend functions and subscription events
  • a row for it in the AGENTS.md rule table

A few of these pin behaviour that looks wrong, on purpose, so a refactor can't change it silently:

  • untyped Maps hand their value to graphql-java as is. Their value type is Object, so there's nothing to unwrap by
  • a subscription that publishes CompletableFutures sends CompletableFuture@...[Completed normally] as the event
  • a value of two unrelated registered wrapper types gets the transformer registered last, whatever the declared type
  • contextClass(DataFetchingEnvironment::class) makes an environment parameter look itself up in the GraphQLContext and fail
  • a [Int] argument reaches a List<Long> parameter as Integers, since only List parameters skip conversion

The property and Map fix has to delete both sets of known findings, the lines in known-findings.txt and the knownFindings map in ResolverCompatibilityTest. 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 verify passes at this head, 469 tests, with the API and jar checks from agent-friendly
  • the check takes 19.6s in that JDK 17 run (load average about 9 from other builds). At the previous head it was 18.2s vs 14.8s on agent-friendly, median of 3 interleaved runs each
  • with all lines in place the check passes apart from the known findings
  • a scratch break that drops the error from each DataFetcherResult in a property's list fails as FAIL on 6 of the 36 list shapes. Before this change they'd have printed as KNOWN
  • a scratch partial fix where only property fields await futures fails as STALE and names the 11 String and 22 object or union return types it fixed (448 -> 437 and 896 -> 874). ResolverCompatibilityTest fails on the 3 @JvmField shapes it fixed
  • off-by-one counts fail: 17 on the 18-type line is NEW, 235 on the 234-type line is STALE, and check.sh exits 1
  • each of these makes ResolverCompatibilityTest fail: binding the environment in place of the context class value, swapping two argument closures, resolve in place of resolveTypeVariables for parameters, picking the outermost wrapper by declared type, skipping the source check in FieldResolver, treating a raw Optional parameter as plain, and letting Collection parameters skip conversion

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.
@sonarqubecloud

sonarqubecloud Bot commented Nov 5, 2026

Copy link
Copy Markdown

@oryan-block
oryan-block merged commit ff829e8 into agent-friendly Nov 5, 2026
5 checks passed
@oryan-block
oryan-block deleted the agent-friendly-p9-resolver-pins branch November 5, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant