Fix overlapping fields validator edge case

[xuorig]

Test shows a case where the validator should pass, but currently fails.

This is because we flatten inline fragments, and fragment names at the the selection level, but lose the context that they are mutually exclusive because of the inline fragment condition. Trying to come up with a fix at the moment.

The fix is to carry the current parent type for fragment spreads, and use this to compute the isMutuallyExclusive condition instead of defaulting to false.

The main changes is that we now need to carry more than the fragmentNames, so fragmentNames becomes fragmentSpreads, a tuple of (FragmentName, ParentType).

[IvanGoncharov]

tuple?

[xuorig]

Not totally done yet: sub selections also need to check for fragment spreads inside different typed inline fragments. Still WIP

[mjmahone]

This example currently fails on the Facebook GraphQL server (which does not depend on graphql-js for validation).

I would need to think about this example and whether it can be valid. I'm not sure whether it's a bug in both graphql-js and the Facebook server, or if it's intended behavior.

[mjmahone]

@xuorig thanks for bringing this up! It's a really interesting bug, and I think you're right that this behavior is not intended. On the other hand, I'm not sure whether GraphQL clients and servers would be able to handle the fix.

@jjergus may have an opinion on this.

[jjergus]

This is a very interesting problem! There is a difficult tradeoff between complexity and correctness here.

It is actually really difficult to decide whether two fields are "mutually exclusive" in the general case. The current implementation (which is also codified in the spec: http://facebook.github.io/graphql/June2018/#sec-Field-Selection-Merging) handles a few simple, well defined cases, but intentionally punts on anything more complicated.

Your proposed implementation handles a few more cases, but still, as far as I can tell, doesn't completely solve the general case. Here are some examples to demonstrate:

Example 1 (from the spec). These fields are correctly recognized as mutually exclusive by both implementations.

fragment safeDifferingFields on Pet {
  ... on Dog {
    volume: barkVolume
  }
  ... on Cat {
    volume: meowVolume
  }
}

Example 2 (from your test case). These fields are correctly recognized by your implementation but not the current one.

fragment stillSafeDifferingFields on Pet {
  ... on Dog {
    ... on Pet {
      volume: barkVolume
    }
  }
  ... on Cat {
    ... on Pet {
      volume: meowVolume
    }
  }
}

Example 3 (the fun one). I don't think even your implementation would currently be able to tell that these fields are mutually exclusive (but please correct me if I'm wrong -- I don't have a graphql-js checkout ready so I didn't actually check).

fragment actuallyStillSafeDifferingFields on Pet {
  ... on Dog {
    ... on Pet {
      volume: barkVolume
    }
  }
  ... on Cat {
    volume: meowVolume
  }
}

You can probably imagine all kinds of more complicated combinations of nested fragments.

So I think we have a few options here:

1. Stick with the current solution, which is simple, easy to describe and handles some common cases.
2. Implement a correct fully general solution. This would make the code (and spec!) more complex and potentially regress performance, so I don't think it's the right tradeoff, even though it's technically the only correct solution.
3. Your proposal adds support for some, but not all additional cases. This seems somewhat arbitrary so I would probably lean towards either 1 or 2 instead -- but if you can make a compelling argument why this looks like the right tradeoff, or suggest some other place on the scale between 1 and 2, I don't really have a problem with it.

[xuorig]

Thanks to both of you for looking into it and the context behind this validator. Let me take a look at some of these examples and come back with more info. The reason I'm here is because this case is currently handled in graphql-ruby, but the other unhandled cases and performance concerns are important to me too, so good point! 👍

[jjergus]

Here are some notes in case you're considering a more general solution:

The current (intentional) limitations are codified by this line in the algorithm description from the spec:

b. If the parent types of fieldA and fieldB are equal or if either is not an Object Type

This assumes that each field has a single "parent type", which is assumed to be the type of its immediate parent fragment/field (so in ... on Dog { ... on Pet { field } } it's Pet).

Solving the general case requires changing this so that we either:

1. Track the full list of parent types for each field, and write a clever algorithm for areMutuallyExclusive(listOfParents1, listOfParents2) -- in the example from my previous comment areMutuallyExclusive(['Dog', 'Pet'], ['Cat']) should be true.
2. Implement a more clever heuristic for a field's parent type than the current "take the immediate parent" approach. For example, it could realize that Dog is a subtype of Pet and treat ... on Dog { ... on Pet { field } } as the field having a parent type "Dog" even though the immediate parent is "Pet". This may look easy but what about things like ... on Mammal { ... on Pet { field } }?

Each of these can have complicated interactions with the memoization used in the code (which is critical for keeping the time complexity of GraphQL validation linear in the size of the query). Currently the memoization relies on the fact that each field AST node has a single unchanging parent type, which would no longer be true because now what we consider "parent type" for the same field changes throughout the validation.

[andimarek]

I wrote about static analysis of queries (https://www.graphql.de/blog/static-query-analysis/) which allows for the detection of these kind of advances type problems. It is implemented here: https://github.com/andimarek/graphql-analyzer. If this kind of analysis should happen as part of the validation is another question and comes with tradeoff mentioned by @jjergus.