Do not send field to the service if it cannot process it

[konalegi]

Problem descrion

When the resolved field type is shared interface and this interface is implemented in different services by types that aren't shared query planner would create queries that cause No such type ... errors.

Example

Service A:

type Query {
  myField: MyInterface
}

type MyInterface {
  name: String
}

type MyTypeA implements MyInterface {
  name: String
}

type MyTypeB implements MyInterface {
  name: String
}

Service B:

interface MyInterface {
 name: String
}

type MyTypeC implements MyInterface {
  name: String
}

The query:

query {
  myField {
    ...MyIntFragment
  }
}

fragment MyIntFragment on MyInterface {
  name
}

result in such query to service A:

query {
  myField {
    ...on MyTypeA { name }
    ...on MyTypeB { name }
    ...on MyTypeC { name }
  }
}

which cause an error

Unknown type "MyTypeC".

[apollo-cla]

@konalegi: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[dchambers]

@konalegi thanks for providing this PR. Any chance you can sign the Apollo CLA for it?

[konalegi]

@dchambers done

[pcmanus]

Thanks for the patch.

As pointed in a comment, the patch creates a problem with value types, but this can be fixed easily enough. It's unfortunate that no test catch that, so I think we'd need to add at least 2 tests: one for the fix here, and one that show the value-type case is not broken. But with the fix and tests, I think this could be committed.

[pcmanus]

Curious as to why you added this exclusion? I don't mind keeping it as it limits the cases changed by this fix and is thus somewhat more conservative, but given the goal is to exclude fields from services that don't the field at all, I'm not sure to see why the field return type would influence the behaviour.

Mostly wondering if you were thinking of a case I don't have in mind.

[konalegi]

Yes, that was the intention, to make changes as little as possible, because I'm not very familiar with the query planner.

[pcmanus]

We actually need to handle the case where owningService === undefined (and return true). Otherwise this will create an issue for value types as they don't have an owningService and would always be skipped, which we don't want.

An example of this would be if a subgraph has:

type Query {
  itf: Itf
}

interface Itf {
  f: String
}

type T1 implements Itf @key(fields: "f") {
  f: String
}

type T2 implements Itf {
  f: String
}

With the patch, if you query:

{
  itf {
    f
  }
}

then the query plan will include T1 but not T2 due to this problem.

[konalegi]

Thanks for your review @pcmanus, I've tried to address all your comments. Could you please take a look again? Thanks!

[pcmanus]

Slowly for the slow turn-around. The last changes look good to me, thanks!

[konalegi]

@pcmanus thanks for your help with the PR :) Do you know when it's going to be released?

[dchambers]

Great work getting a fix for this out there gents!

[pcmanus]

Do you know when it's going to be released?

I believe we're planning to do the next release relatively soon (tracked at #1369). I don't have an exact date but I'd say "within a few days".

[trevor-scheer]

I'll be handling the release for this likely by the end of the week. It's actually currently available via the gateway alpha (you'll also need to install the query-planner alpha to force its usage)
npm i @apollo/gateway@alpha @apollo/query-planner@alpha

Would someone please share a changelog entry here so I can add it before release? And is it fair to say this is just a patch (non-breaking bug fix)?

[pcmanus]

Would someone please share a changelog entry here

I already took the liberty to ninja-push one (acf3ffe) on the version-0.x branch. But yeah, it's a non-breaking bug fix.

[konalegi]

I'm not sure if the changelog is still required, but I'll provide it, "Not owning types are not added into a query on interface unpacking" not sure if it's very accurate though.

[trevor-scheer]

This fix is released in the latest versions of federation packages. Thanks again for providing the fix!