Skip to content

Commit c91d59e

Browse files
devsergiydevsergiy
authored
fix: fix merging fetches and add dependencies update (#1232)
<!-- 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 --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved deduplication of fetch operations to merge duplicates and correctly update dependencies and references. * **Tests** * Added a test case verifying dependency updates after deduplication of fetch nodes with overlapping paths. * **Refactor** * Updated fetch operations to access dependencies via pointers and added explicit coordinate dependency access. * Simplified equality checks for fetch configurations by omitting deep coordinate dependency comparisons. * Enhanced fetch tree initialization to create a flat structure before processing, ensuring consistent handling across response plans. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- Please add any additional information or context regarding your changes here. -->
1 parent 2952d70 commit c91d59e

File tree

4 files changed

+272
-28
lines changed

4 files changed

+272
-28
lines changed

‎v2/pkg/engine/postprocess/deduplicate_single_fetches.go‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import (
66
"github.com/wundergraph/graphql-go-tools/v2/pkg/engine/resolve"
77
)
88

9+
// deduplicateSingleFetches is a post-processing step that removes duplicate single fetches
10+
// from the initial fetch tree. It merges their fetch paths and updates dependencies accordingly.
11+
// NOTE: initial tree structure should be flat and contain a single root item with all fetches as children.
912
type deduplicateSingleFetches struct {
1013
disable bool
1114
}
@@ -16,16 +19,52 @@ func (d *deduplicateSingleFetches) ProcessFetchTree(root *resolve.FetchTreeNode)
1619
}
1720
for i := range root.ChildNodes {
1821
for j := i + 1; j < len(root.ChildNodes); j++ {
19-
if root.ChildNodes[i].Item.Equals(root.ChildNodes[j].Item) {
22+
if root.ChildNodes[i].Item.EqualSingleFetch(root.ChildNodes[j].Item) {
2023
root.ChildNodes[i].Item.FetchPath = d.mergeFetchPath(root.ChildNodes[i].Item.FetchPath, root.ChildNodes[j].Item.FetchPath)
2124

25+
newId := root.ChildNodes[i].Item.Fetch.Dependencies().FetchID
26+
oldId := root.ChildNodes[j].Item.Fetch.Dependencies().FetchID
27+
2228
root.ChildNodes = append(root.ChildNodes[:j], root.ChildNodes[j+1:]...)
2329
j--
30+
31+
// when we merge duplicated fetches, we need to update the dependencies of the other fetches
32+
// because they might depend on the fetch that we are removing
33+
d.replaceDependsOnFetchId(root, oldId, newId)
34+
}
35+
}
36+
}
37+
}
38+
39+
// replaceDependsOnFetchId replaces all occurrences of oldId with newId in the dependencies of the fetch tree.
40+
func (d *deduplicateSingleFetches) replaceDependsOnFetchId(root *resolve.FetchTreeNode, oldId, newId int) {
41+
for i := range root.ChildNodes {
42+
replaced := false
43+
for j := range root.ChildNodes[i].Item.Fetch.Dependencies().DependsOnFetchIDs {
44+
if root.ChildNodes[i].Item.Fetch.Dependencies().DependsOnFetchIDs[j] == oldId {
45+
root.ChildNodes[i].Item.Fetch.Dependencies().DependsOnFetchIDs[j] = newId
46+
replaced = true
47+
}
48+
}
49+
50+
if !replaced {
51+
continue
52+
}
53+
54+
for j := range root.ChildNodes[i].Item.Fetch.DependenciesCoordinates() {
55+
for k := range root.ChildNodes[i].Item.Fetch.DependenciesCoordinates()[j].DependsOn {
56+
if root.ChildNodes[i].Item.Fetch.DependenciesCoordinates()[j].DependsOn[k].FetchID == oldId {
57+
root.ChildNodes[i].Item.Fetch.DependenciesCoordinates()[j].DependsOn[k].FetchID = newId
58+
}
2459
}
2560
}
2661
}
2762
}
2863

64+
// mergeFetchPath merges the fetch paths of two single fetches.
65+
// The goal of this method is to merge typename conditions.
66+
// When fetches originate from different parent fragments -
67+
// they will have different typenames, but the same path in response.
2968
func (d *deduplicateSingleFetches) mergeFetchPath(left, right []resolve.FetchItemPathElement) []resolve.FetchItemPathElement {
3069
for i := range left {
3170
left[i].TypeNames = d.mergeTypeNames(left[i].TypeNames, right[i].TypeNames)

‎v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go‎

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,202 @@ func TestDeduplicateSingleFetches_ProcessFetchTree(t *testing.T) {
7171
assert.Equal(t, output, input)
7272
})
7373

74+
t.Run("same path, same input, different fetch ids - should update dependencies with merged fetch ids", func(t *testing.T) {
75+
input := &resolve.FetchTreeNode{
76+
ChildNodes: []*resolve.FetchTreeNode{
77+
{
78+
Kind: resolve.FetchTreeNodeKindSingle,
79+
Item: &resolve.FetchItem{
80+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root"}}},
81+
Fetch: &resolve.SingleFetch{
82+
FetchDependencies: resolve.FetchDependencies{
83+
FetchID: 0,
84+
DependsOnFetchIDs: []int{},
85+
},
86+
FetchConfiguration: resolve.FetchConfiguration{Input: "rootQuery"},
87+
},
88+
},
89+
},
90+
{
91+
Kind: resolve.FetchTreeNodeKindSingle,
92+
Item: &resolve.FetchItem{
93+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root.a"}}},
94+
Fetch: &resolve.SingleFetch{
95+
FetchDependencies: resolve.FetchDependencies{
96+
FetchID: 1,
97+
DependsOnFetchIDs: []int{0},
98+
},
99+
FetchConfiguration: resolve.FetchConfiguration{
100+
Input: "a",
101+
CoordinateDependencies: []resolve.FetchDependency{
102+
{
103+
DependsOn: []resolve.FetchDependencyOrigin{
104+
{
105+
FetchID: 0,
106+
},
107+
},
108+
},
109+
},
110+
},
111+
},
112+
},
113+
},
114+
{
115+
Kind: resolve.FetchTreeNodeKindSingle,
116+
Item: &resolve.FetchItem{
117+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root.a"}}},
118+
Fetch: &resolve.SingleFetch{
119+
FetchDependencies: resolve.FetchDependencies{
120+
FetchID: 2,
121+
DependsOnFetchIDs: []int{0},
122+
},
123+
FetchConfiguration: resolve.FetchConfiguration{
124+
Input: "a",
125+
CoordinateDependencies: []resolve.FetchDependency{
126+
{
127+
DependsOn: []resolve.FetchDependencyOrigin{
128+
{
129+
FetchID: 0,
130+
},
131+
},
132+
},
133+
},
134+
},
135+
},
136+
},
137+
},
138+
{
139+
Kind: resolve.FetchTreeNodeKindSingle,
140+
Item: &resolve.FetchItem{
141+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root.a.b"}}},
142+
Fetch: &resolve.SingleFetch{
143+
FetchDependencies: resolve.FetchDependencies{
144+
FetchID: 4,
145+
DependsOnFetchIDs: []int{0, 2},
146+
},
147+
FetchConfiguration: resolve.FetchConfiguration{
148+
Input: "b",
149+
CoordinateDependencies: []resolve.FetchDependency{
150+
{
151+
DependsOn: []resolve.FetchDependencyOrigin{
152+
{
153+
FetchID: 0,
154+
},
155+
{
156+
FetchID: 2,
157+
},
158+
},
159+
},
160+
},
161+
},
162+
},
163+
},
164+
},
165+
{
166+
Kind: resolve.FetchTreeNodeKindSingle,
167+
Item: &resolve.FetchItem{
168+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root.a.b"}}},
169+
Fetch: &resolve.SingleFetch{
170+
FetchDependencies: resolve.FetchDependencies{
171+
FetchID: 3,
172+
DependsOnFetchIDs: []int{0, 1},
173+
},
174+
FetchConfiguration: resolve.FetchConfiguration{
175+
Input: "b",
176+
CoordinateDependencies: []resolve.FetchDependency{
177+
{
178+
DependsOn: []resolve.FetchDependencyOrigin{
179+
{
180+
FetchID: 0,
181+
},
182+
{
183+
FetchID: 1,
184+
},
185+
},
186+
},
187+
},
188+
},
189+
},
190+
},
191+
},
192+
},
193+
}
194+
195+
output := &resolve.FetchTreeNode{
196+
ChildNodes: []*resolve.FetchTreeNode{
197+
{
198+
Kind: resolve.FetchTreeNodeKindSingle,
199+
Item: &resolve.FetchItem{
200+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root"}}},
201+
Fetch: &resolve.SingleFetch{
202+
FetchDependencies: resolve.FetchDependencies{
203+
FetchID: 0,
204+
DependsOnFetchIDs: []int{},
205+
},
206+
FetchConfiguration: resolve.FetchConfiguration{Input: "rootQuery"},
207+
},
208+
},
209+
},
210+
{
211+
Kind: resolve.FetchTreeNodeKindSingle,
212+
Item: &resolve.FetchItem{
213+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root.a"}}},
214+
Fetch: &resolve.SingleFetch{
215+
FetchDependencies: resolve.FetchDependencies{
216+
FetchID: 1,
217+
DependsOnFetchIDs: []int{0},
218+
},
219+
FetchConfiguration: resolve.FetchConfiguration{
220+
Input: "a",
221+
CoordinateDependencies: []resolve.FetchDependency{
222+
{
223+
DependsOn: []resolve.FetchDependencyOrigin{
224+
{
225+
FetchID: 0,
226+
},
227+
},
228+
},
229+
},
230+
},
231+
},
232+
},
233+
},
234+
{
235+
Kind: resolve.FetchTreeNodeKindSingle,
236+
Item: &resolve.FetchItem{
237+
FetchPath: []resolve.FetchItemPathElement{{Kind: resolve.FetchItemPathElementKindObject, Path: []string{"root.a.b"}}},
238+
Fetch: &resolve.SingleFetch{
239+
FetchDependencies: resolve.FetchDependencies{
240+
FetchID: 4,
241+
DependsOnFetchIDs: []int{0, 1},
242+
},
243+
FetchConfiguration: resolve.FetchConfiguration{
244+
Input: "b",
245+
CoordinateDependencies: []resolve.FetchDependency{
246+
{
247+
DependsOn: []resolve.FetchDependencyOrigin{
248+
{
249+
FetchID: 0,
250+
},
251+
{
252+
FetchID: 1,
253+
},
254+
},
255+
},
256+
},
257+
},
258+
},
259+
},
260+
},
261+
},
262+
}
263+
264+
dedup := &deduplicateSingleFetches{}
265+
dedup.ProcessFetchTree(input)
266+
267+
assert.Equal(t, output, input)
268+
})
269+
74270
t.Run("different path, same input", func(t *testing.T) {
75271
input := &resolve.FetchTreeNode{
76272
ChildNodes: []*resolve.FetchTreeNode{

‎v2/pkg/engine/postprocess/postprocess.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,10 @@ func (p *Processor) Process(pre plan.Plan) plan.Plan {
134134
for i := range p.processResponseTree {
135135
p.processResponseTree[i].Process(t.Response.Data)
136136
}
137+
// initialize the fetch tree
137138
p.createFetchTree(t.Response)
139+
// NOTE: deduplication relies on the fact that the fetch tree
140+
// have flat structure of child fetches
138141
p.dedupe.ProcessFetchTree(t.Response.Fetches)
139142
p.resolveInputTemplates.ProcessFetchTree(t.Response.Fetches)
140143
for i := range p.processFetchTree {
@@ -156,6 +159,8 @@ func (p *Processor) Process(pre plan.Plan) plan.Plan {
156159
return pre
157160
}
158161

162+
// createFetchTree creates an inital fetch tree from the raw fetches in the GraphQL response.
163+
// The initial fetch tree is a node of sequence fetch kind, with a flat list of fetches as children.
159164
func (p *Processor) createFetchTree(res *resolve.GraphQLResponse) {
160165
if p.disableExtractFetches {
161166
return

0 commit comments

Comments
 (0)