Apollo 1.0 Beta: Fragment name collisions with selection sets are easy

[jimisaacs]

Bug report

Reduced case that does not compile.

Notice a field of dog becomes incompatible with a fragment of Dog.

schema.graphqls

type Query {
  dog: Canine
}

type Canine {
  name: String
}

DogQuery.graphql

query DogQuery {
  dog {
    ...Dog
  }
}

fragment Dog on Canine {
   name
}

Fragments/Dog.swift

// @generated
// This file was automatically generated and should not be edited.

import ApolloAPI
@_exported import enum ApolloAPI.GraphQLEnum
@_exported import enum ApolloAPI.GraphQLNullable

public struct Dog: MyApolloCodeGen.SelectionSet, Fragment {
  public static var fragmentDefinition: StaticString { """
    fragment Dog on Canine {
      __typename
      name
    }
    """ }

  public let __data: DataDict
  public init(data: DataDict) { __data = data }

  public static var __parentType: ParentType { .Object(MyApolloCodeGen.Canine.self) }
  public static var selections: [Selection] { [
    .field("name", String?.self),
  ] }

  public var name: String? { __data["name"] }
}

Operations/Queries/DogQuery.swift

// @generated
// This file was automatically generated and should not be edited.

import ApolloAPI
@_exported import enum ApolloAPI.GraphQLEnum
@_exported import enum ApolloAPI.GraphQLNullable

public class DogQuery: GraphQLQuery {
  public static let operationName: String = "DogQuery"
  public static let document: DocumentType = .notPersisted(
    definition: .init(
      """
      query DogQuery {
        dog {
          __typename
          ...Dog
        }
      }
      """,
      fragments: [Dog.self]
    ))

  public init() {}

  public struct Data: MyApolloCodeGen.SelectionSet {
    public let __data: DataDict
    public init(data: DataDict) { __data = data }

    public static var __parentType: ParentType { .Object(MyApolloCodeGen.Query.self) }
    public static var selections: [Selection] { [
      .field("dog", Dog?.self),
    ] }

    public var dog: Dog? { __data["dog"] }

    /// Dog
    ///
    /// Parent Type: `Canine`
    public struct Dog: MyApolloCodeGen.SelectionSet {
      public let __data: DataDict
      public init(data: DataDict) { __data = data }

      public static var __parentType: ParentType { .Object(MyApolloCodeGen.Canine.self) }
      public static var selections: [Selection] { [
        .fragment(Dog.self),
      ] }

      public var name: String? { __data["name"] }

      public struct Fragments: FragmentContainer {
        public let __data: DataDict
        public init(data: DataDict) { __data = data }

        public var dog: Dog { _toFragment() }
      }
    }
  }
}

Error

Suggestion

Use namespace somehow, can we assume schemaName is a package name?
Sorry, I don't have much else at this time.

[jimisaacs]

You know I see schemaName used as the namespace elsewhere, so yeah, maybe it just needs to be used on the fragment types as well.

[AnthonyMDev]

Thanks for the report!

You know I see schemaName used as the namespace elsewhere, so yeah, maybe it just needs to be used on the fragment types as well.

I think this should work. We tried to reduce the amount of namespacing as much as possible to keep the generated code concise, but I think it has to be here. The only other way to handle that would be if we kept track of the names of Fragments and fields and when they conflict we then and only then namespace them. I think that's way more work that it's worth though. It hurts the performance and memory impact of the codegen execution for very little benefit (and it's a bunch of extra complexity we have to add to the already complex codegen system). I'll add the namespacing to these.

[AnthonyMDev]

As I've been working on #2424, I'm realizing we need to prefix with schema name for other types as well.
When implementing this (hopefully tomorrow!), we need to prefix:

• Named Fragments
• Enum types
• Input Object Types
• Custom Scalars

[AnthonyMDev]

Realizing as I test this, that it only works when the Fragments are in the schema module. If the fragments are placed in your other targets, I don't really know how to resolve this naming conflict...

I'm thinking about how to handle this, but I'm wondering if the only real solution is to disallow fragments to have the name of an object? You could still name the fragment Dog in your .graphql file and reference it that way in your operation graphql, but the generated fragment may need to be named DogFragment.

@jimisaacs How much would that bother you?

[jimisaacs]

I don't think it would bother me 🤔. I would just suggest if that it already ends with Fragment do not append it again.

[AnthonyMDev]

Okay, I've come up with a few options for how to resolve this. I'm looking for feedback on how we should handle it before I can implement it, so I don't think this is going to make it into Beta 2 today. I'll slate it for a Beta 3 next week.

I'd love feedback from anyone interested, but specifically tagging people I know may have opinions:
@calvincestari @jimisaacs @hwillson @martinbonnin @BoD @kimdv

This is also related to #2433, which deals with Fragments file names colliding with the file names of object types.

Please let me know which of these options you prefer:

Input:

Schema.graphqls

type Query {
  dog: Dog!
}

type Dog {
  species: String!
}

Dog.graphql

fragment Dog on Dog {
  species
}

DogQuery.graphql

query DogQuery {
  dog {
    ...Dog
  }
}

Option 1a - Put All Fragments in the Schema Module

It's important to recognize that this is only a problem when your operations & fragments are generated and included in your other modules. If you put the operations & fragments in the schema module (which I believe is what @jimisaacs is doing in the initial examples he created this issue from), then the solution is as easy as the codegen adding the schemaName to the qualified name of the fragment reference as @jimisaacs and I discussed above.

If all fragments are put in the schema module, then this works just fine, but I don't think that's a great solution. It limits some significant functionality for multi-module support.

I'm not a huge fan of this solution. I could be wrong here, but I actually expect that most people will want to have their operations/fragments co-located with feature code that consumes those operations/fragments. This allows you to just have a shared module with your schema types, and compartmentalize the operations/fragments where they are used.

Option 1b - Use Namespace only if Fragments In Schema Module

Of the fragments are in the schema module, we can use the namespacing technique. If they aren't then we have to use one of the below options.

This seems like a really inconsistent solution, and it still requires us to solve the problem using one of the below solutions as well for many cases.

---

Option 2a - Automatically Suffix Generated Fragment Types with Fragment

Allows you to use the name Dog in your .graphql files, but the generated types will have a suffix on their names.
If your fragment already ends in the word "Fragment", then we do not append it again. This is what we currently do with operations (ie. Query, Mutation, Subscription).

Generated Output:

DogFragment.swift

public struct DogFragment: MySchemaModule.SelectionSet, Fragment {
  public static var fragmentDefinition: StaticString { """
    fragment Dog on Dog {
      __typename
      species
    }
    """ }

  // ...
}

DogQuery.swift

class DogQuery: GraphQLQuery {
  public static let operationName: String = "DogQuery"
  public static let document: DocumentType = .notPersisted(
    definition: .init(
      """
      query DogQuery {
        dog {
          __typename
          ...Dog
        }
      }
      """,
      fragments: [DogFragment.self] // <--
    ))

  public init() {}

  public struct Data: MySchemaModule.SelectionSet {
    // ...
    public var dog: Dog { __data["dog"] }

    public struct Dog: MySchemaModule.SelectionSet {
      // ...
      public static var __parentType: ParentType { MySchemaModule.Objects.Dog }
      public static var selections: [Selection] { [
        .fragment(DogFragment.self), // <--
      ] }

      public var species: String { __data["species"] }

      public struct Fragments: FragmentContainer {
        // ...
        public var dog: DogFragment { _toFragment() } // <--
      }
    }
  }
}

Option 2b - Only add Suffix on Fragments matching names of conflicting types

The same as Option 2a, but we would not add the Fragment suffix to the names of all fragments, only those to those whose names conflict with another type. This requires a bit more work and for us to get a little clever. It is easier to do this for just types in the schema (which resolves #2433), but harder to look at all the the SelectionSets generated on all operations to determine if they have a conflicting name.

This also has a major downside that I think precludes us from doing it. When new types are added to the schema and codegen is re-ran, if a type matching the name of an existing fragment is added, the type name of an existing generated fragment may change. This may cause breaking changes to your client code that have to be fixed.

One of our fundamental principals while designing the codegen engine was that we do not want a re-run of codegen to cause breaking changes to the API of your generated objects unless you actually changed the operations yourself. Unless someone can come up with a way to mitigate that issue, I think this is off the table. We would need to go with Option 2a and always add the "Fragment" suffix

Option 2c - Suffix Generated Types and File Names & Allow Addition of Generated typealias on Fragments

The actual type being named DogFragment resolves all the compilation problems. If you want to be able to reference the fragment by another name, we could add a custom directive that generates a typealias to it for you.

The generated operations would still reference it as DogFragment, so when you are consuming them, code completion and quick help is going to show the type as DogFragment, but if you want to call it Dog in your external code, you can.

Input

Dog.graphql

fragment Dog on Dog @apollo_client_ios_typealias(name: "Dog") {
  species
}
#### Generated Output
### DogFragment.swift
```swift
public typealias Dog = DogFragment
public struct DogFragment: MySchemaModule.SelectionSet, Fragment {
  public static var fragmentDefinition: StaticString { """
    fragment Dog on Dog {
      __typename
      species
    }
    """ }

  // ...
}

If your custom typealias creates any other naming conflict, I feel like at that point, it's on you, we aren't going to be able to do much more to cope with that for you. You'll need to change your typealias name.

Option 3 - Do not allow Fragments with the same name as Types

When a problem would occur, we throw a validation error when you attempt to run codegen execution, requiring you to change the name of your fragment. This would happen when:

• You create a fragment which has the same name as a type in the schema (for Name collisions when Operations in Schema Module & Fragment matching type name #2433)
• You include a fragment inside a selection set for a field (or that would be merged into a field) with the same name as the fragment (eg. query { dog { ...Dog } })

This isn't ideal, as it prevents you from using the naming you wanted to use at all. It also has the added downside of possibly making your codegen invalid when re-running codegen after new types are added to your schema. In which case, rather than codegen running, then your code having API breaking changes, codegen fails.

You would still have to then change the names of your fragments and re-run codegen, which will then create those API breaking changes, but at least it will be something you expect rather than happening without your knowledge. Not sure if that's better?

This option doesn't sound great to me.

---

This leads me to options that actually change the structure of the generated code to handle this.

Option 4a - Add Type Aliases for Fragments in operations.

Inside of your operation, we would generate a typealias DogFragment = Dog. And we could keep the Fragment named Dog, so other outside code could reference by that name (though we may still need to change the name of the file to DogFragment.swift to resolve #2433).

This would only fail for a fragment named Data, which would need to be disallowed as a fragment name.

The relevant code changes to the operation look like this:

public class DogQuery: GraphQLQuery {
  public static let operationName: String = "DogQuery"
  public static let document: DocumentType = .notPersisted(
    definition: .init(
      "...",
      fragments: [DogFragment.self]
    ))

  public typealias DogFragment = Dog // <-- This references the `Dog` fragment struct, because the below `SelectionSet` is named `Data.Dog`
  public typealias DataFragment = Data // <-- This does not work, as it references the `Data` `SelectionSet` below.

  // ...

  public struct Data: MySchemaModule.SelectionSet {
    // ...
    public var dog: Dog { __data["dog"] }

    public struct Dog: MySchemaModule.SelectionSet {
      // ...
      public static var __parentType: ParentType { MySchemaModule.Objects.Dog }
      public static var selections: [Selection] { [
        .fragment(DogFragment.self), <--
      ] }

      public struct Fragments: FragmentContainer {
        // ...
        public var dog: DogFragment { _toFragment() } // <--
      }
    }
  }
}

Option 4b - Only Add Type Aliases for Conflicting Names

Similar to Option 2b, we would only need to add these for types that we detect are going to have a naming conflict.
This again requires more work for us to actually detect them, but it's doable.

The issue with the API breaking changes is much less relevant. It still exists if you are referencing the typealias in your client code and then it later goes away, but that seems unlikely. Both because it's rarer that types are removed from a schema (and if they are, you're going to have breaking changes anyways), and because it's not really useful to reference the DogQuery.DogFragment outside of the generated code (you would just reference the generated Dog fragment). This seems so marginal as to be a moot point to me. So I'm willing to implement this option if people like it more than Option 4a.

Option 5a - Add a FragmentsUsed Struct to Contain the Type Aliases

Similar to Option 4a, but with a slightly different organizational structure, putting the typealias inside a struct namespace.
More verbose, more generated code, but maybe slightly better organizationally? I don't really see this option being much better myself, but it's an option.

I wanted to name this struct Fragments, but that conflicts with the SelectionSet's own Fragments struct.
I also wanted to name the typealias inside the FragmentsUsed to typealias Dog = Dog, but that doesn't compile either obviously.

public class DogQuery: GraphQLQuery {
  public static let operationName: String = "DogQuery"
  public static let document: DocumentType = .notPersisted(
    definition: .init(
      "...",
      fragments: [FragmentsUsed.DogFragment.self]
    ))

  struct FragmentsUsed {
    public typealias DogFragment = Dog // <-- This references the `Dog` fragment struct, because the below `SelectionSet` is named `Data.Dog`
  }
  // ...

  public struct Data: MySchemaModule.SelectionSet {
    // ...
    public var dog: Dog { __data["dog"] }

    public struct Dog: MySchemaModule.SelectionSet {
      // ...
      public static var __parentType: ParentType { MySchemaModule.Objects.Dog }
      public static var selections: [Selection] { [
        .fragment(FragmentsUsed.DogFragment.self), <--
      ] }

      public struct Fragments: FragmentContainer {
        // ...
        public var dog: FragmentsUsed.DogFragment { _toFragment() } // <--
      }
    }
  }
}

Option 5b - Only add a FragmentsUsed Struct when Conflicting Types are Detected

This is like Option 2b and 4b, calculate the conflicting types and only generate the FragmentsUsed when necessary.

---

Summary

I, personally am leaning towards Option 2a, which is that we just add "Fragment" to the end of the names of all Fragment Types and File Names. It's certainly the easiest to implement, it will always work regardless of your codegen configuration, and it doesn't cause other complications.

The downside is that is changes the names of your objects. It sounds to me like being able to have your fragment names match the names of your types directly is a strongly desired feature by @jimisaacs. Am I correct in thinking that? Do other people have a strong opinion on how important that functionality is?

I'm very open to adding in the typealias directive as proposed in Option 2c if that's strongly desired. It doesn't feel like an important feature to me, but that seems like a preference thing, and I don't know if others have a strong preference for that.

[jimisaacs]

I'm also good with Option 2a, but just not suffixing if it already ends with Fragment. (no DogFragmentFragment).

[hwillson]

@AnthonyMDev thanks for such a thorough breakdown and exploration of the potential options! I agree with your preference and @jimisaacs - option 2a seems like a great solution and is totally reasonable, especially given that we're doing something similar with operations already. Thanks!

[calvincestari]

Thanks for the detailed breakdown @AnthonyMDev!

• I'm not a fan of any of the "only when conflicting types are detected" solutions, it's inconsistent, I favour predictability.
• Option 2a seems the most acceptable and I do like that it already matches what we do with Query, Mutation, and Subscription.

The one true answer for this is "don't name things the same" and I feel like we're simply pushing that answer further along to a place where we're more comfortable with it being the final answer to the problem. Take the outrageously ridiculous example of both the type and fragment being named DogFragment. Option 2a no longer resolves the problem and the final answer to that user is "don't name things the same".

[BoD]

• I like 2a too
• We don't have this specific problem on Apollo Kotlin, but in other places where conflicts can appear, we suffix with an incrementing number (here).

[AnthonyMDev]

After further consideration, we've decided to make option 2a an opt-in solution. You can choose to use a field alias on a single conflicting name. If you want to handle this across your entire schema with a name suffix because this is a common pattern for you, then you can enable the option to do this. But by making it the default, you remove a lot of control over the expressibility of your view models and generated types, so its not a great default solution.

After talking to @jimisaacs, it looks like he has a workaround for now, so I'm going to push this off until Beta 3.

[calvincestari]

After talking with the team about #3024 and recognizing the similarity in this issue we've decided to implement an error during code generation that will alert the user to this kind of naming conflict and recommend a type alias be used to resolve it.