Fix schema.

[voslartomas]

No description provided.

[mcollina]

@frikille would you have a few minutes to take a look?

[asciidisco]

@mcollina @voslartomas I tend to believe that this is only a sympton of the issue described in #184

[paolochiodi]

I agree with @asciidisco. The error this PR is fixing seems to be related to #184.

As I explained in #219 (comment) we get an error similar to the one described in #184.
I think there's slight difference here though: we get away with removing @key on just one of the extend User. We can either remove it from https://github.com/mcollina/fastify-gql/blob/master/examples/gateway-subscription.js#L111 or from https://github.com/mcollina/fastify-gql/blob/master/examples/gateway-subscription.js#L173 and in both cases the error goes away and the gateway boots.

So, it is true that this PR will fix the immediate issue with the gateway subscription and will make the server boot, but I think it is not the correct fix because as far as I can see the schema is a legitimate one and should work.

The underlying cause should be fixed instead.

[paolochiodi]

So, it is true that this PR will fix the immediate issue with the gateway subscription and will make the server boot, but I think it is not the correct fix because as far as I can see the schema is a legitimate one and should work.

The underlying cause should be fixed instead.

Actually, let me take that back and ask the question: is this a valid schema?

In this case it seems so because @key holds the same definition in both federated services. But what if the two services had different definitions? How could the gateway merge them?

And what's the best way to signal this to the user? Should the gateway silently ignore the conflicts?

[asciidisco]

@paolochiodi Which emphasizes again on what @flux627 #184 (comment) said & something that probably @frikille can answer best, as the original author of that code.

One could ask how apollo behaves in such cases, but then would also need to ask if apollo should be used as the reference implementation here, or not.

[paolochiodi]

@asciidisco yep, that's correct.

For the record, I found this in the apollo docs at https://www.apollographql.com/docs/apollo-server/federation/entities/#referencing

The @key directive indicates that Product uses the upc field as its primary key. This value must match the value of exactly one @key defined in the entity's originating service, even if the entity defines multiple primary keys.

Also worth noting that fastify-gql's gateway seems to be doing the right thing* when an entity is extended only once. The problem occurs when it's extended twice.

• the right thing: one can replace it with "the thing apollo also does"

[flux627]

One could ask how apollo behaves in such cases, but then would also need to ask if apollo should be used as the reference implementation here, or not.

Apollo is the authority here, and this is about more than implementation details. They have docs describing the system, but since they don't have an official spec, then we need to take their docs + implementation together as a de-facto spec. Otherwise, we're implementing something different than Federation. That's ok, but we shouldn't claim that this is Federation in that case. I personally think it's in this library's best interest to match the functionality of Apollo and not diverge into its own flavor of schema delegation (especially if it's marketed as Federation and is almost Federation).

FWIW, I was originally looking into this library for its subscription support in combination with Federation, but have since moved on to using schema stitching via the new graphql-tools stitchSchemas() type merging.

[paolochiodi]

I've done some more research and I may have found the route cause... but I'm moving the discussion to #184 as it seems more relevant there.

[voslartomas]

One could ask how apollo behaves in such cases, but then would also need to ask if apollo should be used as the reference implementation here, or not.

Apollo is the authority here, and this is about more than implementation details. They have docs describing the system, but since they don't have an official spec, then we need to take their docs + implementation together as a de-facto spec. Otherwise, we're implementing something different than Federation. That's ok, but we shouldn't claim that this is Federation in that case. I personally think it's in this library's best interest to match the functionality of Apollo and not diverge into its own flavor of schema delegation (especially if it's marketed as Federation and is almost Federation).

FWIW, I was originally looking into this library for its subscription support in combination with Federation, but have since moved on to using schema stitching via the new graphql-tools stitchSchemas() type merging.

How exactly are you doing this please? You have one server witch is stitching federated schema and subscriptions? Another server is Federation and then services?

[flux627]

I have a gateway server (not an Apollo Federation Gateway™) that merges the types from multiple other subschemas. No directives required, but you do need to implement queries on the subschemas to fetch types via some identifier/key. See the docs here.

As an exercise, I took this Apollo federation demo and converted it to use graphql-tools schema stitching, if you want to see a direct comparison.

For subscriptions, check out the code from this comment.

[voslartomas]

@flux627 I see, so it's the old approach before federation. Or am I missing something?

[flux627]

The stitchSchemas function, along with its type merging abilities, is brand new to graphql-tools v6. The old method of schema stitching (and the one that Apollo talks about as deprecated/obsolete) is much more cumbersome than what this provides.

[voslartomas]

@flux627 I see, will take a look, thanks.

[asciidisco]

@voslartomas As the conversation has been going into the direction of fixing the root cause for this, I think this could be closed.