mercurius-js / mercurius

Implement GraphQL servers and gateways with Fastify
https://mercurius.dev/
MIT License
2.34k stars 234 forks source link

federation gateway should throw on type's conflicts #826

Open Eomm opened 2 years ago

Eomm commented 2 years ago

Given the following schemas:

Node 1

    extend type Query {
      getOrder: Order
    }

    type Order @key(fields: "id") {
      id: ID!
      name: String
    }

Node 2

  type Order @key(fields: "id") {
    id: ID!
    surname: String
  }

The gateway starts correctly instead of throwing.

This is a clear developer's error, as the Node 2's schema should use the extends keyword:

  extends type Order {
    surname: String
  }

In this case, Mercurius should throw an error when:

In both the above cases, Apollo server trigger this error:

This data graph is missing a valid configuration. Field "Order.id" can only be defined once.

There can be only one type named "Order".

This would help developers to spot these schema conflicts at first gateway run.

mcollina commented 2 years ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

JamesLMilner commented 2 years ago

I have been looking at this today and have some code that works for the case above. However I'm trying to get a better understanding of what the defined behaviour should be for when federated service schemas come together more broadly (for example if it is not an entity type as above). Is there any specification or guidance on how federated service schemas should behave when they come together? I have been looking at the Apollo v1 documentation - is this what we want to follow? It would be great to have some more test cases to help write this feature.

jonnydgreen commented 2 years ago

Is there any specification or guidance on how federated service schemas should behave when they come together?

Good question! This is actually being fleshed out as we speak in the GraphQL Composite Schemas Working Group: https://github.com/graphql/composite-schemas-wg . Unfortunately, there is not much behavioural stuff yet so that will be available down the line - although I don't know how long this will be or what it will look like. E.g. it was discussed in the inaugural WG meet the other week about the fact that there are lots of valid ways to combine these schemas and how one would resolve a conflict such as this.

In the meantime, following what Apollo has done is not a bad shout as I understand that Mercurius Federation was originally based on Apollo Federation?

mcollina commented 2 years ago

Indeed

mcollina commented 2 years ago

note that Mercurius fully support @external: https://github.com/mercurius-js/mercurius/blob/ef154c50b3efc3f68a2c95269ebb277f379d1a12/test/gateway/requires-directive.js#L124.

JamesLMilner commented 2 years ago

Just to give an example of some ambiguity here, the example given in the opening post for the first service schemas uses an entity type like so:

   type Order @key(fields: "id") {
      id: ID!
      name: String
    }

For extending entities in Apollo v1, the canonical way is as so:

extend type Order @key(fields: "id") {
  id: String! @external
  surname: String
}

I believe the following attempt to extend (in the initial example) should probably throw an error because it's not the defined way (at least as defined by Apollo v1) to extend an entity (here the key directive is not used so it is not even an entity definition):

extend type Order {
    surname: String
}