Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Type case with only reserved fields is not composite inline fragment #261

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ extension IR.NamedFragment {
inclusionConditions: nil,
givenAllTypesInSchema: .init([type], schemaRootTypes: .mock())
)
]
],
isUserDefined: true
)
let rootEntityField = IR.EntityField.init(
rootField,
Expand Down Expand Up @@ -110,7 +111,8 @@ extension IR.Operation {
forType: .mock(),
inclusionConditions: nil,
givenAllTypesInSchema: .init([], schemaRootTypes: .mock()))
]
],
isUserDefined: true
)
let rootField = IR.EntityField(
.mock(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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] { [
] }
}

"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1447,6 +1447,57 @@
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 {
Expand Down Expand Up @@ -1716,11 +1767,11 @@
))
}

#warning("need more tests here - same test cases as IRRootFieldBuilderTests")

Check warning on line 1770 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests

// MARK: Selections - Deferred Named Fragment

#warning("need more tests here - same test cases as IRRootFieldBuilderTests")

Check warning on line 1774 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests

// MARK: Selections - Include/Skip

Expand Down Expand Up @@ -4664,11 +4715,11 @@
))
}

#warning("need more tests here - same test cases as IRRootFieldBuilderTests")

Check warning on line 4718 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests

// MARK: Field Accessors - Deferred Named Fragments

#warning("need more tests here - same test cases as IRRootFieldBuilderTests")

Check warning on line 4722 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests

// MARK: - Inline Fragment Accessors

Expand Down Expand Up @@ -5522,11 +5573,11 @@
))
}

#warning("need more tests here - same test cases as IRRootFieldBuilderTests")

Check warning on line 5576 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests

// MARK: Fragment Accessors - Deferred Named Fragment

#warning("need more tests here - same test cases as IRRootFieldBuilderTests")

Check warning on line 5580 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests

// MARK: - Nested Selection Sets

Expand Down Expand Up @@ -6952,6 +7003,54 @@
.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 {
Expand Down Expand Up @@ -7532,7 +7631,7 @@
))
}

#warning("need more tests here - same test cases as IRRootFieldBuilderTests (inline fragment only")

Check warning on line 7634 in Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

View workflow job for this annotation

GitHub Actions / Codegen Lib Unit Tests - macOS

need more tests here - same test cases as IRRootFieldBuilderTests (inline fragment only

// MARK: - Documentation Tests

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
query TestOperation {
allAnimals {
... on AnimalObject {
__typename
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
type Query {
allAnimals: [Animal]
}

union Animal = AnimalObject | AnimalError

type AnimalObject {
species: String!
}

type AnimalError {
code: Int!
}
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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 { """
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions apollo-ios-codegen/Sources/IR/IR+ComputedSelectionSet.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
10 changes: 8 additions & 2 deletions apollo-ios-codegen/Sources/IR/IR+DirectSelections.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -209,6 +211,10 @@ public class DirectSelections: Equatable, CustomDebugStringConvertible {
public private(set) var inclusionConditionGroups:
OrderedDictionary<AnyOf<InclusionConditions>, 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 {
Expand Down
6 changes: 4 additions & 2 deletions apollo-ios-codegen/Sources/IR/IR+RootFieldBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class RootFieldBuilder {
) async -> SelectionSet {
let typeInfo = SelectionSet.TypeInfo(
entity: entity,
scopePath: scopePath
scopePath: scopePath,
isUserDefined: true
)

var directSelections: DirectSelections? = nil
Expand Down Expand Up @@ -420,7 +421,8 @@ class RootFieldBuilder {

let typeInfo = SelectionSet.TypeInfo(
entity: parentTypeInfo.entity,
scopePath: scopePath
scopePath: scopePath,
isUserDefined: true
)

let fragmentSpread = NamedFragmentSpread(
Expand Down
12 changes: 11 additions & 1 deletion apollo-ios-codegen/Sources/IR/IR+SelectionSet.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<ScopeDescriptor>

/// 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
calvincestari marked this conversation as resolved.
Show resolved Hide resolved

// 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 }
Expand All @@ -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.
Expand All @@ -38,10 +46,12 @@ public class SelectionSet: Hashable, CustomDebugStringConvertible {

init(
entity: Entity,
scopePath: LinkedList<ScopeDescriptor>
scopePath: LinkedList<ScopeDescriptor>,
isUserDefined: Bool
) {
self.entity = entity
self.scopePath = scopePath
self.isUserDefined = isUserDefined
}

public static func == (lhs: TypeInfo, rhs: TypeInfo) -> Bool {
Expand Down