Fix plan querying a subgraph with an interface it doesn't know due to directives

[pcmanus]

The commit message has more details, but this was due to a faulty condition in the code committed by #652 that I mistakenly forgot to remove when doing some change during the development of that PR.

More precisely, early version of that PR was not always preserving the innermost condition of a scope (so technically Scope.refine was sometimes ignoring the newly added condition) and so the condition was avoid this when we had a directive to avoid incorrectly dropping said directive. But the whole behaviour was wrong and was changed to always preserve the innermost condition, which made that particular condition not only unneeded but actually harmful. Tl;dr, the patch simple remove the leftover condition.

[martijnwalraven]

The query plan in the original failing test was the result from running under 0.25.1. I agree this better reflects the intent, as there is no need to query for upc unless the include condition is met.

It may be worth double checking if it also does the right thing under different circumstances though. I'm thinking of:

• selecting rating both under the include condition and without it, where the order of the inline fragments may also matter (because the code for propagating directives is flawed).
• selecting a different extension field of Shoe without an include condition, to ensure upc is fetched unconditionally in that case.

[pcmanus]

Added a few tests and afaict, it seems to work properly (there is some ugliness in the "argument" part of the dependent fetch related to your other comment but I think that's harmless). I'm not 100% sure I captured what you had in mind for the 1st bullet though. I'm not sure if you mean rating to in nested fragments or not and why order would matter. Let me know if there is another case you had in mind that isn't captured.

[martijnwalraven]

@pcmanus What I was getting at with the ordering was an issue that I now realize may actually have been fixed by your scoping changes. Originally, we only took directives on the first field nested under a particular type condition into account. So here for example, both rating and reviewsCount would be considered to fall under @include(if: true):

... on Shoe @include(if: true) {
  rating
}

... on Shoe {
  rating
  reviewCount
}

And if you changed the order, the condition would be lost completely:

... on Shoe {
  rating
  reviewCount
}

... on Shoe @include(if: true) {
  rating
}

[pcmanus]

I see. Added a quick test for that exact case (which does pass).

[martijnwalraven]

I think strictly taken we'd also want the include condition here. That doesn't matter for this query, but if you select an extension field from one subgraph conditionally and from another unconditionally, you always fetch the key fields but only want to send a representation for the entity if the condition is met.

I just wanted to mention it because I thought of it, but it's definitely out of scope for this PR. Right now, the code that extracts the field values for a representation ignores directives anyway, so it wouldn't even make a difference if the query plan added them.

[pcmanus]

I did notice that we could have directives there too, though as that part is unrelated to the regression this PR is concerned with, I haven't looked further.

That said, I quickly concluded that this should never matter (at least for include/skip; for other directives, that may) so I'm not sure what case you're thinking of for "if you select an extension field from one subgraph conditionally and from another unconditionally" and I'd be curious if you had an example (and if we do have a case where things are broken because of this, even if we don't fix it right away, pushing at least a reproduction case might be nice).

[martijnwalraven]

What I was thinking of is a case where for example you ask for reviews conditionally on Book, and inStock unconditionally. That means you'd be requesting isbn unconditionally from the product subgraph, and you want to send both books and furniture to the inventory subgraph. But ideally you'd filter books out when collecting representations to send to the reviews subgraph (and avoid a subquery altogether when all top 5 products are books).

query {
  topProducts(first: 5) {
    price
    inStock
    ... on Book @skip(if: true) {
      reviews {
        body
      }
    }
    ... on Furniture @skip(if: true) {
      reviews {
        body
      }
    }
  }
}