From 8a95b71fad60218b370565a4eff09ddc415ebac7 Mon Sep 17 00:00:00 2001 From: Sergiy <818351+devsergiy@users.noreply.github.com> Date: Wed, 27 Dec 2023 10:38:24 +0200 Subject: [PATCH] Sergiy/eng 4688 node suggestions algorithm bug (#709) Fixes ENG-4688 Redo a fix for https://github.com/wundergraph/cosmo/issues/167 --- .../engine/plan/datasource_filter_visitor.go | 209 ++++++++++++------ .../plan/datasource_filter_visitor_test.go | 198 ++++++++++++++++- 2 files changed, 335 insertions(+), 72 deletions(-) diff --git a/v2/pkg/engine/plan/datasource_filter_visitor.go b/v2/pkg/engine/plan/datasource_filter_visitor.go index 8e7dab10e..c387c703e 100644 --- a/v2/pkg/engine/plan/datasource_filter_visitor.go +++ b/v2/pkg/engine/plan/datasource_filter_visitor.go @@ -2,6 +2,7 @@ package plan import ( "fmt" + "slices" "github.com/pkg/errors" @@ -16,6 +17,8 @@ type DataSourceFilter struct { operation *ast.Document definition *ast.Document report *operationreport.Report + + nodes NodeSuggestions } func NewDataSourceFilter(operation, definition *ast.Document, report *operationreport.Report) *DataSourceFilter { @@ -47,23 +50,33 @@ func (f *DataSourceFilter) FilterDataSources(dataSources []DataSourceConfigurati } func (f *DataSourceFilter) findBestDataSourceSet(dataSources []DataSourceConfiguration, existingNodes NodeSuggestions) NodeSuggestions { - nodes := f.collectNodes(dataSources, existingNodes) + f.nodes = f.collectNodes(dataSources, existingNodes) if f.report.HasErrors() { return nil } - nodes = selectUniqNodes(nodes) - nodes = selectDuplicateNodes(nodes, false) - nodes = selectDuplicateNodes(nodes, true) + f.selectUniqNodes() + // f.printNodes("uniq nodes") + f.selectDuplicateNodes(false) + // f.printNodes("duplicate nodes") + f.selectDuplicateNodes(true) + // f.printNodes("duplicate nodes after second run") - nodes = selectedNodes(nodes) + f.nodes = f.selectedNodes() - f.isResolvable(nodes) + f.isResolvable(f.nodes) if f.report.HasErrors() { return nil } - return nodes + return f.nodes +} + +func (f *DataSourceFilter) printNodes(msg string) { + fmt.Println(msg) + for i := range f.nodes { + fmt.Println(f.nodes[i].String()) + } } func (f *DataSourceFilter) collectNodes(dataSources []DataSourceConfiguration, existingNodes NodeSuggestions) (nodes NodeSuggestions) { @@ -124,7 +137,8 @@ func (n *NodeSuggestion) selectWithReason(reason string) { } func (n *NodeSuggestion) String() string { - return fmt.Sprintf(`{"ds":%d,"path":"%s","typeName":"%s","fieldName":"%s","isRootNode":%t, "select reason": %v}`, n.DataSourceHash, n.Path, n.TypeName, n.FieldName, n.IsRootNode, n.selectionReasons) + return fmt.Sprintf(`{"ds":%d,"path":"%s","typeName":"%s","fieldName":"%s","isRootNode":%t, "isSelected": %v,"select reason": %v}`, + n.DataSourceHash, n.Path, n.TypeName, n.FieldName, n.IsRootNode, n.selected, n.selectionReasons) } type NodeSuggestions []NodeSuggestion @@ -407,117 +421,172 @@ const ( ReasonStage3SelectAvailableNode = "stage3: select first available node" ) -func selectUniqNodes(nodes NodeSuggestions) []NodeSuggestion { - for i := range nodes { - if nodes[i].selected { +// selectUniqNodes - selects nodes (e.g. fields) which are unique to a single datasource +// In addition we select: +// - parent of such node if the node is a leaf and not nested under the fragment +// - siblings nodes +func (f *DataSourceFilter) selectUniqNodes() { + for i := range f.nodes { + if f.nodes[i].selected { continue } - isNodeUnique := nodes.isNodeUniq(i) + isNodeUnique := f.nodes.isNodeUniq(i) if !isNodeUnique { continue } // unique nodes always have priority - nodes[i].selectWithReason(ReasonStage1Uniq) - - if !nodes[i].onFragment { // on a first stage do not select parent of nodes on fragments - // if node parent of the unique node is on the same source, prioritize it too - parentIdx, ok := nodes.parentNodeOnSameSource(i) - // Only select the parent on this stage if the node is a leaf; otherwise, the parent is selected elsewhere - if ok && nodes.isLeaf(i) { - nodes[parentIdx].selectWithReason(ReasonStage1SameSourceParent) - } + f.nodes[i].selectWithReason(ReasonStage1Uniq) + + if !f.nodes[i].onFragment { // on a first stage do not select parent of nodes on fragments + // if node parents of the unique node is on the same source, prioritize it too + f.selectUniqNodeParentsUpToRootNode(i) } // if node has leaf children on the same source, prioritize them too - children := nodes.childNodesOnSameSource(i) + children := f.nodes.childNodesOnSameSource(i) for _, child := range children { - if nodes.isLeaf(child) && nodes.isNodeUniq(child) { - nodes[child].selectWithReason(ReasonStage1SameSourceLeafChild) + if f.nodes.isLeaf(child) && f.nodes.isNodeUniq(child) { + f.nodes[child].selectWithReason(ReasonStage1SameSourceLeafChild) } } // prioritize leaf siblings of the node on the same source - siblings := nodes.siblingNodesOnSameSource(i) + siblings := f.nodes.siblingNodesOnSameSource(i) for _, sibling := range siblings { - if nodes.isLeaf(sibling) && nodes.isNodeUniq(sibling) { - nodes[sibling].selectWithReason(ReasonStage1SameSourceLeafSibling) + if f.nodes.isLeaf(sibling) && f.nodes.isNodeUniq(sibling) { + f.nodes[sibling].selectWithReason(ReasonStage1SameSourceLeafSibling) } } } - return nodes } -func selectDuplicateNodes(nodes NodeSuggestions, secondRun bool) []NodeSuggestion { - for i := range nodes { - if nodes[i].selected { - continue +func (f *DataSourceFilter) selectUniqNodeParentsUpToRootNode(i int) { + // When we have a chain of datasource child nodes, we should select every parent until we reach the root node + // as root node is a starting point from where we could get all theese child nodes + + if f.nodes[i].IsRootNode { + // no need to select parent of a root node here + // as it could be resolved by itself + return + } + + current := i + for { + parentIdx, ok := f.nodes.parentNodeOnSameSource(current) + if !ok { + break } + f.nodes[parentIdx].selectWithReason(ReasonStage1SameSourceParent) - if nodes.isSelectedOnOtherSource(i) { + current = parentIdx + if f.nodes[current].IsRootNode { + break + } + } +} + +// selectDuplicateNodes - selects nodes (e.g. fields) which are not unique to a single datasource, +// e.g. could be resolved by multiple datasources +// This method checks only nodes not already selected on the other datasource +// On a first run we are doing set of checks of surrounding nodes selection for the current analyzed node and each of its duplicates: +// - check for selected parent of a current node or its duplicates +// - check for selected childs of a current node or its duplicates +// - check for selected siblings of a current node or its duplicates +// +// On a second run in additional to all the checks from the first run +// we select nodes which was not choosen by previous stages, so we just pick first available datasource +func (f *DataSourceFilter) selectDuplicateNodes(secondRun bool) { + for i := range f.nodes { + if f.nodes[i].selected { continue } - // if node parent on the same source as the current node - parentIdx, ok := nodes.parentNodeOnSameSource(i) - if ok && nodes[parentIdx].selected { - nodes[i].selectWithReason(ReasonStage2SameSourceNodeOfSelectedParent) + if f.nodes.isSelectedOnOtherSource(i) { continue } - isSelected := false + nodeDuplicates := f.nodes.duplicatesOf(i) - // check if duplicates are on the same source as parent node - nodeDuplicates := nodes.duplicatesOf(i) - for _, duplicate := range nodeDuplicates { - parentIdx, ok := nodes.parentNodeOnSameSource(duplicate) - if ok && nodes[parentIdx].selected { - nodes[duplicate].selectWithReason(ReasonStage2SameSourceDuplicateNodeOfSelectedParent) - isSelected = true - break - } + // check for selected parent of a current node or its duplicates + if f.checkNodeParent(i) { + continue } - if isSelected { + if f.checkNodeDuplicates(nodeDuplicates, f.checkNodeParent) { continue } - childs := nodes.childNodesOnSameSource(i) - for _, child := range childs { - if nodes[child].selected { - nodes[i].selectWithReason(ReasonStage2SameSourceNodeOfSelectedChild) - isSelected = true - break - } + // check for selected childs of a current node or its duplicates + if f.checkNodeChilds(i) { + continue } - if isSelected { + if f.checkNodeDuplicates(nodeDuplicates, f.checkNodeChilds) { continue } - siblings := nodes.siblingNodesOnSameSource(i) - for _, sibling := range siblings { - if nodes[sibling].selected { - nodes[i].selectWithReason(ReasonStage2SameSourceNodeOfSelectedSibling) - isSelected = true - break - } + // check for selected siblings of a current node or its duplicates + if f.checkNodeSiblings(i) { + continue } - if isSelected { + if f.checkNodeDuplicates(nodeDuplicates, f.checkNodeSiblings) { continue } + // if after all checks node was not selected, select it + // this could happen in case choises are fully equal if secondRun { - nodes[i].selectWithReason(ReasonStage3SelectAvailableNode) + f.nodes[i].selectWithReason(ReasonStage3SelectAvailableNode) } } - return nodes } -func selectedNodes(nodes NodeSuggestions) (out NodeSuggestions) { - for i := range nodes { - if nodes[i].selected { - out = append(out, nodes[i]) +func (f *DataSourceFilter) selectedNodes() (out NodeSuggestions) { + return slices.DeleteFunc(f.nodes, func(e NodeSuggestion) bool { + return !e.selected + }) +} + +func (f *DataSourceFilter) checkNodeDuplicates(duplicates []int, callback func(nodeIdx int) (nodeIsSelected bool)) (nodeIsSelected bool) { + for _, duplicate := range duplicates { + if callback(duplicate) { + nodeIsSelected = true + break + } + } + return +} + +func (f *DataSourceFilter) checkNodeChilds(i int) (nodeIsSelected bool) { + childs := f.nodes.childNodesOnSameSource(i) + for _, child := range childs { + if f.nodes[child].selected { + f.nodes[i].selectWithReason(ReasonStage2SameSourceNodeOfSelectedChild) + nodeIsSelected = true + break } } return } + +func (f *DataSourceFilter) checkNodeSiblings(i int) (nodeIsSelected bool) { + siblings := f.nodes.siblingNodesOnSameSource(i) + for _, sibling := range siblings { + if f.nodes[sibling].selected { + f.nodes[i].selectWithReason(ReasonStage2SameSourceNodeOfSelectedSibling) + nodeIsSelected = true + break + } + } + return +} + +func (f *DataSourceFilter) checkNodeParent(i int) (nodeIsSelected bool) { + parentIdx, ok := f.nodes.parentNodeOnSameSource(i) + if ok && f.nodes[parentIdx].selected { + f.nodes[i].selectWithReason(ReasonStage2SameSourceNodeOfSelectedParent) + nodeIsSelected = true + } + + return +} diff --git a/v2/pkg/engine/plan/datasource_filter_visitor_test.go b/v2/pkg/engine/plan/datasource_filter_visitor_test.go index 6ccb7ab15..60f050b5e 100644 --- a/v2/pkg/engine/plan/datasource_filter_visitor_test.go +++ b/v2/pkg/engine/plan/datasource_filter_visitor_test.go @@ -265,7 +265,7 @@ func TestFindBestDataSourceSet(t *testing.T) { } union Account = User type AccountProvider { - provider: AccountProvider + accounts: [Account] } type User @key(fields: "id") { id: Int @@ -754,6 +754,45 @@ func TestFindBestDataSourceSet(t *testing.T) { }, }, }, + { + Description: "Shareable: should select details from correct ds", + Definition: shareableDefinition, + Query: ` + query { + me { + details { + pets { + name + } + } + } + } + `, + DataSources: []DataSourceConfiguration{ + shareableDS3, + shareableDS1, + }, + ExpectedVariants: []Variant{ + { + dsOrder: []int{0, 1}, + suggestions: NodeSuggestions{ + {TypeName: "Query", FieldName: "me", DataSourceHash: 11, Path: "query.me", ParentPath: "query", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "details", DataSourceHash: 33, Path: "query.me.details", ParentPath: "query.me", IsRootNode: true, selected: true}, + {TypeName: "Details", FieldName: "pets", DataSourceHash: 33, Path: "query.me.details.pets", ParentPath: "query.me.details", IsRootNode: false, selected: true}, + {TypeName: "Pet", FieldName: "name", DataSourceHash: 33, Path: "query.me.details.pets.name", ParentPath: "query.me.details.pets", IsRootNode: false, selected: true}, + }, + }, + { + dsOrder: []int{1, 0}, + suggestions: NodeSuggestions{ + {TypeName: "Query", FieldName: "me", DataSourceHash: 11, Path: "query.me", ParentPath: "query", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "details", DataSourceHash: 33, Path: "query.me.details", ParentPath: "query.me", IsRootNode: true, selected: true}, + {TypeName: "Details", FieldName: "pets", DataSourceHash: 33, Path: "query.me.details.pets", ParentPath: "query.me.details", IsRootNode: false, selected: true}, + {TypeName: "Pet", FieldName: "name", DataSourceHash: 33, Path: "query.me.details.pets.name", ParentPath: "query.me.details.pets", IsRootNode: false, selected: true}, + }, + }, + }, + }, { Description: "Shareable: should use all ds", Definition: shareableDefinition, @@ -881,6 +920,58 @@ func TestFindBestDataSourceSet(t *testing.T) { }, }, }, + { + Description: "Shareable: conflicting paths with relay style pagination over shareable child nodes", + Definition: conflictingShareablePathsRelayStyleDefinition, + Query: ` + query { + users { + edges { + node { + id + firstName + lastName + address { + street + } + } + } + } + } + `, + DataSources: []DataSourceConfiguration{ + conflictingShareablePathsRelayStyle1, + conflictingShareablePathsRelayStyle2, + }, + ExpectedVariants: []Variant{ + { + dsOrder: []int{0, 1}, + suggestions: NodeSuggestions{ + {TypeName: "Query", FieldName: "users", DataSourceHash: 11, Path: "query.users", ParentPath: "query", IsRootNode: true, selected: true}, + {TypeName: "PaginatedUser", FieldName: "edges", DataSourceHash: 11, Path: "query.users.edges", ParentPath: "query.users", IsRootNode: false, selected: true}, + {TypeName: "UserToEdge", FieldName: "node", DataSourceHash: 11, Path: "query.users.edges.node", ParentPath: "query.users.edges", IsRootNode: false, selected: true}, + {TypeName: "User", FieldName: "id", DataSourceHash: 11, Path: "query.users.edges.node.id", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "firstName", DataSourceHash: 11, Path: "query.users.edges.node.firstName", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "lastName", DataSourceHash: 11, Path: "query.users.edges.node.lastName", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "address", DataSourceHash: 22, Path: "query.users.edges.node.address", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "Address", FieldName: "street", DataSourceHash: 22, Path: "query.users.edges.node.address.street", ParentPath: "query.users.edges.node.address", IsRootNode: true, selected: true}, + }, + }, + { + dsOrder: []int{1, 0}, + suggestions: NodeSuggestions{ + {TypeName: "Query", FieldName: "users", DataSourceHash: 11, Path: "query.users", ParentPath: "query", IsRootNode: true, selected: true}, + {TypeName: "PaginatedUser", FieldName: "edges", DataSourceHash: 11, Path: "query.users.edges", ParentPath: "query.users", IsRootNode: false, selected: true}, + {TypeName: "UserToEdge", FieldName: "node", DataSourceHash: 11, Path: "query.users.edges.node", ParentPath: "query.users.edges", IsRootNode: false, selected: true}, + {TypeName: "User", FieldName: "id", DataSourceHash: 11, Path: "query.users.edges.node.id", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "firstName", DataSourceHash: 11, Path: "query.users.edges.node.firstName", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "lastName", DataSourceHash: 11, Path: "query.users.edges.node.lastName", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "User", FieldName: "address", DataSourceHash: 22, Path: "query.users.edges.node.address", ParentPath: "query.users.edges.node", IsRootNode: true, selected: true}, + {TypeName: "Address", FieldName: "street", DataSourceHash: 22, Path: "query.users.edges.node.address.street", ParentPath: "query.users.edges.node.address", IsRootNode: true, selected: true}, + }, + }, + }, + }, } run := func(t *testing.T, Definition, Query string, DataSources []DataSourceConfiguration, expected NodeSuggestions) { @@ -964,6 +1055,11 @@ const shareableDefinition = ` surname: String! middlename: String! age: Int! + pets: [Pet!] + } + + type Pet { + name: String! } type Query { @@ -1022,12 +1118,18 @@ const shareableDS3Schema = ` type Details { age: Int! + pets: [Pet!] + } + + type Pet { + name: String! } ` var shareableDS3 = dsb().Hash(33).Schema(shareableDS3Schema). RootNode("User", "id", "details"). - ChildNode("Details", "age"). + ChildNode("Details", "age", "pets"). + ChildNode("Pet", "name"). DS() const conflictingPaths1Schema = ` @@ -1108,3 +1210,95 @@ var conflictingPathsDefinition = ` uniqueTwo: Int! } ` + +const conflictingShareablePathsRelayStyleSchema1 = ` + type User @key(fields: "id") { + id: ID! + firstName: String! + lastName: String! + } + + type PaginatedUser @shareable { + edges: [UserToEdge!] + nodes: [User!] + totalCount: Int! + hasNextPage: Boolean! + } + + type UserToEdge @shareable { + node: User! + } + + type Query { + users: PaginatedUser! + }` + +var conflictingShareablePathsRelayStyle1 = dsb().Hash(11).Schema(conflictingShareablePathsRelayStyleSchema1). + RootNode("Query", "users"). + RootNode("User", "id", "firstName", "lastName"). + ChildNode("PaginatedUser", "edges", "nodes", "totalCount", "hasNextPage"). + ChildNode("UserToEdge", "node"). + DS() + +const conflictingShareablePathsRelayStyleSchema2 = ` + type User @key(fields: "id") { + id: ID! + address: Address! + } + + type PaginatedUser @shareable { + edges: [UserToEdge!] + nodes: [User!] + totalCount: Int! + hasNextPage: Boolean! + } + + type UserToEdge @shareable { + node: User! + } + + type Address @key(fields: "id") { + street: String! + residents: PaginatedUser! + } + + type Query { + address(id: ID!): Address! + }` + +var conflictingShareablePathsRelayStyle2 = dsb().Hash(22).Schema(conflictingShareablePathsRelayStyleSchema2). + RootNode("Query", "address"). + RootNode("User", "id", "address"). + RootNode("Address", "street", "residents"). + ChildNode("PaginatedUser", "edges", "nodes", "totalCount", "hasNextPage"). + ChildNode("UserToEdge", "node"). + DS() + +var conflictingShareablePathsRelayStyleDefinition = ` + type User { + id: ID! + firstName: String! + lastName: String! + address: Address! + } + + type PaginatedUser { + edges: [UserToEdge!] + nodes: [User!] + totalCount: Int! + hasNextPage: Boolean! + } + + type UserToEdge { + node: User! + } + + type Address { + street: String! + residents: PaginatedUser! + } + + type Query { + users: PaginatedUser! + address(id: ID!): Address! + }`