From 3badb72c5e759dfe10832b86f01331708ff861ee Mon Sep 17 00:00:00 2001 From: Anthony Miller Date: Mon, 31 Jul 2023 15:39:03 -0700 Subject: [PATCH 1/4] WIP: implement naive fix --- .../ApolloCodegenLib/Templates/SelectionSetTemplate.swift | 8 ++++++++ .../SelectionSet/SelectionSetTemplateTests.swift | 8 ++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift b/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift index 6c98479fe1..9fa21f588e 100644 --- a/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift +++ b/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift @@ -647,6 +647,14 @@ fileprivate extension IR.MergedSelections.MergedSource { nodesToSharedRoot += 1 } + /// If the shared root is the root of the definition, we should just generate the fully + /// qualified name. + if sourceTypePathCurrentNode.isHead { + return SelectionSetNameGenerator.generatedSelectionSetName( + for: self, format: .fullyQualified, pluralizer: pluralizer + ) + } + let sharedRootIndex = typeInfo.entity.location.fieldPath!.count - (nodesToSharedRoot + 1) diff --git a/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift b/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift index b2d16d5faf..d475c7cd70 100644 --- a/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift +++ b/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift @@ -3667,7 +3667,7 @@ class SelectionSetTemplateTests: XCTestCase { expect(actual).to(equalLineByLine(expected, atLine: 12, ignoringExtraLines: true)) } - func test__render_fieldAccessors__givenEntityFieldMergedFromParent_atOperationRoot_rendersFieldAccessorWithNameNotIncludingParent() throws { + func test__render_fieldAccessors__givenEntityFieldMergedFromParent_atOperationRoot_rendersFieldAccessorWithFullyQualifiedName() throws { // given schemaSDL = """ type Query { @@ -3696,7 +3696,7 @@ class SelectionSetTemplateTests: XCTestCase { let expected = """ public var name: String { __data["name"] } - public var allAnimals: [AllAnimal]? { __data["allAnimals"] } + public var allAnimals: [TestOperationQuery.Data.AllAnimal]? { __data["allAnimals"] } """ // when @@ -3767,7 +3767,7 @@ class SelectionSetTemplateTests: XCTestCase { expect(actual).to(equalLineByLine(expected, atLine: 12, ignoringExtraLines: true)) } - func test__render_fieldAccessors__givenEntityFieldMergedFromSiblingTypeCase_atOperationRoot_rendersFieldAccessorWithNotIncludingSharedParent() throws { + func test__render_fieldAccessors__givenEntityFieldMergedFromSiblingTypeCase_atOperationRoot_rendersFieldAccessorWithFullyQualifiedName() throws { // given schemaSDL = """ type Query { @@ -3803,7 +3803,7 @@ class SelectionSetTemplateTests: XCTestCase { let expected = """ public var name: String { __data["name"] } - public var allAnimals: [AsModeratorQuery.AllAnimal]? { __data["allAnimals"] } + public var allAnimals: [TestOperationQuery.Data.AsModeratorQuery.AllAnimal]? { __data["allAnimals"] } """ // when From 6ce3feb6f486e81d51c782d6cadd17d6397288ae Mon Sep 17 00:00:00 2001 From: Anthony Miller Date: Mon, 31 Jul 2023 16:04:53 -0700 Subject: [PATCH 2/4] Add integration test notes --- .../README.MD | 16 +++++++++++ .../operation.graphql | 15 +++++++++++ .../schema.graphqls | 27 +++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/README.MD create mode 100644 Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/operation.graphql create mode 100644 Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/schema.graphqls diff --git a/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/README.MD b/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/README.MD new file mode 100644 index 0000000000..c743b3e68b --- /dev/null +++ b/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/README.MD @@ -0,0 +1,16 @@ +# Overview + +When merging a selection set with a nested child selection set into a type case that does not also select additional fields on that child selection set, we use a direct reference to the generated selection set in the defining entity selection set. This directly merged child selection set is referenced by its name relative to the type case. + +To calculate the relative name, we determine where the shared root between the definition of the merged child and the target it is being merged into was, and generated the name as only up to their shared root. When the shared root of the directly merged child is the root of the operation, this causes a naming ambiguity problem and a compliation error. + +We must use the fully qualified name in this situation. + +In the example for this test, the `innerChild` field on `AsEventA.Child.AsChildA` would previously have the +type `Child.AsChildA.InnerChild`, which does not exist, because the first component (`Child`) was inferred to be the `AsEventA.Child`. The intention was to point to the `TestFragment.Child.AsChildA.InnerChild`. + +## Reference Issue: https://github.com/apollographql/apollo-ios/pull/3168 + +## Solution + +When the shared root for a directly merged source is the definition root, use the fully qualified selection set name. diff --git a/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/operation.graphql b/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/operation.graphql new file mode 100644 index 0000000000..03ccdac483 --- /dev/null +++ b/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/operation.graphql @@ -0,0 +1,15 @@ +fragment TestFragment on Event { + child { + ... on ChildA { + innerChild: child { + foo + } + } + } + ... on EventA { + bar + } + ... on EventB { + baz + } +} diff --git a/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/schema.graphqls b/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/schema.graphqls new file mode 100644 index 0000000000..6c80f9329b --- /dev/null +++ b/Tests/CodegenIntegrationTests/Tests/3168-mergedSourceFromDefinitionRootNeedsFullyQualifedName/schema.graphqls @@ -0,0 +1,27 @@ +type Query { + test: Event! +} + +interface Event { + child: Child! +} + +type EventA implements Event { + child: Child! + bar: String! +} + +type EventB implements Event { + child: Child! + baz: String! +} + +interface Child { + foo: String! + child: Child +} + +type ChildA implements Child { + foo: String! + child: Child +} From 5234aaaff2008e14c9dd7be96bcf2eb20289de9b Mon Sep 17 00:00:00 2001 From: Anthony Miller Date: Mon, 31 Jul 2023 16:25:05 -0700 Subject: [PATCH 3/4] Clean up --- .../ApolloCodegenLib/Templates/SelectionSetTemplate.swift | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift b/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift index 9fa21f588e..3ddd2a0189 100644 --- a/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift +++ b/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift @@ -665,11 +665,7 @@ fileprivate extension IR.MergedSelections.MergedSource { /// /// Example: The `height` field on `AllAnimals.AsPet` can reference the `AllAnimals.Height` /// object as just `Height`. - /// - /// However, if the shared root is the root of the definition, the component that would be - /// removed is the location's `source`. This is not included in the field path and is already - /// omitted by this function. In this case, the `sharedRootIndex` is `-1`. - let removeFirstComponent = nodesToSharedRoot <= 1 && !(sharedRootIndex < 0) + let removeFirstComponent = nodesToSharedRoot <= 1 let fieldPath = typeInfo.entity.location.fieldPath!.node( at: max(0, sharedRootIndex) From 023c25dc100e787a3057f3135da9094e97221863 Mon Sep 17 00:00:00 2001 From: Anthony Miller Date: Mon, 31 Jul 2023 16:58:45 -0700 Subject: [PATCH 4/4] Fix incorrect condition name generation with type cases from merged sources in fullfilled fragments --- .../AllAnimalsIncludeSkipQuery.graphql.swift | 2 - .../Queries/AllAnimalsQuery.graphql.swift | 9 +-- .../Templates/SelectionSetTemplate.swift | 2 +- ...ectionSetTemplate_Initializers_Tests.swift | 75 +++++++++++++++++++ 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsIncludeSkipQuery.graphql.swift b/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsIncludeSkipQuery.graphql.swift index 8bd3d1cd7e..b8e9d947af 100644 --- a/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsIncludeSkipQuery.graphql.swift +++ b/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsIncludeSkipQuery.graphql.swift @@ -692,7 +692,6 @@ public class AllAnimalsIncludeSkipQuery: GraphQLQuery { ObjectIdentifier(AllAnimalsIncludeSkipQuery.Data.AllAnimal.AsPet.self), ObjectIdentifier(WarmBloodedDetails.self), ObjectIdentifier(AllAnimalsIncludeSkipQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self), - ObjectIdentifier(AllAnimalsIncludeSkipQuery.Data.AllAnimal.AsPet.AsWarmBlooded.AsWarmBlooded.self), ObjectIdentifier(HeightInMeters.self) ] )) @@ -853,7 +852,6 @@ public class AllAnimalsIncludeSkipQuery: GraphQLQuery { ObjectIdentifier(AllAnimalsIncludeSkipQuery.Data.AllAnimal.AsPet.self), ObjectIdentifier(WarmBloodedDetails.self), ObjectIdentifier(AllAnimalsIncludeSkipQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self), - ObjectIdentifier(AllAnimalsIncludeSkipQuery.Data.AllAnimal.AsPet.AsWarmBlooded.AsWarmBlooded.self), ObjectIdentifier(HeightInMeters.self) ] )) diff --git a/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsQuery.graphql.swift b/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsQuery.graphql.swift index 61c5b7181b..cd6cbaabed 100644 --- a/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsQuery.graphql.swift +++ b/Sources/AnimalKingdomAPI/AnimalKingdomAPI/Sources/Operations/Queries/AllAnimalsQuery.graphql.swift @@ -625,8 +625,7 @@ public class AllAnimalsQuery: GraphQLQuery { ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsWarmBlooded.self), ObjectIdentifier(PetDetails.self), ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.self), - ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self), - ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.AsWarmBlooded.self) + ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self) ] )) } @@ -822,8 +821,7 @@ public class AllAnimalsQuery: GraphQLQuery { ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsWarmBlooded.self), ObjectIdentifier(PetDetails.self), ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.self), - ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self), - ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.AsWarmBlooded.self) + ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self) ] )) } @@ -936,8 +934,7 @@ public class AllAnimalsQuery: GraphQLQuery { ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsWarmBlooded.self), ObjectIdentifier(PetDetails.self), ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.self), - ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self), - ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.AsWarmBlooded.self) + ObjectIdentifier(AllAnimalsQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self) ] )) } diff --git a/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift b/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift index 3ddd2a0189..435f441373 100644 --- a/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift +++ b/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift @@ -732,7 +732,7 @@ fileprivate extension IR.MergedSelections.MergedSource { mergedFragmentEntityConditionPathNode = node } selectionSetNameComponents.append( - SelectionSetNameGenerator.ConditionPath.path(for: node) + node.value.selectionSetNameComponent ) fulfilledFragments.append(selectionSetNameComponents.joined(separator: ".")) } diff --git a/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplate_Initializers_Tests.swift b/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplate_Initializers_Tests.swift index cba5b7db9d..76ea47b284 100644 --- a/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplate_Initializers_Tests.swift +++ b/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplate_Initializers_Tests.swift @@ -477,6 +477,81 @@ class SelectionSetTemplate_Initializers_Tests: XCTestCase { // then expect(actual).to(equalLineByLine(expected, atLine: 16, ignoringExtraLines: true)) } + + func test__render_givenNestedTypeCasesMergedFromSibling_fulfilledFragmentsIncludesAllTypeCasesInScope() throws { + // given + schemaSDL = """ + type Query { + allAnimals: [Animal!] + } + + interface Animal { + species: String! + } + + interface Pet { + species: String! + } + + interface WarmBlooded { + species: String! + } + + type Cat implements Animal & Pet & WarmBlooded { + species: String! + isJellicle: Boolean! + } + """ + + document = """ + query TestOperation { + allAnimals { + ... on Pet { + ... on WarmBlooded { + species + } + } + ... on Cat { + isJellicle + } + } + } + """ + + let expected = + """ + public init( + isJellicle: Bool, + species: String + ) { + self.init(_dataDict: DataDict( + data: [ + "__typename": TestSchema.Objects.Cat.typename, + "isJellicle": isJellicle, + "species": species, + ], + fulfilledFragments: [ + ObjectIdentifier(TestOperationQuery.Data.AllAnimal.self), + ObjectIdentifier(TestOperationQuery.Data.AllAnimal.AsCat.self), + ObjectIdentifier(TestOperationQuery.Data.AllAnimal.AsPet.self), + ObjectIdentifier(TestOperationQuery.Data.AllAnimal.AsPet.AsWarmBlooded.self) + ] + )) + } + """ + + // when + try buildSubjectAndOperation() + + let allAnimals_asCat = try XCTUnwrap( + operation[field: "query"]?[field: "allAnimals"]?[as: "Cat"] + ) + + let actual = subject.render(inlineFragment: allAnimals_asCat) + + // then + expect(actual).to(equalLineByLine(expected, atLine: 17, ignoringExtraLines: true)) + } // MARK: Selection Tests