[TT-5460] Final PR feedback changes

[changelog] internal: Renamed isTypeEntity; changed logic so document is only changed if the extension is valid; separated exported and unexported const blocks in Plan.
wundergraph · Jun 22, 2022 · 16baec3 · 16baec3
1 parent f7dd1be
commit 16baec3
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 20 deletions.
diff --git a/pkg/engine/plan/local_type_field_extractor.go b/pkg/engine/plan/local_type_field_extractor.go
@@ -4,10 +4,11 @@ import (
 	"github.com/jensneuse/graphql-go-tools/pkg/ast"
 )
 
+const FederationKeyDirectiveName = "key"
+
 const (
-	FederationKeyDirectiveName      = "key"
 	federationRequireDirectiveName  = "requires"
-	FederationExternalDirectiveName = "external"
+	federationExternalDirectiveName = "external"
 )
 
 // LocalTypeFieldExtractor takes an ast.Document as input and generates the
@@ -225,7 +226,7 @@ func (e *LocalTypeFieldExtractor) isRootNode(nodeInfo *nodeInformation) bool {
 func (e *LocalTypeFieldExtractor) collectFieldDefinitions(node ast.Node, nodeInfo *nodeInformation) {
 	for _, ref := range e.document.NodeFieldDefinitions(node) {
 		isExternal := e.document.FieldDefinitionHasNamedDirective(ref,
-			FederationExternalDirectiveName)
+			federationExternalDirectiveName)
 
 		if isExternal {
 			nodeInfo.externalFieldRefs = append(nodeInfo.externalFieldRefs, ref)

diff --git a/pkg/engine/plan/required_field_extractor.go b/pkg/engine/plan/required_field_extractor.go
@@ -42,7 +42,7 @@ func (f *RequiredFieldExtractor) addFieldsForObjectExtensionDefinitions(fieldReq
 		}
 
 		for _, fieldDefinitionRef := range objectType.FieldsDefinition.Refs {
-			if f.document.FieldDefinitionHasNamedDirective(fieldDefinitionRef, FederationExternalDirectiveName) {
+			if f.document.FieldDefinitionHasNamedDirective(fieldDefinitionRef, federationExternalDirectiveName) {
 				continue
 			}
 

diff --git a/pkg/federation/sdlmerge/interface_type_extending.go b/pkg/federation/sdlmerge/interface_type_extending.go
@@ -34,26 +34,30 @@ func (e *extendInterfaceTypeDefinitionVisitor) EnterInterfaceTypeExtension(ref i
 	if !exists {
 		return
 	}
-	hasExtended := false
+
+	var nodeToExtend *ast.Node
 	isEntity := false
-	for _, node := range nodes {
-		if node.Kind != ast.NodeKindInterfaceTypeDefinition {
+	for i := range nodes {
+		if nodes[i].Kind != ast.NodeKindInterfaceTypeDefinition {
 			continue
 		}
-		if hasExtended {
+		if nodeToExtend != nil {
 			e.StopWithExternalErr(*multipleExtensionError(isEntity, nameBytes))
 			return
 		}
 		var err *operationreport.ExternalError
 		extension := e.document.InterfaceTypeExtensions[ref]
-		if isEntity, err = e.collectedEntities.isTypeEntity(nameBytes, extension.HasDirectives, extension.Directives.Refs, e.document); err != nil {
+		if isEntity, err = e.collectedEntities.isExtensionForEntity(nameBytes, extension.Directives.Refs, e.document); err != nil {
 			e.StopWithExternalErr(*err)
 			return
 		}
-		e.document.ExtendInterfaceTypeDefinitionByInterfaceTypeExtension(node.Ref, ref)
-		hasExtended = true
+		nodeToExtend = &nodes[i]
 	}
-	if !hasExtended {
+
+	if nodeToExtend == nil {
 		e.StopWithExternalErr(operationreport.ErrExtensionOrphansMustResolveInSupergraph(e.document.InterfaceTypeExtensionNameBytes(ref)))
+		return
 	}
+
+	e.document.ExtendInterfaceTypeDefinitionByInterfaceTypeExtension(nodeToExtend.Ref, ref)
 }
diff --git a/pkg/federation/sdlmerge/object_type_extending.go b/pkg/federation/sdlmerge/object_type_extending.go
@@ -34,30 +34,34 @@ func (e *extendObjectTypeDefinitionVisitor) EnterObjectTypeExtension(ref int) {
 	if !exists {
 		return
 	}
-	hasExtended := false
+
+	var nodeToExtend *ast.Node
 	isEntity := false
 	shouldReturn := ast.IsRootType(nameBytes)
 	for i := range nodes {
 		if nodes[i].Kind != ast.NodeKindObjectTypeDefinition {
 			continue
 		}
-		if hasExtended {
+		if nodeToExtend != nil {
 			e.StopWithExternalErr(*multipleExtensionError(isEntity, nameBytes))
 			return
 		}
 		var err *operationreport.ExternalError
 		extension := e.document.ObjectTypeExtensions[ref]
-		if isEntity, err = e.collectedEntities.isTypeEntity(nameBytes, extension.HasDirectives, extension.Directives.Refs, e.document); err != nil {
+		if isEntity, err = e.collectedEntities.isExtensionForEntity(nameBytes, extension.Directives.Refs, e.document); err != nil {
 			e.StopWithExternalErr(*err)
 			return
 		}
-		e.document.ExtendObjectTypeDefinitionByObjectTypeExtension(nodes[i].Ref, ref)
+		nodeToExtend = &nodes[i]
 		if shouldReturn {
-			return
+			break
 		}
-		hasExtended = true
 	}
-	if !hasExtended {
+
+	if nodeToExtend == nil {
 		e.StopWithExternalErr(operationreport.ErrExtensionOrphansMustResolveInSupergraph(nameBytes))
+		return
 	}
+
+	e.document.ExtendObjectTypeDefinitionByObjectTypeExtension(nodeToExtend.Ref, ref)
 }
diff --git a/pkg/federation/sdlmerge/sdlmerge.go b/pkg/federation/sdlmerge/sdlmerge.go
@@ -164,8 +164,9 @@ func (m *normalizer) normalize(operation *ast.Document) error {
 	return nil
 }
 
-func (e entitySet) isTypeEntity(nameBytes []byte, hasDirectives bool, directiveRefs []int, document *ast.Document) (bool, *operationreport.ExternalError) {
+func (e entitySet) isExtensionForEntity(nameBytes []byte, directiveRefs []int, document *ast.Document) (bool, *operationreport.ExternalError) {
 	name := string(nameBytes)
+	hasDirectives := len(directiveRefs) > 0
 	if _, exists := e[name]; !exists {
 		if !hasDirectives || !isEntityExtension(directiveRefs, document) {
 			return false, nil