Skip to content

Commit ee8555b

Browse files
devsergiyclaude
andauthored
fix: improve ast rewriter field provenance (#1587)
<!-- Important: Before developing new features, please open an issue to discuss your ideas with the maintainers. This ensures project alignment and helps avoid unnecessary work for you. Thank you for your contribution! Please provide a detailed description below and ensure you've met all the requirements. Squashed commit messages must follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard to facilitate changelog generation. Please ensure your PR title follows the Conventional Commits specification, using the appropriate type (e.g., feat, fix, docs) and scope. Examples of good PR titles: - 💥feat!: change implementation in an non-backward compatible way - ✨feat(auth): add support for OAuth2 login - 🐞fix(router): add support for custom metrics - 📚docs(README): update installation instructions - 🧹chore(deps): bump dependencies to latest versions --> @coderabbitai summary ## Checklist - [ ] I have discussed my proposed changes in an issue and have received approval to proceed. - [ ] I have followed the coding standards of the project. - [ ] Tests or benchmarks have been added or updated. ## Open Source AI Manifesto This project follows the principles of the [Open Source AI Manifesto](https://human-oss.dev). Please ensure your contribution aligns with its principles. <!-- Please add any additional information or context regarding your changes here. --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ecfc299 commit ee8555b

14 files changed

Lines changed: 2458 additions & 166 deletions

docs/superpowers/plans/2026-07-15-abstract-rewriter-ref-provenance.md

Lines changed: 1094 additions & 0 deletions
Large diffs are not rendered by default.
Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
# Abstract Selection Rewriter: Field Ref Provenance at Construction
2+
3+
**Date:** 2026-07-15
4+
**Status:** Approved
5+
**Supersedes:** 2026-07-14-abstract-rewriter-skip-refs-design.md (scope-chain matching)
6+
**Files:** `v2/pkg/ast/ast_field.go`, `v2/pkg/ast/ast.go`,
7+
`v2/pkg/engine/plan/abstract_selection_rewriter.go`,
8+
`v2/pkg/engine/plan/abstract_selection_rewriter_helpers.go`,
9+
`v2/pkg/engine/plan/abstract_selection_rewriter_info.go`,
10+
`v2/pkg/engine/plan/node_selection_visitor.go`
11+
12+
## Problem
13+
14+
When the abstract selection rewriter flattens fragments, it rebuilds the field's
15+
selection subtree from scratch and must remap two ref-keyed structures that still
16+
point at pre-rewrite refs:
17+
18+
- `skipFieldsRefs` — planner-added fields hidden from the response
19+
- `fieldDependsOn` / `fieldRefDependsOn` — jump dependencies
20+
21+
The previous approaches *reconstructed* the old→new mapping structurally, by
22+
walking the subtree before and after the rewrite and matching fields on their
23+
query path with inline fragment names stripped. Stripping fragment names
24+
collapses distinct response positions (`nodes.<on A>.id` vs `nodes.<on B>.id`
25+
both become `query.nodes.id`), which conflated a planner-added (skipped) field
26+
with a user-requested field — losing user fields from the response or leaking
27+
planner fields into it.
28+
29+
The first fix (superseded design) kept path matching and added a per-level
30+
type-condition **scope chain** to disambiguate collapsed positions. It worked,
31+
but it re-derived — with two extra subtree walks and set-intersection
32+
heuristics — information the rewriter itself had at the moment it built the new
33+
subtree.
34+
35+
## Insight
36+
37+
The old→new mapping does not need to be reconstructed at all. During a rewrite,
38+
new field refs come into existence at exactly three points, and at each point
39+
the origin is locally known:
40+
41+
1. **Copy.** `createFragmentSelection` copies original selections via
42+
`ast.Document.CopySelection`, which funnels every field through
43+
`Document.CopyField` — a single choke point. The (originalRef, copyRef)
44+
pair is exact at that moment. `preserveTypeNameSelection` copies an
45+
explicitly requested `__typename` through the same choke point, which also
46+
preserves its directives (e.g. defer) verbatim.
47+
2. **Synthesize.** `typeNameSelection` creates a `__typename` with no original
48+
(empty selection set fallback, interface-object case). The rewriter already
49+
appends these to `skipFieldRefs` itself; no mapping is needed.
50+
51+
After building, `replaceFieldSelections` normalizes the subtree with
52+
`AbstractFieldNormalizer`. The only operations that *destroy* a field ref during
53+
that normalization are field merges, and both merge sites
54+
(`deduplicateFields`, `mergeInlineFragmentSelections.mergeFields`) funnel
55+
through a single call: `Document.MergeFieldsDefer(left, right)`, where `left`
56+
survives and `right` is removed. (`inlineSelectionsFromInlineFragments` only
57+
relocates selections; it never merges or removes fields.)
58+
59+
So complete, exact provenance = **copy log** (recorded at points 1–2) composed
60+
with **merge log** (recorded at `MergeFieldsDefer`). No paths, no scopes, no
61+
extra walks, no matching heuristics.
62+
63+
## Design
64+
65+
### Component 1: observation hooks on `ast.Document`
66+
67+
Two optional callback fields on `Document`, nil by default, invoked with a nil
68+
check; cleared in `Document.Reset()` (documents are pooled):
69+
70+
```go
71+
// OnCopyField is called after CopyField copies a field, with the source and the new field ref.
72+
OnCopyField func(originalRef, copyRef int)
73+
// OnMergeFields is called by MergeFieldsDefer when the right field is merged into the left field.
74+
OnMergeFields func(survivorRef, removedRef int)
75+
```
76+
77+
- `CopyField` invokes `OnCopyField(ref, newRef)` before returning.
78+
- `MergeFieldsDefer` invokes `OnMergeFields(left, right)`.
79+
80+
No signatures change; `astminify` (the only other copy caller) and the main
81+
normalization pipeline (the other merge caller) are unaffected because the
82+
callbacks are nil outside the rewriter's window.
83+
84+
### Component 2: provenance recording in the rewriter
85+
86+
`fieldSelectionRewriter` gains two logs:
87+
88+
```go
89+
copyLog []refPair // (originalRef -> newRef): CopyField hook, chronological
90+
mergeLog []refPair // (removedRef -> survivorRef): MergeFieldsDefer hook, chronological
91+
```
92+
93+
- `RewriteFieldSelection` sets both hooks on `r.operation` on entry and clears
94+
them with `defer`. (Copies happen during `rewriteXxxSelection`; merges happen
95+
during the normalizer run inside `replaceFieldSelections` — both inside the
96+
window. The needs-rewrite checks perform neither.)
97+
- `preserveTypeNameSelection` copies the original `__typename` selection via
98+
`CopySelection`, so its provenance is recorded by the hook like any other
99+
copy and its directives survive verbatim. To know what to copy,
100+
`selectionSetInfo` gains `typenameSelectionRef int` (populated in
101+
`selectionSetFieldSelections`, `ast.InvalidRef` when absent).
102+
103+
Copies always source from pre-rewrite refs (selection infos are collected
104+
before any mutation), so the copy log never chains. Merges can chain
105+
(A merged into B, B later merged into C); chronological processing resolves
106+
chains.
107+
108+
### Component 3: composition replaces `collectChangedRefs`
109+
110+
A single function builds both result maps from the logs after the rewrite:
111+
112+
```go
113+
// fieldRefOrigins: newRef -> pre-rewrite refs it represents
114+
origins[copy.to] = append(origins[copy.to], copy.from) // copy log, in order
115+
for each merge (removed -> survivor), chronologically:
116+
origins[survivor] = append(origins[survivor], origins[removed]...)
117+
delete(origins, removed)
118+
119+
// changedFieldRefs: oldRef -> final new refs (deduplicated, empty entries omitted)
120+
redirect := resolve merge chains (removed -> final survivor)
121+
for each copy (from -> to), in order:
122+
changed[from] = appendUnique(changed[from], redirect(to))
123+
```
124+
125+
`RewriteResult` keeps its shape (`changedFieldRefs`, `fieldRefOrigins`);
126+
semantics tighten: both maps now contain only refs that participate in the
127+
rewrite. The previous root-field identity entry (an artifact of the walk-based
128+
collector) disappears.
129+
130+
### Component 4: consumer simplification
131+
132+
`nodeSelectionVisitor.updateSkipFieldRefs` consumes `fieldRefOrigins` directly.
133+
Field refs created by a rewrite are fresh (`Fields` is append-only), so a key of
134+
`fieldRefOrigins` is never a pre-rewrite skipped ref. However, a fresh ref CAN
135+
already be in `skipFieldsRefs`: the rewriter pre-registers its synthesized
136+
skipped `__typename` (interface-object case), and normalization can dedup-merge
137+
a preserved user-requested `__typename` into it. The unskip branch covers
138+
exactly that case:
139+
140+
```go
141+
for newRef, originRefs := range fieldRefOrigins {
142+
if all originRefs are in skipFieldsRefs {
143+
add newRef to skipFieldsRefs (if not already present)
144+
} else if newRef is in skipFieldsRefs {
145+
remove newRef from skipFieldsRefs
146+
}
147+
}
148+
```
149+
150+
`updateFieldDependsOn` is unchanged — it just becomes precise automatically.
151+
152+
### Deletions (the point of the exercise)
153+
154+
- `AbstractFieldPathCollector`, `FieldLimitedVisitor`, `collectFieldPaths`
155+
(no usages outside the rewriter, verified incl. cosmo router)
156+
- `collectedFieldPath`, `scopeChain`, `scopeChainsIntersect`, `scopesIntersect`,
157+
`intersectScopes`, `resolveTypeCondition`
158+
- the pre-rewrite path collection calls in all three `processXxxSelection`
159+
- `collectChangedRefs` (replaced by log composition)
160+
161+
Net effect: the plan package no longer reasons about "which fields could occupy
162+
the same response position" — it records which fields *do*.
163+
164+
## Correctness across the known cases
165+
166+
| Case | Provenance | Result |
167+
|---|---|---|
168+
| user `... on A { id }` + skipped `id` in B | copy: 0→cA, 2→cB (distinct) | cA visible, cB skipped ✓ |
169+
| user `id` at interface level + skipped `id` in B | copies 0→cA, 0→cB1, 2→cB2; merge cB2→cB1 | origins(cB1)={0,2} → visible ✓ |
170+
| aliased duplicate `aliased: id` vs `id` | distinct originals, copies never merge (alias differs) | tracked separately ✓ |
171+
| planner-added `__typename` re-created by a second rewrite | preserveTypeNameSelection logs the pair | skip status transfers ✓ |
172+
| chained rewrites | each rewrite's logs are local; origins are that rewrite's pre-refs | precise per round ✓ |
173+
| dropped fragment (type not in datasource) | never copied → no entry | disappears, unchanged behavior ✓ |
174+
| defer directives on merged fields | `MergeFieldsDefer` semantics untouched; hook only observes | unchanged ✓ |
175+
176+
## Testing
177+
178+
1. `abstract_selection_rewriter_changed_refs_test.go` — keep all three cases;
179+
drop the root-field identity entry (`4: {4}`) from expectations; add a case
180+
asserting `__typename` provenance through `preserveTypeNameSelection`.
181+
2. `TestNodeSelectionVisitor_UpdateSkipFieldRefs` — drop the now-impossible
182+
unskip input; keep the all-origins-skipped / mixed-origins cases.
183+
3. Branch federation tests in `graphql_datasource_federation_test.go` — must
184+
pass unchanged (plan-level behavior identical).
185+
4. Full `plan`, `astnormalization`, `graphql_datasource` suites for regressions.

v2/pkg/ast/ast.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,14 @@ type Document struct {
5555
Refs [][8]int
5656
RefIndex int
5757
Index Index
58+
59+
// OnCopyField, when set, is called by CopyField with the source field ref and the new field ref.
60+
// CopyField is recursive: the hook fires once for every field copied, including fields nested
61+
// in copied selection sets, children before their parent.
62+
OnCopyField func(fieldRef, copyRef int)
63+
// OnMergeFields, when set, is called by MergeFieldsDefer with the surviving (left)
64+
// and the removed (right) field ref when two fields are merged.
65+
OnMergeFields func(survivorRef, removedRef int)
5866
}
5967

6068
func NewDocument() *Document {
@@ -164,6 +172,9 @@ func (d *Document) Reset() {
164172
d.RefIndex = -1
165173
d.Index.Reset()
166174
d.Input.Reset()
175+
176+
d.OnCopyField = nil
177+
d.OnMergeFields = nil
167178
}
168179

169180
func (d *Document) NextRefIndex() int {

v2/pkg/ast/ast_field.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func (d *Document) CopyField(ref int) int {
3333
if d.Fields[ref].HasSelections {
3434
selectionSet = d.CopySelectionSet(d.Fields[ref].SelectionSet)
3535
}
36-
return d.AddField(Field{
36+
copyRef := d.AddField(Field{
3737
Name: d.copyByteSliceReference(d.Fields[ref].Name),
3838
Alias: d.CopyAlias(d.Fields[ref].Alias),
3939
HasArguments: d.Fields[ref].HasArguments,
@@ -43,6 +43,10 @@ func (d *Document) CopyField(ref int) int {
4343
HasSelections: d.Fields[ref].HasSelections,
4444
SelectionSet: selectionSet,
4545
}).Ref
46+
if d.OnCopyField != nil {
47+
d.OnCopyField(ref, copyRef)
48+
}
49+
return copyRef
4650
}
4751

4852
func (d *Document) FieldNameBytes(ref int) ByteSlice {
@@ -187,6 +191,10 @@ func (d *Document) FieldTypeNode(fieldName []byte, enclosingNode Node) (node Nod
187191
// - if both sides defer, the smaller id wins, since lower ids correspond to earlier/outer
188192
// defer scopes that subsume later ones
189193
func (d *Document) MergeFieldsDefer(left, right int) {
194+
if d.OnMergeFields != nil {
195+
d.OnMergeFields(left, right)
196+
}
197+
190198
leftDeferDirectiveRef, leftDeferExists := d.Fields[left].Directives.HasDirectiveByNameBytes(d, literal.DEFER_INTERNAL)
191199
rightDeferDirectiveRef, rightDeferExists := d.Fields[right].Directives.HasDirectiveByNameBytes(d, literal.DEFER_INTERNAL)
192200

v2/pkg/ast/ast_field_hooks_test.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package ast_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
8+
"github.com/wundergraph/graphql-go-tools/v2/pkg/ast"
9+
)
10+
11+
func TestDocumentFieldHooks(t *testing.T) {
12+
t.Run("OnCopyField is called with the source and the copied field ref", func(t *testing.T) {
13+
doc := ast.NewDocument()
14+
fieldRef := doc.AddField(ast.Field{Name: doc.Input.AppendInputString("id")}).Ref
15+
16+
var gotFrom, gotTo int
17+
calls := 0
18+
doc.OnCopyField = func(fieldRef, copyRef int) {
19+
gotFrom, gotTo = fieldRef, copyRef
20+
calls++
21+
}
22+
23+
copyRef := doc.CopyField(fieldRef)
24+
25+
assert.Equal(t, 1, calls)
26+
assert.Equal(t, fieldRef, gotFrom)
27+
assert.Equal(t, copyRef, gotTo)
28+
assert.NotEqual(t, fieldRef, copyRef)
29+
})
30+
31+
t.Run("CopyField without hook does not panic", func(t *testing.T) {
32+
doc := ast.NewDocument()
33+
fieldRef := doc.AddField(ast.Field{Name: doc.Input.AppendInputString("id")}).Ref
34+
35+
copyRef := doc.CopyField(fieldRef)
36+
assert.NotEqual(t, fieldRef, copyRef)
37+
})
38+
39+
t.Run("OnMergeFields is called with the survivor and the removed field ref", func(t *testing.T) {
40+
doc := ast.NewDocument()
41+
left := doc.AddField(ast.Field{Name: doc.Input.AppendInputString("id")}).Ref
42+
right := doc.AddField(ast.Field{Name: doc.Input.AppendInputString("id")}).Ref
43+
44+
var gotSurvivor, gotRemoved int
45+
calls := 0
46+
doc.OnMergeFields = func(survivorRef, removedRef int) {
47+
gotSurvivor, gotRemoved = survivorRef, removedRef
48+
calls++
49+
}
50+
51+
doc.MergeFieldsDefer(left, right)
52+
53+
assert.Equal(t, 1, calls)
54+
assert.Equal(t, left, gotSurvivor)
55+
assert.Equal(t, right, gotRemoved)
56+
})
57+
58+
t.Run("Reset clears the hooks", func(t *testing.T) {
59+
doc := ast.NewDocument()
60+
doc.OnCopyField = func(fieldRef, copyRef int) {}
61+
doc.OnMergeFields = func(survivorRef, removedRef int) {}
62+
63+
doc.Reset()
64+
65+
assert.Nil(t, doc.OnCopyField)
66+
assert.Nil(t, doc.OnMergeFields)
67+
})
68+
}

0 commit comments

Comments
 (0)