Skip to content

Commit 1a7ed16

Browse files
authored
fix: planner fixes for parent entity jumps and unique nodes selections (#1230)
fixes ENG-7397 fixes ENG-7355 fixes ENG-7228 <!-- 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 * **New Features** * Added a new validation rule to detect and report empty selection sets in dynamically generated GraphQL operations. * **Bug Fixes** * Improved internal logic for selecting parent and duplicate nodes in data source planning, resulting in more accurate and streamlined node selection. * **Tests** * Updated test descriptions to reflect changes in node selection reasoning, ensuring test clarity and consistency. <!-- 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 2b96f7e commit 1a7ed16

File tree

4 files changed

+113
-78
lines changed

4 files changed

+113
-78
lines changed
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package astvalidation
2+
3+
import (
4+
"fmt"
5+
6+
"github.com/wundergraph/graphql-go-tools/v2/pkg/ast"
7+
"github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor"
8+
)
9+
10+
// ValidateEmptySelectionSets validates if selection sets are not empty
11+
// should be used only when the operation is created on the fly
12+
func ValidateEmptySelectionSets() Rule {
13+
return func(walker *astvisitor.Walker) {
14+
visitor := emptySelectionSetVisitor{
15+
Walker: walker,
16+
}
17+
walker.RegisterEnterDocumentVisitor(&visitor)
18+
walker.RegisterEnterSelectionSetVisitor(&visitor)
19+
}
20+
}
21+
22+
type emptySelectionSetVisitor struct {
23+
*astvisitor.Walker
24+
operation, definition *ast.Document
25+
}
26+
27+
func (r *emptySelectionSetVisitor) EnterDocument(operation, definition *ast.Document) {
28+
r.operation = operation
29+
r.definition = definition
30+
}
31+
32+
func (r *emptySelectionSetVisitor) EnterSelectionSet(ref int) {
33+
if r.operation.SelectionSetIsEmpty(ref) {
34+
r.Walker.StopWithInternalErr(fmt.Errorf("astvalidation selection set on path %s is empty", r.Walker.Path.DotDelimitedString()))
35+
}
36+
}

v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1684,11 +1684,18 @@ type printKit struct {
16841684
var (
16851685
printKitPool = &sync.Pool{
16861686
New: func() any {
1687+
validator := astvalidation.DefaultOperationValidator()
1688+
// as we are creating operation programmatically in the graphql datasource planner,
1689+
// we need to catch incorrect behavior of the planner
1690+
// as graphql datasource planner should visit only selection sets which has fields,
1691+
// landed to the current planner
1692+
validator.RegisterRule(astvalidation.ValidateEmptySelectionSets())
1693+
16871694
return &printKit{
16881695
buf: &bytes.Buffer{},
16891696
parser: astparser.NewParser(),
16901697
printer: astprinter.NewPrinter(nil),
1691-
validator: astvalidation.DefaultOperationValidator(),
1698+
validator: validator,
16921699
normalizer: astnormalization.NewWithOpts(
16931700
astnormalization.WithExtractVariables(),
16941701
astnormalization.WithRemoveFragmentDefinitions(),

v2/pkg/engine/plan/datasource_filter_visitor.go

Lines changed: 58 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,8 @@ func (f *DataSourceFilter) collectNodes(dataSources []DataSource, existingNodes
197197
}
198198

199199
const (
200-
ReasonStage1Unique = "stage1: unique"
201-
ReasonStage1SameSourceParent = "stage1: same source parent of unique node"
202-
ReasonStage1SameSourceLeafChild = "stage1: same source leaf child of unique node"
203-
ReasonStage1SameSourceLeafSibling = "stage1: same source leaf sibling of unique node"
200+
ReasonStage1Unique = "stage1: unique"
201+
ReasonStage1SameSourceParent = "stage1: same source parent of unique node"
204202

205203
ReasonStage2SameSourceNodeOfSelectedParent = "stage2: node on the same source as selected parent"
206204
ReasonStage2SameSourceNodeOfSelectedChild = "stage2: node on the same source as selected child"
@@ -218,73 +216,63 @@ const (
218216
)
219217

220218
// selectUniqueNodes - selects nodes (e.g. fields) which are unique to a single datasource
221-
// In addition we select:
222-
// - parent of such node if the node is a leaf and not nested under the fragment
223-
// - siblings nodes
224219
func (f *DataSourceFilter) selectUniqueNodes() {
225220

226221
for i := range f.nodes.items {
227222
if f.nodes.items[i].Selected {
228223
continue
229224
}
230225

231-
isNodeUnique := f.nodes.isNodeUnique(i)
232-
if !isNodeUnique {
226+
if !f.nodes.isNodeUnique(i) {
233227
continue
234228
}
235229

236230
// unique nodes always have priority
237231
f.nodes.items[i].selectWithReason(ReasonStage1Unique, f.enableSelectionReasons)
238232

239-
if !f.nodes.items[i].onFragment { // on a first stage do not select parent of nodes on fragments
240-
// if node parents of the unique node is on the same source, prioritize it too
241-
f.selectUniqNodeParentsUpToRootNode(i)
242-
}
243-
244-
// if node has leaf children on the same source, prioritize them too
245-
children := f.nodes.childNodesOnSameSource(i)
246-
for _, child := range children {
247-
if f.nodes.isLeaf(child) && f.nodes.isNodeUnique(child) {
248-
f.nodes.items[child].selectWithReason(ReasonStage1SameSourceLeafChild, f.enableSelectionReasons)
249-
}
250-
}
251-
252-
// prioritize leaf siblings of the node on the same source
253-
siblings := f.nodes.siblingNodesOnSameSource(i)
254-
for _, sibling := range siblings {
255-
if f.nodes.isLeaf(sibling) && f.nodes.isNodeUnique(sibling) {
256-
f.nodes.items[sibling].selectWithReason(ReasonStage1SameSourceLeafSibling, f.enableSelectionReasons)
257-
}
258-
}
233+
f.selectUniqNodeParentsUpToRootNode(i)
259234
}
260235
}
261236

262237
func (f *DataSourceFilter) selectUniqNodeParentsUpToRootNode(i int) {
263238
// When we have a chain of datasource child nodes, we should select every parent until we reach the root node
264-
// as root node is a starting point from where we could get all these child nodes
239+
// as a root node is a starting point from where we could get all these child nodes
265240

266241
if f.nodes.items[i].IsRootNode {
267-
// no need to select parent of a root node here
242+
// no need to select the parent of a root node here
268243
// as it could be resolved by itself
269244
return
270245
}
271246

247+
rootNodeFound := false
248+
nodesIdsToSelect := make([]int, 0, 2)
272249
current := i
273250
for {
274251
parentIdx, ok := f.nodes.parentNodeOnSameSource(current)
275252
if !ok {
276253
break
277254
}
255+
nodesIdsToSelect = append(nodesIdsToSelect, parentIdx)
278256

279-
if !f.selectWithExternalCheck(parentIdx, ReasonStage1SameSourceParent) {
280-
// when we see an external node in the chain, we stop selectiong process
257+
if f.nodes.items[parentIdx].IsExternal && !f.nodes.items[i].IsProvided {
258+
// such parent can't be selected
281259
break
282260
}
283261

284-
current = parentIdx
285-
if f.nodes.items[current].IsRootNode {
262+
if f.nodes.items[parentIdx].IsRootNode && !f.nodes.items[parentIdx].DisabledEntityResolver {
263+
rootNodeFound = true
286264
break
287265
}
266+
267+
current = parentIdx
268+
}
269+
270+
if !rootNodeFound {
271+
return
272+
}
273+
274+
for _, parentIdx := range nodesIdsToSelect {
275+
f.nodes.items[parentIdx].selectWithReason(ReasonStage1SameSourceParent, f.enableSelectionReasons)
288276
}
289277
}
290278

@@ -529,36 +517,9 @@ func (f *DataSourceFilter) selectDuplicateNodes(secondPass bool) {
529517
continue
530518
}
531519

532-
// 2. Lookup for the first parent root node with enabled entity resolver
533-
// when we haven't found a possible duplicate
534-
// we need to find parent node which is a root node and has enabled entity resolver, e.g. the point in the query from where we could jump
535-
// it is a parent entity jump case
536-
537-
if f.checkNodes(itemIDs,
538-
func(i int) bool {
539-
if f.nodes.items[i].IsExternal && !f.nodes.items[i].IsProvided {
540-
return false
541-
}
542-
543-
parents := f.findPossibleParents(i)
544-
if len(parents) > 0 {
545-
if f.selectWithExternalCheck(i, ReasonStage3SelectNodeUnderFirstParentRootNode) {
546-
for _, parent := range parents {
547-
f.nodes.items[parent].selectWithReason(ReasonStage3SelectParentRootNodeWithEnabledEntityResolver, f.enableSelectionReasons)
548-
}
549-
550-
return true
551-
}
552-
}
553-
return false
554-
},
555-
nil) {
556-
continue
557-
}
558-
559-
// stages 3,4,5 - are stages when choices are equal, and we should select first available node
520+
// stages 2,3,4 - are stages when choices are equal, and we should select first available node
560521

561-
// 3. we choose first available leaf node
522+
// 2. we choose the first available leaf node
562523
if f.checkNodes(itemIDs,
563524
func(i int) bool {
564525
return f.selectWithExternalCheck(i, ReasonStage3SelectAvailableLeafNode)
@@ -582,7 +543,7 @@ func (f *DataSourceFilter) selectDuplicateNodes(secondPass bool) {
582543
continue
583544
}
584545

585-
// 4. if node is not a leaf we select a node which could provide more selections on the same source
546+
// 3. if node is not a leaf we select a node which could provide more selections on the same source
586547
currentChildNodeCount := -1
587548
currentItemIDx := -1
588549

@@ -605,7 +566,7 @@ func (f *DataSourceFilter) selectDuplicateNodes(secondPass bool) {
605566
continue
606567
}
607568

608-
// 5. We check here not leaf nodes which could provide keys to the child nodes
569+
// 4. We check here not leaf nodes which could provide keys to the child nodes
609570
// this rule one of the rules responsible for the shareable nodes
610571
if f.checkNodes(itemIDs,
611572
func(i int) bool {
@@ -630,6 +591,37 @@ func (f *DataSourceFilter) selectDuplicateNodes(secondPass bool) {
630591
}) {
631592
continue
632593
}
594+
595+
// 5. Lookup for the first parent root node with the enabled entity resolver.
596+
// When we haven't found a possible duplicate -
597+
// we need to find the parent node which is a root node and has enabled entity resolver,
598+
// e.g., the point in the query from where we could jump.
599+
// It is a parent entity jump case, and it is less preferable,
600+
// than direct entity jump, so it should go last.
601+
602+
// TODO: replace with all nodes check - select smallest parent entity chain
603+
if f.checkNodes(itemIDs,
604+
func(i int) bool {
605+
if f.nodes.items[i].IsExternal && !f.nodes.items[i].IsProvided {
606+
return false
607+
}
608+
609+
parents := f.findPossibleParents(i)
610+
if len(parents) > 0 {
611+
if f.selectWithExternalCheck(i, ReasonStage3SelectNodeUnderFirstParentRootNode) {
612+
for _, parent := range parents {
613+
f.nodes.items[parent].selectWithReason(ReasonStage3SelectParentRootNodeWithEnabledEntityResolver, f.enableSelectionReasons)
614+
}
615+
616+
return true
617+
}
618+
}
619+
return false
620+
},
621+
nil) {
622+
continue
623+
}
624+
633625
}
634626
}
635627

0 commit comments

Comments
 (0)