Missing subType check for AbstractTypes

[NicolasRouquette]

Currently, sangria.schema.TypeComparators.isSubType(schema, subType, superType) returns false
if both subType and superType are AbstractTypes (e.g., Interface).

This prevents us from using a GraphQL schema like this; based on a variant of the Stars Wars schema.

interface Character {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
}

interface Humanoid implements Character {}

type Human implements Humanoid {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  homePlanet: String
}

interface Droid implements Character {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  primaryFunction: String
  follows: Character # Note that this field is typed by an interface.
}

interface HumanoDroid implements Droid {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  primaryFunction: String
  follows: Humanoid # Note that this field overrides Droid.follows and is typed by an interface subtype
}

interface MonoDroid implements Droid {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  primaryFunction: String
  follows: Droid # Note that this field overrides Droid.follows and is typed by an interface subtype
}

type PrimaryDroid implements HumanoDroid {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  primaryFunction: String
  follows: Humanoid
}

type SecondaryDroid implements MonoDroid {
  id: ID!
  name: String!
  friends: [Character]
  appearsIn: [Episode]!
  primaryFunction: String
  follows: Droid
}

To demonstrate this, I implemented this example in a variant of the Sangria playground:

https://github.com/NicolasRouquette/sangria-playground/tree/MissingSubtypeCheckForAbstractTypeExample

Compile & run then go to: localhost:9000/render-schema
This will trigger an exception caused by this:

Caused by: sangria.schema.SchemaValidationException: Schema does not pass validation. Violations:

Droid.follows expects type 'Character', but SecondaryDroid.follows provides type 'Droid'.

Droid.follows expects type 'Character', but PrimaryDroid.follows provides type 'Humanoid'.
	at sangria.schema.SchemaValidationRule$.validateWithException(SchemaValidationRule.scala:42)
	at sangria.schema.Schema.<init>(Schema.scala:901)
	at models.SchemaDefinition$.<init>(SchemaDefinition.scala:273)
	at models.SchemaDefinition$.<clinit>(SchemaDefinition.scala)
	... 30 common frames omitted

The reason is that sangria.schema.InterfaceImplementationValidationRule
verifies that a field in the implementation (subtype)
that is typed byT2 may override a field in the interface (supertype)
that is typed by T1 if T2 is a subtype of T1; however,
this subtyping check is not implemented for T1 and T2 being interfaces.

[OlegIlyenko]

Thanks a lot for describing the issue and putting effort in the PR! I appreciate it.

I looked at the playground example and the PR. To be honest, the scope of changes and the example itself is quite big. I found it a bit overwhelming and hard to identify the essence on the issue. In particular, among other changes, PR has some changes to the introspection and SDL (schema definition AST) which I found confusing.

I think it is important to mention that some of the specifics of abstract types are constrained by the GraphQL specification itself. For example, all union type members must be object types (not abstract types like unions or interfaces). Also, interface type cannot implement/extend other interfaces. These constraints must be upheld in the introspection and SDL as well as various validation rules. So, for instance, following SDL is invalid GraphQL (on the syntax level as well as semantically since it is not possible to represent this concept in GraphQL type system right now):

interface Humanoid implements Character {}

Sangria does provide some helpers that allow interfaces to implement other interfaces. But this is just a helper, all these relationships between interface types are erased and interfaces are linearized (combined in a flat list of interfaces that concrete object type implements).

Based in the example and PR, I'm not quite sure whether these constraints are affected (or there is an intention to change these). I would appreciate if you could create another, simpler example, that contains the bare minimum of setup and demonstrates the essence of the issue. Maybe this snippet might be helpful as a template (i usually use it to reproduce and demonstrate issues): https://gist.github.com/OlegIlyenko/4068ad92e008cd4b5def1baa4ec3a67c

[NicolasRouquette]

Thanks @OlegIlyenko for reviewing this PR.

Let's differentiate between the purpose of the issue (adding support for interface subtyping) vs. the implementation changes needed to do this.

The former turns out to be a moot issue as you correctly point out because the June 2018 GraphQL spec explicitly prevents implementing interfaces by other interfaces and combining interfaces into unions. I had heard so much praise about GraphQL's powerful type system that I thought it was a limitation of the current implementations instead of a limitation of the spec itself. Now, I'm not sure about GraphQL; it forces encoding a domain into a very shallow set of types.

Although the implementation changes needed to support interface subtyping are currently out of scope, they might become useful later if the GraphQL spec where to be improved to include support for interface subtyping. Regarding the volume of changes, well, I didn't expect the change to be significant. However, once I began down the rabbit hole of adding interface subtyping to the Schema DSL several unit tests failed because it turns out that I had to address parsing and introspection as well. In effect, these unit tests provided a lot of useful guidance about what needed to be addressed.

[NicolasRouquette]

This issue is out of scope of the June 2018 Graph QL Spec and therefore out of scope of the Sangria implementation.

[OlegIlyenko]

I forgot to mention that the feature itself has some support behind it. If you would like to contribute to a discussion and push the RFC forward, I would suggest you to check this issue: graphql/graphql-spec#295. It was discussed at several recent GraphQL Working Group meetings and there was a consensus that it is something that might be considered for inclusion in the spec. Though I think it's not quite there yet right now.