From f9110898e3c4a12429f7fd4edae41baee0a55fae Mon Sep 17 00:00:00 2001 From: Calvin Cestari Date: Fri, 9 Feb 2024 13:57:08 -0800 Subject: [PATCH] fix: Type case with only reserved fields is not composite inline fragment (apollographql/apollo-ios-dev#261) --- .../IRBuilder+Mocking.swift | 6 +- .../Templates/FragmentTemplateTests.swift | 2 - .../SelectionSetTemplateTests.swift | 99 +++++++++++++++++++ .../README.MD | 11 +++ .../operation.graphql | 7 ++ .../schema.graphqls | 13 +++ .../Templates/SelectionSetTemplate.swift | 39 ++++---- .../Sources/IR/IR+ComputedSelectionSet.swift | 6 +- .../Sources/IR/IR+DirectSelections.swift | 10 +- .../Sources/IR/IR+RootFieldBuilder.swift | 6 +- .../Sources/IR/IR+SelectionSet.swift | 12 ++- 11 files changed, 182 insertions(+), 29 deletions(-) create mode 100644 Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/README.MD create mode 100644 Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/operation.graphql create mode 100644 Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/schema.graphqls diff --git a/Tests/ApolloCodegenInternalTestHelpers/IRBuilder+Mocking.swift b/Tests/ApolloCodegenInternalTestHelpers/IRBuilder+Mocking.swift index 3fb44ab9f..254fc5154 100644 --- a/Tests/ApolloCodegenInternalTestHelpers/IRBuilder+Mocking.swift +++ b/Tests/ApolloCodegenInternalTestHelpers/IRBuilder+Mocking.swift @@ -71,7 +71,8 @@ extension IR.NamedFragment { inclusionConditions: nil, givenAllTypesInSchema: .init([type], schemaRootTypes: .mock()) ) - ] + ], + isUserDefined: true ) let rootEntityField = IR.EntityField.init( rootField, @@ -110,7 +111,8 @@ extension IR.Operation { forType: .mock(), inclusionConditions: nil, givenAllTypesInSchema: .init([], schemaRootTypes: .mock())) - ] + ], + isUserDefined: true ) let rootField = IR.EntityField( .mock(), diff --git a/Tests/ApolloCodegenTests/CodeGeneration/Templates/FragmentTemplateTests.swift b/Tests/ApolloCodegenTests/CodeGeneration/Templates/FragmentTemplateTests.swift index 1f56a6f07..72cbef648 100644 --- a/Tests/ApolloCodegenTests/CodeGeneration/Templates/FragmentTemplateTests.swift +++ b/Tests/ApolloCodegenTests/CodeGeneration/Templates/FragmentTemplateTests.swift @@ -303,8 +303,6 @@ class FragmentTemplateTests: XCTestCase { init(_dataDict: DataDict) { __data = _dataDict } static var __parentType: ApolloAPI.ParentType { TestSchema.Objects.Query } - static var __selections: [ApolloAPI.Selection] { [ - ] } } """ diff --git a/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift b/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift index 71e01a9da..c2e415ea1 100644 --- a/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift +++ b/Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift @@ -1447,6 +1447,57 @@ class SelectionSetTemplateTests: XCTestCase { expect(actual).to(equalLineByLine(expected, atLine: 7, ignoringExtraLines: true)) } + // Related to https://github.com/apollographql/apollo-ios/issues/3326 + func test__render_selections__givenTypeCaseWithOnlyReservedField_doesNotRenderSelections() async throws { + // given + schemaSDL = """ + type Query { + allAnimals: [Animal] + } + + union Animal = AnimalObject | AnimalError + + type AnimalObject { + species: String! + } + + type AnimalError { + code: Int! + } + """ + + document = """ + query TestOperation { + allAnimals { + ... on AnimalObject { + __typename + } + } + } + """ + + let expected = """ + public struct AsAnimalObject: TestSchema.InlineFragment { + public let __data: DataDict + public init(_dataDict: DataDict) { __data = _dataDict } + + public typealias RootEntityType = TestOperationQuery.Data.AllAnimal + public static var __parentType: ApolloAPI.ParentType { TestSchema.Objects.AnimalObject } + } + """ + + // when + try await buildSubjectAndOperation() + let allAnimals_asAnimalObject = try XCTUnwrap( + operation[field: "query"]?[field: "allAnimals"]?[as: "AnimalObject"] + ) + + let actual = subject.test_render(inlineFragment: allAnimals_asAnimalObject.computed) + + // then + expect(actual).to(equalLineByLine(expected, atLine: 2, ignoringExtraLines: true)) + } + // MARK: Selections - Fragments func test__render_selections__givenFragments_rendersFragmentSelections() async throws { @@ -6952,6 +7003,54 @@ class SelectionSetTemplateTests: XCTestCase { .to(equalLineByLine(predator_expected, atLine: 21, ignoringExtraLines: true)) } +// Related to https://github.com/apollographql/apollo-ios/issues/3326 + func test__render_nestedSelectionSet__givenInlineFragmentWithOnlyReservedField_doesNotRenderAsCompositeInlineFragment() async throws { + // given + schemaSDL = """ + type Query { + allAnimals: [Animal] + } + + union Animal = AnimalObject | AnimalError + + type AnimalObject { + species: String! + } + + type AnimalError { + code: Int! + } + """ + + document = """ + query TestOperation { + allAnimals { + ... on AnimalObject { + __typename + } + } + } + """ + + let expected = """ + public var asAnimalObject: AsAnimalObject? { _asInlineFragment() } + + /// AllAnimal.AsAnimalObject + public struct AsAnimalObject: TestSchema.InlineFragment { + """ + + // when + try await buildSubjectAndOperation() + let allAnimals = try XCTUnwrap( + operation[field: "query"]?[field: "allAnimals"]?.selectionSet + ) + + let actual = subject.test_render(childEntity: allAnimals.computed) + + // then + expect(actual).to(equalLineByLine(expected, atLine: 12, ignoringExtraLines: true)) + } + // MARK: Nested Selection Sets - Reserved Keywords + Special Names func test__render_nestedSelectionSet__givenEntityFieldWithSwiftKeywordAndApolloReservedTypeNames_rendersSelectionSetWithNameSuffixed() async throws { diff --git a/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/README.MD b/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/README.MD new file mode 100644 index 000000000..c1fa2c8d8 --- /dev/null +++ b/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/README.MD @@ -0,0 +1,11 @@ +# Overview + +When a type case selection consisted of only GraphQL reserved fields, such as `__typename` the selection was not being recognized and a composite inline fragment was generated. This resulted in empty `__selections` and `__mergedSources` lists. This in turn caused the `_asInlineFragment` accessor to behave incorrectly because it checks whether the definitions contained within `__mergedSources` are a match for the returned type. The empty list always matches and `_asInlineFragment` incorrectly matches the returned type. + +We should not be generating the inline fragment with `CompositeInlineFragment` conformance, which will then not generate the empty `__mergedSources` list. Codegen will also no longer generate empty `__selections` lists and this integration test was created to ensure that the generated model still conforms to `SelectionSet` through the extensions default implementation of `__selections`. + +## Reference Issue: https://github.com/apollographql/apollo-ios/issues/3326 + +## Solution + +The `TypeInfo` for a selection now has a property indicating whether it was defined by the user or formed through a combination of external selections, i.e.: composite. This allows the root field builder to definitively specify under which conditions a composite inline fragment should be generated. diff --git a/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/operation.graphql b/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/operation.graphql new file mode 100644 index 000000000..f03e52da3 --- /dev/null +++ b/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/operation.graphql @@ -0,0 +1,7 @@ + query TestOperation { + allAnimals { + ... on AnimalObject { + __typename + } + } +} diff --git a/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/schema.graphqls b/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/schema.graphqls new file mode 100644 index 000000000..bea79bca4 --- /dev/null +++ b/Tests/CodegenIntegrationTests/Tests/3326-typeCaseWithOnlyReservedFieldSelectionIsNotCompositeInlineFragment/schema.graphqls @@ -0,0 +1,13 @@ +type Query { + allAnimals: [Animal] +} + +union Animal = AnimalObject | AnimalError + +type AnimalObject { + species: String! +} + +type AnimalError { + code: Int! +} diff --git a/apollo-ios-codegen/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift b/apollo-ios-codegen/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift index 71e841f60..d8d0d0b44 100644 --- a/apollo-ios-codegen/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift +++ b/apollo-ios-codegen/Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift @@ -121,7 +121,7 @@ struct SelectionSetTemplate { \(SelectionSetNameDocumentation(inlineFragment)) \(renderAccessControl())\ struct \(inlineFragment.renderedTypeName): \(SelectionSetType(asInlineFragment: true))\ - \(if: inlineFragment.isCompositeSelectionSet, ", \(config.ApolloAPITargetName).CompositeInlineFragment")\ + \(if: inlineFragment.isCompositeInlineFragment, ", \(config.ApolloAPITargetName).CompositeInlineFragment")\ \(if: inlineFragment.isDeferred, ", \(config.ApolloAPITargetName).Deferrable")\ { \(BodyTemplate(context)) @@ -274,18 +274,25 @@ struct SelectionSetTemplate { var deprecatedArguments: [DeprecatedArgument]? = config.options.warningsOnDeprecatedUsage == .include ? [] : nil - let selectionsTemplate = TemplateString( - """ - \(renderAccessControl())\ - static var __selections: [\(config.ApolloAPITargetName).Selection] { [ - \(if: shouldIncludeTypenameSelection(for: scope), ".field(\"__typename\", String.self),") - \(renderedSelections(groupedSelections.unconditionalSelections, &deprecatedArguments), terminator: ",") - \(groupedSelections.inclusionConditionGroups.map { - renderedConditionalSelectionGroup($0, $1, in: scope, &deprecatedArguments) - }, terminator: ",") - ] } - """ - ) + let shouldIncludeTypenameSelection = shouldIncludeTypenameSelection(for: scope) + let selectionsTemplate: TemplateString + + if !groupedSelections.isEmpty || shouldIncludeTypenameSelection { + selectionsTemplate = TemplateString(""" + \(renderAccessControl())\ + static var __selections: [\(config.ApolloAPITargetName).Selection] { [ + \(if: shouldIncludeTypenameSelection, ".field(\"__typename\", String.self),") + \(renderedSelections(groupedSelections.unconditionalSelections, &deprecatedArguments), terminator: ",") + \(groupedSelections.inclusionConditionGroups.map { + renderedConditionalSelectionGroup($0, $1, in: scope, &deprecatedArguments) + }, terminator: ",") + ] } + """ + ) + } else { + selectionsTemplate = "" + } + return """ \(if: deprecatedArguments != nil && !deprecatedArguments.unsafelyUnwrapped.isEmpty, """ \(deprecatedArguments.unsafelyUnwrapped.map { """ @@ -752,12 +759,8 @@ private class SelectionSetNameCache { extension IR.ComputedSelectionSet { - fileprivate var isCompositeSelectionSet: Bool { - return direct?.isEmpty ?? true - } - fileprivate var isCompositeInlineFragment: Bool { - return !self.isEntityRoot && isCompositeSelectionSet + return !self.isEntityRoot && !self.isUserDefined && (direct?.isEmpty ?? true) } fileprivate var shouldBeRendered: Bool { diff --git a/apollo-ios-codegen/Sources/IR/IR+ComputedSelectionSet.swift b/apollo-ios-codegen/Sources/IR/IR+ComputedSelectionSet.swift index 1af61f507..d885dfc0d 100644 --- a/apollo-ios-codegen/Sources/IR/IR+ComputedSelectionSet.swift +++ b/apollo-ios-codegen/Sources/IR/IR+ComputedSelectionSet.swift @@ -104,7 +104,8 @@ extension ComputedSelectionSet { private func createShallowlyMergedNestedEntityField(from field: EntityField) -> EntityField { let typeInfo = SelectionSet.TypeInfo( entity: entityStorage.entity(for: field.underlyingField, on: typeInfo.entity), - scopePath: self.typeInfo.scopePath.appending(field.selectionSet.typeInfo.scope) + scopePath: self.typeInfo.scopePath.appending(field.selectionSet.typeInfo.scope), + isUserDefined: false ) let newSelectionSet = SelectionSet( @@ -149,7 +150,8 @@ extension ComputedSelectionSet { let typeInfo = SelectionSet.TypeInfo( entity: self.typeInfo.entity, - scopePath: self.typeInfo.scopePath.mutatingLast { $0.appending(condition) } + scopePath: self.typeInfo.scopePath.mutatingLast { $0.appending(condition) }, + isUserDefined: false ) let selectionSet = SelectionSet( diff --git a/apollo-ios-codegen/Sources/IR/IR+DirectSelections.swift b/apollo-ios-codegen/Sources/IR/IR+DirectSelections.swift index 6bdfbc561..7e408141e 100644 --- a/apollo-ios-codegen/Sources/IR/IR+DirectSelections.swift +++ b/apollo-ios-codegen/Sources/IR/IR+DirectSelections.swift @@ -85,7 +85,8 @@ public class DirectSelections: Equatable, CustomDebugStringConvertible { let typeInfo = SelectionSet.TypeInfo( entity: existingField.entity, - scopePath: wrapperScope + scopePath: wrapperScope, + isUserDefined: true ) let selectionSet = SelectionSet( @@ -111,7 +112,8 @@ public class DirectSelections: Equatable, CustomDebugStringConvertible { entity: newField.entity, scopePath: wrapperField.selectionSet.scopePath.mutatingLast { $0.appending(newFieldConditions) - } + }, + isUserDefined: true ) let newFieldSelectionSet = SelectionSet( @@ -209,6 +211,10 @@ public class DirectSelections: Equatable, CustomDebugStringConvertible { public private(set) var inclusionConditionGroups: OrderedDictionary, DirectSelections.ReadOnly> = [:] + public var isEmpty: Bool { + unconditionalSelections.isEmpty && inclusionConditionGroups.isEmpty + } + init(_ directSelections: DirectSelections.ReadOnly) { for selection in directSelections.fields { if let condition = selection.value.inclusionConditions { diff --git a/apollo-ios-codegen/Sources/IR/IR+RootFieldBuilder.swift b/apollo-ios-codegen/Sources/IR/IR+RootFieldBuilder.swift index a8c843eba..967c46b4c 100644 --- a/apollo-ios-codegen/Sources/IR/IR+RootFieldBuilder.swift +++ b/apollo-ios-codegen/Sources/IR/IR+RootFieldBuilder.swift @@ -74,7 +74,8 @@ class RootFieldBuilder { ) async -> SelectionSet { let typeInfo = SelectionSet.TypeInfo( entity: entity, - scopePath: scopePath + scopePath: scopePath, + isUserDefined: true ) var directSelections: DirectSelections? = nil @@ -420,7 +421,8 @@ class RootFieldBuilder { let typeInfo = SelectionSet.TypeInfo( entity: parentTypeInfo.entity, - scopePath: scopePath + scopePath: scopePath, + isUserDefined: true ) let fragmentSpread = NamedFragmentSpread( diff --git a/apollo-ios-codegen/Sources/IR/IR+SelectionSet.swift b/apollo-ios-codegen/Sources/IR/IR+SelectionSet.swift index be467cd44..673d55bd7 100644 --- a/apollo-ios-codegen/Sources/IR/IR+SelectionSet.swift +++ b/apollo-ios-codegen/Sources/IR/IR+SelectionSet.swift @@ -15,6 +15,13 @@ public class SelectionSet: Hashable, CustomDebugStringConvertible { /// The selection set's `scope` is the last element in the list. public let scopePath: LinkedList + /// Indicates if the `SelectionSet` was created directly due to a selection set in the user defined `.graphql` definition file. + /// + /// If `false`, the selection set was artificially created by the IR. Currently, the only reason for this is a `CompositeInlineFragment` created during calculation of merged selections for field merging. + public var isUserDefined: Bool + + // MARK: - Computed Properties + /// Describes all of the types and inclusion conditions the selection set matches. /// Derived from all the selection set's parents. public var scope: ScopeDescriptor { scopePath.last.value } @@ -28,6 +35,7 @@ public class SelectionSet: Hashable, CustomDebugStringConvertible { public var deferCondition: CompilationResult.DeferCondition? { scope.scopePath.last.value.deferCondition } + public var isDeferred: Bool { deferCondition != nil } /// Indicates if the `SelectionSet` represents a root selection set. @@ -38,10 +46,12 @@ public class SelectionSet: Hashable, CustomDebugStringConvertible { init( entity: Entity, - scopePath: LinkedList + scopePath: LinkedList, + isUserDefined: Bool ) { self.entity = entity self.scopePath = scopePath + self.isUserDefined = isUserDefined } public static func == (lhs: TypeInfo, rhs: TypeInfo) -> Bool {