[Validation] Nested inline fragments may be unreachable

[jorgeperezrojas]

I'm not sure if this is the place to put this issue. It is about an unexpected behavior of the inline Type fragments in the current GraphQL spec.

Consider the GraphQL schema defined in the example page http://graphql.org/swapi-graphql. The following query is invalid as "Person" can never be of type "Vehicle" so the object cannot be spread.

query {
  allPeople {
    people {
      ...on Vehicle {
        id
      }
    }
  }
}

Nevertheless, the following query is valid which seems to be counterintuitive

query {
  allPeople {
    people {
      ...on Node {
        ...on Vehicle {
            id  
        }
      }
    }
  }
}

In this case the validity came from the fact that "...on Node" can be spread as a "Person" is always a "Node", and moreover, "...on Vehicle" can be spread in the scope of a "Node". This is counter intuitive since the "...on Node" is in the scope of "Person" which makes the "...on Vehicle" fragment to always be in the scope of the "Person"so anyway it cannot be spread. Executing this last query at http://graphql.org/swapi-graphql gives

{
"data": {
"allPeople": {
"people": [
{},
{},
.... (several times)....
{}
]
} } }

I think that this design would lead to some misunderstanding from users. I have thought on two possible options to deal with this:

1. Forbid the use of expressions of the form "...on Type1 { ...on Type2 { ... } }" in queries (that is, one cannot nest "...on Type"s fragments without a field selection.

or

2. Ensure that in every immediate nesting of "...on Type"s fragments, the nested Type is either equal, or an implementation, or part of a union of the outer Type. That is, for something of the form "...on Type1 { ...on Type2 { ... } }" one should force Type2=Type1, or Type2 be an implementation of Type1, or Type2 be part of the union Type1.

[jorgeperezrojas]

Another option, say option (3) (sorry for not spotting this before), is to just make the following query valid

query {
allPeople {
people {
...on Vehicle {
id } } } }

there seems to be no problem as it would just give an empty result while evaluating and, as expected, would be equivalent to:

query {
allPeople {
people {
...on Node {
...on Vehicle {
id } } } } }

[jlouis]

The area in the specification which is invoked here is section http://facebook.github.io/graphql/October2016/#sec-Fragment-spread-is-possible (Section 5.4.2.3) and its following subsections.

The spec defines that you check a spread in its "immediate" context and not in the possible contextual "path" that the spread occurs in. The "immediate" check is easy to implement, but will exhibit the behavior you mention. It is mostly there to capture nonsensical queries which are valid enough to execute but are likely to be mistakes.

To implement a better solution, one must take the full contextual path into consideration. The path is something like [Person, Node, Vehicle] in your case. You would have to be able to explain that because there is a mismatch between any two pairs, it is an error. This can probably be handled by noting that this is a context [Person, Node] and and attempt to extend this with Vehicle. This holds for Node, the immediate context. But is doesn't hold for Person, which is the contextual path at one more level.

However, it remains to be shown that there are no situations in which this expansion is actually desirable by the developer. In that case, we cannot use it as a validation because the it might occur legally in some query documents and have the intended behavior. It is not a priori clear to me that such a situation can never occur, so one definitely has to be careful here.

[leebyron]

I just updated the title for this.

This is an area where additional validation could be applied and is just presently missing. @jlouis is correct that we would need to be careful not to inadvertently make a request invalid which is actually useful. I think that tightening the validation to only apply for immediately nested inline fragments would be useful, however the same could not be extended to typical fragments.