wip: Fix the Discard() bug, and make the TryToFindNameFromIndex

function also look and ChildMappings to find the name of the index, Add the test that was failing before, also make `TryToFindNameFromIndex` now follow the boolean try pattern instead of returning error.
sourcenetwork · Jul 5, 2022 · 4857a7b · 4857a7b
1 parent e669138
commit 4857a7b
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 68 deletions.
diff --git a/core/doc.go b/core/doc.go
@@ -13,8 +13,6 @@ Package core provides commonly shared interfaces and building blocks.
 */
 package core
 
-import "fmt"
-
 const DocKeyFieldIndex int = 0
 
 type DocFields []interface{}
@@ -225,17 +223,26 @@ func (m *DocumentMapping) SetChildAt(index int, childMapping DocumentMapping) {
 	m.ChildMappings = newMappings
 }
 
-// FindNameFromIndex returns the corresponding name of the given index.
+// TryToFindNameFromIndex returns the corresponding name of the given index.
 //
-// Will return error if the index is not found.
-func (mapping *DocumentMapping) FindNameFromIndex(targetIndex int) (string, error) {
+// Additionally, will also return true if the index was found, and false otherwise.
+func (mapping *DocumentMapping) TryToFindNameFromIndex(targetIndex int) (string, bool) {
+	// Try to find the name of this index in the IndexesByName.
 	for name, indexes := range mapping.IndexesByName {
 		for _, index := range indexes {
 			if index == targetIndex {
-				return name, nil
+				return name, true
 			}
 		}
 	}
 
-	return "", fmt.Errorf("No corresponding name was found for index=%d", targetIndex)
+	// Try to find the name of this index in the ChildMappings.
+	for _, childMapping := range mapping.ChildMappings {
+		name, found := childMapping.TryToFindNameFromIndex(targetIndex)
+		if found {
+			return name, true
+		}
+	}
+
+	return "", false
 }
diff --git a/query/graphql/planner/planner.go b/query/graphql/planner/planner.go
@@ -455,7 +455,7 @@ func (p *Planner) explainRequest(
 
 	explainGraph, err := buildExplainGraph(plan)
 	if err != nil {
-		return nil, err
+		return nil, multiErr(err, plan.Close())
 	}
 
 	topExplainGraph := []map[string]interface{}{

diff --git a/query/graphql/planner/sort.go b/query/graphql/planner/sort.go
@@ -11,6 +11,8 @@
 package planner
 
 import (
+	"fmt"
+
 	"github.com/sourcenetwork/defradb/core"
 	"github.com/sourcenetwork/defradb/query/graphql/mapper"
 )
@@ -110,9 +112,9 @@ func (n *sortNode) Explain() (map[string]interface{}, error) {
 		fieldNames := []string{}
 		for _, fieldIndex := range element.FieldIndexes {
 			// Try to find the name of this index.
-			fieldName, err := n.documentMapping.FindNameFromIndex(fieldIndex)
-			if err != nil {
-				return nil, err
+			fieldName, found := n.documentMapping.TryToFindNameFromIndex(fieldIndex)
+			if !found {
+				return nil, fmt.Errorf("No corresponding name was found for index=%d", fieldIndex)
 			}
 
 			fieldNames = append(fieldNames, fieldName)

diff --git a/tests/integration/query/explain/with_sort_test.go b/tests/integration/query/explain/with_sort_test.go
@@ -382,60 +382,115 @@ func TestExplainQueryWithOrderOnBothTheParentAndChild(t *testing.T) {
 	executeTestCase(t, test)
 }
 
-// Add Following Test Post Bug-Fix (https://github.com/sourcenetwork/defradb/issues/584).
-//func TestExplainQueryWhereParentIsOrderedByChild(t *testing.T) {
-//	test := testUtils.QueryTestCase{
-//		Description: "Explain Query Where The Parent Is Ordered By It's Child.",
-//
-//		Query: `query @explain {
-//			author(
-//				order: {
-//					articles: {name: ASC}
-//				}
-//			) {
-//				name
-//				articles {
-//				    name
-//				}
-//			}
-//		}`,
-//
-//		Docs: map[int][]string{
-//			// articles
-//			0: {
-//				`{
-//					"name": "After Guantánamo, Another Injustice",
-//					"author_id": "bae-41598f0c-19bc-5da6-813b-e80f14a10df3"
-//				}`,
-//				`{
-//					"name": "To my dear readers",
-//					"author_id": "bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04"
-//				}`,
-//				`{
-//					"name": "Twinklestar's Favourite Xmas Cookie",
-//					"author_id": "bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04"
-//				}`,
-//			},
-//
-//			// authors
-//			2: {
-//				// _key: bae-41598f0c-19bc-5da6-813b-e80f14a10df3
-//				`{
-//					"name": "John Grisham",
-//					"age": 65,
-//					"verified": true
-//				}`,
-//				// _key: bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04
-//				`{
-//					"name": "Cornelia Funke",
-//					"age": 62,
-//					"verified": false
-//				}`,
-//			},
-//		},
-//
-//		Results: []dataMap{},
-//	}
-//
-//	executeTestCase(t, test)
-//}
+func TestExplainQueryWhereParentIsOrderedByChild(t *testing.T) {
+	test := testUtils.QueryTestCase{
+		Description: "Explain Query Where The Parent Is Ordered By It's Child.",
+
+		Query: `query @explain {
+			author(
+				order: {
+					articles: {name: ASC}
+				}
+			) {
+				articles {
+				    name
+				}
+			}
+		}`,
+
+		Docs: map[int][]string{
+			// articles
+			0: {
+				`{
+					"name": "After Guantánamo, Another Injustice",
+					"author_id": "bae-41598f0c-19bc-5da6-813b-e80f14a10df3"
+				}`,
+				`{
+					"name": "To my dear readers",
+					"author_id": "bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04"
+				}`,
+				`{
+					"name": "Twinklestar's Favourite Xmas Cookie",
+					"author_id": "bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04"
+				}`,
+			},
+
+			// authors
+			2: {
+				// _key: bae-41598f0c-19bc-5da6-813b-e80f14a10df3
+				`{
+					"name": "John Grisham",
+					"age": 65,
+					"verified": true
+				}`,
+				// _key: bae-b769708d-f552-5c3d-a402-ccfd7ac7fb04
+				`{
+					"name": "Cornelia Funke",
+					"age": 62,
+					"verified": false
+				}`,
+			},
+		},
+
+		Results: []dataMap{
+			{
+				"explain": dataMap{
+					"selectTopNode": dataMap{
+						"sortNode": dataMap{
+							"orderings": []dataMap{
+								{
+									"direction": "ASC",
+									"fields": []string{
+										"articles",
+										"name",
+									},
+								},
+							},
+							"selectNode": dataMap{
+								"filter": nil,
+								"typeIndexJoin": dataMap{
+									"joinType": "typeJoinMany",
+									"rootName": "author",
+									"root": dataMap{
+										"scanNode": dataMap{
+											"collectionID":   "3",
+											"collectionName": "author",
+											"filter":         nil,
+											"spans": []dataMap{
+												{
+													"start": "/3",
+													"end":   "/4",
+												},
+											},
+										},
+									},
+									"subTypeName": "articles",
+									"subType": dataMap{
+										"selectTopNode": dataMap{
+											"selectNode": dataMap{
+												"filter": nil,
+												"scanNode": dataMap{
+													"collectionID":   "1",
+													"collectionName": "article",
+													"filter":         nil,
+													"spans": []dataMap{
+														{
+															"start": "/1",
+															"end":   "/2",
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	executeTestCase(t, test)
+}