fix: batch resolver parent registration - #4281
Conversation
7cd308d to
aa41949
Compare
| Edges []ProfileEdge `json:"edges"` | ||
| TotalCount int `json:"totalCount"` |
There was a problem hiding this comment.
To show this working - I changed one of the models in the example to use a slice of values instead of slice of pointers. Previously this would mean we go back to running N individual queries. But with this change, a model with this structure would still batch correctly.
|
For context, I use gqlgen with I'm happy to iterate on this based on feedback, let me know if there's something I should do differently to fix this bug. |
|
fixed the ci errors |
Generated code recovered batch parents itself: it looked up the group, type-asserted the parents, read the parent index off the path, and derived the field's response key from the alias. Four concerns, emitted inline into every batch field of every generated file. Move them behind BatchParentsFor and BatchParents.FieldResult, and add WithBatchParentValues for the case where the marshaler holds a []T rather than a []*T and has to take the address of each element. Nothing existing changes signature, so code generated by an older gqlgen still compiles against this runtime.
HasBatchResolverFields answers a schema-wide question: does anything, anywhere, use a batch resolver. Marshalers need a narrower one, namely whether this type does, so add HasBatchFields and BatchImplementors. The set is computed once and cached rather than populated in BuildData, because plugins may add fields after the data is built and templates render after every plugin has run. Per-schema builds under the follow-schema layout hold only the objects declared in their own file, so addBuild propagates the schema-wide set the same way it already propagates AllDirectives. Without that an interface whose implementors live in different schema files would only see the ones sharing its file.
Three problems in the slice marshaler, all in the same guard. A []T field stored the values themselves as batch parents, but resolvers take []*T, so the generated type assertion failed and every parent fell back to a single-parent call. Use WithBatchParentValues for that case. $type.Elem.Definition follows through list wrappers, so [[Thing!]!]! reported Thing's kind and the outer list registered a [][]T under Thing's name. For an interface element it emitted a type switch on a slice, which does not compile at all. Only the innermost list may register. The guard was also schema-global: one batch resolver anywhere made every value-slice marshaler register parents, including the introspection types, which can never have batch resolvers. Ask per type instead.
The batch branch emitted a four-deep nest per field to look up the group, assert the parent type, read the parent index and derive the response key. Call BatchParentsFor and FieldResult instead, which leaves one level. Generated code is what people read when debugging gqlgen, so the shape of the output matters as much as the size of the template.
"batch resolver is not supported for field __Directive.name" leaves the reader to work out that introspection types are gqlgen's own. Return the reason from config and include it in the error. TypeSupportsBatchResolver is now defined in terms of the reason so the bool and the explanation cannot drift apart.
Three fixtures for the marshaler guards, each failing before its fix:
valueslices omit_slice_element_pointers, the mainstream way a batch
resolver ends up with []T parents
nestedlist nested lists of an object and of an interface
followschema an interface whose implementors are split across schema
files, which the per-schema builds see only in part
Assertions count exact occurrences rather than test for presence, because
the nested-list bug registered a type twice rather than not at all.
prelude.resolvers.go held 38 batch resolver stubs for the introspection types. It was generated before the __ prefix was excluded from batch resolvers and has been dead since, but it still stopped the fixture package from compiling.
validate built its roots as import paths. The go command skips directories named testdata when it expands "..." in an import path, so for output written beneath one the pattern matched no packages and validation passed having compiled nothing. Every fixture in api/testdata had been unchecked since it was added; that is how prelude.resolvers.go stayed broken. Build the roots from the output directory instead, which is already how gqlgen addresses everything else it writes. validation_catches_errors is a fixture whose package deliberately does not compile, so a return to import paths fails the suite rather than silently skipping it.
A full introspection query over a 2000-type schema spent 8.7ms and 15.8MB across 174k allocations. The cost is linear in schema size and each type is visited exactly once, so there is nothing to memoize; it is allocation. Pre-size the result slices, and stop Types and Directives building a map and a []string and copying out of them purely to sort. Same schema now takes 5.3ms and 7.7MB across 124k allocations, and a full introspection response is byte for byte what it was.
Batch resolvers appeared in the docs only as batch: Boolean in the goField directive listing, with no prose anywhere. Describe the signature and result ordering, and warn about the parents: for a []T field they are addresses into the slice the marshaler is walking concurrently, so mutating or retaining them races, and pointer identity does not match what the non-batch path passes.
The value-slice test counted resolver calls but never looked at the nodes it got back, so a batch that returned its results in the wrong order still passed. Reversing the mapping in the resolver left the whole package green.
Regenerated for the marshaler guards and the resolveBatch_* rewrite. The federation fixtures drop from 28 batch parent registrations to 2: the rest were introspection types and types with no batch fields, which could never read them.
|
@yiblet I just wanted to make a quick tweak to move some of your template logic into library helper code. I hope you don't mind, but I then noticed your tests were partly unfalsifiable (would always pass under some circumstances), and fixing those lead me to uncover a number of existing problems (not your fault). I got a little carried away, but this makes things a lot better. |
buildPattern classified paths with path.IsAbs, which only understands a leading slash. PackageConfig.Dir runs its filenames through filepath.Abs and ToSlash when the config loads, so on Windows it arrives as "D:/a/b" and picked up a "./" prefix. The go command then rejected "./D:/a/b/...", and every test that generates code failed. It passed on Linux and macOS because filepath.Abs produces a leading slash there, which path.IsAbs does recognise. Classify drive letters and UNC paths directly rather than deferring to filepath.IsAbs, which only knows the host's own convention. The rule is now the same on every OS, so the test cases for Windows shapes fail on any machine when the logic is wrong instead of only in Windows CI.
|
I don't mind at all! I deeply appreciate it. This issue was stopping me from being able to use the batch dataloader feature since I use value based slices instead of the pointer based ones. I'm excited to try them out now. Thanks for the review & the merge. |
This schema:
The generated batch resolver receives all organizations at once:
When
omit_slice_element_pointers: trueis set,organizationsis generated as[]Organization. The marshaller stored that value slice in the batch group, thedispatcher asserted
[]*Organization, the assertion failed, and gqlgen fell back to onecall per organization. No error, correct data, N calls.
The marshaller now takes the address of each element before creating the group.
Other Registration Bugs Found Alongside It
ast.Type.Name()follows through list wrappers, so[[Thing!]!]!reported
Thing's kind and the outer list registered a[][]ThingunderThing. Foran interface element it emitted a type switch on a slice, which does not compile.
Only the innermost list registers now.
anywhere made every value-slice marshaller register parents, including introspection
types, which cannot have batch resolvers at all. The federation fixtures drop from 28
registrations to 2.
follow-schema, eachDataholds only its own file'sobjects, so an interface whose implementors live in different files registered only
those sharing its file, a silent N+1 for the rest.
addBuildnow propagates theschema-wide set, as it already does for directives.
Also in This Branch
resolveBatch_*no longer emits a four-deep nest per field; group lookup, parent typerecovery, parent index and response key move behind
graphql.BatchParentsForandBatchParents.FieldResult. No existing runtime signature changes, so code generated byan older gqlgen still compiles.
validatebuilt its roots as import paths, whose...wildcard skipstestdatadirectories, so it matched no packages and compiled nothing. Every
api/testdatafixture had gone unchecked since it landed. Roots now come from the output
directory, and a fixture that deliberately fails to compile keeps it honest.
A 2000-type schema goes from 8.7 ms and 15.8 MB to 5.3 ms and 7.7 MB, with byte
identical output.
[]Tfield the parents point into the slice the marshaller is walking concurrently, sothey must not be mutated or retained.
prelude.resolvers.go, dead since__types were excluded from batch resolvers, is deleted.Tests
Each fix fails without it. Fixtures in
api/testdatacoveromit_slice_element_pointers, nested lists, and split schema files, and count exactoccurrences because the nested-list bug registered a type twice rather than not at all.
The
_examples/batchresolvertest asserts results align with parents; reversing themapping in the resolver used to leave the package green.
go test ./...in both modules,go generate ./...a no-op,golangci-lint run ./...clean.
I have: